Re: [PATCH v3 08/10] iio: accel: mma8452: use pm_ptr() and direct runtime PM calls

[Jonathan Cameron]

On Tue,  5 May 2026 23:16:38 +0530
Sanjay Chitroda <sanjayembeddedse@xxxxxxxxx> wrote:

> From: Sanjay Chitroda <sanjayembeddedse@xxxxxxxxx>
> 
> Use pm_ptr() together with DEFINE_RUNTIME_DEV_PM_OPS() so the PM ops
> pointer is automatically handled when CONFIG_PM is enabled or disabled.
> 
> Switch to direct PM runtime calls and drop
> mma8452_set_runtime_pm_state() wrapper, which is no longer needed.
> This follows modern kernel power-management conventions.
> 
> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@xxxxxxxxx>

Hi Sanjay,

The cleanup you have in here has made what i think are two nasty race
conditions much  more obvious. I think we need to fix those
(and that fix needs to go at the start of this series)

Thanks for you work on this driver - this problem has been
there a long time!

Jonathan

> ---
> Changes in v3:
> - Use direct PM runtime calls and drop redundant wrapper along with CONFIG_PM
> - Link to v2: https://lore.kernel.org/all/20260422165643.2148195-6-sanjayembedded@xxxxxxxxx/
> Changes in v2:
> - Use DEFINE_RUNTIME_DEV_PM_OPS to address review comment and resolve 0-day bot warning
> - Link to v1: https://lore.kernel.org/all/20260414192045.3598010-1-sanjayembedded@xxxxxxxxx/
> ---
>  drivers/iio/accel/mma8452.c | 55 +++++++++++++------------------------
>  1 file changed, 19 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 2cd24b1543af..5ab981481599 100644


>  static ssize_t mma8452_show_int_plus_micros(char *buf, const int (*vals)[2],
> @@ -974,6 +953,7 @@ static int mma8452_write_event_config(struct iio_dev *indio_dev,
>  				      bool state)
>  {
>  	struct mma8452_data *data = iio_priv(indio_dev);
> +	struct device *dev = &data->client->dev;
>  	int val, ret;
>  	const struct mma8452_event_regs *ev_regs;
>  
> @@ -981,8 +961,11 @@ static int mma8452_write_event_config(struct iio_dev *indio_dev,
>  	if (ret)
>  		return ret;
>  
> -	ret = mma8452_set_runtime_pm_state(data->client, state);
This smells like the same bug as below. 
> -	if (ret)
> +	if (state)
> +		ret = pm_runtime_resume_and_get(dev);
> +	else
> +		ret = pm_runtime_put_autosuspend(dev);
> +	if (ret < 0)
>  		return ret;
>  
>  	switch (dir) {
> @@ -1452,10 +1435,15 @@ static int mma8452_data_rdy_trigger_set_state(struct iio_trigger *trig,
>  {
>  	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
>  	struct mma8452_data *data = iio_priv(indio_dev);
> +	struct device *dev = &data->client->dev;
> +
>  	int reg, ret;
>  
> -	ret = mma8452_set_runtime_pm_state(data->client, state);

This functions makes me very suspicious and I think has a nasty existing
race condition.  If the auto suspend were to suspend immediately
vddio would be powered down.  Unless they supplies have odd naming on this
device that means the bus would be down before the write that follows.

As such I think this needs s rewrite to makes sure we only call
pm_runtime_put_autosuspend() after the bus writes in this function are done.

I think that is safe to do without testing as all it will do is delay
when the power gets turned off a tiny bit.

> -	if (ret)
> +	if (state)
> +		ret = pm_runtime_resume_and_get(dev);
> +	else
> +		ret = pm_runtime_put_autosuspend(dev);
> +	if (ret < 0)
>  		return ret;
>  
>  	reg = i2c_smbus_read_byte_data(data->client, MMA8452_CTRL_REG4);
> @@ -1738,7 +1726,6 @@ static void mma8452_remove(struct i2c_client *client)
>  	regulator_bulk_disable(ARRAY_SIZE(data->regs), data->regs);
>  }