Re: [PATCH 01/28] iio: gyro: fxa210002c: Balance runtime pm + use pm_runtime_resume_and_get().

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 13 May 2021 09:34:52 +0100
"Rui Miguel Silva" <rui.silva@linaro.org> wrote:

> Hi Mauro,
> On Wed May 12, 2021 at 2:30 PM WEST, Mauro Carvalho Chehab wrote:
> 
> > Em Wed, 12 May 2021 15:22:43 +0200
> > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> >  
> > > Em Sun,  9 May 2021 12:33:27 +0100
> > > Jonathan Cameron <jic23@kernel.org> escreveu:
> > >   
> > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > 
> > > > In both the probe() error path and remove() pm_runtime_put_noidle()
> > > > is called which will decrement the runtime pm reference count.
> > > > However, there is no matching function to have raised the reference count.
> > > > Not this isn't a fix as the runtime pm core will stop the reference count
> > > > going negative anyway.
> > > > 
> > > > An alternative would have been to raise the count in these paths, but
> > > > it is not clear why that would be necessary.
> > > > 
> > > > Whilst we are here replace some boilerplate with pm_runtime_resume_and_get()
> > > > Found using coccicheck script under review at:
> > > > https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> > > > 
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > Cc: Rui Miguel Silva <rui.silva@linaro.org>    
> > > 
> > > LGTM.
> > > 
> > > Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> >
> > Hmm...not quite...
> >  
> > > > diff --git a/drivers/iio/gyro/fxas21002c_core.c b/drivers/iio/gyro/fxas21002c_core.c
> > > > index 5af7b48ff01a..539585b0d300 100644
> > > > --- a/drivers/iio/gyro/fxas21002c_core.c
> > > > +++ b/drivers/iio/gyro/fxas21002c_core.c
> > > > @@ -366,14 +366,7 @@ static int fxas21002c_write(struct fxas21002c_data *data,
> > > >  
> > > >  static int  fxas21002c_pm_get(struct fxas21002c_data *data)
> > > >  {
> > > > -	struct device *dev = regmap_get_device(data->regmap);
> > > > -	int ret;
> > > > -
> > > > -	ret = pm_runtime_get_sync(dev);
> > > > -	if (ret < 0)
> > > > -		pm_runtime_put_noidle(dev);
> > > > -
> > > > -	return ret;
> > > > +	return pm_runtime_resume_and_get(regmap_get_device(data->regmap));
> > > >  }  
> >
> > fxas21002c_temp_get() and fxas21002c_axis_get() seem to be
> > missing a pm_runtime_put*() if something gets wrong at
> > regmap_field_read(), e. g.:  
> 
> Thanks for catching this, you are right. However I think it should
> have is own patch, since it is a real fix and this one is not.
> 
> Jonathan, bellow goes a proper patch, please feel free to squash this
> on top of yours, or just apply it separately, your call.
> 
> Cheers,
>      Rui
> 
> 8<-------------------------------------------------------------------
> 
> From 314a56247515c05c0c5504c1e305127878af0c9b Mon Sep 17 00:00:00 2001
> From: Rui Miguel Silva <rui.silva@linaro.org>
> Date: Wed, 12 May 2021 23:39:29 +0100
> Subject: [PATCH] iio: gyro: fxas21002c: balance runtime power in error path
> 
> If we fail to read temperature or axis we need to decrement the
> runtime pm reference count to trigger autosuspend.
> 
> Add the call to pm_put to do that in case of error.
> 
> Fixes: a0701b6263ae ("iio: gyro: add core driver for fxas21002c")
> Suggested-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>

I've queued this as a fix in my fixes-togreg branch of iio.git and marked
it for stable.

Thanks,

Jonathan

> ---
>  drivers/iio/gyro/fxas21002c_core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/gyro/fxas21002c_core.c b/drivers/iio/gyro/fxas21002c_core.c
> index 1a20c6b88e7d..645461c70454 100644
> --- a/drivers/iio/gyro/fxas21002c_core.c
> +++ b/drivers/iio/gyro/fxas21002c_core.c
> @@ -399,6 +399,7 @@ static int fxas21002c_temp_get(struct fxas21002c_data *data, int *val)
>  	ret = regmap_field_read(data->regmap_fields[F_TEMP], &temp);
>  	if (ret < 0) {
>  		dev_err(dev, "failed to read temp: %d\n", ret);
> +		fxas21002c_pm_put(data);
>  		goto data_unlock;
>  	}
>  
> @@ -432,6 +433,7 @@ static int fxas21002c_axis_get(struct fxas21002c_data *data,
>  			       &axis_be, sizeof(axis_be));
>  	if (ret < 0) {
>  		dev_err(dev, "failed to read axis: %d: %d\n", index, ret);
> +		fxas21002c_pm_put(data);
>  		goto data_unlock;
>  	}
>