Re: [PATCH 2/2] iio: accel: kxsd9: fix runtime PM leaks and unchecked returns

[Jonathan Cameron]

On Fri, 03 Jul 2026 22:53:23 +0530
Biren Pandya <birenpandya@xxxxxxxxx> wrote:

> The driver fails to check the return value of pm_runtime_get_sync(),
> leading to potential silent failures. Additionally, error paths leak the
> runtime PM usage counter by returning without decrementing it.
> 
> Fix this by transitioning to pm_runtime_resume_and_get():
> 
> - In kxsd9_write_raw() and kxsd9_read_raw(), check the resume status and
>   guarantee pm_runtime_put_autosuspend() is called on all return paths.
>   Simultaneously, refactor the control flow to align with standard IIO
>   idioms (removing the dead -EINVAL initializer in favor of explicit
>   assignments).
> - In kxsd9_buffer_preenable(), propagate the resume error code.
> - In kxsd9_common_remove(), only decrement the usage counter on a
>   successful resume, while retaining the unconditional power-down for a
>   best-effort shutdown.
> 
> Fixes: 9a9a369d6178 ("iio: accel: kxsd9: Deploy system and runtime PM")
> Signed-off-by: Biren Pandya <birenpandya@xxxxxxxxx>
> ---
>  drivers/iio/accel/kxsd9.c | 42 ++++++++++++++++++++++--------------------
>  1 file changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/accel/kxsd9.c b/drivers/iio/accel/kxsd9.c
> index 27adcdd312014..e9a0790e2eea2 100644
> --- a/drivers/iio/accel/kxsd9.c
> +++ b/drivers/iio/accel/kxsd9.c
> @@ -139,18 +139,17 @@ 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);
> +	ret = pm_runtime_resume_and_get(st->dev);
> +	if (ret < 0)
> +		return ret;
>  
> -	if (mask == IIO_CHAN_INFO_SCALE) {
> -		/* Check no integer component */
> -		if (val)
> -			ret = -EINVAL;
> -		else
> -			ret = kxsd9_write_scale(indio_dev, val2);
> -	}
> +	if (mask == IIO_CHAN_INFO_SCALE && !val)
This is a mix of checking for two very different conditions. I'd rather
they were handled separately as before - that will require you to do
	if (mask == IIO_CHAN_INFO_SCALE) {
		if (val)
			ret = -EINVAL;
		else
			ret = kxsd9_write_scale(indio_dev, val2);
	} else {
		ret = -EINVAL;

Given how ugly this is we should probably switch this to the ACQUIRE stuff.
Maybe it would be better to just do that as part of this fix rather
than as a follow up.  PM_RUNTIME_ACQUIRE_AUTOSUSPEND() and
PM_RUNTIME_ACQUIRE_ERR()

Then this can all be eaerly returns.
	if (mask != IIO_CHAN_INFO_SCALE)
		return -EINVAL;

	if (val)
		return -EINVAL;

	return kxsd9_write_scale();


	}
> +		ret = kxsd9_write_scale(indio_dev, val2);
> +	else
> +		ret = -EINVAL;
>  
>  	pm_runtime_put_autosuspend(st->dev);
>  
> @@ -161,20 +160,22 @@ static int kxsd9_read_raw(struct iio_dev *indio_dev,
>  			  struct iio_chan_spec const *chan,
>  			  int *val, int *val2, long mask)
>  {
> -	int ret = -EINVAL;
>  	struct kxsd9_state *st = iio_priv(indio_dev);
>  	unsigned int regval;
>  	__be16 raw_val;
>  	u16 nval;
> +	int ret;
>  
> -	pm_runtime_get_sync(st->dev);
> +	ret = pm_runtime_resume_and_get(st->dev);
> +	if (ret < 0)
> +		return ret;

Similar to above, switching to the ACQUIRE magic will simplify this
flow a lot as well as dealing with resource leaks.

>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  		ret = regmap_bulk_read(st->map, chan->address, &raw_val,
>  				       sizeof(raw_val));
>  		if (ret)
> -			goto error_ret;
> +			break;
>  		nval = be16_to_cpu(raw_val);
>  		/* Only 12 bits are valid */
>  		nval >>= 4;
> @@ -191,18 +192,20 @@ static int kxsd9_read_raw(struct iio_dev *indio_dev,
>  				  KXSD9_REG_CTRL_C,
>  				  &regval);
>  		if (ret < 0)
> -			goto error_ret;
> +			break;
>  		*val = 0;
>  		*val2 = kxsd9_micro_scales[regval & KXSD9_CTRL_C_FS_MASK];
>  		ret = IIO_VAL_INT_PLUS_MICRO;
>  		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
>  	}
>  
> -error_ret:
>  	pm_runtime_put_autosuspend(st->dev);
>  
>  	return ret;
> -};
> +}
>  
>  static irqreturn_t kxsd9_trigger_handler(int irq, void *p)
>  {
> @@ -240,9 +243,7 @@ static int kxsd9_buffer_preenable(struct iio_dev *indio_dev)
>  {
>  	struct kxsd9_state *st = iio_priv(indio_dev);
>  
> -	pm_runtime_get_sync(st->dev);
> -
> -	return 0;
> +	return pm_runtime_resume_and_get(st->dev);
>  }
>  
>  static int kxsd9_buffer_postdisable(struct iio_dev *indio_dev)
> @@ -480,8 +481,9 @@ void kxsd9_common_remove(struct device *dev)
>  
>  	iio_device_unregister(indio_dev);
>  	iio_triggered_buffer_cleanup(indio_dev);
> -	pm_runtime_get_sync(dev);
> -	pm_runtime_put_noidle(dev);
> +
> +	if (pm_runtime_resume_and_get(dev) >= 0)
> +		pm_runtime_put_noidle(dev);

I'd just let it underflow in this case.   Trying to cleanly 
handle errors in a driver unbind is rarely worth the effort
and despite what Sashiko thinks, runtime pm underflow isn't
a real problem - it's just messy and generally indicative
of something being less than ideal.

Jonathan

>  	pm_runtime_disable(dev);
>  	kxsd9_power_down(st);
>  }
>