Re: [PATCH v6 4/9] HID: steelseries: Refactor Arctis driver to use device_info framework

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Wireless status updates are incorrectly gated by battery registration success, causing a feature regression.
- [Medium] Relaxing the Arctis 9 battery report size check may cause spurious headset disconnects.

Pre-existing issues:
- [High] A race condition exists during initialization where `sd->battery` can temporarily hold an `ERR_PTR`, leading to a potential kernel panic if `raw_event` triggers concurrently.
--

--- Patch [4]: [PATCH v6 4/9] HID: steelseries: Refactor Arctis driver to use device_info framework ---
commit 83accfc4c1be2885884d77d1f5dcd98d620ce34b
Author: Sriman Achanta <srimanachanta@gmail.com>

HID: steelseries: Refactor Arctis driver to use device_info framework
    
Replace the per-product if/else quirk bitmap with a
steelseries_device_info struct. Each model provides its capabilities,
sync_interface, and request_status/parse_status callbacks. Report
sending is folded into steelseries_send_report() and the feature and
output wrappers, and the battery identifiers lose their per-model names.

> diff --git a/drivers/hid/hid-steelseries-arctis.c b/drivers/hid/hid-steelseries-arctis.c
> index 079504e6932a1..c54c56db9ddd7 100644
> --- a/drivers/hid/hid-steelseries-arctis.c
> +++ b/drivers/hid/hid-steelseries-arctis.c

[ ... ]

> @@ -88,30 +108,101 @@ static int battery_capacity_to_level(int capacity)
> +static void steelseries_arctis_9_parse_status(struct steelseries_device *sd,
> +					      u8 *data, int size)
> +{
> +	if (size < 5)
> +		return;
> +
> +	if (data[0] == 0xaa && data[1] == 0x01) {
> +		sd->headset_connected = true;
> +		sd->battery_charging = (data[4] == 0x01);
> +		sd->battery_capacity = steelseries_map_capacity(data[3], 0x68, 0x9d);
> +	} else {
> +		/* Device off: 0x55 (no status) or 0x03 (stale status). */
> +		sd->headset_connected = false;
> +		sd->battery_charging = false;
> +	}
> +}

[Severity: Medium]
Could this relaxed size check cause spurious disconnects?

The original code checked for ARCTIS_9_BATTERY_RESPONSE_LEN (64 bytes). By
relaxing the check to 5 bytes, if the headset sends any other non-battery
report between 5 and 63 bytes, it will bypass the filter, fail the signature
check, and fall into the else block.

Would that incorrectly mark the headset as disconnected?

[ ... ]

> @@ -168,7 +244,26 @@ static enum power_supply_property steelseries_headset_battery_props[] = {

[ ... ]

> +static int steelseries_battery_register(struct steelseries_device *sd)
>  {
>  	static atomic_t battery_no = ATOMIC_INIT(0);
>  	struct power_supply_config battery_cfg = { .drv_data = sd, };
> @@ -176,25 +271,27 @@ static int steelseries_headset_battery_register(struct steelseries_device *sd)
>  	int ret;

[ ... ]

>  	sd->battery = devm_power_supply_register(&sd->hdev->dev,
>  			&sd->battery_desc, &battery_cfg);
>  	if (IS_ERR(sd->battery)) {
>  		ret = PTR_ERR(sd->battery);
> +		sd->battery = NULL;
>  		hid_err(sd->hdev,
>  				"%s:power_supply_register failed with error %d\n",
>  				__func__, ret);

[Severity: High]
This is a pre-existing issue, but can this cause a kernel panic if a report
arrives concurrently?

Since hid_hw_start() is called earlier during probe, could raw_event()
trigger while sd->battery temporarily holds an ERR_PTR?

If raw_event() fires before sd->battery is set to NULL, it sees a non-NULL
pointer and passes it to power_supply_changed(), which might dereference the
error pointer and crash.

[ ... ]

> @@ -262,130 +365,92 @@ static void steelseries_arctis_remove(struct hid_device *hdev)

[ ... ]

>  static int steelseries_arctis_raw_event(struct hid_device *hdev,
> -					struct hid_report *report, u8 *read_buf,
> -					int size)
> +				 struct hid_report *report, u8 *data, int size)
>  {

[ ... ]

> -	if (connected != sd->headset_connected) {
> -		hid_dbg(sd->hdev,
> +	if (sd->headset_connected != old_connected) {
> +		hid_dbg(hdev,
>  			"Connected status changed from %sconnected to %sconnected\n",
> -			sd->headset_connected ? "" : "not ",
> -			connected ? "" : "not ");
> -		sd->headset_connected = connected;
> -		steelseries_headset_set_wireless_status(hdev, connected);
> +			old_connected ? "" : "not ",
> +			sd->headset_connected ? "" : "not ");
> +
> +		if (sd->battery) {
> +			steelseries_headset_set_wireless_status(sd->hdev,
> +							       sd->headset_connected);
> +			power_supply_changed(sd->battery);
> +		}
>  	}

[Severity: Medium]
Does this check unintentionally gate the wireless status update behind the
battery registration?

If battery registration fails (and sd->battery is NULL), it looks like the
USB device's wireless status will no longer be reported to userspace. Is
this expected behavior?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709032316.1908460-1-68172138+srimanachanta@users.noreply.github.com?part=4