Re: [PATCH 06/25] media: i2c: imx334: fix the pm runtime get logic

[Jonathan Cameron]

On Wed, 5 May 2021 13:24:26 +0200
Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> wrote:

> Em Wed, 5 May 2021 12:10:40 +0100
> Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> escreveu:
> 
> > On Wed, 5 May 2021 11:41:56 +0200
> > Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> 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@xxxxxxxxxx>
> 
> Regards,
> Mauro
> 
> > 
> > J  
> > > 
> > > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > > Reviewed-by: Daniele Alessandrelli <daniele.alessandrelli@xxxxxxxxx>
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> > > ---
> > >  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