[PATCH v2] iio: accel: kxsd9: fix runtime PM leaks and unchecked returns

[PATCH v2] iio: accel: kxsd9: fix runtime PM leaks and unchecked returns

[Biren Pandya]

The kxsd9 driver uses pm_runtime_get_sync() without checking its return
value, which can lead to silent failures. It also relies on manual
pm_runtime_put_autosuspend() calls, which is prone to leaks.

Specifically, kxsd9_write_raw() contains a bug where returning -EINVAL
bypasses the pm_runtime_put_autosuspend() call, leaving the device
powered on.

Modernize the runtime PM handling to fix these issues:
- kxsd9_write_raw() & kxsd9_read_raw(): Use the scoped
  PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND() macro to safely automate
  cleanup on function return and remove all manual puts/error gotos.
- kxsd9_buffer_preenable(): Use pm_runtime_resume_and_get() to safely
  check for errors and handle usage-counter unwinding.
- kxsd9_common_remove(): Use pm_runtime_resume_and_get(), and reverse
  the disable ordering to call pm_runtime_disable() before powering down
  to prevent background RPM race conditions.

Fixes: 9a9a369d6178 ("iio: accel: kxsd9: Deploy system and runtime PM")
Signed-off-by: Biren Pandya <birenpandya@gmail.com>
---
Changes in v2:
- Changed subject to reflect broader PM modernization.
- In kxsd9_common_remove(), reversed the order to call pm_runtime_disable()
  before kxsd9_power_down() to prevent background RPM race conditions.
- Used standard error checking idiom in kxsd9_common_remove() (Andy).
- Clarified commit message regarding the exact write_raw() bug and added
  the Fixes tag (Jonathan).

 drivers/iio/accel/kxsd9.c | 47 ++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/accel/kxsd9.c b/drivers/iio/accel/kxsd9.c
index 4717d80fc24a..7aa23ae64b7c 100644
--- a/drivers/iio/accel/kxsd9.c
+++ b/drivers/iio/accel/kxsd9.c
@@ -139,68 +139,63 @@ 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);
 
-	pm_runtime_get_sync(st->dev);
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(st->dev, pm);
+	if (PM_RUNTIME_ACQUIRE_ERR(&pm))
+		return PM_RUNTIME_ACQUIRE_ERR(&pm);
 
 	if (mask == IIO_CHAN_INFO_SCALE) {
 		/* Check no integer component */
 		if (val)
 			return -EINVAL;
-		ret = kxsd9_write_scale(indio_dev, val2);
+		return kxsd9_write_scale(indio_dev, val2);
 	}
 
-	pm_runtime_put_autosuspend(st->dev);
-
-	return ret;
+	return -EINVAL;
 }
 
 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);
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(st->dev, pm);
+	if (PM_RUNTIME_ACQUIRE_ERR(&pm))
+		return PM_RUNTIME_ACQUIRE_ERR(&pm);
 
 	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;
+			return ret;
 		nval = be16_to_cpu(raw_val);
 		/* Only 12 bits are valid */
 		nval >>= 4;
 		*val = nval;
-		ret = IIO_VAL_INT;
-		break;
+		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_OFFSET:
 		/* This has a bias of -2048 */
 		*val = KXSD9_ZERO_G_OFFSET;
-		ret = IIO_VAL_INT;
-		break;
+		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		ret = regmap_read(st->map,
 				  KXSD9_REG_CTRL_C,
 				  &regval);
 		if (ret < 0)
-			goto error_ret;
+			return ret;
 		*val = 0;
 		*val2 = kxsd9_micro_scales[regval & KXSD9_CTRL_C_FS_MASK];
-		ret = IIO_VAL_INT_PLUS_MICRO;
-		break;
+		return IIO_VAL_INT_PLUS_MICRO;
 	}
 
-error_ret:
-	pm_runtime_put_autosuspend(st->dev);
-
-	return ret;
+	return -EINVAL;
 };
 
 static irqreturn_t kxsd9_trigger_handler(int irq, void *p)
@@ -239,9 +234,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)
@@ -476,13 +469,17 @@ void kxsd9_common_remove(struct device *dev)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct kxsd9_state *st = iio_priv(indio_dev);
+	int ret;
 
 	iio_triggered_buffer_cleanup(indio_dev);
 	iio_device_unregister(indio_dev);
-	pm_runtime_get_sync(dev);
-	pm_runtime_put_noidle(dev);
+	ret = pm_runtime_resume_and_get(dev);
 	pm_runtime_disable(dev);
+	if (ret < 0)
+		return;
+
 	kxsd9_power_down(st);
+	pm_runtime_put_noidle(dev);
 }
 EXPORT_SYMBOL_NS(kxsd9_common_remove, "IIO_KXSD9");
 
-- 
2.50.1 (Apple Git-155)

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

[Andy Shevchenko]

On Mon, Jun 22, 2026 at 01:00:31AM +0530, Biren Pandya wrote:
> The kxsd9 driver uses pm_runtime_get_sync() without checking its return
> value, which can lead to silent failures. It also relies on manual
> pm_runtime_put_autosuspend() calls, which is prone to leaks.
> 
> Specifically, kxsd9_write_raw() contains a bug where returning -EINVAL
> bypasses the pm_runtime_put_autosuspend() call, leaving the device
> powered on.

...

> void kxsd9_common_remove(struct device *dev)
>  {
>  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>  	struct kxsd9_state *st = iio_priv(indio_dev);
> +	int ret;
>  
>  	iio_triggered_buffer_cleanup(indio_dev);
>  	iio_device_unregister(indio_dev);
> -	pm_runtime_get_sync(dev);
> -	pm_runtime_put_noidle(dev);
> +	ret = pm_runtime_resume_and_get(dev);
>  	pm_runtime_disable(dev);
> +	if (ret < 0)
> +		return;
> +
>  	kxsd9_power_down(st);
> +	pm_runtime_put_noidle(dev);
>  }

I am not sure this is correct change. Now, if the PM runtime fails, it leaves
the device to be on. Is this desired behaviour?

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Mon, 22 Jun 2026 15:57:41 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Mon, Jun 22, 2026 at 01:00:31AM +0530, Biren Pandya wrote:
> > The kxsd9 driver uses pm_runtime_get_sync() without checking its return
> > value, which can lead to silent failures. It also relies on manual
> > pm_runtime_put_autosuspend() calls, which is prone to leaks.
> > 
> > Specifically, kxsd9_write_raw() contains a bug where returning -EINVAL
> > bypasses the pm_runtime_put_autosuspend() call, leaving the device
> > powered on.  
> 
> ...
> 
> > void kxsd9_common_remove(struct device *dev)
> >  {
> >  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> >  	struct kxsd9_state *st = iio_priv(indio_dev);
> > +	int ret;
> >  
> >  	iio_triggered_buffer_cleanup(indio_dev);
> >  	iio_device_unregister(indio_dev);
See below. Sashiko correctly calls out this is badly broken.
Those two lines above need to be swapped - but it's a separate fix.
Given you are working on this driver, would you mind doing that as well
(credit sashiko for the report).

> > -	pm_runtime_get_sync(dev);
> > -	pm_runtime_put_noidle(dev);
> > +	ret = pm_runtime_resume_and_get(dev);
> >  	pm_runtime_disable(dev);
> > +	if (ret < 0)
> > +		return;
> > +
> >  	kxsd9_power_down(st);
> > +	pm_runtime_put_noidle(dev);
> >  }  
> 
> I am not sure this is correct change. Now, if the PM runtime fails, it leaves
> the device to be on. Is this desired behaviour?

Agreed. I'd just ignore that return value and carry on anyway.
If we hit errors in here we are in best attempt territory anyway as
there is no right way to recover.

> 

Please take a look at sashiko's feedback. Some of of it is to my mind
more important than what we have here.
https://sashiko.dev/#/patchset/20260621193036.78549-2-birenpandya%40gmail.com