Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <bf11034c-54c1-c4eb-dc26-0a6872e04121@redhat.com>
Date:   Mon, 31 Jan 2022 13:41:18 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.4.0
Subject: Re: [PATCH 2/3] Input: elan_i2c - Use PM subsystem to manage wake irq
Content-Language: en-US
To:     Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc:     Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        Raul E Rangel <rrangel@chromium.org>,
        linux-kernel@vger.kernel.org, Mario.Limonciello@amd.com,
        linux-input@vger.kernel.org, dianders@chromium.org,
        "jingle.wu" <jingle.wu@emc.com.tw>
References: <20211220234346.2798027-1-rrangel@chromium.org>
 <20211220163823.2.Id022caf53d01112188308520915798f08a33cd3e@changeid>
 <YcE+xrSnS7qw0G1/@google.com>
 <9b004b3d-deed-1b63-2344-a445a9e53b61@redhat.com>
 <YcToUCQ8gzzSWbrm@google.com>
 <a71d4e73-6db8-16e7-2a3c-e50e26c0a07e@redhat.com>
 <CAAd53p4UbV=M_GrDMFz7qqqWRF23mXD=D7bDj-4b6rxgWupDMA@mail.gmail.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <CAAd53p4UbV=M_GrDMFz7qqqWRF23mXD=D7bDj-4b6rxgWupDMA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 12/25/21 14:51, Kai-Heng Feng wrote:
> On Fri, Dec 24, 2021 at 7:11 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 12/23/21 22:21, Dmitry Torokhov wrote:
>>> On Thu, Dec 23, 2021 at 03:42:24PM +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 12/21/21 03:41, Dmitry Torokhov wrote:
>>>>> Hi Raul,
>>>>>
>>>>> On Mon, Dec 20, 2021 at 04:43:45PM -0700, Raul E Rangel wrote:
>>>>>> @@ -1368,11 +1367,13 @@ static int elan_probe(struct i2c_client *client,
>>>>>>    }
>>>>>>
>>>>>>    /*
>>>>>> -   * Systems using device tree should set up wakeup via DTS,
>>>>>> +   * Systems using device tree or ACPI should set up wakeup via DTS/ACPI,
>>>>>>     * the rest will configure device as wakeup source by default.
>>>>>>     */
>>>>>> -  if (!dev->of_node)
>>>>>> +  if (!dev->of_node && !ACPI_COMPANION(dev)) {
>>>>>
>>>>> I think this will break our Rambis that use ACPI for enumeration but
>>>>> actually lack _PRW. As far as I remember their trackpads were capable
>>>>> of waking up the system.
>>>>>
>>>>> I think we should remove this chunk completely and instead add necessary
>>>>> code to drivers/platform/chrome/chrome-laptop.c (I suppose we need to
>>>>> have additional member in struct acpi_peripheral to indicate whether
>>>>> device needs to be configured for wakeup and then act upon it in
>>>>> chromeos_laptop_adjust_client().
>>>
>>> FWIW I looked at Rambi some more and I see that it actually defines a
>>> separate device an ACPI to handle wakeups, it is separate from the ACPI
>>> node for the trackpad:
>>>
>>> Scope (\_SB)
>>> {
>>> #ifdef BOARD_TRACKPAD_IRQ
>>>         /* Wake device for touchpad */
>>>         Device (TPAD)
>>>         {
>>>                 Name (_HID, EisaId ("PNP0C0E"))
>>>                 Name (_UID, 1)
>>>                 Name (_PRW, Package() { BOARD_TRACKPAD_WAKE_GPIO, 0x3 })
>>>
>>>                 Name (RBUF, ResourceTemplate()
>>>                 {
>>>                         Interrupt (ResourceConsumer, Level, ActiveLow)
>>>                         {
>>>                                 BOARD_TRACKPAD_IRQ
>>>                         }
>>>                 })
>>>
>>>                 Method (_CRS)
>>>                 {
>>>                         /* Only return interrupt if I2C1 is PCI mode */
>>>                         If (LEqual (\S1EN, 0)) {
>>>                                 Return (^RBUF)
>>>                         }
>>>
>>>                         /* Return empty resource template otherwise */
>>>                         Return (ResourceTemplate() {})
>>>                 }
>>>         }
>>> #endif
>>>
>>> I am not quite sure why we did this...
>>>
>>>>>
>>>>>>            device_init_wakeup(dev, true);
>>>>>> +          dev_pm_set_wake_irq(dev, client->irq);
>>>>>> +  }
>>>>
>>>> As I already mentioned in my other reply in this thread:
>>>>
>>>> https://lore.kernel.org/linux-input/f594afab-8c1a-8821-a775-e5512e17ce8f@redhat.com/
>>>>
>>>> AFAICT most x86 ACPI laptops do not use GPEs for wakeup by touchpad and
>>>> as such they do not have a _PRW method.
>>>>
>>>> So for wakeup by elan_i2c touchpads to keep working this code is not
>>>> just necessary for some ChromeOS devices, but it is necessary on
>>>> most ACPI devices.
>>>>
>>>> The problem of not making these calls on devices where a GPE is actually
>>>> used for touchpad wakeup (which at least for now is the exception not
>>>> the rule) should probably be fixed by no running this "chunk"
>>>> when the device has an ACPI_COMPANION (as this patch already checks)
>>>> *and* that ACPI_COMPANION has a valid _PRW method.
>>>>
>>>> Simply removing this chunk, or taking this patch as is will very
>>>> likely lead to regressions on various x86 laptop models.
>>>
>>> Hans, could you share a couple of DSDTs for devices that do not use GPEs
>>> for wakeup?
>>>
>>> For OF we already recognize that wakeup source/interrupt might differ
>>> from "main" I2C interrupt, I guess we need to do similar for ACPI cases.
>>> The question is to how determine if a device is supposed to be a wakeup
>>> source if it does not have _PRW.
>>
>> With s2idle (rather then S3) we never really suspend, we just put
>> everything in an as low power state as possible and call halt on the
>> CPU and then hope that the SoC power-management-unit shuts of a whole
>> bunch of power-planes based on all the devices being in a low power
>> state.
>>
>> This means that in practice with s2idle any device can be a wakeup
>> device since regular IRQs work fine as wakeup sources in s2idle.
>>
>> This is what the s2idle support in the i2c-hid code is based on:
>> drivers/hid/i2c-hid/i2c-hid-acpi.c:
>>
>>         if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
>>                 device_set_wakeup_capable(dev, true);
>>                 device_set_wakeup_enable(dev, false);
>>         }
>>
>> So I did just test this on a Lenovo ThinkPad X1 carbon gen 8, which
>> uses i2c_hid_acpi as driver for its touchpad and if I echo
>> enabled to the wakeup attr there, then wakeup by touchpad does work.
>>
>> One interesting thing there is that the touchpad ACPI node does not
>> have _PS0 and _PS3. Which means that the touchpad working as wakeup
>> device makes sense, since it can not be turned off at all.
>>
>> So I guess we could extend the above check in the i2c-hid-acpi
>> code to read:
>>
>>         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);
>>         }
>>
>> Because if there is a _PS3, which presumably is the case for
>> the troublesome touchscreen Raul is trying to fix, then we
>> will call that on suspend; and after that it is likely that
>> the device will not work as a wakeup source.
>>
>> And I just checked the DSDT of a couple of devices where I'm
>> reasonable sure that the touchpad uses I2C-HID and none of
>> them define _PS0/_PS3 methods on the touchpad ACPI node.
>>
>> So I think that the above suggestion should fix things
>> for the i2c-hid case.
>>
>> I've added Kai-Heng, the author of the original change
>> introducing the device_set_wakeup_capable() call, to the Cc.
>> Kai-Heng what do you think about this ?
>>
>> Raul, can you check if this resolves your issue?
>>
>> FWIW here is an acpidump of the X1C8:
>> https://fedorapeople.org/~jwrdegoede/acpidump-lenovo-x1c8
>>
>> Regards,
>>
>> Hans
>>
>>
>> p.s.
>>
>> An other interesting datapoint is that despite not declaring
>> a _PRW method the DSDTs which I've checked do all 3 contain
>> an _S0W method, returning 3 or 4. Which suggests that maybe the
>> ACPI code should look at _S0W even when no GPE is being used?
>>

Sorry for being slow to respond.

> Maybe "ExclusiveAndWake" in _CRS is enough? ACPI spec says "whether it
> is capable of waking the system from a low-power idle or system sleep
> state" without mentioning the need for _PRW.

Ah yes checking for that is probable even better. We probably need to
add some ACPI helper for that though.

Regards,

hans