| Message ID | 20231117111433.1561669-3-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Small Runtime PM API changes | expand |
Hi Sakari, Thank you for the patch. On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote: > Add pm_runtime_put_mark_busy_autosusp() helper function for users that > wish to set the last_busy timestamp to current time and put the > usage_count of the device and set the autosuspend timer. > > Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to > calling first pm_runtime_mark_last_busy() and then > pm_runtime_put_autosuspend(). The vast majority if the pm_runtime_put_autosuspend() users call pm_runtime_mark_last_busy() right before. Let's make the pm_runtime_put_autosuspend() function do that by default, and add a __pm_runtime_put_autosuspend() (name to be bikshedded) for the minority of cases where updating the last busy timestamp isn't desired. We want to simplify the API, not make it more complex. > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > include/linux/pm_runtime.h | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h > index 13cd526634c1..4afe7b0b9d7e 100644 > --- a/include/linux/pm_runtime.h > +++ b/include/linux/pm_runtime.h > @@ -495,6 +495,23 @@ static inline int pm_runtime_put_autosuspend(struct device *dev) > RPM_GET_PUT | RPM_ASYNC | RPM_AUTO); > } > > +/** > + * pm_runtime_put_mark_busy_autosusp - Update the last access time of a device > + * and drop device usage counter and queue > + * autosuspend if 0. > + * @dev: Target device. > + * > + * Update the last access time of @dev using pm_runtime_mark_last_busy(), then > + * 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(). > + */ > +static inline int pm_runtime_put_mark_busy_autosusp(struct device *dev) > +{ > + pm_runtime_mark_last_busy(dev); > + > + return pm_runtime_autosuspend(dev); > +} > + > /** > * pm_runtime_put_sync - Drop device usage counter and run "idle check" if 0. > * @dev: Target device.
On Sat, Nov 18, 2023 at 6:49 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Sakari, > > Thank you for the patch. > > On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote: > > Add pm_runtime_put_mark_busy_autosusp() helper function for users that > > wish to set the last_busy timestamp to current time and put the > > usage_count of the device and set the autosuspend timer. > > > > Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to > > calling first pm_runtime_mark_last_busy() and then > > pm_runtime_put_autosuspend(). > > The vast majority if the pm_runtime_put_autosuspend() users call > pm_runtime_mark_last_busy() right before. Let's make the > pm_runtime_put_autosuspend() function do that by default, and add a > __pm_runtime_put_autosuspend() (name to be bikshedded) for the minority > of cases where updating the last busy timestamp isn't desired. We want > to simplify the API, not make it more complex. I would also prefer it to be done this way if not too problematic.
Hi Rafael, On Sat, Nov 18, 2023 at 10:20:46PM +0100, Rafael J. Wysocki wrote: > On Sat, Nov 18, 2023 at 6:49 PM Laurent Pinchart wrote: > > On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote: > > > Add pm_runtime_put_mark_busy_autosusp() helper function for users that > > > wish to set the last_busy timestamp to current time and put the > > > usage_count of the device and set the autosuspend timer. > > > > > > Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to > > > calling first pm_runtime_mark_last_busy() and then > > > pm_runtime_put_autosuspend(). > > > > The vast majority if the pm_runtime_put_autosuspend() users call > > pm_runtime_mark_last_busy() right before. Let's make the > > pm_runtime_put_autosuspend() function do that by default, and add a > > __pm_runtime_put_autosuspend() (name to be bikshedded) for the minority > > of cases where updating the last busy timestamp isn't desired. We want > > to simplify the API, not make it more complex. > > I would also prefer it to be done this way if not too problematic. I'm glad you agree :-) The change will probably be a bit painful, but I think it's for the best. Sakari, please let me know if I can help.
Hi Laurent, On Sat, Nov 18, 2023 at 11:30:31PM +0200, Laurent Pinchart wrote: > Hi Rafael, > > On Sat, Nov 18, 2023 at 10:20:46PM +0100, Rafael J. Wysocki wrote: > > On Sat, Nov 18, 2023 at 6:49 PM Laurent Pinchart wrote: > > > On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote: > > > > Add pm_runtime_put_mark_busy_autosusp() helper function for users that > > > > wish to set the last_busy timestamp to current time and put the > > > > usage_count of the device and set the autosuspend timer. > > > > > > > > Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to > > > > calling first pm_runtime_mark_last_busy() and then > > > > pm_runtime_put_autosuspend(). > > > > > > The vast majority if the pm_runtime_put_autosuspend() users call > > > pm_runtime_mark_last_busy() right before. Let's make the > > > pm_runtime_put_autosuspend() function do that by default, and add a > > > __pm_runtime_put_autosuspend() (name to be bikshedded) for the minority > > > of cases where updating the last busy timestamp isn't desired. We want > > > to simplify the API, not make it more complex. > > > > I would also prefer it to be done this way if not too problematic. > > I'm glad you agree :-) The change will probably be a bit painful, but I > think it's for the best. Sakari, please let me know if I can help. I actually do prefer this approach, too. There about 350 drivers using pm_runtime_autosuspend() currently. Around 150 uses pm_runtime_autosuspend() which is not preceded by pm_runtime_mark_last_busy(). Call-wise the numbers are ~ 1050 and ~ 330. I checked some of what's left: most do still call both, but in a way that evades Coccinelle matching. Some omissions seem to remain. Given that there are way more users that do also call pm_runtime_mark_last_busy(), I think I'll try to introduce __pm_runtime_put_autosuspend() and pm_runtime_put_autosuspend() documentation change first and then rename the callers that don't use pm_runtime_mark_last_busy().
Hi Sakari, On Mon, Nov 20, 2023 at 09:27:51AM +0000, Sakari Ailus wrote: > On Sat, Nov 18, 2023 at 11:30:31PM +0200, Laurent Pinchart wrote: > > On Sat, Nov 18, 2023 at 10:20:46PM +0100, Rafael J. Wysocki wrote: > > > On Sat, Nov 18, 2023 at 6:49 PM Laurent Pinchart wrote: > > > > On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote: > > > > > Add pm_runtime_put_mark_busy_autosusp() helper function for users that > > > > > wish to set the last_busy timestamp to current time and put the > > > > > usage_count of the device and set the autosuspend timer. > > > > > > > > > > Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to > > > > > calling first pm_runtime_mark_last_busy() and then > > > > > pm_runtime_put_autosuspend(). > > > > > > > > The vast majority if the pm_runtime_put_autosuspend() users call > > > > pm_runtime_mark_last_busy() right before. Let's make the > > > > pm_runtime_put_autosuspend() function do that by default, and add a > > > > __pm_runtime_put_autosuspend() (name to be bikshedded) for the minority > > > > of cases where updating the last busy timestamp isn't desired. We want > > > > to simplify the API, not make it more complex. > > > > > > I would also prefer it to be done this way if not too problematic. > > > > I'm glad you agree :-) The change will probably be a bit painful, but I > > think it's for the best. Sakari, please let me know if I can help. > > I actually do prefer this approach, too. > > There about 350 drivers using pm_runtime_autosuspend() currently. Around > 150 uses pm_runtime_autosuspend() which is not preceded by > pm_runtime_mark_last_busy(). Call-wise the numbers are ~ 1050 and ~ 330. > > I checked some of what's left: most do still call both, but in a way that > evades Coccinelle matching. Some omissions seem to remain. > > Given that there are way more users that do also call > pm_runtime_mark_last_busy(), I think I'll try to introduce > __pm_runtime_put_autosuspend() and pm_runtime_put_autosuspend() > documentation change first and then rename the callers that don't use > pm_runtime_mark_last_busy(). And also drop pm_runtime_mark_last_busy() from the drivers that call pm_runtime_put_autosuspend(), right ? This sounds good to me. Thank you for working on this. Two RPM API simplifications in a week, it feels like Christmas is coming :-)
Hi Laurent, On Mon, Nov 20, 2023 at 11:47:43AM +0200, Laurent Pinchart wrote: > Hi Sakari, > > On Mon, Nov 20, 2023 at 09:27:51AM +0000, Sakari Ailus wrote: > > On Sat, Nov 18, 2023 at 11:30:31PM +0200, Laurent Pinchart wrote: > > > On Sat, Nov 18, 2023 at 10:20:46PM +0100, Rafael J. Wysocki wrote: > > > > On Sat, Nov 18, 2023 at 6:49 PM Laurent Pinchart wrote: > > > > > On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote: > > > > > > Add pm_runtime_put_mark_busy_autosusp() helper function for users that > > > > > > wish to set the last_busy timestamp to current time and put the > > > > > > usage_count of the device and set the autosuspend timer. > > > > > > > > > > > > Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to > > > > > > calling first pm_runtime_mark_last_busy() and then > > > > > > pm_runtime_put_autosuspend(). > > > > > > > > > > The vast majority if the pm_runtime_put_autosuspend() users call > > > > > pm_runtime_mark_last_busy() right before. Let's make the > > > > > pm_runtime_put_autosuspend() function do that by default, and add a > > > > > __pm_runtime_put_autosuspend() (name to be bikshedded) for the minority > > > > > of cases where updating the last busy timestamp isn't desired. We want > > > > > to simplify the API, not make it more complex. > > > > > > > > I would also prefer it to be done this way if not too problematic. > > > > > > I'm glad you agree :-) The change will probably be a bit painful, but I > > > think it's for the best. Sakari, please let me know if I can help. > > > > I actually do prefer this approach, too. > > > > There about 350 drivers using pm_runtime_autosuspend() currently. Around > > 150 uses pm_runtime_autosuspend() which is not preceded by > > pm_runtime_mark_last_busy(). Call-wise the numbers are ~ 1050 and ~ 330. > > > > I checked some of what's left: most do still call both, but in a way that > > evades Coccinelle matching. Some omissions seem to remain. > > > > Given that there are way more users that do also call > > pm_runtime_mark_last_busy(), I think I'll try to introduce > > __pm_runtime_put_autosuspend() and pm_runtime_put_autosuspend() > > documentation change first and then rename the callers that don't use > > pm_runtime_mark_last_busy(). > > And also drop pm_runtime_mark_last_busy() from the drivers that call > pm_runtime_put_autosuspend(), right ? That should be done but as it doesn't affect the functionality, it can (and may only) be done later on --- the current users need to be converted to use the to-be-added __pm_runtime_put_autosuspend() first. > > This sounds good to me. Thank you for working on this. Two RPM API > simplifications in a week, it feels like Christmas is coming :-) Yes. And it's always the case actually! Only the time that it takes differs.
Hi Sakari, On Tue, Nov 21, 2023 at 08:41:34AM +0000, Sakari Ailus wrote: > On Mon, Nov 20, 2023 at 11:47:43AM +0200, Laurent Pinchart wrote: > > On Mon, Nov 20, 2023 at 09:27:51AM +0000, Sakari Ailus wrote: > > > On Sat, Nov 18, 2023 at 11:30:31PM +0200, Laurent Pinchart wrote: > > > > On Sat, Nov 18, 2023 at 10:20:46PM +0100, Rafael J. Wysocki wrote: > > > > > On Sat, Nov 18, 2023 at 6:49 PM Laurent Pinchart wrote: > > > > > > On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote: > > > > > > > Add pm_runtime_put_mark_busy_autosusp() helper function for users that > > > > > > > wish to set the last_busy timestamp to current time and put the > > > > > > > usage_count of the device and set the autosuspend timer. > > > > > > > > > > > > > > Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to > > > > > > > calling first pm_runtime_mark_last_busy() and then > > > > > > > pm_runtime_put_autosuspend(). > > > > > > > > > > > > The vast majority if the pm_runtime_put_autosuspend() users call > > > > > > pm_runtime_mark_last_busy() right before. Let's make the > > > > > > pm_runtime_put_autosuspend() function do that by default, and add a > > > > > > __pm_runtime_put_autosuspend() (name to be bikshedded) for the minority > > > > > > of cases where updating the last busy timestamp isn't desired. We want > > > > > > to simplify the API, not make it more complex. > > > > > > > > > > I would also prefer it to be done this way if not too problematic. > > > > > > > > I'm glad you agree :-) The change will probably be a bit painful, but I > > > > think it's for the best. Sakari, please let me know if I can help. > > > > > > I actually do prefer this approach, too. > > > > > > There about 350 drivers using pm_runtime_autosuspend() currently. Around > > > 150 uses pm_runtime_autosuspend() which is not preceded by > > > pm_runtime_mark_last_busy(). Call-wise the numbers are ~ 1050 and ~ 330. > > > > > > I checked some of what's left: most do still call both, but in a way that > > > evades Coccinelle matching. Some omissions seem to remain. > > > > > > Given that there are way more users that do also call > > > pm_runtime_mark_last_busy(), I think I'll try to introduce > > > __pm_runtime_put_autosuspend() and pm_runtime_put_autosuspend() > > > documentation change first and then rename the callers that don't use > > > pm_runtime_mark_last_busy(). > > > > And also drop pm_runtime_mark_last_busy() from the drivers that call > > pm_runtime_put_autosuspend(), right ? > > That should be done but as it doesn't affect the functionality, it can (and > may only) be done later on --- the current users need to be converted to > use the to-be-added __pm_runtime_put_autosuspend() first. True. If you're going to send a series that change things tree-wide I was thinking that it would be best to address multiple tree-wide changes at the same time, that would be less churn, especially if it can be mostly scripted with Coccinelle. That would be my preference (especially because the issue will then be solved and we'll be able to move to something else, instead of leaving old code lingering on for a long time), but it's up to you. > > This sounds good to me. Thank you for working on this. Two RPM API > > simplifications in a week, it feels like Christmas is coming :-) > > Yes. And it's always the case actually! Only the time that it takes > differs.
Hi Laurent, On Tue, Nov 21, 2023 at 10:50:56AM +0200, Laurent Pinchart wrote: > Hi Sakari, > > On Tue, Nov 21, 2023 at 08:41:34AM +0000, Sakari Ailus wrote: > > On Mon, Nov 20, 2023 at 11:47:43AM +0200, Laurent Pinchart wrote: > > > On Mon, Nov 20, 2023 at 09:27:51AM +0000, Sakari Ailus wrote: > > > > On Sat, Nov 18, 2023 at 11:30:31PM +0200, Laurent Pinchart wrote: > > > > > On Sat, Nov 18, 2023 at 10:20:46PM +0100, Rafael J. Wysocki wrote: > > > > > > On Sat, Nov 18, 2023 at 6:49 PM Laurent Pinchart wrote: > > > > > > > On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote: > > > > > > > > Add pm_runtime_put_mark_busy_autosusp() helper function for users that > > > > > > > > wish to set the last_busy timestamp to current time and put the > > > > > > > > usage_count of the device and set the autosuspend timer. > > > > > > > > > > > > > > > > Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to > > > > > > > > calling first pm_runtime_mark_last_busy() and then > > > > > > > > pm_runtime_put_autosuspend(). > > > > > > > > > > > > > > The vast majority if the pm_runtime_put_autosuspend() users call > > > > > > > pm_runtime_mark_last_busy() right before. Let's make the > > > > > > > pm_runtime_put_autosuspend() function do that by default, and add a > > > > > > > __pm_runtime_put_autosuspend() (name to be bikshedded) for the minority > > > > > > > of cases where updating the last busy timestamp isn't desired. We want > > > > > > > to simplify the API, not make it more complex. > > > > > > > > > > > > I would also prefer it to be done this way if not too problematic. > > > > > > > > > > I'm glad you agree :-) The change will probably be a bit painful, but I > > > > > think it's for the best. Sakari, please let me know if I can help. > > > > > > > > I actually do prefer this approach, too. > > > > > > > > There about 350 drivers using pm_runtime_autosuspend() currently. Around > > > > 150 uses pm_runtime_autosuspend() which is not preceded by > > > > pm_runtime_mark_last_busy(). Call-wise the numbers are ~ 1050 and ~ 330. > > > > > > > > I checked some of what's left: most do still call both, but in a way that > > > > evades Coccinelle matching. Some omissions seem to remain. > > > > > > > > Given that there are way more users that do also call > > > > pm_runtime_mark_last_busy(), I think I'll try to introduce > > > > __pm_runtime_put_autosuspend() and pm_runtime_put_autosuspend() > > > > documentation change first and then rename the callers that don't use > > > > pm_runtime_mark_last_busy(). > > > > > > And also drop pm_runtime_mark_last_busy() from the drivers that call > > > pm_runtime_put_autosuspend(), right ? > > > > That should be done but as it doesn't affect the functionality, it can (and > > may only) be done later on --- the current users need to be converted to > > use the to-be-added __pm_runtime_put_autosuspend() first. > > True. If you're going to send a series that change things tree-wide I > was thinking that it would be best to address multiple tree-wide changes > at the same time, that would be less churn, especially if it can be > mostly scripted with Coccinelle. That would be my preference (especially > because the issue will then be solved and we'll be able to move to > something else, instead of leaving old code lingering on for a long > time), but it's up to you. This will mean around 1000 changed lines in 350 files. Given the number of changes and how they're scattered around, I'd expect to merge first the Runtime PM API change, then later on the driver specific changes via their own trees. Doing it differently risks a large number of conflicts. Hopefully faster than changing the I²C driver probe function though. We also need to have some time before all users of pm_runtime_put_autosuspend() have been converted, including new ones merged meanwhile.
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h index 13cd526634c1..4afe7b0b9d7e 100644 --- a/include/linux/pm_runtime.h +++ b/include/linux/pm_runtime.h @@ -495,6 +495,23 @@ static inline int pm_runtime_put_autosuspend(struct device *dev) RPM_GET_PUT | RPM_ASYNC | RPM_AUTO); } +/** + * pm_runtime_put_mark_busy_autosusp - Update the last access time of a device + * and drop device usage counter and queue + * autosuspend if 0. + * @dev: Target device. + * + * Update the last access time of @dev using pm_runtime_mark_last_busy(), then + * 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(). + */ +static inline int pm_runtime_put_mark_busy_autosusp(struct device *dev) +{ + pm_runtime_mark_last_busy(dev); + + return pm_runtime_autosuspend(dev); +} + /** * pm_runtime_put_sync - Drop device usage counter and run "idle check" if 0. * @dev: Target device.
Add pm_runtime_put_mark_busy_autosusp() helper function for users that wish to set the last_busy timestamp to current time and put the usage_count of the device and set the autosuspend timer. Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to calling first pm_runtime_mark_last_busy() and then pm_runtime_put_autosuspend(). Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- include/linux/pm_runtime.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)