Re: [PATCH] fusion-mptbase: handle failed allocation for workqueue
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 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
>> > ---
>> > 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 cod
Re: [PATCH] fusion-mptbase: handle failed allocation for workqueue
On Tue, 2016-02-16 at 13:33 -0500, Insu Yun wrote:
>
>
> On Tue, Feb 16, 2016 at 1:18 PM, Ewan Milne 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
> > ---
> > 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/
Re: [PATCH] fusion-mptbase: handle failed allocation for workqueue
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
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] 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
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
