> On Nov 25, 2020, at 01:48, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> 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@xxxxxxxxxx> wrote:
>>>
>>> On Tue, Nov 24, 2020 at 8:36 AM Kai-Heng Feng
>>> <kai.heng.feng@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxxxxx>
>
> ---
> 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);
