[PATCH v2 0/4] HID: sony: more cleanup

[PATCH v2 0/4] HID: sony: more cleanup

[Rosalie Wanders]

This series cleans up hid-sony by utilizing modern helper functions.

v2: the dependent patches have been included in this series

Rosalie Wanders (4):
  HID: sony: use dedicated raw_event() handlers in sony_raw_event()
  HID: sony: use guard() and scoped_guard()
  HID: sony: remove unneeded which argument from sony_schedule_work()
  HID: sony: use devm_kasprintf()

 drivers/hid/hid-sony.c | 245 +++++++++++++++++++++--------------------
 1 file changed, 126 insertions(+), 119 deletions(-)

-- 
2.54.0

[PATCH v2 1/4] HID: sony: use dedicated raw_event() handlers in sony_raw_event()

[Rosalie Wanders]

This commit changes the way sony_raw_event() works by adding a function
pointer to a raw_event() handler in the sc struct instead of manually
checking the quirk in order to call the right function, this simplifies
the sony_raw_event() function alongside making the raw_event() handlers
more self-contained, thus making the code more readable.

The raw_event() handler should be configured using the new
sony_init_raw_event_handler() function in sony_input_configured(), where
we already check for quirks and apply device specific workarounds.

Signed-off-by: Rosalie Wanders <rosalie@mailbox.org>
---
 drivers/hid/hid-sony.c | 145 +++++++++++++++++++++++++----------------
 1 file changed, 90 insertions(+), 55 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index e75246d29e16..2d9a5261b63f 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -81,6 +81,7 @@
 #define SONY_FF_SUPPORT (SIXAXIS_CONTROLLER | MOTION_CONTROLLER)
 #define SONY_BT_DEVICE (SIXAXIS_CONTROLLER_BT | MOTION_CONTROLLER_BT | NAVIGATION_CONTROLLER_BT)
 #define NSG_MRXU_REMOTE (NSG_MR5U_REMOTE_BT | NSG_MR7U_REMOTE_BT)
+#define RB4_GUITAR_PS4 (RB4_GUITAR_PS4_USB | RB4_GUITAR_PS4_BT)
 
 #define MAX_LEDS 4
 #define NSG_MRXU_MAX_X 1667
@@ -534,6 +535,7 @@ struct sony_sc {
 	struct input_dev *sensor_dev;
 	struct led_classdev *leds[MAX_LEDS];
 	unsigned long quirks;
+	int (*raw_event)(struct sony_sc *sc, u8 *rd, int size);
 	struct work_struct state_worker;
 	void (*send_output_report)(struct sony_sc *sc);
 	struct power_supply *battery;
@@ -946,7 +948,7 @@ static const u8 *sony_report_fixup(struct hid_device *hdev, u8 *rdesc,
 	return rdesc;
 }
 
-static void sixaxis_parse_report(struct sony_sc *sc, u8 *rd, int size)
+static int sixaxis_raw_event(struct sony_sc *sc, u8 *rd, int size)
 {
 	static const u8 sixaxis_battery_capacity[] = { 0, 1, 25, 50, 75, 100 };
 	unsigned long flags;
@@ -955,6 +957,31 @@ static void sixaxis_parse_report(struct sony_sc *sc, u8 *rd, int size)
 	u8 battery_capacity;
 	int battery_status;
 
+	if (unlikely(size != 49 || rd[0] != 0x01))
+		return 0;
+
+	if (sc->quirks & SIXAXIS_CONTROLLER) {
+		/*
+		 * When connected via Bluetooth the Sixaxis occasionally sends
+		 * a report with the second byte 0xff and the rest zeroed.
+		 *
+		 * This report does not reflect the actual state of the
+		 * controller must be ignored to avoid generating false input
+		 * events.
+		 */
+		if (rd[1] == 0xff)
+			return -EINVAL;
+
+		/*
+		 * Sixaxis HID report has acclerometers/gyro with MSByte first, this
+		 * has to be BYTE_SWAPPED before passing up to joystick interface
+		 */
+		swap(rd[41], rd[42]);
+		swap(rd[43], rd[44]);
+		swap(rd[45], rd[46]);
+		swap(rd[47], rd[48]);
+	}
+
 	/*
 	 * The sixaxis is charging if the battery value is 0xee
 	 * and it is fully charged if the value is 0xef.
@@ -993,13 +1020,18 @@ static void sixaxis_parse_report(struct sony_sc *sc, u8 *rd, int size)
 
 		input_sync(sc->sensor_dev);
 	}
+
+	return 0;
 }
 
-static void nsg_mrxu_parse_report(struct sony_sc *sc, u8 *rd, int size)
+static int nsg_mrxu_raw_event(struct sony_sc *sc, u8 *rd, int size)
 {
 	int n, offset, relx, rely;
 	u8 active;
 
+	if (unlikely(size < 12 || rd[0] != 0x02))
+		return 0;
+
 	/*
 	 * The NSG-MRxU multi-touch trackpad data starts at offset 1 and
 	 *   the touch-related data starts at offset 2.
@@ -1067,10 +1099,33 @@ static void nsg_mrxu_parse_report(struct sony_sc *sc, u8 *rd, int size)
 	input_mt_sync_frame(sc->touchpad);
 
 	input_sync(sc->touchpad);
+	return 0;
+}
+
+static int rb3_pro_instrument_raw_event(struct sony_sc *sc, u8 *rd, int size)
+{
+	/* Rock Band 3 PS3 Pro instruments set rd[24] to 0xE0 when they're
+	 * sending full reports, and 0x02 when only sending navigation.
+	 */
+	if (size < 25 || rd[24] != 0x02)
+		return 0;
+
+	/* Only attempt to enable full report every 8 seconds */
+	if (time_after(jiffies, sc->rb3_pro_poke_jiffies)) {
+		sc->rb3_pro_poke_jiffies = jiffies + secs_to_jiffies(8);
+		rb3_pro_instrument_enable_full_report(sc);
+	}
+
+	return 0;
 }
 
-static void rb4_ps4_guitar_parse_report(struct sony_sc *sc, u8 *rd, int size)
+static int rb4_ps4_guitar_raw_event(struct sony_sc *sc, u8 *rd, int size)
 {
+	const int expected_size = (sc->quirks & RB4_GUITAR_PS4_BT) ? 78 : 64;
+
+	if (unlikely(size != expected_size || rd[0] != 0x01))
+		return 0;
+
 	/*
 	 * Rock Band 4 PS4 guitars have whammy and
 	 * tilt functionality, they're located at
@@ -1084,9 +1139,10 @@ static void rb4_ps4_guitar_parse_report(struct sony_sc *sc, u8 *rd, int size)
 	input_report_abs(sc->input_dev, ABS_RZ, rd[45]);
 
 	input_sync(sc->input_dev);
+	return 0;
 }
 
-static void rb4_ps5_guitar_parse_report(struct sony_sc *sc, u8 *rd, int size)
+static int rb4_ps5_guitar_raw_event(struct sony_sc *sc, u8 *rd, int size)
 {
 	u8 charging_status;
 	u8 battery_data;
@@ -1094,6 +1150,9 @@ static void rb4_ps5_guitar_parse_report(struct sony_sc *sc, u8 *rd, int size)
 	u8 battery_status;
 	unsigned long flags;
 
+	if (unlikely(size != 64 || rd[0] != 0x01))
+		return 0;
+
 	/*
 	 * Rock Band 4 PS5 guitars have whammy and
 	 * tilt functionality, they're located at
@@ -1138,65 +1197,22 @@ static void rb4_ps5_guitar_parse_report(struct sony_sc *sc, u8 *rd, int size)
 	spin_unlock_irqrestore(&sc->lock, flags);
 
 	input_sync(sc->input_dev);
+	return 0;
 }
 
 static int sony_raw_event(struct hid_device *hdev, struct hid_report *report,
 		u8 *rd, int size)
 {
 	struct sony_sc *sc = hid_get_drvdata(hdev);
+	int ret;
 
-	/*
-	 * Sixaxis HID report has acclerometers/gyro with MSByte first, this
-	 * has to be BYTE_SWAPPED before passing up to joystick interface
-	 */
-	if ((sc->quirks & SIXAXIS_CONTROLLER) && rd[0] == 0x01 && size == 49) {
-		/*
-		 * When connected via Bluetooth the Sixaxis occasionally sends
-		 * a report with the second byte 0xff and the rest zeroed.
-		 *
-		 * This report does not reflect the actual state of the
-		 * controller must be ignored to avoid generating false input
-		 * events.
-		 */
-		if (rd[1] == 0xff)
-			return -EINVAL;
-
-		swap(rd[41], rd[42]);
-		swap(rd[43], rd[44]);
-		swap(rd[45], rd[46]);
-		swap(rd[47], rd[48]);
-
-		sixaxis_parse_report(sc, rd, size);
-	} else if ((sc->quirks & MOTION_CONTROLLER_BT) && rd[0] == 0x01 && size == 49) {
-		sixaxis_parse_report(sc, rd, size);
-	} else if ((sc->quirks & NAVIGATION_CONTROLLER) && rd[0] == 0x01 && size == 49) {
-		sixaxis_parse_report(sc, rd, size);
-	} else if ((sc->quirks & NSG_MRXU_REMOTE) && rd[0] == 0x02 && size >= 12) {
-		nsg_mrxu_parse_report(sc, rd, size);
-		return 1;
-	} else if ((sc->quirks & RB4_GUITAR_PS4_USB) && rd[0] == 0x01 && size == 64) {
-		rb4_ps4_guitar_parse_report(sc, rd, size);
-		return 1;
-	} else if ((sc->quirks & RB4_GUITAR_PS4_BT) && rd[0] == 0x01 && size == 78) {
-		rb4_ps4_guitar_parse_report(sc, rd, size);
-		return 1;
-	} else if ((sc->quirks & RB4_GUITAR_PS5) && rd[0] == 0x01 && size == 64) {
-		rb4_ps5_guitar_parse_report(sc, rd, size);
-		return 1;
-	}
-
-	/* Rock Band 3 PS3 Pro instruments set rd[24] to 0xE0 when they're
-	 * sending full reports, and 0x02 when only sending navigation.
-	 */
-	if ((sc->quirks & RB3_PRO_INSTRUMENT) && size >= 25 && rd[24] == 0x02) {
-		/* Only attempt to enable full report every 8 seconds */
-		if (time_after(jiffies, sc->rb3_pro_poke_jiffies)) {
-			sc->rb3_pro_poke_jiffies = jiffies + secs_to_jiffies(8);
-			rb3_pro_instrument_enable_full_report(sc);
-		}
+	if (sc->raw_event) {
+		ret = sc->raw_event(sc, rd, size);
+		if (unlikely(ret < 0))
+			return ret;
 	}
 
-	if (sc->defer_initialization) {
+	if (unlikely(sc->defer_initialization)) {
 		sc->defer_initialization = 0;
 		sony_schedule_work(sc, SONY_WORKER_STATE);
 	}
@@ -1256,7 +1272,7 @@ static int sony_mapping(struct hid_device *hdev, struct hid_input *hi,
 	if (sc->quirks & DJH_TURNTABLE)
 		return djh_turntable_mapping(hdev, hi, field, usage, bit, max);
 
-	if (sc->quirks & (RB4_GUITAR_PS4_USB | RB4_GUITAR_PS4_BT))
+	if (sc->quirks & RB4_GUITAR_PS4)
 		return rb4_guitar_mapping(hdev, hi, field, usage, bit, max);
 
 	if (sc->quirks & RB4_GUITAR_PS5)
@@ -2110,6 +2126,12 @@ static void sony_release_device_id(struct sony_sc *sc)
 	}
 }
 
+static inline void sony_init_raw_event_handler(struct sony_sc *sc,
+				int (*raw_event)(struct sony_sc *, u8 *, int))
+{
+	sc->raw_event = raw_event;
+}
+
 static inline void sony_init_output_report(struct sony_sc *sc,
 				void (*send_output_report)(struct sony_sc *))
 {
@@ -2185,6 +2207,7 @@ static int sony_input_configured(struct hid_device *hdev,
 			goto err_stop;
 		}
 
+		sony_init_raw_event_handler(sc, sixaxis_raw_event);
 		sony_init_output_report(sc, sixaxis_send_output_report);
 	} else if (sc->quirks & NAVIGATION_CONTROLLER_BT) {
 		/*
@@ -2199,6 +2222,7 @@ static int sony_input_configured(struct hid_device *hdev,
 			goto err_stop;
 		}
 
+		sony_init_raw_event_handler(sc, sixaxis_raw_event);
 		sony_init_output_report(sc, sixaxis_send_output_report);
 	} else if (sc->quirks & RB3_PRO_INSTRUMENT) {
 		/*
@@ -2213,6 +2237,8 @@ static int sony_input_configured(struct hid_device *hdev,
 		 */
 		hdev->quirks |= HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP;
 		hdev->quirks |= HID_QUIRK_SKIP_OUTPUT_REPORT_ID;
+
+		sony_init_raw_event_handler(sc, rb3_pro_instrument_raw_event);
 	} else if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
 		/*
 		 * The Sony Sixaxis does not handle HID Output Reports on the
@@ -2237,6 +2263,7 @@ static int sony_input_configured(struct hid_device *hdev,
 			goto err_stop;
 		}
 
+		sony_init_raw_event_handler(sc, sixaxis_raw_event);
 		sony_init_output_report(sc, sixaxis_send_output_report);
 	} else if (sc->quirks & SIXAXIS_CONTROLLER_BT) {
 		/*
@@ -2258,6 +2285,7 @@ static int sony_input_configured(struct hid_device *hdev,
 			goto err_stop;
 		}
 
+		sony_init_raw_event_handler(sc, sixaxis_raw_event);
 		sony_init_output_report(sc, sixaxis_send_output_report);
 	} else if (sc->quirks & NSG_MRXU_REMOTE) {
 		/*
@@ -2273,8 +2301,15 @@ static int sony_input_configured(struct hid_device *hdev,
 			goto err_stop;
 		}
 
+		sony_init_raw_event_handler(sc, nsg_mrxu_raw_event);
 	} else if (sc->quirks & MOTION_CONTROLLER) {
+		if (sc->quirks & MOTION_CONTROLLER_BT)
+			sony_init_raw_event_handler(sc, sixaxis_raw_event);
 		sony_init_output_report(sc, motion_send_output_report);
+	} else if (sc->quirks & RB4_GUITAR_PS4) {
+		sony_init_raw_event_handler(sc, rb4_ps4_guitar_raw_event);
+	} else if (sc->quirks & RB4_GUITAR_PS5) {
+		sony_init_raw_event_handler(sc, rb4_ps5_guitar_raw_event);
 	}
 
 	if (sc->quirks & SONY_LED_SUPPORT) {
-- 
2.54.0

Re: [PATCH v2 1/4] HID: sony: use dedicated raw_event() handlers in sony_raw_event()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Race Condition / Null Pointer Dereference in RB4_GUITAR_PS5 initialization.
- [High] Sleeping in Atomic Context in rb3_pro_instrument_raw_event.
--

commit ce2aa734b3cbe3b5ae8274fe7a1ef679e83470b5
Author: Rosalie Wanders <rosalie@mailbox.org>
Subject: HID: sony: use dedicated raw_event() handlers in sony_raw_event()

This commit simplifies how the sony driver processes raw events by introducing
dedicated handler function pointers inside the sony_sc structure. The
appropriate handler is now assigned during device configuration based on
hardware quirks.

> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 315343415e8f1..7da1fd1e52fea 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c

[ ... ]

> +static int rb3_pro_instrument_raw_event(struct sony_sc *sc, u8 *rd, int size)
> +{
> +	/* Rock Band 3 PS3 Pro instruments set rd[24] to 0xE0 when they're
> +	 * sending full reports, and 0x02 when only sending navigation.
> +	 */
> +	if (size < 25 || rd[24] != 0x02)
> +		return 0;
> +
> +	/* Only attempt to enable full report every 8 seconds */
> +	if (time_after(jiffies, sc->rb3_pro_poke_jiffies)) {
> +		sc->rb3_pro_poke_jiffies = jiffies + secs_to_jiffies(8);
> +		rb3_pro_instrument_enable_full_report(sc);

[Severity: High]
This is a pre-existing issue, but can this call sequence sleep in an atomic
context?

The raw event handler usually executes in an atomic context, such as a USB URB
completion handler. This path calls rb3_pro_instrument_enable_full_report()
which allocates memory using GFP_KERNEL and issues a synchronous USB control
transfer via hid_hw_raw_request().

Could calling these blocking functions in atomic context trigger a might_sleep
warning or lead to a kernel panic?

> +	}
> +
> +	return 0;
> +}

[ ... ]

> -static void rb4_ps5_guitar_parse_report(struct sony_sc *sc, u8 *rd, int size)
> +static int rb4_ps5_guitar_raw_event(struct sony_sc *sc, u8 *rd, int size)
>  {
>  	u8 charging_status;
>  	u8 battery_data;
> @@ -1094,6 +1150,9 @@ static void rb4_ps5_guitar_parse_report(struct sony_sc *sc, u8 *rd, int size)
>  	u8 battery_status;
>  	unsigned long flags;
>  
> +	if (unlikely(size != 64 || rd[0] != 0x01))
> +		return 0;
> +
>  	/*
>  	 * Rock Band 4 PS5 guitars have whammy and
>  	 * tilt functionality, they're located at

[Severity: High]
This isn't a bug introduced by this patch, but could this handler dereference
a null pointer if an event arrives during device initialization?

Looking at sony_input_configured(), the raw event handler for the guitar is
registered, and hid_hw_open() is called because the device has battery support.
However, sc->input_dev is not assigned until after these initialization steps.

If the device sends a report immediately after the hardware connection opens,
would this event handler trigger and attempt to call input_report_abs() using
a null sc->input_dev pointer?

> @@ -1138,65 +1197,22 @@ static void rb4_ps5_guitar_parse_report(struct sony_sc *sc, u8 *rd, int size)
>  	spin_unlock_irqrestore(&sc->lock, flags);
>  
>  	input_sync(sc->input_dev);
> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616205042.6671-2-rosalie@mailbox.org?part=1

[PATCH v2 2/4] HID: sony: use guard() and scoped_guard()

[Rosalie Wanders]

This replaces the spin_lock_irqsave() and spin_unlock_irqrestore() calls
with the RAII guard() and scoped_guard().

Signed-off-by: Rosalie Wanders <rosalie@mailbox.org>
---
 drivers/hid/hid-sony.c | 62 ++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 36 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 2d9a5261b63f..84df55c3cbe1 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -29,6 +29,7 @@
  * There will be no PIN request from the device.
  */
 
+#include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/hid.h>
 #include <linux/module.h>
@@ -571,14 +572,12 @@ static void sony_set_leds(struct sony_sc *sc);
 static inline void sony_schedule_work(struct sony_sc *sc,
 				      enum sony_worker which)
 {
-	unsigned long flags;
-
 	switch (which) {
 	case SONY_WORKER_STATE:
-		spin_lock_irqsave(&sc->lock, flags);
-		if (!sc->defer_initialization && sc->state_worker_initialized)
-			schedule_work(&sc->state_worker);
-		spin_unlock_irqrestore(&sc->lock, flags);
+		scoped_guard(spinlock_irqsave, &sc->lock) {
+			if (!sc->defer_initialization && sc->state_worker_initialized)
+				schedule_work(&sc->state_worker);
+		}
 		break;
 	}
 }
@@ -951,7 +950,6 @@ static const u8 *sony_report_fixup(struct hid_device *hdev, u8 *rdesc,
 static int sixaxis_raw_event(struct sony_sc *sc, u8 *rd, int size)
 {
 	static const u8 sixaxis_battery_capacity[] = { 0, 1, 25, 50, 75, 100 };
-	unsigned long flags;
 	int offset;
 	u8 index;
 	u8 battery_capacity;
@@ -999,10 +997,10 @@ static int sixaxis_raw_event(struct sony_sc *sc, u8 *rd, int size)
 		battery_status = POWER_SUPPLY_STATUS_DISCHARGING;
 	}
 
-	spin_lock_irqsave(&sc->lock, flags);
-	sc->battery_capacity = battery_capacity;
-	sc->battery_status = battery_status;
-	spin_unlock_irqrestore(&sc->lock, flags);
+	scoped_guard(spinlock_irqsave, &sc->lock) {
+		sc->battery_capacity = battery_capacity;
+		sc->battery_status = battery_status;
+	}
 
 	if (sc->quirks & SIXAXIS_CONTROLLER) {
 		int val;
@@ -1148,7 +1146,6 @@ static int rb4_ps5_guitar_raw_event(struct sony_sc *sc, u8 *rd, int size)
 	u8 battery_data;
 	u8 battery_capacity;
 	u8 battery_status;
-	unsigned long flags;
 
 	if (unlikely(size != 64 || rd[0] != 0x01))
 		return 0;
@@ -1191,10 +1188,10 @@ static int rb4_ps5_guitar_raw_event(struct sony_sc *sc, u8 *rd, int size)
 		break;
 	}
 
-	spin_lock_irqsave(&sc->lock, flags);
-	sc->battery_capacity = battery_capacity;
-	sc->battery_status = battery_status;
-	spin_unlock_irqrestore(&sc->lock, flags);
+	scoped_guard(spinlock_irqsave, &sc->lock) {
+		sc->battery_capacity = battery_capacity;
+		sc->battery_status = battery_status;
+	}
 
 	input_sync(sc->input_dev);
 	return 0;
@@ -1885,15 +1882,14 @@ static int sony_battery_get_property(struct power_supply *psy,
 				     union power_supply_propval *val)
 {
 	struct sony_sc *sc = power_supply_get_drvdata(psy);
-	unsigned long flags;
 	int ret = 0;
 	u8 battery_capacity;
 	int battery_status;
 
-	spin_lock_irqsave(&sc->lock, flags);
-	battery_capacity = sc->battery_capacity;
-	battery_status = sc->battery_status;
-	spin_unlock_irqrestore(&sc->lock, flags);
+	scoped_guard(spinlock_irqsave, &sc->lock) {
+		battery_capacity = sc->battery_capacity;
+		battery_status = sc->battery_status;
+	}
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_PRESENT:
@@ -1975,10 +1971,9 @@ static inline int sony_compare_connection_type(struct sony_sc *sc0,
 static int sony_check_add_dev_list(struct sony_sc *sc)
 {
 	struct sony_sc *entry;
-	unsigned long flags;
 	int ret;
 
-	spin_lock_irqsave(&sony_dev_list_lock, flags);
+	guard(spinlock_irqsave)(&sony_dev_list_lock);
 
 	list_for_each_entry(entry, &sony_device_list, list_node) {
 		ret = memcmp(sc->mac_address, entry->mac_address,
@@ -1992,26 +1987,23 @@ static int sony_check_add_dev_list(struct sony_sc *sc)
 				"controller with MAC address %pMR already connected\n",
 				sc->mac_address);
 			}
-			goto unlock;
+			goto out;
 		}
 	}
 
 	ret = 0;
 	list_add(&(sc->list_node), &sony_device_list);
 
-unlock:
-	spin_unlock_irqrestore(&sony_dev_list_lock, flags);
+out:
 	return ret;
 }
 
 static void sony_remove_dev_list(struct sony_sc *sc)
 {
-	unsigned long flags;
-
 	if (sc->list_node.next) {
-		spin_lock_irqsave(&sony_dev_list_lock, flags);
-		list_del(&(sc->list_node));
-		spin_unlock_irqrestore(&sony_dev_list_lock, flags);
+		scoped_guard(spinlock_irqsave, &sony_dev_list_lock) {
+			list_del(&(sc->list_node));
+		}
 	}
 }
 
@@ -2145,12 +2137,10 @@ static inline void sony_init_output_report(struct sony_sc *sc,
 
 static inline void sony_cancel_work_sync(struct sony_sc *sc)
 {
-	unsigned long flags;
-
 	if (sc->state_worker_initialized) {
-		spin_lock_irqsave(&sc->lock, flags);
-		sc->state_worker_initialized = 0;
-		spin_unlock_irqrestore(&sc->lock, flags);
+		scoped_guard(spinlock_irqsave, &sc->lock) {
+			sc->state_worker_initialized = 0;
+		}
 		cancel_work_sync(&sc->state_worker);
 	}
 }
-- 
2.54.0

[PATCH v2 3/4] HID: sony: remove unneeded which argument from sony_schedule_work()

[Rosalie Wanders]

The sony_worker enum only had a single member, so removing it simplifies
sony_schedule_work().

Signed-off-by: Rosalie Wanders <rosalie@mailbox.org>
---
 drivers/hid/hid-sony.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 84df55c3cbe1..ff681ebc76ce 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -523,10 +523,6 @@ static DEFINE_SPINLOCK(sony_dev_list_lock);
 static LIST_HEAD(sony_device_list);
 static DEFINE_IDA(sony_device_id_allocator);
 
-enum sony_worker {
-	SONY_WORKER_STATE
-};
-
 struct sony_sc {
 	spinlock_t lock;
 	struct list_head list_node;
@@ -569,17 +565,11 @@ struct sony_sc {
 
 static void sony_set_leds(struct sony_sc *sc);
 
-static inline void sony_schedule_work(struct sony_sc *sc,
-				      enum sony_worker which)
+static inline void sony_schedule_work(struct sony_sc *sc)
 {
-	switch (which) {
-	case SONY_WORKER_STATE:
-		scoped_guard(spinlock_irqsave, &sc->lock) {
-			if (!sc->defer_initialization && sc->state_worker_initialized)
-				schedule_work(&sc->state_worker);
-		}
-		break;
-	}
+	guard(spinlock_irqsave)(&sc->lock);
+	if (!sc->defer_initialization && sc->state_worker_initialized)
+		schedule_work(&sc->state_worker);
 }
 
 static void ghl_magic_poke_cb(struct urb *urb)
@@ -1211,7 +1201,7 @@ static int sony_raw_event(struct hid_device *hdev, struct hid_report *report,
 
 	if (unlikely(sc->defer_initialization)) {
 		sc->defer_initialization = 0;
-		sony_schedule_work(sc, SONY_WORKER_STATE);
+		sony_schedule_work(sc);
 	}
 
 	return 0;
@@ -1520,7 +1510,7 @@ static void buzz_set_leds(struct sony_sc *sc)
 static void sony_set_leds(struct sony_sc *sc)
 {
 	if (!(sc->quirks & BUZZ_CONTROLLER))
-		sony_schedule_work(sc, SONY_WORKER_STATE);
+		sony_schedule_work(sc);
 	else
 		buzz_set_leds(sc);
 }
@@ -1631,7 +1621,7 @@ static int sony_led_blink_set(struct led_classdev *led, unsigned long *delay_on,
 		new_off != drv_data->led_delay_off[n]) {
 		drv_data->led_delay_on[n] = new_on;
 		drv_data->led_delay_off[n] = new_off;
-		sony_schedule_work(drv_data, SONY_WORKER_STATE);
+		sony_schedule_work(drv_data);
 	}
 
 	return 0;
@@ -1859,7 +1849,7 @@ static int sony_play_effect(struct input_dev *dev, void *data,
 	sc->left = effect->u.rumble.strong_magnitude / 256;
 	sc->right = effect->u.rumble.weak_magnitude / 256;
 
-	sony_schedule_work(sc, SONY_WORKER_STATE);
+	sony_schedule_work(sc);
 	return 0;
 }
 
-- 
2.54.0

Re: [PATCH v2 3/4] HID: sony: remove unneeded which argument from sony_schedule_work()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unbalanced `hid_hw_close()` calls lead to `hdev->ll_open_count` underflow.
--

commit 45be0bef88ed1ff7df93b4882f4865bcd24126c9
Author: Rosalie Wanders <rosalie@mailbox.org>

HID: sony: remove unneeded which argument from sony_schedule_work()

The sony_worker enum only had a single member, so removing it simplifies
sony_schedule_work().

> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index aa5e65a13ecbc..9014c1db1ba5c 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
[ ... ]
> @@ -1859,7 +1849,7 @@ static int sony_play_effect(struct input_dev *dev, void *data,
>  	sc->left = effect->u.rumble.strong_magnitude / 256;
>  	sc->right = effect->u.rumble.weak_magnitude / 256;
>  
> -	sony_schedule_work(sc, SONY_WORKER_STATE);
> +	sony_schedule_work(sc);
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem with
unbalanced hid_hw_close() calls leading to an underflow of the open count
in this driver.

In sony_input_configured(), hid_hw_open() is only called if the device
supports battery reporting. However, both the error path in
sony_input_configured() and sony_remove() unconditionally call
hid_hw_close():

drivers/hid/hid-sony.c:sony_input_configured() {
    ...
err_close:
	hid_hw_close(hdev);
    ...
}

drivers/hid/hid-sony.c:sony_remove() {
    ...
	hid_hw_close(hdev);
    ...
}

For devices without battery support (such as motion controllers or
buzz controllers), does this cause an unbalanced close that steals an active
userspace open reference or underflows the open count?

If the force feedback initialization fails for a motion controller over USB,
would the error path execute an unbalanced hid_hw_close() and corrupt the
lifecycle state of the HID device, breaking the low-level driver open and
close state machine?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616205042.6671-2-rosalie@mailbox.org?part=3

[PATCH v2 4/4] HID: sony: use devm_kasprintf()

[Rosalie Wanders]

Using devm_kasprintf() makes the code less error-prone.

Signed-off-by: Rosalie Wanders <rosalie@mailbox.org>
---
 drivers/hid/hid-sony.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index ff681ebc76ce..253fff4066eb 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1272,8 +1272,6 @@ static int sony_mapping(struct hid_device *hdev, struct hid_input *hi,
 static int sony_register_touchpad(struct sony_sc *sc, int touch_count,
 		int w, int h, int touch_major, int touch_minor, int orientation)
 {
-	size_t name_sz;
-	char *name;
 	int ret;
 
 	sc->touchpad = devm_input_allocate_device(&sc->hdev->dev);
@@ -1295,12 +1293,10 @@ static int sony_register_touchpad(struct sony_sc *sc, int touch_count,
 	 * a suffix. Other devices which were added later like Sony TV remotes
 	 * inhirited this suffix.
 	 */
-	name_sz = strlen(sc->hdev->name) + sizeof(TOUCHPAD_SUFFIX);
-	name = devm_kzalloc(&sc->hdev->dev, name_sz, GFP_KERNEL);
-	if (!name)
+	sc->touchpad->name = devm_kasprintf(&sc->hdev->dev, GFP_KERNEL, "%s" TOUCHPAD_SUFFIX,
+										sc->hdev->name);
+	if (!sc->touchpad->name)
 		return -ENOMEM;
-	snprintf(name, name_sz, "%s" TOUCHPAD_SUFFIX, sc->hdev->name);
-	sc->touchpad->name = name;
 
 	/* We map the button underneath the touchpad to BTN_LEFT. */
 	__set_bit(EV_KEY, sc->touchpad->evbit);
@@ -1337,8 +1333,6 @@ static int sony_register_touchpad(struct sony_sc *sc, int touch_count,
 
 static int sony_register_sensors(struct sony_sc *sc)
 {
-	size_t name_sz;
-	char *name;
 	int ret;
 
 	sc->sensor_dev = devm_input_allocate_device(&sc->hdev->dev);
@@ -1357,12 +1351,10 @@ static int sony_register_sensors(struct sony_sc *sc)
 	/* Append a suffix to the controller name as there are various
 	 * DS4 compatible non-Sony devices with different names.
 	 */
-	name_sz = strlen(sc->hdev->name) + sizeof(SENSOR_SUFFIX);
-	name = devm_kzalloc(&sc->hdev->dev, name_sz, GFP_KERNEL);
-	if (!name)
+	sc->sensor_dev->name = devm_kasprintf(&sc->hdev->dev, GFP_KERNEL, "%s" SENSOR_SUFFIX,
+										sc->hdev->name);
+	if (!sc->sensor_dev->name)
 		return -ENOMEM;
-	snprintf(name, name_sz, "%s" SENSOR_SUFFIX, sc->hdev->name);
-	sc->sensor_dev->name = name;
 
 	if (sc->quirks & SIXAXIS_CONTROLLER) {
 		/* For the DS3 we only support the accelerometer, which works
-- 
2.54.0

Re: [PATCH v2 0/4] HID: sony: more cleanup

[Jiri Kosina]

On Tue, 16 Jun 2026, Rosalie Wanders wrote:

> This series cleans up hid-sony by utilizing modern helper functions.
> 
> v2: the dependent patches have been included in this series

Thanks a lot for this, Rosalie!

Your patches have always been a little bit confusing to me when it came to 
inter-dependencies ... now that this is sent as a series, I can easily 
apply it on top altogether when the merge window is over.

> 
> Rosalie Wanders (4):
>   HID: sony: use dedicated raw_event() handlers in sony_raw_event()
>   HID: sony: use guard() and scoped_guard()
>   HID: sony: remove unneeded which argument from sony_schedule_work()
>   HID: sony: use devm_kasprintf()

Thanks again,

-- 
Jiri Kosina
SUSE Labs