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

[Joe Wang]

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.

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.

> 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?

Thanks,
Joe