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

[Joshua Crofts]

Hi Fabio,

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

...

> 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.

I'd put this paragraph under the --- as the Assisted-by tag already mentions
coccinelle.

> Fixes: 2db5054ac28d ("staging: iio: isl29028: add runtime power management support")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: Claude:claude-opus-5 coccinelle

The standard is to use "Assisted-by: LLM coccinelle" to prevent free
advertising of models.

> Signed-off-by: Fabio Cesari <fabio.cesari@xxxxxxxxx>
> ---
> 
> 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 had a whole paragraph about the functions returning -ENOSYS if PM is
disabled, only then noticing that you already mentioned this... I should
pay more attention :)

...

> @@ -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;
> -
> -	ret = pm_runtime_put_autosuspend(dev);
> -	if (ret < 0)
> -		return ret;
> +	if (pm_ret < 0)
> +		return pm_ret;

I'd suggest rewriting the driver to use the 
PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND macro, as it automatically
increments the refcount on use and decrements the refcount on scope exit,
eliminating the need for multiple _put_autosuspend() calls and manual
checking of the return value of these calls.

-- 
Kind regards,
Joshua Crofts