From: Rafael J. Wysocki <[email protected]>
In some cases (for example, during system-wide suspend and resume of
devices) it is useful to know whether or not runtime PM has ever been
enabled for a given device and, if so, what the runtime PM status of
it had been right before runtime PM was disabled for it last time.
For this reason, introduce a new struct dev_pm_info field called
last_status that will be used for capturing the runtime PM status of
the device when its power.disable_depth counter changes from 0 to 1.
The new field will be set to RPM_INVALID to start with and whenever
power.disable_depth changes from 1 to 0, so it will be valid only
when runtime PM of the device is currently disabled, but it has been
enabled at least once.
Immediately use power.last_status in rpm_resume() to make it handle
the case when PM runtime is disabled for the device, but its runtime
PM status is RPM_ACTIVE more consistently. Namely, make it return 1
if power.last_status is also equal to RPM_ACTIVE in that case (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 runtime PM) and -EACCES otherwise.
Update the documentation to provide a description of last_status and
change the description of pm_runtime_resume() in it to reflect the
new behavior of rpm_active().
While at it, rearrange the code in pm_runtime_enable() to be more
straightforward and replace the WARN() macro in it with a pr_warn()
invocation which is less disruptive.
Link: https://lore.kernel.org/linux-pm/[email protected]/t/#u
Reviewed-by: Ulf Hansson <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
Documentation/power/runtime_pm.rst | 14 +++++++++----
drivers/base/power/runtime.c | 38 +++++++++++++++++++------------------
include/linux/pm.h | 2 +
3 files changed, 32 insertions(+), 22 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;
Index: linux-pm/Documentation/power/runtime_pm.rst
===================================================================
--- linux-pm.orig/Documentation/power/runtime_pm.rst
+++ linux-pm/Documentation/power/runtime_pm.rst
@@ -265,6 +265,10 @@ defined in include/linux/pm.h:
RPM_SUSPENDED, which means that each device is initially regarded by the
PM core as 'suspended', regardless of its real hardware status
+ `enum rpm_status last_status;`
+ - the last runtime PM status of the device captured before disabling runtime
+ PM for it (invalid initially and when disable_depth is 0)
+
`unsigned int runtime_auto;`
- if set, indicates that the user space has allowed the device driver to
power manage the device at run time via the /sys/devices/.../power/control
@@ -333,10 +337,12 @@ drivers/base/power/runtime.c and include
`int pm_runtime_resume(struct device *dev);`
- execute the subsystem-level resume callback for the device; returns 0 on
- success, 1 if the device's runtime PM status was already 'active' or
- error code on failure, where -EAGAIN means it may be safe to attempt to
- resume the device again in future, but 'power.runtime_error' should be
- checked additionally, and -EACCES means that 'power.disable_depth' is
+ success, 1 if the device's runtime PM status is already 'active' (also if
+ 'power.disable_depth' is nonzero, but the status was 'active' when it was
+ changing from 0 to 1) or error code on failure, where -EAGAIN means it may
+ be safe to attempt to resume the device again in future, but
+ 'power.runtime_error' should be checked additionally, and -EACCES means
+ that the callback could not be run, because 'power.disable_depth' was
different from 0
`int pm_runtime_resume_and_get(struct device *dev);`
On Fri, Dec 03, 2021 at 05:24:45PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> In some cases (for example, during system-wide suspend and resume of
> devices) it is useful to know whether or not runtime PM has ever been
> enabled for a given device and, if so, what the runtime PM status of
> it had been right before runtime PM was disabled for it last time.
>
> For this reason, introduce a new struct dev_pm_info field called
> last_status that will be used for capturing the runtime PM status of
> the device when its power.disable_depth counter changes from 0 to 1.
>
> The new field will be set to RPM_INVALID to start with and whenever
> power.disable_depth changes from 1 to 0, so it will be valid only
> when runtime PM of the device is currently disabled, but it has been
> enabled at least once.
>
> Immediately use power.last_status in rpm_resume() to make it handle
> the case when PM runtime is disabled for the device, but its runtime
> PM status is RPM_ACTIVE more consistently. Namely, make it return 1
> if power.last_status is also equal to RPM_ACTIVE in that case (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 runtime PM) and -EACCES otherwise.
>
> Update the documentation to provide a description of last_status and
> change the description of pm_runtime_resume() in it to reflect the
> new behavior of rpm_active().
>
> While at it, rearrange the code in pm_runtime_enable() to be more
> straightforward and replace the WARN() macro in it with a pr_warn()
> invocation which is less disruptive.
>
> Link: https://lore.kernel.org/linux-pm/[email protected]/t/#u
> Reviewed-by: Ulf Hansson <[email protected]>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> Documentation/power/runtime_pm.rst | 14 +++++++++----
> drivers/base/power/runtime.c | 38 +++++++++++++++++++------------------
> include/linux/pm.h | 2 +
> 3 files changed, 32 insertions(+), 22 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;
Suggestion for a small improvement in readability: The way this
statement is broken between two lines, it looks as if the ?: operator
has higher precedence than the && operator, which is very confusing.
Adding parentheses would help. Even better would be to rewrite this as
an "if" statement:
if (dev->power.runtime_status == RPM_ACTIVE &&
dev->power.last_status == RPM_ACTIVE)
retval = 1;
else
retval = -EACCES;
Alan Stern
On Fri, Dec 3, 2021 at 7:27 PM Alan Stern <[email protected]> wrote:
>
> On Fri, Dec 03, 2021 at 05:24:45PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > In some cases (for example, during system-wide suspend and resume of
> > devices) it is useful to know whether or not runtime PM has ever been
> > enabled for a given device and, if so, what the runtime PM status of
> > it had been right before runtime PM was disabled for it last time.
> >
> > For this reason, introduce a new struct dev_pm_info field called
> > last_status that will be used for capturing the runtime PM status of
> > the device when its power.disable_depth counter changes from 0 to 1.
> >
> > The new field will be set to RPM_INVALID to start with and whenever
> > power.disable_depth changes from 1 to 0, so it will be valid only
> > when runtime PM of the device is currently disabled, but it has been
> > enabled at least once.
> >
> > Immediately use power.last_status in rpm_resume() to make it handle
> > the case when PM runtime is disabled for the device, but its runtime
> > PM status is RPM_ACTIVE more consistently. Namely, make it return 1
> > if power.last_status is also equal to RPM_ACTIVE in that case (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 runtime PM) and -EACCES otherwise.
> >
> > Update the documentation to provide a description of last_status and
> > change the description of pm_runtime_resume() in it to reflect the
> > new behavior of rpm_active().
> >
> > While at it, rearrange the code in pm_runtime_enable() to be more
> > straightforward and replace the WARN() macro in it with a pr_warn()
> > invocation which is less disruptive.
> >
> > Link: https://lore.kernel.org/linux-pm/[email protected]/t/#u
> > Reviewed-by: Ulf Hansson <[email protected]>
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> > Documentation/power/runtime_pm.rst | 14 +++++++++----
> > drivers/base/power/runtime.c | 38 +++++++++++++++++++------------------
> > include/linux/pm.h | 2 +
> > 3 files changed, 32 insertions(+), 22 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;
>
> Suggestion for a small improvement in readability: The way this
> statement is broken between two lines, it looks as if the ?: operator
> has higher precedence than the && operator, which is very confusing.
> Adding parentheses would help. Even better would be to rewrite this as
> an "if" statement:
>
> if (dev->power.runtime_status == RPM_ACTIVE &&
> dev->power.last_status == RPM_ACTIVE)
> retval = 1;
> else
> retval = -EACCES;
I would need to add braces around this then as per the coding style,
which I wanted to avoid, but of course that can be done.