linux-kernel - Re: [PATCH v2 07/13] i2c: acpi: Use ACPI wake capability bit to set wake

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHQZ30Dsk2ikhctYSk_eP=1qcOOn_tjgtCftPOqQFkHNfQwBsg@mail.gmail.com>
Date:   Wed, 14 Sep 2022 12:33:17 -0600
From:   Raul Rangel <rrangel@...omium.org>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Linux ACPI <linux-acpi@...r.kernel.org>,
        linux-input <linux-input@...r.kernel.org>,
        "jingle.wu" <jingle.wu@....com.tw>,
        "Limonciello, Mario" <mario.limonciello@....com>,
        Tim Van Patten <timvp@...gle.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Hans de Goede <hdegoede@...hat.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Wolfram Sang <wsa@...nel.org>,
        "open list:I2C SUBSYSTEM HOST DRIVERS" <linux-i2c@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 07/13] i2c: acpi: Use ACPI wake capability bit to set wake_irq

> > > This is similar to what of_i2c_get_board_info() does, no?
> > > Note: _get_ there.
> >
> > `*info` is an out parameter in that case. Ideally I would have
> > `i2c_acpi_get_irq`, `acpi_dev_gpio_irq_get_wake`,
> > `platform_get_irq_optional`, and `i2c_dev_irq_from_resources` all
> > return a `struct irq_info {int irq; bool wake_capable;}`. This would
> > be a larger change though.
>
> Seems the ACPI analogue is i2c_acpi_fill_info(). Can we do it there?
>

So I originally had that thought, but decided against it to avoid
changing too many things,
but since you brought it up, I thought I would try it.

So I moved the GPIO lookup into `i2c_acpi_do_lookup`, but it failed
spectacularly.
I've linked some logs of both cases. grep for `RX:` to see my logging messages.

* https://0paste.com/393416 - Logs with IRQ lookup happening in
`i2c_acpi_do_lookup`
    * We can see that `i2c_acpi_do_lookup` gets called in three cases
         1) Early on from i2c_acpi_notify when the I2C ACPI nodes are
first created
         2) From `i2c_dw_adjust_bus_speed` as part of `dw_i2c_plat_probe`
         3) From `i2c_register_adapter` as part of `i2c_dw_probe_master`.
    * What happens is that all of these calls happen before the GPIO
chip has been registered.
      This means that `acpi_dev_gpio_irq_get` will return `-EPROBE_DEFER`. This
      messes something up in the i2c init sequence and the devices are never
      probed again.
    * You can see the `amd gpio driver loaded` message after all the
i2c probing.
* https://0paste.com/393420 - Logs of a normal boot
    * Here we can see the GPIO controller registers early
    * We can see the i2c devices being probed by `__driver_attach_async_helper`.
      I'm guessing the device was enqueued as part of `i2c_acpi_register_device`
      early on and it gets probed later.

I could try moving the gpio lookup into `i2c_acpi_get_info`, but I
think that suffers from the
same problem, the stack can't handle a PROBE_DEFER. So I think we need to keep
the lookup in `i2c_device_probe` for the PROBE_DEFER logic to work correctly.