Intel GemniLake xHCI connected devices can never wake up the system from suspend

Intel GemniLake xHCI connected devices can never wake up the system from suspend

[Chris Chiu]

Hi,
    I  have a ASUS AIO V222GA and another Acer Desktop XC-830 both
have Intel CPU J5005 and they both hit the same problem. The XHCI
connected USB keyboard/mouse can never wakeup the system from suspend.
It reminds me that similiar thing happens on ApolloLake too which
needs the XHCI_PME_STUCK_QUIRK to get it work. It's also mentioned in
https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/pentium-celeron-n-series-spec-update.pdf
page #14 for N-seris intel CPU. And I also find the same problem
description in https://www.intel.com/content/dam/www/public/us/en/documents/product-briefs/silver-celeron-spec-update.pdf
page #16 for J-series Intel CPU. Seems that they have different
workaround so I can not simply apply XHCI_PME_STUCK_QUIRK to make it
work.

    Anyone can help here?

Chris

Re: Intel GemniLake xHCI connected devices can never wake up the system from suspend

[Mathias Nyman]

On 15.03.2018 06:40, Chris Chiu wrote:
> Hi,
>      I  have a ASUS AIO V222GA and another Acer Desktop XC-830 both
> have Intel CPU J5005 and they both hit the same problem. The XHCI
> connected USB keyboard/mouse can never wakeup the system from suspend.
> It reminds me that similiar thing happens on ApolloLake too which
> needs the XHCI_PME_STUCK_QUIRK to get it work. It's also mentioned in
> https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/pentium-celeron-n-series-spec-update.pdf
> page #14 for N-seris intel CPU. And I also find the same problem
> description in https://www.intel.com/content/dam/www/public/us/en/documents/product-briefs/silver-celeron-spec-update.pdf
> page #16 for J-series Intel CPU. Seems that they have different
> workaround so I can not simply apply XHCI_PME_STUCK_QUIRK to make it
> work.
> 
>      Anyone can help here?
> 

N-Series
CHP8: USB xHCI Controller May Not Re-Enter D3 State After a USB Wake Event
- needs XHCI_PME_STUCK_QUIRK in driver (sets bit 28 at offset 80a4)

Intel® Pentium® Silver N5000
Intel® Pentium® Silver J5005
Intel® Celeron® N4000 and N4100
Intel® Celeron® J4105 and J4005

USB xHCI Controller May Not Re-enter a D3 State After a USB Wake Even
Need to clear PME_EN bit of of the standard PCI PM_CSR register.
I think Linux does this anyway (clears enabling PME when reaching D0)
So if I remember correct there was no specific workaround needed for this.

what is the PCI ID of your xhci controller? (lspci -nn)

One other possible cause is that xHCI never reaches PCI device D3 suspend state during system suspend.
xHC can't generate PME# wake event from normal running PCI device D0 state.

PCI code in Linux will check with ACPI about the lowest possible D state when suspending,
If there is something missing from the xHCI entry in ACPI DSDT table it might select D0.
as the suspend state, causing wake failure.

Is there a BIOS update available for your ASUS and Acer?

-Mathias

  
  

Re: Intel GemniLake xHCI connected devices can never wake up the system from suspend

[Chris Chiu]

On Thu, Mar 15, 2018 at 7:46 PM, Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
> On 15.03.2018 06:40, Chris Chiu wrote:
>>
>> Hi,
>>      I  have a ASUS AIO V222GA and another Acer Desktop XC-830 both
>> have Intel CPU J5005 and they both hit the same problem. The XHCI
>> connected USB keyboard/mouse can never wakeup the system from suspend.
>> It reminds me that similiar thing happens on ApolloLake too which
>> needs the XHCI_PME_STUCK_QUIRK to get it work. It's also mentioned in
>>
>> https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/pentium-celeron-n-series-spec-update.pdf
>> page #14 for N-seris intel CPU. And I also find the same problem
>> description in
>> https://www.intel.com/content/dam/www/public/us/en/documents/product-briefs/silver-celeron-spec-update.pdf
>> page #16 for J-series Intel CPU. Seems that they have different
>> workaround so I can not simply apply XHCI_PME_STUCK_QUIRK to make it
>> work.
>>
>>      Anyone can help here?
>>
>
> N-Series
> CHP8: USB xHCI Controller May Not Re-Enter D3 State After a USB Wake Event
> - needs XHCI_PME_STUCK_QUIRK in driver (sets bit 28 at offset 80a4)
>
> Intel® Pentium® Silver N5000
> Intel® Pentium® Silver J5005
> Intel® Celeron® N4000 and N4100
> Intel® Celeron® J4105 and J4005
>
> USB xHCI Controller May Not Re-enter a D3 State After a USB Wake Even
> Need to clear PME_EN bit of of the standard PCI PM_CSR register.
> I think Linux does this anyway (clears enabling PME when reaching D0)
> So if I remember correct there was no specific workaround needed for this.
>
> what is the PCI ID of your xhci controller? (lspci -nn)

They're both 8086:31a8. So you mean from the workaround description, it should
work w/o any extra code? "Software should clear bit 8 (PME_EN) of PM_CSR" has
been handled somewhere else?

>
> One other possible cause is that xHCI never reaches PCI device D3 suspend
> state during system suspend.
> xHC can't generate PME# wake event from normal running PCI device D0 state.
>
> PCI code in Linux will check with ACPI about the lowest possible D state
> when suspending,
> If there is something missing from the xHCI entry in ACPI DSDT table it
> might select D0.
> as the suspend state, causing wake failure.
>

Here's the DSDT ASL of the ASUS machine.
https://gist.github.com/mschiu77/8e9c8a0e5a98b70a6dfff45620340bf1

Don't know if the "lspci -vvv" output for the USB xHCI controller
provide any useful
information. There's "PME(D0-,D1-,D2-,D3hot+,D3cold+)" in the
following lspci log
https://gist.github.com/mschiu77/8cf523f0e564c1fca44398ebeb21b7bc


> Is there a BIOS update available for your ASUS and Acer?
>
> -Mathias

ASUS said the BIOS has no problem on USB wakeup under Windows so I don't think
there's any update. Anything else could be cause for this?

Re: Intel GemniLake xHCI connected devices can never wake up the system from suspend

[Mathias Nyman]

On 15.03.2018 15:28, Chris Chiu wrote:
> On Thu, Mar 15, 2018 at 7:46 PM, Mathias Nyman
> <mathias.nyman@linux.intel.com> wrote:
>> On 15.03.2018 06:40, Chris Chiu wrote:
>>>
>>> Hi,
>>>       I  have a ASUS AIO V222GA and another Acer Desktop XC-830 both
>>> have Intel CPU J5005 and they both hit the same problem. The XHCI
>>> connected USB keyboard/mouse can never wakeup the system from suspend.
>>> It reminds me that similiar thing happens on ApolloLake too which
>>> needs the XHCI_PME_STUCK_QUIRK to get it work. It's also mentioned in
>>>
>>> https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/pentium-celeron-n-series-spec-update.pdf
>>> page #14 for N-seris intel CPU. And I also find the same problem
>>> description in
>>> https://www.intel.com/content/dam/www/public/us/en/documents/product-briefs/silver-celeron-spec-update.pdf
>>> page #16 for J-series Intel CPU. Seems that they have different
>>> workaround so I can not simply apply XHCI_PME_STUCK_QUIRK to make it
>>> work.
>>>
>>>       Anyone can help here?
>>>
>>
>> N-Series
>> CHP8: USB xHCI Controller May Not Re-Enter D3 State After a USB Wake Event
>> - needs XHCI_PME_STUCK_QUIRK in driver (sets bit 28 at offset 80a4)
>>
>> Intel® Pentium® Silver N5000
>> Intel® Pentium® Silver J5005
>> Intel® Celeron® N4000 and N4100
>> Intel® Celeron® J4105 and J4005
>>
>> USB xHCI Controller May Not Re-enter a D3 State After a USB Wake Even
>> Need to clear PME_EN bit of of the standard PCI PM_CSR register.
>> I think Linux does this anyway (clears enabling PME when reaching D0)
>> So if I remember correct there was no specific workaround needed for this.
>>
>> what is the PCI ID of your xhci controller? (lspci -nn)
> 
> They're both 8086:31a8. So you mean from the workaround description, it should
> work w/o any extra code? "Software should clear bit 8 (PME_EN) of PM_CSR" has
> been handled somewhere else?
> 
>>
>> One other possible cause is that xHCI never reaches PCI device D3 suspend
>> state during system suspend.
>> xHC can't generate PME# wake event from normal running PCI device D0 state.
>>
>> PCI code in Linux will check with ACPI about the lowest possible D state
>> when suspending,
>> If there is something missing from the xHCI entry in ACPI DSDT table it
>> might select D0.
>> as the suspend state, causing wake failure.
>>
> 
> Here's the DSDT ASL of the ASUS machine.
> https://gist.github.com/mschiu77/8e9c8a0e5a98b70a6dfff45620340bf1

Scope (_SB.PCI0.XHC) has _PS0 method, so Linux will look for a _S3W to get the
lowest possible D state in S3, but_S3W is missing, so Linux pci-acpi code will
probably default to D0

> 
> 
> ASUS said the BIOS has no problem on USB wakeup under Windows so I don't think
> there's any update. Anything else could be cause for this?

Linux and Windows probably check different DSDT values

You can try to force your xHC to D3 during suspend with a hack:

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 7815768..da46c52 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -488,6 +488,10 @@ static pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
         else
                 d_max = ACPI_STATE_D3_COLD;
         acpi_state = acpi_pm_device_sleep_state(&pdev->dev, NULL, d_max);
+
+       if (pdev->device == 0x31a8)
+               acpi_state = ACPI_STATE_D3_HOT;
+
         if (acpi_state < 0)
                 return PCI_POWER_ERROR;
  
and see if usb wake starts to work

-Mathias

Intel GemniLake xHCI connected devices can never wake up the system from suspend

[Mathias Nyman]

On 15.03.2018 15:28, Chris Chiu wrote:
> On Thu, Mar 15, 2018 at 7:46 PM, Mathias Nyman
> <mathias.nyman@linux.intel.com> wrote:
>> On 15.03.2018 06:40, Chris Chiu wrote:
>>>
>>> Hi,
>>>       I  have a ASUS AIO V222GA and another Acer Desktop XC-830 both
>>> have Intel CPU J5005 and they both hit the same problem. The XHCI
>>> connected USB keyboard/mouse can never wakeup the system from suspend.
>>> It reminds me that similiar thing happens on ApolloLake too which
>>> needs the XHCI_PME_STUCK_QUIRK to get it work. It's also mentioned in
>>>
>>> https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/pentium-celeron-n-series-spec-update.pdf
>>> page #14 for N-seris intel CPU. And I also find the same problem
>>> description in
>>> https://www.intel.com/content/dam/www/public/us/en/documents/product-briefs/silver-celeron-spec-update.pdf
>>> page #16 for J-series Intel CPU. Seems that they have different
>>> workaround so I can not simply apply XHCI_PME_STUCK_QUIRK to make it
>>> work.
>>>
>>>       Anyone can help here?
>>>
>>
>> N-Series
>> CHP8: USB xHCI Controller May Not Re-Enter D3 State After a USB Wake Event
>> - needs XHCI_PME_STUCK_QUIRK in driver (sets bit 28 at offset 80a4)
>>
>> Intel® Pentium® Silver N5000
>> Intel® Pentium® Silver J5005
>> Intel® Celeron® N4000 and N4100
>> Intel® Celeron® J4105 and J4005
>>
>> USB xHCI Controller May Not Re-enter a D3 State After a USB Wake Even
>> Need to clear PME_EN bit of of the standard PCI PM_CSR register.
>> I think Linux does this anyway (clears enabling PME when reaching D0)
>> So if I remember correct there was no specific workaround needed for this.
>>
>> what is the PCI ID of your xhci controller? (lspci -nn)
> 
> They're both 8086:31a8. So you mean from the workaround description, it should
> work w/o any extra code? "Software should clear bit 8 (PME_EN) of PM_CSR" has
> been handled somewhere else?
> 
>>
>> One other possible cause is that xHCI never reaches PCI device D3 suspend
>> state during system suspend.
>> xHC can't generate PME# wake event from normal running PCI device D0 state.
>>
>> PCI code in Linux will check with ACPI about the lowest possible D state
>> when suspending,
>> If there is something missing from the xHCI entry in ACPI DSDT table it
>> might select D0.
>> as the suspend state, causing wake failure.
>>
> 
> Here's the DSDT ASL of the ASUS machine.
> https://gist.github.com/mschiu77/8e9c8a0e5a98b70a6dfff45620340bf1

Scope (_SB.PCI0.XHC) has _PS0 method, so Linux will look for a _S3W to get the
lowest possible D state in S3, but_S3W is missing, so Linux pci-acpi code will
probably default to D0

> 
> 
> ASUS said the BIOS has no problem on USB wakeup under Windows so I don't think
> there's any update. Anything else could be cause for this?

Linux and Windows probably check different DSDT values

You can try to force your xHC to D3 during suspend with a hack:

and see if usb wake starts to work

-Mathias
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 7815768..da46c52 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -488,6 +488,10 @@ static pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
         else
                 d_max = ACPI_STATE_D3_COLD;
         acpi_state = acpi_pm_device_sleep_state(&pdev->dev, NULL, d_max);
+
+       if (pdev->device == 0x31a8)
+               acpi_state = ACPI_STATE_D3_HOT;
+
         if (acpi_state < 0)
                 return PCI_POWER_ERROR;
  

Re: Intel GemniLake xHCI connected devices can never wake up the system from suspend

[Daniel Drake]

Hi,

I'm working alongside Chris on this issue.

On Fri, Mar 16, 2018 at 12:11 AM, Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
> Scope (_SB.PCI0.XHC) has _PS0 method, so Linux will look for a _S3W to get
> the
> lowest possible D state in S3, but_S3W is missing, so Linux pci-acpi code
> will
> probably default to D0

Yes, _S3W is missing, and in this case, acpi_dev_pm_get_state() is
setting the maximum number permitted device power state to the minimum
value of D0.

So the XHCI controller is in D0 when you go into suspend.

I tried your hack to force it into D3hot. Now the system can wake up
from resume via the USB port. Thanks for finding that.

>> ASUS said the BIOS has no problem on USB wakeup under Windows so I don't
>> think
>> there's any update. Anything else could be cause for this?
>
> Linux and Windows probably check different DSDT values

I've studied the ACPI spec trying to understand better, but I'm
struggling with the question:
What is the maximum number (lowest power) permitted device power state
for a device that is configured as able to wake the system from S3,
**that does not implement the _S3W method**?

As far as I can see, the ACPI spec doesn't give an answer. It's clear
what the behaviour is when _S3W is present, but not unclear what
should happen when it is not there.

As noted above, Linux's interpretation is that in such case, the
device must remain fully on (D0) when going into S3. I am wondering if
Windows just has made an alternative assumption that all available
device power states can wake the system from suspend in such case.

This is working in Windows so hopefully we can find a way to match the
behaviour?

Thanks
Daniel

Intel GemniLake xHCI connected devices can never wake up the system from suspend

[Daniel Drake]

Hi,

I'm working alongside Chris on this issue.

On Fri, Mar 16, 2018 at 12:11 AM, Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
> Scope (_SB.PCI0.XHC) has _PS0 method, so Linux will look for a _S3W to get
> the
> lowest possible D state in S3, but_S3W is missing, so Linux pci-acpi code
> will
> probably default to D0

Yes, _S3W is missing, and in this case, acpi_dev_pm_get_state() is
setting the maximum number permitted device power state to the minimum
value of D0.

So the XHCI controller is in D0 when you go into suspend.

I tried your hack to force it into D3hot. Now the system can wake up
from resume via the USB port. Thanks for finding that.

>> ASUS said the BIOS has no problem on USB wakeup under Windows so I don't
>> think
>> there's any update. Anything else could be cause for this?
>
> Linux and Windows probably check different DSDT values

I've studied the ACPI spec trying to understand better, but I'm
struggling with the question:
What is the maximum number (lowest power) permitted device power state
for a device that is configured as able to wake the system from S3,
**that does not implement the _S3W method**?

As far as I can see, the ACPI spec doesn't give an answer. It's clear
what the behaviour is when _S3W is present, but not unclear what
should happen when it is not there.

As noted above, Linux's interpretation is that in such case, the
device must remain fully on (D0) when going into S3. I am wondering if
Windows just has made an alternative assumption that all available
device power states can wake the system from suspend in such case.

This is working in Windows so hopefully we can find a way to match the
behaviour?

Thanks
Daniel
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Intel GemniLake xHCI connected devices can never wake up the system from suspend

[Daniel Drake]

> I've studied the ACPI spec trying to understand better, but I'm
> struggling with the question:
> What is the maximum number (lowest power) permitted device power state
> for a device that is configured as able to wake the system from S3,
> **that does not implement the _S3W method**?

Actually the ACPI spec has an answer for the case when _S3D is present.
The lack of clarity is only over the situation when both _S3D and _S3W
are missing - like on the platforms being worked on here.

The _S3D docs say:
> If the device can wake the system from the S3 system sleeping state (see
> _PRW) then the device must support wake in the D-state returned by this
> object. However, OSPM cannot assume wake from the S3 system sleeping state
> is supported in any deeper D-state unless specified by a corresponding
> _S3W object

Looking at the design of the existing Linux code, it seems like this
"max = min" assignment that is causing us trouble originates directly
from an attempt to implement that logic: if we didn't get a response from
_S3W, then we must clamp ourselves to the data we got from _S3D.

If I modify the Linux code to be a little more specific in that logic
(only applying when we actually got something from _S3D) then the
problematic behaviour is avoided and USB wakeups work.

I feel that this change makes the Linux implementation more directly
mirror the wording in the ACPI spec and it's associated lack of clarity
for when both methods are missing. Thoughts?

---
 drivers/acpi/device_pm.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index a4c8ad98560d..44f12c5c75ee 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -543,6 +543,7 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
 	unsigned long long ret;
 	int d_min, d_max;
 	bool wakeup = false;
+	acpi_status sxd_status;
 	acpi_status status;
 
 	/*
@@ -565,8 +566,8 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
 		 * provided if AE_NOT_FOUND is returned.
 		 */
 		ret = d_min;
-		status = acpi_evaluate_integer(handle, method, NULL, &ret);
-		if ((ACPI_FAILURE(status) && status != AE_NOT_FOUND)
+		sxd_status = acpi_evaluate_integer(handle, method, NULL, &ret);
+		if ((ACPI_FAILURE(sxd_status) && sxd_status != AE_NOT_FOUND)
 		    || ret > ACPI_STATE_D3_COLD)
 			return -ENODATA;
 
@@ -599,7 +600,11 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
 		method[3] = 'W';
 		status = acpi_evaluate_integer(handle, method, NULL, &ret);
 		if (status == AE_NOT_FOUND) {
-			if (target_state > ACPI_STATE_S0)
+			/* No _SxW. In this case, the ACPI spec says that we
+			 * must not go into any power state deeper than the
+			 * value returned from _SxD.
+			 */
+			if (sxd_status == AE_OK && target_state > ACPI_STATE_S0)
 				d_max = d_min;
 		} else if (ACPI_SUCCESS(status) && ret <= ACPI_STATE_D3_COLD) {
 			/* Fall back to D3cold if ret is not a valid state. */
-- 
2.14.1

Intel GemniLake xHCI connected devices can never wake up the system from suspend

[Daniel Drake]

> I've studied the ACPI spec trying to understand better, but I'm
> struggling with the question:
> What is the maximum number (lowest power) permitted device power state
> for a device that is configured as able to wake the system from S3,
> **that does not implement the _S3W method**?

Actually the ACPI spec has an answer for the case when _S3D is present.
The lack of clarity is only over the situation when both _S3D and _S3W
are missing - like on the platforms being worked on here.

The _S3D docs say:
> If the device can wake the system from the S3 system sleeping state (see
> _PRW) then the device must support wake in the D-state returned by this
> object. However, OSPM cannot assume wake from the S3 system sleeping state
> is supported in any deeper D-state unless specified by a corresponding
> _S3W object

Looking at the design of the existing Linux code, it seems like this
"max = min" assignment that is causing us trouble originates directly
from an attempt to implement that logic: if we didn't get a response from
_S3W, then we must clamp ourselves to the data we got from _S3D.

If I modify the Linux code to be a little more specific in that logic
(only applying when we actually got something from _S3D) then the
problematic behaviour is avoided and USB wakeups work.

I feel that this change makes the Linux implementation more directly
mirror the wording in the ACPI spec and it's associated lack of clarity
for when both methods are missing. Thoughts?
---
 drivers/acpi/device_pm.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index a4c8ad98560d..44f12c5c75ee 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -543,6 +543,7 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
 	unsigned long long ret;
 	int d_min, d_max;
 	bool wakeup = false;
+	acpi_status sxd_status;
 	acpi_status status;
 
 	/*
@@ -565,8 +566,8 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
 		 * provided if AE_NOT_FOUND is returned.
 		 */
 		ret = d_min;
-		status = acpi_evaluate_integer(handle, method, NULL, &ret);
-		if ((ACPI_FAILURE(status) && status != AE_NOT_FOUND)
+		sxd_status = acpi_evaluate_integer(handle, method, NULL, &ret);
+		if ((ACPI_FAILURE(sxd_status) && sxd_status != AE_NOT_FOUND)
 		    || ret > ACPI_STATE_D3_COLD)
 			return -ENODATA;
 
@@ -599,7 +600,11 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
 		method[3] = 'W';
 		status = acpi_evaluate_integer(handle, method, NULL, &ret);
 		if (status == AE_NOT_FOUND) {
-			if (target_state > ACPI_STATE_S0)
+			/* No _SxW. In this case, the ACPI spec says that we
+			 * must not go into any power state deeper than the
+			 * value returned from _SxD.
+			 */
+			if (sxd_status == AE_OK && target_state > ACPI_STATE_S0)
 				d_max = d_min;
 		} else if (ACPI_SUCCESS(status) && ret <= ACPI_STATE_D3_COLD) {
 			/* Fall back to D3cold if ret is not a valid state. */

Re: Intel GemniLake xHCI connected devices can never wake up the system from suspend

[Mathias Nyman]

Adding Rafael directly to CC

In short, if _S3D and _S3W are missing in DSDT then a PCI device
stays in D0 during suspend in Linux, but goes to D3 in Windows.

USB wake doesn't work in Geminilake because of this.

Should this be changed? reasoning below.

On 16.03.2018 10:23, Daniel Drake wrote:
>> I've studied the ACPI spec trying to understand better, but I'm
>> struggling with the question:
>> What is the maximum number (lowest power) permitted device power state
>> for a device that is configured as able to wake the system from S3,
>> **that does not implement the _S3W method**?
> 
> Actually the ACPI spec has an answer for the case when _S3D is present.
> The lack of clarity is only over the situation when both _S3D and _S3W
> are missing - like on the platforms being worked on here.
> 
> The _S3D docs say:
>> If the device can wake the system from the S3 system sleeping state (see
>> _PRW) then the device must support wake in the D-state returned by this
>> object. However, OSPM cannot assume wake from the S3 system sleeping state
>> is supported in any deeper D-state unless specified by a corresponding
>> _S3W object
> 
> Looking at the design of the existing Linux code, it seems like this
> "max = min" assignment that is causing us trouble originates directly
> from an attempt to implement that logic: if we didn't get a response from
> _S3W, then we must clamp ourselves to the data we got from _S3D.
> 
> If I modify the Linux code to be a little more specific in that logic
> (only applying when we actually got something from _S3D) then the
> problematic behaviour is avoided and USB wakeups work.
> 
> I feel that this change makes the Linux implementation more directly
> mirror the wording in the ACPI spec and it's associated lack of clarity
> for when both methods are missing. Thoughts?
> 
> ---
>   drivers/acpi/device_pm.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index a4c8ad98560d..44f12c5c75ee 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -543,6 +543,7 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
>   	unsigned long long ret;
>   	int d_min, d_max;
>   	bool wakeup = false;
> +	acpi_status sxd_status;
>   	acpi_status status;
>   
>   	/*
> @@ -565,8 +566,8 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
>   		 * provided if AE_NOT_FOUND is returned.
>   		 */
>   		ret = d_min;
> -		status = acpi_evaluate_integer(handle, method, NULL, &ret);
> -		if ((ACPI_FAILURE(status) && status != AE_NOT_FOUND)
> +		sxd_status = acpi_evaluate_integer(handle, method, NULL, &ret);
> +		if ((ACPI_FAILURE(sxd_status) && sxd_status != AE_NOT_FOUND)
>   		    || ret > ACPI_STATE_D3_COLD)
>   			return -ENODATA;
>   
> @@ -599,7 +600,11 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
>   		method[3] = 'W';
>   		status = acpi_evaluate_integer(handle, method, NULL, &ret);
>   		if (status == AE_NOT_FOUND) {
> -			if (target_state > ACPI_STATE_S0)
> +			/* No _SxW. In this case, the ACPI spec says that we
> +			 * must not go into any power state deeper than the
> +			 * value returned from _SxD.
> +			 */
> +			if (sxd_status == AE_OK && target_state > ACPI_STATE_S0)
>   				d_max = d_min;
>   		} else if (ACPI_SUCCESS(status) && ret <= ACPI_STATE_D3_COLD) {
>   			/* Fall back to D3cold if ret is not a valid state. */
> 

Intel GemniLake xHCI connected devices can never wake up the system from suspend

[Mathias Nyman]

Adding Rafael directly to CC

In short, if _S3D and _S3W are missing in DSDT then a PCI device
stays in D0 during suspend in Linux, but goes to D3 in Windows.

USB wake doesn't work in Geminilake because of this.

Should this be changed? reasoning below.

On 16.03.2018 10:23, Daniel Drake wrote:
>> I've studied the ACPI spec trying to understand better, but I'm
>> struggling with the question:
>> What is the maximum number (lowest power) permitted device power state
>> for a device that is configured as able to wake the system from S3,
>> **that does not implement the _S3W method**?
> 
> Actually the ACPI spec has an answer for the case when _S3D is present.
> The lack of clarity is only over the situation when both _S3D and _S3W
> are missing - like on the platforms being worked on here.
> 
> The _S3D docs say:
>> If the device can wake the system from the S3 system sleeping state (see
>> _PRW) then the device must support wake in the D-state returned by this
>> object. However, OSPM cannot assume wake from the S3 system sleeping state
>> is supported in any deeper D-state unless specified by a corresponding
>> _S3W object
> 
> Looking at the design of the existing Linux code, it seems like this
> "max = min" assignment that is causing us trouble originates directly
> from an attempt to implement that logic: if we didn't get a response from
> _S3W, then we must clamp ourselves to the data we got from _S3D.
> 
> If I modify the Linux code to be a little more specific in that logic
> (only applying when we actually got something from _S3D) then the
> problematic behaviour is avoided and USB wakeups work.
> 
> I feel that this change makes the Linux implementation more directly
> mirror the wording in the ACPI spec and it's associated lack of clarity
> for when both methods are missing. Thoughts?
> 
> ---
>   drivers/acpi/device_pm.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index a4c8ad98560d..44f12c5c75ee 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -543,6 +543,7 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
>   	unsigned long long ret;
>   	int d_min, d_max;
>   	bool wakeup = false;
> +	acpi_status sxd_status;
>   	acpi_status status;
>   
>   	/*
> @@ -565,8 +566,8 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
>   		 * provided if AE_NOT_FOUND is returned.
>   		 */
>   		ret = d_min;
> -		status = acpi_evaluate_integer(handle, method, NULL, &ret);
> -		if ((ACPI_FAILURE(status) && status != AE_NOT_FOUND)
> +		sxd_status = acpi_evaluate_integer(handle, method, NULL, &ret);
> +		if ((ACPI_FAILURE(sxd_status) && sxd_status != AE_NOT_FOUND)
>   		    || ret > ACPI_STATE_D3_COLD)
>   			return -ENODATA;
>   
> @@ -599,7 +600,11 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
>   		method[3] = 'W';
>   		status = acpi_evaluate_integer(handle, method, NULL, &ret);
>   		if (status == AE_NOT_FOUND) {
> -			if (target_state > ACPI_STATE_S0)
> +			/* No _SxW. In this case, the ACPI spec says that we
> +			 * must not go into any power state deeper than the
> +			 * value returned from _SxD.
> +			 */
> +			if (sxd_status == AE_OK && target_state > ACPI_STATE_S0)
>   				d_max = d_min;
>   		} else if (ACPI_SUCCESS(status) && ret <= ACPI_STATE_D3_COLD) {
>   			/* Fall back to D3cold if ret is not a valid state. */
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Intel GemniLake xHCI connected devices can never wake up the system from suspend

[Rafael J. Wysocki]

On Fri, Mar 16, 2018 at 10:06 AM, Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
> Adding Rafael directly to CC
>
> In short, if _S3D and _S3W are missing in DSDT then a PCI device
> stays in D0 during suspend in Linux, but goes to D3 in Windows.
>
> USB wake doesn't work in Geminilake because of this.
>
> Should this be changed? reasoning below.

It can be changed if that doesn't cause problems to happen.


> On 16.03.2018 10:23, Daniel Drake wrote:
>>>
>>> I've studied the ACPI spec trying to understand better, but I'm
>>> struggling with the question:
>>> What is the maximum number (lowest power) permitted device power state
>>> for a device that is configured as able to wake the system from S3,
>>> **that does not implement the _S3W method**?
>>
>>
>> Actually the ACPI spec has an answer for the case when _S3D is present.
>> The lack of clarity is only over the situation when both _S3D and _S3W
>> are missing - like on the platforms being worked on here.
>>
>> The _S3D docs say:
>>>
>>> If the device can wake the system from the S3 system sleeping state (see
>>> _PRW) then the device must support wake in the D-state returned by this
>>> object. However, OSPM cannot assume wake from the S3 system sleeping
>>> state
>>> is supported in any deeper D-state unless specified by a corresponding
>>> _S3W object
>>
>>
>> Looking at the design of the existing Linux code, it seems like this
>> "max = min" assignment that is causing us trouble originates directly
>> from an attempt to implement that logic: if we didn't get a response from
>> _S3W, then we must clamp ourselves to the data we got from _S3D.
>>
>> If I modify the Linux code to be a little more specific in that logic
>> (only applying when we actually got something from _S3D) then the
>> problematic behaviour is avoided and USB wakeups work.
>>
>> I feel that this change makes the Linux implementation more directly
>> mirror the wording in the ACPI spec and it's associated lack of clarity
>> for when both methods are missing. Thoughts?
>>
>> ---
>>   drivers/acpi/device_pm.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
>> index a4c8ad98560d..44f12c5c75ee 100644
>> --- a/drivers/acpi/device_pm.c
>> +++ b/drivers/acpi/device_pm.c
>> @@ -543,6 +543,7 @@ static int acpi_dev_pm_get_state(struct device *dev,
>> struct acpi_device *adev,
>>         unsigned long long ret;
>>         int d_min, d_max;
>>         bool wakeup = false;
>> +       acpi_status sxd_status;
>>         acpi_status status;
>>         /*
>> @@ -565,8 +566,8 @@ static int acpi_dev_pm_get_state(struct device *dev,
>> struct acpi_device *adev,
>>                  * provided if AE_NOT_FOUND is returned.
>>                  */
>>                 ret = d_min;
>> -               status = acpi_evaluate_integer(handle, method, NULL,
>> &ret);
>> -               if ((ACPI_FAILURE(status) && status != AE_NOT_FOUND)
>> +               sxd_status = acpi_evaluate_integer(handle, method, NULL,
>> &ret);
>> +               if ((ACPI_FAILURE(sxd_status) && sxd_status !=
>> AE_NOT_FOUND)
>>                     || ret > ACPI_STATE_D3_COLD)
>>                         return -ENODATA;
>>   @@ -599,7 +600,11 @@ static int acpi_dev_pm_get_state(struct device
>> *dev, struct acpi_device *adev,
>>                 method[3] = 'W';
>>                 status = acpi_evaluate_integer(handle, method, NULL,
>> &ret);
>>                 if (status == AE_NOT_FOUND) {
>> -                       if (target_state > ACPI_STATE_S0)
>> +                       /* No _SxW. In this case, the ACPI spec says that
>> we
>> +                        * must not go into any power state deeper than
>> the
>> +                        * value returned from _SxD.
>> +                        */
>> +                       if (sxd_status == AE_OK && target_state >
>> ACPI_STATE_S0)
>>                                 d_max = d_min;
>>                 } else if (ACPI_SUCCESS(status) && ret <=
>> ACPI_STATE_D3_COLD) {
>>                         /* Fall back to D3cold if ret is not a valid
>> state. */
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Intel GemniLake xHCI connected devices can never wake up the system from suspend

[Rafael J. Wysocki]

On Fri, Mar 16, 2018 at 10:06 AM, Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
> Adding Rafael directly to CC
>
> In short, if _S3D and _S3W are missing in DSDT then a PCI device
> stays in D0 during suspend in Linux, but goes to D3 in Windows.
>
> USB wake doesn't work in Geminilake because of this.
>
> Should this be changed? reasoning below.

It can be changed if that doesn't cause problems to happen.


> On 16.03.2018 10:23, Daniel Drake wrote:
>>>
>>> I've studied the ACPI spec trying to understand better, but I'm
>>> struggling with the question:
>>> What is the maximum number (lowest power) permitted device power state
>>> for a device that is configured as able to wake the system from S3,
>>> **that does not implement the _S3W method**?
>>
>>
>> Actually the ACPI spec has an answer for the case when _S3D is present.
>> The lack of clarity is only over the situation when both _S3D and _S3W
>> are missing - like on the platforms being worked on here.
>>
>> The _S3D docs say:
>>>
>>> If the device can wake the system from the S3 system sleeping state (see
>>> _PRW) then the device must support wake in the D-state returned by this
>>> object. However, OSPM cannot assume wake from the S3 system sleeping
>>> state
>>> is supported in any deeper D-state unless specified by a corresponding
>>> _S3W object
>>
>>
>> Looking at the design of the existing Linux code, it seems like this
>> "max = min" assignment that is causing us trouble originates directly
>> from an attempt to implement that logic: if we didn't get a response from
>> _S3W, then we must clamp ourselves to the data we got from _S3D.
>>
>> If I modify the Linux code to be a little more specific in that logic
>> (only applying when we actually got something from _S3D) then the
>> problematic behaviour is avoided and USB wakeups work.
>>
>> I feel that this change makes the Linux implementation more directly
>> mirror the wording in the ACPI spec and it's associated lack of clarity
>> for when both methods are missing. Thoughts?
>>
>> ---
>>   drivers/acpi/device_pm.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
>> index a4c8ad98560d..44f12c5c75ee 100644
>> --- a/drivers/acpi/device_pm.c
>> +++ b/drivers/acpi/device_pm.c
>> @@ -543,6 +543,7 @@ static int acpi_dev_pm_get_state(struct device *dev,
>> struct acpi_device *adev,
>>         unsigned long long ret;
>>         int d_min, d_max;
>>         bool wakeup = false;
>> +       acpi_status sxd_status;
>>         acpi_status status;
>>         /*
>> @@ -565,8 +566,8 @@ static int acpi_dev_pm_get_state(struct device *dev,
>> struct acpi_device *adev,
>>                  * provided if AE_NOT_FOUND is returned.
>>                  */
>>                 ret = d_min;
>> -               status = acpi_evaluate_integer(handle, method, NULL,
>> &ret);
>> -               if ((ACPI_FAILURE(status) && status != AE_NOT_FOUND)
>> +               sxd_status = acpi_evaluate_integer(handle, method, NULL,
>> &ret);
>> +               if ((ACPI_FAILURE(sxd_status) && sxd_status !=
>> AE_NOT_FOUND)
>>                     || ret > ACPI_STATE_D3_COLD)
>>                         return -ENODATA;
>>   @@ -599,7 +600,11 @@ static int acpi_dev_pm_get_state(struct device
>> *dev, struct acpi_device *adev,
>>                 method[3] = 'W';
>>                 status = acpi_evaluate_integer(handle, method, NULL,
>> &ret);
>>                 if (status == AE_NOT_FOUND) {
>> -                       if (target_state > ACPI_STATE_S0)
>> +                       /* No _SxW. In this case, the ACPI spec says that
>> we
>> +                        * must not go into any power state deeper than
>> the
>> +                        * value returned from _SxD.
>> +                        */
>> +                       if (sxd_status == AE_OK && target_state >
>> ACPI_STATE_S0)
>>                                 d_max = d_min;
>>                 } else if (ACPI_SUCCESS(status) && ret <=
>> ACPI_STATE_D3_COLD) {
>>                         /* Fall back to D3cold if ret is not a valid
>> state. */
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Intel GemniLake xHCI connected devices can never wake up the system from suspend

[Daniel Drake]

On Mon, Mar 19, 2018 at 6:36 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Fri, Mar 16, 2018 at 10:06 AM, Mathias Nyman
> <mathias.nyman@linux.intel.com> wrote:
>> Adding Rafael directly to CC
>>
>> In short, if _S3D and _S3W are missing in DSDT then a PCI device
>> stays in D0 during suspend in Linux, but goes to D3 in Windows.
>>
>> USB wake doesn't work in Geminilake because of this.
>>
>> Should this be changed? reasoning below.
>
> It can be changed if that doesn't cause problems to happen.

I double checked that Windows 10 is going into S3 suspend and that USB
wakeup works fine on this platform - it works fine there.
Device manager for the XHCI controller clearly shows D3 being used for
S3 suspend: https://imgur.com/lF9U3V0

Current power state:
D0

Power capabilities:
00000089
PDCAP_D0_SUPPORTED
PDCAP_D3_SUPPORTED
PDCAP_WAKE_FROM_D3_SUPPORTED

Power state mappings:
S0 -> D0
S1 -> Unspecified
S2 -> Unspecified
S3 -> D3
S4 -> D3
S5 -> D3

The biggest risk of my proposed change is that we would now end up
putting a wakeup-enabled device into a too low power state where it
can no longer wake up the system. On the other hand, it solves this
issue affecting 2 different vendors, and may even result in some power
savings in general.

Perhaps we can consider the change for inclusion in Linux-4.17, giving
2-3 months of testing time.

I'll submit the patch shortly.

Thanks
Daniel

Intel GemniLake xHCI connected devices can never wake up the system from suspend

[Daniel Drake]

On Mon, Mar 19, 2018 at 6:36 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Fri, Mar 16, 2018 at 10:06 AM, Mathias Nyman
> <mathias.nyman@linux.intel.com> wrote:
>> Adding Rafael directly to CC
>>
>> In short, if _S3D and _S3W are missing in DSDT then a PCI device
>> stays in D0 during suspend in Linux, but goes to D3 in Windows.
>>
>> USB wake doesn't work in Geminilake because of this.
>>
>> Should this be changed? reasoning below.
>
> It can be changed if that doesn't cause problems to happen.

I double checked that Windows 10 is going into S3 suspend and that USB
wakeup works fine on this platform - it works fine there.
Device manager for the XHCI controller clearly shows D3 being used for
S3 suspend: https://imgur.com/lF9U3V0

Current power state:
D0

Power capabilities:
00000089
PDCAP_D0_SUPPORTED
PDCAP_D3_SUPPORTED
PDCAP_WAKE_FROM_D3_SUPPORTED

Power state mappings:
S0 -> D0
S1 -> Unspecified
S2 -> Unspecified
S3 -> D3
S4 -> D3
S5 -> D3

The biggest risk of my proposed change is that we would now end up
putting a wakeup-enabled device into a too low power state where it
can no longer wake up the system. On the other hand, it solves this
issue affecting 2 different vendors, and may even result in some power
savings in general.

Perhaps we can consider the change for inclusion in Linux-4.17, giving
2-3 months of testing time.

I'll submit the patch shortly.

Thanks
Daniel
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html