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

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: <8c626c45-cab0-405f-8418-f36883fc7a38@redhat.com>
Date: Mon, 17 Feb 2025 10:06:49 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: Stanislaw Gruszka <stanislaw.gruszka@...ux.intel.com>
Cc: Wentong Wu <wentong.wu@...el.com>, Jason Chen <jason.z.chen@...el.com>,
 Alexander Usyskin <alexander.usyskin@...el.com>,
 Arnd Bergmann <arnd@...db.de>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mei: vsc: Use "wakeuphostint" when getting the host
 wakeup GPIO

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