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

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

[Mika Westerberg]

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.

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.

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);
 
 		if (wakeirq > 0 && wakeirq != client->irq)
 			status = dev_pm_set_dedicated_wake_irq(dev, wakeirq);
-- 
2.39.1

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

[Andy Shevchenko]

On Tue, Feb 07, 2023 at 09:25:40AM +0200, Mika Westerberg 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.
> 
> 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.

I agree on the reasoning.

Should we have a Fixes tag?

Otherwise
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

In any case it would be nice if the initial contributors may have a chance
to test this and see how their setup is supposed to work.

> 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);
>  
>  		if (wakeirq > 0 && wakeirq != client->irq)
>  			status = dev_pm_set_dedicated_wake_irq(dev, wakeirq);
> -- 
> 2.39.1
> 

-- 
With Best Regards,
Andy Shevchenko

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

[Cezary Rojewski]

On 2023-02-07 11:33 AM, Andy Shevchenko wrote:
> On Tue, Feb 07, 2023 at 09:25:40AM +0200, Mika Westerberg 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.
>>
>> 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.
> 
> I agree on the reasoning.
> 
> Should we have a Fixes tag?
> 
> Otherwise
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> In any case it would be nice if the initial contributors may have a chance
> to test this and see how their setup is supposed to work.


Hello,

Thanks for the patch Mika and your input Andy. While neither I nor 
Amadeusz contributed to the initial change, we will be testing this one 
too - reverting the revert of the offending change that we had 
internally and applying this patch instead. Tests should be concluded by 
Thursday/Friday this week.


Regards,
Czarek

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

[Raul Rangel]

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.

>
> 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?

>
> 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.

>
>                 if (wakeirq > 0 && wakeirq != client->irq)
>                         status = dev_pm_set_dedicated_wake_irq(dev, wakeirq);
> --
> 2.39.1
>

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

[Mika Westerberg]

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 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.

> > 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.

> > 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.

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

[Amadeusz Sławiński]

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.

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

[Mika Westerberg]

Hi,

On Wed, Feb 08, 2023 at 09:28:14AM +0100, Amadeusz Sławiński wrote:
> 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"?

Yes, I think that should be enough.

> 
> > > > 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.

Thanks for the clarification!

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

[Amadeusz Sławiński]

On 2/8/2023 10:29 AM, Mika Westerberg wrote:
> Hi,
> 
> On Wed, Feb 08, 2023 at 09:28:14AM +0100, Amadeusz Sławiński wrote:
>> 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"?
> 
> Yes, I think that should be enough.
> 

And yes, it seems to work when I bump revision. So will use it as 
workaround for now and see about fixing BIOS.

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

[Mika Westerberg]

Hi,

On Thu, Feb 09, 2023 at 10:13:00AM +0100, Amadeusz Sławiński wrote:
> > > 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"?
> > 
> > Yes, I think that should be enough.
> 
> And yes, it seems to work when I bump revision. So will use it as workaround
> for now and see about fixing BIOS.

Okay good to know, then I think we can forget this patch.

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 120 bytes --]


> Okay good to know, then I think we can forget this patch.

Based on this comment, I marked the patch as "Rejected".


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Raul Rangel]

On Wed, Feb 8, 2023 at 1:28 AM Amadeusz Sławiński
<amadeuszx.slawinski@linux.intel.com> wrote:
>
> 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.

You also need the following patch
https://github.com/torvalds/linux/commit/0e3b175f079247f0d40d2ab695999c309d3a7498,
otherwise the ignore flag only applies to _AEI GPIOs.

>
> >
> > 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.

IMO if a device needs to support wake from S3 the ACPI table needs to
define a _PRW and define the proper power resources to keep the device
functional during S3.

>
> >
> >>> 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.
>

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

[Mario Limonciello]

On 2/8/23 09:58, Raul Rangel wrote:
> On Wed, Feb 8, 2023 at 1:28 AM Amadeusz Sławiński
> <amadeuszx.slawinski@linux.intel.com> wrote:
>>
>> 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.
> 
> You also need the following patch
> https://github.com/torvalds/linux/commit/0e3b175f079247f0d40d2ab695999c309d3a7498,
> otherwise the ignore flag only applies to _AEI GPIOs.
> 

I want to point out a non-obvious nuance to that as well.  When it's not 
triggered by an _AEI GPIO then it will not have the parent controller 
for the quirk but rather the string used for the ACPI device that 
declared the GPIO.

That's why the quirk that we applied for the Clevo BIOS bug was not 
AMDI0030 but was instead the string used for the ACPI device.

So if you're experimenting with this make sure you're using the right 
values, and explicitly look for the string in your kernel log that it's 
in use.

>>
>>>
>>> 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.
> 
> IMO if a device needs to support wake from S3 the ACPI table needs to
> define a _PRW and define the proper power resources to keep the device
> functional during S3.
> 
>>
>>>
>>>>> 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.
>>

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

[Rafael J. Wysocki]

On Wed, Feb 8, 2023 at 4:59 PM Raul Rangel <rrangel@chromium.org> wrote:
>
> On Wed, Feb 8, 2023 at 1:28 AM Amadeusz Sławiński
> <amadeuszx.slawinski@linux.intel.com> wrote:
> >
> > 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.
>
> You also need the following patch
> https://github.com/torvalds/linux/commit/0e3b175f079247f0d40d2ab695999c309d3a7498,
> otherwise the ignore flag only applies to _AEI GPIOs.
>
> >
> > >
> > > 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.
>
> IMO if a device needs to support wake from S3 the ACPI table needs to
> define a _PRW and define the proper power resources to keep the device
> functional during S3.

Yes, it should, but there's more to it than this.

First of all, the interrupt in question needs to be enabled prior to
S3 entry.  That will happen if device_may_wakeup() is true for the
given device AFAICS.

Second, the device should be in a suitable power state which _S3W is
about.  That should also happen via ACPI PM.

Next, _PRW should return the list of power resources that need to be
_ON prior to S3 entry for the device to be able to wake the system.

However, _PRW also provides the information on which sleep states the
system can be woken up from by the given device and S3 need not be one
of them.

The kernel will not prevent S3 from being entered if that is the case,
so it doesn't follow the spec literally in that respect.