Message ID | 20180328085530.8453-2-kai.heng.feng@canonical.com |
---|---|
State | Accepted |
Headers | show |
Series | Fix an issue that when system in S3, USB keyboard can't wake up the system. | expand |
On 28.03.2018 10:55, Kai-Heng Feng wrote: > From: Daniel Drake <drake@endlessm.com> > > BugLink: https://bugs.launchpad.net/bugs/1759511 > > acpi_dev_pm_get_state() is used to determine the range of allowable > device power states when going into S3 suspend. This is implemented > by executing the _S3D and _S3W ACPI methods. > > Linux follows the ACPI spec behaviour in that when _S3D is implemented > and _S3W is not, Linux will not go into a power state deeper than the one > returned by _S3D for a wakeup-enabled device. > > However, this same logic is being applied to the case when neither > _S3D nor _S3W are present, and the result is that this function > decides that the device must stay in D0 (fully on) state. > > This is breaking USB wakeups on Asus V222GA and Acer XC-830. _S3D and > _S3W are not present, so the USB controller is left in the D0 running > state during S3, and hence it is unable to generate a PME# wake event. > > The ACPI spec is unclear on which power states are permissable for > wakeup-enabled devices when both _S3D and _S3W are missing. > However, USB wakeups work fine on these platforms under Windows, where > device manager shows that they are using D3 device state for the USB > controller in S3. > > I assume that the "max = min" clamping done by the code here is > specifically written for the _S3D but no _S3W case. By making the > code true to those conditions, avoiding them on these platforms, > the controller will be put into D3 state and USB wakeups start working. > > Additionally I feel that this change makes the code more directly > mirror the wording of the ACPI spec and it's associated lack of clarity. > > Thanks to Mathias Nyman for pointing us in the right direction. > > Signed-off-by: Daniel Drake <drake@endlessm.com> > Link: http://lkml.kernel.org/r/CAB4CAwf_k-WsF3zL4epm9TKAOu0h=Bv1XhXV_gY3bziOo_NPKA@mail.gmail.com > > https://phabricator.endlessm.com/T21410 > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > (cherry picked from commit bf8c6184e0c3d4d5e005e085e9f96f478a267b20 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next) > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- Since this is pulled from linux-next, I would suspect this is also wanted in Bionic, right? > drivers/acpi/device_pm.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c > index 2ed6935d4483..5bb754b1ccb2 100644 > --- a/drivers/acpi/device_pm.c > +++ b/drivers/acpi/device_pm.c > @@ -534,6 +534,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; > + bool has_sxd = false; > acpi_status status; > > /* > @@ -572,6 +573,10 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev, > else > return -ENODATA; > } > + > + if (status == AE_OK) > + has_sxd = true; > + > d_min = ret; > wakeup = device_may_wakeup(dev) && adev->wakeup.flags.valid > && adev->wakeup.sleep_state >= target_state; > @@ -591,7 +596,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 (has_sxd && 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. */ >
Stefan Bader <stefan.bader@canonical.com> wrote: > On 28.03.2018 10:55, Kai-Heng Feng wrote: >> From: Daniel Drake <drake@endlessm.com> >> >> BugLink: https://bugs.launchpad.net/bugs/1759511 >> >> acpi_dev_pm_get_state() is used to determine the range of allowable >> device power states when going into S3 suspend. This is implemented >> by executing the _S3D and _S3W ACPI methods. >> >> Linux follows the ACPI spec behaviour in that when _S3D is implemented >> and _S3W is not, Linux will not go into a power state deeper than the one >> returned by _S3D for a wakeup-enabled device. >> >> However, this same logic is being applied to the case when neither >> _S3D nor _S3W are present, and the result is that this function >> decides that the device must stay in D0 (fully on) state. >> >> This is breaking USB wakeups on Asus V222GA and Acer XC-830. _S3D and >> _S3W are not present, so the USB controller is left in the D0 running >> state during S3, and hence it is unable to generate a PME# wake event. >> >> The ACPI spec is unclear on which power states are permissable for >> wakeup-enabled devices when both _S3D and _S3W are missing. >> However, USB wakeups work fine on these platforms under Windows, where >> device manager shows that they are using D3 device state for the USB >> controller in S3. >> >> I assume that the "max = min" clamping done by the code here is >> specifically written for the _S3D but no _S3W case. By making the >> code true to those conditions, avoiding them on these platforms, >> the controller will be put into D3 state and USB wakeups start working. >> >> Additionally I feel that this change makes the code more directly >> mirror the wording of the ACPI spec and it's associated lack of clarity. >> >> Thanks to Mathias Nyman for pointing us in the right direction. >> >> Signed-off-by: Daniel Drake <drake@endlessm.com> >> Link: >> http://lkml.kernel.org/r/CAB4CAwf_k-WsF3zL4epm9TKAOu0h=Bv1XhXV_gY3bziOo_NPKA@mail.gmail.com >> >> https://phabricator.endlessm.com/T21410 >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> (cherry picked from commit bf8c6184e0c3d4d5e005e085e9f96f478a267b20 >> git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git >> linux-next) >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > Acked-by: Stefan Bader <stefan.bader@canonical.com> > >> --- > > Since this is pulled from linux-next, I would suspect this is also wanted > in > Bionic, right? This patch should be included for all linux version. I planed to ask linux-stable to include the patch once it's in Linus' tree, so we will get this patch through stable update. Kai-Heng > >> drivers/acpi/device_pm.c | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c >> index 2ed6935d4483..5bb754b1ccb2 100644 >> --- a/drivers/acpi/device_pm.c >> +++ b/drivers/acpi/device_pm.c >> @@ -534,6 +534,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; >> + bool has_sxd = false; >> acpi_status status; >> >> /* >> @@ -572,6 +573,10 @@ static int acpi_dev_pm_get_state(struct device >> *dev, struct acpi_device *adev, >> else >> return -ENODATA; >> } >> + >> + if (status == AE_OK) >> + has_sxd = true; >> + >> d_min = ret; >> wakeup = device_may_wakeup(dev) && adev->wakeup.flags.valid >> && adev->wakeup.sleep_state >= target_state; >> @@ -591,7 +596,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 (has_sxd && 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. */
On Wed, Mar 28, 2018 at 11:58:16PM +0800, Kai-Heng Feng wrote: > Stefan Bader <stefan.bader@canonical.com> wrote: > > > On 28.03.2018 10:55, Kai-Heng Feng wrote: > > > From: Daniel Drake <drake@endlessm.com> > > > > > > BugLink: https://bugs.launchpad.net/bugs/1759511 > > > > > > acpi_dev_pm_get_state() is used to determine the range of allowable > > > device power states when going into S3 suspend. This is implemented > > > by executing the _S3D and _S3W ACPI methods. > > > > > > Linux follows the ACPI spec behaviour in that when _S3D is implemented > > > and _S3W is not, Linux will not go into a power state deeper than the one > > > returned by _S3D for a wakeup-enabled device. > > > > > > However, this same logic is being applied to the case when neither > > > _S3D nor _S3W are present, and the result is that this function > > > decides that the device must stay in D0 (fully on) state. > > > > > > This is breaking USB wakeups on Asus V222GA and Acer XC-830. _S3D and > > > _S3W are not present, so the USB controller is left in the D0 running > > > state during S3, and hence it is unable to generate a PME# wake event. > > > > > > The ACPI spec is unclear on which power states are permissable for > > > wakeup-enabled devices when both _S3D and _S3W are missing. > > > However, USB wakeups work fine on these platforms under Windows, where > > > device manager shows that they are using D3 device state for the USB > > > controller in S3. > > > > > > I assume that the "max = min" clamping done by the code here is > > > specifically written for the _S3D but no _S3W case. By making the > > > code true to those conditions, avoiding them on these platforms, > > > the controller will be put into D3 state and USB wakeups start working. > > > > > > Additionally I feel that this change makes the code more directly > > > mirror the wording of the ACPI spec and it's associated lack of clarity. > > > > > > Thanks to Mathias Nyman for pointing us in the right direction. > > > > > > Signed-off-by: Daniel Drake <drake@endlessm.com> > > > Link: http://lkml.kernel.org/r/CAB4CAwf_k-WsF3zL4epm9TKAOu0h=Bv1XhXV_gY3bziOo_NPKA@mail.gmail.com > > > > > > https://phabricator.endlessm.com/T21410 > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > (cherry picked from commit bf8c6184e0c3d4d5e005e085e9f96f478a267b20 > > > git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git > > > linux-next) > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > Acked-by: Stefan Bader <stefan.bader@canonical.com> > > > > > --- > > > > Since this is pulled from linux-next, I would suspect this is also > > wanted in > > Bionic, right? > > This patch should be included for all linux version. > I planed to ask linux-stable to include the patch once it's in Linus' tree, > so we will get this patch through stable update. We should go ahead and get it into bionic anyway. Applied to bionic/master-next.
diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c index 2ed6935d4483..5bb754b1ccb2 100644 --- a/drivers/acpi/device_pm.c +++ b/drivers/acpi/device_pm.c @@ -534,6 +534,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; + bool has_sxd = false; acpi_status status; /* @@ -572,6 +573,10 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev, else return -ENODATA; } + + if (status == AE_OK) + has_sxd = true; + d_min = ret; wakeup = device_may_wakeup(dev) && adev->wakeup.flags.valid && adev->wakeup.sleep_state >= target_state; @@ -591,7 +596,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 (has_sxd && 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. */