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

[Jonathan Cameron <jic23@kernel.org>]

On Sun,  6 Sep 2026 15:11:40 +0200
Fabio Cesari <fabio.cesari@gmail.com> wrote:

> Both isl29028_read_raw() and isl29028_write_raw() take a runtime PM
> reference with pm_runtime_resume_and_get() and are supposed to drop it
> again with pm_runtime_put_autosuspend() before returning. On their error
> paths they return directly instead, leaking the reference.
> 
> The usage count is then never balanced, so the device stops entering
> autosuspend for the rest of its lifetime. The effect accumulates: every
> failed access leaks another reference.
> 
> In isl29028_write_raw() this is reachable from userspace with a single
> rejected sysfs write, for example
> 
>   echo 200 > in_proximity_sampling_frequency
> 
> which is outside the accepted [1:100] range, or
> 
>   echo 999 > in_illuminance_scale
> 
> which is not one of the two accepted scales. Both return -EINVAL with
> the reference still held. In isl29028_read_raw() the leak is reached
> when the underlying regmap access fails.
> 
> Drop the reference before checking the error, reusing the pm_ret
> pattern already present in isl29028_read_raw().
> 
> Found by auditing IIO drivers for runtime PM acquire/release imbalances
> with a Coccinelle semantic patch that models pm_runtime_resume_and_get()
> and pm_runtime_put_autosuspend() along the control flow graph, flagging
> functions that take a reference and then reach a return without dropping
> it.
> 
> Fixes: 2db5054ac28d ("staging: iio: isl29028: add runtime power management support")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5 coccinelle
> Signed-off-by: Fabio Cesari <fabio.cesari@gmail.com>
> ---
> 
> Compile-tested only: arm64 (native) and x86_64 (cross), defconfig plus
> CONFIG_SENSORS_ISL29028=m, 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.
> 
> I also have a version that takes the runtime PM reference only where it
> is needed: isl29028_write_raw() validates its arguments first, and
> isl29028_read_raw() acquires it only for the reads that reach the
> hardware, the sampling frequency and lux scale being cached. It also
> stops propagating the pm_runtime_put_autosuspend() return value to
> userspace, which fixes a second problem: with CONFIG_PM=n that call
> returns -ENOSYS, so every read and write fails today even when the
> access itself succeeded.
> 
> I kept this patch to the one bug, since the rest changes what userspace
> sees. Happy to send that version on top once this lands, or instead of
> this one if you would rather have it that way.
> 
>  drivers/iio/light/isl29028.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/light/isl29028.c b/drivers/iio/light/isl29028.c
> index 33deb1726689..c5146c1d9f39 100644
> --- a/drivers/iio/light/isl29028.c
> +++ b/drivers/iio/light/isl29028.c
> @@ -340,7 +340,7 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
>  {
>  	struct isl29028_chip *chip = iio_priv(indio_dev);
>  	struct device *dev = regmap_get_device(chip->regmap);
> -	int ret;
> +	int ret, pm_ret;
>  
>  	ret = pm_runtime_resume_and_get(dev);
>  	if (ret < 0)
> @@ -392,12 +392,11 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
>  
>  	mutex_unlock(&chip->lock);
>  
> +	pm_ret = pm_runtime_put_autosuspend(dev);
>  	if (ret < 0)
>  		return ret;

Whilst perhaps not best practice as such, it is pretty
common to just not bother checking the return of pm_runtime_put_autosuspend()
at least partly because of that annoying -ENOSYS result if runtime pm isn't
enabled.  That is what happens with the ACQUIRE macros for instance.

Do we have any particular reason to thing it is more likely to fail i this
case than any other?

> -
> -	ret = pm_runtime_put_autosuspend(dev);
> -	if (ret < 0)
> -		return ret;
> +	if (pm_ret < 0)
> +		return pm_ret;
>  
>  	return 0;
>  }
> @@ -461,15 +460,14 @@ 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 (ret < 0)
> +		return ret;
>  	if (pm_ret < 0)
>  		return pm_ret;
>  
Similar applies here.

> 
> base-commit: cee9395acd8043be0644b25c34bfa86623f2b935