Re: [PATCH v4 02/11] HID: asus: prevent binding to all HID devices on ROG

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Mon, 24 Mar 2025, Antheas Kapenekakis wrote:

> ROG keyboards are HID compliant and only care about the endpoint that
> produces vendor events (e.g., fan mode) and has the keyboard backlight.
> 
> Therefore, handle all of the endpoints of ROG keyboards as compliant,
> by adding HID_QUIRK_INPUT_PER_APP and, for devices other than the vendor
> one, by adding QUIRK_HANDLE_GENERIC to stop mutating them.
> 
> Due to HID_QUIRK_INPUT_PER_APP, rgb register is moved into probe, as
> the input_* functions are called multiple times (4 for the Z13).
> 
> Reviewed-by: Luke D. Jones <luke@ljones.dev>
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
>  drivers/hid/hid-asus.c | 69 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 53 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 8d4df1b6f143b..96461321c191c 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -84,6 +84,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>  #define QUIRK_MEDION_E1239T		BIT(10)
>  #define QUIRK_ROG_NKEY_KEYBOARD		BIT(11)
>  #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
> +#define QUIRK_HANDLE_GENERIC		BIT(13)
>  
>  #define I2C_KEYBOARD_QUIRKS			(QUIRK_FIX_NOTEBOOK_REPORT | \
>  						 QUIRK_NO_INIT_REPORTS | \
> @@ -120,7 +121,6 @@ struct asus_drvdata {
>  	struct input_dev *tp_kbd_input;
>  	struct asus_kbd_leds *kbd_backlight;
>  	const struct asus_touchpad_info *tp;
> -	bool enable_backlight;
>  	struct power_supply *battery;
>  	struct power_supply_desc battery_desc;
>  	int battery_capacity;
> @@ -326,6 +326,10 @@ static int asus_raw_event(struct hid_device *hdev,
>  {
>  	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>  
> +	if (drvdata->quirks & QUIRK_HANDLE_GENERIC)
> +		/* NOOP on generic HID devices to avoid side effects. */
> +		return 0;
> +
>  	if (drvdata->battery && data[0] == BATTERY_REPORT_ID)
>  		return asus_report_battery(drvdata, data, size);
>  
> @@ -774,6 +778,10 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>  	struct input_dev *input = hi->input;
>  	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>  
> +	if (drvdata->quirks & QUIRK_HANDLE_GENERIC)
> +		/* NOOP on generic HID devices to avoid side effects. */
> +		return 0;

You'd need braces for multiline indented constructs like this, however, I 
think that comment can appear before the if line which wouldn't require 
braces. The same applies to many cases below.

> +
>  	/* T100CHI uses MULTI_INPUT, bind the touchpad to the mouse hid_input */
>  	if (drvdata->quirks & QUIRK_T100CHI &&
>  	    hi->report->id != T100CHI_MOUSE_REPORT_ID)
> @@ -827,11 +835,6 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>  
>  	drvdata->input = input;
>  
> -	if (drvdata->enable_backlight &&
> -	    !asus_kbd_wmi_led_control_present(hdev) &&
> -	    asus_kbd_register_leds(hdev))
> -		hid_warn(hdev, "Failed to initialize backlight.\n");
> -
>  	return 0;
>  }
>  
> @@ -851,6 +854,10 @@ static int asus_input_mapping(struct hid_device *hdev,
>  		return -1;
>  	}
>  
> +	if (drvdata->quirks & QUIRK_HANDLE_GENERIC)
> +		/* NOOP on generic HID devices to avoid side effects. */
> +		return 0;
> +
>  	/*
>  	 * Ignore a bunch of bogus collections in the T100CHI descriptor.
>  	 * This avoids a bunch of non-functional hid_input devices getting
> @@ -901,15 +908,6 @@ static int asus_input_mapping(struct hid_device *hdev,
>  			return -1;
>  		}
>  
> -		/*
> -		 * Check and enable backlight only on devices with UsagePage ==
> -		 * 0xff31 to avoid initializing the keyboard firmware multiple
> -		 * times on devices with multiple HID descriptors but same
> -		 * PID/VID.
> -		 */
> -		if (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT)
> -			drvdata->enable_backlight = true;
> -
>  		set_bit(EV_REP, hi->input->evbit);
>  		return 1;
>  	}
> @@ -1026,8 +1024,10 @@ static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
>  
>  static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  {
> -	int ret;
> +	struct hid_report_enum *rep_enum;
>  	struct asus_drvdata *drvdata;
> +	struct hid_report *rep;
> +	int ret, is_vendor = 0;
>  
>  	drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
>  	if (drvdata == NULL) {
> @@ -1111,12 +1111,45 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		return ret;
>  	}
>  
> +	/*
> +	 * Check for the vendor interface (0xff31) to init the RGB.

The next line seems to be continuation, is . extra?

> +	 * and handle generic devices properly.
> +	 */
> +	rep_enum = &hdev->report_enum[HID_INPUT_REPORT];
> +	list_for_each_entry(rep, &rep_enum->report_list, list) {
> +		if ((rep->application & HID_USAGE_PAGE) == 0xff310000)

Use a named define for the literal?

> +			is_vendor = true;
> +	}
> +
> +	/*
> +	 * For ROG keyboards, make them hid compliant by
> +	 * creating one input per application. For interfaces other than
> +	 * the vendor one, disable hid-asus handlers.
> +	 */
> +	if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> +		if (!is_vendor)
> +			drvdata->quirks |= QUIRK_HANDLE_GENERIC;
> +		hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
> +	}
> +
>  	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>  	if (ret) {
>  		hid_err(hdev, "Asus hw start failed: %d\n", ret);
>  		return ret;
>  	}
>  
> +	if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
> +	    !asus_kbd_wmi_led_control_present(hdev) &&
> +	    asus_kbd_register_leds(hdev))
> +		hid_warn(hdev, "Failed to initialize backlight.\n");
> +
> +	/*
> +	 * For ROG keyboards, skip rename for consistency and
> +	 * ->input check as some devices do not have inputs.

Please reflow to 80 chars.

> +	 */
> +	if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD)
> +		return 0;
> +
>  	if (!drvdata->input) {
>  		hid_err(hdev, "Asus input not registered\n");
>  		ret = -ENOMEM;
> @@ -1167,6 +1200,10 @@ static const __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc,
>  {
>  	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>  
> +	if (drvdata->quirks & QUIRK_HANDLE_GENERIC)
> +		/* NOOP on generic HID devices to avoid side effects. */
> +		return rdesc;
> +
>  	if (drvdata->quirks & QUIRK_FIX_NOTEBOOK_REPORT &&
>  			*rsize >= 56 && rdesc[54] == 0x25 && rdesc[55] == 0x65) {
>  		hid_info(hdev, "Fixing up Asus notebook report descriptor\n");
> 

-- 
 i.