Re: [PATCH v3] HID: steelseries: Add MSI Raider A18 HX A9WJG RGB support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Returning `-ENODEV` prevents `hid-generic` fallback, permanently breaking the keyboard on other laptops and breaking unsupported interfaces.
- [Low] Unused static function `steelseries_msi_rgb_free_buf` in the `#else` fallback block causes a compiler warning.
--

commit 6fad0ba2106ff9dc9bf3d9ad4063720dca63e6b1
Author: David Glushkov <david.glushkov@sntiq.com>

HID: steelseries: Add MSI Raider A18 HX A9WJG RGB support

This commit adds DMI-gated support for MSI Raider A18 HX A9WJG internal
SteelSeries USB HID devices. The driver exposes them as multicolor LED
class devices on USB interface 0.

> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c
> index f98435631aa18..f0ba59bb28f97 100644
> --- a/drivers/hid/hid-steelseries.c
> +++ b/drivers/hid/hid-steelseries.c
[ ... ]
> @@ -546,6 +546,188 @@ static bool steelseries_is_vendor_usage_page(struct hid_device *hdev, uint8_t us
[ ... ]
> +#else
> +static void steelseries_msi_rgb_free_buf(void *data)
> +{
> +	kfree(data);
> +}
> +
> +static int steelseries_msi_rgb_register(struct steelseries_device *sd)
> +{
> +	return -ENODEV;
> +}
> +#endif

[Severity: Low]
Is steelseries_msi_rgb_free_buf() ever called when
CONFIG_LEDS_CLASS_MULTICOLOR is disabled?

Since it is a static function, defining it without a caller in the #else
fallback block will trigger a -Wunused-function compiler warning.

> @@ -549,6 +749,12 @@ static int steelseries_probe(struct hid_device *hdev, const struct hid_device_id
>  	sd->hdev = hdev;
>  	sd->quirks = id->driver_data;
>  
> +	if (sd->quirks & STEELSERIES_MSI_RGB) {
> +		if (!dmi_check_system(steelseries_msi_rgb_dmi_table) ||
> +		    !steelseries_msi_rgb_is_interface0(hdev))
> +			return -ENODEV;
> +	}

[Severity: High]
Does returning -ENODEV here prevent the device from functioning as a standard
keyboard on other laptops or on unsupported interfaces?

Because the device's USB IDs are explicitly listed in the steelseries_devices[]
id_table, the HID core's fallback logic prevents hid-generic from binding to it.
Returning -ENODEV instead of clearing the quirk flag and continuing
initialization abandons the device entirely.

> +
>  	ret = hid_parse(hdev);
>  	if (ret)
>  		return ret;
[ ... ]
> @@ -565,12 +771,28 @@ static int steelseries_probe(struct hid_device *hdev, const struct hid_device_id
>  
>  	ret = hid_hw_open(hdev);
>  	if (ret)
> -		return ret;
> +		goto err_stop;
> +
> +	if (sd->quirks & STEELSERIES_MSI_RGB) {
> +		ret = steelseries_msi_rgb_register(sd);
> +		if (ret) {
> +			hid_err(hdev, "Failed to register MSI RGB LEDs: %d\n", ret);
> +			goto err_close;
> +		}
> +		return 0;
> +	}

[Severity: High]
Similarly, if LED registration fails (for example, out of memory, or if the LED
subsystem is disabled and returns -ENODEV), does returning an error here also
leave the device completely unbound?

>  
> -	if (steelseries_headset_battery_register(sd) < 0)
> +	if (sd->quirks & (STEELSERIES_ARCTIS_1 | STEELSERIES_ARCTIS_9) &&
> +	    steelseries_headset_battery_register(sd) < 0)
>  		hid_err(sd->hdev,
>  			"Failed to register battery for headset\n");
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260531225329.201327-1-david.glushkov@sntiq.com?part=1