Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Alexandru Gagniuc <alexandru.gagniuc@hp.com>
To:     stern@rowland.harvard.edu,
        Alexandru Gagniuc <alexandru.gagniuc@hp.com>
Cc:     bjorn@mork.no, davem@davemloft.net, edumazet@google.com,
        eniac-xw.zhang@hp.com, hayeswang@realtek.com, jflf_kernel@gmx.com,
        kuba@kernel.org, linux-kernel@vger.kernel.org,
        linux-usb@vger.kernel.org, netdev@vger.kernel.org,
        pabeni@redhat.com, stable@vger.kernel.org, svenva@chromium.org
Subject: Re: [PATCH v2] r8152: Suspend USB device before shutdown when WoL is enabled
Date:   Thu, 10 Aug 2023 16:22:16 +0000
Message-Id: <20230810162216.13455-1-alexandru.gagniuc@hp.com>
In-Reply-To: <3c4fd3d8-2b0b-492e-aacc-afafcea98417@rowland.harvard.edu>
References: <3c4fd3d8-2b0b-492e-aacc-afafcea98417@rowland.harvard.edu>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=WINDOWS-1252; x-default=true
Precedence: bulk

From: Alan Stern <stern@rowland.harvard.edu>

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?
> >=20
> > The objective is to get to xhci_set_link_state() with the USB_SS_PORT_L=
S_U3
> > argument. This is achieved through usb_port_suspend() in drivers/usb/ho=
st/hub.c,
> > and the function is implemented in drivers/usb/host/xhci-hub.c.
> >=20
> > 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 confiden=
t it will
> > show consistent behavior across platforms.
>=20
> That does not answer my question.  I agree that making this change will=
=20
> put the link into the U3 state.  But I don't have any reason to think=20
> 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 stat=
e.
The only conjecture that I can make is that no other software besides the k=
ernel
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 hos=
t=20
> > > controller won't disable the link entirely, thereby preventing WoL fr=
om=20
> > > working?
> >=20
> > We are talking to the USB hub in order to set the link state. I don't s=
ee how
> > specifics of the host controller would influence behavior.
>=20
> Specifics of the host controller probably won't influence behavior. =20
> However, specifics of the _software_ can make a big difference.
>=20
> >  I do expect a
> > controller which advertises S4/S5 in /proc/acpi/wakeup to not do anythi=
ng that
> > would sabotage this capability. Disabling the link entirely would proba=
lby
> > violate that promise.
>=20
> Not if the kernel _tells_ the controller to disable the link.
>=20
> > 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.
>=20
> Indeed.  And I am asking how you can be sure the host controller driver=
=20
> (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 verif=
y
that WoL from S5 now works in this scenario. What are you thinking of in te=
rms
of being sure no current or future bug exists?

Alex