Re: [PATCH v4] iio: accel: kxsd9: fix runtime PM leak in write_raw

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 12 Jul 2026 14:01:06 +0530
Biren Pandya <birenpandya@gmail.com> wrote:

> The kxsd9 driver previously used manual pm_runtime_get_sync() and
> pm_runtime_put_autosuspend() calls around the entire kxsd9_write_raw()
> function. If the user provides a non-zero integer component for scale,
> the function returned -EINVAL directly, leaking the runtime PM usage
> counter.
> 
> Move the mask and value validation checks before pm_runtime_get_sync()
> to ensure the early -EINVAL returns do not leak the usage counter.
> 
> Fixes: 9a9a369d6178 ("iio: accel: kxsd9: Deploy system and runtime PM")
> Cc: stable@vger.kernel.org
> Signed-off-by: Biren Pandya <birenpandya@gmail.com>
> ---
> Changes since the reviewed version:
> - Reduced to the minimal write_raw() leak fix. Dropped the remove()
>   rework (no underflow is possible — pm_runtime_put_noidle() floors at 0)
Ah. I think I wasn't clear on what I was thinking for minimal fix.

The floor thing is something we should only use when we know there is basically
only a single user or we don't mind giving garbage to others (so in teardown flows).

Here a few things are different from that.

A single user:
- We need the power on, no point in continuing to access device.
- Carrying on may result in an error, or maybe garbage data.  Either way
  the real source of the error is hidden.
(I had another one about underflow but as you use get_sync that one didn't
 actually apply)

See below...
>   and deferred the PM-macro conversion and style cleanup to a follow-up
>   series, per Jonathan Cameron and Andy Shevchenko.
>  drivers/iio/accel/kxsd9.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/accel/kxsd9.c b/drivers/iio/accel/kxsd9.c
> index 4717d80fc24af..1af04cb4bf86a 100644
> --- a/drivers/iio/accel/kxsd9.c
> +++ b/drivers/iio/accel/kxsd9.c
> @@ -139,18 +139,18 @@ static int kxsd9_write_raw(struct iio_dev *indio_dev,
>  			   int val2,
>  			   long mask)
>  {
> -	int ret = -EINVAL;
>  	struct kxsd9_state *st = iio_priv(indio_dev);
> +	int ret;
>  
> -	pm_runtime_get_sync(st->dev);
> +	if (mask != IIO_CHAN_INFO_SCALE)
> +		return -EINVAL;
>  
> -	if (mask == IIO_CHAN_INFO_SCALE) {
> -		/* Check no integer component */
> -		if (val)
> -			return -EINVAL;
> -		ret = kxsd9_write_scale(indio_dev, val2);
> -	}
> +	/* Check no integer component */
> +	if (val)
> +		return -EINVAL;
>  
> +	pm_runtime_get_sync(st->dev);

With above in mind, I'd fix this as:

	ret = pm_runtime_resume_and_get(st->dev);
	if (ret < 0)
		return ret;

	ret = kxsd9_..
  	pm_runtime_put_autosuspend(st->dev);

	return ret;

> +	ret = kxsd9_write_scale(indio_dev, val2);
>  	pm_runtime_put_autosuspend(st->dev);
>  
>  	return ret;