diff mbox series

[v2,2/7] pm: runtime: Add pm_runtime_put_mark_busy_autosusp() helper

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

Commit Message

Sakari Ailus Nov. 17, 2023, 11:14 a.m. UTC
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(+)

Comments

Laurent Pinchart Nov. 18, 2023, 5:49 p.m. UTC | #1
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.
Rafael J. Wysocki Nov. 18, 2023, 9:20 p.m. UTC | #2
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.
Laurent Pinchart Nov. 18, 2023, 9:30 p.m. UTC | #3
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.
Sakari Ailus Nov. 20, 2023, 9:27 a.m. UTC | #4
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().
Laurent Pinchart Nov. 20, 2023, 9:47 a.m. UTC | #5
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 :-)
Sakari Ailus Nov. 21, 2023, 8:41 a.m. UTC | #6
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.
Laurent Pinchart Nov. 21, 2023, 8:50 a.m. UTC | #7
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.
Sakari Ailus Nov. 21, 2023, 10 a.m. UTC | #8
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 mbox series

Patch

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.