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@xxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxx>
> >> Reported-by: Dan Carpenter <error27@xxxxxxxxx>
> >> Signed-off-by: Xavier Roumegue <xavier.roumegue@xxxxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxxxxxxxxx>
--
Regards,
Laurent Pinchart
