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@xxxxxxxxxxxxxxx

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