Re: [PATCH 1/3] hid-steam: Move hidraw input (un)registering to work

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Vicki,

On Tue, Feb 04, 2025 at 07:55:27PM -0800, Vicki Pfau wrote:
> Due to an interplay between locking in the input and hid transport subsystems,
> attempting to register or deregister the relevant input devices during the
> hidraw open/close events can lead to a lock ordering issue. Though this
> shouldn't cause a deadlock, this commit moves the input device manipulation to
> deferred work to sidestep the issue.
> 
> Signed-off-by: Vicki Pfau <vi@endrift.com>
> ---
>  drivers/hid/hid-steam.c | 38 +++++++++++++++++++++++++++++++-------
>  1 file changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index af38fc8eb34f..395dbe642f00 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -313,6 +313,7 @@ struct steam_device {
>  	u16 rumble_left;
>  	u16 rumble_right;
>  	unsigned int sensor_timestamp_us;
> +	struct work_struct unregister_work;
>  };
>  
>  static int steam_recv_report(struct steam_device *steam,
> @@ -1072,6 +1073,31 @@ static void steam_mode_switch_cb(struct work_struct *work)
>  	}
>  }
>  
> +static void steam_work_unregister_cb(struct work_struct *work)
> +{
> +	struct steam_device *steam = container_of(work, struct steam_device,
> +							unregister_work);
> +	unsigned long flags;
> +	bool connected;
> +	bool opened;
> +
> +	spin_lock_irqsave(&steam->lock, flags);
> +	opened = steam->client_opened;
> +	connected = steam->connected;
> +	spin_unlock_irqrestore(&steam->lock, flags);
> +
> +	if (connected) {
> +		if (opened) {
> +			steam_sensors_unregister(steam);
> +			steam_input_unregister(steam);
> +		} else {
> +			steam_set_lizard_mode(steam, lizard_mode);
> +			steam_input_register(steam);
> +			steam_sensors_register(steam);
> +		}
> +	}
> +}
> +
>  static bool steam_is_valve_interface(struct hid_device *hdev)
>  {
>  	struct hid_report_enum *rep_enum;
> @@ -1117,8 +1143,7 @@ static int steam_client_ll_open(struct hid_device *hdev)
>  	steam->client_opened++;
>  	spin_unlock_irqrestore(&steam->lock, flags);
>  
> -	steam_sensors_unregister(steam);
> -	steam_input_unregister(steam);
> +	schedule_work(&steam->unregister_work);
>  
>  	return 0;
>  }
> @@ -1135,11 +1160,7 @@ static void steam_client_ll_close(struct hid_device *hdev)
>  	connected = steam->connected && !steam->client_opened;
>  	spin_unlock_irqrestore(&steam->lock, flags);
>  
> -	if (connected) {
> -		steam_set_lizard_mode(steam, lizard_mode);
> -		steam_input_register(steam);
> -		steam_sensors_register(steam);
> -	}
> +	schedule_work(&steam->unregister_work);
>  }
>  
>  static int steam_client_ll_raw_request(struct hid_device *hdev,
> @@ -1231,6 +1252,7 @@ static int steam_probe(struct hid_device *hdev,
>  	INIT_LIST_HEAD(&steam->list);
>  	INIT_WORK(&steam->rumble_work, steam_haptic_rumble_cb);
>  	steam->sensor_timestamp_us = 0;
> +	INIT_WORK(&steam->unregister_work, steam_work_unregister_cb);
>  
>  	/*
>  	 * With the real steam controller interface, do not connect hidraw.
> @@ -1291,6 +1313,7 @@ static int steam_probe(struct hid_device *hdev,
>  	cancel_work_sync(&steam->work_connect);
>  	cancel_delayed_work_sync(&steam->mode_switch);
>  	cancel_work_sync(&steam->rumble_work);
> +	cancel_work_sync(&steam->unregister_work);
>  
>  	return ret;
>  }
> @@ -1307,6 +1330,7 @@ static void steam_remove(struct hid_device *hdev)
>  	cancel_delayed_work_sync(&steam->mode_switch);
>  	cancel_work_sync(&steam->work_connect);
>  	cancel_work_sync(&steam->rumble_work);
> +	cancel_work_sync(&steam->unregister_work);

So what happens if you actually cancel the work here? Will you be
leaking input device?

And bigger question - do you actually need to unregister devices? Or it
is just a matter of not delivering data through them when device is in
the different mode?

Thanks.

-- 
Dmitry