Re: [PATCH] usb: ehci: Quiesce ASPEED EHCI before halting it on shutdown

[Alan Stern]

On Tue, Oct 20, 2026 at 10:42:55AM +0000, Joe Wang wrote:
> Hi Alan,
> 
> Thanks for the review.
> > 
> > Under what circumstances does ehci_shutdown() run without ehci_stop()
> > being called first?  Is controller death the only possibility? 
> 
> No, it's the normal case: ehci_shutdown() runs from the .shutdown
> callback during reboot, poweroff and kexec, where the HCD is not
> removed, so ehci_stop() is never called and the controller is usually
> still running with devices attached. The issue was seen on a host
> using LinuxBoot (NERF) firmware, which boots the final OS with kexec.

Okay, so this will slow down a normal reboot or poweroff.  Hopefully not 
by very much, if everything is working properly.

> Controller death is mentioned in the changelog only because relaxing
> the check in ehci_quiesce() also affects that path.
> 
> > If the controller is dead, will quiescing work?
> 
> It won't help, but it's harmless. The death path has already cleared
> CMD_RUN, CMD_ASE and CMD_PSE, so ehci_quiesce() writes those bits back
> as zero and the handshakes either succeed at once or time out after
> 16 uframes. ehci_shutdown() already touches the registers through
> ehci_halt() in that case anyway.
> 
> > Is there any real reason not to do this on all types of EHCI controller, not
> > just ASPEED?
> 
> No EHCI technical reason that I know of. I limited it to ASPEED because
> that's what the report and my testing cover. Since ehci_stop()
> already quiesces unconditionally, doing it for all controllers should
> be safe.

I think it makes sense for the change to apply to all EHCI types.  It's 
not really a good idea to leave an independent DMA master running while 
the system is shutting down.

> > Don't you want to check whether the controller has already been quiesced?
> 
> The quiesce has to come after rh_state is set to EHCI_RH_STOPPING;
> otherwise a URB submission or hrtimer event could re-enable the
> schedules through ehci_poll_ASS()/ehci_poll_PSS() before the halt.
> That's why v1 relaxes the check in ehci_quiesce(). The side effect is
> that a controller that was already suspended or halted gets quiesced
> again, which is harmless but unnecessary.
> 
> I see two ways to avoid the extra ehci_quiesce() :
> 
> (a) Keep the relaxed check, and in ehci_shutdown() record under
>    ehci->lock whether rh_state was EHCI_RH_RUNNING before setting
>    EHCI_RH_STOPPING, quiescing only in that case. This is the
>    smaller change.
> 
> (b) Same as (a), but leave ehci_quiesce() unchanged and have
>    ehci_shutdown() call a separate helper for the schedule
>    quiescing part. Other callers of ehci_quiesce() are then not
>    affected at all.
> 
> Either can be combined with dropping the `is_aspeed` check. Which would
> you prefer?

(a) is more reasonable.

Alan Stern