| Message ID | 20250410153106.4146265-1-sakari.ailus@linux.intel.com |
|---|---|
| Headers | show |
| Series | Update last busy timestamp in Runtime PM autosuspend callbacks | expand |
Hi Sakari, Thank you for the patch. On Thu, Apr 10, 2025 at 06:31:02PM +0300, Sakari Ailus wrote: > Set device's last busy timestamp to current time in > pm_runtime_put_autosuspend(). Callers wishing not to do that will need to > use __pm_runtime_put_autosuspend(). > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > Documentation/power/runtime_pm.rst | 23 ++++++++++------------- > include/linux/pm_runtime.h | 12 +++++++----- > 2 files changed, 17 insertions(+), 18 deletions(-) > > diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst > index 63344bea8393..e7bbdc66d64c 100644 > --- a/Documentation/power/runtime_pm.rst > +++ b/Documentation/power/runtime_pm.rst > @@ -411,8 +411,9 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h: > pm_request_idle(dev) and return its result > > `int pm_runtime_put_autosuspend(struct device *dev);` > - - does the same as __pm_runtime_put_autosuspend() for now, but in the > - future, will also call pm_runtime_mark_last_busy() as well, DO NOT USE! > + - set the power.last_busy field to the current time and decrement the > + device's usage counter; if the result is 0 then run > + pm_request_autosuspend(dev) and return its result > > `int __pm_runtime_put_autosuspend(struct device *dev);` > - decrement the device's usage counter; if the result is 0 then run > @@ -870,11 +871,9 @@ device is automatically suspended (the subsystem or driver still has to call > the appropriate PM routines); rather it means that runtime suspends will > automatically be delayed until the desired period of inactivity has elapsed. > > -Inactivity is determined based on the power.last_busy field. Drivers should > -call pm_runtime_mark_last_busy() to update this field after carrying out I/O, > -typically just before calling __pm_runtime_put_autosuspend(). The desired > -length of the inactivity period is a matter of policy. Subsystems can set this > -length initially by calling pm_runtime_set_autosuspend_delay(), but after device > +Inactivity is determined based on the power.last_busy field. The desired length > +of the inactivity period is a matter of policy. Subsystems can set this length > +initially by calling pm_runtime_set_autosuspend_delay(), but after device > registration the length should be controlled by user space, using the > /sys/devices/.../power/autosuspend_delay_ms attribute. > > @@ -885,7 +884,7 @@ instead of the non-autosuspend counterparts:: > > Instead of: pm_runtime_suspend use: pm_runtime_autosuspend; > Instead of: pm_schedule_suspend use: pm_request_autosuspend; > - Instead of: pm_runtime_put use: __pm_runtime_put_autosuspend; > + Instead of: pm_runtime_put use: pm_runtime_put_autosuspend; > Instead of: pm_runtime_put_sync use: pm_runtime_put_sync_autosuspend. > > Drivers may also continue to use the non-autosuspend helper functions; they > @@ -922,12 +921,10 @@ Here is a schematic pseudo-code example:: > foo_io_completion(struct foo_priv *foo, void *req) > { > lock(&foo->private_lock); > - if (--foo->num_pending_requests == 0) { > - pm_runtime_mark_last_busy(&foo->dev); > - __pm_runtime_put_autosuspend(&foo->dev); > - } else { > + if (--foo->num_pending_requests == 0) > + pm_runtime_put_autosuspend(&foo->dev); > + else > foo_process_next_request(foo); > - } > unlock(&foo->private_lock); > /* Send req result back to the user ... */ > } > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h > index 3e31cbebc527..0ade3f75d903 100644 > --- a/include/linux/pm_runtime.h > +++ b/include/linux/pm_runtime.h > @@ -562,11 +562,13 @@ static inline int __pm_runtime_put_autosuspend(struct device *dev) > } > > /** > - * pm_runtime_put_autosuspend - Drop device usage counter and queue autosuspend if 0. > + * pm_runtime_put_autosuspend - Update the last access time of a device, drop > + * its usage counter and queue autosuspend if the usage counter becomes 0. > * @dev: Target device. > * > - * 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_autosuspend(). > + * Update the last access time of @dev and decrement its runtime PM usage s/ and decrement/, decrement/ Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + * counter and if it turns out to be equal to 0, queue up a work item for @dev > + * like in pm_request_autosuspend(). > * > * Return: > * * 0: Success. > @@ -581,8 +583,8 @@ static inline int __pm_runtime_put_autosuspend(struct device *dev) > */ > static inline int pm_runtime_put_autosuspend(struct device *dev) > { > - return __pm_runtime_suspend(dev, > - RPM_GET_PUT | RPM_ASYNC | RPM_AUTO); > + pm_runtime_mark_last_busy(dev); > + return __pm_runtime_put_autosuspend(dev); > } > > /**
Hi Sakari, Thank you for the patch. On Thu, Apr 10, 2025 at 06:31:06PM +0300, Sakari Ailus wrote: > Document that the *_autosuspend() variants of the Runtime PM functions > update the last busy timestamp. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Documentation/power/runtime_pm.rst | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst > index 91bc93422262..c8dbdb8595e5 100644 > --- a/Documentation/power/runtime_pm.rst > +++ b/Documentation/power/runtime_pm.rst > @@ -887,7 +887,8 @@ instead of the non-autosuspend counterparts:: > > Drivers may also continue to use the non-autosuspend helper functions; they > will behave normally, which means sometimes taking the autosuspend delay into > -account (see pm_runtime_idle). > +account (see pm_runtime_idle). The autosuspend variants of the functions also > +call pm_runtime_mark_last_busy(). > > Under some circumstances a driver or subsystem may want to prevent a device > from autosuspending immediately, even though the usage counter is zero and the
Hi Laurent, Thanks for the review. On Thu, Apr 10, 2025 at 11:17:11PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Thu, Apr 10, 2025 at 06:31:02PM +0300, Sakari Ailus wrote: > > Set device's last busy timestamp to current time in > > pm_runtime_put_autosuspend(). Callers wishing not to do that will need to > > use __pm_runtime_put_autosuspend(). > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > Documentation/power/runtime_pm.rst | 23 ++++++++++------------- > > include/linux/pm_runtime.h | 12 +++++++----- > > 2 files changed, 17 insertions(+), 18 deletions(-) > > > > diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst > > index 63344bea8393..e7bbdc66d64c 100644 > > --- a/Documentation/power/runtime_pm.rst > > +++ b/Documentation/power/runtime_pm.rst > > @@ -411,8 +411,9 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h: > > pm_request_idle(dev) and return its result > > > > `int pm_runtime_put_autosuspend(struct device *dev);` > > - - does the same as __pm_runtime_put_autosuspend() for now, but in the > > - future, will also call pm_runtime_mark_last_busy() as well, DO NOT USE! > > + - set the power.last_busy field to the current time and decrement the > > + device's usage counter; if the result is 0 then run > > + pm_request_autosuspend(dev) and return its result > > > > `int __pm_runtime_put_autosuspend(struct device *dev);` > > - decrement the device's usage counter; if the result is 0 then run > > @@ -870,11 +871,9 @@ device is automatically suspended (the subsystem or driver still has to call > > the appropriate PM routines); rather it means that runtime suspends will > > automatically be delayed until the desired period of inactivity has elapsed. > > > > -Inactivity is determined based on the power.last_busy field. Drivers should > > -call pm_runtime_mark_last_busy() to update this field after carrying out I/O, > > -typically just before calling __pm_runtime_put_autosuspend(). The desired > > -length of the inactivity period is a matter of policy. Subsystems can set this > > -length initially by calling pm_runtime_set_autosuspend_delay(), but after device > > +Inactivity is determined based on the power.last_busy field. The desired length > > +of the inactivity period is a matter of policy. Subsystems can set this length > > +initially by calling pm_runtime_set_autosuspend_delay(), but after device > > registration the length should be controlled by user space, using the > > /sys/devices/.../power/autosuspend_delay_ms attribute. > > > > @@ -885,7 +884,7 @@ instead of the non-autosuspend counterparts:: > > > > Instead of: pm_runtime_suspend use: pm_runtime_autosuspend; > > Instead of: pm_schedule_suspend use: pm_request_autosuspend; > > - Instead of: pm_runtime_put use: __pm_runtime_put_autosuspend; > > + Instead of: pm_runtime_put use: pm_runtime_put_autosuspend; > > Instead of: pm_runtime_put_sync use: pm_runtime_put_sync_autosuspend. > > > > Drivers may also continue to use the non-autosuspend helper functions; they > > @@ -922,12 +921,10 @@ Here is a schematic pseudo-code example:: > > foo_io_completion(struct foo_priv *foo, void *req) > > { > > lock(&foo->private_lock); > > - if (--foo->num_pending_requests == 0) { > > - pm_runtime_mark_last_busy(&foo->dev); > > - __pm_runtime_put_autosuspend(&foo->dev); > > - } else { > > + if (--foo->num_pending_requests == 0) > > + pm_runtime_put_autosuspend(&foo->dev); > > + else > > foo_process_next_request(foo); > > - } > > unlock(&foo->private_lock); > > /* Send req result back to the user ... */ > > } > > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h > > index 3e31cbebc527..0ade3f75d903 100644 > > --- a/include/linux/pm_runtime.h > > +++ b/include/linux/pm_runtime.h > > @@ -562,11 +562,13 @@ static inline int __pm_runtime_put_autosuspend(struct device *dev) > > } > > > > /** > > - * pm_runtime_put_autosuspend - Drop device usage counter and queue autosuspend if 0. > > + * pm_runtime_put_autosuspend - Update the last access time of a device, drop > > + * its usage counter and queue autosuspend if the usage counter becomes 0. > > * @dev: Target device. > > * > > - * 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_autosuspend(). > > + * Update the last access time of @dev and decrement its runtime PM usage > > s/ and decrement/, decrement/ Ack. I'll address this in v2, but wait for other comments still awhile. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + * counter and if it turns out to be equal to 0, queue up a work item for @dev > > + * like in pm_request_autosuspend(). > > * > > * Return: > > * * 0: Success. > > @@ -581,8 +583,8 @@ static inline int __pm_runtime_put_autosuspend(struct device *dev) > > */ > > static inline int pm_runtime_put_autosuspend(struct device *dev) > > { > > - return __pm_runtime_suspend(dev, > > - RPM_GET_PUT | RPM_ASYNC | RPM_AUTO); > > + pm_runtime_mark_last_busy(dev); > > + return __pm_runtime_put_autosuspend(dev); > > } > > > > /**
On Fri, Apr 11, 2025 at 06:27:06AM +0000, Sakari Ailus wrote: > > s/ and decrement/, decrement/ > > Ack. I'll address this in v2, but wait for other comments still awhile. The paragraph now looks like: * Update the last access time of @dev, decrement 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_autosuspend(). I.e. runtime PM usage counter of @dev is decremented, not the usage counter of the last access time.
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/20241004094101.113349-1-sakari.ailus@linux.intel.com/>. 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. 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); ) Sakari Ailus (7): Documentation: pm: runtime: Fix a reference to pm_runtime_autosuspend() 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(-)