Re: [PATCH v3 5/6] HID: steam: Clean up locking

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Recursive spinlock acquisition causes an immediate deadlock when processing battery events.
--

--- Patch [5]: [PATCH v3 5/6] HID: steam: Clean up locking ---
commit d43242604b3382e36db4c7f009a0ddce883ed333
Author: Vicki Pfau <[email protected]>

HID: steam: Clean up locking
    
This cleans up several issues with locking behavior, including RCU accesses
not being guarded behind a lock.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index d95296b28f92b..0e0ccf263416f 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -2230,8 +2234,6 @@ static void steam_do_deck_sensors_event(struct steam_device *steam,
>  static void steam_do_battery_event(struct steam_device *steam,
>  		struct power_supply *battery, u8 *data)
>  {
> -	unsigned long flags;
> -
>  	s16 volts = steam_le16(data + 12);
>  	u8 batt = data[14];
>  
> @@ -2239,10 +2241,9 @@ static void steam_do_battery_event(struct steam_device *steam,
>  	rcu_read_lock();
>  	battery = rcu_dereference(steam->battery);
>  	if (likely(battery)) {
> -		spin_lock_irqsave(&steam->lock, flags);
> +		guard(spinlock_irqsave)(&steam->lock);

[Severity: High]
Does this code cause a recursive spinlock deadlock?

steam_raw_event() now acquires steam->lock for the entire function scope and
calls steam_do_battery_event().

When a battery event is received, we'll see this call chain:

steam_raw_event()
   guard(spinlock_irqsave)(&steam->lock);
   ...
   steam_do_battery_event()
       guard(spinlock_irqsave)(&steam->lock); <-- deadlock

>  		steam->battery_voltage = volts;
>  		steam->battery_charge = batt;
> -		spin_unlock_irqrestore(&steam->lock, flags);
>  		power_supply_changed(battery);
>  	}
>  	rcu_read_unlock();
[ ... ]
> @@ -2427,15 +2428,13 @@ static void steam_do_ibex_battery_event(struct steam_device *steam,
>  		struct power_supply *battery,
>  		const struct steam_ibex_battery_status *data)
>  {
> -	unsigned long flags;
> -
>  	/* Creating the battery may have failed */
>  	guard(rcu)();
>  	battery = rcu_dereference(steam->battery);
>  	if (!likely(battery))
>  		return;
>  
> -	spin_lock_irqsave(&steam->lock, flags);
> +	guard(spinlock_irqsave)(&steam->lock);

[Severity: High]
Can this also cause a deadlock for the same reason?

steam_raw_event()
   guard(spinlock_irqsave)(&steam->lock);
   ...
   steam_do_ibex_battery_event()
       guard(spinlock_irqsave)(&steam->lock); <-- deadlock

>  	steam->battery_voltage = get_unaligned_le16(&data->battery_voltage);
>  	steam->battery_current = get_unaligned_le16(&data->battery_current);
>  	steam->battery_temp = get_unaligned_le16(&data->temperature);
[ ... ]
> @@ -2470,6 +2468,7 @@ static int steam_raw_event(struct hid_device *hdev,
>  	if (!steam)
>  		return 0;
>  
> +	guard(spinlock_irqsave)(&steam->lock);
>  	if (steam->client_opened)
>  		hid_input_report(steam->client_hdev, report->type, data, size, 0);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5