Message-ID: <7d4f20da-f18b-eccf-19c4-ea7c15492634@redhat.com>
Date:   Tue, 21 Dec 2021 19:49:38 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH 1/3] HID: i2c-hid-acpi: Remove explicit
 device_set_wakeup_capable
Content-Language: en-US
To:     Raul E Rangel <rrangel@chromium.org>, linux-kernel@vger.kernel.org,
        "Rafael J . Wysocki" <rjw@rjwysocki.net>
Cc:     mario.limonciello@amd.com, linux-input@vger.kernel.org,
        dianders@chromium.org,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        Jiri Kosina <jikos@kernel.org>
References: <20211220234346.2798027-1-rrangel@chromium.org>
 <20211220163823.1.Ie20ca47a26d3ea68124d8197b67bb1344c67f650@changeid>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20211220163823.1.Ie20ca47a26d3ea68124d8197b67bb1344c67f650@changeid>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 12/21/21 00:43, Raul E Rangel wrote:
> The ACPI subsystem is responsible for managing the power and wake
> sources for an ACPI device. By explicitly calling
> device_set_wakeup_capable, we are circumvent the ACPI subsystem and
> setting wake capabilities on the device when it doesn't support it.
> 
> Take the following example:
> * We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have
> `_PRW` so that means the device can't wake the system.
> * The IRQ line is active level low for this device and is pulled up by the
> power resource defined in `_PR0`/`_PR3`.
> 
> Since the i2c-hid driver has set the device as wake capable, the wake
> pin gets enabled on suspend.

The IRQ pin should only have a enable_irq_wake() called on it if
something has actually requested the i2c-HID device to be a wakeup source,
the removed code claims the device is wakeup *capable*, but is also
explicitly calls device_set_wakeup_enable(dev, false), disabling wakeup.

And i2c-hid suspend does:

        if (device_may_wakeup(&client->dev)) {
                wake_status = enable_irq_wake(client->irq);

And device_may_wakeup() checks the wakeup *enabled* setting AFAIK.

I've added Rafael to the Cc since he knows all this a lot better then me.

I have the feeling that your userspace is perhaps poking the
wakeup settings in sysfs, triggering this issue.

> As part of suspend, ACPI will power down
> the device since it's not a wake source. When the device is powered
> down, the IRQ line will drop, and it will trigger a wake event.

To me that sounds like the device is not wakeup *capable* at all, so
its ACPI node should not set the ACPI_FADT_LOW_POWER_S0 flag at all.

Note I'm not certain about this at all, but at a first look this feels
like it is not the right fix for your problem.

Regards,

Hans


> 
> See the following debug log:
> [   42.335804] PM: Suspending system (s2idle)
> [   42.340186] amd_gpio AMD0030:00: RX: Setting wake for pin 89 to enable
> [   42.467736]     power-0416 __acpi_power_off      : Power resource [PR00] turned off
> [   42.467739] device_pm-0280 device_set_power      : Device [H05D] transitioned to D3cold
> [   42.475210] PM: pm_system_irq_wakeup: 11 triggered pinctrl_amd
> [   42.535293] PM: Wakeup unrelated to ACPI SCI
> [   42.535294] PM: resume from suspend-to-idle
> 
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> ---
> 
>  drivers/hid/i2c-hid/i2c-hid-acpi.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c
> index a6f0257a26de..fc311a19a19d 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-acpi.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c
> @@ -105,11 +105,6 @@ static int i2c_hid_acpi_probe(struct i2c_client *client)
>  
>  	acpi_device_fix_up_power(adev);
>  
> -	if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
> -		device_set_wakeup_capable(dev, true);
> -		device_set_wakeup_enable(dev, false);
> -	}
> -
>  	return i2c_hid_core_probe(client, &ihid_acpi->ops,
>  				  hid_descriptor_address);
>  }
>