On Fri, Mar 16, 2018 at 10:06 AM, Mathias Nyman
<mathias.nyman@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
