Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1334966 > unrolled thread
| Started by | Insu Yun <wuninsu@gmail.com> |
|---|---|
| First post | 2016-02-16 04:00 +0100 |
| Last post | 2016-02-17 18:00 +0100 |
| Articles | 4 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH] fusion-mptbase: handle failed allocation for workqueue Insu Yun <wuninsu@gmail.com> - 2016-02-16 04:00 +0100
Re: [PATCH] fusion-mptbase: handle failed allocation for workqueue Ewan Milne <emilne@redhat.com> - 2016-02-16 19:20 +0100
Re: [PATCH] fusion-mptbase: handle failed allocation for workqueue Ewan Milne <emilne@redhat.com> - 2016-02-16 22:00 +0100
Re: [PATCH] fusion-mptbase: handle failed allocation for workqueue Tomas Henzl <thenzl@redhat.com> - 2016-02-17 18:00 +0100
| From | Insu Yun <wuninsu@gmail.com> |
|---|---|
| Date | 2016-02-16 04:00 +0100 |
| Subject | [PATCH] fusion-mptbase: handle failed allocation for workqueue |
| Message-ID | <r2E1Q-1z0-11@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Ewan Milne <emilne@redhat.com> |
|---|---|
| Date | 2016-02-16 19:20 +0100 |
| Message-ID | <r2So9-31B-3@gated-at.bofh.it> |
| In reply to | #1334966 |
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>
[toc] | [prev] | [next] | [standalone]
| From | Ewan Milne <emilne@redhat.com> |
|---|---|
| Date | 2016-02-16 22:00 +0100 |
| Message-ID | <r2UT0-4yA-15@gated-at.bofh.it> |
| In reply to | #1335694 |
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
[toc] | [prev] | [next] | [standalone]
| From | Tomas Henzl <thenzl@redhat.com> |
|---|---|
| Date | 2016-02-17 18:00 +0100 |
| Message-ID | <r3dCj-Fn-13@gated-at.bofh.it> |
| In reply to | #1335834 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web