diff mbox series

[v2] media: staging: tegra-vde: fix runtime pm imbalance on error

Message ID 20200521062746.6656-1-dinghao.liu@zju.edu.cn
State New
Headers show
Series [v2] media: staging: tegra-vde: fix runtime pm imbalance on error | expand

Commit Message

Dinghao Liu May 21, 2020, 6:27 a.m. UTC
pm_runtime_get_sync() increments the runtime PM usage counter even
the call returns an error code. Thus a pairing decrement is needed
on the error handling path to keep the counter balanced.

Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
---

Changelog:

v2: - Remove unused label 'unlock'
---
 drivers/staging/media/tegra-vde/vde.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Dan Carpenter May 21, 2020, 11:21 a.m. UTC | #1
On Thu, May 21, 2020 at 02:27:45PM +0800, Dinghao Liu wrote:
> pm_runtime_get_sync() increments the runtime PM usage counter even
> the call returns an error code. Thus a pairing decrement is needed
> on the error handling path to keep the counter balanced.
> 
> Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>

Let's stop working around the bug in pm_runtime_get_sync() and write
a replacement for it instead.

regards,
dan carpenter
Dinghao Liu May 21, 2020, 11:42 a.m. UTC | #2
We need to make sure if pm_runtime_get_sync() is designed with
such behavior before modifying it.  

I received a response from Rafael when I commited a similar patch:
https://lkml.org/lkml/2020/5/20/1100
It seems that this behavior is intentional and needs to be kept.

Regards,
Dinghao

&quot;Dan Carpenter&quot; &lt;dan.carpenter@oracle.com&gt;写道:
> On Thu, May 21, 2020 at 02:27:45PM +0800, Dinghao Liu wrote:
> > pm_runtime_get_sync() increments the runtime PM usage counter even
> > the call returns an error code. Thus a pairing decrement is needed
> > on the error handling path to keep the counter balanced.
> > 
> > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
> 
> Let's stop working around the bug in pm_runtime_get_sync() and write
> a replacement for it instead.
> 
> regards,
> dan carpenter
Dan Carpenter May 21, 2020, 12:03 p.m. UTC | #3
On Thu, May 21, 2020 at 07:42:56PM +0800, dinghao.liu@zju.edu.cn wrote:
> We need to make sure if pm_runtime_get_sync() is designed with
> such behavior before modifying it.  
> 
> I received a response from Rafael when I commited a similar patch:
> https://lkml.org/lkml/2020/5/20/1100
> It seems that this behavior is intentional and needs to be kept.

Yes.  This is why I have said twice or three times to not change
pm_runtime_get_sync() but instead to write a replacement.

A large percent of the callers are buggy.  The pm_runtime_get_sync() is
a -4 on Rusty's API scale.
http://sweng.the-davies.net/Home/rustys-api-design-manifesto
One could argue that anything above a -4 is really a 2 if you read
the implementation thouroughly enough...

regards,
dan carpenter
Dinghao Liu May 21, 2020, 12:21 p.m. UTC | #4
Sorry, I misunderstood your idea before. A new function is 
the best solution for this problem.

Regards,
Dinghao

&quot;Dan Carpenter&quot; &lt;dan.carpenter@oracle.com&gt;写道:
> On Thu, May 21, 2020 at 07:42:56PM +0800, dinghao.liu@zju.edu.cn wrote:
> > We need to make sure if pm_runtime_get_sync() is designed with
> > such behavior before modifying it.  
> > 
> > I received a response from Rafael when I commited a similar patch:
> > https://lkml.org/lkml/2020/5/20/1100
> > It seems that this behavior is intentional and needs to be kept.
> 
> Yes.  This is why I have said twice or three times to not change
> pm_runtime_get_sync() but instead to write a replacement.
> 
> A large percent of the callers are buggy.  The pm_runtime_get_sync() is
> a -4 on Rusty's API scale.
> http://sweng.the-davies.net/Home/rustys-api-design-manifesto
> One could argue that anything above a -4 is really a 2 if you read
> the implementation thouroughly enough...
> 
> regards,
> dan carpenter
>
diff mbox series

Patch

diff --git a/drivers/staging/media/tegra-vde/vde.c b/drivers/staging/media/tegra-vde/vde.c
index d3e63512a765..3fdf2cd0b99e 100644
--- a/drivers/staging/media/tegra-vde/vde.c
+++ b/drivers/staging/media/tegra-vde/vde.c
@@ -777,7 +777,7 @@  static int tegra_vde_ioctl_decode_h264(struct tegra_vde *vde,
 
 	ret = pm_runtime_get_sync(dev);
 	if (ret < 0)
-		goto unlock;
+		goto put_runtime_pm;
 
 	/*
 	 * We rely on the VDE registers reset value, otherwise VDE
@@ -843,8 +843,6 @@  static int tegra_vde_ioctl_decode_h264(struct tegra_vde *vde,
 put_runtime_pm:
 	pm_runtime_mark_last_busy(dev);
 	pm_runtime_put_autosuspend(dev);
-
-unlock:
 	mutex_unlock(&vde->lock);
 
 release_dpb_frames: