* [PATCH] fusion-mptbase: handle failed allocation for workqueue
@ 2016-02-16 2:50 Insu Yun
2016-02-16 18:18 ` Ewan Milne
0 siblings, 1 reply; 4+ messages in thread
From: Insu Yun @ 2016-02-16 2:50 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 | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index 5dcc031..d4907a1 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -1996,6 +1996,13 @@ 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) {
+ destroy_workqueue(ioc->reset_work_q);
+ pci_release_selected_regions(pdev, ioc->bars);
+ kfree(ioc);
+ return -ENOMEM;
+ }
+
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",
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] fusion-mptbase: handle failed allocation for workqueue
2016-02-16 2:50 [PATCH] fusion-mptbase: handle failed allocation for workqueue Insu Yun
@ 2016-02-16 18:18 ` Ewan Milne
[not found] ` <CAGoFzNfFtxHRYz_UF1qS-wWhwNapffBx9xrMA2Lu51FGeTmo7w@mail.gmail.com>
0 siblings, 1 reply; 4+ messages in thread
From: Ewan Milne @ 2016-02-16 18:18 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 Mon, 2016-02-15 at 21:50 -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 | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
> index 5dcc031..d4907a1 100644
> --- a/drivers/message/fusion/mptbase.c
> +++ b/drivers/message/fusion/mptbase.c
> @@ -1996,6 +1996,13 @@ 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) {
> + destroy_workqueue(ioc->reset_work_q);
> + pci_release_selected_regions(pdev, ioc->bars);
> + kfree(ioc);
> + return -ENOMEM;
> + }
> +
> 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",
This does not look correct to me. The error path for the call to
mpt_do_ioc_recovery() after create_singlethread_workqueue() for
ioc->fw_event_q does other cleanup, including:
list_del(&ioc->list);
if (ioc->alt_ioc)
ioc->alt_ioc->alt_ioc = NULL;
iounmap(ioc->memmap);
and
kfree(ioc);
pci_set_drvdata(pdev, NULL);
Here I think you are kfree()ing ioc while it is still on the &ioc_list,
which will cause a crash.
Note to Avago: this code could use a symbolic return code identifier:
if (r != -5)
pci_release_selected_regions(pdev, ioc->bars);
In general, with sequential allocation of resources like this, error
handling can be performed using a series of goto's to labels at the
end of the function that release the resources in reverse order. This
avoids the duplication of code within the function, and reduces the
chance for errors when the function is later modified. See init_sd
in drivers/scsi/sd.c for an example.
Reviewed-by: Ewan D. Milne <emilne@redhat.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] fusion-mptbase: handle failed allocation for workqueue
[not found] ` <CAGoFzNfFtxHRYz_UF1qS-wWhwNapffBx9xrMA2Lu51FGeTmo7w@mail.gmail.com>
@ 2016-02-16 20:52 ` Ewan Milne
2016-02-17 16:53 ` Tomas Henzl
0 siblings, 1 reply; 4+ messages in thread
From: Ewan Milne @ 2016-02-16 20:52 UTC (permalink / raw)
To: Insu Yun
Cc: nagalakshmi.nandigama, praveen.krishnamoorthy, sreekanth.reddy,
abhijit.mahajan, MPT-FusionLinux.pdl, linux-scsi, LKML,
Taesoo Kim, Yeongjin Jang, Yun, Insu, Changwoo Min
On Tue, 2016-02-16 at 13:33 -0500, Insu Yun wrote:
>
>
> On Tue, Feb 16, 2016 at 1:18 PM, Ewan Milne <emilne@redhat.com> wrote:
> On Mon, 2016-02-15 at 21:50 -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 | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/message/fusion/mptbase.c
> b/drivers/message/fusion/mptbase.c
> > index 5dcc031..d4907a1 100644
> > --- a/drivers/message/fusion/mptbase.c
> > +++ b/drivers/message/fusion/mptbase.c
> > @@ -1996,6 +1996,13 @@ 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) {
> > + destroy_workqueue(ioc->reset_work_q);
> > + pci_release_selected_regions(pdev, ioc->bars);
> > + kfree(ioc);
> > + return -ENOMEM;
> > + }
> > +
> > 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",
>
> This does not look correct to me. The error path for the call
> to
> mpt_do_ioc_recovery() after create_singlethread_workqueue()
> for
> ioc->fw_event_q does other cleanup, including:
>
> list_del(&ioc->list);
> if (ioc->alt_ioc)
> ioc->alt_ioc->alt_ioc = NULL;
> iounmap(ioc->memmap);
>
> and
>
> kfree(ioc);
> pci_set_drvdata(pdev, NULL);
>
>
> Oh yes. Right.
> I just copied from above error handling code.
>
>
>
>
> Here I think you are kfree()ing ioc while it is still on the
> &ioc_list,
> which will cause a crash.
>
> Note to Avago: this code could use a symbolic return code
> identifier:
>
> if (r != -5)
> pci_release_selected_regions(pdev,
> ioc->bars);
>
>
> What is -5? it seems strange for me.
>
> Is it error code? then it is better to use macro.
The comments above mpt_do_ioc_recovery() say:
* Returns:
* 0 for success
* -1 if failed to get board READY
* -2 if READY but IOCFacts Failed
* -3 if READY but PrimeIOCFifos Failed
* -4 if READY but IOCInit Failed
* -5 if failed to enable_device and/or request_selected_regions
* -6 if failed to upload firmware
and yes, it would be better to use a macro (symbolic value) hence my
comment to Avago.
>
>
>
> In general, with sequential allocation of resources like this,
> error
> handling can be performed using a series of goto's to labels
> at the
> end of the function that release the resources in reverse
> order. This
> avoids the duplication of code within the function, and
> reduces the
> chance for errors when the function is later modified. See
> init_sd
> in drivers/scsi/sd.c for an example.
>
> Reviewed-by: Ewan D. Milne <emilne@redhat.com>
>
>
>
>
> Then in summary, after failing allocation, I need to call
>
>
> list_del(&ioc->list);
> if (ioc->alt_ioc)
> ioc->alt_ioc->alt_ioc = NULL;
> iounmap(ioc->memmap);
> kfree(ioc);
> pci_set_drvdata(pdev, NULL);
>
>
> right?
I think you also need this:
if (r != -5)
pci_release_selected_regions(pdev, ioc->bars);
destroy_workqueue(ioc->reset_work_q);
ioc->reset_work_q = NULL;
prior to your kfree(ioc) call.
Alternatively it looks like the code to create the ioc->fw_event_q could
be moved up to be right after the code to create the ioc->reset_work_q,
that might simplify the code a little bit as the ioc has not been added
to the &ioc_list and pci_set_drvdata() has not yet been called.
Note however that a failed call to mpt_do_ioc_recovery() still needs to
perform all the recovery actions, including destroying both work queues,
so consider putting the error handling code at the end of the function
as I mentioned above. Otherwise, you should add:
destroy_workqueue(ioc->fw_event_q);
ioc->fw_event_q = NULL;
prior to the call to destroy_workqueue(ioc->reset_work_q) in that path.
-Ewan
>
>
> --
> Regards
> Insu Yun
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] fusion-mptbase: handle failed allocation for workqueue
2016-02-16 20:52 ` Ewan Milne
@ 2016-02-17 16:53 ` Tomas Henzl
0 siblings, 0 replies; 4+ messages in thread
From: Tomas Henzl @ 2016-02-17 16:53 UTC (permalink / raw)
To: emilne, Insu Yun
Cc: nagalakshmi.nandigama, praveen.krishnamoorthy, sreekanth.reddy,
abhijit.mahajan, MPT-FusionLinux.pdl, linux-scsi, LKML,
Taesoo Kim, Yeongjin Jang, Yun, Insu, Changwoo Min
On 16.2.2016 21:52, Ewan Milne wrote:
> On Tue, 2016-02-16 at 13:33 -0500, Insu Yun wrote:
>>
>> On Tue, Feb 16, 2016 at 1:18 PM, Ewan Milne <emilne@redhat.com> wrote:
>> On Mon, 2016-02-15 at 21:50 -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 | 7 +++++++
>> > 1 file changed, 7 insertions(+)
>> >
>> > diff --git a/drivers/message/fusion/mptbase.c
>> b/drivers/message/fusion/mptbase.c
>> > index 5dcc031..d4907a1 100644
>> > --- a/drivers/message/fusion/mptbase.c
>> > +++ b/drivers/message/fusion/mptbase.c
>> > @@ -1996,6 +1996,13 @@ 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) {
>> > + destroy_workqueue(ioc->reset_work_q);
>> > + pci_release_selected_regions(pdev, ioc->bars);
>> > + kfree(ioc);
>> > + return -ENOMEM;
>> > + }
>> > +
>> > 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",
>>
>> This does not look correct to me. The error path for the call
>> to
>> mpt_do_ioc_recovery() after create_singlethread_workqueue()
>> for
>> ioc->fw_event_q does other cleanup, including:
>>
>> list_del(&ioc->list);
>> if (ioc->alt_ioc)
>> ioc->alt_ioc->alt_ioc = NULL;
>> iounmap(ioc->memmap);
>>
>> and
>>
>> kfree(ioc);
>> pci_set_drvdata(pdev, NULL);
>>
>>
>> Oh yes. Right.
>> I just copied from above error handling code.
>>
>>
>>
>>
>> Here I think you are kfree()ing ioc while it is still on the
>> &ioc_list,
>> which will cause a crash.
>>
>> Note to Avago: this code could use a symbolic return code
>> identifier:
>>
>> if (r != -5)
>> pci_release_selected_regions(pdev,
>> ioc->bars);
>>
>>
>> What is -5? it seems strange for me.
>>
>> Is it error code? then it is better to use macro.
> The comments above mpt_do_ioc_recovery() say:
>
> * Returns:
> * 0 for success
> * -1 if failed to get board READY
> * -2 if READY but IOCFacts Failed
> * -3 if READY but PrimeIOCFifos Failed
> * -4 if READY but IOCInit Failed
> * -5 if failed to enable_device and/or request_selected_regions
> * -6 if failed to upload firmware
>
> and yes, it would be better to use a macro (symbolic value) hence my
> comment to Avago.
>>
>>
>> In general, with sequential allocation of resources like this,
>> error
>> handling can be performed using a series of goto's to labels
>> at the
>> end of the function that release the resources in reverse
>> order. This
>> avoids the duplication of code within the function, and
>> reduces the
>> chance for errors when the function is later modified. See
>> init_sd
>> in drivers/scsi/sd.c for an example.
>>
>> Reviewed-by: Ewan D. Milne <emilne@redhat.com>
>>
>>
>>
>>
>> Then in summary, after failing allocation, I need to call
>>
>>
>> list_del(&ioc->list);
>> if (ioc->alt_ioc)
>> ioc->alt_ioc->alt_ioc = NULL;
>> iounmap(ioc->memmap);
>> kfree(ioc);
>> pci_set_drvdata(pdev, NULL);
After 0998d06310 "device-core: Ensure drvdata = NULL when no driver is bound"
the pci_set_drvdata(pdev, NULL); is no more needed.
tomash
>>
>>
>> right?
> I think you also need this:
>
> if (r != -5)
> pci_release_selected_regions(pdev, ioc->bars);
>
> destroy_workqueue(ioc->reset_work_q);
> ioc->reset_work_q = NULL;
>
> prior to your kfree(ioc) call.
>
> Alternatively it looks like the code to create the ioc->fw_event_q could
> be moved up to be right after the code to create the ioc->reset_work_q,
> that might simplify the code a little bit as the ioc has not been added
> to the &ioc_list and pci_set_drvdata() has not yet been called.
>
> Note however that a failed call to mpt_do_ioc_recovery() still needs to
> perform all the recovery actions, including destroying both work queues,
> so consider putting the error handling code at the end of the function
> as I mentioned above. Otherwise, you should add:
>
> destroy_workqueue(ioc->fw_event_q);
> ioc->fw_event_q = NULL;
>
> prior to the call to destroy_workqueue(ioc->reset_work_q) in that path.
>
> -Ewan
>
>>
>> --
>> Regards
>> Insu Yun
>
> --
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-02-17 16:53 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-16 2:50 [PATCH] fusion-mptbase: handle failed allocation for workqueue Insu Yun
2016-02-16 18:18 ` Ewan Milne
[not found] ` <CAGoFzNfFtxHRYz_UF1qS-wWhwNapffBx9xrMA2Lu51FGeTmo7w@mail.gmail.com>
2016-02-16 20:52 ` Ewan Milne
2016-02-17 16:53 ` 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).