Linux IIO development
 help / color / mirror / Atom feed
From: Brian Norris <briannorris@chromium.org>
To: Alan Stern <stern@rowland.harvard.edu>,
	"Rafael J. Wysocki (Intel)" <rafael@kernel.org>
Cc: "Rafael J. Wysocki (Intel)" <rafael@kernel.org>,
	"Fabrice Gasnier" <fabrice.gasnier@st.com>,
	linux-pm@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	"Ulf Hansson" <ulfh@kernel.org>,
	linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
	"Andy Shevchenko" <andy@kernel.org>,
	"Jonathan Cameron" <jic23@kernel.org>,
	"David Lechner" <dlechner@baylibre.com>,
	"Alexandre Torgue" <alexandre.torgue@foss.st.com>,
	"Nuno Sá" <nuno.sa@analog.com>,
	"Maxime Coquelin" <mcoquelin.stm32@gmail.com>
Subject: Re: [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency
Date: Wed, 11 Nov 2026 16:23:55 -0800	[thread overview]
Message-ID: <avUHG8mLuTOUMr8O@google.com> (raw)
In-Reply-To: <fc43eb13-816b-4cd0-9711-848fdab605ef@rowland.harvard.edu>

Hi Alan,

On Wed, Nov 11, 2026 at 03:20:16PM -0500, Alan Stern wrote:
> On Wed, Nov 11, 2026 at 08:42:31AM -0800, Brian Norris wrote:
> > Hi Alan,
> > 
> > On Tue, Nov 10, 2026 at 10:47:59PM -0500, Alan Stern wrote:
> > > So yes, in the current scheme a parent's inactivity period doesn't start 
> > > until all its children are suspended (unless the ignore_children flag is 
> > > set, but never mind that now).
> > 
> > But to be clear, that's not actually how runtime PM works today. It only
> > works this way for the very small handful of drivers where the parent
> > calls pm_runtime_mark_last_busy() in .runtime_idle(). I think several
> > people have acknolwedged that this is a bit of a hack, or at least a
> > missing feature in runtime PM, and probably should be extended more
> > widely.
> 
> I have to agree; it seems like something we missed.  But then, I've 
> always had something of a blind spot for pm_runtime_idle().  In fact, my 
> overall impression has been that in practice, drivers relying on 
> pm_runtime_idle() for anything nontrivial would be more or less mutually 
> exclusive with drivers relying on autosuspend.

I also had that impression. But of the approx 60 drivers that implement
runtime_idle, there are at least 4 GPU drivers: nouveau, radeon, xe_pci
and amdgpu (I'm sure they all copied each other to some extent). There's
also some greybus, drivers/misc/mei/gsc-me.c, and a handful of others.

I suppose you can determine for yourself what's "nontrivial"; and in
many cases, they re-implement many of the same idle checks inside their
runtime_suspend(), so it doesn't matter too much.

> But of course, 
> maintainers are free to do whatever they want.
> 
> > > The parent's autosuspend delay therefore 
> > > should take into account the expected time before one of the children 
> > > may have to be resumed, as well as the expected time before the parent 
> > > itself may be asked to do something that doesn't involve any children.
> > > (Note that if a device has more than one child, the expected time before 
> > > any of its children will be resumed can be different from the expected 
> > > time before any single particular child will be resumed.)
> > 
> > "Expected time before a child may have to resume": isn't that child
> > last_busy + child delay?
> 
> No; that value is the expected time before the child may be 
> autosuspended.  Not to mention that you're mixing up clock times 
> (last_busy) with time intervals (time before).
> 
> >  So that expected time is already in the past by
> > the time the child autosuspends.
> 
> You must have misunderstood what I wrote.  Think of it this way: Let's 
> say the last remaining active child goes into runtime suspend at time T.  
> Let's also say that the parent expects none of the children will need to 
> resume until time T + D.  Is D sufficiently large to make it worthwhile 
> for the parent to suspend and then resume?  If not, the parent should 
> remain active.  If yes, the parent should autosuspend -- and the length 
> of time the parent should wait before autosuspending is on the order of 
> the value of D (let's call it D*) that corresponds to this crossover 
> point.  Thus the parent's autosuspend delay should be set to D*, or 
> something close to it, in order to achieve both objectives.

OK, thanks for explaining again.

I suppose that makes sense, and so Rafael's
rpm_idle()-implies-mark_last_busy() proposal makes sense.

> > My point here was that I think Rafael was overly concerned about
> > precisely tuning parent autosuspend delays at the same time as changing
> > the runtime PM core behavior. Yes, they clearly can be tuned in user
> > space; but he was suggesting a phase-in / opt-in approach to this
> > change, because it might harm system power if it's not re-tuned.
> 
> Is anybody really trying to tune precisely parent autosuspend delays?  
> On what basis?  I'm doubtful that anyone is sufficiently serious about 
> it to be annoyed at this PM core change (i.e., setting last_busy when a 
> child suspends).

I fully agree with you, which is why I don't see why we should make
Rafael's proposal conditional. We should just make the change, IMO.

> > > > At the same time, it makes it harder to reason about the runtime PM API
> > > > surface -- because currently, .runtime_idle() is a primary reason that
> > > > pm_runtime_put() and pm_runtime_put_autosuspend() are different.
> > > 
> > > I wouldn't put it that way.  My feeling is that pm_runtime_put() could 
> > > be replaced by pm_runtime_put_autosuspend(), since they should behave 
> > > identically for devices that don't use autosuspend.
> > 
> > Ah, but that's not actually true! pm_runtime_put_autosuspend() skips
> > .runtime_idle(). It (semi)directly calls rpm_suspend().
> 
> Like I said above, a blind spot regarding pm_runtime_idle().  And/or an 
> impression that a driver might want to use pm_runtime_idle() or 
> autosuspend but not both.
> 
> Anyway, pm_runtime_put_autosuspend() could easily be modified to call 
> pm_runtime_idle() before rpm_suspend() if the device doesn't use 
> autosuspend.  That's not likely to bother anybody.

Sure.

One way or another, I'd like to eliminate the differences. And I'd
prefer doing so by removal, rather than addition, if possible. TBD how
it ends up looking, after the current topic.

> > I appreciate all the responses. One particular part I did have questions
> > on, which I'll mostly copy/paste:
> > 
> > [Rafael:]
> > > Now, let me acknowledge that some drivers evidently want rpm_idle() to
> > > always set the autosuspend timer instead of suspending the device
> > > right away.  That may be because of the "unwanted parent suspends"
> > > problem at hand or for other reasons.
> > 
> > Do we have any idea what the "other reasons" could be? For example,
> > because driver writers found it too difficult to find all the
> > appropriate places to call pm_runtime_mark_last_busy()? (Does commit
> > b3db492e8335 ("PM: runtime: Mark last busy stamp in
> > pm_runtime_put_autosuspend()") help? Or should pm_runtime_put() behave
> > similarly?)
> 
> I have no idea.  Outside the area where I mainly work, I don't know what 
> sorts of things driver writers are doing.  If you know any people who 
> are doing this, you should ask them directly.

There's so much ancient history and copy/paste that it's a bit hard for
me to find out who's doing this for what reasons, outside of nicely
documented changes like commit c537d3457542 ("iio: adc: stm32-adc: fix
runtime autosuspend delay when slow polling").

> > I see that USB is one of the subsystems where .runtime_idle() calls
> > pm_runtime_mark_last_busy(), and I thought you (Alan) might have some
> > more background on that.
> 
> I don't remember exactly why that was done, but probably it was meant to 
> carry out the behavior we've been discussing (autosuspend delay should 
> start when the device and all its children become idle).

OK, thanks for your thoughts. In any case, maybe it doesn't matter why,
if we're simply going to move that behavior into the runtime PM core
as-is.

I'll give this some time in case Rafael had any other thoughts/replies,
but unless something major changes, I'll plan to write/test/submit v3
where rpm_idle() calls pm_runtime_mark_last_busy() (part of Rafael's
proposal), and *not* make it conditional (i.e., no
pm_runtime_use_autosuspend_always()).

(Or, Rafael is free to do so, since it's his idea. Just let me know so I
don't waste my time on it. But I figure he'd rather someone else do the
work.)

Regards,
Brian

  reply	other threads:[~2026-11-12  0:23 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-11-03 22:14 [PATCH v2 0/5] PM: runtime: Improve autosuspend child to parent propagation Brian Norris
2026-11-03 22:14 ` [PATCH v2 1/5] PM: runtime: Only queue an idle check for RPM-linked suppliers (part 2) Brian Norris
2026-11-04 14:44   ` Rafael J. Wysocki (Intel)
2026-11-03 22:14 ` [PATCH v2 2/5] PM: runtime: Convert last_busy to atomic64_t Brian Norris
2026-11-04  8:21   ` Joshua Crofts
2026-11-03 22:14 ` [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency Brian Norris
2026-11-03 23:17   ` Jonathan Cameron
2026-11-04  0:32     ` Brian Norris
2026-11-04  1:40       ` Jonathan Cameron
2026-11-04 13:34   ` Rafael J. Wysocki (Intel)
2026-11-04 19:31     ` Brian Norris
2026-11-05 11:59       ` Rafael J. Wysocki (Intel)
2026-11-05 20:21         ` Brian Norris
2026-11-05 21:18           ` Rafael J. Wysocki (Intel)
2026-11-06  1:06             ` Brian Norris
2026-11-09 15:24               ` Rafael J. Wysocki (Intel)
2026-11-11  0:04                 ` Brian Norris
2026-11-11  3:47                   ` Alan Stern
2026-11-11 16:42                     ` Brian Norris
2026-11-11 20:20                       ` Alan Stern
2026-11-12  0:23                         ` Brian Norris [this message]
2026-11-03 22:14 ` [PATCH v2 4/5] PM: runtime: Add tests for last_busy propagation Brian Norris
2026-11-03 23:21   ` Jonathan Cameron
2026-11-03 22:14 ` [PATCH v2 5/5] iio: adc: stm32-adc: Drop runtime_idle() Brian Norris

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=avUHG8mLuTOUMr8O@google.com \
    --to=briannorris@chromium.org \
    --cc=alexandre.torgue@foss.st.com \
    --cc=andy@kernel.org \
    --cc=dlechner@baylibre.com \
    --cc=fabrice.gasnier@st.com \
    --cc=jic23@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=mcoquelin.stm32@gmail.com \
    --cc=nuno.sa@analog.com \
    --cc=rafael@kernel.org \
    --cc=stern@rowland.harvard.edu \
    --cc=ulfh@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox