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

From: Friedrich Vock <friedrich.vock@gmx.de>
To: "Limonciello, Mario" <mario.limonciello@amd.com>,
	linux-input@vger.kernel.org, "Natikar,
	Basavaraj" <Basavaraj.Natikar@amd.com>,
	"S-k, Shyam-sundar" <Shyam-sundar.S-k@amd.com>
Cc: Jiri Kosina <jikos@kernel.org>,
	Benjamin Tissoires <benjamin.tissoires@redhat.com>
Subject: Re: [PATCH] HID: i2c-hid: Block a rogue device on ASUS TUF A16
Date: Thu, 15 Jun 2023 14:41:53 +0200	[thread overview]
Message-ID: <7b2ec797-2fdd-9707-7f53-d0a24ed036fc@gmx.de> (raw)
In-Reply-To: <dc4da9cf-a66f-6e09-165d-f16a405f2a38@amd.com>

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

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