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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.

-- 
Regards,

Laurent Pinchart