| Message ID | 20230103105534.3018257-1-xavier.roumegue@oss.nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: dw100: Add a missing unwind goto in dw100_probe() | expand |
Hi, Am Dienstag, 3. Januar 2023, 11:55:34 CET schrieb Xavier Roumegue (OSS): > From: Xavier Roumegue <xavier.roumegue@oss.nxp.com> > > In case the IRQ allocation returns an error in dw100_probe(), the pm > runtime is not disabled before to return. > > Add the missing unwind goto on the error handling path of the IRQ > allocation request. > > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <error27@gmail.com> > Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com> > --- > drivers/media/platform/nxp/dw100/dw100.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/nxp/dw100/dw100.c > b/drivers/media/platform/nxp/dw100/dw100.c index f6d48c36f386..189d60cd5ed1 > 100644 > --- a/drivers/media/platform/nxp/dw100/dw100.c > +++ b/drivers/media/platform/nxp/dw100/dw100.c > @@ -1571,7 +1571,7 @@ static int dw100_probe(struct platform_device *pdev) > dev_name(&pdev->dev), dw_dev); > if (ret < 0) { > dev_err(&pdev->dev, "Failed to request irq: %d\n", ret); > - return ret; > + goto err_pm; > } > > ret = v4l2_device_register(&pdev->dev, &dw_dev->v4l2_dev); Doesn't it make more sense to request/allocate the IRQ (and other resources) before enabling runtime PM? Best regards, Alexander
Hi Alexander, On 1/3/23 12:01, Alexander Stein wrote: > Hi, > > Am Dienstag, 3. Januar 2023, 11:55:34 CET schrieb Xavier Roumegue (OSS): >> From: Xavier Roumegue <xavier.roumegue@oss.nxp.com> >> >> In case the IRQ allocation returns an error in dw100_probe(), the pm >> runtime is not disabled before to return. >> >> Add the missing unwind goto on the error handling path of the IRQ >> allocation request. >> >> Reported-by: kernel test robot <lkp@intel.com> >> Reported-by: Dan Carpenter <error27@gmail.com> >> Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com> >> --- >> drivers/media/platform/nxp/dw100/dw100.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/media/platform/nxp/dw100/dw100.c >> b/drivers/media/platform/nxp/dw100/dw100.c index f6d48c36f386..189d60cd5ed1 >> 100644 >> --- a/drivers/media/platform/nxp/dw100/dw100.c >> +++ b/drivers/media/platform/nxp/dw100/dw100.c >> @@ -1571,7 +1571,7 @@ static int dw100_probe(struct platform_device *pdev) >> dev_name(&pdev->dev), dw_dev); >> if (ret < 0) { >> dev_err(&pdev->dev, "Failed to request irq: %d\n", ret); >> - return ret; >> + goto err_pm; >> } >> >> ret = v4l2_device_register(&pdev->dev, &dw_dev->v4l2_dev); > > Doesn't it make more sense to request/allocate the IRQ (and other resources) > before enabling runtime PM? I would say this does as much sense as the other way around, as soon as something wrong happens, you have to restore things as it was prior to enter your routine. The most optimal function call ordering should depend on the failing occurrence likelihood of each individual function. On the probe path, I assume none of the functions are expected to fail. But I understand one could argue differently. So for the time being, this oneliner patch addresses the issue reported by the robot. Regards, Xavier > > Best regards, > Alexander > > >
Hi Xavier, Am Dienstag, 3. Januar 2023, 14:35:35 CET schrieb Xavier Roumegue (OSS): > Hi Alexander, > > On 1/3/23 12:01, Alexander Stein wrote: > > Hi, > > > > Am Dienstag, 3. Januar 2023, 11:55:34 CET schrieb Xavier Roumegue (OSS): > >> From: Xavier Roumegue <xavier.roumegue@oss.nxp.com> > >> > >> In case the IRQ allocation returns an error in dw100_probe(), the pm > >> runtime is not disabled before to return. > >> > >> Add the missing unwind goto on the error handling path of the IRQ > >> allocation request. > >> > >> Reported-by: kernel test robot <lkp@intel.com> > >> Reported-by: Dan Carpenter <error27@gmail.com> > >> Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com> > >> --- > >> > >> drivers/media/platform/nxp/dw100/dw100.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/media/platform/nxp/dw100/dw100.c > >> b/drivers/media/platform/nxp/dw100/dw100.c index > >> f6d48c36f386..189d60cd5ed1 > >> 100644 > >> --- a/drivers/media/platform/nxp/dw100/dw100.c > >> +++ b/drivers/media/platform/nxp/dw100/dw100.c > >> @@ -1571,7 +1571,7 @@ static int dw100_probe(struct platform_device > >> *pdev) > >> > >> dev_name(&pdev->dev), dw_dev); > >> > >> if (ret < 0) { > >> > >> dev_err(&pdev->dev, "Failed to request irq: %d\n", ret); > >> > >> - return ret; > >> + goto err_pm; > >> > >> } > >> > >> ret = v4l2_device_register(&pdev->dev, &dw_dev->v4l2_dev); > > > > Doesn't it make more sense to request/allocate the IRQ (and other > > resources) before enabling runtime PM? > > I would say this does as much sense as the other way around, as soon as > something wrong happens, you have to restore things as it was prior to enter > your routine. The most optimal function call ordering should depend on the > failing occurrence likelihood of each individual function. > On the probe path, I assume none of the functions are expected to fail. > But I understand one could argue differently. -EPROBE_DEFER teached me otherwise ;-) What I actually wanted to highlight is that calling the devm_* functions first, reduces the cleanup path for the following setup calls. > So for the time being, this oneliner patch addresses the issue reported by > the robot. Sure, on the other hand it's less complex if you can just return in an error path. Best regards, Alexander
Hello Xavier, On Tue, Jan 03, 2023 at 02:35:35PM +0100, Xavier Roumegue (OSS) wrote: > On 1/3/23 12:01, Alexander Stein wrote: > > Am Dienstag, 3. Januar 2023, 11:55:34 CET schrieb Xavier Roumegue (OSS): > >> From: Xavier Roumegue <xavier.roumegue@oss.nxp.com> > >> > >> In case the IRQ allocation returns an error in dw100_probe(), the pm > >> runtime is not disabled before to return. > >> > >> Add the missing unwind goto on the error handling path of the IRQ > >> allocation request. > >> > >> Reported-by: kernel test robot <lkp@intel.com> > >> Reported-by: Dan Carpenter <error27@gmail.com> > >> Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com> > >> --- > >> drivers/media/platform/nxp/dw100/dw100.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/media/platform/nxp/dw100/dw100.c > >> b/drivers/media/platform/nxp/dw100/dw100.c index f6d48c36f386..189d60cd5ed1 > >> 100644 > >> --- a/drivers/media/platform/nxp/dw100/dw100.c > >> +++ b/drivers/media/platform/nxp/dw100/dw100.c > >> @@ -1571,7 +1571,7 @@ static int dw100_probe(struct platform_device *pdev) > >> dev_name(&pdev->dev), dw_dev); > >> if (ret < 0) { > >> dev_err(&pdev->dev, "Failed to request irq: %d\n", ret); > >> - return ret; > >> + goto err_pm; > >> } > >> > >> ret = v4l2_device_register(&pdev->dev, &dw_dev->v4l2_dev); > > > > Doesn't it make more sense to request/allocate the IRQ (and other resources) > > before enabling runtime PM? > > I would say this does as much sense as the other way around, as soon as > something wrong happens, you have to restore things as it was prior to enter > your routine. The most optimal function call ordering should depend on the > failing occurrence likelihood of each individual function. > On the probe path, I assume none of the functions are expected to fail. > But I understand one could argue differently. > > So for the time being, this oneliner patch addresses the issue reported by the > robot. I think that Alexander's point was that, as you request the IRQ with devm_request_irq(), you could just return in case of error if this was done before any other operation that requires a cleanup. In this case, however, enabling runtime PM is done so that the device gets reset, which I think is important to do before requesting the IRQ, otherwise spurious IRQs could happen if the device was left in a weird state. A comment above runtime PM enable would be useful to record the reason why the current order is required. You could add that in a v2 of this patch, or in a separate patch. In either case, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
diff --git a/drivers/media/platform/nxp/dw100/dw100.c b/drivers/media/platform/nxp/dw100/dw100.c index f6d48c36f386..189d60cd5ed1 100644 --- a/drivers/media/platform/nxp/dw100/dw100.c +++ b/drivers/media/platform/nxp/dw100/dw100.c @@ -1571,7 +1571,7 @@ static int dw100_probe(struct platform_device *pdev) dev_name(&pdev->dev), dw_dev); if (ret < 0) { dev_err(&pdev->dev, "Failed to request irq: %d\n", ret); - return ret; + goto err_pm; } ret = v4l2_device_register(&pdev->dev, &dw_dev->v4l2_dev);