Re: [PATCH v2] r8152: Suspend USB device before shutdown when WoL is enabled

[Alexandru Gagniuc]

From: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>

On Wed, Aug 02, 2023 at 11:23:46AM -0400, Alan Stern wrote:
> On Wed, Aug 02, 2023 at 02:56:43PM +0000, Gagniuc, Alexandru wrote:
> > On Wed, Jul 19, 2023 at 02:36:25PM -0400, Alan Stern wrote:
> > > How do you know that the link will _remain_ in the correct state?
> > 
> > The objective is to get to xhci_set_link_state() with the USB_SS_PORT_LS_U3
> > argument. This is achieved through usb_port_suspend() in drivers/usb/host/hub.c,
> > and the function is implemented in drivers/usb/host/xhci-hub.c.
> > 
> > This is the only path in the kernel that I am aware of for setting the U3 link
> > state. Given that it is part of the USB subsystem, I am fairly confident it will
> > show consistent behavior across platforms.
> 
> That does not answer my question.  I agree that making this change will 
> put the link into the U3 state.  But I don't have any reason to think 
> that some other software won't later put the link into some other state.

I don't have a rigurous proof that the link will remain in the correct state.
The only conjecture that I can make is that no other software besides the kernel
will be running at this time. Thus, if the kernel manages to not break the link
state, things should work as intended.

> > > That is, how do you know that the shutdown processing for the USB host 
> > > controller won't disable the link entirely, thereby preventing WoL from 
> > > working?
> > 
> > We are talking to the USB hub in order to set the link state. I don't see how
> > specifics of the host controller would influence behavior.
> 
> Specifics of the host controller probably won't influence behavior.  
> However, specifics of the _software_ can make a big difference.
> 
> >  I do expect a
> > controller which advertises S4/S5 in /proc/acpi/wakeup to not do anything that
> > would sabotage this capability. Disabling the link entirely would probalby
> > violate that promise.
> 
> Not if the kernel _tells_ the controller to disable the link.
> 
> > Think of USB-C docks with a power button showing up as a HID class. The scenario
> > herein would disable the power button. I would take that to be a bug in the host
> > controller driver if the S4/S5 capability is advertised.
> 
> Indeed.  And I am asking how you can be sure the host controller driver 
> (or some other part of the software stack) doesn't have this bug.

The only way that I have to show that is empirical. I observe that WoL from S5
does not work on a device with an r8153 chip. I apply the change, and verify
that WoL from S5 now works in this scenario. What are you thinking of in terms
of being sure no current or future bug exists?

Alex