Re: [RFC] i2c: core: Do not enable wakeup by default

["Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>]

On 2/8/2023 7:57 AM, Mika Westerberg wrote:
> Hi,
> 
> On Tue, Feb 07, 2023 at 09:33:55AM -0700, Raul Rangel wrote:
>> Sorry, resending in plain text mode.
>>
>> On Tue, Feb 7, 2023 at 12:25 AM Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>>>
>>> After commit b38f2d5d9615 ("i2c: acpi: Use ACPI wake capability bit to
>>> set wake_irq") the I2C core has been setting I2C_CLIENT_WAKE for ACPI
>>> devices if they announce to be wake capable in their device description.
>>> However, on certain systems where audio codec has been connected through
>>> I2C this causes system suspend to wake up immediately because power to
>>> the codec is turned off which pulls the interrupt line "low" triggering
>>> wake up.
>>>
>>> Possible reason why the interrupt is marked as wake capable is that some
>>> codecs apparently support "Wake on Voice" or similar functionality.
>>
>> That's generally a bug in the ACPI tables. The wake bit shouldn't be
>> set if the power domain for the device is powered off on suspend. The
>> best thing is to fix the ACPI tables, but if you can't, then you can
>> set the ignore_wake flag for the device:
>> https://github.com/torvalds/linux/blob/master/drivers/gpio/gpiolib-acpi.c#L31.
>> If that works we can add a quirk for the device:
>> https://github.com/torvalds/linux/blob/master/drivers/gpio/gpiolib-acpi.c#L1633.

I've seen this one already and also tried to use it, but it didn't work. 
Also when I was reading code I wasn't really convinced that it is linked 
to i2c in any straightforward way. I mean i2c decides in different 
places that it has wake support (I even added some prints to make sure 
;). The code you pointed out decides in 
https://github.com/torvalds/linux/blob/master/drivers/gpio/gpiolib-acpi.c#L387 
but i2c code seems to decide in 
https://github.com/torvalds/linux/blob/master/drivers/i2c/i2c-core-acpi.c#L176 
where it just checks if irq flags has wake_capable flag set. When I 
looked at it previously I was pretty sure it comes straight from BIOS 
and passes the quirk code you mentioned, still I may have missed something.

> 
> I think (hope) these systems are not yet available for public so there
> is a chance that the tables can still be fixed, without need to add any
> quirks.
> 
> @Amadeusz, @Cezary, if that's the case I suggest filing a bug against
> the BIOS.
> 

Well, I tried custom DSDT and had problems, but I just remembered that I 
probably need to pass "revision+1" in file, so kernel sees it as a newer 
version, let me try again. Is it enough to replace "ExclusiveAndWake" 
with "Exclusive"?

>>> In any case, I don't think we should be enabling wakeup by default on
>>> all I2C devices that are wake capable. According to device_init_wakeup()
>>> documentation most devices should leave it disabled, with exceptions on
>>> devices such as keyboards, power buttons etc. Userspace can enable
>>> wakeup as needed by writing to device "power/wakeup" attribute.
>>
>> Enabling wake by default was an unintended side-effect. I didn't catch
>> this when I wrote the patch :/ It's been exposing all the incorrect
>> ACPI configurations for better or worse. Mario pushed a patch up
>> earlier to disable thes Wake GPIOs when using S3:
>> https://github.com/torvalds/linux/commit/d63f11c02b8d3e54bdb65d8c309f73b7f474aec4.
>> Are you having problems with S3 or S0iX?
> 
> I think this case is S0ix.

We test both cases in our setups.

> 
>>> Reported-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
>>> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>>> ---
>>> Hi,
>>>
>>> Sending this as RFC because I'm not too familiar with the usage of
>>> I2C_CLIENT_WAKE and whether this is something that is expected behaviour
>>> in users of I2C devices. On ACPI side I think this is the correct thing
>>> to do at least.
>>>
>>>   drivers/i2c/i2c-core-base.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>>> index 087e480b624c..7046549bdae7 100644
>>> --- a/drivers/i2c/i2c-core-base.c
>>> +++ b/drivers/i2c/i2c-core-base.c
>>> @@ -527,7 +527,7 @@ static int i2c_device_probe(struct device *dev)
>>>                          goto put_sync_adapter;
>>>                  }
>>>
>>> -               device_init_wakeup(&client->dev, true);
>>> +               device_init_wakeup(&client->dev, false);
>>
>> This would be a change in behavior for Device Tree. Maybe you can
>> declare a `bool enable_wake = true`, then in the ACPI branch
>> (https://github.com/torvalds/linux/blob/master/drivers/i2c/i2c-core-base.c#L495)
>> set `enable_wake = false`. This would keep wakes enabled by default on
>> device tree and disabled for ACPI. This matches the original behavior
>> before my patch.
> 
> I don't think it's a good idea to make the behaviour different. Drivers
> in general do not need to know whether the device was enumerated on ACPI
> or DT or whatnot. Same goes for users who should expect similar
> behaviour on the same device.
> 
> I wonder what is the reason why I2C bus does this for all wake capable
> devices in the first place? Typically it should be up to the user to
> enable them not the opposite.