Re: [PATCH v7 3/8] HID: steelseries: Refactor Arctis driver to use device_info framework

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Race condition exposing ERR_PTR leading to kernel panic in raw_event.
--

--- Patch [3]: [PATCH v7 3/8] HID: steelseries: Refactor Arctis driver to use device_info framework ---
commit e1b5fdcaec23ba5cc7a2a2139ba1c9fa983d7894
Author: Sriman Achanta <srimanachanta@gmail.com>

HID: steelseries: Refactor Arctis driver to use device_info framework

This commit refactors the Arctis driver to use a device_info struct
instead of a quirk bitmap for device capabilities. It also changes
battery polling to use a periodic delayed work instead of being
requested from raw_event.

> diff --git a/drivers/hid/hid-steelseries-arctis.c b/drivers/hid/hid-steelseries-arctis.c
> index 6cae2e00bd8ba..c54c56db9ddd7 100644
> --- a/drivers/hid/hid-steelseries-arctis.c
> +++ b/drivers/hid/hid-steelseries-arctis.c
[ ... ]
> @@ -177,25 +271,27 @@ static int steelseries_headset_battery_register(struct steelseries_device *sd)
[ ... ]
>  	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]
Could this assignment expose an ERR_PTR to a concurrent event handler?

Because hid_hw_start() and hid_hw_open() are called before 
steelseries_battery_register() in steelseries_arctis_probe(), USB interrupts 
can trigger steelseries_arctis_raw_event() concurrently.

If devm_power_supply_register() fails due to memory pressure or other errors, 
sd->battery will briefly hold an ERR_PTR before it is set to NULL. A 
concurrent execution of raw_event() evaluates if (sd->battery). Since an 
ERR_PTR is not NULL, the check passes, and power_supply_changed(sd->battery) 
will dereference the invalid pointer, causing a kernel panic.

Would it be safer to assign the return value to a local variable first, and 
only assign it to sd->battery upon success?

Additionally, does the unconditional assignment of sd->battery_capacity = 100 
just before registration risk silently overwriting a real status update 
received by raw_event() during this window?

[ ... ]
> @@ -272,142 +365,92 @@ static void steelseries_arctis_remove(struct hid_device *hdev)
[ ... ]
> -	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);
> +		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803191553.66368-1-srimanachanta@gmail.com?part=3