| Message ID | 20230530154058.17594-1-friedrich.vock@gmx.de (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: i2c-hid: Block a rogue device on ASUS TUF A16 | expand |
+ 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 > >
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 >> >>
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. >> >> 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. >>> 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 >>> >>>
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> > >>> >>> 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? > >>>> 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 >>>> >>>> >
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
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/
On Jun 30 2023, Limonciello, Mario wrote: [...] > > 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. Great, thanks for the heads up :) Cheers, Benjamin > > https://lore.kernel.org/linux-gpio/20230630194716.6497-1-mario.limonciello@amd.com/
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; + } + 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 */ +}; 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 /**
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. 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> --- 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(+) -- 2.40.1