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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing memory barrier when publishing `sd->battery_registered`, leading to a potential race condition and kernel panic in `raw_event`.
--

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

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

This patch adds DMI-gated support for internal SteelSeries USB HID devices
for RGB lighting on the MSI Raider A18 HX A9WJG laptop. It exposes them as
multicolor LED class devices.

Link: https://lore.kernel.org/oe-kbuild-all/202606010709.X0QYNjFZ-lkp@intel.com/

> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c
> --- a/drivers/hid/hid-steelseries.c
> +++ b/drivers/hid/hid-steelseries.c
> @@ -509,6 +528,7 @@ static int steelseries_headset_battery_register(struct steelseries_device *sd)
>  	power_supply_powers(sd->battery, &sd->hdev->dev);
>  
>  	INIT_DELAYED_WORK(&sd->battery_work, steelseries_headset_battery_timer_tick);
> +	sd->battery_registered = true;

[Severity: High]
Since hid_hw_start() is called earlier in the steelseries_probe() sequence,
could the store to sd->battery_registered be reordered before the
initialization of sd->battery_work by the compiler or on weakly-ordered CPUs?

If this reordering occurs, the interrupt-driven steelseries_headset_raw_event
handler might locklessly observe battery_registered as true prematurely. This
would lead to schedule_delayed_work() being called on a partially initialized
work_struct, which could cause a kernel panic in the timer core.

Would it be safer to publish this flag using smp_store_release() to ensure
the delayed work initialization is fully visible before the flag is set?

>  	steelseries_headset_fetch_battery(sd->hdev);
>  
>  	if (sd->quirks & STEELSERIES_ARCTIS_9) {

[ ... ]

> @@ -855,6 +885,9 @@ static int steelseries_headset_raw_event(struct hid_device *hdev,
>  	if (!steelseries_is_headset(hdev))
>  		return 0;
>  
> +	sd = hid_get_drvdata(hdev);
> +	if (!sd || !sd->battery_registered)
> +		return 0;
> +
>  	capacity = sd->battery_capacity;

[Severity: High]
To safely pair with the suggested smp_store_release() in the initialization
path, should this lockless read of sd->battery_registered be done using
smp_load_acquire()?

This check happens directly in the raw_event handler, so it needs memory
barriers to guarantee that if it reads true, the fully initialized work_struct
is visible to this CPU before we subsequently call schedule_delayed_work() on
it later in this function.

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