| Message ID | 20200331133850.23757-1-jmaneyrol@invensense.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: imu: inv_mpu6050: fix suspend/resume with runtime power | expand |
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.
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.
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
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)
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)
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) > > >
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)
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(-)