Re: [PATCH v10 03/10] iio: accel: mma8452: Fix runtime PM bugs in mma8452_read()

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 28 Sep 2026 10:26:19 +0200
Esben Haabendal <esben@geanix.com> wrote:

> When runtime PM support was added [1], the implementation in mma8452_read()
> was done incorrectly, as it calls mma8452_drdy() before ensuring runtime pm
> has set the device active, so it can cause read from STATUS register while
> device is suspended/off. At the same time, the return value from
> i2c_smbus_read_i2c_block_data() was discarded, and the function would
> return the value returned from pm_runtime_put_autosuspend() instead.
> 
> When the latter bug described above was fixed [2], another bug was
> introduced, as mma8452_read() would now leak the runtime PM reference
> counter when i2c_smbus_read_i2c_block_data() failed.
> 
> This commit corrects this mess.
> 
> [1] commit 96c0cb2bbfe0 ("iio: mma8452: add support for runtime power management")
> [2] commit 5bdff291d20c ("iio: accel: mma8452: handle I2C read error(s) in mma8452_read()")
> 
> Fixes: 96c0cb2bbfe0 ("iio: mma8452: add support for runtime power management")
> Cc: stable@vger.kernel.org
> Signed-off-by: Esben Haabendal <esben@geanix.com>

I suppose not directly relevant to this patch but that
set_runtime_pm_state() helper is definitely not helping.
I'd follow up with ripping that out and calling the two runtimme
pm calls directly.  Then you would be trivially able to use
ACQUIRE_RUNTIME_PM() etc to reduce complexity in places like this.

In more recent drivers I've always pushed back against that
sort of helper precisely because it stops other sensible refactors.

In general that also follows the common pattern of not checking for
errors in the put as that trips up on things like !CONFIG_PM and
is generally not that useful!

Jonathan
> ---
>  drivers/iio/accel/mma8452.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index a937cbd84f30..a77c4b88b61d 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -242,21 +242,25 @@ static int mma8452_set_runtime_pm_state(struct i2c_client *client, bool on)
>  
>  static int mma8452_read(struct mma8452_data *data, __be16 buf[3])
>  {
> -	int ret = mma8452_drdy(data);
> -
> -	if (ret < 0)
> -		return ret;
> +	int ret;
>  
>  	ret = mma8452_set_runtime_pm_state(data->client, true);
>  	if (ret)
>  		return ret;
>  
> +	ret = mma8452_drdy(data);
> +	if (ret < 0)
> +		goto out_runtime_put;
> +
>  	ret = i2c_smbus_read_i2c_block_data(data->client, MMA8452_OUT_X,
>  					    3 * sizeof(__be16), (u8 *)buf);
>  	if (ret < 0)
> -		return ret;
> +		goto out_runtime_put;
> +
> +	return mma8452_set_runtime_pm_state(data->client, false);
>  
> -	ret = mma8452_set_runtime_pm_state(data->client, false);
> +out_runtime_put:
> +	mma8452_set_runtime_pm_state(data->client, false);
>  
>  	return ret;
>  }
>