| Message ID | 20180316082320.12636-1-drake@endlessm.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
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-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-acpi" 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/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. */