Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1337026 > unrolled thread

[PATCH v2] fusion-mptbase: handle failed allocation for workqueue

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] fusion-mptbase: handle failed allocation for workqueue Insu Yun <wuninsu@gmail.com> - 2016-02-18 05:50 +0100
    Re: [PATCH v2] fusion-mptbase: handle failed allocation for workqueue Johannes Thumshirn <jthumshirn@suse.de> - 2016-02-18 10:10 +0100
      Re: [PATCH v2] fusion-mptbase: handle failed allocation for  workqueue Ewan Milne <emilne@redhat.com> - 2016-02-18 17:50 +0100
        Re: [PATCH v2] fusion-mptbase: handle failed allocation for  workqueue Ewan Milne <emilne@redhat.com> - 2016-02-18 18:00 +0100
          Re: [PATCH v2] fusion-mptbase: handle failed allocation for workqueue Tomas Henzl <thenzl@redhat.com> - 2016-02-19 14:50 +0100

#1337026 — [PATCH v2] fusion-mptbase: handle failed allocation for workqueue

the failure of ioc->reset_work_q is checked,
but not ioc->fw_event_q.

Signed-off-by: Insu Yun <wuninsu@gmail.com>
---
 drivers/message/fusion/mptbase.c | 44 ++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index 5dcc031..53a5015 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -1871,9 +1871,8 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (!ioc->reset_work_q) {
 		printk(MYIOC_s_ERR_FMT "Insufficient memory to add adapter!\n",
 		    ioc->name);
-		pci_release_selected_regions(pdev, ioc->bars);
-		kfree(ioc);
-		return -ENOMEM;
+		r = -ENOMEM;
+		goto err3;
 	}
 
 	dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "facts @ %p, pfacts[0] @ %p\n",
@@ -1996,24 +1995,16 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
 	snprintf(ioc->fw_event_q_name, MPT_KOBJ_NAME_LEN, "mpt/%d", ioc->id);
 	ioc->fw_event_q = create_singlethread_workqueue(ioc->fw_event_q_name);
 
+	if (!ioc->fw_event_q) {
+		r = -ENOMEM;
+		goto err2;
+	}
+
 	if ((r = mpt_do_ioc_recovery(ioc, MPT_HOSTEVENT_IOC_BRINGUP,
 	    CAN_SLEEP)) != 0){
 		printk(MYIOC_s_ERR_FMT "didn't initialize properly! (%d)\n",
 		    ioc->name, r);
-
-		list_del(&ioc->list);
-		if (ioc->alt_ioc)
-			ioc->alt_ioc->alt_ioc = NULL;
-		iounmap(ioc->memmap);
-		if (r != -5)
-			pci_release_selected_regions(pdev, ioc->bars);
-
-		destroy_workqueue(ioc->reset_work_q);
-		ioc->reset_work_q = NULL;
-
-		kfree(ioc);
-		pci_set_drvdata(pdev, NULL);
-		return r;
+		goto err1;
 	}
 
 	/* call per device driver probe entry point */
@@ -2040,6 +2031,25 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
 			msecs_to_jiffies(MPT_POLLING_INTERVAL));
 
 	return 0;
+
+err1:
+		destroy_workqueue(ioc->fw_event_q);
+		ioc->fw_event_q = NULL;
+err2::
+		destroy_workqueue(ioc->reset_work_q);
+		ioc->reset_work_q = NULL;
+
+		list_del(&ioc->list);
+		if (ioc->alt_ioc)
+			ioc->alt_ioc->alt_ioc = NULL;
+		iounmap(ioc->memmap);
+		pci_set_drvdata(pdev, NULL);
+err3:
+		if (r != -5)
+			pci_release_selected_regions(pdev, ioc->bars);
+		kfree(ioc);
+		return r;
+
 }
 
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
-- 
1.9.1

[toc] | [next] | [standalone]

#1337174

On Wed, Feb 17, 2016 at 11:40:59PM -0500, Insu Yun wrote:
> the failure of ioc->reset_work_q is checked,
> but not ioc->fw_event_q.
> 
> Signed-off-by: Insu Yun <wuninsu@gmail.com>
> ---
>  drivers/message/fusion/mptbase.c | 44 ++++++++++++++++++++++++----------------
>  1 file changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
> index 5dcc031..53a5015 100644
> --- a/drivers/message/fusion/mptbase.c
> +++ b/drivers/message/fusion/mptbase.c
> @@ -1871,9 +1871,8 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (!ioc->reset_work_q) {
>  		printk(MYIOC_s_ERR_FMT "Insufficient memory to add adapter!\n",
>  		    ioc->name);
> -		pci_release_selected_regions(pdev, ioc->bars);
> -		kfree(ioc);
> -		return -ENOMEM;
> +		r = -ENOMEM;
> +		goto err3;
>  	}
>  
>  	dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "facts @ %p, pfacts[0] @ %p\n",
> @@ -1996,24 +1995,16 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
>  	snprintf(ioc->fw_event_q_name, MPT_KOBJ_NAME_LEN, "mpt/%d", ioc->id);
>  	ioc->fw_event_q = create_singlethread_workqueue(ioc->fw_event_q_name);
>  
> +	if (!ioc->fw_event_q) {
> +		r = -ENOMEM;
> +		goto err2;
> +	}
> +
>  	if ((r = mpt_do_ioc_recovery(ioc, MPT_HOSTEVENT_IOC_BRINGUP,
>  	    CAN_SLEEP)) != 0){
>  		printk(MYIOC_s_ERR_FMT "didn't initialize properly! (%d)\n",
>  		    ioc->name, r);
> -
> -		list_del(&ioc->list);
> -		if (ioc->alt_ioc)
> -			ioc->alt_ioc->alt_ioc = NULL;
> -		iounmap(ioc->memmap);
> -		if (r != -5)
> -			pci_release_selected_regions(pdev, ioc->bars);
> -
> -		destroy_workqueue(ioc->reset_work_q);
> -		ioc->reset_work_q = NULL;
> -
> -		kfree(ioc);
> -		pci_set_drvdata(pdev, NULL);
> -		return r;
> +		goto err1;
>  	}
>  
>  	/* call per device driver probe entry point */
> @@ -2040,6 +2031,25 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
>  			msecs_to_jiffies(MPT_POLLING_INTERVAL));
>  
>  	return 0;
> +
> +err1:
> +		destroy_workqueue(ioc->fw_event_q);
> +		ioc->fw_event_q = NULL;
> +err2::
> +		destroy_workqueue(ioc->reset_work_q);
> +		ioc->reset_work_q = NULL;
> +
> +		list_del(&ioc->list);
> +		if (ioc->alt_ioc)
> +			ioc->alt_ioc->alt_ioc = NULL;
> +		iounmap(ioc->memmap);
> +		pci_set_drvdata(pdev, NULL);
> +err3:
> +		if (r != -5)
> +			pci_release_selected_regions(pdev, ioc->bars);
> +		kfree(ioc);
> +		return r;
> +
>  }

Please no. Not err1, err2 and err3.

err1 could be "goto destroy_fw_event_q", err2 "destroy_reset_workq", err3
goto "free_ioc".

>  
>  /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[toc] | [prev] | [next] | [standalone]

#1337522 — Re: [PATCH v2] fusion-mptbase: handle failed allocation for workqueue

[Multipart message — attachments visible in raw view] — view raw

On Thu, 2016-02-18 at 10:00 +0100, Johannes Thumshirn wrote:
> On Wed, Feb 17, 2016 at 11:40:59PM -0500, Insu Yun wrote:
> > the failure of ioc->reset_work_q is checked,
> > but not ioc->fw_event_q.
> > 
> > Signed-off-by: Insu Yun <wuninsu@gmail.com>
> > ---
> >  drivers/message/fusion/mptbase.c | 44 ++++++++++++++++++++++++----------------
> >  1 file changed, 27 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
> > index 5dcc031..53a5015 100644
> > --- a/drivers/message/fusion/mptbase.c
> > +++ b/drivers/message/fusion/mptbase.c
> > @@ -1871,9 +1871,8 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
> >  	if (!ioc->reset_work_q) {
> >  		printk(MYIOC_s_ERR_FMT "Insufficient memory to add adapter!\n",
> >  		    ioc->name);
> > -		pci_release_selected_regions(pdev, ioc->bars);
> > -		kfree(ioc);
> > -		return -ENOMEM;
> > +		r = -ENOMEM;
> > +		goto err3;
> >  	}
> >  
> >  	dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "facts @ %p, pfacts[0] @ %p\n",
> > @@ -1996,24 +1995,16 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
> >  	snprintf(ioc->fw_event_q_name, MPT_KOBJ_NAME_LEN, "mpt/%d", ioc->id);
> >  	ioc->fw_event_q = create_singlethread_workqueue(ioc->fw_event_q_name);
> >  
> > +	if (!ioc->fw_event_q) {
> > +		r = -ENOMEM;
> > +		goto err2;
> > +	}
> > +
> >  	if ((r = mpt_do_ioc_recovery(ioc, MPT_HOSTEVENT_IOC_BRINGUP,
> >  	    CAN_SLEEP)) != 0){
> >  		printk(MYIOC_s_ERR_FMT "didn't initialize properly! (%d)\n",
> >  		    ioc->name, r);
> > -
> > -		list_del(&ioc->list);
> > -		if (ioc->alt_ioc)
> > -			ioc->alt_ioc->alt_ioc = NULL;
> > -		iounmap(ioc->memmap);
> > -		if (r != -5)
> > -			pci_release_selected_regions(pdev, ioc->bars);
> > -
> > -		destroy_workqueue(ioc->reset_work_q);
> > -		ioc->reset_work_q = NULL;
> > -
> > -		kfree(ioc);
> > -		pci_set_drvdata(pdev, NULL);
> > -		return r;
> > +		goto err1;
> >  	}
> >  
> >  	/* call per device driver probe entry point */
> > @@ -2040,6 +2031,25 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
> >  			msecs_to_jiffies(MPT_POLLING_INTERVAL));
> >  
> >  	return 0;
> > +
> > +err1:
> > +		destroy_workqueue(ioc->fw_event_q);
> > +		ioc->fw_event_q = NULL;
> > +err2::
> > +		destroy_workqueue(ioc->reset_work_q);
> > +		ioc->reset_work_q = NULL;
> > +
> > +		list_del(&ioc->list);
> > +		if (ioc->alt_ioc)
> > +			ioc->alt_ioc->alt_ioc = NULL;
> > +		iounmap(ioc->memmap);
> > +		pci_set_drvdata(pdev, NULL);
> > +err3:
> > +		if (r != -5)
> > +			pci_release_selected_regions(pdev, ioc->bars);
> > +		kfree(ioc);
> > +		return r;
> > +
> >  }
> 
> Please no. Not err1, err2 and err3.
> 
> err1 could be "goto destroy_fw_event_q", err2 "destroy_reset_workq", err3
> goto "free_ioc".

It also appears to me upon further inspection that the existing code has
other problems.  In particular, once mpt_mapresources() has returned
with a nonzero error code, it looks like iounmap() should be called, but
it is not in the case of a failed allocation of reset_work_q.  I'm also
not sure why pci_release_selected_regions() is only called for the case
of mpt_do_ioc_recovery() returning != -5 when it is called whenever
there is a failed allocation of reset_work_q.

Consider the attached patch (untested, because I don't have hardware):
It shows what I would do for labels & error handling.  If the rc != -5
case of return from mpt_do_ioc_recovery() could be eliminated, then
another label "out_free_fw_event_q:" could be added prior to the other
error cases at the end, and all the code after the printk() in that path
could be replaced by "goto out_free_fw_event_q;"

	if ((r = mpt_do_ioc_recovery(ioc, MPT_HOSTEVENT_IOC_BRINGUP,
	    CAN_SLEEP)) != 0){
		printk(MYIOC_s_ERR_FMT "didn't initialize properly! (%d)\n",
		    ioc->name, r);
		goto out_free_fw_event_q;
	}
...

out_free_fw_event_q:
	destroy_workqueue(ioc->fw_event_q);
	ioc->fw_event_q = NULL;

out_remove_ioc:
...

However I do not know if that change is legitimate.

-Ewan

> >  
> >  /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
> > -- 
> > 1.9.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [prev] | [next] | [standalone]

#1337524 — Re: [PATCH v2] fusion-mptbase: handle failed allocation for workqueue

[Multipart message — attachments visible in raw view] — view raw

On Thu, 2016-02-18 at 11:40 -0500, Ewan Milne wrote:
> It also appears to me upon further inspection that the existing code has
> other problems.  In particular, once mpt_mapresources() has returned
> with a nonzero error code, it looks like iounmap() should be called, but
> it is not in the case of a failed allocation of reset_work_q.  I'm also
> not sure why pci_release_selected_regions() is only called for the case
> of mpt_do_ioc_recovery() returning != -5 when it is called whenever
> there is a failed allocation of reset_work_q.
> 
> Consider the attached patch (untested, because I don't have hardware):
> It shows what I would do for labels & error handling.  If the rc != -5
> case of return from mpt_do_ioc_recovery() could be eliminated, then
> another label "out_free_fw_event_q:" could be added prior to the other
> error cases at the end, and all the code after the printk() in that path
> could be replaced by "goto out_free_fw_event_q;"
> 
> 	if ((r = mpt_do_ioc_recovery(ioc, MPT_HOSTEVENT_IOC_BRINGUP,
> 	    CAN_SLEEP)) != 0){
> 		printk(MYIOC_s_ERR_FMT "didn't initialize properly! (%d)\n",
> 		    ioc->name, r);
> 		goto out_free_fw_event_q;
> 	}
> ...
> 
> out_free_fw_event_q:
> 	destroy_workqueue(ioc->fw_event_q);
> 	ioc->fw_event_q = NULL;
> 
> out_remove_ioc:
> ...
> 
> However I do not know if that change is legitimate.
> 
> -Ewan

There is an error in the patch attached in my previous mail.
Please refer to the attached PATCH v2 RFC.

-Ewan

[toc] | [prev] | [next] | [standalone]

#1338201

On 18.2.2016 17:55, Ewan Milne wrote:
> On Thu, 2016-02-18 at 11:40 -0500, Ewan Milne wrote:
>> It also appears to me upon further inspection that the existing code has
>> other problems.  In particular, once mpt_mapresources() has returned
>> with a nonzero error code, it looks like iounmap() should be called, but
>> it is not in the case of a failed allocation of reset_work_q.  I'm also
>> not sure why pci_release_selected_regions() is only called for the case
>> of mpt_do_ioc_recovery() returning != -5 when it is called whenever
>> there is a failed allocation of reset_work_q.
>>
>> Consider the attached patch (untested, because I don't have hardware):
>> It shows what I would do for labels & error handling.  If the rc != -5
>> case of return from mpt_do_ioc_recovery() could be eliminated, then
>> another label "out_free_fw_event_q:" could be added prior to the other
>> error cases at the end, and all the code after the printk() in that path
>> could be replaced by "goto out_free_fw_event_q;"
>>
>> 	if ((r = mpt_do_ioc_recovery(ioc, MPT_HOSTEVENT_IOC_BRINGUP,
>> 	    CAN_SLEEP)) != 0){
>> 		printk(MYIOC_s_ERR_FMT "didn't initialize properly! (%d)\n",
>> 		    ioc->name, r);
>> 		goto out_free_fw_event_q;
>> 	}
>> ...
>>
>> out_free_fw_event_q:
>> 	destroy_workqueue(ioc->fw_event_q);
>> 	ioc->fw_event_q = NULL;
>>
>> out_remove_ioc:
>> ...
>>
>> However I do not know if that change is legitimate.
>>
>> -Ewan
> There is an error in the patch attached in my previous mail.
> Please refer to the attached PATCH v2 RFC.

Looks fine to me, do you we could add this change to your patch ?


diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index a4f362b3b4..4a42c1534c 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -2012,6 +2012,8 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
                if (ioc->alt_ioc)
                        ioc->alt_ioc->alt_ioc = NULL;
                iounmap(ioc->memmap);
+               if (pci_is_enabled(pdev)
+                       pci_disable_device(pdev);
                if (r != -5)
                        pci_release_selected_regions(pdev, ioc->bars);

@@ -2019,7 +2021,6 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
                ioc->reset_work_q = NULL;

                kfree(ioc);
-               pci_set_drvdata(pdev, NULL);
                return r;
        }

@@ -2052,13 +2053,13 @@ out_remove_ioc:
        list_del(&ioc->list);
        if (ioc->alt_ioc)
                ioc->alt_ioc->alt_ioc = NULL;
-       pci_set_drvdata(ioc->pcidev, NULL);

        destroy_workqueue(ioc->reset_work_q);
        ioc->reset_work_q = NULL;

 out_unmap_resources:
        iounmap(ioc->memmap);
+       pci_disable_device(pdev);
        pci_release_selected_regions(pdev, ioc->bars);

 out_free_ioc:

-tomash

>
> -Ewan
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web