diff mbox series

media: dw100: Add a missing unwind goto in dw100_probe()

Message ID 20230103105534.3018257-1-xavier.roumegue@oss.nxp.com (mailing list archive)
State Accepted
Delegated to: Hans Verkuil
Headers
Series media: dw100: Add a missing unwind goto in dw100_probe() |

Commit Message

Xavier Roumegue (OSS) Jan. 3, 2023, 10:55 a.m. UTC 
  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(-)

Comments

Alexander Stein Jan. 3, 2023, 11:01 a.m. UTC | #1 
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
Xavier Roumegue (OSS) Jan. 3, 2023, 1:35 p.m. UTC | #2 
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
> 
> 
>
Alexander Stein Jan. 3, 2023, 1:48 p.m. UTC | #3 
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
Laurent Pinchart Jan. 3, 2023, 1:52 p.m. UTC | #4 
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 mbox series

Patch

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);