[PATCH 0/2] Put XHCI controllers into D3 at S4/S5

[PATCH 0/2] Put XHCI controllers into D3 at S4/S5

[superm1]

From: Mario Limonciello <mario.limonciello@amd.com>

When the system is put into S4 or S5 XHCI controllers remain in D0.  This
causes higher power consumption and may compromise energy certifications.
Consequently some systems consume more power in S5 than s0i3.

This affects all PCIe devices, but looking at breakdowns XHCI is the
biggest offender for power consumption.

This series checks if any wakeups are needed and puts controllers into D3
if no wakeup necessary.

This series is a spiritual successor to [1] which aimed to do this more
generally in PCI.  It also accomplishes similar goals as [2], but aims for
both S4 and S5.

[1] https://lore.kernel.org/linux-pci/20231213182656.6165-1-mario.limonciello@amd.com/#t
[2] https://lore.kernel.org/linux-pci/9d2f1619-1c61-4e8c-b28d-d4eddefa45c3@amd.com/T/

Mario Limonciello (2):
  xhci: pci: If no ports have wakeup enabled then disable PCI device at
    S4
  xhci: pci: Put XHCI controllers into D3hot at shutdown

 drivers/usb/host/xhci-pci.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

-- 
2.43.0

[PATCH 1/2] xhci: pci: If no ports have wakeup enabled then disable PCI device at S4

[superm1]

From: Mario Limonciello <mario.limonciello@amd.com>

If a port on an XHCI controller hasn't been marked for wakeup at S4, then
leaving it at D0 will needlessly consume power than necessary.

Explicitly check ports configured for wakeup and if none are found then
put the controller into D3hot before hibernate.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/usb/host/xhci-pci.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 05881153883ec..4408d4caf66d2 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -823,6 +823,7 @@ static int xhci_pci_poweroff_late(struct usb_hcd *hcd, bool do_wakeup)
 	struct usb_device	*udev;
 	u32			portsc;
 	int			i;
+	bool			wakeup = false;
 
 	/*
 	 * Systems with XHCI_RESET_TO_DEFAULT quirk have boot firmware that
@@ -860,6 +861,14 @@ static int xhci_pci_poweroff_late(struct usb_hcd *hcd, bool do_wakeup)
 			 port->rhub->hcd->self.busnum, port->hcd_portnum + 1);
 		portsc = xhci_port_state_to_neutral(portsc);
 		writel(portsc | PORT_PE, port->addr);
+		wakeup = true;
+	}
+
+	if (!wakeup) {
+		struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
+
+		xhci_shutdown(hcd);
+		pci_set_power_state(pdev, PCI_D3hot);
 	}
 
 	return 0;
-- 
2.43.0

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

[superm1]

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);
 }
 
 /*-------------------------------------------------------------------------*/
-- 
2.43.0

Re: [PATCH 0/2] Put XHCI controllers into D3 at S4/S5

[Mario Limonciello]

On 7/12/24 13:54, superm1@kernel.org wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> When the system is put into S4 or S5 XHCI controllers remain in D0.  This
> causes higher power consumption and may compromise energy certifications.
> Consequently some systems consume more power in S5 than s0i3.
> 
> This affects all PCIe devices, but looking at breakdowns XHCI is the
> biggest offender for power consumption.
> 
> This series checks if any wakeups are needed and puts controllers into D3
> if no wakeup necessary.
> 
> This series is a spiritual successor to [1] which aimed to do this more
> generally in PCI.  It also accomplishes similar goals as [2], but aims for
> both S4 and S5.
> 
> [1] https://lore.kernel.org/linux-pci/20231213182656.6165-1-mario.limonciello@amd.com/#t
> [2] https://lore.kernel.org/linux-pci/9d2f1619-1c61-4e8c-b28d-d4eddefa45c3@amd.com/T/
> 
> Mario Limonciello (2):
>    xhci: pci: If no ports have wakeup enabled then disable PCI device at
>      S4
>    xhci: pci: Put XHCI controllers into D3hot at shutdown
> 
>   drivers/usb/host/xhci-pci.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
> 

Hello,

Any feedback for this series?

Thanks,

Re: [PATCH 1/2] xhci: pci: If no ports have wakeup enabled then disable PCI device at S4

[Mathias Nyman]

On 12.7.2024 21.54, superm1@kernel.org wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> If a port on an XHCI controller hasn't been marked for wakeup at S4, then
> leaving it at D0 will needlessly consume power than necessary.
> 
> Explicitly check ports configured for wakeup and if none are found then
> put the controller into D3hot before hibernate.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/usb/host/xhci-pci.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 05881153883ec..4408d4caf66d2 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -823,6 +823,7 @@ static int xhci_pci_poweroff_late(struct usb_hcd *hcd, bool do_wakeup)
>   	struct usb_device	*udev;
>   	u32			portsc;
>   	int			i;
> +	bool			wakeup = false;
>   
>   	/*
>   	 * Systems with XHCI_RESET_TO_DEFAULT quirk have boot firmware that
> @@ -860,6 +861,14 @@ static int xhci_pci_poweroff_late(struct usb_hcd *hcd, bool do_wakeup)
>   			 port->rhub->hcd->self.busnum, port->hcd_portnum + 1);
>   		portsc = xhci_port_state_to_neutral(portsc);
>   		writel(portsc | PORT_PE, port->addr);
> +		wakeup = true;
> +	}
> +
> +	if (!wakeup) {
> +		struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
> +
> +		xhci_shutdown(hcd);
> +		pci_set_power_state(pdev, PCI_D3hot);

Not sure we should force D3 here.
I think usb core will set the PCI D state in the next .poweroff_noirq stage,

for s4:
.poweroff	= hcd_pci_suspend,
.poweroff_late	= hcd_pci_poweroff_late,
.poweroff_noirq	= hcd_pci_suspend_noirq,

hcd_pci_suspend_noirq()
   pci_prepare_to_sleep();
     target_state = pci_target_state(dev, wakeup)
     pci_set_power_state(dev, target_state)

Maybe the target_state isn't D3 for some reason? (missing ACPI entries?)

Thanks
Mathias

Re: [PATCH 1/2] xhci: pci: If no ports have wakeup enabled then disable PCI device at S4

[Mario Limonciello]

On 8/21/2024 04:25, Mathias Nyman wrote:
> On 12.7.2024 21.54, superm1@kernel.org wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> If a port on an XHCI controller hasn't been marked for wakeup at S4, then
>> leaving it at D0 will needlessly consume power than necessary.
>>
>> Explicitly check ports configured for wakeup and if none are found then
>> put the controller into D3hot before hibernate.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/usb/host/xhci-pci.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
>> index 05881153883ec..4408d4caf66d2 100644
>> --- a/drivers/usb/host/xhci-pci.c
>> +++ b/drivers/usb/host/xhci-pci.c
>> @@ -823,6 +823,7 @@ static int xhci_pci_poweroff_late(struct usb_hcd 
>> *hcd, bool do_wakeup)
>>       struct usb_device    *udev;
>>       u32            portsc;
>>       int            i;
>> +    bool            wakeup = false;
>>       /*
>>        * Systems with XHCI_RESET_TO_DEFAULT quirk have boot firmware that
>> @@ -860,6 +861,14 @@ static int xhci_pci_poweroff_late(struct usb_hcd 
>> *hcd, bool do_wakeup)
>>                port->rhub->hcd->self.busnum, port->hcd_portnum + 1);
>>           portsc = xhci_port_state_to_neutral(portsc);
>>           writel(portsc | PORT_PE, port->addr);
>> +        wakeup = true;
>> +    }
>> +
>> +    if (!wakeup) {
>> +        struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
>> +
>> +        xhci_shutdown(hcd);
>> +        pci_set_power_state(pdev, PCI_D3hot);
> 
> Not sure we should force D3 here.
> I think usb core will set the PCI D state in the next .poweroff_noirq 
> stage,
> 
> for s4:
> .poweroff    = hcd_pci_suspend,
> .poweroff_late    = hcd_pci_poweroff_late,
> .poweroff_noirq    = hcd_pci_suspend_noirq,
> 
> hcd_pci_suspend_noirq()
>    pci_prepare_to_sleep();
>      target_state = pci_target_state(dev, wakeup)
>      pci_set_power_state(dev, target_state)
> 
> Maybe the target_state isn't D3 for some reason? (missing ACPI entries?)
> 

Thanks for looking.

Even a lack of ACPI entries *should* still lead to D3hot if wakeup is 
turned off with that flow.

I'll add some more debugging for the various callbacks and see what I 
can come up with to explain it.

Can you please also review the other patch in the series?

Thanks!

Re: [PATCH 0/2] Put XHCI controllers into D3 at S4/S5

[Kai-Heng Feng]

Hi Mario,

On Tue, Aug 20, 2024 at 10:06 AM Mario Limonciello <superm1@kernel.org> wrote:
>
> On 7/12/24 13:54, superm1@kernel.org wrote:
> > From: Mario Limonciello <mario.limonciello@amd.com>
> >
> > When the system is put into S4 or S5 XHCI controllers remain in D0.  This
> > causes higher power consumption and may compromise energy certifications.
> > Consequently some systems consume more power in S5 than s0i3.
> >
> > This affects all PCIe devices, but looking at breakdowns XHCI is the
> > biggest offender for power consumption.
> >
> > This series checks if any wakeups are needed and puts controllers into D3
> > if no wakeup necessary.
> >
> > This series is a spiritual successor to [1] which aimed to do this more
> > generally in PCI.  It also accomplishes similar goals as [2], but aims for
> > both S4 and S5.
> >
> > [1] https://lore.kernel.org/linux-pci/20231213182656.6165-1-mario.limonciello@amd.com/#t
> > [2] https://lore.kernel.org/linux-pci/9d2f1619-1c61-4e8c-b28d-d4eddefa45c3@amd.com/T/
> >
> > Mario Limonciello (2):
> >    xhci: pci: If no ports have wakeup enabled then disable PCI device at
> >      S4
> >    xhci: pci: Put XHCI controllers into D3hot at shutdown
> >
> >   drivers/usb/host/xhci-pci.c | 17 +++++++++++------
> >   1 file changed, 11 insertions(+), 6 deletions(-)
> >
>
> Hello,
>
> Any feedback for this series?

Does commit 0fab972eef49 ("drivers: core: Detach device from power
domain on shutdown") work for your case?
The commit was reverted because it regressed some DT based devices,
but probably still worth doing for ACPI based systems.

Kai-Heng

>
> Thanks,

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

[Mathias Nyman]

On 12.7.2024 21.54, 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);

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

Re: [PATCH 0/2] Put XHCI controllers into D3 at S4/S5

[Mario Limonciello]

On 8/22/2024 02:15, Kai-Heng Feng wrote:
> Hi Mario,
> 
> On Tue, Aug 20, 2024 at 10:06 AM Mario Limonciello <superm1@kernel.org> wrote:
>>
>> On 7/12/24 13:54, superm1@kernel.org wrote:
>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>
>>> When the system is put into S4 or S5 XHCI controllers remain in D0.  This
>>> causes higher power consumption and may compromise energy certifications.
>>> Consequently some systems consume more power in S5 than s0i3.
>>>
>>> This affects all PCIe devices, but looking at breakdowns XHCI is the
>>> biggest offender for power consumption.
>>>
>>> This series checks if any wakeups are needed and puts controllers into D3
>>> if no wakeup necessary.
>>>
>>> This series is a spiritual successor to [1] which aimed to do this more
>>> generally in PCI.  It also accomplishes similar goals as [2], but aims for
>>> both S4 and S5.
>>>
>>> [1] https://lore.kernel.org/linux-pci/20231213182656.6165-1-mario.limonciello@amd.com/#t
>>> [2] https://lore.kernel.org/linux-pci/9d2f1619-1c61-4e8c-b28d-d4eddefa45c3@amd.com/T/
>>>
>>> Mario Limonciello (2):
>>>     xhci: pci: If no ports have wakeup enabled then disable PCI device at
>>>       S4
>>>     xhci: pci: Put XHCI controllers into D3hot at shutdown
>>>
>>>    drivers/usb/host/xhci-pci.c | 17 +++++++++++------
>>>    1 file changed, 11 insertions(+), 6 deletions(-)
>>>
>>
>> Hello,
>>
>> Any feedback for this series?
> 
> Does commit 0fab972eef49 ("drivers: core: Detach device from power
> domain on shutdown") work for your case?
> The commit was reverted because it regressed some DT based devices,
> but probably still worth doing for ACPI based systems.
> 
> Kai-Heng
> 
>>
>> Thanks,

Thanks for pointing that out and Mathias thanks for the comments.  I've 
spent some time today looking at the different permutations of patches 
to see what happens at shutdown to devices.

These are all the patches I tested (in various combinations)

Baseline: 6.11-rc4
FS: 6.11-rc4 + 0fab972eef49
ML: 6.11-rc4 + this series
KH: 6.11-rc4 + 
https://lore.kernel.org/linux-pci/9d2f1619-1c61-4e8c-b28d-d4eddefa45c3@amd.com/T/#md69ca96133ae0191eefb2f7f5003ce9cc180ec76

The results are available here (in markdown):
https://gist.github.com/superm1/f8f81e52f5b1d55b64493fdaec38e31c

It seems to me:
* FS didn't change anything
* ML only affects the XHCI controllers (into D3 at S5).
* KH in any combination is the best

Considering this I will scrap this patch series and I will leave a 
comment your patch KH.  Hope we can get that in, it's great.

Thanks!

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

[Peter Chen]

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

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

[Mario Limonciello]

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.

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

[Peter Chen]

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

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

[Mathias Nyman]

>>> 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.
> 

I think you are right.  At shutdown we only halt the xHC.
We don't force ports to suspend or disable state.
We only put some selected xHC to D3

USB 2 ports might suspend themselves if there is no activity.

Doesn't seem like usb core hcd code, or hub driver does anything either.

Thanks
Mathias

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

[Alan Stern]

On Wed, Aug 28, 2024 at 05:02:10PM +0300, Mathias Nyman wrote:
> > > > 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.
> > 
> 
> I think you are right.  At shutdown we only halt the xHC.
> We don't force ports to suspend or disable state.
> We only put some selected xHC to D3
> 
> USB 2 ports might suspend themselves if there is no activity.
> 
> Doesn't seem like usb core hcd code, or hub driver does anything either.

That's right.  Host controller drivers are supposed to handle shutdown 
operations by themselves.  The USB core doesn't do anything.

Alan Stern