| Message ID | 20201124073619.771940-1-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | ACPI: PM: Re-enable ACPI GPE if it's already enabled | expand |
On Tue, Nov 24, 2020 at 8:36 AM Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > > Dell Precision 5550 fails to detect Thunderbolt device hotplug events, > once the Thunderbolt device and its root port are runtime-suspended to > D3cold. > > While putting the entire hierarchy to D3cold, the root port ACPI GPE is > enabled via acpi_pci_propagate_wakeup() when suspending Thunderbolt > bridges/switches. So when putting the root port to D3cold as last step, > ACPI GPE is untouched as it's already enabled. > > However, platform may need PCI devices to be in D3hot or PME enabled > prior enabling GPE to make it work. What platforms and why. > So re-enable ACPI GPE to address this. > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/acpi/device_pm.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c > index 94d91c67aeae..dc25d9d204ae 100644 > --- a/drivers/acpi/device_pm.c > +++ b/drivers/acpi/device_pm.c > @@ -757,11 +757,10 @@ static int __acpi_device_wakeup_enable(struct acpi_device *adev, > > mutex_lock(&acpi_wakeup_lock); > > - if (wakeup->enable_count >= max_count) > - goto out; > - > - if (wakeup->enable_count > 0) > - goto inc; > + if (wakeup->enable_count > 0) { > + acpi_disable_gpe(wakeup->gpe_device, wakeup->gpe_number); > + acpi_disable_wakeup_device_power(adev); > + } An event occurring at this point may be lost after this patch. It looks like you are trying to work around a hardware issue. Can you please describe that issue in detail? > > error = acpi_enable_wakeup_device_power(adev, target_state); > if (error) > @@ -777,8 +776,8 @@ static int __acpi_device_wakeup_enable(struct acpi_device *adev, > acpi_handle_debug(adev->handle, "GPE%2X enabled for wakeup\n", > (unsigned int)wakeup->gpe_number); > > -inc: > - wakeup->enable_count++; > + if (wakeup->enable_count < max_count) > + wakeup->enable_count++; > > out: > mutex_unlock(&acpi_wakeup_lock); > -- > 2.29.2 >
> On Nov 24, 2020, at 22:00, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Tue, Nov 24, 2020 at 8:36 AM Kai-Heng Feng > <kai.heng.feng@canonical.com> wrote: >> >> Dell Precision 5550 fails to detect Thunderbolt device hotplug events, >> once the Thunderbolt device and its root port are runtime-suspended to >> D3cold. >> >> While putting the entire hierarchy to D3cold, the root port ACPI GPE is >> enabled via acpi_pci_propagate_wakeup() when suspending Thunderbolt >> bridges/switches. So when putting the root port to D3cold as last step, >> ACPI GPE is untouched as it's already enabled. >> >> However, platform may need PCI devices to be in D3hot or PME enabled >> prior enabling GPE to make it work. > > What platforms and why. Dell Precision 5550. Its thunderbolt port can't detect newly plugged thunderbolt devices. > >> So re-enable ACPI GPE to address this. >> >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >> --- >> drivers/acpi/device_pm.c | 13 ++++++------- >> 1 file changed, 6 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c >> index 94d91c67aeae..dc25d9d204ae 100644 >> --- a/drivers/acpi/device_pm.c >> +++ b/drivers/acpi/device_pm.c >> @@ -757,11 +757,10 @@ static int __acpi_device_wakeup_enable(struct acpi_device *adev, >> >> mutex_lock(&acpi_wakeup_lock); >> >> - if (wakeup->enable_count >= max_count) >> - goto out; >> - >> - if (wakeup->enable_count > 0) >> - goto inc; >> + if (wakeup->enable_count > 0) { >> + acpi_disable_gpe(wakeup->gpe_device, wakeup->gpe_number); >> + acpi_disable_wakeup_device_power(adev); >> + } > > An event occurring at this point may be lost after this patch. Yes, so this approach is not optimal. > > It looks like you are trying to work around a hardware issue. Windows doesn't have this issue. So I don't think it's hardware issue. > Can you > please describe that issue in detail? The GPE used by Thunderbolt root port, was previously enabled by Thunderbolt switches/bridges. So when the GPE is already enabled when Thunderbolt root port is suspended. However, the GPE needs to be enabled after root port is suspended, and that's the approach this patch takes. Is there any actual hardware benefits from acpi_pci_propagate_wakeup()? If there's no actual device benefits from it, we can remove it and only enable GPE for the root port. Otherwise we need to quirk off Thunderbolt bridges/switches, since their native PME just work without the need to enable root port GPE. Kai-Heng > >> >> error = acpi_enable_wakeup_device_power(adev, target_state); >> if (error) >> @@ -777,8 +776,8 @@ static int __acpi_device_wakeup_enable(struct acpi_device *adev, >> acpi_handle_debug(adev->handle, "GPE%2X enabled for wakeup\n", >> (unsigned int)wakeup->gpe_number); >> >> -inc: >> - wakeup->enable_count++; >> + if (wakeup->enable_count < max_count) >> + wakeup->enable_count++; >> >> out: >> mutex_unlock(&acpi_wakeup_lock); >> -- >> 2.29.2
On Tuesday, November 24, 2020 6:31:56 PM CET Kai-Heng Feng wrote: > > > On Nov 24, 2020, at 22:00, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Tue, Nov 24, 2020 at 8:36 AM Kai-Heng Feng > > <kai.heng.feng@canonical.com> wrote: > >> > >> Dell Precision 5550 fails to detect Thunderbolt device hotplug events, > >> once the Thunderbolt device and its root port are runtime-suspended to > >> D3cold. > >> > >> While putting the entire hierarchy to D3cold, the root port ACPI GPE is > >> enabled via acpi_pci_propagate_wakeup() when suspending Thunderbolt > >> bridges/switches. So when putting the root port to D3cold as last step, > >> ACPI GPE is untouched as it's already enabled. > >> > >> However, platform may need PCI devices to be in D3hot or PME enabled > >> prior enabling GPE to make it work. > > > > What platforms and why. > > Dell Precision 5550. Its thunderbolt port can't detect newly plugged thunderbolt devices. OK > > > >> So re-enable ACPI GPE to address this. > >> > >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > >> --- > >> drivers/acpi/device_pm.c | 13 ++++++------- > >> 1 file changed, 6 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c > >> index 94d91c67aeae..dc25d9d204ae 100644 > >> --- a/drivers/acpi/device_pm.c > >> +++ b/drivers/acpi/device_pm.c > >> @@ -757,11 +757,10 @@ static int __acpi_device_wakeup_enable(struct acpi_device *adev, > >> > >> mutex_lock(&acpi_wakeup_lock); > >> > >> - if (wakeup->enable_count >= max_count) > >> - goto out; > >> - > >> - if (wakeup->enable_count > 0) > >> - goto inc; > >> + if (wakeup->enable_count > 0) { > >> + acpi_disable_gpe(wakeup->gpe_device, wakeup->gpe_number); > >> + acpi_disable_wakeup_device_power(adev); > >> + } > > > > An event occurring at this point may be lost after this patch. > > Yes, so this approach is not optimal. > > > > > It looks like you are trying to work around a hardware issue. > > Windows doesn't have this issue. So I don't think it's hardware issue. Windows may exercise the hardware in a different way. > > Can you > > please describe that issue in detail? > > The GPE used by Thunderbolt root port, was previously enabled by Thunderbolt switches/bridges. This shouldn't matter, because enabling a GPE means flipping its bit in the "enable" register. There's no dependency between that and the devices below the port. There may be dependency there for enabling the device wakeup power, however. > So when the GPE is already enabled when Thunderbolt root port is suspended. > However, the GPE needs to be enabled after root port is suspended, and that's the approach this patch takes. No, it is not. It still enables the GPE and the device wakeup power before putting the port into D3. Please see pci_finish_runtime_suspend() for details. However, it enables wakeup for after putting the subordinate device(s) into D3hot which may matter in theory. > Is there any actual hardware benefits from acpi_pci_propagate_wakeup()? Yes, there is AFAICS. > If there's no actual device benefits from it, we can remove it and only enable GPE for the root port. > Otherwise we need to quirk off Thunderbolt bridges/switches, since their native PME just work without the need to enable root port GPE. Can you please check if the alternative (untested) patch below still helps? --- drivers/acpi/device_pm.c | 40 ++++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 14 deletions(-) Index: linux-pm/drivers/acpi/device_pm.c =================================================================== --- linux-pm.orig/drivers/acpi/device_pm.c +++ linux-pm/drivers/acpi/device_pm.c @@ -749,7 +749,7 @@ static void acpi_pm_notify_work_func(str static DEFINE_MUTEX(acpi_wakeup_lock); static int __acpi_device_wakeup_enable(struct acpi_device *adev, - u32 target_state, int max_count) + u32 target_state) { struct acpi_device_wakeup *wakeup = &adev->wakeup; acpi_status status; @@ -757,15 +757,26 @@ static int __acpi_device_wakeup_enable(s mutex_lock(&acpi_wakeup_lock); - if (wakeup->enable_count >= max_count) - goto out; - + /* + * If the device wakeup power is already enabled, disable it and enable + * it again in case it depends on the configuration of subordinate + * devices and the conditions have changed since it was enabled last + * time. + */ if (wakeup->enable_count > 0) - goto inc; + acpi_disable_wakeup_device_power(adev); error = acpi_enable_wakeup_device_power(adev, target_state); - if (error) + if (error) { + if (wakeup->enable_count > 0) { + acpi_disable_gpe(wakeup->gpe_device, wakeup->gpe_number); + wakeup->enable_count = 0; + } goto out; + } + + if (wakeup->enable_count > 0) + goto inc; status = acpi_enable_gpe(wakeup->gpe_device, wakeup->gpe_number); if (ACPI_FAILURE(status)) { @@ -778,7 +789,10 @@ static int __acpi_device_wakeup_enable(s (unsigned int)wakeup->gpe_number); inc: - wakeup->enable_count++; + if (wakeup->enable_count < INT_MAX) + wakeup->enable_count++; + else + acpi_handle_info(adev->handle, "Wakeup enable count out of bounds!\n"); out: mutex_unlock(&acpi_wakeup_lock); @@ -799,7 +813,7 @@ out: */ static int acpi_device_wakeup_enable(struct acpi_device *adev, u32 target_state) { - return __acpi_device_wakeup_enable(adev, target_state, 1); + return __acpi_device_wakeup_enable(adev, target_state); } /** @@ -829,8 +843,7 @@ out: mutex_unlock(&acpi_wakeup_lock); } -static int __acpi_pm_set_device_wakeup(struct device *dev, bool enable, - int max_count) +static int __acpi_pm_set_device_wakeup(struct device *dev, bool enable) { struct acpi_device *adev; int error; @@ -850,8 +863,7 @@ static int __acpi_pm_set_device_wakeup(s return 0; } - error = __acpi_device_wakeup_enable(adev, acpi_target_system_state(), - max_count); + error = __acpi_device_wakeup_enable(adev, acpi_target_system_state()); if (!error) dev_dbg(dev, "Wakeup enabled by ACPI\n"); @@ -865,7 +877,7 @@ static int __acpi_pm_set_device_wakeup(s */ int acpi_pm_set_device_wakeup(struct device *dev, bool enable) { - return __acpi_pm_set_device_wakeup(dev, enable, 1); + return __acpi_pm_set_device_wakeup(dev, enable); } EXPORT_SYMBOL_GPL(acpi_pm_set_device_wakeup); @@ -876,7 +888,7 @@ EXPORT_SYMBOL_GPL(acpi_pm_set_device_wak */ int acpi_pm_set_bridge_wakeup(struct device *dev, bool enable) { - return __acpi_pm_set_device_wakeup(dev, enable, INT_MAX); + return __acpi_pm_set_device_wakeup(dev, enable); } EXPORT_SYMBOL_GPL(acpi_pm_set_bridge_wakeup);
> On Nov 25, 2020, at 01:48, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Tuesday, November 24, 2020 6:31:56 PM CET Kai-Heng Feng wrote: >> >>> On Nov 24, 2020, at 22:00, Rafael J. Wysocki <rafael@kernel.org> wrote: >>> >>> On Tue, Nov 24, 2020 at 8:36 AM Kai-Heng Feng >>> <kai.heng.feng@canonical.com> wrote: >>>> >>>> Dell Precision 5550 fails to detect Thunderbolt device hotplug events, >>>> once the Thunderbolt device and its root port are runtime-suspended to >>>> D3cold. >>>> >>>> While putting the entire hierarchy to D3cold, the root port ACPI GPE is >>>> enabled via acpi_pci_propagate_wakeup() when suspending Thunderbolt >>>> bridges/switches. So when putting the root port to D3cold as last step, >>>> ACPI GPE is untouched as it's already enabled. >>>> >>>> However, platform may need PCI devices to be in D3hot or PME enabled >>>> prior enabling GPE to make it work. >>> >>> What platforms and why. >> >> Dell Precision 5550. Its thunderbolt port can't detect newly plugged thunderbolt devices. > > OK > >>> >>>> So re-enable ACPI GPE to address this. >>>> >>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >>>> --- >>>> drivers/acpi/device_pm.c | 13 ++++++------- >>>> 1 file changed, 6 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c >>>> index 94d91c67aeae..dc25d9d204ae 100644 >>>> --- a/drivers/acpi/device_pm.c >>>> +++ b/drivers/acpi/device_pm.c >>>> @@ -757,11 +757,10 @@ static int __acpi_device_wakeup_enable(struct acpi_device *adev, >>>> >>>> mutex_lock(&acpi_wakeup_lock); >>>> >>>> - if (wakeup->enable_count >= max_count) >>>> - goto out; >>>> - >>>> - if (wakeup->enable_count > 0) >>>> - goto inc; >>>> + if (wakeup->enable_count > 0) { >>>> + acpi_disable_gpe(wakeup->gpe_device, wakeup->gpe_number); >>>> + acpi_disable_wakeup_device_power(adev); >>>> + } >>> >>> An event occurring at this point may be lost after this patch. >> >> Yes, so this approach is not optimal. >> >>> >>> It looks like you are trying to work around a hardware issue. >> >> Windows doesn't have this issue. So I don't think it's hardware issue. > > Windows may exercise the hardware in a different way. > >>> Can you >>> please describe that issue in detail? >> >> The GPE used by Thunderbolt root port, was previously enabled by Thunderbolt switches/bridges. > > This shouldn't matter, because enabling a GPE means flipping its bit in the > "enable" register. There's no dependency between that and the devices below > the port. > > There may be dependency there for enabling the device wakeup power, however. Right, didn't notice re-enabling the wakeup power alone can solve this. > >> So when the GPE is already enabled when Thunderbolt root port is suspended. >> However, the GPE needs to be enabled after root port is suspended, and that's the approach this patch takes. > > No, it is not. > > It still enables the GPE and the device wakeup power before putting the port > into D3. Please see pci_finish_runtime_suspend() for details. What I meant "already enabled" is that GPE doesn't get touched because of "wakeup->enable_count > 0" check. > > However, it enables wakeup for after putting the subordinate device(s) into D3hot > which may matter in theory. > >> Is there any actual hardware benefits from acpi_pci_propagate_wakeup()? > > Yes, there is AFAICS. > >> If there's no actual device benefits from it, we can remove it and only enable GPE for the root port. >> Otherwise we need to quirk off Thunderbolt bridges/switches, since their native PME just work without the need to enable root port GPE. > > Can you please check if the alternative (untested) patch below still helps? Yes, it helps. Thanks a lot! Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > --- > drivers/acpi/device_pm.c | 40 ++++++++++++++++++++++++++-------------- > 1 file changed, 26 insertions(+), 14 deletions(-) > > Index: linux-pm/drivers/acpi/device_pm.c > =================================================================== > --- linux-pm.orig/drivers/acpi/device_pm.c > +++ linux-pm/drivers/acpi/device_pm.c > @@ -749,7 +749,7 @@ static void acpi_pm_notify_work_func(str > static DEFINE_MUTEX(acpi_wakeup_lock); > > static int __acpi_device_wakeup_enable(struct acpi_device *adev, > - u32 target_state, int max_count) > + u32 target_state) > { > struct acpi_device_wakeup *wakeup = &adev->wakeup; > acpi_status status; > @@ -757,15 +757,26 @@ static int __acpi_device_wakeup_enable(s > > mutex_lock(&acpi_wakeup_lock); > > - if (wakeup->enable_count >= max_count) > - goto out; > - > + /* > + * If the device wakeup power is already enabled, disable it and enable > + * it again in case it depends on the configuration of subordinate > + * devices and the conditions have changed since it was enabled last > + * time. > + */ > if (wakeup->enable_count > 0) > - goto inc; > + acpi_disable_wakeup_device_power(adev); > > error = acpi_enable_wakeup_device_power(adev, target_state); > - if (error) > + if (error) { > + if (wakeup->enable_count > 0) { > + acpi_disable_gpe(wakeup->gpe_device, wakeup->gpe_number); > + wakeup->enable_count = 0; > + } > goto out; > + } > + > + if (wakeup->enable_count > 0) > + goto inc; > > status = acpi_enable_gpe(wakeup->gpe_device, wakeup->gpe_number); > if (ACPI_FAILURE(status)) { > @@ -778,7 +789,10 @@ static int __acpi_device_wakeup_enable(s > (unsigned int)wakeup->gpe_number); > > inc: > - wakeup->enable_count++; > + if (wakeup->enable_count < INT_MAX) > + wakeup->enable_count++; > + else > + acpi_handle_info(adev->handle, "Wakeup enable count out of bounds!\n"); > > out: > mutex_unlock(&acpi_wakeup_lock); > @@ -799,7 +813,7 @@ out: > */ > static int acpi_device_wakeup_enable(struct acpi_device *adev, u32 target_state) > { > - return __acpi_device_wakeup_enable(adev, target_state, 1); > + return __acpi_device_wakeup_enable(adev, target_state); > } > > /** > @@ -829,8 +843,7 @@ out: > mutex_unlock(&acpi_wakeup_lock); > } > > -static int __acpi_pm_set_device_wakeup(struct device *dev, bool enable, > - int max_count) > +static int __acpi_pm_set_device_wakeup(struct device *dev, bool enable) > { > struct acpi_device *adev; > int error; > @@ -850,8 +863,7 @@ static int __acpi_pm_set_device_wakeup(s > return 0; > } > > - error = __acpi_device_wakeup_enable(adev, acpi_target_system_state(), > - max_count); > + error = __acpi_device_wakeup_enable(adev, acpi_target_system_state()); > if (!error) > dev_dbg(dev, "Wakeup enabled by ACPI\n"); > > @@ -865,7 +877,7 @@ static int __acpi_pm_set_device_wakeup(s > */ > int acpi_pm_set_device_wakeup(struct device *dev, bool enable) > { > - return __acpi_pm_set_device_wakeup(dev, enable, 1); > + return __acpi_pm_set_device_wakeup(dev, enable); > } > EXPORT_SYMBOL_GPL(acpi_pm_set_device_wakeup); > > @@ -876,7 +888,7 @@ EXPORT_SYMBOL_GPL(acpi_pm_set_device_wak > */ > int acpi_pm_set_bridge_wakeup(struct device *dev, bool enable) > { > - return __acpi_pm_set_device_wakeup(dev, enable, INT_MAX); > + return __acpi_pm_set_device_wakeup(dev, enable); > } > EXPORT_SYMBOL_GPL(acpi_pm_set_bridge_wakeup);
diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c index 94d91c67aeae..dc25d9d204ae 100644 --- a/drivers/acpi/device_pm.c +++ b/drivers/acpi/device_pm.c @@ -757,11 +757,10 @@ static int __acpi_device_wakeup_enable(struct acpi_device *adev, mutex_lock(&acpi_wakeup_lock); - if (wakeup->enable_count >= max_count) - goto out; - - if (wakeup->enable_count > 0) - goto inc; + if (wakeup->enable_count > 0) { + acpi_disable_gpe(wakeup->gpe_device, wakeup->gpe_number); + acpi_disable_wakeup_device_power(adev); + } error = acpi_enable_wakeup_device_power(adev, target_state); if (error) @@ -777,8 +776,8 @@ static int __acpi_device_wakeup_enable(struct acpi_device *adev, acpi_handle_debug(adev->handle, "GPE%2X enabled for wakeup\n", (unsigned int)wakeup->gpe_number); -inc: - wakeup->enable_count++; + if (wakeup->enable_count < max_count) + wakeup->enable_count++; out: mutex_unlock(&acpi_wakeup_lock);
Dell Precision 5550 fails to detect Thunderbolt device hotplug events, once the Thunderbolt device and its root port are runtime-suspended to D3cold. While putting the entire hierarchy to D3cold, the root port ACPI GPE is enabled via acpi_pci_propagate_wakeup() when suspending Thunderbolt bridges/switches. So when putting the root port to D3cold as last step, ACPI GPE is untouched as it's already enabled. However, platform may need PCI devices to be in D3hot or PME enabled prior enabling GPE to make it work. So re-enable ACPI GPE to address this. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/acpi/device_pm.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)