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@xxxxxxxxxxx>
> ---
> 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
