| Message ID | 20210509113354.660190-2-jic23@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IIO: Runtime PM related cleanups. | expand |
Hi Jonathan, thanks for the patch. On Sun May 9, 2021 at 12:33 PM WEST, Jonathan Cameron wrote: > 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> With s/fxa210002c:/fxa21002c:/ in the patch subject: Reviewed-by: Rui Miguel Silva <rui.silva@linaro.org> Cheers, Rui > --- > drivers/iio/gyro/fxas21002c_core.c | 11 +---------- > 1 file changed, 1 insertion(+), 10 deletions(-) > > 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)); > } > > static int fxas21002c_pm_put(struct fxas21002c_data *data) > @@ -1002,7 +995,6 @@ int fxas21002c_core_probe(struct device *dev, struct regmap *regmap, int irq, > pm_disable: > pm_runtime_disable(dev); > pm_runtime_set_suspended(dev); > - pm_runtime_put_noidle(dev); > > return ret; > } > @@ -1016,7 +1008,6 @@ void fxas21002c_core_remove(struct device *dev) > > pm_runtime_disable(dev); > pm_runtime_set_suspended(dev); > - pm_runtime_put_noidle(dev); > } > EXPORT_SYMBOL_GPL(fxas21002c_core_remove); > > -- > 2.31.1
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> > --- > drivers/iio/gyro/fxas21002c_core.c | 11 +---------- > 1 file changed, 1 insertion(+), 10 deletions(-) > > 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)); > } > > static int fxas21002c_pm_put(struct fxas21002c_data *data) > @@ -1002,7 +995,6 @@ int fxas21002c_core_probe(struct device *dev, struct regmap *regmap, int irq, > pm_disable: > pm_runtime_disable(dev); > pm_runtime_set_suspended(dev); > - pm_runtime_put_noidle(dev); > > return ret; > } > @@ -1016,7 +1008,6 @@ void fxas21002c_core_remove(struct device *dev) > > pm_runtime_disable(dev); > pm_runtime_set_suspended(dev); > - pm_runtime_put_noidle(dev); > } > EXPORT_SYMBOL_GPL(fxas21002c_core_remove); > Thanks, Mauro
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.: static int fxas21002c_axis_get(struct fxas21002c_data *data, int index, int *val) { struct device *dev = regmap_get_device(data->regmap); __be16 axis_be; int ret; mutex_lock(&data->lock); ret = fxas21002c_pm_get(data); if (ret < 0) goto data_unlock; ret = regmap_bulk_read(data->regmap, FXAS21002C_AXIS_TO_REG(index), &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; } *val = sign_extend32(be16_to_cpu(axis_be), 15); ret = fxas21002c_pm_put(data); if (ret < 0) goto data_unlock; ret = IIO_VAL_INT; data_unlock: mutex_unlock(&data->lock); return ret; } Thanks, Mauro
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> --- 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; }
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; > } >
On Thu, 13 May 2021 17:31:31 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > 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... Because the fix and the cleanup are in substantially different places in the driver I'm going to assume git will sort it out and apply the original patch to my togreg branch - initially pushed out as testing for bots to poke at. Thanks, Jonathan > > > > > > > > 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; > > } > > >
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)); } static int fxas21002c_pm_put(struct fxas21002c_data *data) @@ -1002,7 +995,6 @@ int fxas21002c_core_probe(struct device *dev, struct regmap *regmap, int irq, pm_disable: pm_runtime_disable(dev); pm_runtime_set_suspended(dev); - pm_runtime_put_noidle(dev); return ret; } @@ -1016,7 +1008,6 @@ void fxas21002c_core_remove(struct device *dev) pm_runtime_disable(dev); pm_runtime_set_suspended(dev); - pm_runtime_put_noidle(dev); } EXPORT_SYMBOL_GPL(fxas21002c_core_remove);