lkml.org 
[lkml]   [2016]   [Feb]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] fusion-mptbase: handle failed allocation for workqueue
From
Date
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


\
 
 \ /
  Last update: 2016-02-16 22:01    [W:0.183 / U:0.000 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site