[PATCH] iio: imu: inv_mpu6050: fix suspend/resume with runtime power

[PATCH] iio: imu: inv_mpu6050: fix suspend/resume with runtime power

[Jean-Baptiste Maneyrol]

Suspend/resume were not working correctly with pm runtime.
Now suspend check if the chip is already suspended, and
resume put runtime pm in the correct state.

Fixes: 4599cac84614 ("iio: imu: inv_mpu6050: use runtime pm with autosuspend")
Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index e4b0d368c2f9..a58bab03f0b0 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -1636,6 +1636,10 @@ static int __maybe_unused inv_mpu_resume(struct device *dev)
 	if (result)
 		goto out_unlock;
 
+	pm_runtime_disable(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	result = inv_mpu6050_switch_engine(st, true, st->suspended_sensors);
 	if (result)
 		goto out_unlock;
@@ -1657,13 +1661,18 @@ static int __maybe_unused inv_mpu_suspend(struct device *dev)
 
 	mutex_lock(&st->lock);
 
+	st->suspended_sensors = 0;
+	if (pm_runtime_suspended(dev)) {
+		result = 0;
+		goto out_unlock;
+	}
+
 	if (iio_buffer_enabled(indio_dev)) {
 		result = inv_mpu6050_prepare_fifo(st, false);
 		if (result)
 			goto out_unlock;
 	}
 
-	st->suspended_sensors = 0;
 	if (st->chip_config.accl_en)
 		st->suspended_sensors |= INV_MPU6050_SENSOR_ACCL;
 	if (st->chip_config.gyro_en)
-- 
2.17.1

Re: [PATCH] iio: imu: inv_mpu6050: fix suspend/resume with runtime power

[Andy Shevchenko]

On Tue, Mar 31, 2020 at 4:39 PM Jean-Baptiste Maneyrol
<jmaneyrol@invensense.com> wrote:
>
> Suspend/resume were not working correctly with pm runtime.
> Now suspend check if the chip is already suspended, and
> resume put runtime pm in the correct state.
>
> Fixes: 4599cac84614 ("iio: imu: inv_mpu6050: use runtime pm with autosuspend")
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>

> +       st->suspended_sensors = 0;
> +       if (pm_runtime_suspended(dev)) {
> +               result = 0;
> +               goto out_unlock;
> +       }

AFAIK this is not enough to guarantee that device *will be* suspended.
That said, in one thread you may get device in the middle of RPM
suspend, while here you are checking if it's okay or not, but after in
the other thread you will get an error and roll back to the resumed
state.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: imu: inv_mpu6050: fix suspend/resume with runtime power

[Jean-Baptiste Maneyrol]

Hello,

by reading kernel documentation, I was thinking that PM was excluding this possibility.

Quote from power/runtime_pm:
"The PM core does its best to reduce the probability of race conditions between the runtime PM and system suspend/resume (and hibernation) callbacks by carrying out the following operations:
        * During system suspend pm_runtime_get_noresume() is called for every device right before executing the subsystem-level .prepare() callback for it and pm_runtime_barrier() is called for every device right before executing the subsystem-level .suspend() callback for it. In addition to that the PM core calls __pm_runtime_disable() with ‘false’ as the second argument for every device right before executing the subsystem-level .suspend_late() callback for it.
         * During system resume pm_runtime_enable() and pm_runtime_put() are called for every device right after executing the subsystem-level .resume_early() callback and right after executing the subsystem-level .complete() callback for it, respectively"

The 2 suspend callbacks are also locking the device mutex.

But I can totally misunderstood the situation. If you can confirm if it is the case or not, that would be really helpful.

Thanks a lot,
JB


From: Andy Shevchenko <andy.shevchenko@gmail.com>

Sent: Tuesday, March 31, 2020 19:29

To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>

Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>

Subject: Re: [PATCH] iio: imu: inv_mpu6050: fix suspend/resume with runtime power

 


 CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe.



On Tue, Mar 31, 2020 at 4:39 PM Jean-Baptiste Maneyrol

<jmaneyrol@invensense.com> wrote:

>

> Suspend/resume were not working correctly with pm runtime.

> Now suspend check if the chip is already suspended, and

> resume put runtime pm in the correct state.

>

> Fixes: 4599cac84614 ("iio: imu: inv_mpu6050: use runtime pm with autosuspend")

> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>



> +       st->suspended_sensors = 0;

> +       if (pm_runtime_suspended(dev)) {

> +               result = 0;

> +               goto out_unlock;

> +       }



AFAIK this is not enough to guarantee that device *will be* suspended.

That said, in one thread you may get device in the middle of RPM

suspend, while here you are checking if it's okay or not, but after in

the other thread you will get an error and roll back to the resumed

state.



-- 

With Best Regards,

Andy Shevchenko

Re: [PATCH] iio: imu: inv_mpu6050: fix suspend/resume with runtime power

[Andy Shevchenko]

On Tue, Mar 31, 2020 at 10:46 PM Jean-Baptiste Maneyrol
<JManeyrol@invensense.com> wrote:

> by reading kernel documentation, I was thinking that PM was excluding this possibility.
>
> Quote from power/runtime_pm:
> "The PM core does its best to reduce the probability of race conditions between the runtime PM and system suspend/resume (and hibernation) callbacks by carrying out the following operations:
>         * During system suspend pm_runtime_get_noresume() is called for every device right before executing the subsystem-level .prepare() callback for it and pm_runtime_barrier() is called for every device right before executing the subsystem-level .suspend() callback for it. In addition to that the PM core calls __pm_runtime_disable() with ‘false’ as the second argument for every device right before executing the subsystem-level .suspend_late() callback for it.
>          * During system resume pm_runtime_enable() and pm_runtime_put() are called for every device right after executing the subsystem-level .resume_early() callback and right after executing the subsystem-level .complete() callback for it, respectively"
>
> The 2 suspend callbacks are also locking the device mutex.
>
> But I can totally misunderstood the situation. If you can confirm if it is the case or not, that would be really helpful.

I have re-read the thread where I remember traces from. So, it was
rather IRQ handler context. So, in here probably everything is okay.





--
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: imu: inv_mpu6050: fix suspend/resume with runtime power

[Jonathan Cameron]

On Tue, 31 Mar 2020 15:38:50 +0200
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:

> Suspend/resume were not working correctly with pm runtime.

Need more info than that!
Anyhow, when you say "not working correctly" what is happening that
is wrong?

Jonathan


> Now suspend check if the chip is already suspended, and
> resume put runtime pm in the correct state.
> 
> Fixes: 4599cac84614 ("iio: imu: inv_mpu6050: use runtime pm with autosuspend")
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index e4b0d368c2f9..a58bab03f0b0 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -1636,6 +1636,10 @@ static int __maybe_unused inv_mpu_resume(struct device *dev)
>  	if (result)
>  		goto out_unlock;
>  
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +

Looking at the docs, we should do this if we were previously suspended and no longer
are.  Not sure we should do it we weren't previously in runtime suspend?

I guess it is idempotent anyway so if we were previously enabled we just set it again.
So probably fine.

Jonathan

>  	result = inv_mpu6050_switch_engine(st, true, st->suspended_sensors);
>  	if (result)
>  		goto out_unlock;
> @@ -1657,13 +1661,18 @@ static int __maybe_unused inv_mpu_suspend(struct device *dev)
>  
>  	mutex_lock(&st->lock);
>  
> +	st->suspended_sensors = 0;
> +	if (pm_runtime_suspended(dev)) {
> +		result = 0;
> +		goto out_unlock;
> +	}
> +
>  	if (iio_buffer_enabled(indio_dev)) {
>  		result = inv_mpu6050_prepare_fifo(st, false);
>  		if (result)
>  			goto out_unlock;
>  	}
>  
> -	st->suspended_sensors = 0;
>  	if (st->chip_config.accl_en)
>  		st->suspended_sensors |= INV_MPU6050_SENSOR_ACCL;
>  	if (st->chip_config.gyro_en)

Re: [PATCH] iio: imu: inv_mpu6050: fix suspend/resume with runtime power

[Jean-Baptiste Maneyrol]

Hello Jonathan,

there were 2 issues with suspend/resume when the device was already suspended by runtime pm.

When entering suspend, there was an error in logs because we were disabling vddio regulator although it was already disabled.
And when resuming, the chip was pull back to full power but the pm_runtime state was not updated. So it was believing it was still suspended.

Do you need a new patch with full description?

Thanks,
JB


From: Jonathan Cameron <jic23@kernel.org>

Sent: Sunday, April 5, 2020 15:15

To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>

Cc: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>

Subject: Re: [PATCH] iio: imu: inv_mpu6050: fix suspend/resume with runtime power

 


 CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe.



On Tue, 31 Mar 2020 15:38:50 +0200

Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:



> Suspend/resume were not working correctly with pm runtime.



Need more info than that!

Anyhow, when you say "not working correctly" what is happening that

is wrong?



Jonathan





> Now suspend check if the chip is already suspended, and

> resume put runtime pm in the correct state.

> 

> Fixes: 4599cac84614 ("iio: imu: inv_mpu6050: use runtime pm with autosuspend")

> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>

> ---

>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 11 ++++++++++-

>  1 file changed, 10 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c

> index e4b0d368c2f9..a58bab03f0b0 100644

> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c

> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c

> @@ -1636,6 +1636,10 @@ static int __maybe_unused inv_mpu_resume(struct device *dev)

>        if (result)

>                goto out_unlock;

>  

> +     pm_runtime_disable(dev);

> +     pm_runtime_set_active(dev);

> +     pm_runtime_enable(dev);

> +



Looking at the docs, we should do this if we were previously suspended and no longer

are.  Not sure we should do it we weren't previously in runtime suspend?



I guess it is idempotent anyway so if we were previously enabled we just set it again.

So probably fine.



Jonathan



>        result = inv_mpu6050_switch_engine(st, true, st->suspended_sensors);

>        if (result)

>                goto out_unlock;

> @@ -1657,13 +1661,18 @@ static int __maybe_unused inv_mpu_suspend(struct device *dev)

>  

>        mutex_lock(&st->lock);

>  

> +     st->suspended_sensors = 0;

> +     if (pm_runtime_suspended(dev)) {

> +             result = 0;

> +             goto out_unlock;

> +     }

> +

>        if (iio_buffer_enabled(indio_dev)) {

>                result = inv_mpu6050_prepare_fifo(st, false);

>                if (result)

>                        goto out_unlock;

>        }

>  

> -     st->suspended_sensors = 0;

>        if (st->chip_config.accl_en)

>                st->suspended_sensors |= INV_MPU6050_SENSOR_ACCL;

>        if (st->chip_config.gyro_en)

Re: [PATCH] iio: imu: inv_mpu6050: fix suspend/resume with runtime power

[Jonathan Cameron]

On Mon, 6 Apr 2020 07:33:44 +0000
Jean-Baptiste Maneyrol <JManeyrol@invensense.com> wrote:

> Hello Jonathan,
> 
> there were 2 issues with suspend/resume when the device was already suspended by runtime pm.
> 
> When entering suspend, there was an error in logs because we were disabling vddio regulator although it was already disabled.
> And when resuming, the chip was pull back to full power but the pm_runtime state was not updated. So it was believing it was still suspended.
> 
> Do you need a new patch with full description?
> 
I edited to the description to add this info.

Applied to the fixes-togreg branch of iio.git

Thanks

Jonathan


> Thanks,
> JB
> 
> 
> From: Jonathan Cameron <jic23@kernel.org>
> 
> Sent: Sunday, April 5, 2020 15:15
> 
> To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>
> 
> Cc: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>
> 
> Subject: Re: [PATCH] iio: imu: inv_mpu6050: fix suspend/resume with runtime power
> 
>  
> 
> 
>  CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> 
> On Tue, 31 Mar 2020 15:38:50 +0200
> 
> Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:
> 
> 
> 
> > Suspend/resume were not working correctly with pm runtime.  
> 
> 
> 
> Need more info than that!
> 
> Anyhow, when you say "not working correctly" what is happening that
> 
> is wrong?
> 
> 
> 
> Jonathan
> 
> 
> 
> 
> 
> > Now suspend check if the chip is already suspended, and  
> 
> > resume put runtime pm in the correct state.  
> 
> >   
> 
> > Fixes: 4599cac84614 ("iio: imu: inv_mpu6050: use runtime pm with autosuspend")  
> 
> > Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>  
> 
> > ---  
> 
> >  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 11 ++++++++++-  
> 
> >  1 file changed, 10 insertions(+), 1 deletion(-)  
> 
> >   
> 
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c  
> 
> > index e4b0d368c2f9..a58bab03f0b0 100644  
> 
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c  
> 
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c  
> 
> > @@ -1636,6 +1636,10 @@ static int __maybe_unused inv_mpu_resume(struct device *dev)  
> 
> >        if (result)  
> 
> >                goto out_unlock;  
> 
> >    
> 
> > +     pm_runtime_disable(dev);  
> 
> > +     pm_runtime_set_active(dev);  
> 
> > +     pm_runtime_enable(dev);  
> 
> > +  
> 
> 
> 
> Looking at the docs, we should do this if we were previously suspended and no longer
> 
> are.  Not sure we should do it we weren't previously in runtime suspend?
> 
> 
> 
> I guess it is idempotent anyway so if we were previously enabled we just set it again.
> 
> So probably fine.
> 
> 
> 
> Jonathan
> 
> 
> 
> >        result = inv_mpu6050_switch_engine(st, true, st->suspended_sensors);  
> 
> >        if (result)  
> 
> >                goto out_unlock;  
> 
> > @@ -1657,13 +1661,18 @@ static int __maybe_unused inv_mpu_suspend(struct device *dev)  
> 
> >    
> 
> >        mutex_lock(&st->lock);  
> 
> >    
> 
> > +     st->suspended_sensors = 0;  
> 
> > +     if (pm_runtime_suspended(dev)) {  
> 
> > +             result = 0;  
> 
> > +             goto out_unlock;  
> 
> > +     }  
> 
> > +  
> 
> >        if (iio_buffer_enabled(indio_dev)) {  
> 
> >                result = inv_mpu6050_prepare_fifo(st, false);  
> 
> >                if (result)  
> 
> >                        goto out_unlock;  
> 
> >        }  
> 
> >    
> 
> > -     st->suspended_sensors = 0;  
> 
> >        if (st->chip_config.accl_en)  
> 
> >                st->suspended_sensors |= INV_MPU6050_SENSOR_ACCL;  
> 
> >        if (st->chip_config.gyro_en)  
> 
> 
>