Message-ID: <CAHQZ30DjvDbvequmnWimMiCBT3txmndQaiGu12D0EKN=1MvjKQ@mail.gmail.com>
Date:   Tue, 30 Aug 2022 11:10:20 -0600
From:   Raul Rangel <rrangel@...omium.org>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     Hans de Goede <hdegoede@...hat.com>,
        "Limonciello, Mario" <mario.limonciello@....com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux ACPI <linux-acpi@...r.kernel.org>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        linux-input <linux-input@...r.kernel.org>,
        Kai-Heng Feng <kai.heng.feng@...onical.com>,
        Tim Van Patten <timvp@...gle.com>
Subject: Re: [PATCH] i2c: ACPI: Do not check ACPI_FADT_LOW_POWER_S0

So I think this is only half of the puzzle. The patches I linked above
work great if the wake bit is set. It falls apart once I add a power
resource.

i.e., given the following ACPI node:
    Device (D015)
    {
        Name (_HID, "ELAN0000")  // _HID: Hardware ID
        Name (_UID, Zero)  // _UID: Unique ID
        Name (_DDN, "ELAN Touchpad")  // _DDN: DOS Device Name
        Method (_STA, 0, NotSerialized)  // _STA: Status
        {
            Return (0x0F)
        }

        Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
        {
            I2cSerialBusV2 (0x0015, ControllerInitiated, 0x00061A80,
                AddressingMode7Bit, "\\_SB.I2C0",
                0x00, ResourceConsumer, , Exclusive,
                )
            GpioInt (Edge, ActiveLow, ExclusiveAndWake, PullDefault, 0x0000,
                "\\_SB.GPIO", 0x00, ResourceConsumer, ,
                )
                {   // Pin list
                    0x0028
                }
        })
        Name (_S0W, 0x03)  // _S0W: S0 Device Wake State
        Name (_PR0, Package (0x01)  // _PR0: Power Resources for D0
        {
            PR00
        })
        Name (_PR3, Package (0x01)  // _PR3: Power Resources for D3hot
        {
            PR00
        })
        PowerResource (PR00, 0x00, 0x0000)
        {
            Method (_STA, 0, Serialized) {} // _STA: Status
            Method (_ON, 0, Serialized) {} // _ON_: Power On
            Method (_OFF, 0, Serialized) {} // _OFF: Power Off
        }
    }

The above snippet has a power resource that can turn the device on and
off. It also uses the ExclusiveAndWake bit to signal that the IRQ can
be used to wake the system.
What ends up happening on suspend is the device_pm subsystem enables
the GPIO for wake, but the ACPI device_pm system, specifically
`acpi_dev_pm_get_state`, doesn't know the device is armed for wake and
powers off the device.

[acpi_dev_pm_get_state](https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/v5.15/drivers/acpi/device_pm.c;l=640)
uses `adev->wakeup.flags.valid` to decide if the device is armed for
wake. The `adev->wakeup` field is used to determine if the device has
a `_PRW` and associated GPE. Since we aren't using `_PRW` there is no
associated wake GPE. On suspend the ACPI device_pm subsystem puts the
device into D3Cold and thus the device can't wake the system. Ideally
`wakeup` would be true if we have a GPIO armed, and
`acpi_dev_pm_get_state` would call `_S0W`. This would put the device
into `D3Hot` so it can wake the system.

So I was thinking of having `acpi_dev_pm_get_state` check if a wakeup
IRQ is armed:

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 19b33c028f356b..c68fa869dc6f75 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -634,7 +634,8 @@ static int acpi_dev_pm_get_state(struct device
*dev, struct acpi_device *adev,
                wakeup = device_may_wakeup(dev) && adev->wakeup.flags.valid
                        && adev->wakeup.sleep_state >= target_state;
        } else {
-               wakeup = adev->wakeup.flags.valid;
+               wakeup = adev->wakeup.flags.valid ||
+                        (device_may_wakeup(dev) && dev->power.wakeirq);
        }

        /*

I haven't tested this approach yet, but it seems clean and simple.

The alternative is to have the ACPI subsystem manage the Wake IRQ and
forgo calling `dev_pm_set_wake_irq` from the drivers. I'm not sure
what the best hook point is though. I'm thinking that when a driver
calls `acpi_dev_gpio_irq_get_by` we can store the wake_irq on the ACPI
node. Then in `acpi_pm_set_device_wakeup` we can arm/disarm the IRQ.
The benefit of this approach is that we call `_DSW` via
`acpi_device_sleep_wake` so we can enable/disable the device's wake
capabilities.

Or we take a hybrid approach and use `dev_pm_set_wake_irq` and still
call `_DSW`.

Thoughts?

On Fri, Aug 26, 2022 at 3:48 PM Raul Rangel <rrangel@...omium.org> wrote:
>
> So after tracing a bunch of code, I finally got a solution that I
> think will work. I just uploaded the patch train here:
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3858568.
> I'll push it to the mailing list once I do a bit more testing.
>
> Do we need to support setting the wake_irq for systems that don't use
> DT or ACPI? Ideally I would drop the following block:
>
> if (!dev->of_node && !has_acpi_companion(dev)) {
>     device_init_wakeup(dev, true);
>     dev_pm_set_wake_irq(dev, client->irq);
> }
>
> There are also a few other i2c drivers that need cleanup:
> * https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/v5.15/drivers/mfd/max8925-i2c.c;l=218
> * https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/v5.15/drivers/input/touchscreen/elants_i2c.c;l=1629
> * https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/v5.15/drivers/input/touchscreen/raydium_i2c_ts.c;l=1190
>
> I can send CLs to delete the enable_irq_wake calls from those drivers
> if we don't need to support non-DT/non-ACPI boards. Or I can send CLs
> to add the boiler plate from above. Do we even need the `disable_irq`
> calls in the suspend handlers or can the PM subsystem take care of
> that?
>
> Do we also need to handle reading the wake bit from Interrupt/IRQ ACPI
> resources? Can those actually wake the system? On AMD platforms the
> IO-APIC/PIC can't actually wake the system. It either needs to be an
> ACPI GPE or the GPIO controller. If we do need to support it, I can
> add some more plumbing.
>
> Thanks!
>
>
> On Mon, Aug 8, 2022 at 11:10 AM Rafael J. Wysocki <rafael@...nel.org> wrote:
> >
> > On Sat, Aug 6, 2022 at 4:20 AM Raul Rangel <rrangel@...omium.org> wrote:
> > >
> > > I do plan on coming back and updating those patches. I got derailed
> > > with other priorities.
> >
> > I'll leave it to you then.  I'm mostly interested in dropping the
> > misguided ACPI_FADT_LOW_POWER_S0 check.
> >
> > > But as Hans pointed out, we wanted to use
> > > `ExclusiveAndWake` to make the decision since not all IRQs can be wake
> > > sources while in s0i3.
> >
> > S0i3 is still S0, so all of the interrupts that work in S0 will still work.
> >
> > What really matters is whether or not enable_irq_wake() is called for
> > the given IRQ, but I'm not sufficiently familiar with the code in
> > question to comment on it any further without thorough investigation.
> >
> > And of course the device needs to be able to generate interrupts in
> > the first place and if it is power-manageable by ACPI, I would just
> > leave the wakeup handling to the generic ACPI code.
> >
> > >
> > > On Fri, Aug 5, 2022 at 12:54 PM Hans de Goede <hdegoede@...hat.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On 8/5/22 19:08, Rafael J. Wysocki wrote:
> > > > > On Fri, Aug 5, 2022 at 6:59 PM Limonciello, Mario
> > > > > <mario.limonciello@....com> wrote:
> > > > >>
> > > > >> On 8/5/2022 11:51, Rafael J. Wysocki wrote:
> > > > >>> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > > > >>>
> > > > >>> The ACPI_FADT_LOW_POWER_S0 flag merely means that it is better to
> > > > >>> use low-power S0 idle on the given platform than S3 (provided that
> > > > >>> the latter is supported) and it doesn't preclude using either of
> > > > >>> them (which of them will be used depends on the choices made by user
> > > > >>> space).
> > > > >>>
> > > > >>> Because of that, ACPI_FADT_LOW_POWER_S0 is generally not sufficient
> > > > >>> for making decisions in device drivers and so i2c_hid_acpi_probe()
> > > > >>> should not use it.
> > > > >>>
> > > > >>> Moreover, Linux always supports suspend-to-idle, so if a given
> > > > >>> device can wake up the system from suspend-to-idle, then it can be
> > > > >>> marked as wakeup capable unconditionally, so make that happen in
> > > > >>> i2c_hid_acpi_probe().
> > > > >>>
> > > > >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > > > >>
> > > > >> +Raul
> > > > >> +Hans
> > > > >> +KH
> > > > >>
> > > > >> Raul had a patch that was actually going to just tear out this code
> > > > >> entirely:
> > > > >> https://lkml.kernel.org/lkml/20211220163823.1.Ie20ca47a26d3ea68124d8197b67bb1344c67f650@changeid/
> > > > >>
> > > > >> As part of that patch series discussion another suggestion had
> > > > >> transpired
> > > > >> (https://patchwork.kernel.org/project/linux-input/patch/20211220163823.2.Id022caf53d01112188308520915798f08a33cd3e@changeid/#24681016):
> > > > >>
> > > > >> ```
> > > > >> if ((acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) &&
> > > > >>             !adev->flags.power_manageable) {
> > > > >>                  device_set_wakeup_capable(dev, true);
> > > > >>                  device_set_wakeup_enable(dev, false);
> > > > >>          }
> > > > >> ```
> > > > >>
> > > > >> If this is being changed, maybe consider that suggestion to
> > > > >> check `adev->flags.power_manageable`.
> > > > >
> > > > > Fair enough, I'll send a v2 with this check added.
> > > >
> > > > Re-reading the original thread:
> > > > https://lkml.kernel.org/lkml/20211220163823.1.Ie20ca47a26d3ea68124d8197b67bb1344c67f650@changeid/T/#u
> > > >
> > > > The conclusion there was that the :
> > > >
> > > >                  device_set_wakeup_capable(dev, true);
> > > >                  device_set_wakeup_enable(dev, false);
> > > >
> > > > Calls should be made conditional on the IRQ being
> > > > marked ExclusiveAndWake instead of the ACPI_FADT_LOW_POWER_S0
> > > > check.
> > > >
> > > > Regards,
> > > >
> > > > Hans
> > > >

Powered by blists - more mailing lists