linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] media: vsp1: Fix a reference count leak.
@ 2020-06-13 23:23 wu000273
  2020-06-16  2:07 ` Laurent Pinchart
  0 siblings, 1 reply; 4+ messages in thread
From: wu000273 @ 2020-06-13 23:23 UTC (permalink / raw)
  To: kjlu
  Cc: wu000273, Laurent Pinchart, Kieran Bingham,
	Mauro Carvalho Chehab, linux-media, linux-renesas-soc,
	linux-kernel

From: Qiushi Wu <wu000273@umn.edu>

pm_runtime_get_sync() increments the runtime PM usage counter even
when it returns an error code, causing incorrect ref count if
pm_runtime_put_noidle() is not called in error handling paths.
Thus call pm_runtime_put_noidle() if pm_runtime_get_sync() fails.

Fixes: 1e6af546ee66 ("[media] v4l: vsp1: Implement runtime PM support")
Signed-off-by: Qiushi Wu <wu000273@umn.edu>
---
 drivers/media/platform/vsp1/vsp1_drv.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c
index c650e45bb0ad..222c9e1261a0 100644
--- a/drivers/media/platform/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/vsp1/vsp1_drv.c
@@ -846,8 +846,10 @@ static int vsp1_probe(struct platform_device *pdev)
 	pm_runtime_enable(&pdev->dev);
 
 	ret = pm_runtime_get_sync(&pdev->dev);
-	if (ret < 0)
+	if (ret < 0) {
+		pm_runtime_put_noidle(&pdev->dev);
 		goto done;
+	}
 
 	vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
 	pm_runtime_put_sync(&pdev->dev);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] media: vsp1: Fix a reference count leak.
  2020-06-13 23:23 [PATCH] media: vsp1: Fix a reference count leak wu000273
@ 2020-06-16  2:07 ` Laurent Pinchart
  2020-06-16  2:09   ` Laurent Pinchart
  0 siblings, 1 reply; 4+ messages in thread
From: Laurent Pinchart @ 2020-06-16  2:07 UTC (permalink / raw)
  To: wu000273
  Cc: kjlu, Kieran Bingham, Mauro Carvalho Chehab, Geert Uytterhoeven,
	Rafael J. Wysocki, linux-media, linux-renesas-soc, linux-kernel

Hi Qiushi,

(CC'ing Rafael and Geert)

Thank you for the patch.

On Sat, Jun 13, 2020 at 06:23:57PM -0500, wu000273@umn.edu wrote:
> From: Qiushi Wu <wu000273@umn.edu>
> 
> pm_runtime_get_sync() increments the runtime PM usage counter even
> when it returns an error code, causing incorrect ref count if
> pm_runtime_put_noidle() is not called in error handling paths.
> Thus call pm_runtime_put_noidle() if pm_runtime_get_sync() fails.
> 
> Fixes: 1e6af546ee66 ("[media] v4l: vsp1: Implement runtime PM support")
> Signed-off-by: Qiushi Wu <wu000273@umn.edu>

https://lore.kernel.org/dri-devel/20200614134655.GA5960@pendragon.ideasonboard.com/

I really wonder if mass-patching all drivers is the best way forward.

> ---
>  drivers/media/platform/vsp1/vsp1_drv.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c
> index c650e45bb0ad..222c9e1261a0 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -846,8 +846,10 @@ static int vsp1_probe(struct platform_device *pdev)
>  	pm_runtime_enable(&pdev->dev);
>  
>  	ret = pm_runtime_get_sync(&pdev->dev);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(&pdev->dev);
>  		goto done;
> +	}
>  
>  	vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
>  	pm_runtime_put_sync(&pdev->dev);

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] media: vsp1: Fix a reference count leak.
  2020-06-16  2:07 ` Laurent Pinchart
@ 2020-06-16  2:09   ` Laurent Pinchart
  2020-06-22 10:21     ` Hans Verkuil
  0 siblings, 1 reply; 4+ messages in thread
From: Laurent Pinchart @ 2020-06-16  2:09 UTC (permalink / raw)
  To: wu000273
  Cc: kjlu, Kieran Bingham, Mauro Carvalho Chehab, Geert Uytterhoeven,
	Rafael J. Wysocki, linux-media, linux-renesas-soc, linux-kernel

On Tue, Jun 16, 2020 at 05:07:34AM +0300, Laurent Pinchart wrote:
> Hi Qiushi,
> 
> (CC'ing Rafael and Geert)
> 
> Thank you for the patch.
> 
> On Sat, Jun 13, 2020 at 06:23:57PM -0500, wu000273@umn.edu wrote:
> > From: Qiushi Wu <wu000273@umn.edu>
> > 
> > pm_runtime_get_sync() increments the runtime PM usage counter even
> > when it returns an error code, causing incorrect ref count if
> > pm_runtime_put_noidle() is not called in error handling paths.
> > Thus call pm_runtime_put_noidle() if pm_runtime_get_sync() fails.
> > 
> > Fixes: 1e6af546ee66 ("[media] v4l: vsp1: Implement runtime PM support")
> > Signed-off-by: Qiushi Wu <wu000273@umn.edu>
> 
> https://lore.kernel.org/dri-devel/20200614134655.GA5960@pendragon.ideasonboard.com/
> 
> I really wonder if mass-patching all drivers is the best way forward.

Also,

https://lore.kernel.org/linux-media/20200608052919.4984-1-dinghao.liu@zju.edu.cn/

> > ---
> >  drivers/media/platform/vsp1/vsp1_drv.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c
> > index c650e45bb0ad..222c9e1261a0 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drv.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> > @@ -846,8 +846,10 @@ static int vsp1_probe(struct platform_device *pdev)
> >  	pm_runtime_enable(&pdev->dev);
> >  
> >  	ret = pm_runtime_get_sync(&pdev->dev);
> > -	if (ret < 0)
> > +	if (ret < 0) {
> > +		pm_runtime_put_noidle(&pdev->dev);
> >  		goto done;
> > +	}
> >  
> >  	vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
> >  	pm_runtime_put_sync(&pdev->dev);

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] media: vsp1: Fix a reference count leak.
  2020-06-16  2:09   ` Laurent Pinchart
@ 2020-06-22 10:21     ` Hans Verkuil
  0 siblings, 0 replies; 4+ messages in thread
From: Hans Verkuil @ 2020-06-22 10:21 UTC (permalink / raw)
  To: Laurent Pinchart, wu000273
  Cc: kjlu, Kieran Bingham, Mauro Carvalho Chehab, Geert Uytterhoeven,
	Rafael J. Wysocki, linux-media, linux-renesas-soc, linux-kernel,
	Dinghao Liu, Aditya Pakki

On 16/06/2020 04:09, Laurent Pinchart wrote:
> On Tue, Jun 16, 2020 at 05:07:34AM +0300, Laurent Pinchart wrote:
>> Hi Qiushi,
>>
>> (CC'ing Rafael and Geert)
>>
>> Thank you for the patch.
>>
>> On Sat, Jun 13, 2020 at 06:23:57PM -0500, wu000273@umn.edu wrote:
>>> From: Qiushi Wu <wu000273@umn.edu>
>>>
>>> pm_runtime_get_sync() increments the runtime PM usage counter even
>>> when it returns an error code, causing incorrect ref count if
>>> pm_runtime_put_noidle() is not called in error handling paths.
>>> Thus call pm_runtime_put_noidle() if pm_runtime_get_sync() fails.
>>>
>>> Fixes: 1e6af546ee66 ("[media] v4l: vsp1: Implement runtime PM support")
>>> Signed-off-by: Qiushi Wu <wu000273@umn.edu>
>>
>> https://lore.kernel.org/dri-devel/20200614134655.GA5960@pendragon.ideasonboard.com/
>>
>> I really wonder if mass-patching all drivers is the best way forward.
> 
> Also,
> 
> https://lore.kernel.org/linux-media/20200608052919.4984-1-dinghao.liu@zju.edu.cn/

I also stop applying these patches. In part because of what Laurent says (I'd
like to have some consensus on this as well), and in part because there are
at least three different devs working on this (Qiushi Wu <wu000273@umn.edu>,
Aditya Pakki <pakki001@umn.edu> and Dinghao Liu <dinghao.liu@zju.edu.cn>) and
I am getting duplicate patches.

So I stop applying these pm_runtime_get_sync() patches until it is clear that
this is the way forward. Other ref count issues I will still apply, but it
would be great if Qiushi Wu, Aditya Pakki and Dinghao Liu can work together
to avoid duplicate patches.

Regards,

	Hans

> 
>>> ---
>>>  drivers/media/platform/vsp1/vsp1_drv.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c
>>> index c650e45bb0ad..222c9e1261a0 100644
>>> --- a/drivers/media/platform/vsp1/vsp1_drv.c
>>> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
>>> @@ -846,8 +846,10 @@ static int vsp1_probe(struct platform_device *pdev)
>>>  	pm_runtime_enable(&pdev->dev);
>>>  
>>>  	ret = pm_runtime_get_sync(&pdev->dev);
>>> -	if (ret < 0)
>>> +	if (ret < 0) {
>>> +		pm_runtime_put_noidle(&pdev->dev);
>>>  		goto done;
>>> +	}
>>>  
>>>  	vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
>>>  	pm_runtime_put_sync(&pdev->dev);
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-06-22 10:22 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-13 23:23 [PATCH] media: vsp1: Fix a reference count leak wu000273
2020-06-16  2:07 ` Laurent Pinchart
2020-06-16  2:09   ` Laurent Pinchart
2020-06-22 10:21     ` Hans Verkuil

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).