Re: [PATCH] ACPI: PM: Re-enable ACPI GPE if it's already enabled

[Kai-Heng Feng <kai.heng.feng@canonical.com>]

> 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);