* [PATCH v2] fusion-mptbase: handle failed allocation for workqueue
@ 2016-02-18 4:40 Insu Yun
2016-02-18 9:00 ` Johannes Thumshirn
0 siblings, 1 reply; 5+ messages in thread
From: Insu Yun @ 2016-02-18 4:40 UTC (permalink / raw)
To: nagalakshmi.nandigama, praveen.krishnamoorthy, sreekanth.reddy,
abhijit.mahajan, MPT-FusionLinux.pdl, linux-scsi, linux-kernel
Cc: taesoo, yeongjin.jang, insu, changwoo, Insu Yun
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] fusion-mptbase: handle failed allocation for workqueue
2016-02-18 4:40 [PATCH v2] fusion-mptbase: handle failed allocation for workqueue Insu Yun
@ 2016-02-18 9:00 ` Johannes Thumshirn
2016-02-18 16:40 ` Ewan Milne
0 siblings, 1 reply; 5+ messages in thread
From: Johannes Thumshirn @ 2016-02-18 9:00 UTC (permalink / raw)
To: Insu Yun
Cc: nagalakshmi.nandigama, praveen.krishnamoorthy, sreekanth.reddy,
abhijit.mahajan, MPT-FusionLinux.pdl, linux-scsi, linux-kernel,
taesoo, yeongjin.jang, insu, changwoo
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] fusion-mptbase: handle failed allocation for workqueue
2016-02-18 9:00 ` Johannes Thumshirn
@ 2016-02-18 16:40 ` Ewan Milne
2016-02-18 16:55 ` Ewan Milne
0 siblings, 1 reply; 5+ messages in thread
From: Ewan Milne @ 2016-02-18 16:40 UTC (permalink / raw)
To: Johannes Thumshirn, thenzl
Cc: Insu Yun, nagalakshmi.nandigama, praveen.krishnamoorthy,
sreekanth.reddy, abhijit.mahajan, MPT-FusionLinux.pdl, linux-scsi,
linux-kernel, taesoo, yeongjin.jang, insu, changwoo
[-- Attachment #1: Type: text/plain, Size: 4385 bytes --]
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
>
[-- Attachment #2: 0001-mptbase-fixup-error-handling-paths-in-mpt_attach.patch --]
[-- Type: text/x-patch, Size: 3017 bytes --]
>From 83f117b81d58d01bfe7e01e04e4f2f2a602e2c9f Mon Sep 17 00:00:00 2001
From: "Ewan D. Milne" <emilne@redhat.com>
Date: Thu, 18 Feb 2016 11:24:02 -0500
Subject: [PATCH RFC] mptbase: fixup error handling paths in mpt_attach()
mpt_attach() was not checking for the failure to create fw_event_q.
Also, iounmap() was not being called in all error cases after ioremap()
had been called by mpt_mapresources().
---
drivers/message/fusion/mptbase.c | 37 +++++++++++++++++++++++++++++++------
1 file changed, 31 insertions(+), 6 deletions(-)
diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index 5dcc031..1fdc882 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -1801,8 +1801,7 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
ioc->pcidev = pdev;
if (mpt_mapresources(ioc)) {
- kfree(ioc);
- return r;
+ goto out_free_ioc;
}
/*
@@ -1871,9 +1870,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 out_unmap_resources;
}
dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "facts @ %p, pfacts[0] @ %p\n",
@@ -1995,12 +1993,21 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
spin_lock_init(&ioc->fw_event_lock);
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->reset_work_q) {
+ printk(MYIOC_s_ERR_FMT "Insufficient memory to add adapter!\n",
+ ioc->name);
+ r = -ENOMEM;
+ goto out_remove_ioc;
+ }
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);
+ destroy_workqueue(ioc->fw_event_q);
+ ioc->fw_event_q = NULL;
+
list_del(&ioc->list);
if (ioc->alt_ioc)
ioc->alt_ioc->alt_ioc = NULL;
@@ -2040,6 +2047,24 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
msecs_to_jiffies(MPT_POLLING_INTERVAL));
return 0;
+
+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_release_selected_regions(pdev, ioc->bars);
+
+out_free_ioc:
+ kfree(ioc);
+
+ return r;
}
/*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
@@ -6229,7 +6254,7 @@ mpt_get_manufacturing_pg_0(MPT_ADAPTER *ioc)
memcpy(ioc->board_assembly, pbuf->BoardAssembly, sizeof(ioc->board_assembly));
memcpy(ioc->board_tracer, pbuf->BoardTracerNumber, sizeof(ioc->board_tracer));
- out:
+out:
if (pbuf)
pci_free_consistent(ioc->pcidev, hdr.PageLength * 4, pbuf, buf_dma);
--
2.1.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] fusion-mptbase: handle failed allocation for workqueue
2016-02-18 16:40 ` Ewan Milne
@ 2016-02-18 16:55 ` Ewan Milne
2016-02-19 13:46 ` Tomas Henzl
0 siblings, 1 reply; 5+ messages in thread
From: Ewan Milne @ 2016-02-18 16:55 UTC (permalink / raw)
To: Johannes Thumshirn
Cc: thenzl, Insu Yun, nagalakshmi.nandigama, praveen.krishnamoorthy,
sreekanth.reddy, abhijit.mahajan, MPT-FusionLinux.pdl, linux-scsi,
linux-kernel, taesoo, yeongjin.jang, insu, changwoo
[-- Attachment #1: Type: text/plain, Size: 1480 bytes --]
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
[-- Attachment #2: 0001-mptbase-fixup-error-handling-paths-in-mpt_attach.patch --]
[-- Type: text/x-patch, Size: 3018 bytes --]
>From 79fd59c66c3afb339cf9fdd1fda65e78e32831a4 Mon Sep 17 00:00:00 2001
From: "Ewan D. Milne" <emilne@redhat.com>
Date: Thu, 18 Feb 2016 11:49:37 -0500
Subject: [PATCH v2 RFC] mptbase: fixup error handling paths in mpt_attach()
mpt_attach() was not checking for the failure to create fw_event_q.
Also, iounmap() was not being called in all error cases after ioremap()
had been called by mpt_mapresources().
---
drivers/message/fusion/mptbase.c | 37 +++++++++++++++++++++++++++++++------
1 file changed, 31 insertions(+), 6 deletions(-)
diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index 5dcc031..a4f362b 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -1801,8 +1801,7 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
ioc->pcidev = pdev;
if (mpt_mapresources(ioc)) {
- kfree(ioc);
- return r;
+ goto out_free_ioc;
}
/*
@@ -1871,9 +1870,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 out_unmap_resources;
}
dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "facts @ %p, pfacts[0] @ %p\n",
@@ -1995,12 +1993,21 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
spin_lock_init(&ioc->fw_event_lock);
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) {
+ printk(MYIOC_s_ERR_FMT "Insufficient memory to add adapter!\n",
+ ioc->name);
+ r = -ENOMEM;
+ goto out_remove_ioc;
+ }
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);
+ destroy_workqueue(ioc->fw_event_q);
+ ioc->fw_event_q = NULL;
+
list_del(&ioc->list);
if (ioc->alt_ioc)
ioc->alt_ioc->alt_ioc = NULL;
@@ -2040,6 +2047,24 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
msecs_to_jiffies(MPT_POLLING_INTERVAL));
return 0;
+
+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_release_selected_regions(pdev, ioc->bars);
+
+out_free_ioc:
+ kfree(ioc);
+
+ return r;
}
/*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
@@ -6229,7 +6254,7 @@ mpt_get_manufacturing_pg_0(MPT_ADAPTER *ioc)
memcpy(ioc->board_assembly, pbuf->BoardAssembly, sizeof(ioc->board_assembly));
memcpy(ioc->board_tracer, pbuf->BoardTracerNumber, sizeof(ioc->board_tracer));
- out:
+out:
if (pbuf)
pci_free_consistent(ioc->pcidev, hdr.PageLength * 4, pbuf, buf_dma);
--
2.1.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] fusion-mptbase: handle failed allocation for workqueue
2016-02-18 16:55 ` Ewan Milne
@ 2016-02-19 13:46 ` Tomas Henzl
0 siblings, 0 replies; 5+ messages in thread
From: Tomas Henzl @ 2016-02-19 13:46 UTC (permalink / raw)
To: emilne, Johannes Thumshirn
Cc: Insu Yun, nagalakshmi.nandigama, praveen.krishnamoorthy,
sreekanth.reddy, abhijit.mahajan, MPT-FusionLinux.pdl, linux-scsi,
linux-kernel, taesoo, yeongjin.jang, insu, changwoo
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
>
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-02-19 13:46 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-18 4:40 [PATCH v2] fusion-mptbase: handle failed allocation for workqueue Insu Yun
2016-02-18 9:00 ` Johannes Thumshirn
2016-02-18 16:40 ` Ewan Milne
2016-02-18 16:55 ` Ewan Milne
2016-02-19 13:46 ` Tomas Henzl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).