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

From: "Limonciello, Mario" <mario.limonciello@amd.com>
To: Friedrich Vock <friedrich.vock@gmx.de>,
	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: Fri, 2 Jun 2023 13:43:22 -0500	[thread overview]
Message-ID: <dc4da9cf-a66f-6e09-165d-f16a405f2a38@amd.com> (raw)
In-Reply-To: <20230530154058.17594-1-friedrich.vock@gmx.de>

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

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.

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

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