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

[Peter Chen <peter.chen@kernel.org>]

On 24-08-27 13:44:02, Mario Limonciello wrote:
> On 8/27/2024 01:32, Peter Chen wrote:
> > On 24-07-12 13:54:18, superm1@kernel.org wrote:
> > > From: Mario Limonciello <mario.limonciello@amd.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@amd.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);
> > >   }
> > > @@ -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);
> > 
> > Hi Mario & Mathias,
> > 
> > According to xHCI spec v1.2: A.1.2 Power State Definitions:
> > 
> > 	Software shall place each downstream USB port with power
> > 	enabled into the Suspend or Disabled state before it
> > 	attempts to move the xHC out of the D0 power state.
> > 
> > But I have not found any USB core code does it, do you have any ideas
> > about it?
> > 
> > We have added the similar codes at non-PCI USB platform, but met above
> > concerns. In fact, we met kernel dump that the thread usb-storage try
> > to access the port status when the platform xHCI code has already put
> > the controller to D3.
> > 
> > Best regards,
> > Peter
> > 
> > 
> 
> This is pretty tangential to my patch.  But FWIW in case you missed we're
> going to discard this patch in favor of another approach in PCI core.
> 
> Regarding your point though If I'm not mistaken this should be handled by
> the Linux parent/child device model.  Each of the ports should be children
> of the hub they're connected to and the hub a child of the controller.  So
> when doing any actions that start runtime PM on the host controller the
> children need to first be in runtime PM.
> 

It seems there is no runtime PM suspend for xhci and USB core at
.shutdown currently. Alan & Mathias, please correct me if I was wrong.

Thanks,
Peter