* [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).