Re: PM runtime_error handling missing in many drivers?

[Brian Norris <briannorris@google.com>]

Hi Ajay,

On Mon, Feb 10, 2025 at 09:02:41AM +0530, Ajay Agarwal wrote:
> On Wed, Jul 27, 2022 at 06:31:48PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Jul 27, 2022 at 10:08 AM Oliver Neukum <oneukum@suse.com> wrote:
> > > On 26.07.22 17:41, Rafael J. Wysocki wrote:
> > > > Well, in general suspending or resuming a device is a collaborative
> > > > effort and if one of the pieces falls over, making it work again
> > > > involves fixing up the failing piece and notifying the others that it
> > > > is ready again.  However, that part isn't covered and I'm not sure if
> > > > it can be covered in a sufficiently generic way.
> > >
> > > True. But that still cannot solve the question what is to be done
> > > if error handling fails. Hence my proposal:
> > > - record all failures
> > > - heed the record only when suspending
> > 
> > I guess that would boil down to moving the power.runtime_error update
> > from rpm_callback() to rpm_suspend()?
> Resuming this discussion. One of the ways the device drivers are
> clearing the runtime_error flag is by calling pm_runtime_set_suspended
> [1].
> 
> To me, it feels weird that a device driver calls pm_runtime_set_suspended
> if the runtime_resume() has failed. It should be implied that the device
> is in suspended state if the resume failed.
> 
> So how really should the runtime_error flag be cleared? Should there be
> a new API exposed to device drivers for this? Or should we plan for it
> in the framework itself?

While the API naming is unclear, that's exactly what
pm_runtime_set_suspended() is about. Personally, I find it nice when a
driver adds the comment "clear runtime_error flag", because otherwise
it's not really obvious why a driver has to take care of "suspending"
after a failed resume. But that's not the biggest question here, IMO.

The real reson I pointed you at this thread was because I think it's
useful to pursue the proposal above: to avoid setting a persistent
"runtime_error" for resume failures. This seems to just create a pitfall
for clients, as asked by Vincent and Oliver upthread.

And along this line, there are relatively few drivers that actually
bother to reset this error flag ever (e.g., commit f2bc2afe34c1
("accel/ivpu: Clear runtime_error after pm_runtime_resume_and_get()
fails")).

So to me, we should simply answer Rafael's question:

(repeated:)
> > I guess that would boil down to moving the power.runtime_error update
> > from rpm_callback() to rpm_suspend()?

Yes, I think so. (Although I'm not sure if this leaves undesirable spam
where persistent .runtime_resume() failures occur.)

...and then write/test/submit such a patch, provided it achieves the
desired results.

Unless of course one of the thread participants here has some other
update in the intervening 2.5 years, or if Rafael was simply asking the
above rhetorically, and wasn't actually interested in fielding such a
change.

Brian