Re: [PATCH] HID: i2c-hid: Block a rogue device on ASUS TUF A16

["Limonciello, Mario" <mario.limonciello@amd.com>]

On 6/27/2023 09:38, Friedrich Vock wrote:
> Hi,
> 
> On 26.06.23 17:10, Limonciello, Mario wrote:
>>
>> On 6/18/2023 10:05 PM, Mario Limonciello wrote:
>>> On 6/15/23 07:41, Friedrich Vock wrote:
>>>> Hi,
>>>>
>>>> sorry for taking so long to reply.
>>>>
>>>> On 02.06.23 20:43, Limonciello, Mario wrote:
>>>>> + some AMD guys
>>>>>
>>>>> On 5/30/2023 10:40 AM, Friedrich Vock wrote:
>>>>>> On these laptops, there seems to be a device that, when probed by
>>>>>> i2c-hid, constantly sends bogus interrupts and interferes with the
>>>>>> keyboard controller. When the device is enabled, it takes the
>>>>>> keyboard
>>>>>> around 8 seconds to register that keys are being pressed or released.
>>>>>
>>>>> Do you know what interrupt is firing constantly?
>>>>> Presumably it is the GPIO controller master interrupt, right?
>>>>> And it's for GPIO 7 (guessed from acpidump on one of the bug
>>>>> reports).
>>>>>
>>>>> To confirm check /proc/interrupts.
>>>> Seems likely that you guessed correctly. The corresponsing line in
>>>> /proc/interrupts (with the interrupts counts omitted):
>>>> 71:   amd_gpio    7  ITE5570:00
>>>
>>> OK.
>> I had asked in the past for R/W everything output to compare to
>> /sys/kernel/debug/gpio.
>>
>> I've added more explicit instructions how to get this to
>> the kernel bugzilla 217336 – keyboard not working Asus TUF FA617NS
>> (kernel.org) <https://bugzilla.kernel.org/show_bug.cgi?id=217336#c126>
> Thanks for this. R/W everything didn't work for the other people with
> the same models, so I spent this day getting Windows and R/W everything
> running myself. I managed to make it run and left a comment with the
> results in that bugzilla report.
>>>
>>>>>
>>>>> If it's not obvious which GPIO is firing there is also a dynamic
>>>>> debug statement in pinctrl-amd.c that may be helpful to figure
>>>>> this out.
>>>>>
>>>>> I would also suspect in Windows this doesn't happen.  If possible
>>>>> can you confirm that? Check in Device Manager what driver is bound
>>>>> to this device. Is it "inbox" from Microsoft or is it an ASUS
>>>>> specific driver?
>>>>>
>>>>> I wonder if the GPIO controller got programmed differently in
>>>>> Windows for some reason. We may want to confirm the values for
>>>>> GPIO registers from /sys/kernel/debug/gpio in Linux against those
>>>>> that are programmed in Windows.
>>>>>
>>>>> This can be accomplished using R/W everything in Windows.
>>>>
>>>> Unfortunately I don't have Windows installed on this system. I know
>>>> some
>>>> people with the Ryzen 9 7940HS model which might, I'll ask them if they
>>>> can give me the configuration on Windows and Linux.
>>>
>>> OK, I suggest directing everyone over to the bugs I linked and we
>>> should gather the relevant GPIO controller register dumps from
>>> Windows and Linux on the same hardware there.
>>>
>>>>
>>>>>
>>>>>>
>>>>>> Nothing I tried seemed to make the device work, and preventing the
>>>>>> device from being probed doesn't seem to break any functionality of
>>>>>> the laptop.
>>>>>>
>>>>>> Signed-off-by: Friedrich Vock <friedrich.vock@gmx.de>
>>>>>
>>>>> There are a few bug reports that popped up around this issue that
>>>>> should
>>>>> probably also be tagged.
>>>>>
>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217336
>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217493
>>>>>
>>>>>> ---
>>>>>>   drivers/hid/i2c-hid/i2c-hid-core.c       |  5 +++
>>>>>>   drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c | 48
>>>>>> ++++++++++++++++++++++++
>>>>>>   drivers/hid/i2c-hid/i2c-hid.h            |  3 ++
>>>>>>   3 files changed, 56 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c
>>>>>> b/drivers/hid/i2c-hid/i2c-hid-core.c
>>>>>> index efbba0465eef..5f0686d058df 100644
>>>>>> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
>>>>>> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
>>>>>> @@ -1035,6 +1035,11 @@ int i2c_hid_core_probe(struct i2c_client
>>>>>> *client, struct i2chid_ops *ops,
>>>>>>
>>>>>>       ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
>>>>>>
>>>>>> +    if (i2c_hid_device_blocked(hid->vendor, hid->product)) {
>>>>>> +        ret = -ENODEV;
>>>>>> +        goto err_irq;
>>>>>> +    }
>>>>>> +
>>>>> The thing I worry about here is that an unserviced interrupt can
>>>>> prevent the
>>>>> hardware from going into proper low power states; particularly at
>>>>> runtime.
>>>>>
>>>>> I think we should better understand what's going on before going down
>>>>> this
>>>>> path of just ignoring it.
>>>> Yeah, I guess I should've searched more for a proper explanation/fix
>>>> before submitting hacks like this. Let's see if this can be fixed in a
>>>> cleaner manner than preemptively disabling parts of the system.
>>>
>>> Can you please have a try with linux-next or apply this series:
>>> https://lore.kernel.org/linux-gpio/20230421120625.3366-1-mario.limonciello@amd.com/
>>>
>>>
>>> That does change some of the configuration for the GPIO controller
>>> registers to align how windows is handling debouncing.
>>>
>>> I don't think it will change the outcome of your bug, but I'd love
>>> to be surprised.
>> Any updates for this?
> Tried this out today. You won't be surprised, it didn't change anything.
>>>
>>>>>>       ret = hid_add_device(hid);
>>>>>>       if (ret) {
>>>>>>           if (ret != -ENODEV)
>>>>>> diff --git a/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
>>>>>> b/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
>>>>>> index 210f17c3a0be..d2c2806b64b4 100644
>>>>>> --- a/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
>>>>>> +++ b/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
>>>>>> @@ -440,6 +440,38 @@ static const struct dmi_system_id
>>>>>> i2c_hid_dmi_quirk_table[] = {
>>>>>>       { }    /* Terminate list */
>>>>>>   };
>>>>>>
>>>>>> +static const struct hid_device_id i2c_hid_blocked_ite_device = {
>>>>>> +    HID_DEVICE(BUS_I2C, HID_GROUP_GENERIC, USB_VENDOR_ID_ITE,
>>>>>> 0x8051)
>>>>>> +};
>>>>>> +
>>>>>> +/*
>>>>>> + * This list contains devices that can't be activated at all, for
>>>>>> example
>>>>>> + * because activating them breaks other important parts of the
>>>>>> system.
>>>>>> + */
>>>>>> +static const struct dmi_system_id i2c_hid_dmi_block_table[] = {
>>>>>> +    /*
>>>>>> +     * On ASUS TUF Gaming A16 laptops, there is a device that will
>>>>>> make the
>>>>>> +     * keyboard controller stop working correctly and flood the CPU
>>>>>> with bogus
>>>>>> +     * interrupts.
>>>>>> +     */
>>>>>> +    {
>>>>>> +        .ident = "ASUS TUF Gaming A16 (Ryzen 7 7735HS)",
>>>>>> +        .matches = {
>>>>>> +            DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER
>>>>>> INC."),
>>>>>> +            DMI_MATCH(DMI_PRODUCT_NAME, "FA617NS"),
>>>>>> +        },
>>>>>> +        .driver_data = (void *)&i2c_hid_blocked_ite_device,
>>>>>> +    },
>>>>>> +    {
>>>>>> +        .ident = "ASUS TUF Gaming A16 (Ryzen 9 7940HS)",
>>>>>> +        .matches = {
>>>>>> +            DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER
>>>>>> INC."),
>>>>>> +            DMI_MATCH(DMI_PRODUCT_NAME, "FA617XS"),
>>>>>> +        },
>>>>>> +        .driver_data = (void *)&i2c_hid_blocked_ite_device,
>>>>>> +    },
>>>>>> +    { }    /* Terminate list */
>>>>> If this *does* end up being the best solution, I think it's better
>>>>> to patch in the DMI to gpiolib-acpi.c similar to other quirks for
>>>>> floating
>>>>> GPIOs.  Example:
>>>>>
>>>>> https://github.com/torvalds/linux/blob/master/drivers/gpio/gpiolib-acpi.c#L1654
>>>>>
>>>>>
>>>>>
>>>>>> +};
>>>>>>
>>>>>>   struct i2c_hid_desc *i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t
>>>>>> *i2c_name)
>>>>>>   {
>>>>>> @@ -492,3 +524,19 @@ u32 i2c_hid_get_dmi_quirks(const u16 vendor,
>>>>>> const u16 product)
>>>>>>
>>>>>>       return quirks;
>>>>>>   }
>>>>>> +
>>>>>> +bool i2c_hid_device_blocked(const u16 vendor, const u16 product)
>>>>>> +{
>>>>>> +    const struct dmi_system_id *system_id =
>>>>>> +            dmi_first_match(i2c_hid_dmi_block_table);
>>>>>> +
>>>>>> +    if (system_id) {
>>>>>> +        const struct hid_device_id *device_id =
>>>>>> +                (struct hid_device_id *)(system_id->driver_data);
>>>>>> +
>>>>>> +        if (device_id && device_id->vendor == vendor &&
>>>>>> +            device_id->product == product)
>>>>>> +            return true;
>>>>>> +    }
>>>>>> +    return false;
>>>>>> +}
>>>>>> diff --git a/drivers/hid/i2c-hid/i2c-hid.h
>>>>>> b/drivers/hid/i2c-hid/i2c-hid.h
>>>>>> index 2c7b66d5caa0..e17bdd758f39 100644
>>>>>> --- a/drivers/hid/i2c-hid/i2c-hid.h
>>>>>> +++ b/drivers/hid/i2c-hid/i2c-hid.h
>>>>>> @@ -10,6 +10,7 @@ struct i2c_hid_desc
>>>>>> *i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t *i2c_name);
>>>>>>   char *i2c_hid_get_dmi_hid_report_desc_override(uint8_t *i2c_name,
>>>>>>                              unsigned int *size);
>>>>>>   u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product);
>>>>>> +bool i2c_hid_device_blocked(const u16 vendor, const u16 product);
>>>>>>   #else
>>>>>>   static inline struct i2c_hid_desc
>>>>>> *i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t *i2c_name)
>>>>>> @@ -19,6 +20,8 @@ static inline char
>>>>>> *i2c_hid_get_dmi_hid_report_desc_override(uint8_t *i2c_name,
>>>>>>   { return NULL; }
>>>>>>   static inline u32 i2c_hid_get_dmi_quirks(const u16 vendor, const
>>>>>> u16 product)
>>>>>>   { return 0; }
>>>>>> +static inline bool i2c_hid_device_blocked(const u16 vendor, const
>>>>>> u16 product)
>>>>>> +{ return false; }
>>>>>>   #endif
>>>>>>
>>>>>>   /**
>>>>>> -- 
>>>>>> 2.40.1
>>>>>>
>>>>>>
>>>
> Thanks,
> Friedrich

Friderich and some people on CC are already aware of this, but mostly 
for Benjamin and Jiri I wanted to let you know that the additional 
register fetching comparing Windows and Linux allowed me to come up with 
a proper root cause.

This series has been sent out to fix the issue properly.

https://lore.kernel.org/linux-gpio/20230630194716.6497-1-mario.limonciello@amd.com/