[PATCH v3] iio: light: isl29028: fix runtime PM reference leak on error paths

[PATCH v3] iio: light: isl29028: fix runtime PM reference leak on error paths

[Fabio Cesari]

isl29028_read_raw() and isl29028_write_raw() take a runtime PM reference
with pm_runtime_resume_and_get() but return directly on their error
paths without dropping it. The usage count never balances again and the
device stops entering autosuspend for good. In isl29028_read_raw() this
needs a regmap access to fail; in isl29028_write_raw() one rejected
sysfs write is enough, for example

  echo 200 > in_proximity_sampling_frequency

which is outside the [1:100] range and returns -EINVAL with the
reference still held.

Take the reference with PM_RUNTIME_ACQUIRE_AUTOSUSPEND() instead, so it
is released on every return path.

This also stops the return value of pm_runtime_put_autosuspend() from
reaching userspace. That value only says whether the device could be
suspended right away, so -EAGAIN or -EPERM turns a successful access
into a failure, and with CONFIG_PM=n the stub returns -ENOSYS on every
access.

PM_RUNTIME_ACQUIRE_AUTOSUSPEND() exists since v6.19. Older trees need
the manual form instead: keep pm_runtime_resume_and_get() and drop the
reference on all paths with an unchecked pm_runtime_put_autosuspend().

Fixes: 2db5054ac28d ("staging: iio: isl29028: add runtime power management support")
Suggested-by: Joshua Crofts <joshua.crofts1@gmail.com>
Cc: <stable@vger.kernel.org> # see patch description, needs adjustments for < 6.19
Assisted-by: LLM coccinelle
Signed-off-by: Fabio Cesari <fabio.cesari@gmail.com>
---

Changes in v3, from the review of v2:
  - use PM_RUNTIME_ACQUIRE_AUTOSUSPEND() rather than the _IF_ENABLED_
    variant
  - sent as its own thread rather than as a reply to v2

v1: https://lore.kernel.org/linux-iio/20260906131203.125407-1-fabio.cesari@gmail.com/
v2: https://lore.kernel.org/linux-iio/20260906223737.206730-1-fabio.cesari@gmail.com/

Found by auditing IIO drivers with a Coccinelle semantic patch for
runtime PM acquire/release imbalances.

Compile-tested only: arm64 (native) and x86_64 (cross), defconfig plus
CONFIG_SENSORS_ISL29028=m, plus an arm64 CONFIG_PM=n build to cover the
stubs, with gcc 15.2.0, W=1 and sparse v0.6.5-rc1: no warnings. I have no
isl29028 hardware, so this is untested at runtime.

 drivers/iio/light/isl29028.c | 33 ++++++++-------------------------
 1 file changed, 8 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/light/isl29028.c b/drivers/iio/light/isl29028.c
index 33deb1726689..e481ac908fc1 100644
--- a/drivers/iio/light/isl29028.c
+++ b/drivers/iio/light/isl29028.c
@@ -342,8 +342,9 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
 	struct device *dev = regmap_get_device(chip->regmap);
 	int ret;
 
-	ret = pm_runtime_resume_and_get(dev);
-	if (ret < 0)
+	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
 		return ret;
 
 	mutex_lock(&chip->lock);
@@ -392,14 +393,7 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
 
 	mutex_unlock(&chip->lock);
 
-	if (ret < 0)
-		return ret;
-
-	ret = pm_runtime_put_autosuspend(dev);
-	if (ret < 0)
-		return ret;
-
-	return 0;
+	return ret;
 }
 
 static int isl29028_read_raw(struct iio_dev *indio_dev,
@@ -408,10 +402,11 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
 {
 	struct isl29028_chip *chip = iio_priv(indio_dev);
 	struct device *dev = regmap_get_device(chip->regmap);
-	int ret, pm_ret;
+	int ret;
 
-	ret = pm_runtime_resume_and_get(dev);
-	if (ret < 0)
+	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
 		return ret;
 
 	mutex_lock(&chip->lock);
@@ -461,18 +456,6 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
 
 	mutex_unlock(&chip->lock);
 
-	if (ret < 0)
-		return ret;
-
-	/**
-	 * Preserve the ret variable if the call to
-	 * pm_runtime_put_autosuspend() is successful so the reading
-	 * (if applicable) is returned to user space.
-	 */
-	pm_ret = pm_runtime_put_autosuspend(dev);
-	if (pm_ret < 0)
-		return pm_ret;
-
 	return ret;
 }
 

base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.53.0

Re: [PATCH v3] iio: light: isl29028: fix runtime PM reference leak on error paths

[Joshua Crofts]

On Thu, 10 Sep 2026 08:24:35 +0200
Fabio Cesari <fabio.cesari@gmail.com> wrote:

> isl29028_read_raw() and isl29028_write_raw() take a runtime PM reference
> with pm_runtime_resume_and_get() but return directly on their error
> paths without dropping it. The usage count never balances again and the
> device stops entering autosuspend for good. In isl29028_read_raw() this
> needs a regmap access to fail; in isl29028_write_raw() one rejected
> sysfs write is enough, for example
> 
>   echo 200 > in_proximity_sampling_frequency
> 
> which is outside the [1:100] range and returns -EINVAL with the
> reference still held.
> 
> Take the reference with PM_RUNTIME_ACQUIRE_AUTOSUSPEND() instead, so it
> is released on every return path.
> 
> This also stops the return value of pm_runtime_put_autosuspend() from
> reaching userspace. That value only says whether the device could be
> suspended right away, so -EAGAIN or -EPERM turns a successful access
> into a failure, and with CONFIG_PM=n the stub returns -ENOSYS on every
> access.
> 
> PM_RUNTIME_ACQUIRE_AUTOSUSPEND() exists since v6.19. Older trees need
> the manual form instead: keep pm_runtime_resume_and_get() and drop the
> reference on all paths with an unchecked pm_runtime_put_autosuspend().
> 
> Fixes: 2db5054ac28d ("staging: iio: isl29028: add runtime power management support")
> Suggested-by: Joshua Crofts <joshua.crofts1@gmail.com>
> Cc: <stable@vger.kernel.org> # see patch description, needs adjustments for < 6.19
> Assisted-by: LLM coccinelle
> Signed-off-by: Fabio Cesari <fabio.cesari@gmail.com>
> ---

LGTM now.

Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>

-- 
Kind regards,
Joshua Crofts

Re: [PATCH v3] iio: light: isl29028: fix runtime PM reference leak on error paths

[Jonathan Cameron]

On Thu, 10 Sep 2026 08:24:35 +0200
Fabio Cesari <fabio.cesari@gmail.com> wrote:

> isl29028_read_raw() and isl29028_write_raw() take a runtime PM reference
> with pm_runtime_resume_and_get() but return directly on their error
> paths without dropping it. The usage count never balances again and the
> device stops entering autosuspend for good. In isl29028_read_raw() this
> needs a regmap access to fail; in isl29028_write_raw() one rejected
> sysfs write is enough, for example
> 
>   echo 200 > in_proximity_sampling_frequency
> 
> which is outside the [1:100] range and returns -EINVAL with the
> reference still held.
> 
> Take the reference with PM_RUNTIME_ACQUIRE_AUTOSUSPEND() instead, so it
> is released on every return path.
> 
> This also stops the return value of pm_runtime_put_autosuspend() from
> reaching userspace. That value only says whether the device could be
> suspended right away, so -EAGAIN or -EPERM turns a successful access
> into a failure, and with CONFIG_PM=n the stub returns -ENOSYS on every
> access.
> 
> PM_RUNTIME_ACQUIRE_AUTOSUSPEND() exists since v6.19. Older trees need
> the manual form instead: keep pm_runtime_resume_and_get() and drop the
> reference on all paths with an unchecked pm_runtime_put_autosuspend().

No need to talk about how to backport.  That just bloats the patch
description. By all means add that info below the ---

If you fancy doing it, a follow up cleanup would be a nice to have (see inline).

I'll take this the slow route so applying it to the testing branch of iio.git
for the next merge window.

> 
> Fixes: 2db5054ac28d ("staging: iio: isl29028: add runtime power management support")
> Suggested-by: Joshua Crofts <joshua.crofts1@gmail.com>
> Cc: <stable@vger.kernel.org> # see patch description, needs adjustments for < 6.19
Will drop that comment as well.  It's kind of obvious and may not be true
as often these macros get backported to make it easier to backport fixes like
this one.

> Assisted-by: LLM coccinelle
> Signed-off-by: Fabio Cesari <fabio.cesari@gmail.com>
> ---
> 
> Changes in v3, from the review of v2:
>   - use PM_RUNTIME_ACQUIRE_AUTOSUSPEND() rather than the _IF_ENABLED_
>     variant
>   - sent as its own thread rather than as a reply to v2
> 
> v1: https://lore.kernel.org/linux-iio/20260906131203.125407-1-fabio.cesari@gmail.com/
> v2: https://lore.kernel.org/linux-iio/20260906223737.206730-1-fabio.cesari@gmail.com/
> 
> Found by auditing IIO drivers with a Coccinelle semantic patch for
> runtime PM acquire/release imbalances.
> 
> Compile-tested only: arm64 (native) and x86_64 (cross), defconfig plus
> CONFIG_SENSORS_ISL29028=m, plus an arm64 CONFIG_PM=n build to cover the
> stubs, with gcc 15.2.0, W=1 and sparse v0.6.5-rc1: no warnings. I have no
> isl29028 hardware, so this is untested at runtime.
> 
>  drivers/iio/light/isl29028.c | 33 ++++++++-------------------------
>  1 file changed, 8 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/iio/light/isl29028.c b/drivers/iio/light/isl29028.c
> index 33deb1726689..e481ac908fc1 100644
> --- a/drivers/iio/light/isl29028.c
> +++ b/drivers/iio/light/isl29028.c
> @@ -342,8 +342,9 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
>  	struct device *dev = regmap_get_device(chip->regmap);
>  	int ret;
>  
> -	ret = pm_runtime_resume_and_get(dev);
> -	if (ret < 0)
> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
>  		return ret;
>  
>  	mutex_lock(&chip->lock);
> @@ -392,14 +393,7 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
>  
>  	mutex_unlock(&chip->lock);
Seeing this manual unlock made me take a closer look.
Given your patch takes uses the cleanup stuff already maybe we can
do better on the rest. We could leave the lock where it is, or reduce the scope.
Something like ...

	

	switch (chan->type) {
	case IIO_PROXIMITY: {
		if (mask != IIO_CHAN_INFO_SAMP_FREQ) {
			dev_err(dev,
				"%s(): proximity: Mask value 0x%08lx is not supported\n",
				__func__, mask);
			return -EINVAL;
		}

		if (val < 1 || val > 100) {
			dev_err(dev,
				"%s(): proximity: Sampling frequency %d is not in the range [1:100]\n",
				__func__, val);
			return -EINVAL;
		}
		guard(mutex)(&chip->lock);

		return isl29028_set_proxim_sampling(chip, val, val2);
	}
	case IIO_LIGHT: {
		if (mask != IIO_CHAN_INFO_SCALE) {
			dev_err(dev,
				"%s(): light: Mask value 0x%08lx is not supported\n",
				__func__, mask);
			break;
		}

		if (val != 125 && val != 2000) {
			dev_err(dev,
				"%s(): light: Lux scale %d is not in the set {125, 2000}\n",
				__func__, val);
			break;
		}
		guard(mutex)(&chip->lock);

		ret = isl29028_set_als_scale(chip, val);
		break;
	}
	default:
		dev_err(dev, "%s(): Unsupported channel type %x\n",
			__func__, chan->type);
		return -EINVAL;
Though the print can probably go as well as I assume this path can't
be hit.

	}
>  
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = pm_runtime_put_autosuspend(dev);
> -	if (ret < 0)
> -		return ret;
> -
> -	return 0;
> +	return ret;
>  }
>  
>  static int isl29028_read_raw(struct iio_dev *indio_dev,
> @@ -408,10 +402,11 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
>  {
>  	struct isl29028_chip *chip = iio_priv(indio_dev);
>  	struct device *dev = regmap_get_device(chip->regmap);
> -	int ret, pm_ret;
> +	int ret;
>  
> -	ret = pm_runtime_resume_and_get(dev);
> -	if (ret < 0)
> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
>  		return ret;
>  
>  	mutex_lock(&chip->lock);
> @@ -461,18 +456,6 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
>  
>  	mutex_unlock(&chip->lock);
>  
> -	if (ret < 0)
> -		return ret;
> -
> -	/**
> -	 * Preserve the ret variable if the call to
> -	 * pm_runtime_put_autosuspend() is successful so the reading
> -	 * (if applicable) is returned to user space.
> -	 */
> -	pm_ret = pm_runtime_put_autosuspend(dev);
> -	if (pm_ret < 0)
> -		return pm_ret;
> -
>  	return ret;
>  }
>  
> 
> base-commit: cee9395acd8043be0644b25c34bfa86623f2b935