linux-kernel - Re: [PATCH 2/2] xhci: pci: Put XHCI controllers into D3hot at shutdown

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <060bf2de-d231-4ed6-b7cc-cda40bfd397a@linux.intel.com>
Date: Thu, 22 Aug 2024 18:28:33 +0300
From: Mathias Nyman <mathias.nyman@...ux.intel.com>
To: superm1@...nel.org, Mathias Nyman <mathias.nyman@...el.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: "open list:USB XHCI DRIVER" <linux-usb@...r.kernel.org>,
 open list <linux-kernel@...r.kernel.org>,
 Kai-Heng Feng <kai.heng.feng@...onical.com>,
 mika.westerberg@...ux.intel.com,
 Mario Limonciello <mario.limonciello@....com>
Subject: Re: [PATCH 2/2] xhci: pci: Put XHCI controllers into D3hot at
 shutdown

On 12.7.2024 21.54, superm1@...nel.org wrote:
> From: Mario Limonciello <mario.limonciello@....com>
> 
> A workaround was put in place for Haswell systems with spurious events
> to put XHCI controllers into D3hot at shutdown.  This solution actually
> makes sense for all XHCI controllers though because XHCI controllers
> left in D0 by the OS may remain in D0 when the SoC goes into S5.
> 
> Explicitly put all XHCI controllers into D3hot at shutdown and when
> module is unloaded.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
> ---
>   drivers/usb/host/xhci-pci.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 4408d4caf66d2..dde5e4a210719 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -667,9 +667,7 @@ static void xhci_pci_remove(struct pci_dev *dev)
>   		xhci->shared_hcd = NULL;
>   	}
>   
> -	/* Workaround for spurious wakeups at shutdown with HSW */
> -	if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
> -		pci_set_power_state(dev, PCI_D3hot);
> +	pci_set_power_state(dev, PCI_D3hot);
>   
>   	usb_hcd_pci_remove(dev);

Just noticed these have been the wrong way around for a while (impacting HSW).

We should first call usb_hcd_pci_remove() and then pci_set_power_state(D3),
otherwise we force a fully running xHC into D3.

Note, with this change we end up first calling
pci_disable_device(), then pci_set_power_state(D3)

>   }
> @@ -882,9 +880,7 @@ static void xhci_pci_shutdown(struct usb_hcd *hcd)
>   	xhci_shutdown(hcd);
>   	xhci_cleanup_msix(xhci);
>   
> -	/* Yet another workaround for spurious wakeups at shutdown with HSW */
> -	if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
> -		pci_set_power_state(pdev, PCI_D3hot);
> +	pci_set_power_state(pdev, PCI_D3hot);

Looks good

Note that we now end up first calling pci_set_power_state(D3) and then
pci_disable_device(). The other way around than the remove case above.
I don't know if it matters.

Thanks
Mathias