Re: [PATCH] media: dw100: Add a missing unwind goto in dw100_probe()

[Alexander Stein <alexander.stein@ew.tq-group.com>]

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