linux-kernel - Re: [PATCH v5 00/12] Fix USB suspend on TI J7200 (cdns3-ti, cdns3, xhci)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <D42NSY6SFTBG.2EEB1S4CB54QN@bootlin.com>
Date: Tue, 10 Sep 2024 16:04:17 +0200
From: Théo Lebrun <theo.lebrun@...tlin.com>
To: "Peter Chen" <peter.chen@...nel.org>
Cc: "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>, "Rob Herring"
 <robh@...nel.org>, "Krzysztof Kozlowski" <krzk+dt@...nel.org>, "Conor
 Dooley" <conor+dt@...nel.org>, "Roger Quadros" <rogerq@...nel.org>, "Pawel
 Laszczak" <pawell@...ence.com>, "Mathias Nyman" <mathias.nyman@...el.com>,
 "Nishanth Menon" <nm@...com>, "Vignesh Raghavendra" <vigneshr@...com>,
 "Tero Kristo" <kristo@...nel.org>, <linux-usb@...r.kernel.org>,
 <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <linux-arm-kernel@...ts.infradead.org>, "Kevin Hilman"
 <khilman@...nel.org>, Grégory Clement
 <gregory.clement@...tlin.com>, "Thomas Petazzoni"
 <thomas.petazzoni@...tlin.com>, "Conor Dooley" <conor.dooley@...rochip.com>
Subject: Re: [PATCH v5 00/12] Fix USB suspend on TI J7200 (cdns3-ti, cdns3,
 xhci)

Hello Peter,

On Fri Aug 9, 2024 at 3:19 AM CEST, Peter Chen wrote:
> On 24-07-26 20:17:48, Théo Lebrun wrote:
> > Currently, system-wide suspend is broken on J7200 because of a
> > controller reset. The TI wrapper does not get re-initialised at resume
> > and the first register access from cdns core fails.
> > 
> > We address that in a few ways:
> >  - In cdns3-ti, if a reset has occured at resume, we reconfigure the HW.
> >  - We pass the XHCI_RESET_ON_RESUME quirk, meaning the XHCI core expects
> >    a resume.
> >  - We add a xhci->lost_power flag.
> > 
> > The previous revision had one big issue: we had to know if
> > reset-on-resume was true, at probe-time. This is where the main
> > difference with previous revisions is. We now pass the information from
> > wrapper devices back up into XHCI. The xhci->lost_power flag gets its
> > default value from the XHCI_RESET_ON_RESUME quirk. It however allows
> > wrappers to signal *at resume* if they still expect a reset.
> > 
> > That means wrappers that are unsure if they will reset should:
> >  - (1) set the quirk at probe and,
> >  - (2) potentially set xhci->lost_power to false at resume.
>
> Judge if controller is power lost has implemented at cdns_power_is_lost
> Please check if you could use that.

That function is being used! Its return value is passed as second
argument to the resume() callback in struct cdns_role_driver. We set
xhci->lost_power using that exact value.

My cover letter explanation was slightly off, as it is not wrappers that
set xhci->lost_power, but instead role drivers. Wrappers don't have any
reason to touch the xhci struct directly, they are one layer above.

Related: [PATCH v5 08/15] commit message looks like this:

------------------------------------------------------------------------

The cdns_role_driver->resume() callback takes a second boolean argument
named `hibernated` in its implementations. This is mistaken; the only
potential caller is:

int cdns_resume(struct cdns *cdns)
{
	/* ... */

	if (cdns->roles[cdns->role]->resume)
		cdns->roles[cdns->role]->resume(cdns, cdns_power_is_lost(cdns));

	return 0;
}

The argument can be true in cases outside of return from hibernation.
Reflect the true meaning by renaming both arguments to `lost_power`.

------------------------------------------------------------------------

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com