Re: [PATCH] drvier: usb: dwc3: Fix runtime PM trying to activate child device xxx.dwc3 but parent is not active

Rafael J. Wysocki
2025-09-04T17:34:35Z

On Thu, Sep 4, 2025 at 4:19 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Thu, Sep 04, 2025 at 04:08:47PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Sep 3, 2025 at 11:54 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > On Wed, Sep 03, 2025 at 09:30:47PM +0200, Rafael J. Wysocki wrote:
> > > > I personally think that it would be reasonable to simply preserve
> > > > device states in error paths unless they have been changed already
> > > > before the error (or suspend abort due to a wakeup signal).
> > >
> > > The problem is complicated by the interaction between runtime-PM states
> > > and system-sleep states. In the case, we've been considering, B changes
> > > from runtime-suspended to runtime-suspended + system-suspended.
> > > Therefore the error path is allowed to modify B's state.
> >
> > Yes, it is, but retaining the B's state in an error path is also fine
> > so long as no changes have been made to it so far.
> >
> > If B was runtime-suspended to start with and none of the suspend
> > callbacks invoked for it so far has done anything to it, then it is de
> > facto still runtime-suspended and its state need not be changed in an
> > error path.
> >
> > > > By this rule, B would be left in runtime suspend if it were still in
> > > > runtime suspend when the error (or suspend abort in general) occurred
> > > > and then it doesn't matter what happens to A.
> > >
> > > More fully, B would be changed from runtime-suspended + system-suspended
> > > back to simply runtime-suspended. Unfortunately, none of the PM
> > > callbacks in the kernel are defined to make this change -- at least, not
> > > without some cooperation from the driver.
> >
> > Except when runtime-suspended + system-suspended is effectively the
> > same as runtime-suspended.
> >
> > Say this is not the case and say that the device is runtime-suspended
> > to start with. Then the "suspend" callback has two choices: either
> > (1) it can runtime-resume the device before doing anything to it,
> > which will also cause the device's parent and suppliers to
> > runtime-resume, or (2) it can update the device's state without
> > resuming it.
> >
> > If it chooses (1), then "resume" is straightforward. If it chooses
> > (2), "resume" may just reverse the changes made by "suspend" and
> > declare that the device is runtime-suspended. And if it really really
> > wants to resume the device then, why not call runtime_resume() on it?
>
> That's what I meant by needing "cooperation from the driver". The
> driver's ->resume callback needs to do this check to see which pathway
> to follow: (1) or (2).

Unless "suspend" always does the same thing, so it always does (1) or
it always does (2).

In that case, "resume" will know what to do without checking.

I'd like to mention that if "suspend" chooses (2), it may need to
resume the suspended parent or suppliers to be able to access the
device even though the device itself won't be resumed. I'm not sure
if (2) is really attractive then, though.

Also, in the example we've been considering so far, the assumption is
that B can just stay in runtime suspend, so why would it need to be
resumed by "resume"? And if there is a specific reason for resuming
it, "resume" can just call runtime_resume() on it AFAICS.