During system suspend, the PM core sets dev->power.is_suspended for the
device that is being suspended. This flag is also being used in
rpm_resume(), to allow it to succeed by returning 1, assuming that runtime
PM has been disabled and the runtime PM status is RPM_ACTIVE, for the
device.
To make this behaviour a bit more useful, let's drop the check for the
dev->power.is_suspended flag in rpm_resume(), as it doesn't really need to
be limited to this anyway.
Signed-off-by: Ulf Hansson <[email protected]>
---
drivers/base/power/runtime.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index ec94049442b9..fadc278e3a66 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -742,8 +742,8 @@ static int rpm_resume(struct device *dev, int rpmflags)
repeat:
if (dev->power.runtime_error)
retval = -EINVAL;
- else if (dev->power.disable_depth == 1 && dev->power.is_suspended
- && dev->power.runtime_status == RPM_ACTIVE)
+ else if (dev->power.disable_depth > 0 &&
+ dev->power.runtime_status == RPM_ACTIVE)
retval = 1;
else if (dev->power.disable_depth > 0)
retval = -EACCES;
--
2.25.1
On Wed, Oct 27, 2021 at 12:26:26AM +0200, Ulf Hansson wrote:
> During system suspend, the PM core sets dev->power.is_suspended for the
> device that is being suspended. This flag is also being used in
> rpm_resume(), to allow it to succeed by returning 1, assuming that runtime
> PM has been disabled and the runtime PM status is RPM_ACTIVE, for the
> device.
>
> To make this behaviour a bit more useful, let's drop the check for the
> dev->power.is_suspended flag in rpm_resume(), as it doesn't really need to
> be limited to this anyway.
>
> Signed-off-by: Ulf Hansson <[email protected]>
> ---
> drivers/base/power/runtime.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index ec94049442b9..fadc278e3a66 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -742,8 +742,8 @@ static int rpm_resume(struct device *dev, int rpmflags)
> repeat:
> if (dev->power.runtime_error)
> retval = -EINVAL;
> - else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> - && dev->power.runtime_status == RPM_ACTIVE)
> + else if (dev->power.disable_depth > 0 &&
> + dev->power.runtime_status == RPM_ACTIVE)
IIRC there was a good reason why the original code checked for
disable_depth == 1 rather than > 0. But I don't remember exactly what
the reason was. Maybe it had something to do with the fact that during
a system sleep __device_suspend_late calls __pm_runtime_disable, and the
code was checking that there were no other disables in effect. This is
related to the documented behavior of rpm_resume (it's supposed to fail
with -EACCES if the device is disabled for runtime PM, no matter what
power state the device is in).
That probably is also the explanation for why dev->power.is_suspended
gets checked: It's how the code tells whether a system sleep is in
progress.
So overall, I suspect this change should not be made. But some other
improvement (like a nice comment) might be in order.
Alan Stern
On Wed, 27 Oct 2021 at 04:02, Alan Stern <[email protected]> wrote:
>
> On Wed, Oct 27, 2021 at 12:26:26AM +0200, Ulf Hansson wrote:
> > During system suspend, the PM core sets dev->power.is_suspended for the
> > device that is being suspended. This flag is also being used in
> > rpm_resume(), to allow it to succeed by returning 1, assuming that runtime
> > PM has been disabled and the runtime PM status is RPM_ACTIVE, for the
> > device.
> >
> > To make this behaviour a bit more useful, let's drop the check for the
> > dev->power.is_suspended flag in rpm_resume(), as it doesn't really need to
> > be limited to this anyway.
> >
> > Signed-off-by: Ulf Hansson <[email protected]>
> > ---
> > drivers/base/power/runtime.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index ec94049442b9..fadc278e3a66 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -742,8 +742,8 @@ static int rpm_resume(struct device *dev, int rpmflags)
> > repeat:
> > if (dev->power.runtime_error)
> > retval = -EINVAL;
> > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> > - && dev->power.runtime_status == RPM_ACTIVE)
> > + else if (dev->power.disable_depth > 0 &&
> > + dev->power.runtime_status == RPM_ACTIVE)
>
> IIRC there was a good reason why the original code checked for
> disable_depth == 1 rather than > 0. But I don't remember exactly what
> the reason was. Maybe it had something to do with the fact that during
> a system sleep __device_suspend_late calls __pm_runtime_disable, and the
> code was checking that there were no other disables in effect.
The check was introduced in the below commit:
Commit 6f3c77b040fc
Author: Kevin Hilman <[email protected]>
Date: Fri Sep 21 22:47:34 2012 +0000
PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2
By reading the commit message it's pretty clear to me that the check
was added to cover only one specific use case, during system suspend.
That is, that a driver may want to call pm_runtime_get_sync() from a
late/noirq callback (when the PM core has disabled runtime PM), to
understand whether the device is still powered on and accessible.
> This is
> related to the documented behavior of rpm_resume (it's supposed to fail
> with -EACCES if the device is disabled for runtime PM, no matter what
> power state the device is in).
>
> That probably is also the explanation for why dev->power.is_suspended
> gets checked: It's how the code tells whether a system sleep is in
> progress.
Yes, you are certainly correct about the current behaviour. It's there
for a reason.
On the other hand I would be greatly surprised if this change would
cause any issues. Of course, I can't make guarantees, but I am, of
course, willing to help to fix problems if those happen.
As a matter of fact, I think the current behaviour looks quite
inconsistent, as it depends on whether the device is being system
suspended.
Moreover, for syscore devices (dev->power.syscore is set for them),
the PM core doesn't set the "is_suspended" flag. Those can benefit
from a common behaviour.
Finally, I think the "is_suspended" flag actually needs to be
protected by a lock when set by the PM core, as it's being used in two
separate execution paths. Although, rather than adding a lock for
protection, we can just rely on the "disable_depth" in rpm_resume().
It would be easier and makes the behaviour consistent too.
>
> So overall, I suspect this change should not be made. But some other
> improvement (like a nice comment) might be in order.
>
> Alan Stern
Thanks for reviewing!
Kind regards
Uffe
On Wed, Oct 27, 2021 at 12:55:43PM +0200, Ulf Hansson wrote:
> On Wed, 27 Oct 2021 at 04:02, Alan Stern <[email protected]> wrote:
> >
> > On Wed, Oct 27, 2021 at 12:26:26AM +0200, Ulf Hansson wrote:
> > > During system suspend, the PM core sets dev->power.is_suspended for the
> > > device that is being suspended. This flag is also being used in
> > > rpm_resume(), to allow it to succeed by returning 1, assuming that runtime
> > > PM has been disabled and the runtime PM status is RPM_ACTIVE, for the
> > > device.
> > >
> > > To make this behaviour a bit more useful, let's drop the check for the
> > > dev->power.is_suspended flag in rpm_resume(), as it doesn't really need to
> > > be limited to this anyway.
> > >
> > > Signed-off-by: Ulf Hansson <[email protected]>
> > > ---
> > > drivers/base/power/runtime.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > index ec94049442b9..fadc278e3a66 100644
> > > --- a/drivers/base/power/runtime.c
> > > +++ b/drivers/base/power/runtime.c
> > > @@ -742,8 +742,8 @@ static int rpm_resume(struct device *dev, int rpmflags)
> > > repeat:
> > > if (dev->power.runtime_error)
> > > retval = -EINVAL;
> > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> > > - && dev->power.runtime_status == RPM_ACTIVE)
> > > + else if (dev->power.disable_depth > 0 &&
> > > + dev->power.runtime_status == RPM_ACTIVE)
> >
> > IIRC there was a good reason why the original code checked for
> > disable_depth == 1 rather than > 0. But I don't remember exactly what
> > the reason was. Maybe it had something to do with the fact that during
> > a system sleep __device_suspend_late calls __pm_runtime_disable, and the
> > code was checking that there were no other disables in effect.
>
> The check was introduced in the below commit:
>
> Commit 6f3c77b040fc
> Author: Kevin Hilman <[email protected]>
> Date: Fri Sep 21 22:47:34 2012 +0000
> PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2
>
> By reading the commit message it's pretty clear to me that the check
> was added to cover only one specific use case, during system suspend.
>
> That is, that a driver may want to call pm_runtime_get_sync() from a
> late/noirq callback (when the PM core has disabled runtime PM), to
> understand whether the device is still powered on and accessible.
>
> > This is
> > related to the documented behavior of rpm_resume (it's supposed to fail
> > with -EACCES if the device is disabled for runtime PM, no matter what
> > power state the device is in).
> >
> > That probably is also the explanation for why dev->power.is_suspended
> > gets checked: It's how the code tells whether a system sleep is in
> > progress.
>
> Yes, you are certainly correct about the current behaviour. It's there
> for a reason.
>
> On the other hand I would be greatly surprised if this change would
> cause any issues. Of course, I can't make guarantees, but I am, of
> course, willing to help to fix problems if those happen.
>
> As a matter of fact, I think the current behaviour looks quite
> inconsistent, as it depends on whether the device is being system
> suspended.
>
> Moreover, for syscore devices (dev->power.syscore is set for them),
> the PM core doesn't set the "is_suspended" flag. Those can benefit
> from a common behaviour.
>
> Finally, I think the "is_suspended" flag actually needs to be
> protected by a lock when set by the PM core, as it's being used in two
> separate execution paths. Although, rather than adding a lock for
> protection, we can just rely on the "disable_depth" in rpm_resume().
> It would be easier and makes the behaviour consistent too.
As long as is_suspended isn't _written_ in two separate execution paths,
we're probably okay without a lock -- provided the code doesn't mind
getting an indefinite result when a read races with a write.
> > So overall, I suspect this change should not be made. But some other
> > improvement (like a nice comment) might be in order.
> >
> > Alan Stern
>
> Thanks for reviewing!
You're welcome. Whatever you eventually decide to do should be okay
with me. I just wanted to make sure that you understood the deeper
issue here and had given it some thought. For example, it may turn out
that you can resolve matters simply by updating the documentation.
Alan Stern
On Wed, 27 Oct 2021 at 16:33, Alan Stern <[email protected]> wrote:
>
> On Wed, Oct 27, 2021 at 12:55:43PM +0200, Ulf Hansson wrote:
> > On Wed, 27 Oct 2021 at 04:02, Alan Stern <[email protected]> wrote:
> > >
> > > On Wed, Oct 27, 2021 at 12:26:26AM +0200, Ulf Hansson wrote:
> > > > During system suspend, the PM core sets dev->power.is_suspended for the
> > > > device that is being suspended. This flag is also being used in
> > > > rpm_resume(), to allow it to succeed by returning 1, assuming that runtime
> > > > PM has been disabled and the runtime PM status is RPM_ACTIVE, for the
> > > > device.
> > > >
> > > > To make this behaviour a bit more useful, let's drop the check for the
> > > > dev->power.is_suspended flag in rpm_resume(), as it doesn't really need to
> > > > be limited to this anyway.
> > > >
> > > > Signed-off-by: Ulf Hansson <[email protected]>
> > > > ---
> > > > drivers/base/power/runtime.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > index ec94049442b9..fadc278e3a66 100644
> > > > --- a/drivers/base/power/runtime.c
> > > > +++ b/drivers/base/power/runtime.c
> > > > @@ -742,8 +742,8 @@ static int rpm_resume(struct device *dev, int rpmflags)
> > > > repeat:
> > > > if (dev->power.runtime_error)
> > > > retval = -EINVAL;
> > > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> > > > - && dev->power.runtime_status == RPM_ACTIVE)
> > > > + else if (dev->power.disable_depth > 0 &&
> > > > + dev->power.runtime_status == RPM_ACTIVE)
> > >
> > > IIRC there was a good reason why the original code checked for
> > > disable_depth == 1 rather than > 0. But I don't remember exactly what
> > > the reason was. Maybe it had something to do with the fact that during
> > > a system sleep __device_suspend_late calls __pm_runtime_disable, and the
> > > code was checking that there were no other disables in effect.
> >
> > The check was introduced in the below commit:
> >
> > Commit 6f3c77b040fc
> > Author: Kevin Hilman <[email protected]>
> > Date: Fri Sep 21 22:47:34 2012 +0000
> > PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2
> >
> > By reading the commit message it's pretty clear to me that the check
> > was added to cover only one specific use case, during system suspend.
> >
> > That is, that a driver may want to call pm_runtime_get_sync() from a
> > late/noirq callback (when the PM core has disabled runtime PM), to
> > understand whether the device is still powered on and accessible.
> >
> > > This is
> > > related to the documented behavior of rpm_resume (it's supposed to fail
> > > with -EACCES if the device is disabled for runtime PM, no matter what
> > > power state the device is in).
> > >
> > > That probably is also the explanation for why dev->power.is_suspended
> > > gets checked: It's how the code tells whether a system sleep is in
> > > progress.
> >
> > Yes, you are certainly correct about the current behaviour. It's there
> > for a reason.
> >
> > On the other hand I would be greatly surprised if this change would
> > cause any issues. Of course, I can't make guarantees, but I am, of
> > course, willing to help to fix problems if those happen.
> >
> > As a matter of fact, I think the current behaviour looks quite
> > inconsistent, as it depends on whether the device is being system
> > suspended.
> >
> > Moreover, for syscore devices (dev->power.syscore is set for them),
> > the PM core doesn't set the "is_suspended" flag. Those can benefit
> > from a common behaviour.
> >
> > Finally, I think the "is_suspended" flag actually needs to be
> > protected by a lock when set by the PM core, as it's being used in two
> > separate execution paths. Although, rather than adding a lock for
> > protection, we can just rely on the "disable_depth" in rpm_resume().
> > It would be easier and makes the behaviour consistent too.
>
> As long as is_suspended isn't _written_ in two separate execution paths,
> we're probably okay without a lock -- provided the code doesn't mind
> getting an indefinite result when a read races with a write.
Well, indefinite doesn't sound very good to me for these cases, even
if it most likely never will happen.
>
> > > So overall, I suspect this change should not be made. But some other
> > > improvement (like a nice comment) might be in order.
> > >
> > > Alan Stern
> >
> > Thanks for reviewing!
>
> You're welcome. Whatever you eventually decide to do should be okay
> with me. I just wanted to make sure that you understood the deeper
> issue here and had given it some thought. For example, it may turn out
> that you can resolve matters simply by updating the documentation.
I observed the issue on cpuidle-psci. The devices it operates upon are
assigned as syscore devices and these are hooked up to a genpd.
A call to pm_runtime_get_sync() can happen even after the PM core has
disabled runtime PM in the "late" phase. So the error code is received
for these real use-cases.
Now, as we currently don't check the return value of
pm_runtime_get_sync() in cpuidle-psci, it's not a big deal. But it
certainly seems worth fixing in my opinion.
Let's see if Rafael has some thoughts around this.
Again, thanks for your input!
Kind regards
Uffe
On Fri, Oct 29, 2021 at 12:20 AM Ulf Hansson <[email protected]> wrote:
>
> On Wed, 27 Oct 2021 at 16:33, Alan Stern <[email protected]> wrote:
> >
> > On Wed, Oct 27, 2021 at 12:55:43PM +0200, Ulf Hansson wrote:
> > > On Wed, 27 Oct 2021 at 04:02, Alan Stern <[email protected]> wrote:
> > > >
> > > > On Wed, Oct 27, 2021 at 12:26:26AM +0200, Ulf Hansson wrote:
> > > > > During system suspend, the PM core sets dev->power.is_suspended for the
> > > > > device that is being suspended. This flag is also being used in
> > > > > rpm_resume(), to allow it to succeed by returning 1, assuming that runtime
> > > > > PM has been disabled and the runtime PM status is RPM_ACTIVE, for the
> > > > > device.
> > > > >
> > > > > To make this behaviour a bit more useful, let's drop the check for the
> > > > > dev->power.is_suspended flag in rpm_resume(), as it doesn't really need to
> > > > > be limited to this anyway.
> > > > >
> > > > > Signed-off-by: Ulf Hansson <[email protected]>
> > > > > ---
> > > > > drivers/base/power/runtime.c | 4 ++--
> > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > > index ec94049442b9..fadc278e3a66 100644
> > > > > --- a/drivers/base/power/runtime.c
> > > > > +++ b/drivers/base/power/runtime.c
> > > > > @@ -742,8 +742,8 @@ static int rpm_resume(struct device *dev, int rpmflags)
> > > > > repeat:
> > > > > if (dev->power.runtime_error)
> > > > > retval = -EINVAL;
> > > > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> > > > > - && dev->power.runtime_status == RPM_ACTIVE)
> > > > > + else if (dev->power.disable_depth > 0 &&
> > > > > + dev->power.runtime_status == RPM_ACTIVE)
> > > >
> > > > IIRC there was a good reason why the original code checked for
> > > > disable_depth == 1 rather than > 0. But I don't remember exactly what
> > > > the reason was. Maybe it had something to do with the fact that during
> > > > a system sleep __device_suspend_late calls __pm_runtime_disable, and the
> > > > code was checking that there were no other disables in effect.
> > >
> > > The check was introduced in the below commit:
> > >
> > > Commit 6f3c77b040fc
> > > Author: Kevin Hilman <[email protected]>
> > > Date: Fri Sep 21 22:47:34 2012 +0000
> > > PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2
> > >
> > > By reading the commit message it's pretty clear to me that the check
> > > was added to cover only one specific use case, during system suspend.
> > >
> > > That is, that a driver may want to call pm_runtime_get_sync() from a
> > > late/noirq callback (when the PM core has disabled runtime PM), to
> > > understand whether the device is still powered on and accessible.
> > >
> > > > This is
> > > > related to the documented behavior of rpm_resume (it's supposed to fail
> > > > with -EACCES if the device is disabled for runtime PM, no matter what
> > > > power state the device is in).
> > > >
> > > > That probably is also the explanation for why dev->power.is_suspended
> > > > gets checked: It's how the code tells whether a system sleep is in
> > > > progress.
> > >
> > > Yes, you are certainly correct about the current behaviour. It's there
> > > for a reason.
> > >
> > > On the other hand I would be greatly surprised if this change would
> > > cause any issues. Of course, I can't make guarantees, but I am, of
> > > course, willing to help to fix problems if those happen.
> > >
> > > As a matter of fact, I think the current behaviour looks quite
> > > inconsistent, as it depends on whether the device is being system
> > > suspended.
> > >
> > > Moreover, for syscore devices (dev->power.syscore is set for them),
> > > the PM core doesn't set the "is_suspended" flag. Those can benefit
> > > from a common behaviour.
> > >
> > > Finally, I think the "is_suspended" flag actually needs to be
> > > protected by a lock when set by the PM core, as it's being used in two
> > > separate execution paths. Although, rather than adding a lock for
> > > protection, we can just rely on the "disable_depth" in rpm_resume().
> > > It would be easier and makes the behaviour consistent too.
> >
> > As long as is_suspended isn't _written_ in two separate execution paths,
> > we're probably okay without a lock -- provided the code doesn't mind
> > getting an indefinite result when a read races with a write.
>
> Well, indefinite doesn't sound very good to me for these cases, even
> if it most likely never will happen.
>
> >
> > > > So overall, I suspect this change should not be made. But some other
> > > > improvement (like a nice comment) might be in order.
> > > >
> > > > Alan Stern
> > >
> > > Thanks for reviewing!
> >
> > You're welcome. Whatever you eventually decide to do should be okay
> > with me. I just wanted to make sure that you understood the deeper
> > issue here and had given it some thought. For example, it may turn out
> > that you can resolve matters simply by updating the documentation.
>
> I observed the issue on cpuidle-psci. The devices it operates upon are
> assigned as syscore devices and these are hooked up to a genpd.
>
> A call to pm_runtime_get_sync() can happen even after the PM core has
> disabled runtime PM in the "late" phase. So the error code is received
> for these real use-cases.
>
> Now, as we currently don't check the return value of
> pm_runtime_get_sync() in cpuidle-psci, it's not a big deal. But it
> certainly seems worth fixing in my opinion.
>
> Let's see if Rafael has some thoughts around this.
Am I thinking correctly that this is mostly about working around the
limitations of pm_runtime_force_suspend()?
On Fri, 29 Oct 2021 at 20:27, Rafael J. Wysocki <[email protected]> wrote:
>
> On Fri, Oct 29, 2021 at 12:20 AM Ulf Hansson <[email protected]> wrote:
> >
> > On Wed, 27 Oct 2021 at 16:33, Alan Stern <[email protected]> wrote:
> > >
> > > On Wed, Oct 27, 2021 at 12:55:43PM +0200, Ulf Hansson wrote:
> > > > On Wed, 27 Oct 2021 at 04:02, Alan Stern <[email protected]> wrote:
> > > > >
> > > > > On Wed, Oct 27, 2021 at 12:26:26AM +0200, Ulf Hansson wrote:
> > > > > > During system suspend, the PM core sets dev->power.is_suspended for the
> > > > > > device that is being suspended. This flag is also being used in
> > > > > > rpm_resume(), to allow it to succeed by returning 1, assuming that runtime
> > > > > > PM has been disabled and the runtime PM status is RPM_ACTIVE, for the
> > > > > > device.
> > > > > >
> > > > > > To make this behaviour a bit more useful, let's drop the check for the
> > > > > > dev->power.is_suspended flag in rpm_resume(), as it doesn't really need to
> > > > > > be limited to this anyway.
> > > > > >
> > > > > > Signed-off-by: Ulf Hansson <[email protected]>
> > > > > > ---
> > > > > > drivers/base/power/runtime.c | 4 ++--
> > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > > > index ec94049442b9..fadc278e3a66 100644
> > > > > > --- a/drivers/base/power/runtime.c
> > > > > > +++ b/drivers/base/power/runtime.c
> > > > > > @@ -742,8 +742,8 @@ static int rpm_resume(struct device *dev, int rpmflags)
> > > > > > repeat:
> > > > > > if (dev->power.runtime_error)
> > > > > > retval = -EINVAL;
> > > > > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> > > > > > - && dev->power.runtime_status == RPM_ACTIVE)
> > > > > > + else if (dev->power.disable_depth > 0 &&
> > > > > > + dev->power.runtime_status == RPM_ACTIVE)
> > > > >
> > > > > IIRC there was a good reason why the original code checked for
> > > > > disable_depth == 1 rather than > 0. But I don't remember exactly what
> > > > > the reason was. Maybe it had something to do with the fact that during
> > > > > a system sleep __device_suspend_late calls __pm_runtime_disable, and the
> > > > > code was checking that there were no other disables in effect.
> > > >
> > > > The check was introduced in the below commit:
> > > >
> > > > Commit 6f3c77b040fc
> > > > Author: Kevin Hilman <[email protected]>
> > > > Date: Fri Sep 21 22:47:34 2012 +0000
> > > > PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2
> > > >
> > > > By reading the commit message it's pretty clear to me that the check
> > > > was added to cover only one specific use case, during system suspend.
> > > >
> > > > That is, that a driver may want to call pm_runtime_get_sync() from a
> > > > late/noirq callback (when the PM core has disabled runtime PM), to
> > > > understand whether the device is still powered on and accessible.
> > > >
> > > > > This is
> > > > > related to the documented behavior of rpm_resume (it's supposed to fail
> > > > > with -EACCES if the device is disabled for runtime PM, no matter what
> > > > > power state the device is in).
> > > > >
> > > > > That probably is also the explanation for why dev->power.is_suspended
> > > > > gets checked: It's how the code tells whether a system sleep is in
> > > > > progress.
> > > >
> > > > Yes, you are certainly correct about the current behaviour. It's there
> > > > for a reason.
> > > >
> > > > On the other hand I would be greatly surprised if this change would
> > > > cause any issues. Of course, I can't make guarantees, but I am, of
> > > > course, willing to help to fix problems if those happen.
> > > >
> > > > As a matter of fact, I think the current behaviour looks quite
> > > > inconsistent, as it depends on whether the device is being system
> > > > suspended.
> > > >
> > > > Moreover, for syscore devices (dev->power.syscore is set for them),
> > > > the PM core doesn't set the "is_suspended" flag. Those can benefit
> > > > from a common behaviour.
> > > >
> > > > Finally, I think the "is_suspended" flag actually needs to be
> > > > protected by a lock when set by the PM core, as it's being used in two
> > > > separate execution paths. Although, rather than adding a lock for
> > > > protection, we can just rely on the "disable_depth" in rpm_resume().
> > > > It would be easier and makes the behaviour consistent too.
> > >
> > > As long as is_suspended isn't _written_ in two separate execution paths,
> > > we're probably okay without a lock -- provided the code doesn't mind
> > > getting an indefinite result when a read races with a write.
> >
> > Well, indefinite doesn't sound very good to me for these cases, even
> > if it most likely never will happen.
> >
> > >
> > > > > So overall, I suspect this change should not be made. But some other
> > > > > improvement (like a nice comment) might be in order.
> > > > >
> > > > > Alan Stern
> > > >
> > > > Thanks for reviewing!
> > >
> > > You're welcome. Whatever you eventually decide to do should be okay
> > > with me. I just wanted to make sure that you understood the deeper
> > > issue here and had given it some thought. For example, it may turn out
> > > that you can resolve matters simply by updating the documentation.
> >
> > I observed the issue on cpuidle-psci. The devices it operates upon are
> > assigned as syscore devices and these are hooked up to a genpd.
> >
> > A call to pm_runtime_get_sync() can happen even after the PM core has
> > disabled runtime PM in the "late" phase. So the error code is received
> > for these real use-cases.
> >
> > Now, as we currently don't check the return value of
> > pm_runtime_get_sync() in cpuidle-psci, it's not a big deal. But it
> > certainly seems worth fixing in my opinion.
> >
> > Let's see if Rafael has some thoughts around this.
>
> Am I thinking correctly that this is mostly about working around the
> limitations of pm_runtime_force_suspend()?
No, this isn't related at all.
The cpuidle-psci driver doesn't have PM callbacks, thus using
pm_runtime_force_suspend() would not work here.
Kind regards
Uffe
On 01/11/2021 11:27, Ulf Hansson wrote:
> On Fri, 29 Oct 2021 at 20:27, Rafael J. Wysocki <[email protected]> wrote:
>>
>> On Fri, Oct 29, 2021 at 12:20 AM Ulf Hansson <[email protected]> wrote:
>>>
>>> On Wed, 27 Oct 2021 at 16:33, Alan Stern <[email protected]> wrote:
>>>>
>>>> On Wed, Oct 27, 2021 at 12:55:43PM +0200, Ulf Hansson wrote:
>>>>> On Wed, 27 Oct 2021 at 04:02, Alan Stern <[email protected]> wrote:
>>>>>>
>>>>>> On Wed, Oct 27, 2021 at 12:26:26AM +0200, Ulf Hansson wrote:
>>>>>>> During system suspend, the PM core sets dev->power.is_suspended for the
>>>>>>> device that is being suspended. This flag is also being used in
>>>>>>> rpm_resume(), to allow it to succeed by returning 1, assuming that runtime
>>>>>>> PM has been disabled and the runtime PM status is RPM_ACTIVE, for the
>>>>>>> device.
>>>>>>>
>>>>>>> To make this behaviour a bit more useful, let's drop the check for the
>>>>>>> dev->power.is_suspended flag in rpm_resume(), as it doesn't really need to
>>>>>>> be limited to this anyway.
>>>>>>>
>>>>>>> Signed-off-by: Ulf Hansson <[email protected]>
>>>>>>> ---
>>>>>>> drivers/base/power/runtime.c | 4 ++--
>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>>>>>>> index ec94049442b9..fadc278e3a66 100644
>>>>>>> --- a/drivers/base/power/runtime.c
>>>>>>> +++ b/drivers/base/power/runtime.c
>>>>>>> @@ -742,8 +742,8 @@ static int rpm_resume(struct device *dev, int rpmflags)
>>>>>>> repeat:
>>>>>>> if (dev->power.runtime_error)
>>>>>>> retval = -EINVAL;
>>>>>>> - else if (dev->power.disable_depth == 1 && dev->power.is_suspended
>>>>>>> - && dev->power.runtime_status == RPM_ACTIVE)
>>>>>>> + else if (dev->power.disable_depth > 0 &&
>>>>>>> + dev->power.runtime_status == RPM_ACTIVE)
>>>>>>
>>>>>> IIRC there was a good reason why the original code checked for
>>>>>> disable_depth == 1 rather than > 0. But I don't remember exactly what
>>>>>> the reason was. Maybe it had something to do with the fact that during
>>>>>> a system sleep __device_suspend_late calls __pm_runtime_disable, and the
>>>>>> code was checking that there were no other disables in effect.
>>>>>
>>>>> The check was introduced in the below commit:
>>>>>
>>>>> Commit 6f3c77b040fc
>>>>> Author: Kevin Hilman <[email protected]>
>>>>> Date: Fri Sep 21 22:47:34 2012 +0000
>>>>> PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2
>>>>>
>>>>> By reading the commit message it's pretty clear to me that the check
>>>>> was added to cover only one specific use case, during system suspend.
>>>>>
>>>>> That is, that a driver may want to call pm_runtime_get_sync() from a
>>>>> late/noirq callback (when the PM core has disabled runtime PM), to
>>>>> understand whether the device is still powered on and accessible.
>>>>>
>>>>>> This is
>>>>>> related to the documented behavior of rpm_resume (it's supposed to fail
>>>>>> with -EACCES if the device is disabled for runtime PM, no matter what
>>>>>> power state the device is in).
>>>>>>
>>>>>> That probably is also the explanation for why dev->power.is_suspended
>>>>>> gets checked: It's how the code tells whether a system sleep is in
>>>>>> progress.
>>>>>
>>>>> Yes, you are certainly correct about the current behaviour. It's there
>>>>> for a reason.
>>>>>
>>>>> On the other hand I would be greatly surprised if this change would
>>>>> cause any issues. Of course, I can't make guarantees, but I am, of
>>>>> course, willing to help to fix problems if those happen.
>>>>>
>>>>> As a matter of fact, I think the current behaviour looks quite
>>>>> inconsistent, as it depends on whether the device is being system
>>>>> suspended.
>>>>>
>>>>> Moreover, for syscore devices (dev->power.syscore is set for them),
>>>>> the PM core doesn't set the "is_suspended" flag. Those can benefit
>>>>> from a common behaviour.
>>>>>
>>>>> Finally, I think the "is_suspended" flag actually needs to be
>>>>> protected by a lock when set by the PM core, as it's being used in two
>>>>> separate execution paths. Although, rather than adding a lock for
>>>>> protection, we can just rely on the "disable_depth" in rpm_resume().
>>>>> It would be easier and makes the behaviour consistent too.
>>>>
>>>> As long as is_suspended isn't _written_ in two separate execution paths,
>>>> we're probably okay without a lock -- provided the code doesn't mind
>>>> getting an indefinite result when a read races with a write.
>>>
>>> Well, indefinite doesn't sound very good to me for these cases, even
>>> if it most likely never will happen.
>>>
>>>>
>>>>>> So overall, I suspect this change should not be made. But some other
>>>>>> improvement (like a nice comment) might be in order.
>>>>>>
>>>>>> Alan Stern
>>>>>
>>>>> Thanks for reviewing!
>>>>
>>>> You're welcome. Whatever you eventually decide to do should be okay
>>>> with me. I just wanted to make sure that you understood the deeper
>>>> issue here and had given it some thought. For example, it may turn out
>>>> that you can resolve matters simply by updating the documentation.
>>>
>>> I observed the issue on cpuidle-psci. The devices it operates upon are
>>> assigned as syscore devices and these are hooked up to a genpd.
>>>
>>> A call to pm_runtime_get_sync() can happen even after the PM core has
>>> disabled runtime PM in the "late" phase. So the error code is received
>>> for these real use-cases.
>>>
>>> Now, as we currently don't check the return value of
>>> pm_runtime_get_sync() in cpuidle-psci, it's not a big deal. But it
>>> certainly seems worth fixing in my opinion.
>>>
>>> Let's see if Rafael has some thoughts around this.
>>
>> Am I thinking correctly that this is mostly about working around the
>> limitations of pm_runtime_force_suspend()?
>
> No, this isn't related at all.
>
> The cpuidle-psci driver doesn't have PM callbacks, thus using
> pm_runtime_force_suspend() would not work here.
>
i think reason for (dev->power.disable_depth == 1 && dev->power.is_suspended)
can be found in [1], as other related comments:
Rafael J. Wysocki:
>>>
I've discussed that with Kevin. The problem is that the runtime PM
status may be changed at will when runtime PM is disabled by using
__pm_runtime_set_status(), so the status generally cannod be trusted
if power.disable_depth > 0.
During system suspend, however, runtime PM is disabled by the core and
if neither the driver nor the subsystem has disabled it in the meantime,
the status should be actually valid.
<<<
Hence, this is about using PM runtime for CPU PM and, CPU PM is pretty specific case,
wouldn't manual check for CPU PM status work for you, like !pm_runtime_status_suspended()?
(if i'm not mistaken - CPU PM done in atomic context).
[1] http://lkml.iu.edu/hypermail/linux/kernel/1209.2/03256.html
--
Best regards,
grygorii
On Mon, 1 Nov 2021 at 15:41, Grygorii Strashko <[email protected]> wrote:
>
>
>
> On 01/11/2021 11:27, Ulf Hansson wrote:
> > On Fri, 29 Oct 2021 at 20:27, Rafael J. Wysocki <[email protected]> wrote:
> >>
> >> On Fri, Oct 29, 2021 at 12:20 AM Ulf Hansson <[email protected]> wrote:
> >>>
> >>> On Wed, 27 Oct 2021 at 16:33, Alan Stern <[email protected]> wrote:
> >>>>
> >>>> On Wed, Oct 27, 2021 at 12:55:43PM +0200, Ulf Hansson wrote:
> >>>>> On Wed, 27 Oct 2021 at 04:02, Alan Stern <[email protected]> wrote:
> >>>>>>
> >>>>>> On Wed, Oct 27, 2021 at 12:26:26AM +0200, Ulf Hansson wrote:
> >>>>>>> During system suspend, the PM core sets dev->power.is_suspended for the
> >>>>>>> device that is being suspended. This flag is also being used in
> >>>>>>> rpm_resume(), to allow it to succeed by returning 1, assuming that runtime
> >>>>>>> PM has been disabled and the runtime PM status is RPM_ACTIVE, for the
> >>>>>>> device.
> >>>>>>>
> >>>>>>> To make this behaviour a bit more useful, let's drop the check for the
> >>>>>>> dev->power.is_suspended flag in rpm_resume(), as it doesn't really need to
> >>>>>>> be limited to this anyway.
> >>>>>>>
> >>>>>>> Signed-off-by: Ulf Hansson <[email protected]>
> >>>>>>> ---
> >>>>>>> drivers/base/power/runtime.c | 4 ++--
> >>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> >>>>>>> index ec94049442b9..fadc278e3a66 100644
> >>>>>>> --- a/drivers/base/power/runtime.c
> >>>>>>> +++ b/drivers/base/power/runtime.c
> >>>>>>> @@ -742,8 +742,8 @@ static int rpm_resume(struct device *dev, int rpmflags)
> >>>>>>> repeat:
> >>>>>>> if (dev->power.runtime_error)
> >>>>>>> retval = -EINVAL;
> >>>>>>> - else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> >>>>>>> - && dev->power.runtime_status == RPM_ACTIVE)
> >>>>>>> + else if (dev->power.disable_depth > 0 &&
> >>>>>>> + dev->power.runtime_status == RPM_ACTIVE)
> >>>>>>
> >>>>>> IIRC there was a good reason why the original code checked for
> >>>>>> disable_depth == 1 rather than > 0. But I don't remember exactly what
> >>>>>> the reason was. Maybe it had something to do with the fact that during
> >>>>>> a system sleep __device_suspend_late calls __pm_runtime_disable, and the
> >>>>>> code was checking that there were no other disables in effect.
> >>>>>
> >>>>> The check was introduced in the below commit:
> >>>>>
> >>>>> Commit 6f3c77b040fc
> >>>>> Author: Kevin Hilman <[email protected]>
> >>>>> Date: Fri Sep 21 22:47:34 2012 +0000
> >>>>> PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2
> >>>>>
> >>>>> By reading the commit message it's pretty clear to me that the check
> >>>>> was added to cover only one specific use case, during system suspend.
> >>>>>
> >>>>> That is, that a driver may want to call pm_runtime_get_sync() from a
> >>>>> late/noirq callback (when the PM core has disabled runtime PM), to
> >>>>> understand whether the device is still powered on and accessible.
> >>>>>
> >>>>>> This is
> >>>>>> related to the documented behavior of rpm_resume (it's supposed to fail
> >>>>>> with -EACCES if the device is disabled for runtime PM, no matter what
> >>>>>> power state the device is in).
> >>>>>>
> >>>>>> That probably is also the explanation for why dev->power.is_suspended
> >>>>>> gets checked: It's how the code tells whether a system sleep is in
> >>>>>> progress.
> >>>>>
> >>>>> Yes, you are certainly correct about the current behaviour. It's there
> >>>>> for a reason.
> >>>>>
> >>>>> On the other hand I would be greatly surprised if this change would
> >>>>> cause any issues. Of course, I can't make guarantees, but I am, of
> >>>>> course, willing to help to fix problems if those happen.
> >>>>>
> >>>>> As a matter of fact, I think the current behaviour looks quite
> >>>>> inconsistent, as it depends on whether the device is being system
> >>>>> suspended.
> >>>>>
> >>>>> Moreover, for syscore devices (dev->power.syscore is set for them),
> >>>>> the PM core doesn't set the "is_suspended" flag. Those can benefit
> >>>>> from a common behaviour.
> >>>>>
> >>>>> Finally, I think the "is_suspended" flag actually needs to be
> >>>>> protected by a lock when set by the PM core, as it's being used in two
> >>>>> separate execution paths. Although, rather than adding a lock for
> >>>>> protection, we can just rely on the "disable_depth" in rpm_resume().
> >>>>> It would be easier and makes the behaviour consistent too.
> >>>>
> >>>> As long as is_suspended isn't _written_ in two separate execution paths,
> >>>> we're probably okay without a lock -- provided the code doesn't mind
> >>>> getting an indefinite result when a read races with a write.
> >>>
> >>> Well, indefinite doesn't sound very good to me for these cases, even
> >>> if it most likely never will happen.
> >>>
> >>>>
> >>>>>> So overall, I suspect this change should not be made. But some other
> >>>>>> improvement (like a nice comment) might be in order.
> >>>>>>
> >>>>>> Alan Stern
> >>>>>
> >>>>> Thanks for reviewing!
> >>>>
> >>>> You're welcome. Whatever you eventually decide to do should be okay
> >>>> with me. I just wanted to make sure that you understood the deeper
> >>>> issue here and had given it some thought. For example, it may turn out
> >>>> that you can resolve matters simply by updating the documentation.
> >>>
> >>> I observed the issue on cpuidle-psci. The devices it operates upon are
> >>> assigned as syscore devices and these are hooked up to a genpd.
> >>>
> >>> A call to pm_runtime_get_sync() can happen even after the PM core has
> >>> disabled runtime PM in the "late" phase. So the error code is received
> >>> for these real use-cases.
> >>>
> >>> Now, as we currently don't check the return value of
> >>> pm_runtime_get_sync() in cpuidle-psci, it's not a big deal. But it
> >>> certainly seems worth fixing in my opinion.
> >>>
> >>> Let's see if Rafael has some thoughts around this.
> >>
> >> Am I thinking correctly that this is mostly about working around the
> >> limitations of pm_runtime_force_suspend()?
> >
> > No, this isn't related at all.
> >
> > The cpuidle-psci driver doesn't have PM callbacks, thus using
> > pm_runtime_force_suspend() would not work here.
> >
>
> i think reason for (dev->power.disable_depth == 1 && dev->power.is_suspended)
> can be found in [1], as other related comments:
>
> Rafael J. Wysocki:
> >>>
> I've discussed that with Kevin. The problem is that the runtime PM
> status may be changed at will when runtime PM is disabled by using
> __pm_runtime_set_status(), so the status generally cannod be trusted
> if power.disable_depth > 0.
>
> During system suspend, however, runtime PM is disabled by the core and
> if neither the driver nor the subsystem has disabled it in the meantime,
> the status should be actually valid.
I don't quite understand this comment from the past, but I guess it's
also kind of difficult without having the complete context.
In any case, if anyone updates the runtime PM status for a device
through __pm_runtime_set_status(), protection from concurrent accesses
is managed by using the spin lock (dev->power.lock).
> <<<
>
> Hence, this is about using PM runtime for CPU PM and, CPU PM is pretty specific case,
> wouldn't manual check for CPU PM status work for you, like !pm_runtime_status_suspended()?
> (if i'm not mistaken - CPU PM done in atomic context).
No, that doesn't work. If I want to call pm_runtime_status_suspended()
to check the runtime PM status, I would first need to disable runtime
PM.
>
>
> [1] http://lkml.iu.edu/hypermail/linux/kernel/1209.2/03256.html
>
> --
> Best regards,
> grygorii
Kind regards
Uffe
On Mon, 1 Nov 2021 at 10:27, Ulf Hansson <[email protected]> wrote:
>
> On Fri, 29 Oct 2021 at 20:27, Rafael J. Wysocki <[email protected]> wrote:
> >
> > On Fri, Oct 29, 2021 at 12:20 AM Ulf Hansson <[email protected]> wrote:
> > >
> > > On Wed, 27 Oct 2021 at 16:33, Alan Stern <[email protected]> wrote:
> > > >
> > > > On Wed, Oct 27, 2021 at 12:55:43PM +0200, Ulf Hansson wrote:
> > > > > On Wed, 27 Oct 2021 at 04:02, Alan Stern <[email protected]> wrote:
> > > > > >
> > > > > > On Wed, Oct 27, 2021 at 12:26:26AM +0200, Ulf Hansson wrote:
> > > > > > > During system suspend, the PM core sets dev->power.is_suspended for the
> > > > > > > device that is being suspended. This flag is also being used in
> > > > > > > rpm_resume(), to allow it to succeed by returning 1, assuming that runtime
> > > > > > > PM has been disabled and the runtime PM status is RPM_ACTIVE, for the
> > > > > > > device.
> > > > > > >
> > > > > > > To make this behaviour a bit more useful, let's drop the check for the
> > > > > > > dev->power.is_suspended flag in rpm_resume(), as it doesn't really need to
> > > > > > > be limited to this anyway.
> > > > > > >
> > > > > > > Signed-off-by: Ulf Hansson <[email protected]>
> > > > > > > ---
> > > > > > > drivers/base/power/runtime.c | 4 ++--
> > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > > > > index ec94049442b9..fadc278e3a66 100644
> > > > > > > --- a/drivers/base/power/runtime.c
> > > > > > > +++ b/drivers/base/power/runtime.c
> > > > > > > @@ -742,8 +742,8 @@ static int rpm_resume(struct device *dev, int rpmflags)
> > > > > > > repeat:
> > > > > > > if (dev->power.runtime_error)
> > > > > > > retval = -EINVAL;
> > > > > > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> > > > > > > - && dev->power.runtime_status == RPM_ACTIVE)
> > > > > > > + else if (dev->power.disable_depth > 0 &&
> > > > > > > + dev->power.runtime_status == RPM_ACTIVE)
> > > > > >
> > > > > > IIRC there was a good reason why the original code checked for
> > > > > > disable_depth == 1 rather than > 0. But I don't remember exactly what
> > > > > > the reason was. Maybe it had something to do with the fact that during
> > > > > > a system sleep __device_suspend_late calls __pm_runtime_disable, and the
> > > > > > code was checking that there were no other disables in effect.
> > > > >
> > > > > The check was introduced in the below commit:
> > > > >
> > > > > Commit 6f3c77b040fc
> > > > > Author: Kevin Hilman <[email protected]>
> > > > > Date: Fri Sep 21 22:47:34 2012 +0000
> > > > > PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2
> > > > >
> > > > > By reading the commit message it's pretty clear to me that the check
> > > > > was added to cover only one specific use case, during system suspend.
> > > > >
> > > > > That is, that a driver may want to call pm_runtime_get_sync() from a
> > > > > late/noirq callback (when the PM core has disabled runtime PM), to
> > > > > understand whether the device is still powered on and accessible.
> > > > >
> > > > > > This is
> > > > > > related to the documented behavior of rpm_resume (it's supposed to fail
> > > > > > with -EACCES if the device is disabled for runtime PM, no matter what
> > > > > > power state the device is in).
> > > > > >
> > > > > > That probably is also the explanation for why dev->power.is_suspended
> > > > > > gets checked: It's how the code tells whether a system sleep is in
> > > > > > progress.
> > > > >
> > > > > Yes, you are certainly correct about the current behaviour. It's there
> > > > > for a reason.
> > > > >
> > > > > On the other hand I would be greatly surprised if this change would
> > > > > cause any issues. Of course, I can't make guarantees, but I am, of
> > > > > course, willing to help to fix problems if those happen.
> > > > >
> > > > > As a matter of fact, I think the current behaviour looks quite
> > > > > inconsistent, as it depends on whether the device is being system
> > > > > suspended.
> > > > >
> > > > > Moreover, for syscore devices (dev->power.syscore is set for them),
> > > > > the PM core doesn't set the "is_suspended" flag. Those can benefit
> > > > > from a common behaviour.
> > > > >
> > > > > Finally, I think the "is_suspended" flag actually needs to be
> > > > > protected by a lock when set by the PM core, as it's being used in two
> > > > > separate execution paths. Although, rather than adding a lock for
> > > > > protection, we can just rely on the "disable_depth" in rpm_resume().
> > > > > It would be easier and makes the behaviour consistent too.
> > > >
> > > > As long as is_suspended isn't _written_ in two separate execution paths,
> > > > we're probably okay without a lock -- provided the code doesn't mind
> > > > getting an indefinite result when a read races with a write.
> > >
> > > Well, indefinite doesn't sound very good to me for these cases, even
> > > if it most likely never will happen.
> > >
> > > >
> > > > > > So overall, I suspect this change should not be made. But some other
> > > > > > improvement (like a nice comment) might be in order.
> > > > > >
> > > > > > Alan Stern
> > > > >
> > > > > Thanks for reviewing!
> > > >
> > > > You're welcome. Whatever you eventually decide to do should be okay
> > > > with me. I just wanted to make sure that you understood the deeper
> > > > issue here and had given it some thought. For example, it may turn out
> > > > that you can resolve matters simply by updating the documentation.
> > >
> > > I observed the issue on cpuidle-psci. The devices it operates upon are
> > > assigned as syscore devices and these are hooked up to a genpd.
> > >
> > > A call to pm_runtime_get_sync() can happen even after the PM core has
> > > disabled runtime PM in the "late" phase. So the error code is received
> > > for these real use-cases.
> > >
> > > Now, as we currently don't check the return value of
> > > pm_runtime_get_sync() in cpuidle-psci, it's not a big deal. But it
> > > certainly seems worth fixing in my opinion.
> > >
> > > Let's see if Rafael has some thoughts around this.
> >
> > Am I thinking correctly that this is mostly about working around the
> > limitations of pm_runtime_force_suspend()?
>
> No, this isn't related at all.
>
> The cpuidle-psci driver doesn't have PM callbacks, thus using
> pm_runtime_force_suspend() would not work here.
Just wanted to send a ping on this to see if we can come to a
conclusion. Or maybe we did? :-)
I think in the end, what slightly bothers me, is that the behavior is
a bit inconsistent. Although, maybe it's the best we can do.
Kind regards
Uffe
On Fri, Nov 26, 2021 at 1:20 PM Ulf Hansson <[email protected]> wrote:
>
> On Mon, 1 Nov 2021 at 10:27, Ulf Hansson <[email protected]> wrote:
> >
> > On Fri, 29 Oct 2021 at 20:27, Rafael J. Wysocki <[email protected]> wrote:
> > >
> > > On Fri, Oct 29, 2021 at 12:20 AM Ulf Hansson <[email protected]> wrote:
> > > >
> > > > On Wed, 27 Oct 2021 at 16:33, Alan Stern <[email protected]> wrote:
> > > > >
> > > > > On Wed, Oct 27, 2021 at 12:55:43PM +0200, Ulf Hansson wrote:
> > > > > > On Wed, 27 Oct 2021 at 04:02, Alan Stern <[email protected]> wrote:
> > > > > > >
> > > > > > > On Wed, Oct 27, 2021 at 12:26:26AM +0200, Ulf Hansson wrote:
> > > > > > > > During system suspend, the PM core sets dev->power.is_suspended for the
> > > > > > > > device that is being suspended. This flag is also being used in
> > > > > > > > rpm_resume(), to allow it to succeed by returning 1, assuming that runtime
> > > > > > > > PM has been disabled and the runtime PM status is RPM_ACTIVE, for the
> > > > > > > > device.
> > > > > > > >
> > > > > > > > To make this behaviour a bit more useful, let's drop the check for the
> > > > > > > > dev->power.is_suspended flag in rpm_resume(), as it doesn't really need to
> > > > > > > > be limited to this anyway.
> > > > > > > >
> > > > > > > > Signed-off-by: Ulf Hansson <[email protected]>
> > > > > > > > ---
> > > > > > > > drivers/base/power/runtime.c | 4 ++--
> > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > > > > > index ec94049442b9..fadc278e3a66 100644
> > > > > > > > --- a/drivers/base/power/runtime.c
> > > > > > > > +++ b/drivers/base/power/runtime.c
> > > > > > > > @@ -742,8 +742,8 @@ static int rpm_resume(struct device *dev, int rpmflags)
> > > > > > > > repeat:
> > > > > > > > if (dev->power.runtime_error)
> > > > > > > > retval = -EINVAL;
> > > > > > > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> > > > > > > > - && dev->power.runtime_status == RPM_ACTIVE)
> > > > > > > > + else if (dev->power.disable_depth > 0 &&
> > > > > > > > + dev->power.runtime_status == RPM_ACTIVE)
> > > > > > >
> > > > > > > IIRC there was a good reason why the original code checked for
> > > > > > > disable_depth == 1 rather than > 0. But I don't remember exactly what
> > > > > > > the reason was. Maybe it had something to do with the fact that during
> > > > > > > a system sleep __device_suspend_late calls __pm_runtime_disable, and the
> > > > > > > code was checking that there were no other disables in effect.
> > > > > >
> > > > > > The check was introduced in the below commit:
> > > > > >
> > > > > > Commit 6f3c77b040fc
> > > > > > Author: Kevin Hilman <[email protected]>
> > > > > > Date: Fri Sep 21 22:47:34 2012 +0000
> > > > > > PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2
> > > > > >
> > > > > > By reading the commit message it's pretty clear to me that the check
> > > > > > was added to cover only one specific use case, during system suspend.
> > > > > >
> > > > > > That is, that a driver may want to call pm_runtime_get_sync() from a
> > > > > > late/noirq callback (when the PM core has disabled runtime PM), to
> > > > > > understand whether the device is still powered on and accessible.
> > > > > >
> > > > > > > This is
> > > > > > > related to the documented behavior of rpm_resume (it's supposed to fail
> > > > > > > with -EACCES if the device is disabled for runtime PM, no matter what
> > > > > > > power state the device is in).
> > > > > > >
> > > > > > > That probably is also the explanation for why dev->power.is_suspended
> > > > > > > gets checked: It's how the code tells whether a system sleep is in
> > > > > > > progress.
> > > > > >
> > > > > > Yes, you are certainly correct about the current behaviour. It's there
> > > > > > for a reason.
> > > > > >
> > > > > > On the other hand I would be greatly surprised if this change would
> > > > > > cause any issues. Of course, I can't make guarantees, but I am, of
> > > > > > course, willing to help to fix problems if those happen.
> > > > > >
> > > > > > As a matter of fact, I think the current behaviour looks quite
> > > > > > inconsistent, as it depends on whether the device is being system
> > > > > > suspended.
> > > > > >
> > > > > > Moreover, for syscore devices (dev->power.syscore is set for them),
> > > > > > the PM core doesn't set the "is_suspended" flag. Those can benefit
> > > > > > from a common behaviour.
> > > > > >
> > > > > > Finally, I think the "is_suspended" flag actually needs to be
> > > > > > protected by a lock when set by the PM core, as it's being used in two
> > > > > > separate execution paths. Although, rather than adding a lock for
> > > > > > protection, we can just rely on the "disable_depth" in rpm_resume().
> > > > > > It would be easier and makes the behaviour consistent too.
> > > > >
> > > > > As long as is_suspended isn't _written_ in two separate execution paths,
> > > > > we're probably okay without a lock -- provided the code doesn't mind
> > > > > getting an indefinite result when a read races with a write.
> > > >
> > > > Well, indefinite doesn't sound very good to me for these cases, even
> > > > if it most likely never will happen.
> > > >
> > > > >
> > > > > > > So overall, I suspect this change should not be made. But some other
> > > > > > > improvement (like a nice comment) might be in order.
> > > > > > >
> > > > > > > Alan Stern
> > > > > >
> > > > > > Thanks for reviewing!
> > > > >
> > > > > You're welcome. Whatever you eventually decide to do should be okay
> > > > > with me. I just wanted to make sure that you understood the deeper
> > > > > issue here and had given it some thought. For example, it may turn out
> > > > > that you can resolve matters simply by updating the documentation.
> > > >
> > > > I observed the issue on cpuidle-psci. The devices it operates upon are
> > > > assigned as syscore devices and these are hooked up to a genpd.
> > > >
> > > > A call to pm_runtime_get_sync() can happen even after the PM core has
> > > > disabled runtime PM in the "late" phase. So the error code is received
> > > > for these real use-cases.
> > > >
> > > > Now, as we currently don't check the return value of
> > > > pm_runtime_get_sync() in cpuidle-psci, it's not a big deal. But it
> > > > certainly seems worth fixing in my opinion.
> > > >
> > > > Let's see if Rafael has some thoughts around this.
> > >
> > > Am I thinking correctly that this is mostly about working around the
> > > limitations of pm_runtime_force_suspend()?
> >
> > No, this isn't related at all.
> >
> > The cpuidle-psci driver doesn't have PM callbacks, thus using
> > pm_runtime_force_suspend() would not work here.
>
> Just wanted to send a ping on this to see if we can come to a
> conclusion. Or maybe we did? :-)
>
> I think in the end, what slightly bothers me, is that the behavior is
> a bit inconsistent. Although, maybe it's the best we can do.
I've been thinking about this and it looks like we can do better, but
instead of talking about this I'd rather send a patch.
Stay tuned.
On Fri, 26 Nov 2021 at 14:30, Rafael J. Wysocki <[email protected]> wrote:
>
> On Fri, Nov 26, 2021 at 1:20 PM Ulf Hansson <[email protected]> wrote:
> >
> > On Mon, 1 Nov 2021 at 10:27, Ulf Hansson <[email protected]> wrote:
> > >
> > > On Fri, 29 Oct 2021 at 20:27, Rafael J. Wysocki <[email protected]> wrote:
> > > >
> > > > On Fri, Oct 29, 2021 at 12:20 AM Ulf Hansson <[email protected]> wrote:
> > > > >
> > > > > On Wed, 27 Oct 2021 at 16:33, Alan Stern <[email protected]> wrote:
> > > > > >
> > > > > > On Wed, Oct 27, 2021 at 12:55:43PM +0200, Ulf Hansson wrote:
> > > > > > > On Wed, 27 Oct 2021 at 04:02, Alan Stern <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Wed, Oct 27, 2021 at 12:26:26AM +0200, Ulf Hansson wrote:
> > > > > > > > > During system suspend, the PM core sets dev->power.is_suspended for the
> > > > > > > > > device that is being suspended. This flag is also being used in
> > > > > > > > > rpm_resume(), to allow it to succeed by returning 1, assuming that runtime
> > > > > > > > > PM has been disabled and the runtime PM status is RPM_ACTIVE, for the
> > > > > > > > > device.
> > > > > > > > >
> > > > > > > > > To make this behaviour a bit more useful, let's drop the check for the
> > > > > > > > > dev->power.is_suspended flag in rpm_resume(), as it doesn't really need to
> > > > > > > > > be limited to this anyway.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Ulf Hansson <[email protected]>
> > > > > > > > > ---
> > > > > > > > > drivers/base/power/runtime.c | 4 ++--
> > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > > > > > > index ec94049442b9..fadc278e3a66 100644
> > > > > > > > > --- a/drivers/base/power/runtime.c
> > > > > > > > > +++ b/drivers/base/power/runtime.c
> > > > > > > > > @@ -742,8 +742,8 @@ static int rpm_resume(struct device *dev, int rpmflags)
> > > > > > > > > repeat:
> > > > > > > > > if (dev->power.runtime_error)
> > > > > > > > > retval = -EINVAL;
> > > > > > > > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> > > > > > > > > - && dev->power.runtime_status == RPM_ACTIVE)
> > > > > > > > > + else if (dev->power.disable_depth > 0 &&
> > > > > > > > > + dev->power.runtime_status == RPM_ACTIVE)
> > > > > > > >
> > > > > > > > IIRC there was a good reason why the original code checked for
> > > > > > > > disable_depth == 1 rather than > 0. But I don't remember exactly what
> > > > > > > > the reason was. Maybe it had something to do with the fact that during
> > > > > > > > a system sleep __device_suspend_late calls __pm_runtime_disable, and the
> > > > > > > > code was checking that there were no other disables in effect.
> > > > > > >
> > > > > > > The check was introduced in the below commit:
> > > > > > >
> > > > > > > Commit 6f3c77b040fc
> > > > > > > Author: Kevin Hilman <[email protected]>
> > > > > > > Date: Fri Sep 21 22:47:34 2012 +0000
> > > > > > > PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2
> > > > > > >
> > > > > > > By reading the commit message it's pretty clear to me that the check
> > > > > > > was added to cover only one specific use case, during system suspend.
> > > > > > >
> > > > > > > That is, that a driver may want to call pm_runtime_get_sync() from a
> > > > > > > late/noirq callback (when the PM core has disabled runtime PM), to
> > > > > > > understand whether the device is still powered on and accessible.
> > > > > > >
> > > > > > > > This is
> > > > > > > > related to the documented behavior of rpm_resume (it's supposed to fail
> > > > > > > > with -EACCES if the device is disabled for runtime PM, no matter what
> > > > > > > > power state the device is in).
> > > > > > > >
> > > > > > > > That probably is also the explanation for why dev->power.is_suspended
> > > > > > > > gets checked: It's how the code tells whether a system sleep is in
> > > > > > > > progress.
> > > > > > >
> > > > > > > Yes, you are certainly correct about the current behaviour. It's there
> > > > > > > for a reason.
> > > > > > >
> > > > > > > On the other hand I would be greatly surprised if this change would
> > > > > > > cause any issues. Of course, I can't make guarantees, but I am, of
> > > > > > > course, willing to help to fix problems if those happen.
> > > > > > >
> > > > > > > As a matter of fact, I think the current behaviour looks quite
> > > > > > > inconsistent, as it depends on whether the device is being system
> > > > > > > suspended.
> > > > > > >
> > > > > > > Moreover, for syscore devices (dev->power.syscore is set for them),
> > > > > > > the PM core doesn't set the "is_suspended" flag. Those can benefit
> > > > > > > from a common behaviour.
> > > > > > >
> > > > > > > Finally, I think the "is_suspended" flag actually needs to be
> > > > > > > protected by a lock when set by the PM core, as it's being used in two
> > > > > > > separate execution paths. Although, rather than adding a lock for
> > > > > > > protection, we can just rely on the "disable_depth" in rpm_resume().
> > > > > > > It would be easier and makes the behaviour consistent too.
> > > > > >
> > > > > > As long as is_suspended isn't _written_ in two separate execution paths,
> > > > > > we're probably okay without a lock -- provided the code doesn't mind
> > > > > > getting an indefinite result when a read races with a write.
> > > > >
> > > > > Well, indefinite doesn't sound very good to me for these cases, even
> > > > > if it most likely never will happen.
> > > > >
> > > > > >
> > > > > > > > So overall, I suspect this change should not be made. But some other
> > > > > > > > improvement (like a nice comment) might be in order.
> > > > > > > >
> > > > > > > > Alan Stern
> > > > > > >
> > > > > > > Thanks for reviewing!
> > > > > >
> > > > > > You're welcome. Whatever you eventually decide to do should be okay
> > > > > > with me. I just wanted to make sure that you understood the deeper
> > > > > > issue here and had given it some thought. For example, it may turn out
> > > > > > that you can resolve matters simply by updating the documentation.
> > > > >
> > > > > I observed the issue on cpuidle-psci. The devices it operates upon are
> > > > > assigned as syscore devices and these are hooked up to a genpd.
> > > > >
> > > > > A call to pm_runtime_get_sync() can happen even after the PM core has
> > > > > disabled runtime PM in the "late" phase. So the error code is received
> > > > > for these real use-cases.
> > > > >
> > > > > Now, as we currently don't check the return value of
> > > > > pm_runtime_get_sync() in cpuidle-psci, it's not a big deal. But it
> > > > > certainly seems worth fixing in my opinion.
> > > > >
> > > > > Let's see if Rafael has some thoughts around this.
> > > >
> > > > Am I thinking correctly that this is mostly about working around the
> > > > limitations of pm_runtime_force_suspend()?
> > >
> > > No, this isn't related at all.
> > >
> > > The cpuidle-psci driver doesn't have PM callbacks, thus using
> > > pm_runtime_force_suspend() would not work here.
> >
> > Just wanted to send a ping on this to see if we can come to a
> > conclusion. Or maybe we did? :-)
> >
> > I think in the end, what slightly bothers me, is that the behavior is
> > a bit inconsistent. Although, maybe it's the best we can do.
>
> I've been thinking about this and it looks like we can do better, but
> instead of talking about this I'd rather send a patch.
Alright.
I was thinking along the lines of make similar changes for
rpm_idle|suspend(). That would make the behaviour even more
consistent, I think.
Perhaps that's what you have in mind? :-)
>
> Stay tuned.
Cool, thanks!
Kind regards
Uffe
On Friday, November 26, 2021 2:46:02 PM CET Ulf Hansson wrote:
> On Fri, 26 Nov 2021 at 14:30, Rafael J. Wysocki <[email protected]> wrote:
> >
> > On Fri, Nov 26, 2021 at 1:20 PM Ulf Hansson <[email protected]> wrote:
> > >
> > > On Mon, 1 Nov 2021 at 10:27, Ulf Hansson <[email protected]> wrote:
> > > >
> > > > On Fri, 29 Oct 2021 at 20:27, Rafael J. Wysocki <[email protected]> wrote:
> > > > >
> > > > > On Fri, Oct 29, 2021 at 12:20 AM Ulf Hansson <[email protected]> wrote:
> > > > > >
> > > > > > On Wed, 27 Oct 2021 at 16:33, Alan Stern <[email protected]> wrote:
> > > > > > >
> > > > > > > On Wed, Oct 27, 2021 at 12:55:43PM +0200, Ulf Hansson wrote:
> > > > > > > > On Wed, 27 Oct 2021 at 04:02, Alan Stern <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > On Wed, Oct 27, 2021 at 12:26:26AM +0200, Ulf Hansson wrote:
> > > > > > > > > > During system suspend, the PM core sets dev->power.is_suspended for the
> > > > > > > > > > device that is being suspended. This flag is also being used in
> > > > > > > > > > rpm_resume(), to allow it to succeed by returning 1, assuming that runtime
> > > > > > > > > > PM has been disabled and the runtime PM status is RPM_ACTIVE, for the
> > > > > > > > > > device.
> > > > > > > > > >
> > > > > > > > > > To make this behaviour a bit more useful, let's drop the check for the
> > > > > > > > > > dev->power.is_suspended flag in rpm_resume(), as it doesn't really need to
> > > > > > > > > > be limited to this anyway.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Ulf Hansson <[email protected]>
> > > > > > > > > > ---
> > > > > > > > > > drivers/base/power/runtime.c | 4 ++--
> > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > > > > > > > index ec94049442b9..fadc278e3a66 100644
> > > > > > > > > > --- a/drivers/base/power/runtime.c
> > > > > > > > > > +++ b/drivers/base/power/runtime.c
> > > > > > > > > > @@ -742,8 +742,8 @@ static int rpm_resume(struct device *dev, int rpmflags)
> > > > > > > > > > repeat:
> > > > > > > > > > if (dev->power.runtime_error)
> > > > > > > > > > retval = -EINVAL;
> > > > > > > > > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> > > > > > > > > > - && dev->power.runtime_status == RPM_ACTIVE)
> > > > > > > > > > + else if (dev->power.disable_depth > 0 &&
> > > > > > > > > > + dev->power.runtime_status == RPM_ACTIVE)
> > > > > > > > >
> > > > > > > > > IIRC there was a good reason why the original code checked for
> > > > > > > > > disable_depth == 1 rather than > 0. But I don't remember exactly what
> > > > > > > > > the reason was. Maybe it had something to do with the fact that during
> > > > > > > > > a system sleep __device_suspend_late calls __pm_runtime_disable, and the
> > > > > > > > > code was checking that there were no other disables in effect.
> > > > > > > >
> > > > > > > > The check was introduced in the below commit:
> > > > > > > >
> > > > > > > > Commit 6f3c77b040fc
> > > > > > > > Author: Kevin Hilman <[email protected]>
> > > > > > > > Date: Fri Sep 21 22:47:34 2012 +0000
> > > > > > > > PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2
> > > > > > > >
> > > > > > > > By reading the commit message it's pretty clear to me that the check
> > > > > > > > was added to cover only one specific use case, during system suspend.
> > > > > > > >
> > > > > > > > That is, that a driver may want to call pm_runtime_get_sync() from a
> > > > > > > > late/noirq callback (when the PM core has disabled runtime PM), to
> > > > > > > > understand whether the device is still powered on and accessible.
> > > > > > > >
> > > > > > > > > This is
> > > > > > > > > related to the documented behavior of rpm_resume (it's supposed to fail
> > > > > > > > > with -EACCES if the device is disabled for runtime PM, no matter what
> > > > > > > > > power state the device is in).
> > > > > > > > >
> > > > > > > > > That probably is also the explanation for why dev->power.is_suspended
> > > > > > > > > gets checked: It's how the code tells whether a system sleep is in
> > > > > > > > > progress.
> > > > > > > >
> > > > > > > > Yes, you are certainly correct about the current behaviour. It's there
> > > > > > > > for a reason.
> > > > > > > >
> > > > > > > > On the other hand I would be greatly surprised if this change would
> > > > > > > > cause any issues. Of course, I can't make guarantees, but I am, of
> > > > > > > > course, willing to help to fix problems if those happen.
> > > > > > > >
> > > > > > > > As a matter of fact, I think the current behaviour looks quite
> > > > > > > > inconsistent, as it depends on whether the device is being system
> > > > > > > > suspended.
> > > > > > > >
> > > > > > > > Moreover, for syscore devices (dev->power.syscore is set for them),
> > > > > > > > the PM core doesn't set the "is_suspended" flag. Those can benefit
> > > > > > > > from a common behaviour.
> > > > > > > >
> > > > > > > > Finally, I think the "is_suspended" flag actually needs to be
> > > > > > > > protected by a lock when set by the PM core, as it's being used in two
> > > > > > > > separate execution paths. Although, rather than adding a lock for
> > > > > > > > protection, we can just rely on the "disable_depth" in rpm_resume().
> > > > > > > > It would be easier and makes the behaviour consistent too.
> > > > > > >
> > > > > > > As long as is_suspended isn't _written_ in two separate execution paths,
> > > > > > > we're probably okay without a lock -- provided the code doesn't mind
> > > > > > > getting an indefinite result when a read races with a write.
> > > > > >
> > > > > > Well, indefinite doesn't sound very good to me for these cases, even
> > > > > > if it most likely never will happen.
> > > > > >
> > > > > > >
> > > > > > > > > So overall, I suspect this change should not be made. But some other
> > > > > > > > > improvement (like a nice comment) might be in order.
> > > > > > > > >
> > > > > > > > > Alan Stern
> > > > > > > >
> > > > > > > > Thanks for reviewing!
> > > > > > >
> > > > > > > You're welcome. Whatever you eventually decide to do should be okay
> > > > > > > with me. I just wanted to make sure that you understood the deeper
> > > > > > > issue here and had given it some thought. For example, it may turn out
> > > > > > > that you can resolve matters simply by updating the documentation.
> > > > > >
> > > > > > I observed the issue on cpuidle-psci. The devices it operates upon are
> > > > > > assigned as syscore devices and these are hooked up to a genpd.
> > > > > >
> > > > > > A call to pm_runtime_get_sync() can happen even after the PM core has
> > > > > > disabled runtime PM in the "late" phase. So the error code is received
> > > > > > for these real use-cases.
> > > > > >
> > > > > > Now, as we currently don't check the return value of
> > > > > > pm_runtime_get_sync() in cpuidle-psci, it's not a big deal. But it
> > > > > > certainly seems worth fixing in my opinion.
> > > > > >
> > > > > > Let's see if Rafael has some thoughts around this.
> > > > >
> > > > > Am I thinking correctly that this is mostly about working around the
> > > > > limitations of pm_runtime_force_suspend()?
> > > >
> > > > No, this isn't related at all.
> > > >
> > > > The cpuidle-psci driver doesn't have PM callbacks, thus using
> > > > pm_runtime_force_suspend() would not work here.
> > >
> > > Just wanted to send a ping on this to see if we can come to a
> > > conclusion. Or maybe we did? :-)
> > >
> > > I think in the end, what slightly bothers me, is that the behavior is
> > > a bit inconsistent. Although, maybe it's the best we can do.
> >
> > I've been thinking about this and it looks like we can do better, but
> > instead of talking about this I'd rather send a patch.
>
> Alright.
>
> I was thinking along the lines of make similar changes for
> rpm_idle|suspend(). That would make the behaviour even more
> consistent, I think.
>
> Perhaps that's what you have in mind? :-)
Well, not exactly.
The idea is to add another counter (called restrain_depth in the patch)
to prevent rpm_resume() from running the callback when that is potentially
problematic. With that, it is possible to actually distinguish devices
with PM-runtime enabled and it allows the PM-runtime status to be checked
when it is still known to be meaningful.
It requires quite a few changes, but is rather straightforward, unless I'm
missing something.
Please see the patch below. I've only checked that it builds on x86-64.
---
drivers/base/power/main.c | 18 +++----
drivers/base/power/runtime.c | 105 ++++++++++++++++++++++++++++++++++++-------
include/linux/pm.h | 2
include/linux/pm_runtime.h | 2
4 files changed, 101 insertions(+), 26 deletions(-)
Index: linux-pm/include/linux/pm.h
===================================================================
--- linux-pm.orig/include/linux/pm.h
+++ linux-pm/include/linux/pm.h
@@ -598,6 +598,7 @@ struct dev_pm_info {
atomic_t usage_count;
atomic_t child_count;
unsigned int disable_depth:3;
+ unsigned int restrain_depth:3; /* PM core private */
unsigned int idle_notification:1;
unsigned int request_pending:1;
unsigned int deferred_resume:1;
@@ -609,6 +610,7 @@ struct dev_pm_info {
unsigned int use_autosuspend:1;
unsigned int timer_autosuspends:1;
unsigned int memalloc_noio:1;
+ unsigned int already_suspended:1; /* PM core private */
unsigned int links_count;
enum rpm_request request;
enum rpm_status runtime_status;
Index: linux-pm/include/linux/pm_runtime.h
===================================================================
--- linux-pm.orig/include/linux/pm_runtime.h
+++ linux-pm/include/linux/pm_runtime.h
@@ -46,6 +46,8 @@ extern void pm_runtime_enable(struct dev
extern void __pm_runtime_disable(struct device *dev, bool check_resume);
extern void pm_runtime_allow(struct device *dev);
extern void pm_runtime_forbid(struct device *dev);
+extern void pm_runtime_restrain(struct device *dev);
+extern void pm_runtime_relinquish(struct device *dev);
extern void pm_runtime_no_callbacks(struct device *dev);
extern void pm_runtime_irq_safe(struct device *dev);
extern void __pm_runtime_use_autosuspend(struct device *dev, bool use);
Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -744,11 +744,11 @@ static int rpm_resume(struct device *dev
repeat:
if (dev->power.runtime_error)
retval = -EINVAL;
- else if (dev->power.disable_depth == 1 && dev->power.is_suspended
- && dev->power.runtime_status == RPM_ACTIVE)
- retval = 1;
else if (dev->power.disable_depth > 0)
retval = -EACCES;
+ else if (dev->power.restrain_depth > 0)
+ retval = dev->power.runtime_status == RPM_ACTIVE ? 1 : -EAGAIN;
+
if (retval)
goto out;
@@ -1164,9 +1164,9 @@ EXPORT_SYMBOL_GPL(pm_runtime_get_if_acti
* @dev: Device to handle.
* @status: New runtime PM status of the device.
*
- * If runtime PM of the device is disabled or its power.runtime_error field is
- * different from zero, the status may be changed either to RPM_ACTIVE, or to
- * RPM_SUSPENDED, as long as that reflects the actual state of the device.
+ * If runtime PM of the device is disabled or restrained, or its
+ * power.runtime_error field is nonzero, the status may be changed either to
+ * RPM_ACTIVE, or to RPM_SUSPENDED, as long as that reflects its actual state.
* However, if the device has a parent and the parent is not active, and the
* parent's power.ignore_children flag is unset, the device's status cannot be
* set to RPM_ACTIVE, so -EBUSY is returned in that case.
@@ -1195,13 +1195,16 @@ int __pm_runtime_set_status(struct devic
spin_lock_irq(&dev->power.lock);
/*
- * Prevent PM-runtime from being enabled for the device or return an
- * error if it is enabled already and working.
+ * Prevent PM-runtime from being used for the device or return an
+ * error if it is in use already.
*/
- if (dev->power.runtime_error || dev->power.disable_depth)
- dev->power.disable_depth++;
- else
+ if (dev->power.runtime_error || dev->power.disable_depth ||
+ dev->power.restrain_depth) {
+ pm_runtime_get_noresume(dev);
+ dev->power.restrain_depth++;
+ } else {
error = -EAGAIN;
+ }
spin_unlock_irq(&dev->power.lock);
@@ -1278,7 +1281,7 @@ int __pm_runtime_set_status(struct devic
device_links_read_unlock(idx);
}
- pm_runtime_enable(dev);
+ pm_runtime_relinquish(dev);
return error;
}
@@ -1513,6 +1516,72 @@ void pm_runtime_allow(struct device *dev
EXPORT_SYMBOL_GPL(pm_runtime_allow);
/**
+ * pm_runtime_restrain - Temporarily block runtime PM of a device.
+ * @dev: Device to handle.
+ *
+ * Increase the device's usage count and its restrain_dpeth count. If the
+ * latter was 0 initially, cancel the runtime PM work for @dev if pending and
+ * wait for all of the runtime PM operations on it in progress to complete.
+ *
+ * After this function has been called, attempts to runtime-suspend @dev will
+ * fail with -EAGAIN and attempts to runtime-resume it will succeed if its
+ * runtime PM status is RPM_ACTIVE and will fail with -EAGAIN otherwise.
+ *
+ * This function can only be called by the PM core.
+ */
+void pm_runtime_restrain(struct device *dev)
+{
+ pm_runtime_get_noresume(dev);
+
+ spin_lock_irq(&dev->power.lock);
+
+ if (dev->power.restrain_depth++ > 0)
+ goto out;
+
+ if (dev->power.disable_depth > 0) {
+ dev->power.already_suspended = false;
+ goto out;
+ }
+
+ /* Update time accounting before blocking PM-runtime. */
+ update_pm_runtime_accounting(dev);
+
+ __pm_runtime_barrier(dev);
+
+ dev->power.already_suspended = pm_runtime_status_suspended(dev);
+
+out:
+ spin_unlock_irq(&dev->power.lock);
+}
+
+/**
+ * pm_runtime_relinquish - Unblock runtime PM of a device.
+ * @dev: Device to handle.
+ *
+ * Decrease the device's usage count and its restrain_dpeth count.
+ *
+ * This function can only be called by the PM core.
+ */
+void pm_runtime_relinquish(struct device *dev)
+{
+ spin_lock_irq(&dev->power.lock);
+
+ if (dev->power.restrain_depth > 0) {
+ dev->power.restrain_depth--;
+
+ /* About to unbolck runtime PM, set accounting_timestamp to now */
+ if (!dev->power.restrain_depth && !dev->power.disable_depth)
+ dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
+ } else {
+ dev_warn(dev, "Unbalanced %s!\n", __func__);
+ }
+
+ spin_unlock_irq(&dev->power.lock);
+
+ pm_runtime_put_noidle(dev);
+}
+
+/**
* pm_runtime_no_callbacks - Ignore runtime PM callbacks for a device.
* @dev: Device to handle.
*
@@ -1806,8 +1875,10 @@ int pm_runtime_force_suspend(struct devi
int (*callback)(struct device *);
int ret;
- pm_runtime_disable(dev);
- if (pm_runtime_status_suspended(dev))
+ pm_runtime_restrain(dev);
+
+ /* No suspend if the device has already been suspended by PM-runtime. */
+ if (!dev->power.already_suspended)
return 0;
callback = RPM_GET_CALLBACK(dev, runtime_suspend);
@@ -1832,7 +1903,7 @@ int pm_runtime_force_suspend(struct devi
return 0;
err:
- pm_runtime_enable(dev);
+ pm_runtime_relinquish(dev);
return ret;
}
EXPORT_SYMBOL_GPL(pm_runtime_force_suspend);
@@ -1854,7 +1925,7 @@ int pm_runtime_force_resume(struct devic
int (*callback)(struct device *);
int ret = 0;
- if (!pm_runtime_status_suspended(dev) || !dev->power.needs_force_resume)
+ if (!dev->power.already_suspended || !dev->power.needs_force_resume)
goto out;
/*
@@ -1874,7 +1945,7 @@ int pm_runtime_force_resume(struct devic
pm_runtime_mark_last_busy(dev);
out:
dev->power.needs_force_resume = 0;
- pm_runtime_enable(dev);
+ pm_runtime_relinquish(dev);
return ret;
}
EXPORT_SYMBOL_GPL(pm_runtime_force_resume);
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -809,7 +809,7 @@ Skip:
Out:
TRACE_RESUME(error);
- pm_runtime_enable(dev);
+ pm_runtime_relinquish(dev);
complete_all(&dev->power.completion);
return error;
}
@@ -907,8 +907,8 @@ static int device_resume(struct device *
goto Complete;
if (dev->power.direct_complete) {
- /* Match the pm_runtime_disable() in __device_suspend(). */
- pm_runtime_enable(dev);
+ /* Match the pm_runtime_restrict() in __device_suspend(). */
+ pm_runtime_relinquish(dev);
goto Complete;
}
@@ -1392,7 +1392,7 @@ static int __device_suspend_late(struct
TRACE_DEVICE(dev);
TRACE_SUSPEND(0);
- __pm_runtime_disable(dev, false);
+ pm_runtime_restrain(dev);
dpm_wait_for_subordinate(dev, async);
@@ -1627,9 +1627,9 @@ static int __device_suspend(struct devic
* callbacks for it.
*
* If the system-wide suspend callbacks below change the configuration
- * of the device, they must disable runtime PM for it or otherwise
- * ensure that its runtime-resume callbacks will not be confused by that
- * change in case they are invoked going forward.
+ * of the device, they must ensure that its runtime-resume callbacks
+ * will not be confused by that change in case they are invoked going
+ * forward.
*/
pm_runtime_barrier(dev);
@@ -1648,13 +1648,13 @@ static int __device_suspend(struct devic
if (dev->power.direct_complete) {
if (pm_runtime_status_suspended(dev)) {
- pm_runtime_disable(dev);
+ pm_runtime_restrain(dev);
if (pm_runtime_status_suspended(dev)) {
pm_dev_dbg(dev, state, "direct-complete ");
goto Complete;
}
- pm_runtime_enable(dev);
+ pm_runtime_relinquish(dev);
}
dev->power.direct_complete = false;
}
On Fri, Nov 26, 2021 at 7:00 PM Rafael J. Wysocki <[email protected]> wrote:
>
> On Friday, November 26, 2021 2:46:02 PM CET Ulf Hansson wrote:
> > On Fri, 26 Nov 2021 at 14:30, Rafael J. Wysocki <[email protected]> wrote:
> > >
> > > On Fri, Nov 26, 2021 at 1:20 PM Ulf Hansson <[email protected]> wrote:
> > > >
> > > > On Mon, 1 Nov 2021 at 10:27, Ulf Hansson <[email protected]> wrote:
> > > > >
> > > > > On Fri, 29 Oct 2021 at 20:27, Rafael J. Wysocki <[email protected]> wrote:
> > > > > >
> > > > > > On Fri, Oct 29, 2021 at 12:20 AM Ulf Hansson <[email protected]> wrote:
> > > > > > >
> > > > > > > On Wed, 27 Oct 2021 at 16:33, Alan Stern <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Wed, Oct 27, 2021 at 12:55:43PM +0200, Ulf Hansson wrote:
> > > > > > > > > On Wed, 27 Oct 2021 at 04:02, Alan Stern <[email protected]> wrote:
> > > > > > > > > >
> > > > > > > > > > On Wed, Oct 27, 2021 at 12:26:26AM +0200, Ulf Hansson wrote:
> > > > > > > > > > > During system suspend, the PM core sets dev->power.is_suspended for the
> > > > > > > > > > > device that is being suspended. This flag is also being used in
> > > > > > > > > > > rpm_resume(), to allow it to succeed by returning 1, assuming that runtime
> > > > > > > > > > > PM has been disabled and the runtime PM status is RPM_ACTIVE, for the
> > > > > > > > > > > device.
> > > > > > > > > > >
> > > > > > > > > > > To make this behaviour a bit more useful, let's drop the check for the
> > > > > > > > > > > dev->power.is_suspended flag in rpm_resume(), as it doesn't really need to
> > > > > > > > > > > be limited to this anyway.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Ulf Hansson <[email protected]>
> > > > > > > > > > > ---
> > > > > > > > > > > drivers/base/power/runtime.c | 4 ++--
> > > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > > > > > > > > index ec94049442b9..fadc278e3a66 100644
> > > > > > > > > > > --- a/drivers/base/power/runtime.c
> > > > > > > > > > > +++ b/drivers/base/power/runtime.c
> > > > > > > > > > > @@ -742,8 +742,8 @@ static int rpm_resume(struct device *dev, int rpmflags)
> > > > > > > > > > > repeat:
> > > > > > > > > > > if (dev->power.runtime_error)
> > > > > > > > > > > retval = -EINVAL;
> > > > > > > > > > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> > > > > > > > > > > - && dev->power.runtime_status == RPM_ACTIVE)
> > > > > > > > > > > + else if (dev->power.disable_depth > 0 &&
> > > > > > > > > > > + dev->power.runtime_status == RPM_ACTIVE)
> > > > > > > > > >
> > > > > > > > > > IIRC there was a good reason why the original code checked for
> > > > > > > > > > disable_depth == 1 rather than > 0. But I don't remember exactly what
> > > > > > > > > > the reason was. Maybe it had something to do with the fact that during
> > > > > > > > > > a system sleep __device_suspend_late calls __pm_runtime_disable, and the
> > > > > > > > > > code was checking that there were no other disables in effect.
> > > > > > > > >
> > > > > > > > > The check was introduced in the below commit:
> > > > > > > > >
> > > > > > > > > Commit 6f3c77b040fc
> > > > > > > > > Author: Kevin Hilman <[email protected]>
> > > > > > > > > Date: Fri Sep 21 22:47:34 2012 +0000
> > > > > > > > > PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2
> > > > > > > > >
> > > > > > > > > By reading the commit message it's pretty clear to me that the check
> > > > > > > > > was added to cover only one specific use case, during system suspend.
> > > > > > > > >
> > > > > > > > > That is, that a driver may want to call pm_runtime_get_sync() from a
> > > > > > > > > late/noirq callback (when the PM core has disabled runtime PM), to
> > > > > > > > > understand whether the device is still powered on and accessible.
> > > > > > > > >
> > > > > > > > > > This is
> > > > > > > > > > related to the documented behavior of rpm_resume (it's supposed to fail
> > > > > > > > > > with -EACCES if the device is disabled for runtime PM, no matter what
> > > > > > > > > > power state the device is in).
> > > > > > > > > >
> > > > > > > > > > That probably is also the explanation for why dev->power.is_suspended
> > > > > > > > > > gets checked: It's how the code tells whether a system sleep is in
> > > > > > > > > > progress.
> > > > > > > > >
> > > > > > > > > Yes, you are certainly correct about the current behaviour. It's there
> > > > > > > > > for a reason.
> > > > > > > > >
> > > > > > > > > On the other hand I would be greatly surprised if this change would
> > > > > > > > > cause any issues. Of course, I can't make guarantees, but I am, of
> > > > > > > > > course, willing to help to fix problems if those happen.
> > > > > > > > >
> > > > > > > > > As a matter of fact, I think the current behaviour looks quite
> > > > > > > > > inconsistent, as it depends on whether the device is being system
> > > > > > > > > suspended.
> > > > > > > > >
> > > > > > > > > Moreover, for syscore devices (dev->power.syscore is set for them),
> > > > > > > > > the PM core doesn't set the "is_suspended" flag. Those can benefit
> > > > > > > > > from a common behaviour.
> > > > > > > > >
> > > > > > > > > Finally, I think the "is_suspended" flag actually needs to be
> > > > > > > > > protected by a lock when set by the PM core, as it's being used in two
> > > > > > > > > separate execution paths. Although, rather than adding a lock for
> > > > > > > > > protection, we can just rely on the "disable_depth" in rpm_resume().
> > > > > > > > > It would be easier and makes the behaviour consistent too.
> > > > > > > >
> > > > > > > > As long as is_suspended isn't _written_ in two separate execution paths,
> > > > > > > > we're probably okay without a lock -- provided the code doesn't mind
> > > > > > > > getting an indefinite result when a read races with a write.
> > > > > > >
> > > > > > > Well, indefinite doesn't sound very good to me for these cases, even
> > > > > > > if it most likely never will happen.
> > > > > > >
> > > > > > > >
> > > > > > > > > > So overall, I suspect this change should not be made. But some other
> > > > > > > > > > improvement (like a nice comment) might be in order.
> > > > > > > > > >
> > > > > > > > > > Alan Stern
> > > > > > > > >
> > > > > > > > > Thanks for reviewing!
> > > > > > > >
> > > > > > > > You're welcome. Whatever you eventually decide to do should be okay
> > > > > > > > with me. I just wanted to make sure that you understood the deeper
> > > > > > > > issue here and had given it some thought. For example, it may turn out
> > > > > > > > that you can resolve matters simply by updating the documentation.
> > > > > > >
> > > > > > > I observed the issue on cpuidle-psci. The devices it operates upon are
> > > > > > > assigned as syscore devices and these are hooked up to a genpd.
> > > > > > >
> > > > > > > A call to pm_runtime_get_sync() can happen even after the PM core has
> > > > > > > disabled runtime PM in the "late" phase. So the error code is received
> > > > > > > for these real use-cases.
> > > > > > >
> > > > > > > Now, as we currently don't check the return value of
> > > > > > > pm_runtime_get_sync() in cpuidle-psci, it's not a big deal. But it
> > > > > > > certainly seems worth fixing in my opinion.
> > > > > > >
> > > > > > > Let's see if Rafael has some thoughts around this.
> > > > > >
> > > > > > Am I thinking correctly that this is mostly about working around the
> > > > > > limitations of pm_runtime_force_suspend()?
> > > > >
> > > > > No, this isn't related at all.
> > > > >
> > > > > The cpuidle-psci driver doesn't have PM callbacks, thus using
> > > > > pm_runtime_force_suspend() would not work here.
> > > >
> > > > Just wanted to send a ping on this to see if we can come to a
> > > > conclusion. Or maybe we did? :-)
> > > >
> > > > I think in the end, what slightly bothers me, is that the behavior is
> > > > a bit inconsistent. Although, maybe it's the best we can do.
> > >
> > > I've been thinking about this and it looks like we can do better, but
> > > instead of talking about this I'd rather send a patch.
> >
> > Alright.
> >
> > I was thinking along the lines of make similar changes for
> > rpm_idle|suspend(). That would make the behaviour even more
> > consistent, I think.
> >
> > Perhaps that's what you have in mind? :-)
>
> Well, not exactly.
>
> The idea is to add another counter (called restrain_depth in the patch)
> to prevent rpm_resume() from running the callback when that is potentially
> problematic. With that, it is possible to actually distinguish devices
> with PM-runtime enabled and it allows the PM-runtime status to be checked
> when it is still known to be meaningful.
>
> It requires quite a few changes, but is rather straightforward, unless I'm
> missing something.
>
> Please see the patch below. I've only checked that it builds on x86-64.
>
> ---
> drivers/base/power/main.c | 18 +++----
> drivers/base/power/runtime.c | 105 ++++++++++++++++++++++++++++++++++++-------
> include/linux/pm.h | 2
> include/linux/pm_runtime.h | 2
> 4 files changed, 101 insertions(+), 26 deletions(-)
>
> Index: linux-pm/include/linux/pm.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm.h
> +++ linux-pm/include/linux/pm.h
> @@ -598,6 +598,7 @@ struct dev_pm_info {
> atomic_t usage_count;
> atomic_t child_count;
> unsigned int disable_depth:3;
> + unsigned int restrain_depth:3; /* PM core private */
> unsigned int idle_notification:1;
> unsigned int request_pending:1;
> unsigned int deferred_resume:1;
> @@ -609,6 +610,7 @@ struct dev_pm_info {
> unsigned int use_autosuspend:1;
> unsigned int timer_autosuspends:1;
> unsigned int memalloc_noio:1;
> + unsigned int already_suspended:1; /* PM core private */
> unsigned int links_count;
> enum rpm_request request;
> enum rpm_status runtime_status;
> Index: linux-pm/include/linux/pm_runtime.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm_runtime.h
> +++ linux-pm/include/linux/pm_runtime.h
> @@ -46,6 +46,8 @@ extern void pm_runtime_enable(struct dev
> extern void __pm_runtime_disable(struct device *dev, bool check_resume);
> extern void pm_runtime_allow(struct device *dev);
> extern void pm_runtime_forbid(struct device *dev);
> +extern void pm_runtime_restrain(struct device *dev);
> +extern void pm_runtime_relinquish(struct device *dev);
> extern void pm_runtime_no_callbacks(struct device *dev);
> extern void pm_runtime_irq_safe(struct device *dev);
> extern void __pm_runtime_use_autosuspend(struct device *dev, bool use);
> Index: linux-pm/drivers/base/power/runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -744,11 +744,11 @@ static int rpm_resume(struct device *dev
> repeat:
> if (dev->power.runtime_error)
> retval = -EINVAL;
> - else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> - && dev->power.runtime_status == RPM_ACTIVE)
> - retval = 1;
> else if (dev->power.disable_depth > 0)
> retval = -EACCES;
> + else if (dev->power.restrain_depth > 0)
> + retval = dev->power.runtime_status == RPM_ACTIVE ? 1 : -EAGAIN;
> +
> if (retval)
> goto out;
>
> @@ -1164,9 +1164,9 @@ EXPORT_SYMBOL_GPL(pm_runtime_get_if_acti
> * @dev: Device to handle.
> * @status: New runtime PM status of the device.
> *
> - * If runtime PM of the device is disabled or its power.runtime_error field is
> - * different from zero, the status may be changed either to RPM_ACTIVE, or to
> - * RPM_SUSPENDED, as long as that reflects the actual state of the device.
> + * If runtime PM of the device is disabled or restrained, or its
> + * power.runtime_error field is nonzero, the status may be changed either to
> + * RPM_ACTIVE, or to RPM_SUSPENDED, as long as that reflects its actual state.
> * However, if the device has a parent and the parent is not active, and the
> * parent's power.ignore_children flag is unset, the device's status cannot be
> * set to RPM_ACTIVE, so -EBUSY is returned in that case.
> @@ -1195,13 +1195,16 @@ int __pm_runtime_set_status(struct devic
> spin_lock_irq(&dev->power.lock);
>
> /*
> - * Prevent PM-runtime from being enabled for the device or return an
> - * error if it is enabled already and working.
> + * Prevent PM-runtime from being used for the device or return an
> + * error if it is in use already.
> */
> - if (dev->power.runtime_error || dev->power.disable_depth)
> - dev->power.disable_depth++;
> - else
> + if (dev->power.runtime_error || dev->power.disable_depth ||
> + dev->power.restrain_depth) {
> + pm_runtime_get_noresume(dev);
> + dev->power.restrain_depth++;
> + } else {
> error = -EAGAIN;
> + }
>
> spin_unlock_irq(&dev->power.lock);
>
> @@ -1278,7 +1281,7 @@ int __pm_runtime_set_status(struct devic
> device_links_read_unlock(idx);
> }
>
> - pm_runtime_enable(dev);
> + pm_runtime_relinquish(dev);
>
> return error;
> }
> @@ -1513,6 +1516,72 @@ void pm_runtime_allow(struct device *dev
> EXPORT_SYMBOL_GPL(pm_runtime_allow);
>
> /**
> + * pm_runtime_restrain - Temporarily block runtime PM of a device.
> + * @dev: Device to handle.
> + *
> + * Increase the device's usage count and its restrain_dpeth count. If the
> + * latter was 0 initially, cancel the runtime PM work for @dev if pending and
> + * wait for all of the runtime PM operations on it in progress to complete.
> + *
> + * After this function has been called, attempts to runtime-suspend @dev will
> + * fail with -EAGAIN and attempts to runtime-resume it will succeed if its
> + * runtime PM status is RPM_ACTIVE and will fail with -EAGAIN otherwise.
> + *
> + * This function can only be called by the PM core.
> + */
> +void pm_runtime_restrain(struct device *dev)
> +{
> + pm_runtime_get_noresume(dev);
> +
> + spin_lock_irq(&dev->power.lock);
> +
> + if (dev->power.restrain_depth++ > 0)
> + goto out;
> +
> + if (dev->power.disable_depth > 0) {
> + dev->power.already_suspended = false;
> + goto out;
> + }
> +
> + /* Update time accounting before blocking PM-runtime. */
> + update_pm_runtime_accounting(dev);
> +
> + __pm_runtime_barrier(dev);
> +
> + dev->power.already_suspended = pm_runtime_status_suspended(dev);
> +
> +out:
> + spin_unlock_irq(&dev->power.lock);
> +}
> +
> +/**
> + * pm_runtime_relinquish - Unblock runtime PM of a device.
> + * @dev: Device to handle.
> + *
> + * Decrease the device's usage count and its restrain_dpeth count.
> + *
> + * This function can only be called by the PM core.
> + */
> +void pm_runtime_relinquish(struct device *dev)
> +{
> + spin_lock_irq(&dev->power.lock);
> +
> + if (dev->power.restrain_depth > 0) {
> + dev->power.restrain_depth--;
> +
> + /* About to unbolck runtime PM, set accounting_timestamp to now */
> + if (!dev->power.restrain_depth && !dev->power.disable_depth)
> + dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
> + } else {
> + dev_warn(dev, "Unbalanced %s!\n", __func__);
> + }
> +
> + spin_unlock_irq(&dev->power.lock);
> +
> + pm_runtime_put_noidle(dev);
> +}
> +
> +/**
> * pm_runtime_no_callbacks - Ignore runtime PM callbacks for a device.
> * @dev: Device to handle.
> *
> @@ -1806,8 +1875,10 @@ int pm_runtime_force_suspend(struct devi
> int (*callback)(struct device *);
> int ret;
>
> - pm_runtime_disable(dev);
> - if (pm_runtime_status_suspended(dev))
> + pm_runtime_restrain(dev);
> +
> + /* No suspend if the device has already been suspended by PM-runtime. */
> + if (!dev->power.already_suspended)
I got the check here the other way around, sorry.
> return 0;
>
> callback = RPM_GET_CALLBACK(dev, runtime_suspend);
> @@ -1832,7 +1903,7 @@ int pm_runtime_force_suspend(struct devi
> return 0;
>
> err:
> - pm_runtime_enable(dev);
> + pm_runtime_relinquish(dev);
> return ret;
> }
> EXPORT_SYMBOL_GPL(pm_runtime_force_suspend);
> @@ -1854,7 +1925,7 @@ int pm_runtime_force_resume(struct devic
> int (*callback)(struct device *);
> int ret = 0;
>
> - if (!pm_runtime_status_suspended(dev) || !dev->power.needs_force_resume)
> + if (!dev->power.already_suspended || !dev->power.needs_force_resume)
And here I probably should leave the original check the way it is.
> goto out;
>
> /*
> @@ -1874,7 +1945,7 @@ int pm_runtime_force_resume(struct devic
> pm_runtime_mark_last_busy(dev);
> out:
> dev->power.needs_force_resume = 0;
> - pm_runtime_enable(dev);
> + pm_runtime_relinquish(dev);
> return ret;
> }
> EXPORT_SYMBOL_GPL(pm_runtime_force_resume);
> Index: linux-pm/drivers/base/power/main.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/main.c
> +++ linux-pm/drivers/base/power/main.c
> @@ -809,7 +809,7 @@ Skip:
> Out:
> TRACE_RESUME(error);
>
> - pm_runtime_enable(dev);
> + pm_runtime_relinquish(dev);
> complete_all(&dev->power.completion);
> return error;
> }
> @@ -907,8 +907,8 @@ static int device_resume(struct device *
> goto Complete;
>
> if (dev->power.direct_complete) {
> - /* Match the pm_runtime_disable() in __device_suspend(). */
> - pm_runtime_enable(dev);
> + /* Match the pm_runtime_restrict() in __device_suspend(). */
> + pm_runtime_relinquish(dev);
> goto Complete;
> }
>
> @@ -1392,7 +1392,7 @@ static int __device_suspend_late(struct
> TRACE_DEVICE(dev);
> TRACE_SUSPEND(0);
>
> - __pm_runtime_disable(dev, false);
> + pm_runtime_restrain(dev);
>
> dpm_wait_for_subordinate(dev, async);
>
> @@ -1627,9 +1627,9 @@ static int __device_suspend(struct devic
> * callbacks for it.
> *
> * If the system-wide suspend callbacks below change the configuration
> - * of the device, they must disable runtime PM for it or otherwise
> - * ensure that its runtime-resume callbacks will not be confused by that
> - * change in case they are invoked going forward.
> + * of the device, they must ensure that its runtime-resume callbacks
> + * will not be confused by that change in case they are invoked going
> + * forward.
> */
> pm_runtime_barrier(dev);
>
> @@ -1648,13 +1648,13 @@ static int __device_suspend(struct devic
>
> if (dev->power.direct_complete) {
> if (pm_runtime_status_suspended(dev)) {
> - pm_runtime_disable(dev);
> + pm_runtime_restrain(dev);
> if (pm_runtime_status_suspended(dev)) {
> pm_dev_dbg(dev, state, "direct-complete ");
> goto Complete;
> }
>
> - pm_runtime_enable(dev);
> + pm_runtime_relinquish(dev);
> }
> dev->power.direct_complete = false;
> }
>
>
>
[...]
> > > > > >
> > > > > > Am I thinking correctly that this is mostly about working around the
> > > > > > limitations of pm_runtime_force_suspend()?
> > > > >
> > > > > No, this isn't related at all.
> > > > >
> > > > > The cpuidle-psci driver doesn't have PM callbacks, thus using
> > > > > pm_runtime_force_suspend() would not work here.
> > > >
> > > > Just wanted to send a ping on this to see if we can come to a
> > > > conclusion. Or maybe we did? :-)
> > > >
> > > > I think in the end, what slightly bothers me, is that the behavior is
> > > > a bit inconsistent. Although, maybe it's the best we can do.
> > >
> > > I've been thinking about this and it looks like we can do better, but
> > > instead of talking about this I'd rather send a patch.
> >
> > Alright.
> >
> > I was thinking along the lines of make similar changes for
> > rpm_idle|suspend(). That would make the behaviour even more
> > consistent, I think.
> >
> > Perhaps that's what you have in mind? :-)
>
> Well, not exactly.
>
> The idea is to add another counter (called restrain_depth in the patch)
> to prevent rpm_resume() from running the callback when that is potentially
> problematic. With that, it is possible to actually distinguish devices
> with PM-runtime enabled and it allows the PM-runtime status to be checked
> when it is still known to be meaningful.
Hmm, I don't quite understand the benefit of introducing a new flag
for this. rpm_resume() already checks the disable_depth to understand
when it's safe to invoke the callback. Maybe there is a reason why
that isn't sufficient?
>
> It requires quite a few changes, but is rather straightforward, unless I'm
> missing something.
>
> Please see the patch below. I've only checked that it builds on x86-64.
>
> ---
> drivers/base/power/main.c | 18 +++----
> drivers/base/power/runtime.c | 105 ++++++++++++++++++++++++++++++++++++-------
> include/linux/pm.h | 2
> include/linux/pm_runtime.h | 2
> 4 files changed, 101 insertions(+), 26 deletions(-)
>
> Index: linux-pm/include/linux/pm.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm.h
> +++ linux-pm/include/linux/pm.h
> @@ -598,6 +598,7 @@ struct dev_pm_info {
> atomic_t usage_count;
> atomic_t child_count;
> unsigned int disable_depth:3;
> + unsigned int restrain_depth:3; /* PM core private */
> unsigned int idle_notification:1;
> unsigned int request_pending:1;
> unsigned int deferred_resume:1;
> @@ -609,6 +610,7 @@ struct dev_pm_info {
> unsigned int use_autosuspend:1;
> unsigned int timer_autosuspends:1;
> unsigned int memalloc_noio:1;
> + unsigned int already_suspended:1; /* PM core private */
> unsigned int links_count;
> enum rpm_request request;
> enum rpm_status runtime_status;
> Index: linux-pm/include/linux/pm_runtime.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm_runtime.h
> +++ linux-pm/include/linux/pm_runtime.h
> @@ -46,6 +46,8 @@ extern void pm_runtime_enable(struct dev
> extern void __pm_runtime_disable(struct device *dev, bool check_resume);
> extern void pm_runtime_allow(struct device *dev);
> extern void pm_runtime_forbid(struct device *dev);
> +extern void pm_runtime_restrain(struct device *dev);
> +extern void pm_runtime_relinquish(struct device *dev);
> extern void pm_runtime_no_callbacks(struct device *dev);
> extern void pm_runtime_irq_safe(struct device *dev);
> extern void __pm_runtime_use_autosuspend(struct device *dev, bool use);
> Index: linux-pm/drivers/base/power/runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -744,11 +744,11 @@ static int rpm_resume(struct device *dev
> repeat:
> if (dev->power.runtime_error)
> retval = -EINVAL;
> - else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> - && dev->power.runtime_status == RPM_ACTIVE)
> - retval = 1;
> else if (dev->power.disable_depth > 0)
> retval = -EACCES;
> + else if (dev->power.restrain_depth > 0)
> + retval = dev->power.runtime_status == RPM_ACTIVE ? 1 : -EAGAIN;
> +
> if (retval)
> goto out;
>
> @@ -1164,9 +1164,9 @@ EXPORT_SYMBOL_GPL(pm_runtime_get_if_acti
> * @dev: Device to handle.
> * @status: New runtime PM status of the device.
> *
> - * If runtime PM of the device is disabled or its power.runtime_error field is
> - * different from zero, the status may be changed either to RPM_ACTIVE, or to
> - * RPM_SUSPENDED, as long as that reflects the actual state of the device.
> + * If runtime PM of the device is disabled or restrained, or its
> + * power.runtime_error field is nonzero, the status may be changed either to
> + * RPM_ACTIVE, or to RPM_SUSPENDED, as long as that reflects its actual state.
> * However, if the device has a parent and the parent is not active, and the
> * parent's power.ignore_children flag is unset, the device's status cannot be
> * set to RPM_ACTIVE, so -EBUSY is returned in that case.
> @@ -1195,13 +1195,16 @@ int __pm_runtime_set_status(struct devic
> spin_lock_irq(&dev->power.lock);
>
> /*
> - * Prevent PM-runtime from being enabled for the device or return an
> - * error if it is enabled already and working.
> + * Prevent PM-runtime from being used for the device or return an
> + * error if it is in use already.
> */
> - if (dev->power.runtime_error || dev->power.disable_depth)
> - dev->power.disable_depth++;
> - else
> + if (dev->power.runtime_error || dev->power.disable_depth ||
> + dev->power.restrain_depth) {
> + pm_runtime_get_noresume(dev);
Why do we need to bump the usage count here? Except for balancing with
pm_runtime_relinquish() a few lines below, of course?
> + dev->power.restrain_depth++;
> + } else {
> error = -EAGAIN;
> + }
>
> spin_unlock_irq(&dev->power.lock);
>
> @@ -1278,7 +1281,7 @@ int __pm_runtime_set_status(struct devic
> device_links_read_unlock(idx);
> }
>
> - pm_runtime_enable(dev);
> + pm_runtime_relinquish(dev);
>
> return error;
> }
> @@ -1513,6 +1516,72 @@ void pm_runtime_allow(struct device *dev
> EXPORT_SYMBOL_GPL(pm_runtime_allow);
>
> /**
> + * pm_runtime_restrain - Temporarily block runtime PM of a device.
> + * @dev: Device to handle.
> + *
> + * Increase the device's usage count and its restrain_dpeth count. If the
> + * latter was 0 initially, cancel the runtime PM work for @dev if pending and
> + * wait for all of the runtime PM operations on it in progress to complete.
> + *
> + * After this function has been called, attempts to runtime-suspend @dev will
> + * fail with -EAGAIN and attempts to runtime-resume it will succeed if its
> + * runtime PM status is RPM_ACTIVE and will fail with -EAGAIN otherwise.
> + *
> + * This function can only be called by the PM core.
> + */
> +void pm_runtime_restrain(struct device *dev)
> +{
> + pm_runtime_get_noresume(dev);
> +
> + spin_lock_irq(&dev->power.lock);
> +
> + if (dev->power.restrain_depth++ > 0)
> + goto out;
> +
> + if (dev->power.disable_depth > 0) {
> + dev->power.already_suspended = false;
> + goto out;
> + }
> +
> + /* Update time accounting before blocking PM-runtime. */
> + update_pm_runtime_accounting(dev);
> +
> + __pm_runtime_barrier(dev);
> +
> + dev->power.already_suspended = pm_runtime_status_suspended(dev);
> +
> +out:
> + spin_unlock_irq(&dev->power.lock);
> +}
What if someone calls pm_runtime_disable() after the PM core has
called pm_runtime_restrain() for a device? It looks like we may run
another round of __pm_runtime_barrier() and
update_pm_runtime_accounting(), does that really make sense?
> +
> +/**
> + * pm_runtime_relinquish - Unblock runtime PM of a device.
> + * @dev: Device to handle.
> + *
> + * Decrease the device's usage count and its restrain_dpeth count.
> + *
> + * This function can only be called by the PM core.
> + */
> +void pm_runtime_relinquish(struct device *dev)
> +{
> + spin_lock_irq(&dev->power.lock);
> +
> + if (dev->power.restrain_depth > 0) {
> + dev->power.restrain_depth--;
> +
> + /* About to unbolck runtime PM, set accounting_timestamp to now */
> + if (!dev->power.restrain_depth && !dev->power.disable_depth)
> + dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
> + } else {
> + dev_warn(dev, "Unbalanced %s!\n", __func__);
> + }
> +
> + spin_unlock_irq(&dev->power.lock);
> +
> + pm_runtime_put_noidle(dev);
> +}
> +
> +/**
> * pm_runtime_no_callbacks - Ignore runtime PM callbacks for a device.
> * @dev: Device to handle.
> *
> @@ -1806,8 +1875,10 @@ int pm_runtime_force_suspend(struct devi
> int (*callback)(struct device *);
> int ret;
>
> - pm_runtime_disable(dev);
> - if (pm_runtime_status_suspended(dev))
> + pm_runtime_restrain(dev);
> +
> + /* No suspend if the device has already been suspended by PM-runtime. */
> + if (!dev->power.already_suspended)
I assume you are looking at using pm_runtime_force_suspend|resume() to
support my use case for the cpuidle-psci driver? In other words,
replace pm_runtime_get_sync() and pm_runtime_put_sync_suspend() in
__psci_enter_domain_idle_state(), right?
If so, that doesn't really fit well, I think. Not only because we
don't have system suspend/resume callbacks available, which is really
the proper place to call the pm_runtime_force_*() functions from, but
also because we don't want to call __pm_runtime_barrier(), etc, every
time in the idle path of a CPU. If anything, we should instead strive
towards a more lightweight path than what we currently have.
> return 0;
>
> callback = RPM_GET_CALLBACK(dev, runtime_suspend);
> @@ -1832,7 +1903,7 @@ int pm_runtime_force_suspend(struct devi
> return 0;
>
> err:
> - pm_runtime_enable(dev);
> + pm_runtime_relinquish(dev);
> return ret;
> }
> EXPORT_SYMBOL_GPL(pm_runtime_force_suspend);
> @@ -1854,7 +1925,7 @@ int pm_runtime_force_resume(struct devic
> int (*callback)(struct device *);
> int ret = 0;
>
> - if (!pm_runtime_status_suspended(dev) || !dev->power.needs_force_resume)
> + if (!dev->power.already_suspended || !dev->power.needs_force_resume)
> goto out;
>
> /*
> @@ -1874,7 +1945,7 @@ int pm_runtime_force_resume(struct devic
> pm_runtime_mark_last_busy(dev);
> out:
> dev->power.needs_force_resume = 0;
> - pm_runtime_enable(dev);
> + pm_runtime_relinquish(dev);
> return ret;
> }
> EXPORT_SYMBOL_GPL(pm_runtime_force_resume);
> Index: linux-pm/drivers/base/power/main.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/main.c
> +++ linux-pm/drivers/base/power/main.c
> @@ -809,7 +809,7 @@ Skip:
> Out:
> TRACE_RESUME(error);
>
> - pm_runtime_enable(dev);
> + pm_runtime_relinquish(dev);
> complete_all(&dev->power.completion);
> return error;
> }
> @@ -907,8 +907,8 @@ static int device_resume(struct device *
> goto Complete;
>
> if (dev->power.direct_complete) {
> - /* Match the pm_runtime_disable() in __device_suspend(). */
> - pm_runtime_enable(dev);
> + /* Match the pm_runtime_restrict() in __device_suspend(). */
> + pm_runtime_relinquish(dev);
> goto Complete;
> }
>
> @@ -1392,7 +1392,7 @@ static int __device_suspend_late(struct
> TRACE_DEVICE(dev);
> TRACE_SUSPEND(0);
>
> - __pm_runtime_disable(dev, false);
> + pm_runtime_restrain(dev);
>
> dpm_wait_for_subordinate(dev, async);
>
> @@ -1627,9 +1627,9 @@ static int __device_suspend(struct devic
> * callbacks for it.
> *
> * If the system-wide suspend callbacks below change the configuration
> - * of the device, they must disable runtime PM for it or otherwise
> - * ensure that its runtime-resume callbacks will not be confused by that
> - * change in case they are invoked going forward.
> + * of the device, they must ensure that its runtime-resume callbacks
> + * will not be confused by that change in case they are invoked going
> + * forward.
> */
> pm_runtime_barrier(dev);
>
> @@ -1648,13 +1648,13 @@ static int __device_suspend(struct devic
>
> if (dev->power.direct_complete) {
> if (pm_runtime_status_suspended(dev)) {
> - pm_runtime_disable(dev);
> + pm_runtime_restrain(dev);
> if (pm_runtime_status_suspended(dev)) {
> pm_dev_dbg(dev, state, "direct-complete ");
> goto Complete;
> }
>
> - pm_runtime_enable(dev);
> + pm_runtime_relinquish(dev);
> }
> dev->power.direct_complete = false;
> }
>
>
>
Kind regards
Uffe
On Tue, Nov 30, 2021 at 12:58 PM Ulf Hansson <[email protected]> wrote:
>
> [...]
>
> > > > > > >
> > > > > > > Am I thinking correctly that this is mostly about working around the
> > > > > > > limitations of pm_runtime_force_suspend()?
> > > > > >
> > > > > > No, this isn't related at all.
> > > > > >
> > > > > > The cpuidle-psci driver doesn't have PM callbacks, thus using
> > > > > > pm_runtime_force_suspend() would not work here.
> > > > >
> > > > > Just wanted to send a ping on this to see if we can come to a
> > > > > conclusion. Or maybe we did? :-)
> > > > >
> > > > > I think in the end, what slightly bothers me, is that the behavior is
> > > > > a bit inconsistent. Although, maybe it's the best we can do.
> > > >
> > > > I've been thinking about this and it looks like we can do better, but
> > > > instead of talking about this I'd rather send a patch.
> > >
> > > Alright.
> > >
> > > I was thinking along the lines of make similar changes for
> > > rpm_idle|suspend(). That would make the behaviour even more
> > > consistent, I think.
> > >
> > > Perhaps that's what you have in mind? :-)
> >
> > Well, not exactly.
> >
> > The idea is to add another counter (called restrain_depth in the patch)
> > to prevent rpm_resume() from running the callback when that is potentially
> > problematic. With that, it is possible to actually distinguish devices
> > with PM-runtime enabled and it allows the PM-runtime status to be checked
> > when it is still known to be meaningful.
>
> Hmm, I don't quite understand the benefit of introducing a new flag
> for this. rpm_resume() already checks the disable_depth to understand
> when it's safe to invoke the callback. Maybe there is a reason why
> that isn't sufficient?
The problem is that disable_depth > 0 may very well mean that runtime
PM has not been enabled at all for the given device which IMO is a
problem.
As it stands, it is necessary to make assumptions, like disable_depth
== 1 meaning that runtime PM is really enabled, but the PM core has
disabled it temporarily, which is somewhat questionable.
Another problem with disabling is that it causes rpm_resume() to fail
even if the status is RPM_ACTIVE and it has to do that exactly because
it cannot know why runtime PM has been disabled. If it has never been
enabled, rpm_resume() must fail, but if it has been disabled
temporarily, rpm_resume() may return 1 when the status is RPM_ACTIVE.
The new count allows the "enabled in general, but temporarily disabled
at the moment" to be handled cleanly.
> >
> > It requires quite a few changes, but is rather straightforward, unless I'm
> > missing something.
> >
> > Please see the patch below. I've only checked that it builds on x86-64.
> >
> > ---
> > drivers/base/power/main.c | 18 +++----
> > drivers/base/power/runtime.c | 105 ++++++++++++++++++++++++++++++++++++-------
> > include/linux/pm.h | 2
> > include/linux/pm_runtime.h | 2
> > 4 files changed, 101 insertions(+), 26 deletions(-)
> >
> > Index: linux-pm/include/linux/pm.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/pm.h
> > +++ linux-pm/include/linux/pm.h
> > @@ -598,6 +598,7 @@ struct dev_pm_info {
> > atomic_t usage_count;
> > atomic_t child_count;
> > unsigned int disable_depth:3;
> > + unsigned int restrain_depth:3; /* PM core private */
> > unsigned int idle_notification:1;
> > unsigned int request_pending:1;
> > unsigned int deferred_resume:1;
> > @@ -609,6 +610,7 @@ struct dev_pm_info {
> > unsigned int use_autosuspend:1;
> > unsigned int timer_autosuspends:1;
> > unsigned int memalloc_noio:1;
> > + unsigned int already_suspended:1; /* PM core private */
> > unsigned int links_count;
> > enum rpm_request request;
> > enum rpm_status runtime_status;
> > Index: linux-pm/include/linux/pm_runtime.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/pm_runtime.h
> > +++ linux-pm/include/linux/pm_runtime.h
> > @@ -46,6 +46,8 @@ extern void pm_runtime_enable(struct dev
> > extern void __pm_runtime_disable(struct device *dev, bool check_resume);
> > extern void pm_runtime_allow(struct device *dev);
> > extern void pm_runtime_forbid(struct device *dev);
> > +extern void pm_runtime_restrain(struct device *dev);
> > +extern void pm_runtime_relinquish(struct device *dev);
> > extern void pm_runtime_no_callbacks(struct device *dev);
> > extern void pm_runtime_irq_safe(struct device *dev);
> > extern void __pm_runtime_use_autosuspend(struct device *dev, bool use);
> > Index: linux-pm/drivers/base/power/runtime.c
> > ===================================================================
> > --- linux-pm.orig/drivers/base/power/runtime.c
> > +++ linux-pm/drivers/base/power/runtime.c
> > @@ -744,11 +744,11 @@ static int rpm_resume(struct device *dev
> > repeat:
> > if (dev->power.runtime_error)
> > retval = -EINVAL;
> > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> > - && dev->power.runtime_status == RPM_ACTIVE)
> > - retval = 1;
> > else if (dev->power.disable_depth > 0)
> > retval = -EACCES;
> > + else if (dev->power.restrain_depth > 0)
> > + retval = dev->power.runtime_status == RPM_ACTIVE ? 1 : -EAGAIN;
> > +
> > if (retval)
> > goto out;
> >
> > @@ -1164,9 +1164,9 @@ EXPORT_SYMBOL_GPL(pm_runtime_get_if_acti
> > * @dev: Device to handle.
> > * @status: New runtime PM status of the device.
> > *
> > - * If runtime PM of the device is disabled or its power.runtime_error field is
> > - * different from zero, the status may be changed either to RPM_ACTIVE, or to
> > - * RPM_SUSPENDED, as long as that reflects the actual state of the device.
> > + * If runtime PM of the device is disabled or restrained, or its
> > + * power.runtime_error field is nonzero, the status may be changed either to
> > + * RPM_ACTIVE, or to RPM_SUSPENDED, as long as that reflects its actual state.
> > * However, if the device has a parent and the parent is not active, and the
> > * parent's power.ignore_children flag is unset, the device's status cannot be
> > * set to RPM_ACTIVE, so -EBUSY is returned in that case.
> > @@ -1195,13 +1195,16 @@ int __pm_runtime_set_status(struct devic
> > spin_lock_irq(&dev->power.lock);
> >
> > /*
> > - * Prevent PM-runtime from being enabled for the device or return an
> > - * error if it is enabled already and working.
> > + * Prevent PM-runtime from being used for the device or return an
> > + * error if it is in use already.
> > */
> > - if (dev->power.runtime_error || dev->power.disable_depth)
> > - dev->power.disable_depth++;
> > - else
> > + if (dev->power.runtime_error || dev->power.disable_depth ||
> > + dev->power.restrain_depth) {
> > + pm_runtime_get_noresume(dev);
>
> Why do we need to bump the usage count here? Except for balancing with
> pm_runtime_relinquish() a few lines below, of course?
First off, I need the usage count to be greater than 0 to prevent the
runtime suspend callback from running while "restrained" (and the
suspend could check the restrain count, but that's one more check in
the suspend path which isn't necessary if the usage counter is always
bumped up upfront).
Second, the PM core bumps up the usage counter during system-wide
suspend, so bumping it up again isn't strictly needed if this
"temporary disabling" is limited to the system-wide suspend-resume
paths, but I'm not sure if it should be limited this way.
I would prefer the "temporarily unavailable" case to be clearly
different from the "disabled" one in any case, not just during
system-wide suspend-resume.
> > + dev->power.restrain_depth++;
> > + } else {
> > error = -EAGAIN;
> > + }
> >
> > spin_unlock_irq(&dev->power.lock);
> >
> > @@ -1278,7 +1281,7 @@ int __pm_runtime_set_status(struct devic
> > device_links_read_unlock(idx);
> > }
> >
> > - pm_runtime_enable(dev);
> > + pm_runtime_relinquish(dev);
> >
> > return error;
> > }
> > @@ -1513,6 +1516,72 @@ void pm_runtime_allow(struct device *dev
> > EXPORT_SYMBOL_GPL(pm_runtime_allow);
> >
> > /**
> > + * pm_runtime_restrain - Temporarily block runtime PM of a device.
> > + * @dev: Device to handle.
> > + *
> > + * Increase the device's usage count and its restrain_dpeth count. If the
> > + * latter was 0 initially, cancel the runtime PM work for @dev if pending and
> > + * wait for all of the runtime PM operations on it in progress to complete.
> > + *
> > + * After this function has been called, attempts to runtime-suspend @dev will
> > + * fail with -EAGAIN and attempts to runtime-resume it will succeed if its
> > + * runtime PM status is RPM_ACTIVE and will fail with -EAGAIN otherwise.
> > + *
> > + * This function can only be called by the PM core.
> > + */
> > +void pm_runtime_restrain(struct device *dev)
> > +{
> > + pm_runtime_get_noresume(dev);
> > +
> > + spin_lock_irq(&dev->power.lock);
> > +
> > + if (dev->power.restrain_depth++ > 0)
> > + goto out;
> > +
> > + if (dev->power.disable_depth > 0) {
> > + dev->power.already_suspended = false;
> > + goto out;
> > + }
> > +
> > + /* Update time accounting before blocking PM-runtime. */
> > + update_pm_runtime_accounting(dev);
> > +
> > + __pm_runtime_barrier(dev);
> > +
> > + dev->power.already_suspended = pm_runtime_status_suspended(dev);
> > +
> > +out:
> > + spin_unlock_irq(&dev->power.lock);
> > +}
>
> What if someone calls pm_runtime_disable() after the PM core has
> called pm_runtime_restrain() for a device? It looks like we may run
> another round of __pm_runtime_barrier() and
> update_pm_runtime_accounting(), does that really make sense?
No, it doesn't, but it's a bug in the patch. And there are other bugs in it ...
In this particular case, __pm_runtime_disable() should check the
"restrain" count and do nothing when it is nonzero.
> > +
> > +/**
> > + * pm_runtime_relinquish - Unblock runtime PM of a device.
> > + * @dev: Device to handle.
> > + *
> > + * Decrease the device's usage count and its restrain_dpeth count.
> > + *
> > + * This function can only be called by the PM core.
> > + */
> > +void pm_runtime_relinquish(struct device *dev)
> > +{
> > + spin_lock_irq(&dev->power.lock);
> > +
> > + if (dev->power.restrain_depth > 0) {
> > + dev->power.restrain_depth--;
> > +
> > + /* About to unbolck runtime PM, set accounting_timestamp to now */
> > + if (!dev->power.restrain_depth && !dev->power.disable_depth)
> > + dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
> > + } else {
> > + dev_warn(dev, "Unbalanced %s!\n", __func__);
> > + }
> > +
> > + spin_unlock_irq(&dev->power.lock);
> > +
> > + pm_runtime_put_noidle(dev);
> > +}
> > +
> > +/**
> > * pm_runtime_no_callbacks - Ignore runtime PM callbacks for a device.
> > * @dev: Device to handle.
> > *
> > @@ -1806,8 +1875,10 @@ int pm_runtime_force_suspend(struct devi
> > int (*callback)(struct device *);
> > int ret;
> >
> > - pm_runtime_disable(dev);
> > - if (pm_runtime_status_suspended(dev))
> > + pm_runtime_restrain(dev);
> > +
> > + /* No suspend if the device has already been suspended by PM-runtime. */
> > + if (!dev->power.already_suspended)
>
> I assume you are looking at using pm_runtime_force_suspend|resume() to
> support my use case for the cpuidle-psci driver? In other words,
> replace pm_runtime_get_sync() and pm_runtime_put_sync_suspend() in
> __psci_enter_domain_idle_state(), right?
Not really. I've been looking at a general "temporarily unavailable"
vs "disabled" problem.
> If so, that doesn't really fit well, I think. Not only because we
> don't have system suspend/resume callbacks available, which is really
> the proper place to call the pm_runtime_force_*() functions from, but
> also because we don't want to call __pm_runtime_barrier(), etc, every
> time in the idle path of a CPU. If anything, we should instead strive
> towards a more lightweight path than what we currently have.
So IMO this can be done with the new counter in place, because for
anything called between device_suspend_late() and
device_resume_early(), PM-runtime would be restrained by the PM core
(it is disabled now), so rpm_resume() would return 1 for devices with
PM-runtime status equal to RPM_ACTIVE (it fails now, unless the usage
counter is exactly 1) and you resume the devices in question upfront,
so it would be always safe to call rpm_resume() and rpm_suspend() for
them during the noirq suspend and resume phases (it is now tricky,
because it depends on the exact usage counter value).
Between dpm_suspend_noirq() and dpm_resume_noirq(), you need to switch
over to a different type of handling anyway, because all of the
devices are expected to be suspended then.
On Tue, 30 Nov 2021 at 14:02, Rafael J. Wysocki <[email protected]> wrote:
>
> On Tue, Nov 30, 2021 at 12:58 PM Ulf Hansson <[email protected]> wrote:
> >
> > [...]
> >
> > > > > > > >
> > > > > > > > Am I thinking correctly that this is mostly about working around the
> > > > > > > > limitations of pm_runtime_force_suspend()?
> > > > > > >
> > > > > > > No, this isn't related at all.
> > > > > > >
> > > > > > > The cpuidle-psci driver doesn't have PM callbacks, thus using
> > > > > > > pm_runtime_force_suspend() would not work here.
> > > > > >
> > > > > > Just wanted to send a ping on this to see if we can come to a
> > > > > > conclusion. Or maybe we did? :-)
> > > > > >
> > > > > > I think in the end, what slightly bothers me, is that the behavior is
> > > > > > a bit inconsistent. Although, maybe it's the best we can do.
> > > > >
> > > > > I've been thinking about this and it looks like we can do better, but
> > > > > instead of talking about this I'd rather send a patch.
> > > >
> > > > Alright.
> > > >
> > > > I was thinking along the lines of make similar changes for
> > > > rpm_idle|suspend(). That would make the behaviour even more
> > > > consistent, I think.
> > > >
> > > > Perhaps that's what you have in mind? :-)
> > >
> > > Well, not exactly.
> > >
> > > The idea is to add another counter (called restrain_depth in the patch)
> > > to prevent rpm_resume() from running the callback when that is potentially
> > > problematic. With that, it is possible to actually distinguish devices
> > > with PM-runtime enabled and it allows the PM-runtime status to be checked
> > > when it is still known to be meaningful.
> >
> > Hmm, I don't quite understand the benefit of introducing a new flag
> > for this. rpm_resume() already checks the disable_depth to understand
> > when it's safe to invoke the callback. Maybe there is a reason why
> > that isn't sufficient?
>
> The problem is that disable_depth > 0 may very well mean that runtime
> PM has not been enabled at all for the given device which IMO is a
> problem.
>
> As it stands, it is necessary to make assumptions, like disable_depth
> == 1 meaning that runtime PM is really enabled, but the PM core has
> disabled it temporarily, which is somewhat questionable.
>
> Another problem with disabling is that it causes rpm_resume() to fail
> even if the status is RPM_ACTIVE and it has to do that exactly because
> it cannot know why runtime PM has been disabled. If it has never been
> enabled, rpm_resume() must fail, but if it has been disabled
> temporarily, rpm_resume() may return 1 when the status is RPM_ACTIVE.
>
> The new count allows the "enabled in general, but temporarily disabled
> at the moment" to be handled cleanly.
My overall comment is that I fail to understand why we need to
distinguish between these two cases. To me, it shouldn't really
matter, *why* runtime PM is (or have been) disabled for the device.
The important point is that the default state for a device is
RPM_SUSPENDED and someone has moved into RPM_ACTIVE, for whatever
reason. That should be sufficient to allow rpm_resume() to return '1'
when disable_depth > 0, shouldn't it?
>
> > >
> > > It requires quite a few changes, but is rather straightforward, unless I'm
> > > missing something.
> > >
> > > Please see the patch below. I've only checked that it builds on x86-64.
> > >
> > > ---
> > > drivers/base/power/main.c | 18 +++----
> > > drivers/base/power/runtime.c | 105 ++++++++++++++++++++++++++++++++++++-------
> > > include/linux/pm.h | 2
> > > include/linux/pm_runtime.h | 2
> > > 4 files changed, 101 insertions(+), 26 deletions(-)
> > >
> > > Index: linux-pm/include/linux/pm.h
> > > ===================================================================
> > > --- linux-pm.orig/include/linux/pm.h
> > > +++ linux-pm/include/linux/pm.h
> > > @@ -598,6 +598,7 @@ struct dev_pm_info {
> > > atomic_t usage_count;
> > > atomic_t child_count;
> > > unsigned int disable_depth:3;
> > > + unsigned int restrain_depth:3; /* PM core private */
> > > unsigned int idle_notification:1;
> > > unsigned int request_pending:1;
> > > unsigned int deferred_resume:1;
> > > @@ -609,6 +610,7 @@ struct dev_pm_info {
> > > unsigned int use_autosuspend:1;
> > > unsigned int timer_autosuspends:1;
> > > unsigned int memalloc_noio:1;
> > > + unsigned int already_suspended:1; /* PM core private */
> > > unsigned int links_count;
> > > enum rpm_request request;
> > > enum rpm_status runtime_status;
> > > Index: linux-pm/include/linux/pm_runtime.h
> > > ===================================================================
> > > --- linux-pm.orig/include/linux/pm_runtime.h
> > > +++ linux-pm/include/linux/pm_runtime.h
> > > @@ -46,6 +46,8 @@ extern void pm_runtime_enable(struct dev
> > > extern void __pm_runtime_disable(struct device *dev, bool check_resume);
> > > extern void pm_runtime_allow(struct device *dev);
> > > extern void pm_runtime_forbid(struct device *dev);
> > > +extern void pm_runtime_restrain(struct device *dev);
> > > +extern void pm_runtime_relinquish(struct device *dev);
> > > extern void pm_runtime_no_callbacks(struct device *dev);
> > > extern void pm_runtime_irq_safe(struct device *dev);
> > > extern void __pm_runtime_use_autosuspend(struct device *dev, bool use);
> > > Index: linux-pm/drivers/base/power/runtime.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/base/power/runtime.c
> > > +++ linux-pm/drivers/base/power/runtime.c
> > > @@ -744,11 +744,11 @@ static int rpm_resume(struct device *dev
> > > repeat:
> > > if (dev->power.runtime_error)
> > > retval = -EINVAL;
> > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> > > - && dev->power.runtime_status == RPM_ACTIVE)
> > > - retval = 1;
> > > else if (dev->power.disable_depth > 0)
> > > retval = -EACCES;
> > > + else if (dev->power.restrain_depth > 0)
> > > + retval = dev->power.runtime_status == RPM_ACTIVE ? 1 : -EAGAIN;
> > > +
> > > if (retval)
> > > goto out;
> > >
> > > @@ -1164,9 +1164,9 @@ EXPORT_SYMBOL_GPL(pm_runtime_get_if_acti
> > > * @dev: Device to handle.
> > > * @status: New runtime PM status of the device.
> > > *
> > > - * If runtime PM of the device is disabled or its power.runtime_error field is
> > > - * different from zero, the status may be changed either to RPM_ACTIVE, or to
> > > - * RPM_SUSPENDED, as long as that reflects the actual state of the device.
> > > + * If runtime PM of the device is disabled or restrained, or its
> > > + * power.runtime_error field is nonzero, the status may be changed either to
> > > + * RPM_ACTIVE, or to RPM_SUSPENDED, as long as that reflects its actual state.
> > > * However, if the device has a parent and the parent is not active, and the
> > > * parent's power.ignore_children flag is unset, the device's status cannot be
> > > * set to RPM_ACTIVE, so -EBUSY is returned in that case.
> > > @@ -1195,13 +1195,16 @@ int __pm_runtime_set_status(struct devic
> > > spin_lock_irq(&dev->power.lock);
> > >
> > > /*
> > > - * Prevent PM-runtime from being enabled for the device or return an
> > > - * error if it is enabled already and working.
> > > + * Prevent PM-runtime from being used for the device or return an
> > > + * error if it is in use already.
> > > */
> > > - if (dev->power.runtime_error || dev->power.disable_depth)
> > > - dev->power.disable_depth++;
> > > - else
> > > + if (dev->power.runtime_error || dev->power.disable_depth ||
> > > + dev->power.restrain_depth) {
> > > + pm_runtime_get_noresume(dev);
> >
> > Why do we need to bump the usage count here? Except for balancing with
> > pm_runtime_relinquish() a few lines below, of course?
>
> First off, I need the usage count to be greater than 0 to prevent the
> runtime suspend callback from running while "restrained" (and the
> suspend could check the restrain count, but that's one more check in
> the suspend path which isn't necessary if the usage counter is always
> bumped up upfront).
If disable_depth > 0 (or restrain_depth > 0), the runtime PM core
should prevent the runtime suspend callback from being invoked, no
matter whether the usage count has been bumped or not. Or did I get
that wrong?
>
> Second, the PM core bumps up the usage counter during system-wide
> suspend, so bumping it up again isn't strictly needed if this
> "temporary disabling" is limited to the system-wide suspend-resume
> paths, but I'm not sure if it should be limited this way.
>
> I would prefer the "temporarily unavailable" case to be clearly
> different from the "disabled" one in any case, not just during
> system-wide suspend-resume.
>
> > > + dev->power.restrain_depth++;
> > > + } else {
> > > error = -EAGAIN;
> > > + }
> > >
> > > spin_unlock_irq(&dev->power.lock);
> > >
> > > @@ -1278,7 +1281,7 @@ int __pm_runtime_set_status(struct devic
> > > device_links_read_unlock(idx);
> > > }
> > >
> > > - pm_runtime_enable(dev);
> > > + pm_runtime_relinquish(dev);
> > >
> > > return error;
> > > }
> > > @@ -1513,6 +1516,72 @@ void pm_runtime_allow(struct device *dev
> > > EXPORT_SYMBOL_GPL(pm_runtime_allow);
> > >
> > > /**
> > > + * pm_runtime_restrain - Temporarily block runtime PM of a device.
> > > + * @dev: Device to handle.
> > > + *
> > > + * Increase the device's usage count and its restrain_dpeth count. If the
> > > + * latter was 0 initially, cancel the runtime PM work for @dev if pending and
> > > + * wait for all of the runtime PM operations on it in progress to complete.
> > > + *
> > > + * After this function has been called, attempts to runtime-suspend @dev will
> > > + * fail with -EAGAIN and attempts to runtime-resume it will succeed if its
> > > + * runtime PM status is RPM_ACTIVE and will fail with -EAGAIN otherwise.
> > > + *
> > > + * This function can only be called by the PM core.
> > > + */
> > > +void pm_runtime_restrain(struct device *dev)
> > > +{
> > > + pm_runtime_get_noresume(dev);
> > > +
> > > + spin_lock_irq(&dev->power.lock);
> > > +
> > > + if (dev->power.restrain_depth++ > 0)
> > > + goto out;
> > > +
> > > + if (dev->power.disable_depth > 0) {
> > > + dev->power.already_suspended = false;
> > > + goto out;
> > > + }
> > > +
> > > + /* Update time accounting before blocking PM-runtime. */
> > > + update_pm_runtime_accounting(dev);
> > > +
> > > + __pm_runtime_barrier(dev);
> > > +
> > > + dev->power.already_suspended = pm_runtime_status_suspended(dev);
> > > +
> > > +out:
> > > + spin_unlock_irq(&dev->power.lock);
> > > +}
> >
> > What if someone calls pm_runtime_disable() after the PM core has
> > called pm_runtime_restrain() for a device? It looks like we may run
> > another round of __pm_runtime_barrier() and
> > update_pm_runtime_accounting(), does that really make sense?
>
> No, it doesn't, but it's a bug in the patch. And there are other bugs in it ...
>
> In this particular case, __pm_runtime_disable() should check the
> "restrain" count and do nothing when it is nonzero.
>
> > > +
> > > +/**
> > > + * pm_runtime_relinquish - Unblock runtime PM of a device.
> > > + * @dev: Device to handle.
> > > + *
> > > + * Decrease the device's usage count and its restrain_dpeth count.
> > > + *
> > > + * This function can only be called by the PM core.
> > > + */
> > > +void pm_runtime_relinquish(struct device *dev)
> > > +{
> > > + spin_lock_irq(&dev->power.lock);
> > > +
> > > + if (dev->power.restrain_depth > 0) {
> > > + dev->power.restrain_depth--;
> > > +
> > > + /* About to unbolck runtime PM, set accounting_timestamp to now */
> > > + if (!dev->power.restrain_depth && !dev->power.disable_depth)
> > > + dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
> > > + } else {
> > > + dev_warn(dev, "Unbalanced %s!\n", __func__);
> > > + }
> > > +
> > > + spin_unlock_irq(&dev->power.lock);
> > > +
> > > + pm_runtime_put_noidle(dev);
> > > +}
> > > +
> > > +/**
> > > * pm_runtime_no_callbacks - Ignore runtime PM callbacks for a device.
> > > * @dev: Device to handle.
> > > *
> > > @@ -1806,8 +1875,10 @@ int pm_runtime_force_suspend(struct devi
> > > int (*callback)(struct device *);
> > > int ret;
> > >
> > > - pm_runtime_disable(dev);
> > > - if (pm_runtime_status_suspended(dev))
> > > + pm_runtime_restrain(dev);
> > > +
> > > + /* No suspend if the device has already been suspended by PM-runtime. */
> > > + if (!dev->power.already_suspended)
> >
> > I assume you are looking at using pm_runtime_force_suspend|resume() to
> > support my use case for the cpuidle-psci driver? In other words,
> > replace pm_runtime_get_sync() and pm_runtime_put_sync_suspend() in
> > __psci_enter_domain_idle_state(), right?
>
> Not really. I've been looking at a general "temporarily unavailable"
> vs "disabled" problem.
Okay, so I understand that you want to distinguish between these two
cases, but honestly I fail to understand *why* that is needed, sorry.
>
> > If so, that doesn't really fit well, I think. Not only because we
> > don't have system suspend/resume callbacks available, which is really
> > the proper place to call the pm_runtime_force_*() functions from, but
> > also because we don't want to call __pm_runtime_barrier(), etc, every
> > time in the idle path of a CPU. If anything, we should instead strive
> > towards a more lightweight path than what we currently have.
>
> So IMO this can be done with the new counter in place, because for
> anything called between device_suspend_late() and
> device_resume_early(), PM-runtime would be restrained by the PM core
> (it is disabled now), so rpm_resume() would return 1 for devices with
> PM-runtime status equal to RPM_ACTIVE (it fails now, unless the usage
> counter is exactly 1) and you resume the devices in question upfront,
> so it would be always safe to call rpm_resume() and rpm_suspend() for
> them during the noirq suspend and resume phases (it is now tricky,
> because it depends on the exact usage counter value).
>
> Between dpm_suspend_noirq() and dpm_resume_noirq(), you need to switch
> over to a different type of handling anyway, because all of the
> devices are expected to be suspended then.
Not sure I understand correctly, but I don't think I need to switch to
another handling. The devices in __psci_enter_domain_idle_state() are
managed as syscore devices with genpd, for the later system suspend
phases, this works well.
Perhaps you also saying that the goal with your change is to allow
rpm_resume() to return 1, when the state is RPM_ACTIVE for the device
and when the PM core has called pm_runtime_restrain() instead of
__pm_runtime_disable()? Right?
Kind regards
Uffe
On Tue, Nov 30, 2021 at 5:41 PM Ulf Hansson <[email protected]> wrote:
>
> On Tue, 30 Nov 2021 at 14:02, Rafael J. Wysocki <[email protected]> wrote:
> >
> > On Tue, Nov 30, 2021 at 12:58 PM Ulf Hansson <[email protected]> wrote:
> > >
> > > [...]
> > >
> > > > > > > > >
> > > > > > > > > Am I thinking correctly that this is mostly about working around the
> > > > > > > > > limitations of pm_runtime_force_suspend()?
> > > > > > > >
> > > > > > > > No, this isn't related at all.
> > > > > > > >
> > > > > > > > The cpuidle-psci driver doesn't have PM callbacks, thus using
> > > > > > > > pm_runtime_force_suspend() would not work here.
> > > > > > >
> > > > > > > Just wanted to send a ping on this to see if we can come to a
> > > > > > > conclusion. Or maybe we did? :-)
> > > > > > >
> > > > > > > I think in the end, what slightly bothers me, is that the behavior is
> > > > > > > a bit inconsistent. Although, maybe it's the best we can do.
> > > > > >
> > > > > > I've been thinking about this and it looks like we can do better, but
> > > > > > instead of talking about this I'd rather send a patch.
> > > > >
> > > > > Alright.
> > > > >
> > > > > I was thinking along the lines of make similar changes for
> > > > > rpm_idle|suspend(). That would make the behaviour even more
> > > > > consistent, I think.
> > > > >
> > > > > Perhaps that's what you have in mind? :-)
> > > >
> > > > Well, not exactly.
> > > >
> > > > The idea is to add another counter (called restrain_depth in the patch)
> > > > to prevent rpm_resume() from running the callback when that is potentially
> > > > problematic. With that, it is possible to actually distinguish devices
> > > > with PM-runtime enabled and it allows the PM-runtime status to be checked
> > > > when it is still known to be meaningful.
> > >
> > > Hmm, I don't quite understand the benefit of introducing a new flag
> > > for this. rpm_resume() already checks the disable_depth to understand
> > > when it's safe to invoke the callback. Maybe there is a reason why
> > > that isn't sufficient?
> >
> > The problem is that disable_depth > 0 may very well mean that runtime
> > PM has not been enabled at all for the given device which IMO is a
> > problem.
> >
> > As it stands, it is necessary to make assumptions, like disable_depth
> > == 1 meaning that runtime PM is really enabled, but the PM core has
> > disabled it temporarily, which is somewhat questionable.
> >
> > Another problem with disabling is that it causes rpm_resume() to fail
> > even if the status is RPM_ACTIVE and it has to do that exactly because
> > it cannot know why runtime PM has been disabled. If it has never been
> > enabled, rpm_resume() must fail, but if it has been disabled
> > temporarily, rpm_resume() may return 1 when the status is RPM_ACTIVE.
> >
> > The new count allows the "enabled in general, but temporarily disabled
> > at the moment" to be handled cleanly.
>
> My overall comment is that I fail to understand why we need to
> distinguish between these two cases. To me, it shouldn't really
> matter, *why* runtime PM is (or have been) disabled for the device.
It matters if you want to trust the status, because "disabled" means
"the status doesn't matter".
If you want the status to stay meaningful, but prevent callbacks from
running, you need something else.
> The important point is that the default state for a device is
> RPM_SUSPENDED and someone has moved into RPM_ACTIVE, for whatever
> reason. That should be sufficient to allow rpm_resume() to return '1'
> when disable_depth > 0, shouldn't it?
No, because there is no rule by which the status of devices with
PM-runtime disabled must be RPM_SUSPENDED.
> >
> > > >
> > > > It requires quite a few changes, but is rather straightforward, unless I'm
> > > > missing something.
> > > >
> > > > Please see the patch below. I've only checked that it builds on x86-64.
> > > >
> > > > ---
> > > > drivers/base/power/main.c | 18 +++----
> > > > drivers/base/power/runtime.c | 105 ++++++++++++++++++++++++++++++++++++-------
> > > > include/linux/pm.h | 2
> > > > include/linux/pm_runtime.h | 2
> > > > 4 files changed, 101 insertions(+), 26 deletions(-)
> > > >
> > > > Index: linux-pm/include/linux/pm.h
> > > > ===================================================================
> > > > --- linux-pm.orig/include/linux/pm.h
> > > > +++ linux-pm/include/linux/pm.h
> > > > @@ -598,6 +598,7 @@ struct dev_pm_info {
> > > > atomic_t usage_count;
> > > > atomic_t child_count;
> > > > unsigned int disable_depth:3;
> > > > + unsigned int restrain_depth:3; /* PM core private */
> > > > unsigned int idle_notification:1;
> > > > unsigned int request_pending:1;
> > > > unsigned int deferred_resume:1;
> > > > @@ -609,6 +610,7 @@ struct dev_pm_info {
> > > > unsigned int use_autosuspend:1;
> > > > unsigned int timer_autosuspends:1;
> > > > unsigned int memalloc_noio:1;
> > > > + unsigned int already_suspended:1; /* PM core private */
> > > > unsigned int links_count;
> > > > enum rpm_request request;
> > > > enum rpm_status runtime_status;
> > > > Index: linux-pm/include/linux/pm_runtime.h
> > > > ===================================================================
> > > > --- linux-pm.orig/include/linux/pm_runtime.h
> > > > +++ linux-pm/include/linux/pm_runtime.h
> > > > @@ -46,6 +46,8 @@ extern void pm_runtime_enable(struct dev
> > > > extern void __pm_runtime_disable(struct device *dev, bool check_resume);
> > > > extern void pm_runtime_allow(struct device *dev);
> > > > extern void pm_runtime_forbid(struct device *dev);
> > > > +extern void pm_runtime_restrain(struct device *dev);
> > > > +extern void pm_runtime_relinquish(struct device *dev);
> > > > extern void pm_runtime_no_callbacks(struct device *dev);
> > > > extern void pm_runtime_irq_safe(struct device *dev);
> > > > extern void __pm_runtime_use_autosuspend(struct device *dev, bool use);
> > > > Index: linux-pm/drivers/base/power/runtime.c
> > > > ===================================================================
> > > > --- linux-pm.orig/drivers/base/power/runtime.c
> > > > +++ linux-pm/drivers/base/power/runtime.c
> > > > @@ -744,11 +744,11 @@ static int rpm_resume(struct device *dev
> > > > repeat:
> > > > if (dev->power.runtime_error)
> > > > retval = -EINVAL;
> > > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> > > > - && dev->power.runtime_status == RPM_ACTIVE)
> > > > - retval = 1;
> > > > else if (dev->power.disable_depth > 0)
> > > > retval = -EACCES;
> > > > + else if (dev->power.restrain_depth > 0)
> > > > + retval = dev->power.runtime_status == RPM_ACTIVE ? 1 : -EAGAIN;
> > > > +
> > > > if (retval)
> > > > goto out;
> > > >
> > > > @@ -1164,9 +1164,9 @@ EXPORT_SYMBOL_GPL(pm_runtime_get_if_acti
> > > > * @dev: Device to handle.
> > > > * @status: New runtime PM status of the device.
> > > > *
> > > > - * If runtime PM of the device is disabled or its power.runtime_error field is
> > > > - * different from zero, the status may be changed either to RPM_ACTIVE, or to
> > > > - * RPM_SUSPENDED, as long as that reflects the actual state of the device.
> > > > + * If runtime PM of the device is disabled or restrained, or its
> > > > + * power.runtime_error field is nonzero, the status may be changed either to
> > > > + * RPM_ACTIVE, or to RPM_SUSPENDED, as long as that reflects its actual state.
> > > > * However, if the device has a parent and the parent is not active, and the
> > > > * parent's power.ignore_children flag is unset, the device's status cannot be
> > > > * set to RPM_ACTIVE, so -EBUSY is returned in that case.
> > > > @@ -1195,13 +1195,16 @@ int __pm_runtime_set_status(struct devic
> > > > spin_lock_irq(&dev->power.lock);
> > > >
> > > > /*
> > > > - * Prevent PM-runtime from being enabled for the device or return an
> > > > - * error if it is enabled already and working.
> > > > + * Prevent PM-runtime from being used for the device or return an
> > > > + * error if it is in use already.
> > > > */
> > > > - if (dev->power.runtime_error || dev->power.disable_depth)
> > > > - dev->power.disable_depth++;
> > > > - else
> > > > + if (dev->power.runtime_error || dev->power.disable_depth ||
> > > > + dev->power.restrain_depth) {
> > > > + pm_runtime_get_noresume(dev);
> > >
> > > Why do we need to bump the usage count here? Except for balancing with
> > > pm_runtime_relinquish() a few lines below, of course?
> >
> > First off, I need the usage count to be greater than 0 to prevent the
> > runtime suspend callback from running while "restrained" (and the
> > suspend could check the restrain count, but that's one more check in
> > the suspend path which isn't necessary if the usage counter is always
> > bumped up upfront).
>
> If disable_depth > 0 (or restrain_depth > 0), the runtime PM core
> should prevent the runtime suspend callback from being invoked, no
> matter whether the usage count has been bumped or not. Or did I get
> that wrong?
Yes, that's right. I guess I've tried to over-optimize the
system-wide suspend-resume case.
> >
> > Second, the PM core bumps up the usage counter during system-wide
> > suspend, so bumping it up again isn't strictly needed if this
> > "temporary disabling" is limited to the system-wide suspend-resume
> > paths, but I'm not sure if it should be limited this way.
> >
> > I would prefer the "temporarily unavailable" case to be clearly
> > different from the "disabled" one in any case, not just during
> > system-wide suspend-resume.
> >
> > > > + dev->power.restrain_depth++;
> > > > + } else {
> > > > error = -EAGAIN;
> > > > + }
> > > >
> > > > spin_unlock_irq(&dev->power.lock);
> > > >
> > > > @@ -1278,7 +1281,7 @@ int __pm_runtime_set_status(struct devic
> > > > device_links_read_unlock(idx);
> > > > }
> > > >
> > > > - pm_runtime_enable(dev);
> > > > + pm_runtime_relinquish(dev);
> > > >
> > > > return error;
> > > > }
> > > > @@ -1513,6 +1516,72 @@ void pm_runtime_allow(struct device *dev
> > > > EXPORT_SYMBOL_GPL(pm_runtime_allow);
> > > >
> > > > /**
> > > > + * pm_runtime_restrain - Temporarily block runtime PM of a device.
> > > > + * @dev: Device to handle.
> > > > + *
> > > > + * Increase the device's usage count and its restrain_dpeth count. If the
> > > > + * latter was 0 initially, cancel the runtime PM work for @dev if pending and
> > > > + * wait for all of the runtime PM operations on it in progress to complete.
> > > > + *
> > > > + * After this function has been called, attempts to runtime-suspend @dev will
> > > > + * fail with -EAGAIN and attempts to runtime-resume it will succeed if its
> > > > + * runtime PM status is RPM_ACTIVE and will fail with -EAGAIN otherwise.
> > > > + *
> > > > + * This function can only be called by the PM core.
> > > > + */
> > > > +void pm_runtime_restrain(struct device *dev)
> > > > +{
> > > > + pm_runtime_get_noresume(dev);
> > > > +
> > > > + spin_lock_irq(&dev->power.lock);
> > > > +
> > > > + if (dev->power.restrain_depth++ > 0)
> > > > + goto out;
> > > > +
> > > > + if (dev->power.disable_depth > 0) {
> > > > + dev->power.already_suspended = false;
> > > > + goto out;
> > > > + }
> > > > +
> > > > + /* Update time accounting before blocking PM-runtime. */
> > > > + update_pm_runtime_accounting(dev);
> > > > +
> > > > + __pm_runtime_barrier(dev);
> > > > +
> > > > + dev->power.already_suspended = pm_runtime_status_suspended(dev);
> > > > +
> > > > +out:
> > > > + spin_unlock_irq(&dev->power.lock);
> > > > +}
> > >
> > > What if someone calls pm_runtime_disable() after the PM core has
> > > called pm_runtime_restrain() for a device? It looks like we may run
> > > another round of __pm_runtime_barrier() and
> > > update_pm_runtime_accounting(), does that really make sense?
> >
> > No, it doesn't, but it's a bug in the patch. And there are other bugs in it ...
> >
> > In this particular case, __pm_runtime_disable() should check the
> > "restrain" count and do nothing when it is nonzero.
> >
> > > > +
> > > > +/**
> > > > + * pm_runtime_relinquish - Unblock runtime PM of a device.
> > > > + * @dev: Device to handle.
> > > > + *
> > > > + * Decrease the device's usage count and its restrain_dpeth count.
> > > > + *
> > > > + * This function can only be called by the PM core.
> > > > + */
> > > > +void pm_runtime_relinquish(struct device *dev)
> > > > +{
> > > > + spin_lock_irq(&dev->power.lock);
> > > > +
> > > > + if (dev->power.restrain_depth > 0) {
> > > > + dev->power.restrain_depth--;
> > > > +
> > > > + /* About to unbolck runtime PM, set accounting_timestamp to now */
> > > > + if (!dev->power.restrain_depth && !dev->power.disable_depth)
> > > > + dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
> > > > + } else {
> > > > + dev_warn(dev, "Unbalanced %s!\n", __func__);
> > > > + }
> > > > +
> > > > + spin_unlock_irq(&dev->power.lock);
> > > > +
> > > > + pm_runtime_put_noidle(dev);
> > > > +}
> > > > +
> > > > +/**
> > > > * pm_runtime_no_callbacks - Ignore runtime PM callbacks for a device.
> > > > * @dev: Device to handle.
> > > > *
> > > > @@ -1806,8 +1875,10 @@ int pm_runtime_force_suspend(struct devi
> > > > int (*callback)(struct device *);
> > > > int ret;
> > > >
> > > > - pm_runtime_disable(dev);
> > > > - if (pm_runtime_status_suspended(dev))
> > > > + pm_runtime_restrain(dev);
> > > > +
> > > > + /* No suspend if the device has already been suspended by PM-runtime. */
> > > > + if (!dev->power.already_suspended)
> > >
> > > I assume you are looking at using pm_runtime_force_suspend|resume() to
> > > support my use case for the cpuidle-psci driver? In other words,
> > > replace pm_runtime_get_sync() and pm_runtime_put_sync_suspend() in
> > > __psci_enter_domain_idle_state(), right?
> >
> > Not really. I've been looking at a general "temporarily unavailable"
> > vs "disabled" problem.
>
> Okay, so I understand that you want to distinguish between these two
> cases, but honestly I fail to understand *why* that is needed, sorry.
Well, see above for the reason. :-)
>
> >
> > > If so, that doesn't really fit well, I think. Not only because we
> > > don't have system suspend/resume callbacks available, which is really
> > > the proper place to call the pm_runtime_force_*() functions from, but
> > > also because we don't want to call __pm_runtime_barrier(), etc, every
> > > time in the idle path of a CPU. If anything, we should instead strive
> > > towards a more lightweight path than what we currently have.
> >
> > So IMO this can be done with the new counter in place, because for
> > anything called between device_suspend_late() and
> > device_resume_early(), PM-runtime would be restrained by the PM core
> > (it is disabled now), so rpm_resume() would return 1 for devices with
> > PM-runtime status equal to RPM_ACTIVE (it fails now, unless the usage
> > counter is exactly 1) and you resume the devices in question upfront,
> > so it would be always safe to call rpm_resume() and rpm_suspend() for
> > them during the noirq suspend and resume phases (it is now tricky,
> > because it depends on the exact usage counter value).
> >
> > Between dpm_suspend_noirq() and dpm_resume_noirq(), you need to switch
> > over to a different type of handling anyway, because all of the
> > devices are expected to be suspended then.
>
> Not sure I understand correctly, but I don't think I need to switch to
> another handling. The devices in __psci_enter_domain_idle_state() are
> managed as syscore devices with genpd, for the later system suspend
> phases, this works well.
>
> Perhaps you also saying that the goal with your change is to allow
> rpm_resume() to return 1, when the state is RPM_ACTIVE for the device
> and when the PM core has called pm_runtime_restrain() instead of
> __pm_runtime_disable()? Right?
Basically, yes, but generally that's a mechanism by which callbacks
may be prevented from running without confusion with full disabling of
runtime PM.
On Tue, 30 Nov 2021 at 18:26, Rafael J. Wysocki <[email protected]> wrote:
>
> On Tue, Nov 30, 2021 at 5:41 PM Ulf Hansson <[email protected]> wrote:
> >
> > On Tue, 30 Nov 2021 at 14:02, Rafael J. Wysocki <[email protected]> wrote:
> > >
> > > On Tue, Nov 30, 2021 at 12:58 PM Ulf Hansson <[email protected]> wrote:
> > > >
> > > > [...]
> > > >
> > > > > > > > > >
> > > > > > > > > > Am I thinking correctly that this is mostly about working around the
> > > > > > > > > > limitations of pm_runtime_force_suspend()?
> > > > > > > > >
> > > > > > > > > No, this isn't related at all.
> > > > > > > > >
> > > > > > > > > The cpuidle-psci driver doesn't have PM callbacks, thus using
> > > > > > > > > pm_runtime_force_suspend() would not work here.
> > > > > > > >
> > > > > > > > Just wanted to send a ping on this to see if we can come to a
> > > > > > > > conclusion. Or maybe we did? :-)
> > > > > > > >
> > > > > > > > I think in the end, what slightly bothers me, is that the behavior is
> > > > > > > > a bit inconsistent. Although, maybe it's the best we can do.
> > > > > > >
> > > > > > > I've been thinking about this and it looks like we can do better, but
> > > > > > > instead of talking about this I'd rather send a patch.
> > > > > >
> > > > > > Alright.
> > > > > >
> > > > > > I was thinking along the lines of make similar changes for
> > > > > > rpm_idle|suspend(). That would make the behaviour even more
> > > > > > consistent, I think.
> > > > > >
> > > > > > Perhaps that's what you have in mind? :-)
> > > > >
> > > > > Well, not exactly.
> > > > >
> > > > > The idea is to add another counter (called restrain_depth in the patch)
> > > > > to prevent rpm_resume() from running the callback when that is potentially
> > > > > problematic. With that, it is possible to actually distinguish devices
> > > > > with PM-runtime enabled and it allows the PM-runtime status to be checked
> > > > > when it is still known to be meaningful.
> > > >
> > > > Hmm, I don't quite understand the benefit of introducing a new flag
> > > > for this. rpm_resume() already checks the disable_depth to understand
> > > > when it's safe to invoke the callback. Maybe there is a reason why
> > > > that isn't sufficient?
> > >
> > > The problem is that disable_depth > 0 may very well mean that runtime
> > > PM has not been enabled at all for the given device which IMO is a
> > > problem.
> > >
> > > As it stands, it is necessary to make assumptions, like disable_depth
> > > == 1 meaning that runtime PM is really enabled, but the PM core has
> > > disabled it temporarily, which is somewhat questionable.
> > >
> > > Another problem with disabling is that it causes rpm_resume() to fail
> > > even if the status is RPM_ACTIVE and it has to do that exactly because
> > > it cannot know why runtime PM has been disabled. If it has never been
> > > enabled, rpm_resume() must fail, but if it has been disabled
> > > temporarily, rpm_resume() may return 1 when the status is RPM_ACTIVE.
> > >
> > > The new count allows the "enabled in general, but temporarily disabled
> > > at the moment" to be handled cleanly.
> >
> > My overall comment is that I fail to understand why we need to
> > distinguish between these two cases. To me, it shouldn't really
> > matter, *why* runtime PM is (or have been) disabled for the device.
>
> It matters if you want to trust the status, because "disabled" means
> "the status doesn't matter".
Well, that doesn't really match how the runtime PM interface is being
used today.
For example, we have a whole bunch of helper functions, allowing us to
update and check the runtime PM state of the device, even when the
disable_depth > 0. Some functions, like pm_runtime_set_active() for
example, even take parents and device-links into account.
>
> If you want the status to stay meaningful, but prevent callbacks from
> running, you need something else.
>
> > The important point is that the default state for a device is
> > RPM_SUSPENDED and someone has moved into RPM_ACTIVE, for whatever
> > reason. That should be sufficient to allow rpm_resume() to return '1'
> > when disable_depth > 0, shouldn't it?
>
> No, because there is no rule by which the status of devices with
> PM-runtime disabled must be RPM_SUSPENDED.
That's not what I was trying to say.
The initial/default runtime PM state for a device is RPM_SUSPENDED,
which is being set in pm_runtime_init(). Although, I agree that it
can't be trusted that this state actually reflects the state of the
HW, it's still a valid state for the device from a runtime PM point of
view.
However, and more importantly, if the state has moved to RPM_ACTIVE,
someone must have deliberately moved the device into that state. For
this reason, I believe it seems reasonable to trust it, both from HW
point of view, but definitely also from a runtime PM point of view. If
not, then what should we do?
[...]
Kind regards
Uffe
On Wed, Dec 1, 2021 at 10:02 AM Ulf Hansson <[email protected]> wrote:
>
> On Tue, 30 Nov 2021 at 18:26, Rafael J. Wysocki <[email protected]> wrote:
> >
> > On Tue, Nov 30, 2021 at 5:41 PM Ulf Hansson <[email protected]> wrote:
> > >
> > > On Tue, 30 Nov 2021 at 14:02, Rafael J. Wysocki <[email protected]> wrote:
> > > >
> > > > On Tue, Nov 30, 2021 at 12:58 PM Ulf Hansson <[email protected]> wrote:
> > > > >
> > > > > [...]
> > > > >
> > > > > > > > > > >
> > > > > > > > > > > Am I thinking correctly that this is mostly about working around the
> > > > > > > > > > > limitations of pm_runtime_force_suspend()?
> > > > > > > > > >
> > > > > > > > > > No, this isn't related at all.
> > > > > > > > > >
> > > > > > > > > > The cpuidle-psci driver doesn't have PM callbacks, thus using
> > > > > > > > > > pm_runtime_force_suspend() would not work here.
> > > > > > > > >
> > > > > > > > > Just wanted to send a ping on this to see if we can come to a
> > > > > > > > > conclusion. Or maybe we did? :-)
> > > > > > > > >
> > > > > > > > > I think in the end, what slightly bothers me, is that the behavior is
> > > > > > > > > a bit inconsistent. Although, maybe it's the best we can do.
> > > > > > > >
> > > > > > > > I've been thinking about this and it looks like we can do better, but
> > > > > > > > instead of talking about this I'd rather send a patch.
> > > > > > >
> > > > > > > Alright.
> > > > > > >
> > > > > > > I was thinking along the lines of make similar changes for
> > > > > > > rpm_idle|suspend(). That would make the behaviour even more
> > > > > > > consistent, I think.
> > > > > > >
> > > > > > > Perhaps that's what you have in mind? :-)
> > > > > >
> > > > > > Well, not exactly.
> > > > > >
> > > > > > The idea is to add another counter (called restrain_depth in the patch)
> > > > > > to prevent rpm_resume() from running the callback when that is potentially
> > > > > > problematic. With that, it is possible to actually distinguish devices
> > > > > > with PM-runtime enabled and it allows the PM-runtime status to be checked
> > > > > > when it is still known to be meaningful.
> > > > >
> > > > > Hmm, I don't quite understand the benefit of introducing a new flag
> > > > > for this. rpm_resume() already checks the disable_depth to understand
> > > > > when it's safe to invoke the callback. Maybe there is a reason why
> > > > > that isn't sufficient?
> > > >
> > > > The problem is that disable_depth > 0 may very well mean that runtime
> > > > PM has not been enabled at all for the given device which IMO is a
> > > > problem.
> > > >
> > > > As it stands, it is necessary to make assumptions, like disable_depth
> > > > == 1 meaning that runtime PM is really enabled, but the PM core has
> > > > disabled it temporarily, which is somewhat questionable.
> > > >
> > > > Another problem with disabling is that it causes rpm_resume() to fail
> > > > even if the status is RPM_ACTIVE and it has to do that exactly because
> > > > it cannot know why runtime PM has been disabled. If it has never been
> > > > enabled, rpm_resume() must fail, but if it has been disabled
> > > > temporarily, rpm_resume() may return 1 when the status is RPM_ACTIVE.
> > > >
> > > > The new count allows the "enabled in general, but temporarily disabled
> > > > at the moment" to be handled cleanly.
> > >
> > > My overall comment is that I fail to understand why we need to
> > > distinguish between these two cases. To me, it shouldn't really
> > > matter, *why* runtime PM is (or have been) disabled for the device.
> >
> > It matters if you want to trust the status, because "disabled" means
> > "the status doesn't matter".
>
> Well, that doesn't really match how the runtime PM interface is being
> used today.
Well, I clearly disagree.
> For example, we have a whole bunch of helper functions, allowing us to
> update and check the runtime PM state of the device, even when the
> disable_depth > 0. Some functions, like pm_runtime_set_active() for
> example, even take parents and device-links into account.
That's true, but that's for a purpose.
If runtime PM becomes enabled after using pm_runtime_set_active(), the
status should better be consistent with the settings of the parent
etc.
> >
> > If you want the status to stay meaningful, but prevent callbacks from
> > running, you need something else.
> >
> > > The important point is that the default state for a device is
> > > RPM_SUSPENDED and someone has moved into RPM_ACTIVE, for whatever
> > > reason. That should be sufficient to allow rpm_resume() to return '1'
> > > when disable_depth > 0, shouldn't it?
> >
> > No, because there is no rule by which the status of devices with
> > PM-runtime disabled must be RPM_SUSPENDED.
>
> That's not what I was trying to say.
>
> The initial/default runtime PM state for a device is RPM_SUSPENDED,
> which is being set in pm_runtime_init(). Although, I agree that it
> can't be trusted that this state actually reflects the state of the
> HW, it's still a valid state for the device from a runtime PM point of
> view.
No, it is not. It's just the default.
> However, and more importantly, if the state has moved to RPM_ACTIVE,
> someone must have deliberately moved the device into that state.
Sure, but it cannot be regarded as an indication on whether or not
runtime PM is supported and has ever been enabled for the given
device.
Again, there is no rule regarding the status value for devices with
runtime PM disabled, either way.
> For this reason, I believe it seems reasonable to trust it, both from HW
> point of view, but definitely also from a runtime PM point of view. If
> not, then what should we do?
Trust it only when runtime PM is enabled, ie. dev->power.disable_depth == 0.
That's exactly the reason why pm_runtime_suspended() returns false if
runtime PM is disabled for the target device and if
pm_runtime_suspended() is called during system-wide suspend and
resume, it is not clear how to interpret its return value.
If it returns true outside the system suspend-resume code path, that
means "runtime PM has been enabled and the device has been
runtime-suspended" and I want it to mean exactly the same thing during
system-wide suspend and resume, so people don't need to wonder about
the context in which the code is running.
On Wed, 1 Dec 2021 at 14:49, Rafael J. Wysocki <[email protected]> wrote:
>
> On Wed, Dec 1, 2021 at 10:02 AM Ulf Hansson <[email protected]> wrote:
> >
> > On Tue, 30 Nov 2021 at 18:26, Rafael J. Wysocki <[email protected]> wrote:
> > >
> > > On Tue, Nov 30, 2021 at 5:41 PM Ulf Hansson <[email protected]> wrote:
> > > >
> > > > On Tue, 30 Nov 2021 at 14:02, Rafael J. Wysocki <[email protected]> wrote:
> > > > >
> > > > > On Tue, Nov 30, 2021 at 12:58 PM Ulf Hansson <[email protected]> wrote:
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > Am I thinking correctly that this is mostly about working around the
> > > > > > > > > > > > limitations of pm_runtime_force_suspend()?
> > > > > > > > > > >
> > > > > > > > > > > No, this isn't related at all.
> > > > > > > > > > >
> > > > > > > > > > > The cpuidle-psci driver doesn't have PM callbacks, thus using
> > > > > > > > > > > pm_runtime_force_suspend() would not work here.
> > > > > > > > > >
> > > > > > > > > > Just wanted to send a ping on this to see if we can come to a
> > > > > > > > > > conclusion. Or maybe we did? :-)
> > > > > > > > > >
> > > > > > > > > > I think in the end, what slightly bothers me, is that the behavior is
> > > > > > > > > > a bit inconsistent. Although, maybe it's the best we can do.
> > > > > > > > >
> > > > > > > > > I've been thinking about this and it looks like we can do better, but
> > > > > > > > > instead of talking about this I'd rather send a patch.
> > > > > > > >
> > > > > > > > Alright.
> > > > > > > >
> > > > > > > > I was thinking along the lines of make similar changes for
> > > > > > > > rpm_idle|suspend(). That would make the behaviour even more
> > > > > > > > consistent, I think.
> > > > > > > >
> > > > > > > > Perhaps that's what you have in mind? :-)
> > > > > > >
> > > > > > > Well, not exactly.
> > > > > > >
> > > > > > > The idea is to add another counter (called restrain_depth in the patch)
> > > > > > > to prevent rpm_resume() from running the callback when that is potentially
> > > > > > > problematic. With that, it is possible to actually distinguish devices
> > > > > > > with PM-runtime enabled and it allows the PM-runtime status to be checked
> > > > > > > when it is still known to be meaningful.
> > > > > >
> > > > > > Hmm, I don't quite understand the benefit of introducing a new flag
> > > > > > for this. rpm_resume() already checks the disable_depth to understand
> > > > > > when it's safe to invoke the callback. Maybe there is a reason why
> > > > > > that isn't sufficient?
> > > > >
> > > > > The problem is that disable_depth > 0 may very well mean that runtime
> > > > > PM has not been enabled at all for the given device which IMO is a
> > > > > problem.
> > > > >
> > > > > As it stands, it is necessary to make assumptions, like disable_depth
> > > > > == 1 meaning that runtime PM is really enabled, but the PM core has
> > > > > disabled it temporarily, which is somewhat questionable.
> > > > >
> > > > > Another problem with disabling is that it causes rpm_resume() to fail
> > > > > even if the status is RPM_ACTIVE and it has to do that exactly because
> > > > > it cannot know why runtime PM has been disabled. If it has never been
> > > > > enabled, rpm_resume() must fail, but if it has been disabled
> > > > > temporarily, rpm_resume() may return 1 when the status is RPM_ACTIVE.
> > > > >
> > > > > The new count allows the "enabled in general, but temporarily disabled
> > > > > at the moment" to be handled cleanly.
> > > >
> > > > My overall comment is that I fail to understand why we need to
> > > > distinguish between these two cases. To me, it shouldn't really
> > > > matter, *why* runtime PM is (or have been) disabled for the device.
> > >
> > > It matters if you want to trust the status, because "disabled" means
> > > "the status doesn't matter".
> >
> > Well, that doesn't really match how the runtime PM interface is being
> > used today.
>
> Well, I clearly disagree.
Alright, then we can agree to disagree. :-)
>
> > For example, we have a whole bunch of helper functions, allowing us to
> > update and check the runtime PM state of the device, even when the
> > disable_depth > 0. Some functions, like pm_runtime_set_active() for
> > example, even take parents and device-links into account.
>
> That's true, but that's for a purpose.
>
> If runtime PM becomes enabled after using pm_runtime_set_active(), the
> status should better be consistent with the settings of the parent
> etc.
>
> > >
> > > If you want the status to stay meaningful, but prevent callbacks from
> > > running, you need something else.
> > >
> > > > The important point is that the default state for a device is
> > > > RPM_SUSPENDED and someone has moved into RPM_ACTIVE, for whatever
> > > > reason. That should be sufficient to allow rpm_resume() to return '1'
> > > > when disable_depth > 0, shouldn't it?
> > >
> > > No, because there is no rule by which the status of devices with
> > > PM-runtime disabled must be RPM_SUSPENDED.
> >
> > That's not what I was trying to say.
> >
> > The initial/default runtime PM state for a device is RPM_SUSPENDED,
> > which is being set in pm_runtime_init(). Although, I agree that it
> > can't be trusted that this state actually reflects the state of the
> > HW, it's still a valid state for the device from a runtime PM point of
> > view.
>
> No, it is not. It's just the default.
>
> > However, and more importantly, if the state has moved to RPM_ACTIVE,
> > someone must have deliberately moved the device into that state.
>
> Sure, but it cannot be regarded as an indication on whether or not
> runtime PM is supported and has ever been enabled for the given
> device.
>
> Again, there is no rule regarding the status value for devices with
> runtime PM disabled, either way.
If I understand correctly, that means you think the
pm_runtime_status_suspended() should really be converted to an
internal runtime PM interface, not being exported to users outside.
Right?
When it comes to the pm_runtime_active() interface, I noticed that
it's actually completely broken, as it returns "true" when runtime PM
has been disabled for the device - no matter whether the state is
RPM_ACTIVE or not. It's quite heavily used, so I guess the behaviour
is what the callers expect?
>
> > For this reason, I believe it seems reasonable to trust it, both from HW
> > point of view, but definitely also from a runtime PM point of view. If
> > not, then what should we do?
>
> Trust it only when runtime PM is enabled, ie. dev->power.disable_depth == 0.
>
> That's exactly the reason why pm_runtime_suspended() returns false if
> runtime PM is disabled for the target device and if
> pm_runtime_suspended() is called during system-wide suspend and
> resume, it is not clear how to interpret its return value.
To me, I don't think there is a problem interpreting
pm_runtime_suspended()'s return value. As you said, it tells whether
runtime PM has been enabled and whether the state is RPM_SUSPENDED.
For those that want to know the state when runtime PM has been
disabled, they can use pm_runtime_status_suspended().
What's the issue?
>
> If it returns true outside the system suspend-resume code path, that
> means "runtime PM has been enabled and the device has been
> runtime-suspended" and I want it to mean exactly the same thing during
> system-wide suspend and resume, so people don't need to wonder about
> the context in which the code is running.
Well, even if this seems nice, I wonder how useful this would be in the end.
Perhaps you have some concrete examples where this could improve things?
Kind regards
Uffe
On Wed, Dec 1, 2021 at 4:23 PM Ulf Hansson <[email protected]> wrote:
>
> On Wed, 1 Dec 2021 at 14:49, Rafael J. Wysocki <[email protected]> wrote:
> >
> > On Wed, Dec 1, 2021 at 10:02 AM Ulf Hansson <[email protected]> wrote:
> > >
> > > On Tue, 30 Nov 2021 at 18:26, Rafael J. Wysocki <[email protected]> wrote:
> > > >
> > > > On Tue, Nov 30, 2021 at 5:41 PM Ulf Hansson <[email protected]> wrote:
> > > > >
> > > > > On Tue, 30 Nov 2021 at 14:02, Rafael J. Wysocki <[email protected]> wrote:
> > > > > >
> > > > > > On Tue, Nov 30, 2021 at 12:58 PM Ulf Hansson <[email protected]> wrote:
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > Am I thinking correctly that this is mostly about working around the
> > > > > > > > > > > > > limitations of pm_runtime_force_suspend()?
> > > > > > > > > > > >
> > > > > > > > > > > > No, this isn't related at all.
> > > > > > > > > > > >
> > > > > > > > > > > > The cpuidle-psci driver doesn't have PM callbacks, thus using
> > > > > > > > > > > > pm_runtime_force_suspend() would not work here.
> > > > > > > > > > >
> > > > > > > > > > > Just wanted to send a ping on this to see if we can come to a
> > > > > > > > > > > conclusion. Or maybe we did? :-)
> > > > > > > > > > >
> > > > > > > > > > > I think in the end, what slightly bothers me, is that the behavior is
> > > > > > > > > > > a bit inconsistent. Although, maybe it's the best we can do.
> > > > > > > > > >
> > > > > > > > > > I've been thinking about this and it looks like we can do better, but
> > > > > > > > > > instead of talking about this I'd rather send a patch.
> > > > > > > > >
> > > > > > > > > Alright.
> > > > > > > > >
> > > > > > > > > I was thinking along the lines of make similar changes for
> > > > > > > > > rpm_idle|suspend(). That would make the behaviour even more
> > > > > > > > > consistent, I think.
> > > > > > > > >
> > > > > > > > > Perhaps that's what you have in mind? :-)
> > > > > > > >
> > > > > > > > Well, not exactly.
> > > > > > > >
> > > > > > > > The idea is to add another counter (called restrain_depth in the patch)
> > > > > > > > to prevent rpm_resume() from running the callback when that is potentially
> > > > > > > > problematic. With that, it is possible to actually distinguish devices
> > > > > > > > with PM-runtime enabled and it allows the PM-runtime status to be checked
> > > > > > > > when it is still known to be meaningful.
> > > > > > >
> > > > > > > Hmm, I don't quite understand the benefit of introducing a new flag
> > > > > > > for this. rpm_resume() already checks the disable_depth to understand
> > > > > > > when it's safe to invoke the callback. Maybe there is a reason why
> > > > > > > that isn't sufficient?
> > > > > >
> > > > > > The problem is that disable_depth > 0 may very well mean that runtime
> > > > > > PM has not been enabled at all for the given device which IMO is a
> > > > > > problem.
> > > > > >
> > > > > > As it stands, it is necessary to make assumptions, like disable_depth
> > > > > > == 1 meaning that runtime PM is really enabled, but the PM core has
> > > > > > disabled it temporarily, which is somewhat questionable.
> > > > > >
> > > > > > Another problem with disabling is that it causes rpm_resume() to fail
> > > > > > even if the status is RPM_ACTIVE and it has to do that exactly because
> > > > > > it cannot know why runtime PM has been disabled. If it has never been
> > > > > > enabled, rpm_resume() must fail, but if it has been disabled
> > > > > > temporarily, rpm_resume() may return 1 when the status is RPM_ACTIVE.
> > > > > >
> > > > > > The new count allows the "enabled in general, but temporarily disabled
> > > > > > at the moment" to be handled cleanly.
> > > > >
> > > > > My overall comment is that I fail to understand why we need to
> > > > > distinguish between these two cases. To me, it shouldn't really
> > > > > matter, *why* runtime PM is (or have been) disabled for the device.
> > > >
> > > > It matters if you want to trust the status, because "disabled" means
> > > > "the status doesn't matter".
> > >
> > > Well, that doesn't really match how the runtime PM interface is being
> > > used today.
> >
> > Well, I clearly disagree.
>
> Alright, then we can agree to disagree. :-)
>
> >
> > > For example, we have a whole bunch of helper functions, allowing us to
> > > update and check the runtime PM state of the device, even when the
> > > disable_depth > 0. Some functions, like pm_runtime_set_active() for
> > > example, even take parents and device-links into account.
> >
> > That's true, but that's for a purpose.
> >
> > If runtime PM becomes enabled after using pm_runtime_set_active(), the
> > status should better be consistent with the settings of the parent
> > etc.
> >
> > > >
> > > > If you want the status to stay meaningful, but prevent callbacks from
> > > > running, you need something else.
> > > >
> > > > > The important point is that the default state for a device is
> > > > > RPM_SUSPENDED and someone has moved into RPM_ACTIVE, for whatever
> > > > > reason. That should be sufficient to allow rpm_resume() to return '1'
> > > > > when disable_depth > 0, shouldn't it?
> > > >
> > > > No, because there is no rule by which the status of devices with
> > > > PM-runtime disabled must be RPM_SUSPENDED.
> > >
> > > That's not what I was trying to say.
> > >
> > > The initial/default runtime PM state for a device is RPM_SUSPENDED,
> > > which is being set in pm_runtime_init(). Although, I agree that it
> > > can't be trusted that this state actually reflects the state of the
> > > HW, it's still a valid state for the device from a runtime PM point of
> > > view.
> >
> > No, it is not. It's just the default.
> >
> > > However, and more importantly, if the state has moved to RPM_ACTIVE,
> > > someone must have deliberately moved the device into that state.
> >
> > Sure, but it cannot be regarded as an indication on whether or not
> > runtime PM is supported and has ever been enabled for the given
> > device.
> >
> > Again, there is no rule regarding the status value for devices with
> > runtime PM disabled, either way.
>
> If I understand correctly, that means you think the
> pm_runtime_status_suspended() should really be converted to an
> internal runtime PM interface, not being exported to users outside.
> Right?
Not really.
I'm just saying that its usefulness is limited.
My basic concern is that system-wide PM transitions must always invoke
callbacks for devices with PM-runtime disabled, because they may (or
may not) be functional regardless of the PM-runtime status and if they
are functional, they must be suspended. And note that supporting
system-wide PM is not optional and the only way to kind of disable it
is to return an error from a device suspend callback (but that's nasty
for some use cases).
So the "Has PM-runtime been enabled?" question is really fundamental
for system-wide PM and it is not sufficient to look at the PM-runtime
status to find out, but if the PM-core itself disables PM-runtime
(which is has to do at one point to prevent PM-runtime from racing
with system-wide PM), it is hard to answer definitely in general.
IMO the only way to make it possible to find that out in all cases is
to make the PM core retain the power.disable_depth value and that can
be done by making it use a different mechanism to prevent PM-runtime
callbacks from being run.
Alternatively, the current PM-runtime status could be "latched" during
the PM-runtime disable operation if power.disable_depth is 0 (and that
"latched" value would be initialized to "invalid" in case PM-runtime
is never enabled).
> When it comes to the pm_runtime_active() interface, I noticed that
> it's actually completely broken, as it returns "true" when runtime PM
> has been disabled for the device - no matter whether the state is
> RPM_ACTIVE or not. It's quite heavily used, so I guess the behaviour
> is what the callers expect?
I'm not sure about that after looking at some of them briefly.
I'm guessing that the original author of it wanted to use it in the
system-wide suspend path to decide whether or not to suspend the
device, but that obviously interferes with what the PM core does.
Anway, it looks like its name should be changed to something like
pm_runtime_active_or_disabled(), so it reflects the functionality.
> >
> > > For this reason, I believe it seems reasonable to trust it, both from HW
> > > point of view, but definitely also from a runtime PM point of view. If
> > > not, then what should we do?
> >
> > Trust it only when runtime PM is enabled, ie. dev->power.disable_depth == 0.
> >
> > That's exactly the reason why pm_runtime_suspended() returns false if
> > runtime PM is disabled for the target device and if
> > pm_runtime_suspended() is called during system-wide suspend and
> > resume, it is not clear how to interpret its return value.
>
> To me, I don't think there is a problem interpreting
> pm_runtime_suspended()'s return value. As you said, it tells whether
> runtime PM has been enabled and whether the state is RPM_SUSPENDED.
>
> For those that want to know the state when runtime PM has been
> disabled, they can use pm_runtime_status_suspended().
>
> What's the issue?
It may not match the real state of the device which may be relevant.
> >
> > If it returns true outside the system suspend-resume code path, that
> > means "runtime PM has been enabled and the device has been
> > runtime-suspended" and I want it to mean exactly the same thing during
> > system-wide suspend and resume, so people don't need to wonder about
> > the context in which the code is running.
>
> Well, even if this seems nice, I wonder how useful this would be in the end.
>
> Perhaps you have some concrete examples where this could improve things?
For example, what if pm_runtime_force_suspend() is called for a device
with PM-runtime disabled and the PM-runtime status equal to
RPM_SUSPENDED? As I said above, the callback must be invoked for it
in case it is functional and needs to be suspended, but that won't
happen AFAICS.
However, if pm_runtime_force_suspend() can tell whether or not
PM-runtime has been enabled for the device, it can do the right thing
(basically, avoid running the callback if it has been run once already
by PM-runtime, which is the case when PM-runtime is enabled and the
status is RPM_SUSPENDED).
On Wednesday, December 1, 2021 6:44:08 PM CET Rafael J. Wysocki wrote:
> On Wed, Dec 1, 2021 at 4:23 PM Ulf Hansson <[email protected]> wrote:
> >
> > On Wed, 1 Dec 2021 at 14:49, Rafael J. Wysocki <[email protected]> wrote:
> > >
> > > On Wed, Dec 1, 2021 at 10:02 AM Ulf Hansson <[email protected]> wrote:
> > > >
> > > > On Tue, 30 Nov 2021 at 18:26, Rafael J. Wysocki <[email protected]> wrote:
> > > > >
> > > > > On Tue, Nov 30, 2021 at 5:41 PM Ulf Hansson <[email protected]> wrote:
> > > > > >
> > > > > > On Tue, 30 Nov 2021 at 14:02, Rafael J. Wysocki <[email protected]> wrote:
> > > > > > >
> > > > > > > On Tue, Nov 30, 2021 at 12:58 PM Ulf Hansson <[email protected]> wrote:
> > > > > > > >
> > > > > > > > [...]
> > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Am I thinking correctly that this is mostly about working around the
> > > > > > > > > > > > > > limitations of pm_runtime_force_suspend()?
> > > > > > > > > > > > >
> > > > > > > > > > > > > No, this isn't related at all.
> > > > > > > > > > > > >
> > > > > > > > > > > > > The cpuidle-psci driver doesn't have PM callbacks, thus using
> > > > > > > > > > > > > pm_runtime_force_suspend() would not work here.
> > > > > > > > > > > >
> > > > > > > > > > > > Just wanted to send a ping on this to see if we can come to a
> > > > > > > > > > > > conclusion. Or maybe we did? :-)
> > > > > > > > > > > >
> > > > > > > > > > > > I think in the end, what slightly bothers me, is that the behavior is
> > > > > > > > > > > > a bit inconsistent. Although, maybe it's the best we can do.
> > > > > > > > > > >
> > > > > > > > > > > I've been thinking about this and it looks like we can do better, but
> > > > > > > > > > > instead of talking about this I'd rather send a patch.
> > > > > > > > > >
> > > > > > > > > > Alright.
> > > > > > > > > >
> > > > > > > > > > I was thinking along the lines of make similar changes for
> > > > > > > > > > rpm_idle|suspend(). That would make the behaviour even more
> > > > > > > > > > consistent, I think.
> > > > > > > > > >
> > > > > > > > > > Perhaps that's what you have in mind? :-)
> > > > > > > > >
> > > > > > > > > Well, not exactly.
> > > > > > > > >
> > > > > > > > > The idea is to add another counter (called restrain_depth in the patch)
> > > > > > > > > to prevent rpm_resume() from running the callback when that is potentially
> > > > > > > > > problematic. With that, it is possible to actually distinguish devices
> > > > > > > > > with PM-runtime enabled and it allows the PM-runtime status to be checked
> > > > > > > > > when it is still known to be meaningful.
> > > > > > > >
> > > > > > > > Hmm, I don't quite understand the benefit of introducing a new flag
> > > > > > > > for this. rpm_resume() already checks the disable_depth to understand
> > > > > > > > when it's safe to invoke the callback. Maybe there is a reason why
> > > > > > > > that isn't sufficient?
> > > > > > >
> > > > > > > The problem is that disable_depth > 0 may very well mean that runtime
> > > > > > > PM has not been enabled at all for the given device which IMO is a
> > > > > > > problem.
> > > > > > >
> > > > > > > As it stands, it is necessary to make assumptions, like disable_depth
> > > > > > > == 1 meaning that runtime PM is really enabled, but the PM core has
> > > > > > > disabled it temporarily, which is somewhat questionable.
> > > > > > >
> > > > > > > Another problem with disabling is that it causes rpm_resume() to fail
> > > > > > > even if the status is RPM_ACTIVE and it has to do that exactly because
> > > > > > > it cannot know why runtime PM has been disabled. If it has never been
> > > > > > > enabled, rpm_resume() must fail, but if it has been disabled
> > > > > > > temporarily, rpm_resume() may return 1 when the status is RPM_ACTIVE.
> > > > > > >
> > > > > > > The new count allows the "enabled in general, but temporarily disabled
> > > > > > > at the moment" to be handled cleanly.
> > > > > >
> > > > > > My overall comment is that I fail to understand why we need to
> > > > > > distinguish between these two cases. To me, it shouldn't really
> > > > > > matter, *why* runtime PM is (or have been) disabled for the device.
> > > > >
> > > > > It matters if you want to trust the status, because "disabled" means
> > > > > "the status doesn't matter".
> > > >
> > > > Well, that doesn't really match how the runtime PM interface is being
> > > > used today.
> > >
> > > Well, I clearly disagree.
> >
> > Alright, then we can agree to disagree. :-)
> >
> > >
> > > > For example, we have a whole bunch of helper functions, allowing us to
> > > > update and check the runtime PM state of the device, even when the
> > > > disable_depth > 0. Some functions, like pm_runtime_set_active() for
> > > > example, even take parents and device-links into account.
> > >
> > > That's true, but that's for a purpose.
> > >
> > > If runtime PM becomes enabled after using pm_runtime_set_active(), the
> > > status should better be consistent with the settings of the parent
> > > etc.
> > >
> > > > >
> > > > > If you want the status to stay meaningful, but prevent callbacks from
> > > > > running, you need something else.
> > > > >
> > > > > > The important point is that the default state for a device is
> > > > > > RPM_SUSPENDED and someone has moved into RPM_ACTIVE, for whatever
> > > > > > reason. That should be sufficient to allow rpm_resume() to return '1'
> > > > > > when disable_depth > 0, shouldn't it?
> > > > >
> > > > > No, because there is no rule by which the status of devices with
> > > > > PM-runtime disabled must be RPM_SUSPENDED.
> > > >
> > > > That's not what I was trying to say.
> > > >
> > > > The initial/default runtime PM state for a device is RPM_SUSPENDED,
> > > > which is being set in pm_runtime_init(). Although, I agree that it
> > > > can't be trusted that this state actually reflects the state of the
> > > > HW, it's still a valid state for the device from a runtime PM point of
> > > > view.
> > >
> > > No, it is not. It's just the default.
> > >
> > > > However, and more importantly, if the state has moved to RPM_ACTIVE,
> > > > someone must have deliberately moved the device into that state.
> > >
> > > Sure, but it cannot be regarded as an indication on whether or not
> > > runtime PM is supported and has ever been enabled for the given
> > > device.
> > >
> > > Again, there is no rule regarding the status value for devices with
> > > runtime PM disabled, either way.
> >
> > If I understand correctly, that means you think the
> > pm_runtime_status_suspended() should really be converted to an
> > internal runtime PM interface, not being exported to users outside.
> > Right?
>
> Not really.
>
> I'm just saying that its usefulness is limited.
>
> My basic concern is that system-wide PM transitions must always invoke
> callbacks for devices with PM-runtime disabled, because they may (or
> may not) be functional regardless of the PM-runtime status and if they
> are functional, they must be suspended. And note that supporting
> system-wide PM is not optional and the only way to kind of disable it
> is to return an error from a device suspend callback (but that's nasty
> for some use cases).
>
> So the "Has PM-runtime been enabled?" question is really fundamental
> for system-wide PM and it is not sufficient to look at the PM-runtime
> status to find out, but if the PM-core itself disables PM-runtime
> (which is has to do at one point to prevent PM-runtime from racing
> with system-wide PM), it is hard to answer definitely in general.
>
> IMO the only way to make it possible to find that out in all cases is
> to make the PM core retain the power.disable_depth value and that can
> be done by making it use a different mechanism to prevent PM-runtime
> callbacks from being run.
>
> Alternatively, the current PM-runtime status could be "latched" during
> the PM-runtime disable operation if power.disable_depth is 0 (and that
> "latched" value would be initialized to "invalid" in case PM-runtime
> is never enabled).
Which would be something like the patch below (which additionally cleans up
pm_runtime_enable() while at it).
The idea being that if the status was RPM_ACTIVE last time when
power.disable_depth was changing from 0 to 1 and it is still RPM_ACTIVE, it
can be assumed to reflect what happened to the device last time when it was
using PM-runtime.
---
drivers/base/power/runtime.c | 38 ++++++++++++++++++++------------------
include/linux/pm.h | 2 ++
2 files changed, 22 insertions(+), 18 deletions(-)
Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -744,11 +744,10 @@ static int rpm_resume(struct device *dev
repeat:
if (dev->power.runtime_error)
retval = -EINVAL;
- else if (dev->power.disable_depth == 1 && dev->power.is_suspended
- && dev->power.runtime_status == RPM_ACTIVE)
- retval = 1;
else if (dev->power.disable_depth > 0)
- retval = -EACCES;
+ retval = dev->power.runtime_status == RPM_ACTIVE &&
+ dev->power.last_status == RPM_ACTIVE ? 1 : -EACCES;
+
if (retval)
goto out;
@@ -1410,8 +1409,10 @@ void __pm_runtime_disable(struct device
/* Update time accounting before disabling PM-runtime. */
update_pm_runtime_accounting(dev);
- if (!dev->power.disable_depth++)
+ if (!dev->power.disable_depth++) {
__pm_runtime_barrier(dev);
+ dev->power.last_status = dev->power.runtime_status;
+ }
out:
spin_unlock_irq(&dev->power.lock);
@@ -1428,23 +1429,23 @@ void pm_runtime_enable(struct device *de
spin_lock_irqsave(&dev->power.lock, flags);
- if (dev->power.disable_depth > 0) {
- dev->power.disable_depth--;
-
- /* About to enable runtime pm, set accounting_timestamp to now */
- if (!dev->power.disable_depth)
- dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
- } else {
+ if (!dev->power.disable_depth) {
dev_warn(dev, "Unbalanced %s!\n", __func__);
+ goto out;
}
- WARN(!dev->power.disable_depth &&
- dev->power.runtime_status == RPM_SUSPENDED &&
- !dev->power.ignore_children &&
- atomic_read(&dev->power.child_count) > 0,
- "Enabling runtime PM for inactive device (%s) with active children\n",
- dev_name(dev));
+ if (--dev->power.disable_depth > 0)
+ goto out;
+
+ dev->power.last_status = RPM_INVALID;
+ dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
+
+ if (dev->power.runtime_status == RPM_SUSPENDED &&
+ !dev->power.ignore_children &&
+ atomic_read(&dev->power.child_count) > 0)
+ dev_warn(dev, "Enabling runtime PM for inactive device with active children\n");
+out:
spin_unlock_irqrestore(&dev->power.lock, flags);
}
EXPORT_SYMBOL_GPL(pm_runtime_enable);
@@ -1640,6 +1641,7 @@ EXPORT_SYMBOL_GPL(__pm_runtime_use_autos
void pm_runtime_init(struct device *dev)
{
dev->power.runtime_status = RPM_SUSPENDED;
+ dev->power.last_status = RPM_INVALID;
dev->power.idle_notification = false;
dev->power.disable_depth = 1;
Index: linux-pm/include/linux/pm.h
===================================================================
--- linux-pm.orig/include/linux/pm.h
+++ linux-pm/include/linux/pm.h
@@ -499,6 +499,7 @@ const struct dev_pm_ops __maybe_unused n
*/
enum rpm_status {
+ RPM_INVALID = -1,
RPM_ACTIVE = 0,
RPM_RESUMING,
RPM_SUSPENDED,
@@ -612,6 +613,7 @@ struct dev_pm_info {
unsigned int links_count;
enum rpm_request request;
enum rpm_status runtime_status;
+ enum rpm_status last_status;
int runtime_error;
int autosuspend_delay;
u64 last_busy;
On Wed, 1 Dec 2021 at 21:11, Rafael J. Wysocki <[email protected]> wrote:
>
> On Wednesday, December 1, 2021 6:44:08 PM CET Rafael J. Wysocki wrote:
> > On Wed, Dec 1, 2021 at 4:23 PM Ulf Hansson <[email protected]> wrote:
> > >
> > > On Wed, 1 Dec 2021 at 14:49, Rafael J. Wysocki <[email protected]> wrote:
> > > >
> > > > On Wed, Dec 1, 2021 at 10:02 AM Ulf Hansson <[email protected]> wrote:
> > > > >
> > > > > On Tue, 30 Nov 2021 at 18:26, Rafael J. Wysocki <[email protected]> wrote:
> > > > > >
> > > > > > On Tue, Nov 30, 2021 at 5:41 PM Ulf Hansson <[email protected]> wrote:
> > > > > > >
> > > > > > > On Tue, 30 Nov 2021 at 14:02, Rafael J. Wysocki <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Tue, Nov 30, 2021 at 12:58 PM Ulf Hansson <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > [...]
> > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Am I thinking correctly that this is mostly about working around the
> > > > > > > > > > > > > > > limitations of pm_runtime_force_suspend()?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > No, this isn't related at all.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > The cpuidle-psci driver doesn't have PM callbacks, thus using
> > > > > > > > > > > > > > pm_runtime_force_suspend() would not work here.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Just wanted to send a ping on this to see if we can come to a
> > > > > > > > > > > > > conclusion. Or maybe we did? :-)
> > > > > > > > > > > > >
> > > > > > > > > > > > > I think in the end, what slightly bothers me, is that the behavior is
> > > > > > > > > > > > > a bit inconsistent. Although, maybe it's the best we can do.
> > > > > > > > > > > >
> > > > > > > > > > > > I've been thinking about this and it looks like we can do better, but
> > > > > > > > > > > > instead of talking about this I'd rather send a patch.
> > > > > > > > > > >
> > > > > > > > > > > Alright.
> > > > > > > > > > >
> > > > > > > > > > > I was thinking along the lines of make similar changes for
> > > > > > > > > > > rpm_idle|suspend(). That would make the behaviour even more
> > > > > > > > > > > consistent, I think.
> > > > > > > > > > >
> > > > > > > > > > > Perhaps that's what you have in mind? :-)
> > > > > > > > > >
> > > > > > > > > > Well, not exactly.
> > > > > > > > > >
> > > > > > > > > > The idea is to add another counter (called restrain_depth in the patch)
> > > > > > > > > > to prevent rpm_resume() from running the callback when that is potentially
> > > > > > > > > > problematic. With that, it is possible to actually distinguish devices
> > > > > > > > > > with PM-runtime enabled and it allows the PM-runtime status to be checked
> > > > > > > > > > when it is still known to be meaningful.
> > > > > > > > >
> > > > > > > > > Hmm, I don't quite understand the benefit of introducing a new flag
> > > > > > > > > for this. rpm_resume() already checks the disable_depth to understand
> > > > > > > > > when it's safe to invoke the callback. Maybe there is a reason why
> > > > > > > > > that isn't sufficient?
> > > > > > > >
> > > > > > > > The problem is that disable_depth > 0 may very well mean that runtime
> > > > > > > > PM has not been enabled at all for the given device which IMO is a
> > > > > > > > problem.
> > > > > > > >
> > > > > > > > As it stands, it is necessary to make assumptions, like disable_depth
> > > > > > > > == 1 meaning that runtime PM is really enabled, but the PM core has
> > > > > > > > disabled it temporarily, which is somewhat questionable.
> > > > > > > >
> > > > > > > > Another problem with disabling is that it causes rpm_resume() to fail
> > > > > > > > even if the status is RPM_ACTIVE and it has to do that exactly because
> > > > > > > > it cannot know why runtime PM has been disabled. If it has never been
> > > > > > > > enabled, rpm_resume() must fail, but if it has been disabled
> > > > > > > > temporarily, rpm_resume() may return 1 when the status is RPM_ACTIVE.
> > > > > > > >
> > > > > > > > The new count allows the "enabled in general, but temporarily disabled
> > > > > > > > at the moment" to be handled cleanly.
> > > > > > >
> > > > > > > My overall comment is that I fail to understand why we need to
> > > > > > > distinguish between these two cases. To me, it shouldn't really
> > > > > > > matter, *why* runtime PM is (or have been) disabled for the device.
> > > > > >
> > > > > > It matters if you want to trust the status, because "disabled" means
> > > > > > "the status doesn't matter".
> > > > >
> > > > > Well, that doesn't really match how the runtime PM interface is being
> > > > > used today.
> > > >
> > > > Well, I clearly disagree.
> > >
> > > Alright, then we can agree to disagree. :-)
> > >
> > > >
> > > > > For example, we have a whole bunch of helper functions, allowing us to
> > > > > update and check the runtime PM state of the device, even when the
> > > > > disable_depth > 0. Some functions, like pm_runtime_set_active() for
> > > > > example, even take parents and device-links into account.
> > > >
> > > > That's true, but that's for a purpose.
> > > >
> > > > If runtime PM becomes enabled after using pm_runtime_set_active(), the
> > > > status should better be consistent with the settings of the parent
> > > > etc.
> > > >
> > > > > >
> > > > > > If you want the status to stay meaningful, but prevent callbacks from
> > > > > > running, you need something else.
> > > > > >
> > > > > > > The important point is that the default state for a device is
> > > > > > > RPM_SUSPENDED and someone has moved into RPM_ACTIVE, for whatever
> > > > > > > reason. That should be sufficient to allow rpm_resume() to return '1'
> > > > > > > when disable_depth > 0, shouldn't it?
> > > > > >
> > > > > > No, because there is no rule by which the status of devices with
> > > > > > PM-runtime disabled must be RPM_SUSPENDED.
> > > > >
> > > > > That's not what I was trying to say.
> > > > >
> > > > > The initial/default runtime PM state for a device is RPM_SUSPENDED,
> > > > > which is being set in pm_runtime_init(). Although, I agree that it
> > > > > can't be trusted that this state actually reflects the state of the
> > > > > HW, it's still a valid state for the device from a runtime PM point of
> > > > > view.
> > > >
> > > > No, it is not. It's just the default.
> > > >
> > > > > However, and more importantly, if the state has moved to RPM_ACTIVE,
> > > > > someone must have deliberately moved the device into that state.
> > > >
> > > > Sure, but it cannot be regarded as an indication on whether or not
> > > > runtime PM is supported and has ever been enabled for the given
> > > > device.
> > > >
> > > > Again, there is no rule regarding the status value for devices with
> > > > runtime PM disabled, either way.
> > >
> > > If I understand correctly, that means you think the
> > > pm_runtime_status_suspended() should really be converted to an
> > > internal runtime PM interface, not being exported to users outside.
> > > Right?
> >
> > Not really.
> >
> > I'm just saying that its usefulness is limited.
> >
> > My basic concern is that system-wide PM transitions must always invoke
> > callbacks for devices with PM-runtime disabled, because they may (or
> > may not) be functional regardless of the PM-runtime status and if they
> > are functional, they must be suspended. And note that supporting
> > system-wide PM is not optional and the only way to kind of disable it
> > is to return an error from a device suspend callback (but that's nasty
> > for some use cases).
> >
> > So the "Has PM-runtime been enabled?" question is really fundamental
> > for system-wide PM and it is not sufficient to look at the PM-runtime
> > status to find out, but if the PM-core itself disables PM-runtime
> > (which is has to do at one point to prevent PM-runtime from racing
> > with system-wide PM), it is hard to answer definitely in general.
> >
> > IMO the only way to make it possible to find that out in all cases is
> > to make the PM core retain the power.disable_depth value and that can
> > be done by making it use a different mechanism to prevent PM-runtime
> > callbacks from being run.
> >
> > Alternatively, the current PM-runtime status could be "latched" during
> > the PM-runtime disable operation if power.disable_depth is 0 (and that
> > "latched" value would be initialized to "invalid" in case PM-runtime
> > is never enabled).
>
> Which would be something like the patch below (which additionally cleans up
> pm_runtime_enable() while at it).
>
> The idea being that if the status was RPM_ACTIVE last time when
> power.disable_depth was changing from 0 to 1 and it is still RPM_ACTIVE, it
> can be assumed to reflect what happened to the device last time when it was
> using PM-runtime.
Alright, this sounds reasonable to me. I have also looked at the code
below and it looks good to me.
Do you intend to post a formal patch? In any case, feel free to add my
reviewed-by tag.
Kind regards
Uffe
>
> ---
> drivers/base/power/runtime.c | 38 ++++++++++++++++++++------------------
> include/linux/pm.h | 2 ++
> 2 files changed, 22 insertions(+), 18 deletions(-)
>
> Index: linux-pm/drivers/base/power/runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -744,11 +744,10 @@ static int rpm_resume(struct device *dev
> repeat:
> if (dev->power.runtime_error)
> retval = -EINVAL;
> - else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> - && dev->power.runtime_status == RPM_ACTIVE)
> - retval = 1;
> else if (dev->power.disable_depth > 0)
> - retval = -EACCES;
> + retval = dev->power.runtime_status == RPM_ACTIVE &&
> + dev->power.last_status == RPM_ACTIVE ? 1 : -EACCES;
> +
> if (retval)
> goto out;
>
> @@ -1410,8 +1409,10 @@ void __pm_runtime_disable(struct device
> /* Update time accounting before disabling PM-runtime. */
> update_pm_runtime_accounting(dev);
>
> - if (!dev->power.disable_depth++)
> + if (!dev->power.disable_depth++) {
> __pm_runtime_barrier(dev);
> + dev->power.last_status = dev->power.runtime_status;
> + }
>
> out:
> spin_unlock_irq(&dev->power.lock);
> @@ -1428,23 +1429,23 @@ void pm_runtime_enable(struct device *de
>
> spin_lock_irqsave(&dev->power.lock, flags);
>
> - if (dev->power.disable_depth > 0) {
> - dev->power.disable_depth--;
> -
> - /* About to enable runtime pm, set accounting_timestamp to now */
> - if (!dev->power.disable_depth)
> - dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
> - } else {
> + if (!dev->power.disable_depth) {
> dev_warn(dev, "Unbalanced %s!\n", __func__);
> + goto out;
> }
>
> - WARN(!dev->power.disable_depth &&
> - dev->power.runtime_status == RPM_SUSPENDED &&
> - !dev->power.ignore_children &&
> - atomic_read(&dev->power.child_count) > 0,
> - "Enabling runtime PM for inactive device (%s) with active children\n",
> - dev_name(dev));
> + if (--dev->power.disable_depth > 0)
> + goto out;
> +
> + dev->power.last_status = RPM_INVALID;
> + dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
> +
> + if (dev->power.runtime_status == RPM_SUSPENDED &&
> + !dev->power.ignore_children &&
> + atomic_read(&dev->power.child_count) > 0)
> + dev_warn(dev, "Enabling runtime PM for inactive device with active children\n");
>
> +out:
> spin_unlock_irqrestore(&dev->power.lock, flags);
> }
> EXPORT_SYMBOL_GPL(pm_runtime_enable);
> @@ -1640,6 +1641,7 @@ EXPORT_SYMBOL_GPL(__pm_runtime_use_autos
> void pm_runtime_init(struct device *dev)
> {
> dev->power.runtime_status = RPM_SUSPENDED;
> + dev->power.last_status = RPM_INVALID;
> dev->power.idle_notification = false;
>
> dev->power.disable_depth = 1;
> Index: linux-pm/include/linux/pm.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm.h
> +++ linux-pm/include/linux/pm.h
> @@ -499,6 +499,7 @@ const struct dev_pm_ops __maybe_unused n
> */
>
> enum rpm_status {
> + RPM_INVALID = -1,
> RPM_ACTIVE = 0,
> RPM_RESUMING,
> RPM_SUSPENDED,
> @@ -612,6 +613,7 @@ struct dev_pm_info {
> unsigned int links_count;
> enum rpm_request request;
> enum rpm_status runtime_status;
> + enum rpm_status last_status;
> int runtime_error;
> int autosuspend_delay;
> u64 last_busy;
>
>
>
On Thu, Dec 2, 2021 at 12:29 PM Ulf Hansson <[email protected]> wrote:
>
> On Wed, 1 Dec 2021 at 21:11, Rafael J. Wysocki <[email protected]> wrote:
> >
> > On Wednesday, December 1, 2021 6:44:08 PM CET Rafael J. Wysocki wrote:
> > > On Wed, Dec 1, 2021 at 4:23 PM Ulf Hansson <[email protected]> wrote:
> > > >
> > > > On Wed, 1 Dec 2021 at 14:49, Rafael J. Wysocki <[email protected]> wrote:
> > > > >
> > > > > On Wed, Dec 1, 2021 at 10:02 AM Ulf Hansson <[email protected]> wrote:
> > > > > >
> > > > > > On Tue, 30 Nov 2021 at 18:26, Rafael J. Wysocki <[email protected]> wrote:
> > > > > > >
> > > > > > > On Tue, Nov 30, 2021 at 5:41 PM Ulf Hansson <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Tue, 30 Nov 2021 at 14:02, Rafael J. Wysocki <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, Nov 30, 2021 at 12:58 PM Ulf Hansson <[email protected]> wrote:
> > > > > > > > > >
> > > > > > > > > > [...]
> > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Am I thinking correctly that this is mostly about working around the
> > > > > > > > > > > > > > > > limitations of pm_runtime_force_suspend()?
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > No, this isn't related at all.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > The cpuidle-psci driver doesn't have PM callbacks, thus using
> > > > > > > > > > > > > > > pm_runtime_force_suspend() would not work here.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Just wanted to send a ping on this to see if we can come to a
> > > > > > > > > > > > > > conclusion. Or maybe we did? :-)
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I think in the end, what slightly bothers me, is that the behavior is
> > > > > > > > > > > > > > a bit inconsistent. Although, maybe it's the best we can do.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I've been thinking about this and it looks like we can do better, but
> > > > > > > > > > > > > instead of talking about this I'd rather send a patch.
> > > > > > > > > > > >
> > > > > > > > > > > > Alright.
> > > > > > > > > > > >
> > > > > > > > > > > > I was thinking along the lines of make similar changes for
> > > > > > > > > > > > rpm_idle|suspend(). That would make the behaviour even more
> > > > > > > > > > > > consistent, I think.
> > > > > > > > > > > >
> > > > > > > > > > > > Perhaps that's what you have in mind? :-)
> > > > > > > > > > >
> > > > > > > > > > > Well, not exactly.
> > > > > > > > > > >
> > > > > > > > > > > The idea is to add another counter (called restrain_depth in the patch)
> > > > > > > > > > > to prevent rpm_resume() from running the callback when that is potentially
> > > > > > > > > > > problematic. With that, it is possible to actually distinguish devices
> > > > > > > > > > > with PM-runtime enabled and it allows the PM-runtime status to be checked
> > > > > > > > > > > when it is still known to be meaningful.
> > > > > > > > > >
> > > > > > > > > > Hmm, I don't quite understand the benefit of introducing a new flag
> > > > > > > > > > for this. rpm_resume() already checks the disable_depth to understand
> > > > > > > > > > when it's safe to invoke the callback. Maybe there is a reason why
> > > > > > > > > > that isn't sufficient?
> > > > > > > > >
> > > > > > > > > The problem is that disable_depth > 0 may very well mean that runtime
> > > > > > > > > PM has not been enabled at all for the given device which IMO is a
> > > > > > > > > problem.
> > > > > > > > >
> > > > > > > > > As it stands, it is necessary to make assumptions, like disable_depth
> > > > > > > > > == 1 meaning that runtime PM is really enabled, but the PM core has
> > > > > > > > > disabled it temporarily, which is somewhat questionable.
> > > > > > > > >
> > > > > > > > > Another problem with disabling is that it causes rpm_resume() to fail
> > > > > > > > > even if the status is RPM_ACTIVE and it has to do that exactly because
> > > > > > > > > it cannot know why runtime PM has been disabled. If it has never been
> > > > > > > > > enabled, rpm_resume() must fail, but if it has been disabled
> > > > > > > > > temporarily, rpm_resume() may return 1 when the status is RPM_ACTIVE.
> > > > > > > > >
> > > > > > > > > The new count allows the "enabled in general, but temporarily disabled
> > > > > > > > > at the moment" to be handled cleanly.
> > > > > > > >
> > > > > > > > My overall comment is that I fail to understand why we need to
> > > > > > > > distinguish between these two cases. To me, it shouldn't really
> > > > > > > > matter, *why* runtime PM is (or have been) disabled for the device.
> > > > > > >
> > > > > > > It matters if you want to trust the status, because "disabled" means
> > > > > > > "the status doesn't matter".
> > > > > >
> > > > > > Well, that doesn't really match how the runtime PM interface is being
> > > > > > used today.
> > > > >
> > > > > Well, I clearly disagree.
> > > >
> > > > Alright, then we can agree to disagree. :-)
> > > >
> > > > >
> > > > > > For example, we have a whole bunch of helper functions, allowing us to
> > > > > > update and check the runtime PM state of the device, even when the
> > > > > > disable_depth > 0. Some functions, like pm_runtime_set_active() for
> > > > > > example, even take parents and device-links into account.
> > > > >
> > > > > That's true, but that's for a purpose.
> > > > >
> > > > > If runtime PM becomes enabled after using pm_runtime_set_active(), the
> > > > > status should better be consistent with the settings of the parent
> > > > > etc.
> > > > >
> > > > > > >
> > > > > > > If you want the status to stay meaningful, but prevent callbacks from
> > > > > > > running, you need something else.
> > > > > > >
> > > > > > > > The important point is that the default state for a device is
> > > > > > > > RPM_SUSPENDED and someone has moved into RPM_ACTIVE, for whatever
> > > > > > > > reason. That should be sufficient to allow rpm_resume() to return '1'
> > > > > > > > when disable_depth > 0, shouldn't it?
> > > > > > >
> > > > > > > No, because there is no rule by which the status of devices with
> > > > > > > PM-runtime disabled must be RPM_SUSPENDED.
> > > > > >
> > > > > > That's not what I was trying to say.
> > > > > >
> > > > > > The initial/default runtime PM state for a device is RPM_SUSPENDED,
> > > > > > which is being set in pm_runtime_init(). Although, I agree that it
> > > > > > can't be trusted that this state actually reflects the state of the
> > > > > > HW, it's still a valid state for the device from a runtime PM point of
> > > > > > view.
> > > > >
> > > > > No, it is not. It's just the default.
> > > > >
> > > > > > However, and more importantly, if the state has moved to RPM_ACTIVE,
> > > > > > someone must have deliberately moved the device into that state.
> > > > >
> > > > > Sure, but it cannot be regarded as an indication on whether or not
> > > > > runtime PM is supported and has ever been enabled for the given
> > > > > device.
> > > > >
> > > > > Again, there is no rule regarding the status value for devices with
> > > > > runtime PM disabled, either way.
> > > >
> > > > If I understand correctly, that means you think the
> > > > pm_runtime_status_suspended() should really be converted to an
> > > > internal runtime PM interface, not being exported to users outside.
> > > > Right?
> > >
> > > Not really.
> > >
> > > I'm just saying that its usefulness is limited.
> > >
> > > My basic concern is that system-wide PM transitions must always invoke
> > > callbacks for devices with PM-runtime disabled, because they may (or
> > > may not) be functional regardless of the PM-runtime status and if they
> > > are functional, they must be suspended. And note that supporting
> > > system-wide PM is not optional and the only way to kind of disable it
> > > is to return an error from a device suspend callback (but that's nasty
> > > for some use cases).
> > >
> > > So the "Has PM-runtime been enabled?" question is really fundamental
> > > for system-wide PM and it is not sufficient to look at the PM-runtime
> > > status to find out, but if the PM-core itself disables PM-runtime
> > > (which is has to do at one point to prevent PM-runtime from racing
> > > with system-wide PM), it is hard to answer definitely in general.
> > >
> > > IMO the only way to make it possible to find that out in all cases is
> > > to make the PM core retain the power.disable_depth value and that can
> > > be done by making it use a different mechanism to prevent PM-runtime
> > > callbacks from being run.
> > >
> > > Alternatively, the current PM-runtime status could be "latched" during
> > > the PM-runtime disable operation if power.disable_depth is 0 (and that
> > > "latched" value would be initialized to "invalid" in case PM-runtime
> > > is never enabled).
> >
> > Which would be something like the patch below (which additionally cleans up
> > pm_runtime_enable() while at it).
> >
> > The idea being that if the status was RPM_ACTIVE last time when
> > power.disable_depth was changing from 0 to 1 and it is still RPM_ACTIVE, it
> > can be assumed to reflect what happened to the device last time when it was
> > using PM-runtime.
>
> Alright, this sounds reasonable to me. I have also looked at the code
> below and it looks good to me.
>
> Do you intend to post a formal patch? In any case, feel free to add my
> reviewed-by tag.
I will, thank you!
On Thu, Dec 02, 2021 at 05:18:26PM +0100, Rafael J. Wysocki wrote:
> On Thu, Dec 2, 2021 at 12:29 PM Ulf Hansson <[email protected]> wrote:
> >
> > Alright, this sounds reasonable to me. I have also looked at the code
> > below and it looks good to me.
> >
> > Do you intend to post a formal patch? In any case, feel free to add my
> > reviewed-by tag.
>
> I will, thank you!
Don't forget to check for spelling errors (I noticed one in the first version of
the patch you posted). And please include a documentation update.
On the whole, the idea seems like a good improvement.
Alan Stern
On Thu, Dec 2, 2021 at 5:50 PM Alan Stern <[email protected]> wrote:
>
> On Thu, Dec 02, 2021 at 05:18:26PM +0100, Rafael J. Wysocki wrote:
> > On Thu, Dec 2, 2021 at 12:29 PM Ulf Hansson <[email protected]> wrote:
> > >
> > > Alright, this sounds reasonable to me. I have also looked at the code
> > > below and it looks good to me.
> > >
> > > Do you intend to post a formal patch? In any case, feel free to add my
> > > reviewed-by tag.
> >
> > I will, thank you!
>
> Don't forget to check for spelling errors (I noticed one in the first version of
> the patch you posted). And please include a documentation update.
I will.
> On the whole, the idea seems like a good improvement.
Thanks!