Re: [PATCH] mei: vsc: Use "wakeuphostint" when getting the host wakeup GPIO

From: Stanislaw Gruszka
Date: Mon Feb 17 2025 - 03:54:21 EST


Hi Hans,

Thanks for working on this, this issue annoyed lots of people.

On Fri, Feb 14, 2025 at 10:24:25PM +0100, Hans de Goede wrote:
> The _CRS ACPI resources table has 2 entries for the host wakeup GPIO,
> the first one being a regular GpioIo () resource while the second one
> is a GpioInt () resource for the same pin.
>
> The acpi_gpio_mapping table used by vsc-tp.c maps the first Gpio ()
> resource to "wakeuphost-gpios" where as the second GpioInt () entry
> is mapped to "wakeuphostint-gpios".
>
> Using "wakeuphost" to request the GPIO as was done until now, means
> that the gpiolib-acpi code does not know that the GPIO is active-low
> as that info is only available in the GpioInt () entry.
>
> Things were still working before due to the following happening:
>
> 1. Since the 2 entries point to the same pin they share a struct gpio_desc
> 2. The SPI core creates the SPI device vsc-tp.c binds to and calls
> acpi_dev_gpio_irq_get(). This does use the second entry and sets
> FLAG_ACTIVE_LOW in gpio_desc.flags .
> 3. vsc_tp_probe() requests the "wakeuphost" GPIO and inherits the
> active-low flag set by acpi_dev_gpio_irq_get()
>
> But there is a possible scenario where things do not work:
>
> 1. - 3. happen as above
> 4. After requesting the "wakeuphost" GPIO, the "resetfw" GPIO is requested
> next, but its USB GPIO controller is not available yet, so this call
> returns -EPROBE_DEFER.
> 5. The gpio_desc for "wakeuphost" is put() and during this the active-low
> flag is cleared from gpio_desc.flags .
> 6. Later on vsc_tp_probe() requests the "wakeuphost" GPIO again, but now it
> is not marked active-low.
>
> The difference can also be seen in /sys/kernel/debug/gpio, which contains
> the following line for this GPIO:
>
> gpio-535 ( |wakeuphost ) in hi IRQ ACTIVE LOW
>
> If the second scenario is hit the "ACTIVE LOW" at the end disappears and
> things do not work.
>
> Fix this by requesting the GPIO through the "wakeuphostint" mapping instead
> which provides active-low info without relying on acpi_dev_gpio_irq_get()
> pre-populating this info in the gpio_desc.
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2316918
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>

The problem explanation and the fix looks good to me.

Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@xxxxxxxxxxxxxxx>

Regards
Stanislaw

> ---
> drivers/misc/mei/vsc-tp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
> index 35d349fee769..7be1649b1972 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -502,7 +502,7 @@ static int vsc_tp_probe(struct spi_device *spi)
> if (ret)
> return ret;
>
> - tp->wakeuphost = devm_gpiod_get(dev, "wakeuphost", GPIOD_IN);
> + tp->wakeuphost = devm_gpiod_get(dev, "wakeuphostint", GPIOD_IN);
> if (IS_ERR(tp->wakeuphost))
> return PTR_ERR(tp->wakeuphost);
>
> --
> 2.48.1
>