[06/25] media: i2c: imx334: fix the pm runtime get logic
diff mbox series

Message ID 9552f3daece8bec6869b518410b2998c3fc0a1fc.1620207353.git.mchehab+huawei@kernel.org
State New, archived
Headers show
Series
  • Fix some PM runtime issues at the media subsystem
Related show

Commit Message

Mauro Carvalho Chehab May 5, 2021, 9:41 a.m. UTC
The PM runtime get logic is currently broken, as it checks if
ret is zero instead of checking if it is an error code,
as reported by Dan Carpenter.

While here, use the pm_runtime_resume_and_get() as added by:
commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
added pm_runtime_resume_and_get() in order to automatically handle
dev->power.usage_count decrement on errors. As a bonus, such function
always return zero on success.

It should also be noticed that a fail of pm_runtime_get_sync() would
potentially result in a spurious runtime_suspend(), instead of
using pm_runtime_put_noidle().

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/i2c/imx334.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Jonathan Cameron May 5, 2021, 11:10 a.m. UTC | #1
On Wed, 5 May 2021 11:41:56 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> The PM runtime get logic is currently broken, as it checks if
> ret is zero instead of checking if it is an error code,
> as reported by Dan Carpenter.
> 
> While here, use the pm_runtime_resume_and_get() as added by:
> commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> added pm_runtime_resume_and_get() in order to automatically handle
> dev->power.usage_count decrement on errors. As a bonus, such function
> always return zero on success.
> 
> It should also be noticed that a fail of pm_runtime_get_sync() would
> potentially result in a spurious runtime_suspend(), instead of
> using pm_runtime_put_noidle().

Irony here is that pm_runtime_resume_and_get() returns <= 0 so with that
function change, you can stick with if (ret) and still be correct.

So only one of the two changes is needed to fix the bug.

J
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Reviewed-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/media/i2c/imx334.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index 047aa7658d21..23f28606e570 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -717,9 +717,9 @@ static int imx334_set_stream(struct v4l2_subdev *sd, int enable)
>  	}
>  
>  	if (enable) {
> -		ret = pm_runtime_get_sync(imx334->dev);
> -		if (ret)
> -			goto error_power_off;
> +		ret = pm_runtime_resume_and_get(imx334->dev);
> +		if (ret < 0)
> +			goto error_unlock;
>  
>  		ret = imx334_start_streaming(imx334);
>  		if (ret)
> @@ -737,6 +737,7 @@ static int imx334_set_stream(struct v4l2_subdev *sd, int enable)
>  
>  error_power_off:
>  	pm_runtime_put(imx334->dev);
> +error_unlock:
>  	mutex_unlock(&imx334->mutex);
>  
>  	return ret;
Mauro Carvalho Chehab May 5, 2021, 11:24 a.m. UTC | #2
Em Wed, 5 May 2021 12:10:40 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> escreveu:

> On Wed, 5 May 2021 11:41:56 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 
> > The PM runtime get logic is currently broken, as it checks if
> > ret is zero instead of checking if it is an error code,
> > as reported by Dan Carpenter.
> > 
> > While here, use the pm_runtime_resume_and_get() as added by:
> > commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> > added pm_runtime_resume_and_get() in order to automatically handle
> > dev->power.usage_count decrement on errors. As a bonus, such function
> > always return zero on success.
> > 
> > It should also be noticed that a fail of pm_runtime_get_sync() would
> > potentially result in a spurious runtime_suspend(), instead of
> > using pm_runtime_put_noidle().  
> 
> Irony here is that pm_runtime_resume_and_get() returns <= 0 so with that
> function change, you can stick with if (ret) and still be correct.
> 
> So only one of the two changes is needed to fix the bug.

Yeah, I noticed ;-)

On media, almost all devices have I2C bus(es), and I2C send/receive functions
return positive values. So, a good practice is to check for errors with:

	if (ret < 0)

That's why I opted to keep both changes here ;-)

Regards,
Mauro

> 
> J
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Reviewed-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> >  drivers/media/i2c/imx334.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > index 047aa7658d21..23f28606e570 100644
> > --- a/drivers/media/i2c/imx334.c
> > +++ b/drivers/media/i2c/imx334.c
> > @@ -717,9 +717,9 @@ static int imx334_set_stream(struct v4l2_subdev *sd, int enable)
> >  	}
> >  
> >  	if (enable) {
> > -		ret = pm_runtime_get_sync(imx334->dev);
> > -		if (ret)
> > -			goto error_power_off;
> > +		ret = pm_runtime_resume_and_get(imx334->dev);
> > +		if (ret < 0)
> > +			goto error_unlock;
> >  
> >  		ret = imx334_start_streaming(imx334);
> >  		if (ret)
> > @@ -737,6 +737,7 @@ static int imx334_set_stream(struct v4l2_subdev *sd, int enable)
> >  
> >  error_power_off:
> >  	pm_runtime_put(imx334->dev);
> > +error_unlock:
> >  	mutex_unlock(&imx334->mutex);
> >  
> >  	return ret;  
> 



Thanks,
Mauro
Jonathan Cameron May 5, 2021, 12:26 p.m. UTC | #3
On Wed, 5 May 2021 13:24:26 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Wed, 5 May 2021 12:10:40 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> escreveu:
> 
> > On Wed, 5 May 2021 11:41:56 +0200
> > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> >   
> > > The PM runtime get logic is currently broken, as it checks if
> > > ret is zero instead of checking if it is an error code,
> > > as reported by Dan Carpenter.
> > > 
> > > While here, use the pm_runtime_resume_and_get() as added by:
> > > commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> > > added pm_runtime_resume_and_get() in order to automatically handle
> > > dev->power.usage_count decrement on errors. As a bonus, such function
> > > always return zero on success.
> > > 
> > > It should also be noticed that a fail of pm_runtime_get_sync() would
> > > potentially result in a spurious runtime_suspend(), instead of
> > > using pm_runtime_put_noidle().    
> > 
> > Irony here is that pm_runtime_resume_and_get() returns <= 0 so with that
> > function change, you can stick with if (ret) and still be correct.
> > 
> > So only one of the two changes is needed to fix the bug.  
> 
> Yeah, I noticed ;-)
> 
> On media, almost all devices have I2C bus(es), and I2C send/receive functions
> return positive values. So, a good practice is to check for errors with:
> 
> 	if (ret < 0)
> 
> That's why I opted to keep both changes here ;-)

Fair enough.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Regards,
> Mauro
> 
> > 
> > J  
> > > 
> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > Reviewed-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > ---
> > >  drivers/media/i2c/imx334.c | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > > index 047aa7658d21..23f28606e570 100644
> > > --- a/drivers/media/i2c/imx334.c
> > > +++ b/drivers/media/i2c/imx334.c
> > > @@ -717,9 +717,9 @@ static int imx334_set_stream(struct v4l2_subdev *sd, int enable)
> > >  	}
> > >  
> > >  	if (enable) {
> > > -		ret = pm_runtime_get_sync(imx334->dev);
> > > -		if (ret)
> > > -			goto error_power_off;
> > > +		ret = pm_runtime_resume_and_get(imx334->dev);
> > > +		if (ret < 0)
> > > +			goto error_unlock;
> > >  
> > >  		ret = imx334_start_streaming(imx334);
> > >  		if (ret)
> > > @@ -737,6 +737,7 @@ static int imx334_set_stream(struct v4l2_subdev *sd, int enable)
> > >  
> > >  error_power_off:
> > >  	pm_runtime_put(imx334->dev);
> > > +error_unlock:
> > >  	mutex_unlock(&imx334->mutex);
> > >  
> > >  	return ret;    
> >   
> 
> 
> 
> Thanks,
> Mauro

Patch
diff mbox series

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index 047aa7658d21..23f28606e570 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -717,9 +717,9 @@  static int imx334_set_stream(struct v4l2_subdev *sd, int enable)
 	}
 
 	if (enable) {
-		ret = pm_runtime_get_sync(imx334->dev);
-		if (ret)
-			goto error_power_off;
+		ret = pm_runtime_resume_and_get(imx334->dev);
+		if (ret < 0)
+			goto error_unlock;
 
 		ret = imx334_start_streaming(imx334);
 		if (ret)
@@ -737,6 +737,7 @@  static int imx334_set_stream(struct v4l2_subdev *sd, int enable)
 
 error_power_off:
 	pm_runtime_put(imx334->dev);
+error_unlock:
 	mutex_unlock(&imx334->mutex);
 
 	return ret;