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

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

[Hans de Goede]

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@redhat.com>
---
 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

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

[Stanislaw Gruszka]

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@redhat.com>

The problem explanation and the fix looks good to me.

Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

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
> 

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

[Hans de Goede]

Hi Stanislaw,

On 17-Feb-25 09:53, Stanislaw Gruszka wrote:
> Hi Hans,
> 
> Thanks for working on this, this issue annoyed lots of people.

You're welcome. Note I'm not convinced that this actually fixes all cases
of the vsc-tp -ETIMEOUT error. Normally the GPIO part of the LJCA USB-io
expander will become available before the SPI part and then we do not hit
this.

Could still be a rare race where we do hit this though.

Also as I mentioned in the bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=2316918

If I force hitting the race my XPS 9320 recovers after a simple reboot,
where as for other users the VSC seems to get in some stuck state.

Anyways lets continue discussing this in:
https://bugzilla.redhat.com/show_bug.cgi?id=2316918

where I have also written down some more observations from my ivsc debugging
session.

Regards,

Hans



> 
> 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@redhat.com>
> 
> The problem explanation and the fix looks good to me.
> 
> Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> 
> 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
>>
> 

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

[Hans de Goede]

Hi all,

On 17-Feb-25 10:06, Hans de Goede wrote:
> Hi Stanislaw,
> 
> On 17-Feb-25 09:53, Stanislaw Gruszka wrote:
>> Hi Hans,
>>
>> Thanks for working on this, this issue annoyed lots of people.
> 
> You're welcome. Note I'm not convinced that this actually fixes all cases
> of the vsc-tp -ETIMEOUT error. Normally the GPIO part of the LJCA USB-io
> expander will become available before the SPI part and then we do not hit
> this.

p.s.

Greg, to be clear this patch is correct and fixed a real bug so please
apply it, preferably as a fix for the current cycle. Also please add:

Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
Cc: stable@vger.kernel.org

Regards,

Hans





> Could still be a rare race where we do hit this though.
> 
> Also as I mentioned in the bugzilla:
> https://bugzilla.redhat.com/show_bug.cgi?id=2316918
> 
> If I force hitting the race my XPS 9320 recovers after a simple reboot,
> where as for other users the VSC seems to get in some stuck state.
> 
> Anyways lets continue discussing this in:
> https://bugzilla.redhat.com/show_bug.cgi?id=2316918
> 
> where I have also written down some more observations from my ivsc debugging
> session.
> 
> Regards,
> 
> Hans
> 
> 
> 
>>
>> 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@redhat.com>
>>
>> The problem explanation and the fix looks good to me.
>>
>> Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
>>
>> 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
>>>
>>
> 

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

[Sakari Ailus]

Hi Hans,

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@redhat.com>

Great detective work! :-)

Tested-by: Sakari Ailus <sakari.ailus@linux.intel.com>

This was on Arch Linux where I've seen this only, never on Debian.

-- 
Regards,

Sakari Ailus