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

[Mika Westerberg <mika.westerberg@linux.intel.com>]

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!