[PATCH v2 0/6] IIO: Runtime PM related cleanups.

[PATCH v2 0/6] IIO: Runtime PM related cleanups.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Changes since v1:
* Drop all patches since applied.
* Clean up some more unbalanced cases that Mauro pointed out.
* One additional fix for lack of put in an error path.
* Dropped zpa2326 patch for now.  The use of runtime pm in that driver
  is sufficiently odd that it needs a closer look before any changes.

Note the unbalanced put is not a bug as such, because the runtime pm core
protects against the reference count going negative.  However, it is
a bad pattern to have copied into new drivers (as it confuses me)
so I'd like to clear it out.

Inspired by Mauro's work on similar issues in media and Julia's
coccicheck script.

There will be at least one more set of these once this first set
have flushed out any mistakes I may have made.

I checked my assumptions around the excess pm_runtime_put_noidle
calls in QEMU but may well be missing something even so.

Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
Cc: Matt Ranostay <matt.ranostay@konsulko.com>

Jonathan Cameron (6):
  iio: imu: mpu6050: Balance runtime pm + use
    pm_runtime_resume_and_get()
  iio: adc: ads1015: Balance runtime pm +  pm_runtime_resume_and_get()
  iio: chemical: atlas-sensor: Balance runtime pm +
    pm_runtime_resume_and_get()
  iio: prox: pulsed-light-v2: Fix misbalance runtime pm in error path
  iio: prox: pulsed-light-v2: Use pm_runtime_resume_and_get()
  iio: pressure: icp10100: Balance runtime pm + use
    pm_runtime_resume_and_get()

 drivers/iio/adc/ti-ads1015.c                  |  5 +----
 drivers/iio/chemical/atlas-sensor.c           | 13 ++++---------
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 19 ++++++-------------
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  6 ++----
 drivers/iio/pressure/icp10100.c               |  5 +++--
 .../iio/proximity/pulsedlight-lidar-lite-v2.c |  7 +++++--
 6 files changed, 21 insertions(+), 34 deletions(-)

-- 
2.31.1

[PATCH v2 1/6] iio: imu: mpu6050: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Remove an unblanced pm_runtime_put_sync_suspend() call
in inv_pu_pm_disable().  Not this call is not a bug, because the runtime
pm core will not allow the reference counter to go negative.  It is
however confusing and serves no purpose.

pm_runtime_resume_and_get() case found using coccicheck script under
review at:
https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/

pm_runtime_resume_and_get() returns <= 0 only so simplify related checks
to bring this more inline with nearby calls.

This is a prequel to taking a closer look at the runtime pm in IIO drivers
in general.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 19 ++++++-------------
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  6 ++----
 2 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 6244a07048df..4cae3765e327 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -570,11 +570,9 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
 	freq_hz = INV_MPU6050_DIVIDER_TO_FIFO_RATE(st->chip_config.divider);
 	period_us = 1000000 / freq_hz;
 
-	result = pm_runtime_get_sync(pdev);
-	if (result < 0) {
-		pm_runtime_put_noidle(pdev);
+	result = pm_runtime_resume_and_get(pdev);
+	if (result)
 		return result;
-	}
 
 	switch (chan->type) {
 	case IIO_ANGL_VEL:
@@ -812,11 +810,9 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
 		return result;
 
 	mutex_lock(&st->lock);
-	result = pm_runtime_get_sync(pdev);
-	if (result < 0) {
-		pm_runtime_put_noidle(pdev);
+	result = pm_runtime_resume_and_get(pdev);
+	if (result)
 		goto error_write_raw_unlock;
-	}
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
@@ -930,11 +926,9 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
 		result = 0;
 		goto fifo_rate_fail_unlock;
 	}
-	result = pm_runtime_get_sync(pdev);
-	if (result < 0) {
-		pm_runtime_put_noidle(pdev);
+	result = pm_runtime_resume_and_get(pdev);
+	if (result)
 		goto fifo_rate_fail_unlock;
-	}
 
 	result = regmap_write(st->map, st->reg->sample_rate_div, d);
 	if (result)
@@ -1422,7 +1416,6 @@ static void inv_mpu_pm_disable(void *data)
 {
 	struct device *dev = data;
 
-	pm_runtime_put_sync_suspend(dev);
 	pm_runtime_disable(dev);
 }
 
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
index e21ba778595a..2d0e8cdd4848 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
@@ -173,11 +173,9 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
 
 	if (enable) {
 		scan = inv_scan_query(indio_dev);
-		result = pm_runtime_get_sync(pdev);
-		if (result < 0) {
-			pm_runtime_put_noidle(pdev);
+		result = pm_runtime_resume_and_get(pdev);
+		if (result)
 			return result;
-		}
 		/*
 		 * In case autosuspend didn't trigger, turn off first not
 		 * required sensors.
-- 
2.31.1

[PATCH v2 2/6] iio: adc: ads1015: Balance runtime pm + pm_runtime_resume_and_get()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

The call to pm_runtime_put_noidle() in remove() is not balancing a
counter increment.  Note this doesn't matter as the runtime pm core
will not allow the counter to go negative.  However, it is confusing
to the reader so let's remove it.

The pm_runtime_resume_and_get() replacement was found using coccicheck
script under review at:
https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/

This is a prequel to taking a closer look at the runtime pm in IIO drivers
in general.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ti-ads1015.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index 5b828428be77..b0352e91ac16 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -323,9 +323,7 @@ static int ads1015_set_power_state(struct ads1015_data *data, bool on)
 	struct device *dev = regmap_get_device(data->regmap);
 
 	if (on) {
-		ret = pm_runtime_get_sync(dev);
-		if (ret < 0)
-			pm_runtime_put_noidle(dev);
+		ret = pm_runtime_resume_and_get(dev);
 	} else {
 		pm_runtime_mark_last_busy(dev);
 		ret = pm_runtime_put_autosuspend(dev);
@@ -1070,7 +1068,6 @@ static int ads1015_remove(struct i2c_client *client)
 
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
-	pm_runtime_put_noidle(&client->dev);
 
 	/* power down single shot mode */
 	return ads1015_set_conv_mode(data, ADS1015_SINGLESHOT);
-- 
2.31.1

[PATCH v2 3/6] iio: chemical: atlas-sensor: Balance runtime pm + pm_runtime_resume_and_get()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

The pm_runtime_put_noidle() call in remove isn't balanced with any get, so
drop it.  Note this isn't a bug as the runtime pm core will not allow the
reference count to go negative, making this a noop. However, it is
confusing to the reader so let's drop it.

pm_runtime_resume_and_get() replacement found using the coccicheck script
under review at:
https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/

As pm_runtime_resume_and_get() returns <= 0 take advantage of that to
change the error checking to if (ret) which is more in keeping with the
rest of this driver.

This is a prequel to taking a closer look at the runtime pm in IIO drivers
in general.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Matt Ranostay <matt.ranostay@konsulko.com>
---
 drivers/iio/chemical/atlas-sensor.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c
index 0fdb3b29c5eb..9cb99585b6ff 100644
--- a/drivers/iio/chemical/atlas-sensor.c
+++ b/drivers/iio/chemical/atlas-sensor.c
@@ -410,11 +410,9 @@ static int atlas_buffer_postenable(struct iio_dev *indio_dev)
 	struct atlas_data *data = iio_priv(indio_dev);
 	int ret;
 
-	ret = pm_runtime_get_sync(&data->client->dev);
-	if (ret < 0) {
-		pm_runtime_put_noidle(&data->client->dev);
+	ret = pm_runtime_resume_and_get(&data->client->dev);
+	if (ret)
 		return ret;
-	}
 
 	return atlas_set_interrupt(data, true);
 }
@@ -487,11 +485,9 @@ static int atlas_read_measurement(struct atlas_data *data, int reg, __be32 *val)
 	int suspended = pm_runtime_suspended(dev);
 	int ret;
 
-	ret = pm_runtime_get_sync(dev);
-	if (ret < 0) {
-		pm_runtime_put_noidle(dev);
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
 		return ret;
-	}
 
 	if (suspended)
 		msleep(data->chip->delay);
@@ -741,7 +737,6 @@ static int atlas_remove(struct i2c_client *client)
 
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
-	pm_runtime_put_noidle(&client->dev);
 
 	return atlas_set_powermode(data, 0);
 }
-- 
2.31.1

[PATCH v2 4/6] iio: prox: pulsed-light-v2: Fix misbalance runtime pm in error path

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

There is one path in which we don't do a runtime pm put and so
leave the device enabled for ever more.

Reported-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Fixes: 4ac4e086fd8c ("iio: pulsedlight-lidar-lite: add runtime PM")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Matt Ranostay <matt.ranostay@konsulko.com>
---
 drivers/iio/proximity/pulsedlight-lidar-lite-v2.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
index 822a68ae5e03..ecaeb1e11007 100644
--- a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
+++ b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
@@ -164,7 +164,7 @@ static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
 	ret = lidar_write_control(data, LIDAR_REG_CONTROL_ACQUIRE);
 	if (ret < 0) {
 		dev_err(&client->dev, "cannot send start measurement command");
-		return ret;
+		goto err;
 	}
 
 	while (tries--) {
@@ -188,6 +188,7 @@ static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
 		}
 		ret = -EIO;
 	}
+err:
 	pm_runtime_mark_last_busy(&client->dev);
 	pm_runtime_put_autosuspend(&client->dev);
 
-- 
2.31.1

[PATCH v2 5/6] iio: prox: pulsed-light-v2: Use pm_runtime_resume_and_get()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Using this new call makes it easy to handle any errors as a result
of runtime resume as it exits without leaving the reference count
elevated.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Matt Ranostay <matt.ranostay@konsulko.com>
---
 drivers/iio/proximity/pulsedlight-lidar-lite-v2.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
index ecaeb1e11007..e94f63932edb 100644
--- a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
+++ b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
@@ -158,7 +158,9 @@ static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
 	int tries = 10;
 	int ret;
 
-	pm_runtime_get_sync(&client->dev);
+	ret = pm_runtime_resume_and_get(&client->dev);
+	if (ret < 0)
+		return ret;
 
 	/* start sample */
 	ret = lidar_write_control(data, LIDAR_REG_CONTROL_ACQUIRE);
-- 
2.31.1

[PATCH v2 6/6] iio: pressure: icp10100: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

The devm_ handled runtime pm disable calls pm_runtime_put_sync_suspend()
which isn't balancing a matching get call.  It isn't a bug as such,
because the runtime pm core doesn't decrement the reference count below
zero, but it is missleading so let's drop it.

Using pm_runtime_resume_and_get() new call makes it easy to handle
failures in resume as it doesn't hold a reference count if it exits
with an error.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/pressure/icp10100.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/pressure/icp10100.c b/drivers/iio/pressure/icp10100.c
index 48759fc4bf18..af4621eaa6b5 100644
--- a/drivers/iio/pressure/icp10100.c
+++ b/drivers/iio/pressure/icp10100.c
@@ -250,7 +250,9 @@ static int icp10100_get_measures(struct icp10100_state *st,
 	__be16 measures[3];
 	int ret;
 
-	pm_runtime_get_sync(&st->client->dev);
+	ret = pm_runtime_resume_and_get(&st->client->dev);
+	if (ret < 0)
+		return ret;
 
 	mutex_lock(&st->lock);
 	cmd = &icp10100_cmd_measure[st->mode];
@@ -525,7 +527,6 @@ static void icp10100_pm_disable(void *data)
 {
 	struct device *dev = data;
 
-	pm_runtime_put_sync_suspend(dev);
 	pm_runtime_disable(dev);
 }
 
-- 
2.31.1

Re: [PATCH v2 4/6] iio: prox: pulsed-light-v2: Fix misbalance runtime pm in error path

[Matt Ranostay]

On Sun, May 16, 2021 at 9:22 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> There is one path in which we don't do a runtime pm put and so
> leave the device enabled for ever more.

Acked-by: Matt Ranostay <matt.ranostay@konsulko.com>

>
> Reported-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Fixes: 4ac4e086fd8c ("iio: pulsedlight-lidar-lite: add runtime PM")
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> ---
>  drivers/iio/proximity/pulsedlight-lidar-lite-v2.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> index 822a68ae5e03..ecaeb1e11007 100644
> --- a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> +++ b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> @@ -164,7 +164,7 @@ static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
>         ret = lidar_write_control(data, LIDAR_REG_CONTROL_ACQUIRE);
>         if (ret < 0) {
>                 dev_err(&client->dev, "cannot send start measurement command");
> -               return ret;
> +               goto err;
>         }
>
>         while (tries--) {
> @@ -188,6 +188,7 @@ static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
>                 }
>                 ret = -EIO;
>         }
> +err:
>         pm_runtime_mark_last_busy(&client->dev);
>         pm_runtime_put_autosuspend(&client->dev);
>
> --
> 2.31.1
>

Re: [PATCH v2 3/6] iio: chemical: atlas-sensor: Balance runtime pm + pm_runtime_resume_and_get()

[Matt Ranostay]

On Sun, May 16, 2021 at 9:22 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> The pm_runtime_put_noidle() call in remove isn't balanced with any get, so
> drop it.  Note this isn't a bug as the runtime pm core will not allow the
> reference count to go negative, making this a noop. However, it is
> confusing to the reader so let's drop it.
>
> pm_runtime_resume_and_get() replacement found using the coccicheck script
> under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
>
> As pm_runtime_resume_and_get() returns <= 0 take advantage of that to
> change the error checking to if (ret) which is more in keeping with the
> rest of this driver.
>
> This is a prequel to taking a closer look at the runtime pm in IIO drivers
> in general.
>

Acked-by: Matt Ranostay <matt.ranostay@konsulko.com>

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> ---
>  drivers/iio/chemical/atlas-sensor.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c
> index 0fdb3b29c5eb..9cb99585b6ff 100644
> --- a/drivers/iio/chemical/atlas-sensor.c
> +++ b/drivers/iio/chemical/atlas-sensor.c
> @@ -410,11 +410,9 @@ static int atlas_buffer_postenable(struct iio_dev *indio_dev)
>         struct atlas_data *data = iio_priv(indio_dev);
>         int ret;
>
> -       ret = pm_runtime_get_sync(&data->client->dev);
> -       if (ret < 0) {
> -               pm_runtime_put_noidle(&data->client->dev);
> +       ret = pm_runtime_resume_and_get(&data->client->dev);
> +       if (ret)
>                 return ret;
> -       }
>
>         return atlas_set_interrupt(data, true);
>  }
> @@ -487,11 +485,9 @@ static int atlas_read_measurement(struct atlas_data *data, int reg, __be32 *val)
>         int suspended = pm_runtime_suspended(dev);
>         int ret;
>
> -       ret = pm_runtime_get_sync(dev);
> -       if (ret < 0) {
> -               pm_runtime_put_noidle(dev);
> +       ret = pm_runtime_resume_and_get(dev);
> +       if (ret)
>                 return ret;
> -       }
>
>         if (suspended)
>                 msleep(data->chip->delay);
> @@ -741,7 +737,6 @@ static int atlas_remove(struct i2c_client *client)
>
>         pm_runtime_disable(&client->dev);
>         pm_runtime_set_suspended(&client->dev);
> -       pm_runtime_put_noidle(&client->dev);
>
>         return atlas_set_powermode(data, 0);
>  }
> --
> 2.31.1
>

Re: [PATCH v2 5/6] iio: prox: pulsed-light-v2: Use pm_runtime_resume_and_get()

[Matt Ranostay]

On Sun, May 16, 2021 at 9:22 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Using this new call makes it easy to handle any errors as a result
> of runtime resume as it exits without leaving the reference count
> elevated.
>

Acked-by: Matt Ranostay <matt.ranostay@konsulko.com>

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> ---
>  drivers/iio/proximity/pulsedlight-lidar-lite-v2.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> index ecaeb1e11007..e94f63932edb 100644
> --- a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> +++ b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> @@ -158,7 +158,9 @@ static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
>         int tries = 10;
>         int ret;
>
> -       pm_runtime_get_sync(&client->dev);
> +       ret = pm_runtime_resume_and_get(&client->dev);
> +       if (ret < 0)
> +               return ret;
>
>         /* start sample */
>         ret = lidar_write_control(data, LIDAR_REG_CONTROL_ACQUIRE);
> --
> 2.31.1
>

Re: [PATCH v2 1/6] iio: imu: mpu6050: Balance runtime pm + use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Sun, 16 May 2021 17:20:58 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Remove an unblanced pm_runtime_put_sync_suspend() call
> in inv_pu_pm_disable().  Not this call is not a bug, because the runtime
> pm core will not allow the reference counter to go negative.  It is
> however confusing and serves no purpose.
> 
> pm_runtime_resume_and_get() case found using coccicheck script under
> review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> 
> pm_runtime_resume_and_get() returns <= 0 only so simplify related checks
> to bring this more inline with nearby calls.
> 
> This is a prequel to taking a closer look at the runtime pm in IIO drivers
> in general.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>


> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 19 ++++++-------------
>  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  6 ++----
>  2 files changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 6244a07048df..4cae3765e327 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -570,11 +570,9 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
>  	freq_hz = INV_MPU6050_DIVIDER_TO_FIFO_RATE(st->chip_config.divider);
>  	period_us = 1000000 / freq_hz;
>  
> -	result = pm_runtime_get_sync(pdev);
> -	if (result < 0) {
> -		pm_runtime_put_noidle(pdev);
> +	result = pm_runtime_resume_and_get(pdev);
> +	if (result)
>  		return result;
> -	}
>  
>  	switch (chan->type) {
>  	case IIO_ANGL_VEL:
> @@ -812,11 +810,9 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
>  		return result;
>  
>  	mutex_lock(&st->lock);
> -	result = pm_runtime_get_sync(pdev);
> -	if (result < 0) {
> -		pm_runtime_put_noidle(pdev);
> +	result = pm_runtime_resume_and_get(pdev);
> +	if (result)
>  		goto error_write_raw_unlock;
> -	}
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
> @@ -930,11 +926,9 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
>  		result = 0;
>  		goto fifo_rate_fail_unlock;
>  	}
> -	result = pm_runtime_get_sync(pdev);
> -	if (result < 0) {
> -		pm_runtime_put_noidle(pdev);
> +	result = pm_runtime_resume_and_get(pdev);
> +	if (result)
>  		goto fifo_rate_fail_unlock;
> -	}
>  
>  	result = regmap_write(st->map, st->reg->sample_rate_div, d);
>  	if (result)
> @@ -1422,7 +1416,6 @@ static void inv_mpu_pm_disable(void *data)
>  {
>  	struct device *dev = data;
>  
> -	pm_runtime_put_sync_suspend(dev);
>  	pm_runtime_disable(dev);
>  }
>  
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> index e21ba778595a..2d0e8cdd4848 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> @@ -173,11 +173,9 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
>  
>  	if (enable) {
>  		scan = inv_scan_query(indio_dev);
> -		result = pm_runtime_get_sync(pdev);
> -		if (result < 0) {
> -			pm_runtime_put_noidle(pdev);
> +		result = pm_runtime_resume_and_get(pdev);
> +		if (result)
>  			return result;
> -		}
>  		/*
>  		 * In case autosuspend didn't trigger, turn off first not
>  		 * required sensors.



Thanks,
Mauro

Re: [PATCH v2 2/6] iio: adc: ads1015: Balance runtime pm + pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Sun, 16 May 2021 17:20:59 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> The call to pm_runtime_put_noidle() in remove() is not balancing a
> counter increment.  Note this doesn't matter as the runtime pm core
> will not allow the counter to go negative.  However, it is confusing
> to the reader so let's remove it.
> 
> The pm_runtime_resume_and_get() replacement was found using coccicheck
> script under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> 
> This is a prequel to taking a closer look at the runtime pm in IIO drivers
> in general.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  drivers/iio/adc/ti-ads1015.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index 5b828428be77..b0352e91ac16 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -323,9 +323,7 @@ static int ads1015_set_power_state(struct ads1015_data *data, bool on)
>  	struct device *dev = regmap_get_device(data->regmap);
>  
>  	if (on) {
> -		ret = pm_runtime_get_sync(dev);
> -		if (ret < 0)
> -			pm_runtime_put_noidle(dev);
> +		ret = pm_runtime_resume_and_get(dev);
>  	} else {
>  		pm_runtime_mark_last_busy(dev);
>  		ret = pm_runtime_put_autosuspend(dev);
> @@ -1070,7 +1068,6 @@ static int ads1015_remove(struct i2c_client *client)
>  
>  	pm_runtime_disable(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
> -	pm_runtime_put_noidle(&client->dev);
>  
>  	/* power down single shot mode */
>  	return ads1015_set_conv_mode(data, ADS1015_SINGLESHOT);



Thanks,
Mauro

Re: [PATCH v2 3/6] iio: chemical: atlas-sensor: Balance runtime pm + pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Sun, 16 May 2021 17:21:00 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> The pm_runtime_put_noidle() call in remove isn't balanced with any get, so
> drop it.  Note this isn't a bug as the runtime pm core will not allow the
> reference count to go negative, making this a noop. However, it is
> confusing to the reader so let's drop it.
> 
> pm_runtime_resume_and_get() replacement found using the coccicheck script
> under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> 
> As pm_runtime_resume_and_get() returns <= 0 take advantage of that to
> change the error checking to if (ret) which is more in keeping with the
> rest of this driver.
> 
> This is a prequel to taking a closer look at the runtime pm in IIO drivers
> in general.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Matt Ranostay <matt.ranostay@konsulko.com>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  drivers/iio/chemical/atlas-sensor.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c
> index 0fdb3b29c5eb..9cb99585b6ff 100644
> --- a/drivers/iio/chemical/atlas-sensor.c
> +++ b/drivers/iio/chemical/atlas-sensor.c
> @@ -410,11 +410,9 @@ static int atlas_buffer_postenable(struct iio_dev *indio_dev)
>  	struct atlas_data *data = iio_priv(indio_dev);
>  	int ret;
>  
> -	ret = pm_runtime_get_sync(&data->client->dev);
> -	if (ret < 0) {
> -		pm_runtime_put_noidle(&data->client->dev);
> +	ret = pm_runtime_resume_and_get(&data->client->dev);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	return atlas_set_interrupt(data, true);
>  }
> @@ -487,11 +485,9 @@ static int atlas_read_measurement(struct atlas_data *data, int reg, __be32 *val)
>  	int suspended = pm_runtime_suspended(dev);
>  	int ret;
>  
> -	ret = pm_runtime_get_sync(dev);
> -	if (ret < 0) {
> -		pm_runtime_put_noidle(dev);
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	if (suspended)
>  		msleep(data->chip->delay);
> @@ -741,7 +737,6 @@ static int atlas_remove(struct i2c_client *client)
>  
>  	pm_runtime_disable(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
> -	pm_runtime_put_noidle(&client->dev);
>  
>  	return atlas_set_powermode(data, 0);
>  }



Thanks,
Mauro

Re: [PATCH v2 6/6] iio: pressure: icp10100: Balance runtime pm + use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Sun, 16 May 2021 17:21:03 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> The devm_ handled runtime pm disable calls pm_runtime_put_sync_suspend()
> which isn't balancing a matching get call.  It isn't a bug as such,
> because the runtime pm core doesn't decrement the reference count below
> zero, but it is missleading so let's drop it.
> 
> Using pm_runtime_resume_and_get() new call makes it easy to handle
> failures in resume as it doesn't hold a reference count if it exits
> with an error.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  drivers/iio/pressure/icp10100.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/pressure/icp10100.c b/drivers/iio/pressure/icp10100.c
> index 48759fc4bf18..af4621eaa6b5 100644
> --- a/drivers/iio/pressure/icp10100.c
> +++ b/drivers/iio/pressure/icp10100.c
> @@ -250,7 +250,9 @@ static int icp10100_get_measures(struct icp10100_state *st,
>  	__be16 measures[3];
>  	int ret;
>  
> -	pm_runtime_get_sync(&st->client->dev);
> +	ret = pm_runtime_resume_and_get(&st->client->dev);
> +	if (ret < 0)
> +		return ret;
>  
>  	mutex_lock(&st->lock);
>  	cmd = &icp10100_cmd_measure[st->mode];
> @@ -525,7 +527,6 @@ static void icp10100_pm_disable(void *data)
>  {
>  	struct device *dev = data;
>  
> -	pm_runtime_put_sync_suspend(dev);
>  	pm_runtime_disable(dev);
>  }
>  



Thanks,
Mauro

Re: [PATCH v2 4/6] iio: prox: pulsed-light-v2: Fix misbalance runtime pm in error path

[Mauro Carvalho Chehab]

Em Sun, 16 May 2021 17:21:01 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> There is one path in which we don't do a runtime pm put and so
> leave the device enabled for ever more.
> 
> Reported-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Fixes: 4ac4e086fd8c ("iio: pulsedlight-lidar-lite: add runtime PM")
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Matt Ranostay <matt.ranostay@konsulko.com>

I was unable to apply it on the top of next-20210615, although
the patch looks good to me.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  drivers/iio/proximity/pulsedlight-lidar-lite-v2.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> index 822a68ae5e03..ecaeb1e11007 100644
> --- a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> +++ b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> @@ -164,7 +164,7 @@ static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
>  	ret = lidar_write_control(data, LIDAR_REG_CONTROL_ACQUIRE);
>  	if (ret < 0) {
>  		dev_err(&client->dev, "cannot send start measurement command");
> -		return ret;
> +		goto err;
>  	}
>  
>  	while (tries--) {
> @@ -188,6 +188,7 @@ static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
>  		}
>  		ret = -EIO;
>  	}
> +err:
>  	pm_runtime_mark_last_busy(&client->dev);
>  	pm_runtime_put_autosuspend(&client->dev);
>  



Thanks,
Mauro

Re: [PATCH v2 5/6] iio: prox: pulsed-light-v2: Use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Sun, 16 May 2021 13:01:29 -0700
Matt Ranostay <matt.ranostay@konsulko.com> escreveu:

> On Sun, May 16, 2021 at 9:22 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > Using this new call makes it easy to handle any errors as a result
> > of runtime resume as it exits without leaving the reference count
> > elevated.
> >  
> 
> Acked-by: Matt Ranostay <matt.ranostay@konsulko.com>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> > ---
> >  drivers/iio/proximity/pulsedlight-lidar-lite-v2.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> > index ecaeb1e11007..e94f63932edb 100644
> > --- a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> > +++ b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> > @@ -158,7 +158,9 @@ static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
> >         int tries = 10;
> >         int ret;
> >
> > -       pm_runtime_get_sync(&client->dev);
> > +       ret = pm_runtime_resume_and_get(&client->dev);
> > +       if (ret < 0)
> > +               return ret;
> >
> >         /* start sample */
> >         ret = lidar_write_control(data, LIDAR_REG_CONTROL_ACQUIRE);
> > --
> > 2.31.1
> >  



Thanks,
Mauro

Re: [PATCH v2 1/6] iio: imu: mpu6050: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

On Wed, 16 Jun 2021 09:15:23 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Sun, 16 May 2021 17:20:58 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Remove an unblanced pm_runtime_put_sync_suspend() call
> > in inv_pu_pm_disable().  Not this call is not a bug, because the runtime
> > pm core will not allow the reference counter to go negative.  It is
> > however confusing and serves no purpose.
> > 
> > pm_runtime_resume_and_get() case found using coccicheck script under
> > review at:
> > https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> > 
> > pm_runtime_resume_and_get() returns <= 0 only so simplify related checks
> > to bring this more inline with nearby calls.
> > 
> > This is a prequel to taking a closer look at the runtime pm in IIO drivers
> > in general.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>  
> 
> LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Thanks!

Applied to the togreg branch of iio.git and pushed out as testing for
0-day to see if it can find anything we missed.

Jonathan

> 
> 
> > ---
> >  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 19 ++++++-------------
> >  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  6 ++----
> >  2 files changed, 8 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > index 6244a07048df..4cae3765e327 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > @@ -570,11 +570,9 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
> >  	freq_hz = INV_MPU6050_DIVIDER_TO_FIFO_RATE(st->chip_config.divider);
> >  	period_us = 1000000 / freq_hz;
> >  
> > -	result = pm_runtime_get_sync(pdev);
> > -	if (result < 0) {
> > -		pm_runtime_put_noidle(pdev);
> > +	result = pm_runtime_resume_and_get(pdev);
> > +	if (result)
> >  		return result;
> > -	}
> >  
> >  	switch (chan->type) {
> >  	case IIO_ANGL_VEL:
> > @@ -812,11 +810,9 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
> >  		return result;
> >  
> >  	mutex_lock(&st->lock);
> > -	result = pm_runtime_get_sync(pdev);
> > -	if (result < 0) {
> > -		pm_runtime_put_noidle(pdev);
> > +	result = pm_runtime_resume_and_get(pdev);
> > +	if (result)
> >  		goto error_write_raw_unlock;
> > -	}
> >  
> >  	switch (mask) {
> >  	case IIO_CHAN_INFO_SCALE:
> > @@ -930,11 +926,9 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
> >  		result = 0;
> >  		goto fifo_rate_fail_unlock;
> >  	}
> > -	result = pm_runtime_get_sync(pdev);
> > -	if (result < 0) {
> > -		pm_runtime_put_noidle(pdev);
> > +	result = pm_runtime_resume_and_get(pdev);
> > +	if (result)
> >  		goto fifo_rate_fail_unlock;
> > -	}
> >  
> >  	result = regmap_write(st->map, st->reg->sample_rate_div, d);
> >  	if (result)
> > @@ -1422,7 +1416,6 @@ static void inv_mpu_pm_disable(void *data)
> >  {
> >  	struct device *dev = data;
> >  
> > -	pm_runtime_put_sync_suspend(dev);
> >  	pm_runtime_disable(dev);
> >  }
> >  
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > index e21ba778595a..2d0e8cdd4848 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > @@ -173,11 +173,9 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
> >  
> >  	if (enable) {
> >  		scan = inv_scan_query(indio_dev);
> > -		result = pm_runtime_get_sync(pdev);
> > -		if (result < 0) {
> > -			pm_runtime_put_noidle(pdev);
> > +		result = pm_runtime_resume_and_get(pdev);
> > +		if (result)
> >  			return result;
> > -		}
> >  		/*
> >  		 * In case autosuspend didn't trigger, turn off first not
> >  		 * required sensors.  
> 
> 
> 
> Thanks,
> Mauro

Re: [PATCH v2 2/6] iio: adc: ads1015: Balance runtime pm + pm_runtime_resume_and_get()

[Jonathan Cameron]

On Wed, 16 Jun 2021 09:15:55 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Sun, 16 May 2021 17:20:59 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > The call to pm_runtime_put_noidle() in remove() is not balancing a
> > counter increment.  Note this doesn't matter as the runtime pm core
> > will not allow the counter to go negative.  However, it is confusing
> > to the reader so let's remove it.
> > 
> > The pm_runtime_resume_and_get() replacement was found using coccicheck
> > script under review at:
> > https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> > 
> > This is a prequel to taking a closer look at the runtime pm in IIO drivers
> > in general.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Applied
> 
> > ---
> >  drivers/iio/adc/ti-ads1015.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> > index 5b828428be77..b0352e91ac16 100644
> > --- a/drivers/iio/adc/ti-ads1015.c
> > +++ b/drivers/iio/adc/ti-ads1015.c
> > @@ -323,9 +323,7 @@ static int ads1015_set_power_state(struct ads1015_data *data, bool on)
> >  	struct device *dev = regmap_get_device(data->regmap);
> >  
> >  	if (on) {
> > -		ret = pm_runtime_get_sync(dev);
> > -		if (ret < 0)
> > -			pm_runtime_put_noidle(dev);
> > +		ret = pm_runtime_resume_and_get(dev);
> >  	} else {
> >  		pm_runtime_mark_last_busy(dev);
> >  		ret = pm_runtime_put_autosuspend(dev);
> > @@ -1070,7 +1068,6 @@ static int ads1015_remove(struct i2c_client *client)
> >  
> >  	pm_runtime_disable(&client->dev);
> >  	pm_runtime_set_suspended(&client->dev);
> > -	pm_runtime_put_noidle(&client->dev);
> >  
> >  	/* power down single shot mode */
> >  	return ads1015_set_conv_mode(data, ADS1015_SINGLESHOT);  
> 
> 
> 
> Thanks,
> Mauro

Re: [PATCH v2 3/6] iio: chemical: atlas-sensor: Balance runtime pm + pm_runtime_resume_and_get()

[Jonathan Cameron]

On Wed, 16 Jun 2021 09:16:20 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Sun, 16 May 2021 17:21:00 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > The pm_runtime_put_noidle() call in remove isn't balanced with any get, so
> > drop it.  Note this isn't a bug as the runtime pm core will not allow the
> > reference count to go negative, making this a noop. However, it is
> > confusing to the reader so let's drop it.
> > 
> > pm_runtime_resume_and_get() replacement found using the coccicheck script
> > under review at:
> > https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> > 
> > As pm_runtime_resume_and_get() returns <= 0 take advantage of that to
> > change the error checking to if (ret) which is more in keeping with the
> > rest of this driver.
> > 
> > This is a prequel to taking a closer look at the runtime pm in IIO drivers
> > in general.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Matt Ranostay <matt.ranostay@konsulko.com>  
> 
> LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Applied

> 
> > ---
> >  drivers/iio/chemical/atlas-sensor.c | 13 ++++---------
> >  1 file changed, 4 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c
> > index 0fdb3b29c5eb..9cb99585b6ff 100644
> > --- a/drivers/iio/chemical/atlas-sensor.c
> > +++ b/drivers/iio/chemical/atlas-sensor.c
> > @@ -410,11 +410,9 @@ static int atlas_buffer_postenable(struct iio_dev *indio_dev)
> >  	struct atlas_data *data = iio_priv(indio_dev);
> >  	int ret;
> >  
> > -	ret = pm_runtime_get_sync(&data->client->dev);
> > -	if (ret < 0) {
> > -		pm_runtime_put_noidle(&data->client->dev);
> > +	ret = pm_runtime_resume_and_get(&data->client->dev);
> > +	if (ret)
> >  		return ret;
> > -	}
> >  
> >  	return atlas_set_interrupt(data, true);
> >  }
> > @@ -487,11 +485,9 @@ static int atlas_read_measurement(struct atlas_data *data, int reg, __be32 *val)
> >  	int suspended = pm_runtime_suspended(dev);
> >  	int ret;
> >  
> > -	ret = pm_runtime_get_sync(dev);
> > -	if (ret < 0) {
> > -		pm_runtime_put_noidle(dev);
> > +	ret = pm_runtime_resume_and_get(dev);
> > +	if (ret)
> >  		return ret;
> > -	}
> >  
> >  	if (suspended)
> >  		msleep(data->chip->delay);
> > @@ -741,7 +737,6 @@ static int atlas_remove(struct i2c_client *client)
> >  
> >  	pm_runtime_disable(&client->dev);
> >  	pm_runtime_set_suspended(&client->dev);
> > -	pm_runtime_put_noidle(&client->dev);
> >  
> >  	return atlas_set_powermode(data, 0);
> >  }  
> 
> 
> 
> Thanks,
> Mauro

Re: [PATCH v2 4/6] iio: prox: pulsed-light-v2: Fix misbalance runtime pm in error path

[Jonathan Cameron]

On Wed, 16 Jun 2021 09:18:42 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Sun, 16 May 2021 17:21:01 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > There is one path in which we don't do a runtime pm put and so
> > leave the device enabled for ever more.
> > 
> > Reported-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > Fixes: 4ac4e086fd8c ("iio: pulsedlight-lidar-lite: add runtime PM")
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Matt Ranostay <matt.ranostay@konsulko.com>  
> 
> I was unable to apply it on the top of next-20210615, although
> the patch looks good to me.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Ah. This crossed with another fix for the same issue.

I'll take another look at whether the alternate fix works
as well as this and revisit this one at a future date.

Thanks,

Jonathan

> 
> > ---
> >  drivers/iio/proximity/pulsedlight-lidar-lite-v2.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> > index 822a68ae5e03..ecaeb1e11007 100644
> > --- a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> > +++ b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> > @@ -164,7 +164,7 @@ static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
> >  	ret = lidar_write_control(data, LIDAR_REG_CONTROL_ACQUIRE);
> >  	if (ret < 0) {
> >  		dev_err(&client->dev, "cannot send start measurement command");
> > -		return ret;
> > +		goto err;
> >  	}
> >  
> >  	while (tries--) {
> > @@ -188,6 +188,7 @@ static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
> >  		}
> >  		ret = -EIO;
> >  	}
> > +err:
> >  	pm_runtime_mark_last_busy(&client->dev);
> >  	pm_runtime_put_autosuspend(&client->dev);
> >    
> 
> 
> 
> Thanks,
> Mauro

Re: [PATCH v2 5/6] iio: prox: pulsed-light-v2: Use pm_runtime_resume_and_get()

[Jonathan Cameron]

On Wed, 16 Jun 2021 09:19:13 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Sun, 16 May 2021 13:01:29 -0700
> Matt Ranostay <matt.ranostay@konsulko.com> escreveu:
> 
> > On Sun, May 16, 2021 at 9:22 AM Jonathan Cameron <jic23@kernel.org> wrote:  
> > >
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >
> > > Using this new call makes it easy to handle any errors as a result
> > > of runtime resume as it exits without leaving the reference count
> > > elevated.
> > >    
> > 
> > Acked-by: Matt Ranostay <matt.ranostay@konsulko.com>  
> 
> LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Applied.

> 
> >   
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> > > ---
> > >  drivers/iio/proximity/pulsedlight-lidar-lite-v2.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> > > index ecaeb1e11007..e94f63932edb 100644
> > > --- a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> > > +++ b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> > > @@ -158,7 +158,9 @@ static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
> > >         int tries = 10;
> > >         int ret;
> > >
> > > -       pm_runtime_get_sync(&client->dev);
> > > +       ret = pm_runtime_resume_and_get(&client->dev);
> > > +       if (ret < 0)
> > > +               return ret;
> > >
> > >         /* start sample */
> > >         ret = lidar_write_control(data, LIDAR_REG_CONTROL_ACQUIRE);
> > > --
> > > 2.31.1
> > >    
> 
> 
> 
> Thanks,
> Mauro

Re: [PATCH v2 6/6] iio: pressure: icp10100: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

On Wed, 16 Jun 2021 09:16:38 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Sun, 16 May 2021 17:21:03 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > The devm_ handled runtime pm disable calls pm_runtime_put_sync_suspend()
> > which isn't balancing a matching get call.  It isn't a bug as such,
> > because the runtime pm core doesn't decrement the reference count below
> > zero, but it is missleading so let's drop it.
> > 
> > Using pm_runtime_resume_and_get() new call makes it easy to handle
> > failures in resume as it doesn't hold a reference count if it exits
> > with an error.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>  
> 
> LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Applied.

> 
> > ---
> >  drivers/iio/pressure/icp10100.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/pressure/icp10100.c b/drivers/iio/pressure/icp10100.c
> > index 48759fc4bf18..af4621eaa6b5 100644
> > --- a/drivers/iio/pressure/icp10100.c
> > +++ b/drivers/iio/pressure/icp10100.c
> > @@ -250,7 +250,9 @@ static int icp10100_get_measures(struct icp10100_state *st,
> >  	__be16 measures[3];
> >  	int ret;
> >  
> > -	pm_runtime_get_sync(&st->client->dev);
> > +	ret = pm_runtime_resume_and_get(&st->client->dev);
> > +	if (ret < 0)
> > +		return ret;
> >  
> >  	mutex_lock(&st->lock);
> >  	cmd = &icp10100_cmd_measure[st->mode];
> > @@ -525,7 +527,6 @@ static void icp10100_pm_disable(void *data)
> >  {
> >  	struct device *dev = data;
> >  
> > -	pm_runtime_put_sync_suspend(dev);
> >  	pm_runtime_disable(dev);
> >  }
> >    
> 
> 
> 
> Thanks,
> Mauro