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

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

[Vicki Pfau]

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);
 	hid_destroy_device(steam->client_hdev);
 	steam->client_hdev = NULL;
 	steam->client_opened = 0;
-- 
2.48.1

[PATCH 2/3] hid-steam: Mutex cleanup

[Vicki Pfau]

Both branches of this if/else start with mutex_lock and end with mutex_unlock.
This hoists the mutex lock/unlock outside of the if statement for simplicity.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-steam.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 395dbe642f00..d6be0deea330 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -559,15 +559,13 @@ static void steam_set_lizard_mode(struct steam_device *steam, bool enable)
 	if (steam->gamepad_mode)
 		enable = false;
 
+	mutex_lock(&steam->report_mutex);
 	if (enable) {
-		mutex_lock(&steam->report_mutex);
 		/* enable esc, enter, cursors */
 		steam_send_report_byte(steam, ID_SET_DEFAULT_DIGITAL_MAPPINGS);
 		/* reset settings */
 		steam_send_report_byte(steam, ID_LOAD_DEFAULT_SETTINGS);
-		mutex_unlock(&steam->report_mutex);
 	} else {
-		mutex_lock(&steam->report_mutex);
 		/* disable esc, enter, cursor */
 		steam_send_report_byte(steam, ID_CLEAR_DIGITAL_MAPPINGS);
 
@@ -579,15 +577,14 @@ static void steam_set_lizard_mode(struct steam_device *steam, bool enable)
 				SETTING_RIGHT_TRACKPAD_CLICK_PRESSURE, 0xFFFF, /* disable haptic click */
 				SETTING_STEAM_WATCHDOG_ENABLE, 0, /* disable watchdog that tests if Steam is active */
 				0);
-			mutex_unlock(&steam->report_mutex);
 		} else {
 			steam_write_settings(steam,
 				SETTING_LEFT_TRACKPAD_MODE, TRACKPAD_NONE, /* disable mouse */
 				SETTING_RIGHT_TRACKPAD_MODE, TRACKPAD_NONE, /* disable mouse */
 				0);
-			mutex_unlock(&steam->report_mutex);
 		}
 	}
+	mutex_unlock(&steam->report_mutex);
 }
 
 static int steam_input_open(struct input_dev *dev)
-- 
2.48.1

Re: [PATCH 2/3] hid-steam: Mutex cleanup

[Jiri Kosina]

On Tue, 4 Feb 2025, Vicki Pfau wrote:

> Both branches of this if/else start with mutex_lock and end with mutex_unlock.
> This hoists the mutex lock/unlock outside of the if statement for simplicity.
> 
> Signed-off-by: Vicki Pfau <vi@endrift.com>

Applied to hid.git#for-6.15/steam.

-- 
Jiri Kosina
SUSE Labs

[PATCH 3/3] hid-steam: Don't use cancel_delayed_work_sync in IRQ context

[Vicki Pfau]

Lockdep reported that, as steam_do_deck_input_event is called from
steam_raw_event inside of an IRQ context, it can lead to issues if that IRQ
occurs while the work to be cancelled is running. By using cancel_delayed_work,
this issue can be avoided. The exact ordering of the work and the event
processing is not super important, so this is safe.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-steam.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index d6be0deea330..0f4d55f82423 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -1614,7 +1614,7 @@ static void steam_do_deck_input_event(struct steam_device *steam,
 
 	if (!(b9 & BIT(6)) && steam->did_mode_switch) {
 		steam->did_mode_switch = false;
-		cancel_delayed_work_sync(&steam->mode_switch);
+		cancel_delayed_work(&steam->mode_switch);
 	} else if (!steam->client_opened && (b9 & BIT(6)) && !steam->did_mode_switch) {
 		steam->did_mode_switch = true;
 		schedule_delayed_work(&steam->mode_switch, 45 * HZ / 100);
-- 
2.48.1

Re: [PATCH 3/3] hid-steam: Don't use cancel_delayed_work_sync in IRQ context

[Jiri Kosina]

On Tue, 4 Feb 2025, Vicki Pfau wrote:

> Lockdep reported that, as steam_do_deck_input_event is called from
> steam_raw_event inside of an IRQ context, it can lead to issues if that IRQ
> occurs while the work to be cancelled is running. By using cancel_delayed_work,
> this issue can be avoided. The exact ordering of the work and the event
> processing is not super important, so this is safe.
> 
> Signed-off-by: Vicki Pfau <vi@endrift.com>

Applied to hid.git#for-6.14/upstream-fixes. Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Jiri Kosina]

Vicki,

all three patches look good, thanks for fixing the issues.

Is it possible to identify commits that could be used as 'Fixes:' tag for 
each of the patches? If this has been the issue since the beginning, it's 
good to reference the initial commit by the Fixes: tag still.

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Vicki Pfau]

Jiri

On 2/5/25 1:07 AM, Jiri Kosina wrote:
> Vicki,
> 
> all three patches look good, thanks for fixing the issues.
> 
> Is it possible to identify commits that could be used as 'Fixes:' tag for 
> each of the patches? If this has been the issue since the beginning, it's 
> good to reference the initial commit by the Fixes: tag still.

Sure thing. Patch 2 is just some code cleanup and doesn't fix any bugs. Patch 3 Fixes: cd438e57dd05 ("HID: hid-steam: Add gamepad-only mode switched to by holding options"), but Patch 1 is a bit rough to track down since it's so old. I believe it Fixes: 385a4886778f6 ("HID: steam: remove input device when a hid client is running."), but I'm not sure, and testing it would be kind of rough.
> 
> Thanks,
> 

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

[Jiri Kosina]

On Tue, 4 Feb 2025, 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>

Applied to hid.git#for-6.14/upstream-fixes.

-- 
Jiri Kosina
SUSE Labs

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

[Dmitry Torokhov]

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