Update last busy timestamp in Runtime PM autosuspend callbacks

Document return values for device suspend and idle related API functions.

Signed-off-by Sakari Ailus <[email protected]>

---
 include/linux/pm_runtime.h | 147 ++++++++++++++++++++++++++++++++++---
 1 file changed, 138 insertions(+), 9 deletions(-)

include/linux/pm_runtime.h +0 -0

index e7cb70fcc0af..9dd2e4031a27 100644

Hi,

Quoted 3 lines · Sakari Ailus

Document return values for device suspend and idle related API functions.

Signed-off-by Sakari Ailus <[email protected]>

I appreciate the documentation attempt here. I've often found it a maze trying to weave through the indirection and figure out the actual API contract for some of these.

But I have a few questions below:

Quoted 8 lines

---
 include/linux/pm_runtime.h | 147 ++++++++++++++++++++++++++++++++++---
 1 file changed, 138 insertions(+), 9 deletions(-)

include/linux/pm_runtime.h +0 -0

index e7cb70fcc0af..9dd2e4031a27 100644

include/linux/pm_runtime.h static inline int pm_runtime_resume_and_get(struct device *dev) · line 533

Quoted 10 lines

include/linux/pm_runtime.h +6 -0

@@ -464,6 +525,17 @@ static inline int pm_runtime_resume_and_get(struct device *dev)
  *
  * Decrement the runtime PM usage counter of @dev and if it turns out to be
  * equal to 0, queue up a work item for @dev like in pm_request_idle().
+ *
+ * Return:
+ * * 0: Success.
+ * * -EINVAL: Runtime PM error.
+ * * -EACCES: Runtime PM disabled.
+ * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.

^^ Is the "usage_count non-zero" true? For RPM_GET_PUT, we drop the refcount, and if it's still nonzero, we simply return 0.

include/linux/pm_runtime.h static inline void pm_runtime_release_supplier(struct device_link *link) {} · line 351

Quoted 5 lines

include/linux/pm_runtime.h +5 -0

+ * * -EBUSY: Runtime PM child_count non-zero.
+ * * -EPERM: Device PM QoS resume latency 0.
+ * * -EINPROGRESS: Suspend already in progress.
+ * * -ENOSYS: CONFIG_PM not enabled.
+ * * 1: Device already suspended.

This part isn't very clear to me: can we even hit this case? If the usage count was already 0, we'd hit EINVAL, since this is a PUT operation. If the usage count was non-zero, we can't already be suspended. At a minimum, we'd be RESUMING (e.g., an async resume), no?

include/linux/pm_runtime.h static inline int pm_runtime_resume_and_get(struct device *dev) · line 541

Quoted 3 lines

include/linux/pm_runtime.h +0 -0

  */
 static inline int pm_runtime_put(struct device *dev)
 {

If these are indeed errors, I expect they're repeated on some of the other related APIs too (like pm_runtime_put_sync(), pm_runtime_idle(), and probably more).

I ask mostly for my own understanding, but I might consider patching the docs if I'm not hallucinating these errors.

After reading and rereading ... and then writing unit tests, because I can't trust my reading ... I think I can answer my own questions:

Quoted 23 lines · Brian Norris

Hi,

Quoted 3 lines · Sakari Ailus

Document return values for device suspend and idle related API functions.

Signed-off-by Sakari Ailus <[email protected]>

I appreciate the documentation attempt here. I've often found it a maze trying to weave through the indirection and figure out the actual API contract for some of these.

But I have a few questions below:

Quoted 8 lines

---
 include/linux/pm_runtime.h | 147 ++++++++++++++++++++++++++++++++++---
 1 file changed, 138 insertions(+), 9 deletions(-)

include/linux/pm_runtime.h +0 -0

index e7cb70fcc0af..9dd2e4031a27 100644

Quoted 13 lines

include/linux/pm_runtime.h static inline int pm_runtime_resume_and_get(struct device *dev) · line 533

Quoted 10 lines

include/linux/pm_runtime.h +6 -0

@@ -464,6 +525,17 @@ static inline int pm_runtime_resume_and_get(struct device *dev)
  *
  * Decrement the runtime PM usage counter of @dev and if it turns out to be
  * equal to 0, queue up a work item for @dev like in pm_request_idle().
+ *
+ * Return:
+ * * 0: Success.
+ * * -EINVAL: Runtime PM error.
+ * * -EACCES: Runtime PM disabled.
+ * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.

^^ Is the "usage_count non-zero" true? For RPM_GET_PUT, we drop the refcount, and if it's still nonzero, we simply return 0.

It *is* possible, but it would only occur with the following:

(1) we've acquired our usage_count by way of pm_runtime_get_noresume(); and
(2) some other actor is racing with us, acquiring a usage_count between when we decremented usage_count to 0, and when we check again in rpm_idle()

IMO, as-is, the language is a bit misleading though. But then, the behavior is pretty subtle and hard to describe succinctly...

Quoted 7 lines

Quoted 6 lines

include/linux/pm_runtime.h +5 -0

+ * * -EBUSY: Runtime PM child_count non-zero.
+ * * -EPERM: Device PM QoS resume latency 0.
+ * * -EINPROGRESS: Suspend already in progress.
+ * * -ENOSYS: CONFIG_PM not enabled.
+ * * 1: Device already suspended.

Quoted 4 lines

This part isn't very clear to me: can we even hit this case? If the usage count was already 0, we'd hit EINVAL, since this is a PUT operation. If the usage count was non-zero, we can't already be suspended. At a minimum, we'd be RESUMING (e.g., an async resume), no?

This is sort of possible in the same scenario as above. But it doesn't actually return 1; it still returns -EAGAIN.

Confusingly, pm_runtime_put_autosuspend() behaves differently, because it's based on rpm_suspend() instead of rpm_idle().

I plan to fix this, because I don't see why pm_runtime_put() and pm_runtime_put_autosuspend() should differ here.

Quoted 9 lines

include/linux/pm_runtime.h static inline int pm_runtime_resume_and_get(struct device *dev) · line 541

Quoted 4 lines

include/linux/pm_runtime.h +0 -0

  */
 static inline int pm_runtime_put(struct device *dev)
 {

If these are indeed errors, I expect they're repeated on some of the other related APIs too (like pm_runtime_put_sync(), pm_runtime_idle(), and probably more).

Quoted 2 lines

I ask mostly for my own understanding, but I might consider patching the docs if I'm not hallucinating these errors.

I've sent a patch series to fix some of the inconsistencies in the API and to fix the API docs:

Subject: [PATCH 3/3] PM: runtime: Update kerneldoc return codes
https://lore.kernel.org/all/[email protected]/

Folks,

The original plan for adding pm_runtime_mark_last_busy() calls to functions dealing with Runtime PM autosuspend originally included a few intermediate steps of driver conversion, including the use of recently added __pm_runtime_put_autosuspend(). The review of the set converting the users first to __pm_runtime_put_autosuspend() concluded this wasn't necessary. See <URL:https://lore.kernel.org/all/[email protected]/>.

This set extends the inclusion of the pm_runtime_mark_last_busy() call to the _autosuspend() variants of the Runtime PM functions dealing with suspending devices, i.e. pm_runtime_put_autosuspend(), pm_runtime_put_sync_autosuspend(), pm_runtime_autosuspend() and pm_request_autosuspend(). This will introduce, for a brief amount of time, unnecessary calls to pm_runtime_mark_last_busy() but this wasn't seen as an issue. Also, all users of these functions, including those that did not call pm_runtime_mark_last_busy(), will now include that call. Presumably in the vast majority of the cases a missing call would have been a bug.

Once this set is merged, I'll post further patches to remove the extra pm_runtime_mark_last_busy() calls. The current set of these patches is
here <URL:https://git.kernel.org/pub/scm/linux/kernel/git/sailus/linux-next.git/log/?h=pm-direct-on-next>.

It'd be best to have all merged within the same cycle.

Rafael: any thoughts on the merging?

Would an immutable branch on top of rc1 be an option?

The patches apply cleanly on v6.16-rc1, v6.16-rc2 and linux-next.

The changes in the patches to remove the pm_runtime_mark_last_busy() calls have been generated using the following Coccinelle spatch:

@@
expression E;
identifier label, rval;
@@
- pm_runtime_mark_last_busy(E);
...
(
label:
|
)
...
(
pm_runtime_put_autosuspend(E);
|
pm_runtime_put_sync_autosuspend(E);
|
pm_runtime_autosuspend(E);
|
pm_request_autosuspend(E);
|
(void)pm_runtime_put_autosuspend(E);
|
(void)pm_runtime_put_sync_autosuspend(E);
|
(void)pm_runtime_autosuspend(E);
|
(void)pm_request_autosuspend(E);
|
return pm_runtime_put_autosuspend(E);
|
return pm_runtime_put_sync_autosuspend(E);
|
return pm_runtime_autosuspend(E);
|
return pm_request_autosuspend(E);
|
rval = pm_runtime_put_autosuspend(E);
|
rval = pm_runtime_put_sync_autosuspend(E);
|
rval = pm_runtime_autosuspend(E);
|
rval = pm_request_autosuspend(E);
)

since v1:

- kernel-doc fix in patch "pm: runtime: Mark last busy stamp in pm_runtime_put_sync_autosuspend()".

- Dropped the first patch which has been merged already.

Sakari Ailus (6):
pm: runtime: Document return values of suspend related API functions
pm: runtime: Mark last busy stamp in pm_runtime_put_autosuspend()
pm: runtime: Mark last busy stamp in pm_runtime_put_sync_autosuspend()
pm: runtime: Mark last busy stamp in pm_runtime_autosuspend()
pm: runtime: Mark last busy stamp in pm_request_autosuspend()
Documentation: PM: *_autosuspend() functions update last busy time

 Documentation/power/runtime_pm.rst |  50 ++++----
 include/linux/pm_runtime.h         | 187 +++++++++++++++++++++++++----
 2 files changed, 186 insertions(+), 51 deletions(-)

--

Quoted 26 lines · Sakari Ailus

Folks,

The original plan for adding pm_runtime_mark_last_busy() calls to functions dealing with Runtime PM autosuspend originally included a few intermediate steps of driver conversion, including the use of recently added __pm_runtime_put_autosuspend(). The review of the set converting the users first to __pm_runtime_put_autosuspend() concluded this wasn't necessary. See <URL:https://lore.kernel.org/all/[email protected]/>.

This set extends the inclusion of the pm_runtime_mark_last_busy() call to the _autosuspend() variants of the Runtime PM functions dealing with suspending devices, i.e. pm_runtime_put_autosuspend(), pm_runtime_put_sync_autosuspend(), pm_runtime_autosuspend() and pm_request_autosuspend(). This will introduce, for a brief amount of time, unnecessary calls to pm_runtime_mark_last_busy() but this wasn't seen as an issue. Also, all users of these functions, including those that did not call pm_runtime_mark_last_busy(), will now include that call. Presumably in the vast majority of the cases a missing call would have been a bug.

Once this set is merged, I'll post further patches to remove the extra pm_runtime_mark_last_busy() calls. The current set of these patches is
here <URL:https://git.kernel.org/pub/scm/linux/kernel/git/sailus/linux-next.git/log/?h=pm-direct-on-next>.

Quoted 3 lines

It'd be best to have all merged within the same cycle.

Rafael: any thoughts on the merging?

I'm going to queue this up for 6.17.

Quoted 2 lines

Would an immutable branch on top of rc1 be an option?

I think so, but does anyone need it?

Quoted 27 lines · Rafael J. Wysocki

Quoted 26 lines · Sakari Ailus

Folks,

The original plan for adding pm_runtime_mark_last_busy() calls to functions dealing with Runtime PM autosuspend originally included a few intermediate steps of driver conversion, including the use of recently added __pm_runtime_put_autosuspend(). The review of the set converting the users first to __pm_runtime_put_autosuspend() concluded this wasn't necessary. See <URL:https://lore.kernel.org/all/[email protected]/>.

This set extends the inclusion of the pm_runtime_mark_last_busy() call to the _autosuspend() variants of the Runtime PM functions dealing with suspending devices, i.e. pm_runtime_put_autosuspend(), pm_runtime_put_sync_autosuspend(), pm_runtime_autosuspend() and pm_request_autosuspend(). This will introduce, for a brief amount of time, unnecessary calls to pm_runtime_mark_last_busy() but this wasn't seen as an issue. Also, all users of these functions, including those that did not call pm_runtime_mark_last_busy(), will now include that call. Presumably in the vast majority of the cases a missing call would have been a bug.

Once this set is merged, I'll post further patches to remove the extra pm_runtime_mark_last_busy() calls. The current set of these patches is
here <URL:https://git.kernel.org/pub/scm/linux/kernel/git/sailus/linux-next.git/log/?h=pm-direct-on-next>.

Quoted 5 lines

Quoted 3 lines

It'd be best to have all merged within the same cycle.

Rafael: any thoughts on the merging?

I'm going to queue this up for 6.17.

Thank you! :-)

Quoted 3 lines

Quoted 1 line

Would an immutable branch on top of rc1 be an option?

I think so, but does anyone need it?

I guess it's not mandatory but we'll have now a lot of redundant calls to the pm_runtime_mark_last_busy(). It just doesn't look very elegant. In the end it's all up to what the maintainers prefer.

Hi Rafael,

Quoted 28 lines · Rafael J. Wysocki

Quoted 26 lines · Sakari Ailus

Folks,

The original plan for adding pm_runtime_mark_last_busy() calls to functions dealing with Runtime PM autosuspend originally included a few intermediate steps of driver conversion, including the use of recently added __pm_runtime_put_autosuspend(). The review of the set converting the users first to __pm_runtime_put_autosuspend() concluded this wasn't necessary. See <URL:https://lore.kernel.org/all/[email protected]/>.

This set extends the inclusion of the pm_runtime_mark_last_busy() call to the _autosuspend() variants of the Runtime PM functions dealing with suspending devices, i.e. pm_runtime_put_autosuspend(), pm_runtime_put_sync_autosuspend(), pm_runtime_autosuspend() and pm_request_autosuspend(). This will introduce, for a brief amount of time, unnecessary calls to pm_runtime_mark_last_busy() but this wasn't seen as an issue. Also, all users of these functions, including those that did not call pm_runtime_mark_last_busy(), will now include that call. Presumably in the vast majority of the cases a missing call would have been a bug.

Once this set is merged, I'll post further patches to remove the extra pm_runtime_mark_last_busy() calls. The current set of these patches is
here <URL:https://git.kernel.org/pub/scm/linux/kernel/git/sailus/linux-next.git/log/?h=pm-direct-on-next>.

Quoted 5 lines

Quoted 3 lines

It'd be best to have all merged within the same cycle.

Rafael: any thoughts on the merging?

I'm going to queue this up for 6.17.

Thank you! :-)

Quoted 4 lines

Quoted 2 lines

Would an immutable branch on top of rc1 be an option?

I think so, but does anyone need it?

I guess it's not mandatory but we'll have now a lot of redundant calls to the pm_runtime_mark_last_busy(). It just doesn't look very elegant. In the end it's all up to what the maintainers prefer.

Quoted 32 lines · Sakari Ailus

Hi Rafael,

Quoted 28 lines · Rafael J. Wysocki

Quoted 26 lines · Sakari Ailus

Folks,

The original plan for adding pm_runtime_mark_last_busy() calls to functions dealing with Runtime PM autosuspend originally included a few intermediate steps of driver conversion, including the use of recently added __pm_runtime_put_autosuspend(). The review of the set converting the users first to __pm_runtime_put_autosuspend() concluded this wasn't necessary. See <URL:https://lore.kernel.org/all/[email protected]/>.

This set extends the inclusion of the pm_runtime_mark_last_busy() call to the _autosuspend() variants of the Runtime PM functions dealing with suspending devices, i.e. pm_runtime_put_autosuspend(), pm_runtime_put_sync_autosuspend(), pm_runtime_autosuspend() and pm_request_autosuspend(). This will introduce, for a brief amount of time, unnecessary calls to pm_runtime_mark_last_busy() but this wasn't seen as an issue. Also, all users of these functions, including those that did not call pm_runtime_mark_last_busy(), will now include that call. Presumably in the vast majority of the cases a missing call would have been a bug.

Once this set is merged, I'll post further patches to remove the extra pm_runtime_mark_last_busy() calls. The current set of these patches is
here <URL:https://git.kernel.org/pub/scm/linux/kernel/git/sailus/linux-next.git/log/?h=pm-direct-on-next>.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

It'd be best to have all merged within the same cycle.

Rafael: any thoughts on the merging?

I'm going to queue this up for 6.17.

Thank you! :-)

Now applied.

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

Would an immutable branch on top of rc1 be an option?

Quoted 5 lines

Quoted 1 line

I think so, but does anyone need it?

I guess it's not mandatory but we'll have now a lot of redundant calls to the pm_runtime_mark_last_busy(). It just doesn't look very elegant. In the end it's all up to what the maintainers prefer.

I'll let you know when the branch is ready to pull from.

Folks,

This set extends the inclusion of the pm_runtime_mark_last_busy() call to the _autosuspend() variants of the Runtime PM functions dealing with suspending devices, i.e. pm_runtime_put_autosuspend(), pm_runtime_put_sync_autosuspend(), pm_runtime_autosuspend() and pm_request_autosuspend(). This will introduce, for a brief amount of time, unnecessary calls to pm_runtime_mark_last_busy() but this wasn't seen as an issue. Also, all users of these functions, including those that did not call pm_runtime_mark_last_busy(), will now include that call. Presumably in the vast majority of the cases a missing call would have been a bug.

Quoted 1 line · Sakari Ailus

Folks,

Hi,

Quoted 9 lines

This set extends the inclusion of the pm_runtime_mark_last_busy() call to the _autosuspend() variants of the Runtime PM functions dealing with suspending devices, i.e. pm_runtime_put_autosuspend(), pm_runtime_put_sync_autosuspend(), pm_runtime_autosuspend() and pm_request_autosuspend(). This will introduce, for a brief amount of time, unnecessary calls to pm_runtime_mark_last_busy() but this wasn't seen as an issue. Also, all users of these functions, including those that did not call pm_runtime_mark_last_busy(), will now include that call. Presumably in the vast majority of the cases a missing call would have been a bug.

Now that I think about this, I am not sure of the logic behind this. It seems to me that you are making marking a device busy and dropping the reference synonymous.

Is the time you use to derive the likelihood determined by the start of IO or the end of IO?