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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Guaranteed self-deadlock during device registration and unregistration due to recursive acquisition of `steam->report_mutex`.
--

--- Patch [5]: [PATCH v2 5/6] HID: steam: Clean up locking ---
commit f00f469b42e54055547cfa7401a79f8895766441
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 8c97302c35deb..01801f2a58864 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -921,9 +920,10 @@ static int steam_input_open(struct input_dev *dev)
>  	 */
>  	if (!(steam->quirks & (STEAM_QUIRK_DECK | STEAM_QUIRK_IBEX))) {
>  		spin_lock_irqsave(&steam->lock, flags);
> -		set_lizard_mode = !steam->client_opened && lizard_mode;
> +		client_opened = steam->client_opened;
>  		spin_unlock_irqrestore(&steam->lock, flags);
> -		if (set_lizard_mode)
> +		guard(mutex)(&steam->report_mutex);

[Severity: High]
Does this create a self-deadlock during device registration?

When steam_register() is called, it now holds steam->report_mutex. It then
calls steam_input_register(), which registers the input device. If an input
handler (like ledtrig-input-events) is present, the input core will
synchronously open the device, calling steam_input_open().

The call chain would look like:

steam_register()
  mutex_lock(&steam->report_mutex);
  steam_input_register(steam);
    input_register_device()
      ... (input core handler connect)
      input_open_device()
        steam_input_open()
          guard(mutex)(&steam->report_mutex);  <-- deadlocks here

The same sequence applies during steam_unregister() when it calls
steam_input_close() and steam_sensor_close().

> +		if (!client_opened && lizard_mode)
>  			steam_set_lizard_mode(steam, false);
>  	}
>  
[ ... ]
> @@ -1379,14 +1374,17 @@ static int steam_register(struct steam_device *steam)
>  {
>  	int ret;
>  
> +	mutex_lock(&steam->report_mutex);
>  	/*
>  	 * This function can be called several times in a row with the
>  	 * wireless adaptor, without steam_unregister() between them, because
[ ... ]
> @@ -1439,9 +1439,11 @@ static void steam_unregister(struct steam_device *steam)
>  	hid_info(steam->hdev, "Steam %s '%s' disconnected",
>  			steam->quirks & STEAM_QUIRK_DECK ? "Deck" : "Controller",
>  			steam->serial_no);
> +	mutex_lock(&steam->report_mutex);
>  	steam_battery_unregister(steam);
>  	steam_sensors_unregister(steam);
>  	steam_input_unregister(steam);
> +	mutex_unlock(&steam->report_mutex);

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