linux-kernel - Re: [PATCH 1/3] HID: i2c-hid-acpi: Remove explicit device_set_wakeup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHQZ30Czx_ZOPHBBc0yRqdcyVqrT9nxqKTpe8QZU2-fJGVs9_A@mail.gmail.com>
Date:   Tue, 21 Dec 2021 16:51:36 -0700
From:   Raul Rangel <rrangel@...omium.org>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     linux-kernel@...r.kernel.org,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        mario.limonciello@....com, linux-input@...r.kernel.org,
        dianders@...omium.org,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Jiri Kosina <jikos@...nel.org>
Subject: Re: [PATCH 1/3] HID: i2c-hid-acpi: Remove explicit device_set_wakeup_capable

On Tue, Dec 21, 2021 at 11:49 AM Hans de Goede <hdegoede@...hat.com> wrote:
>
> 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.
>

I thought of something else:

> And i2c-hid suspend does:
>
>         if (device_may_wakeup(&client->dev)) {
>                 wake_status = enable_irq_wake(client->irq);
>

I think we also need to guard the enable_irq_wake call with
`!ACPI_COMPANION(dev)`.
ACPI will handle enabling the correct GPIO or GPE defined in `_PRW`.

We might also be able to remove manually calling {enable,disable}_irq_wake by
switching over to `dev_pm_set_wake_irq`. I did this for the elan_i2c
driver in the 2nd patch:
https://patchwork.kernel.org/project/linux-input/patch/20211220163823.2.Id022caf53d01112188308520915798f08a33cd3e@changeid/

> 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@...omium.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);
> >  }
> >
>