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

[Sanjay Chitroda]

On 6 May 2026 11:36:54 pm IST, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>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
>
Hi Jonathan,

Thank you for your review and kind words.

Your guidance and feedback are very motivating and help contributors stay consistent and improve their work.

It is an honor to collaborate and learn from experienced maintainers like you.

Also, I would like to know if there are any opportunities in other IIO drivers or within the kernel framework where contributions would be helpful, especially around fixes, resource management, power management, or general code cleanup and coding style improvements. I would be glad to work on such areas and continue contributing.

Thanks,
Sanjay Chitroda

>> ---
>> 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.
>
While in-corporating direct PM runtime call; I also observed that this is against the expectation and design as per my knowledge.

However, I assume that thisust be well tested and something which I may not aware specific to driver. So I done changes as per requirement and waited for input from reviewer and potentially guidance on the same.

Now, this is clear issue and will handle same in next series.

>> -	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);
>>  }
>