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
