* [PATCH v2 1/4] HID: sony: use dedicated raw_event() handlers in sony_raw_event()
From: Rosalie Wanders @ 2026-06-16 20:50 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, Rosalie Wanders
In-Reply-To: <20260616205042.6671-2-rosalie@mailbox.org>
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
^ permalink raw reply related
* [PATCH v2 2/4] HID: sony: use guard() and scoped_guard()
From: Rosalie Wanders @ 2026-06-16 20:50 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, Rosalie Wanders
In-Reply-To: <20260616205042.6671-2-rosalie@mailbox.org>
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
^ permalink raw reply related
* [PATCH v2 3/4] HID: sony: remove unneeded which argument from sony_schedule_work()
From: Rosalie Wanders @ 2026-06-16 20:50 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, Rosalie Wanders
In-Reply-To: <20260616205042.6671-2-rosalie@mailbox.org>
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
^ permalink raw reply related
* [PATCH v2 4/4] HID: sony: use devm_kasprintf()
From: Rosalie Wanders @ 2026-06-16 20:50 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, Rosalie Wanders
In-Reply-To: <20260616205042.6671-2-rosalie@mailbox.org>
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
^ permalink raw reply related
* Re: [PATCH v2 0/4] HID: sony: more cleanup
From: Jiri Kosina @ 2026-06-16 20:55 UTC (permalink / raw)
To: Rosalie Wanders; +Cc: Benjamin Tissoires, linux-input, linux-kernel
In-Reply-To: <20260616205042.6671-2-rosalie@mailbox.org>
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
^ permalink raw reply
* Re: [PATCH v2 3/4] HID: sony: remove unneeded which argument from sony_schedule_work()
From: sashiko-bot @ 2026-06-16 21:04 UTC (permalink / raw)
To: Rosalie Wanders; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260616205042.6671-5-rosalie@mailbox.org>
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
^ permalink raw reply
* Re: [PATCH v2 1/4] HID: sony: use dedicated raw_event() handlers in sony_raw_event()
From: sashiko-bot @ 2026-06-16 21:08 UTC (permalink / raw)
To: Rosalie Wanders; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260616205042.6671-3-rosalie@mailbox.org>
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
^ permalink raw reply
* Re: [PATCH v8 4/7] input: keyboard: Add driver for ASUS Transformer dock multimedia keys
From: Dmitry Torokhov @ 2026-06-16 21:23 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Lee Jones,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <CAPVz0n2fnM6zYtP9bCtsY9mcLdce_R+1UPmMo0o4JRgjnhuhKA@mail.gmail.com>
On Tue, Jun 16, 2026 at 09:25:25AM +0300, Svyatoslav Ryhel wrote:
> вт, 16 черв. 2026 р. о 07:26 Dmitry Torokhov <dmitry.torokhov@gmail.com> пише:
> >
> > Hi Svyatoslav,
> >
> > On Thu, May 28, 2026 at 08:32:00AM +0300, Svyatoslav Ryhel wrote:
> > > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > >
> > > Add support for multimedia top button row of ASUS Transformer's Mobile
> > > Dock keyboard. Driver is made that function keys (F1-F12) are used by
> > > default which suits average Linux use better and with pressing
> > > ScreenLock + AltGr function keys layout is switched to multimedia keys.
> > > Since this only modifies codes sent by asus-ec-keys it doesn't affect
> > > normal keyboards at all.
> >
> > I think using input handler to intercept ScreenLock + AltGr is quite
> > awkward. I think this also passes the original key events (unless you
> > make it a filter not a regular handler).
> >
> > I do not see benefit for reacting to AltGr+ScreenLock on other keyboards
> > to activate the special mode on this one. So given the fact that you
> > already mange the data stream when you split it into "serio" ports,
> > maybe just intercept this key combo right there and create the input
> > device and signal input events right there?
> >
>
> Though it seems awkward at a first glance, media keys are integrated
> with a standard keyboard in a detachable dock. It is highly unlikely
> that media keys will be used with a different keyboard then the one
> that is integrated with dock. Additionally, the ScreenLock key has a
> code specific to this driver and is not in general use, so even if any
> standard keyboard has AltGr but none has ScreenLock specific to this
> driver except the dock itself. Handler is also set as observer so it
> should not interfere with work of other input devices.
I am not concerned about it interfering with other drivers, I am
concerned about it unnecessarily connecting to unrelated devices
(anything that declares EV_KEY).
Again, I think having input handler is not appropriate here. I would
fold this patch into the patch that introduces the 2 serio ports,
enhanced the data stream analysis to detect your key combo, and then
report through this new input device. You do not need to have the round
trip through atkbd and the new input handler for this.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/6] Input: mms114 - fix touch indexing for MMS134S and MMS136
From: Dmitry Torokhov @ 2026-06-16 21:58 UTC (permalink / raw)
To: Bryam Vargas; +Cc: linux-input, Linus Walleij, linux-kernel
In-Reply-To: <20260616070529.156342-1-hexlabsecurity@proton.me>
Hi Bryam,
On Tue, Jun 16, 2026 at 07:05:34AM +0000, Bryam Vargas wrote:
> Hi Dmitry,
>
> The indexing fix looks correct -- deriving the byte offset from event_size
> instead of leaning on sizeof(struct mms114_touch) is the right call, and the
> cast is fine since the struct is __packed (no alignment issue at the 6-byte
> stride, and the consumers only touch bytes 0..5).
>
> Two things that might be worth folding into the series:
>
> 1) Fixes: tag. The 6-byte event path for MMS136 -- and therefore this
> stride-vs-index mismatch -- predates ab108678195f. It was introduced in
>
> 53fefdd1d3a3 ("Input: mms114 - support MMS136")
>
> which added MMS136_EVENT_SIZE and the "packet_size / MMS136_EVENT_SIZE"
> branch while the loop already indexed with the 8-byte struct stride;
> ab108678195f ("support MMS134S") only extended that branch to MMS134S.
> So MMS136 hardware has mis-parsed multi-touch since v5.13. Tagging
>
> Fixes: 53fefdd1d3a3 ("Input: mms114 - support MMS136")
>
> (in addition to, or instead of, ab108678195f) lets stable pick it up for
> the MMS136 range as well.
Thanks, I'll update the tag.
>
> 2) Unbounded packet_size. Since 1/6 already rewrites this handler: packet_size
> comes straight from the device's PACKET_SIZE register (a single byte, so
> 1..255 after the "<= 0" guard) and is then used unclamped both as the read
> length
>
> __mms114_read_reg(data, MMS114_INFORMATION, packet_size, (u8 *)touch);
>
> into the 80-byte touch[MMS114_MAX_TOUCH] stack buffer, and as the divisor
> for touch_size -- which is never bounded by MMS114_MAX_TOUCH. A device that
> reports packet_size > 80 overflows the stack buffer on the read, and even
> with the indexing fix the loop still walks past it (touch_size > 10). A
> one-liner before the division closes both:
>
> if (packet_size <= 0)
> goto out;
> + packet_size = min_t(int, packet_size, sizeof(touch));
>
> This one is pre-existing -- the unbounded read goes back to the original
> driver -- so it is really a separate patch in the series rather than part
> of the indexing fix, but it seemed worth raising while the code is in
> flight.
This is fixed by the patch you sent earlier, right? It's been applied so
I did not send it out again with the series.
Thank you for looking at this, please do not hesitate to add your
Reviewed-by to any patches that you are satisfied with.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH bpf-next 1/2] HID: bpf: Fix hid_bpf_get_data() range check
From: Emil Tsalapatis @ 2026-06-16 22:52 UTC (permalink / raw)
To: Yiyang Chen, Jiri Kosina, Benjamin Tissoires, bpf, linux-input
Cc: Shuah Khan, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Kumar Kartikeya Dwivedi,
Song Liu, Yonghong Song, Jiri Olsa, linux-kselftest, linux-kernel
In-Reply-To: <783681f27625a11ed58f00221be046643232a2fc.1781627122.git.chenyy23@mails.tsinghua.edu.cn>
On Tue Jun 16, 2026 at 12:35 PM EDT, Yiyang Chen wrote:
> hid_bpf_get_data() returns a pointer into the HID-BPF context data when
> the caller-provided offset and size fit inside ctx->allocated_size.
>
> The current check adds rdwr_buf_size and offset before comparing the
> result against ctx->allocated_size. Since both values are unsigned, a
> very large size can wrap the sum below ctx->allocated_size and make the
> helper return a pointer even though the requested range is not contained
> in the backing buffer.
>
> Use a non-wrapping range check instead: reject offsets beyond the
> allocation, then compare the requested size with the remaining bytes
> after the offset.
>
Reviewed-by: Emil Tsalapatis <emil@etsalapatis.com>
> Fixes: 4171954f56fb ("HID: bpf/dispatch: regroup kfuncs definitions")
> Signed-off-by: Yiyang Chen <chenyy23@mails.tsinghua.edu.cn>
> ---
> drivers/hid/bpf/hid_bpf_dispatch.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
> index d0130658091b0..09b45c40d84f0 100644
> --- a/drivers/hid/bpf/hid_bpf_dispatch.c
> +++ b/drivers/hid/bpf/hid_bpf_dispatch.c
> @@ -299,7 +299,8 @@ hid_bpf_get_data(struct hid_bpf_ctx *ctx, unsigned int offset, const size_t rdwr
>
> ctx_kern = container_of(ctx, struct hid_bpf_ctx_kern, ctx);
>
> - if (rdwr_buf_size + offset > ctx->allocated_size)
> + if (offset > ctx->allocated_size ||
> + rdwr_buf_size > ctx->allocated_size - offset)
> return NULL;
>
> return ctx_kern->data + offset;
^ permalink raw reply
* Re: [PATCH bpf-next 2/2] selftests/hid: Cover hid_bpf_get_data() size overflow
From: Emil Tsalapatis @ 2026-06-16 23:03 UTC (permalink / raw)
To: Yiyang Chen, Jiri Kosina, Benjamin Tissoires, bpf, linux-input
Cc: Shuah Khan, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Kumar Kartikeya Dwivedi,
Song Liu, Yonghong Song, Jiri Olsa, linux-kselftest, linux-kernel
In-Reply-To: <49d892af0a5e994676723a81d2584fb91bc22a9d.1781627122.git.chenyy23@mails.tsinghua.edu.cn>
On Tue Jun 16, 2026 at 12:35 PM EDT, Yiyang Chen wrote:
> Add a HID-BPF regression check for hid_bpf_get_data() requests whose
> size would overflow when added to the offset.
>
> The new rdesc fixup callback asks for offset 2 and size ~0ULL, then
> records whether the helper returns NULL. A vulnerable kernel returns a
> non-NULL pointer because the runtime check wraps the addition. A fixed
> kernel rejects the request. The test only checks the helper result and
> does not dereference the returned pointer.
>
> Also add KHDR_INCLUDES to the HID selftest build so hid_bpf.c sees the
> current kernel UAPI HID definitions on systems whose installed headers do
> not provide enum hid_report_type.
>
> Signed-off-by: Yiyang Chen <chenyy23@mails.tsinghua.edu.cn>
> ---
> tools/testing/selftests/hid/Makefile | 2 +-
> tools/testing/selftests/hid/hid_bpf.c | 11 +++++++++++
> tools/testing/selftests/hid/progs/hid.c | 18 ++++++++++++++++++
> 3 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/hid/Makefile b/tools/testing/selftests/hid/Makefile
> index 50ec9e0406aba..357c6eb5ff5ee 100644
> --- a/tools/testing/selftests/hid/Makefile
> +++ b/tools/testing/selftests/hid/Makefile
> @@ -24,7 +24,7 @@ CXX ?= $(CROSS_COMPILE)g++
>
> HOSTPKG_CONFIG := pkg-config
>
> -CFLAGS += -g -O0 -rdynamic -Wall -Werror -I$(OUTPUT)
> +CFLAGS += -g -O0 -rdynamic -Wall -Werror -I$(OUTPUT) $(KHDR_INCLUDES)
> CFLAGS += -I$(OUTPUT)/tools/include
>
> LDLIBS += -lelf -lz -lrt -lpthread
> diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c
> index 1e979fb3542ba..f0a210900e63d 100644
> --- a/tools/testing/selftests/hid/hid_bpf.c
> +++ b/tools/testing/selftests/hid/hid_bpf.c
> @@ -887,6 +887,17 @@ TEST_F(hid_bpf, test_rdesc_fixup)
> ASSERT_EQ(rpt_desc.value[4], 0x42);
> }
>
> +TEST_F(hid_bpf, test_rdesc_fixup_get_data_overflow)
> +{
> + const struct test_program progs[] = {
> + { .name = "hid_rdesc_fixup_get_data_overflow" },
> + };
> +
> + LOAD_PROGRAMS(progs);
> +
> + ASSERT_EQ(self->skel->bss->get_data_overflow_check, 1);
Can you just use the return value of the method? Why the separate
variable?
> +}
> +
> static int libbpf_print_fn(enum libbpf_print_level level,
> const char *format, va_list args)
> {
> diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
> index 5ecc845ef7921..c6ae2cd045b0e 100644
> --- a/tools/testing/selftests/hid/progs/hid.c
> +++ b/tools/testing/selftests/hid/progs/hid.c
> @@ -13,6 +13,7 @@ struct attach_prog_args {
>
> __u64 callback_check = 52;
> __u64 callback2_check = 52;
> +__u64 get_data_overflow_check;
>
> SEC("?struct_ops/hid_device_event")
> int BPF_PROG(hid_first_event, struct hid_bpf_ctx *hid_ctx, enum hid_report_type type)
> @@ -240,6 +241,23 @@ struct hid_bpf_ops rdesc_fixup = {
> .hid_rdesc_fixup = (void *)hid_rdesc_fixup,
> };
>
> +SEC("?struct_ops.s/hid_rdesc_fixup")
> +int BPF_PROG(hid_rdesc_fixup_get_data_overflow, struct hid_bpf_ctx *hid_ctx)
> +{
> + __u8 *data;
> +
> + data = hid_bpf_get_data(hid_ctx, 2 /* offset */, ~0ULL /* size */);
> + if (!data)
> + get_data_overflow_check = 1;
> +
> + return 0;
> +}
> +
> +SEC(".struct_ops.link")
> +struct hid_bpf_ops rdesc_fixup_get_data_overflow = {
> + .hid_rdesc_fixup = (void *)hid_rdesc_fixup_get_data_overflow,
> +};
> +
> SEC("?struct_ops/hid_device_event")
> int BPF_PROG(hid_test_insert1, struct hid_bpf_ctx *hid_ctx, enum hid_report_type type)
> {
^ permalink raw reply
* Re: [PATCH v4 4/5] HID: asus: add support for xgm led
From: Denis Benato @ 2026-06-17 0:18 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: linux-kernel, linux-input, Benjamin Tissoires, Jiri Kosina,
Luke D . Jones, Mateusz Schyboll, Denis Benato, Connor Belli
In-Reply-To: <CAGwozwGfwXeC-SLbbHhHN7_a+Uz8G9ToQcb54XRrcGru3GG2Lg@mail.gmail.com>
On 6/15/26 23:55, Antheas Kapenekakis wrote:
> On Mon, 15 Jun 2026 at 18:51, Denis Benato <denis.benato@linux.dev> wrote:
>> XG mobile stations have very bright leds behind the fan that can be
>> turned either ON or OFF: add a cled interface to allow controlling the
>> brightness of those red leds.
>>
>> Signed-off-by: Denis Benato <denis.benato@linux.dev>
>> ---
>> drivers/hid/hid-asus.c | 91 ++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 91 insertions(+)
>>
>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>> index 6896730efafc..e68a6d93369d 100644
>> --- a/drivers/hid/hid-asus.c
>> +++ b/drivers/hid/hid-asus.c
>> @@ -51,6 +51,8 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>> #define FEATURE_KBD_LED_REPORT_ID1 0x5d
>> #define FEATURE_KBD_LED_REPORT_ID2 0x5e
>>
>> +#define ROG_XGM_REPORT_SIZE 300
>> +
>> #define ROG_ALLY_REPORT_SIZE 64
>> #define ROG_ALLY_X_MIN_MCU 313
>> #define ROG_ALLY_MIN_MCU 319
>> @@ -143,6 +145,11 @@ struct asus_worker {
>> bool removed;
>> };
>>
>> +struct asus_xgm_led {
>> + struct led_classdev cdev;
>> + struct hid_device *hdev;
>> +};
>> +
>> struct asus_touchpad_info {
>> int max_x;
>> int max_y;
>> @@ -169,6 +176,7 @@ struct asus_drvdata {
>> unsigned long battery_next_query;
>> struct asus_hid_listener listener;
>> bool fn_lock;
>> + struct asus_xgm_led *xgm_led;
>> };
>>
>> static int asus_report_battery(struct asus_drvdata *, u8 *, int);
>> @@ -1119,6 +1127,26 @@ static int asus_battery_probe(struct hid_device *hdev)
>> return ret;
>> }
>>
>> +static int asus_xgm_led_set(struct led_classdev *led_cdev, enum led_brightness value)
>> +{
>> + const u8 buf[ROG_XGM_REPORT_SIZE] = {
>> + FEATURE_KBD_LED_REPORT_ID2, 0xC5, (value) ? 0x50 : 0x00
>> + };
>> + struct asus_xgm_led *xgm = container_of(led_cdev, struct asus_xgm_led, cdev);
>> + int ret;
>> +
>> + ret = asus_kbd_set_report(xgm->hdev, buf, ROG_XGM_REPORT_SIZE);
>> + if (ret < 0) {
>> + hid_err(xgm->hdev, "Unable to set XG mobile led state: %d\n", ret);
>> + return ret;
>> + } else if (ret != ROG_XGM_REPORT_SIZE) {
>> + hid_err(xgm->hdev, "Unexpected partial transfer to XG mobile: %d\n", ret);
>> + return -EIO;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>> {
>> struct input_dev *input = hi->input;
>> @@ -1343,9 +1371,52 @@ static int asus_start_multitouch(struct hid_device *hdev)
>> return 0;
>> }
>>
>> +static int asus_xgm_init(struct hid_device *hdev, struct asus_drvdata *drvdata)
>> +{
>> + const char *name;
>> + int ret;
>> +
>> + drvdata->xgm_led = devm_kzalloc(&hdev->dev, sizeof(*drvdata->xgm_led), GFP_KERNEL);
>> + if (drvdata->xgm_led == NULL)
>> + return -ENOMEM;
>> +
>> + name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "asus:xgm-%s:led",
>> + strlen(hdev->uniq) ? hdev->uniq : dev_name(&hdev->dev));
>> +
>> + if (name == NULL) {
>> + ret = -ENOMEM;
>> + goto asus_xgm_init_err;
>> + }
>> +
>> + drvdata->xgm_led->hdev = hdev;
>> + drvdata->xgm_led->cdev.name = name;
>> + drvdata->xgm_led->cdev.brightness = 1;
>> + drvdata->xgm_led->cdev.max_brightness = 1;
>> + drvdata->xgm_led->cdev.brightness_set_blocking = asus_xgm_led_set;
>> +
>> + /* LED state is arbitrary on boot, set a default */
>> + ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
>> + if (ret) {
>> + hid_err(hdev, "Asus failed to set xgm led: %d\n", ret);
>> + goto asus_xgm_init_err;
>> + }
>> +
>> + ret = devm_led_classdev_register(&hdev->dev, &drvdata->xgm_led->cdev);
>> + if (ret) {
>> + hid_err(hdev, "Asus failed to register xgm led: %d\n", ret);
>> + goto asus_xgm_init_err;
>> + }
>> +
>> + return 0;
>> +asus_xgm_init_err:
>> + drvdata->xgm_led = NULL;
>> + return ret;
>> +}
>> +
>> static int __maybe_unused asus_resume(struct hid_device *hdev)
>> {
>> struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>> + int ret;
>>
>> /*
>> * If we have a backlight listener registered, restore the previous state,
>> @@ -1355,7 +1426,17 @@ static int __maybe_unused asus_resume(struct hid_device *hdev)
>> if (drvdata->listener.brightness_set)
>> asus_kbd_backlight_set(&drvdata->listener, drvdata->kbd_backlight_brightness);
>>
>> + if (drvdata->xgm_led) {
>> + ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
>> + if (ret) {
>> + hid_err(hdev, "Asus failed to restore xgm brightness: %d\n", ret);
>> + goto asus_resume_err;
>> + }
>> + }
>> +
>> return 0;
>> +asus_resume_err:
>> + return ret;
>> }
>>
>> static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
>> @@ -1484,6 +1565,16 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> }
>> }
>>
>> + if (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID) &&
>> + ((hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2022) ||
>> + (hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2023))) {
>> + ret = asus_xgm_init(hdev, drvdata);
>> + if (ret) {
>> + hid_err(hdev, "Failed to initialize xg mobile: %d\n", ret);
>> + goto err_stop_hw;
>> + }
>> + }
>> +
> reset_resume has a special meaning and perhaps should not have been
> used for this driver at all. Check if resume works and if it does use
> that instead.
I am not using reset_resume for xgm.
reset_resume will become important to recover the ally,
but for xg mobile is unused (and useless).
> Other than that and the refactor patch I'd rather see separately, the
> other 4 patches look fine to me.
Fair, I'll wait for maintainers to tell what they like the most
and go from there.
I agree there are things not really much related here
but at least this way the order to apply them is clear.
> Reviewed-by: Antheas Kapenekakis <lkml@antheas.dev>
Thanks,
Denis
> Best,
> Antheas
>
>> /* Laptops keyboard backlight is always at 0x5a */
>> if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
>> (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID)) &&
>> --
>> 2.47.3
>>
>>
^ permalink raw reply
* Re: [PATCH v4 1/5] HID: asus: refactor the two workqueues and init sequence
From: Denis Benato @ 2026-06-17 1:26 UTC (permalink / raw)
To: Denis Benato, Antheas Kapenekakis
Cc: linux-kernel, linux-input, Benjamin Tissoires, Jiri Kosina,
Luke D . Jones, Mateusz Schyboll, Connor Belli, sahiko-bot
In-Reply-To: <8072c613-5f1a-4760-946a-49deada3cc68@linux.dev>
On 6/16/26 03:00, Denis Benato wrote:
> On 6/15/26 23:49, Antheas Kapenekakis wrote:
>> On Mon, 15 Jun 2026 at 18:51, Denis Benato <denis.benato@linux.dev> wrote:
>>> Multiple issues have been found within the hid-asus driver:
>>> - unchecked size in asus_raw_event()
>>> - unclean teardown of asus_probe on failure
>>> - possible use-after-free in asus_probe
>>> - multiple workqueue used for jobs where one was enough
>>> - sleeping calls in atomic context
>>> - packets of incorrect size being sent to the keyboard controller
>>>
>>> Join the two workqueues into one reusing the stopping mechanism
>>> of the brightness workqueue, use the joined workqueue to also
>>> move the asus_wmi_send_event() sleeping call away from atomic
>>> context and add a size check in asus_raw_event().
>>>
>>> Fixes: f631011e36b8 ("HID: hid-asus: Implement fn lock for Asus ProArt P16")
>>> Fixes: 1489a34e97ef ("HID: asus: Implement Fn+F5 fan control key handler")
>>> Fixes: b34b5945a769 ("HID: asus: listen to the asus-wmi brightness device instead of creating one")
>> I'm not sure b34b5945a769 caused an issue, perhaps drop it from fixes.
>> Does f631011e36b8 do a WMI from a hid interrupt? If not, perhaps it
>> does not need to be changed
> Maybe I confused some tags.
I looked it up: https://lore.kernel.org/all/20260612143838.BD8651F000E9@smtp.kernel.org/
>> But moreso, this patch is too large and will take a bit to review. Can
>> you send it separately after the rest of the series goes through
>> unless users report warnings?
> I agree on it being large. I tried doing it by pieces, but got a bunch of compile
> errors.
>
> I ended up deciding trying it when after a few hours I had to modify code
> I did not want to modify (just yet) just for the thing to compile.
>
> Probably result of frustration more than technical limitation...
>
> I am fine with waiting and will be unavailable for a few days,
> in the meanwhile I hope in some input from hid maintainers
> too.
>> Antheas
>>
>>> Reported-by: sahiko-bot@kernel.org
>>> Closes: https://lore.kernel.org/all/20260613154732.60A4B1F000E9@smtp.kernel.org/
>>> Signed-off-by: Denis Benato <denis.benato@linux.dev>
>>> ---
>>> drivers/hid/hid-asus.c | 393 +++++++++++++++++++++++++++++------------
>>> 1 file changed, 282 insertions(+), 111 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>>> index d34d74df3dc0..05ca6665e0a4 100644
>>> --- a/drivers/hid/hid-asus.c
>>> +++ b/drivers/hid/hid-asus.c
>>> @@ -109,11 +109,36 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>>>
>>> #define TRKID_SGN ((TRKID_MAX + 1) >> 1)
>>>
>>> -struct asus_kbd_leds {
>>> - struct asus_hid_listener listener;
>>> +enum asus_work_action_type {
>>> + FN_LOCK_SYNC,
>>> + BRIGHTNESS_SET,
>>> + WMI_FAN,
>>> +};
>>> +
>>> +struct hid_raw_event_data {
>>> + u8 report_data[FEATURE_KBD_REPORT_SIZE];
>>> + size_t report_size;
>>> +};
>>> +
>>> +struct asus_work_action {
>>> + struct list_head node;
>>> + enum asus_work_action_type type;
>>> + union {
>>> + /* Data for BRIGHTNESS_SET */
>>> + unsigned int brightness;
>>> +
>>> + /* Data for FN_LOCK_SYNC */
>>> + bool fn_lock;
>>> +
>>> + /* Data for WMI_FAN */
>>> + struct hid_raw_event_data fan_hid_data;
>>> + } data;
>>> +};
>>> +
>>> +struct asus_worker {
>>> struct hid_device *hdev;
>>> struct work_struct work;
>>> - unsigned int brightness;
>>> + struct list_head actions;
>>> spinlock_t lock;
>>> bool removed;
>>> };
>>> @@ -133,7 +158,8 @@ struct asus_drvdata {
>>> struct hid_device *hdev;
>>> struct input_dev *input;
>>> struct input_dev *tp_kbd_input;
>>> - struct asus_kbd_leds *kbd_backlight;
>>> + struct asus_worker *worker;
>>> + unsigned int kbd_backlight_brightness;
>>> const struct asus_touchpad_info *tp;
>>> struct power_supply *battery;
>>> struct power_supply_desc battery_desc;
>>> @@ -141,7 +167,7 @@ struct asus_drvdata {
>>> int battery_stat;
>>> bool battery_in_query;
>>> unsigned long battery_next_query;
>>> - struct work_struct fn_lock_sync_work;
>>> + struct asus_hid_listener listener;
>>> bool fn_lock;
>>> };
>>>
>>> @@ -211,6 +237,29 @@ static const u8 asus_report_id_init[] = {
>>> FEATURE_KBD_LED_REPORT_ID2
>>> };
>>>
>>> +/*
>>> + * Send events to asus-wmi driver for handling special keys
>>> + */
>>> +static int asus_wmi_send_event(struct asus_drvdata *drvdata, u8 code)
>>> +{
>>> + int err;
>>> + u32 retval;
>>> +
>>> + err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
>>> + ASUS_WMI_METHODID_NOTIF, code, &retval);
>>> + if (err) {
>>> + pr_warn("Failed to notify asus-wmi: %d\n", err);
>>> + return err;
>>> + }
>>> +
>>> + if (retval != 0) {
>>> + pr_warn("Failed to notify asus-wmi (retval): 0x%x\n", retval);
>>> + return -EIO;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static void asus_report_contact_down(struct asus_drvdata *drvdat,
>>> int toolType, u8 *data)
>>> {
>>> @@ -331,25 +380,71 @@ static int asus_e1239t_event(struct asus_drvdata *drvdat, u8 *data, int size)
>>> }
>>>
>>> /*
>>> - * Send events to asus-wmi driver for handling special keys
>>> + * Used in atomic contexts to schedule work involving sleeps operations or
>>> + * asus-wmi interactions.
>>> + *
>>> + * Caller is responsible to store relevant data in the structure to carry out
>>> + * the required action.
>>> + *
>>> + * This function must be called while the spin lock protecting the workqueue
>>> + * is already being held.
>>> */
>>> -static int asus_wmi_send_event(struct asus_drvdata *drvdata, u8 code)
>>> +static void asus_worker_schedule(struct asus_worker *worker, struct asus_work_action *action)
>>> {
>>> - int err;
>>> - u32 retval;
>>> -
>>> - err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
>>> - ASUS_WMI_METHODID_NOTIF, code, &retval);
>>> - if (err) {
>>> - pr_warn("Failed to notify asus-wmi: %d\n", err);
>>> - return err;
>>> + if (worker->removed) {
>>> + kfree(action);
>>> + return;
>>> }
>>>
>>> - if (retval != 0) {
>>> - pr_warn("Failed to notify asus-wmi (retval): 0x%x\n", retval);
>>> - return -EIO;
>>> + list_add_tail(&action->node, &worker->actions);
>>> + schedule_work(&worker->work);
>>> +}
>>> +
>>> +static int asus_kbd_fn_lock_set(struct asus_drvdata *drvdata, bool enabled)
>>> +{
>>> + struct asus_work_action *action;
>>> + unsigned long flags;
>>> +
>>> + action = kzalloc_obj(struct asus_work_action);
>>> + if (!action)
>>> + return -ENOMEM;
>>> +
>>> + drvdata->fn_lock = enabled;
>>> + action->type = FN_LOCK_SYNC;
>>> + action->data.fn_lock = drvdata->fn_lock;
>>> + INIT_LIST_HEAD(&action->node);
>>> +
>>> + spin_lock_irqsave(&drvdata->worker->lock, flags);
>>> + asus_worker_schedule(drvdata->worker, action);
>>> + spin_unlock_irqrestore(&drvdata->worker->lock, flags);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int asus_kbd_wmi_fan_send(struct asus_drvdata *drvdata, u8 *report_data,
>>> + size_t report_size)
>>> +{
>>> + struct asus_work_action *action;
>>> + unsigned long flags;
>>> +
>>> + if (report_size > FEATURE_KBD_REPORT_SIZE) {
>>> + hid_err(drvdata->hdev, "Invalid report size for fan event: %zu\n", report_size);
>>> + return -EINVAL;
>>> }
>>>
>>> + action = kzalloc_obj(struct asus_work_action);
>>> + if (!action)
>>> + return -ENOMEM;
>>> +
>>> + action->type = WMI_FAN;
>>> + action->data.fan_hid_data.report_size = report_size;
>>> + memcpy(action->data.fan_hid_data.report_data, report_data, report_size);
>>> + INIT_LIST_HEAD(&action->node);
>>> +
>>> + spin_lock_irqsave(&drvdata->worker->lock, flags);
>>> + asus_worker_schedule(drvdata->worker, action);
>>> + spin_unlock_irqrestore(&drvdata->worker->lock, flags);
>>> +
>>> return 0;
>>> }
>>>
>>> @@ -357,6 +452,7 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
>>> struct hid_usage *usage, __s32 value)
>>> {
>>> struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>>> + int ret;
>>>
>>> if ((usage->hid & HID_USAGE_PAGE) == HID_UP_ASUSVENDOR &&
>>> (usage->hid & HID_USAGE) != 0x00 &&
>>> @@ -375,8 +471,11 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
>>> return !asus_hid_event(ASUS_EV_BRTTOGGLE);
>>> case KEY_FN_ESC:
>>> if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
>>> - drvdata->fn_lock = !drvdata->fn_lock;
>>> - schedule_work(&drvdata->fn_lock_sync_work);
>>> + ret = asus_kbd_fn_lock_set(drvdata, !drvdata->fn_lock);
>>> + if (ret) {
>>> + hid_err(hdev, "Error while toggling FN lock: %d\n", ret);
>>> + return ret;
>>> + }
>>> }
>>> break;
>>> }
>>> @@ -389,6 +488,12 @@ static int asus_raw_event(struct hid_device *hdev,
>>> struct hid_report *report, u8 *data, int size)
>>> {
>>> struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>>> + int ret;
>>> +
>>> + if (size < 2) {
>>> + hid_dbg(hdev, "Unexpected keyboard report size %d\n", size);
>>> + return 0;
>>> + }
>>>
>>> if (drvdata->battery && data[0] == BATTERY_REPORT_ID)
>>> return asus_report_battery(drvdata, data, size);
>>> @@ -414,19 +519,13 @@ static int asus_raw_event(struct hid_device *hdev,
>>> * pass to userspace so it can implement its own fan control.
>>> */
>>> if (data[1] == ASUS_FAN_CTRL_KEY_CODE) {
>>> - int ret = asus_wmi_send_event(drvdata, ASUS_FAN_CTRL_KEY_CODE);
>>> + ret = asus_kbd_wmi_fan_send(drvdata, data, size);
>>>
>>> - if (ret == 0) {
>>> - /* Successfully handled by asus-wmi, block event */
>>> + /* if execution deferred successfully block event */
>>> + if (ret == 0)
>>> return -1;
>>> - }
>>>
>>> - /*
>>> - * Warn if asus-wmi failed (but not if it's unavailable).
>>> - * Let the event reach userspace in all failure cases.
>>> - */
>>> - if (ret != -ENODEV)
>>> - hid_warn(hdev, "Failed to notify asus-wmi: %d\n", ret);
>>> + return ret;
>>> }
>>>
>>> /*
>>> @@ -569,59 +668,151 @@ static int asus_kbd_disable_oobe(struct hid_device *hdev)
>>> return 0;
>>> }
>>>
>>> -static int asus_kbd_set_fn_lock(struct hid_device *hdev, bool enabled)
>>> +static void asus_kbd_set_fn_lock(struct hid_device *hdev, bool enabled)
>>> {
>>> - u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xd0, 0x4e, !!enabled };
>>> + const u8 buf[FEATURE_KBD_REPORT_SIZE] = { FEATURE_KBD_REPORT_ID, 0xd0, 0x4e, !!enabled };
>>> + int ret;
>>>
>>> - return asus_kbd_set_report(hdev, buf, sizeof(buf));
>>> + ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
>>> + if (ret < 0)
>>> + hid_err(hdev, "Asus failed to set fn lock: %d\n", ret);
>>> }
>>>
>>> -static void asus_sync_fn_lock(struct work_struct *work)
>>> +static void asus_kbd_set_brightness(struct hid_device *hdev, u8 brightness)
>>> {
>>> - struct asus_drvdata *drvdata =
>>> - container_of(work, struct asus_drvdata, fn_lock_sync_work);
>>> + const u8 buf[FEATURE_KBD_REPORT_SIZE] = {
>>> + FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, brightness
>>> + };
>>> + int ret;
>>>
>>> - asus_kbd_set_fn_lock(drvdata->hdev, drvdata->fn_lock);
>>> + ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
>>> + if (ret < 0)
>>> + hid_err(hdev, "Asus failed to set keyboard backlight: %d\n", ret);
>>> }
>>>
>>> -static void asus_schedule_work(struct asus_kbd_leds *led)
>>> +static void asus_kbd_wmi_fan(struct hid_device *hdev, struct hid_raw_event_data *data)
>>> {
>>> + struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>>> + int ret;
>>> +
>>> + ret = asus_wmi_send_event(drvdata, ASUS_FAN_CTRL_KEY_CODE);
>>> +
>>> + /*
>>> + * Warn if asus-wmi failed (but not if it's unavailable).
>>> + * Let the event reach userspace in all failure cases.
>>> + */
>>> + switch (ret) {
>>> + case -ENODEV:
>>> + break;
>>> + case 0:
>>> + return;
>>> + default:
>>> + hid_warn(hdev, "Failed to notify asus-wmi: %d\n", ret);
>>> + break;
>>> + }
>>> +
>>> + /* Fallback: pass the raw event to the HID core */
>>> + hid_report_raw_event(hdev, HID_INPUT_REPORT,
>>> + data->report_data, data->report_size,
>>> + data->report_size, 1);
>>> +}
>>> +
>>> +static void asus_kbd_backlight_set(struct asus_hid_listener *listener, int brightness)
>>> +{
>>> + struct asus_drvdata *drvdata = container_of(listener, struct asus_drvdata, listener);
>>> + struct asus_worker *worker = drvdata->worker;
>>> + struct asus_work_action *action;
>>> unsigned long flags;
>>>
>>> - spin_lock_irqsave(&led->lock, flags);
>>> - if (!led->removed)
>>> - schedule_work(&led->work);
>>> - spin_unlock_irqrestore(&led->lock, flags);
>>> + drvdata->kbd_backlight_brightness = brightness;
>>> +
>>> + action = kzalloc_obj(struct asus_work_action);
>>> + if (!action) {
>>> + hid_warn(drvdata->hdev, "Failed to allocate memory for backlight action\n");
>>> + return;
>>> + }
>>> +
>>> + action->type = BRIGHTNESS_SET;
>>> + action->data.brightness = brightness;
>>> + INIT_LIST_HEAD(&action->node);
>>> +
>>> + spin_lock_irqsave(&worker->lock, flags);
>>> + asus_worker_schedule(worker, action);
>>> + spin_unlock_irqrestore(&worker->lock, flags);
>>> }
>>>
>>> -static void asus_kbd_backlight_set(struct asus_hid_listener *listener,
>>> - int brightness)
>>> +static void asus_work(struct work_struct *work)
>>> {
>>> - struct asus_kbd_leds *led = container_of(listener, struct asus_kbd_leds,
>>> - listener);
>>> + struct asus_worker *worker = container_of(work, struct asus_worker, work);
>>> + struct asus_work_action *action = NULL;
>>> unsigned long flags;
>>>
>>> - spin_lock_irqsave(&led->lock, flags);
>>> - led->brightness = brightness;
>>> - spin_unlock_irqrestore(&led->lock, flags);
>>> + /* Save the action to be performed and clear the flag */
>>> + spin_lock_irqsave(&worker->lock, flags);
>>> + if (!list_empty(&worker->actions)) {
>>> + action = list_first_entry(&worker->actions,
>>> + struct asus_work_action, node);
>>> + list_del(&action->node);
>>> + }
>>> + spin_unlock_irqrestore(&worker->lock, flags);
>>> +
>>> + if (!action)
>>> + return;
>>> +
>>> + switch (action->type) {
>>> + case BRIGHTNESS_SET:
>>> + asus_kbd_set_brightness(worker->hdev, action->data.brightness);
>>> + break;
>>> + case FN_LOCK_SYNC:
>>> + asus_kbd_set_fn_lock(worker->hdev, action->data.fn_lock);
>>> + break;
>>> + case WMI_FAN:
>>> + asus_kbd_wmi_fan(worker->hdev, &action->data.fan_hid_data);
>>> + break;
>>> + default:
>>> + hid_err(worker->hdev, "Invalid action type: %d\n", action->type);
>>> + break;
>>> + }
>>> +
>>> + kfree(action);
>>>
>>> - asus_schedule_work(led);
>>> + /* Re-schedule if there are more pending actions */
>>> + spin_lock_irqsave(&worker->lock, flags);
>>> + if (!list_empty(&worker->actions))
>>> + schedule_work(&worker->work);
>>> + spin_unlock_irqrestore(&worker->lock, flags);
>>> }
>>>
>>> -static void asus_kbd_backlight_work(struct work_struct *work)
>>> +static int asus_worker_create(struct hid_device *hdev, struct asus_drvdata *drvdata)
>>> {
>>> - struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds, work);
>>> - u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, 0x00 };
>>> - int ret;
>>> + drvdata->worker = devm_kzalloc(&hdev->dev, sizeof(struct asus_worker), GFP_KERNEL);
>>> + if (!drvdata->worker)
>>> + return -ENOMEM;
>>> +
>>> + drvdata->worker->removed = false;
>>> + drvdata->worker->hdev = hdev;
>>> + INIT_LIST_HEAD(&drvdata->worker->actions);
>>> +
>>> + INIT_WORK(&drvdata->worker->work, asus_work);
>>> + spin_lock_init(&drvdata->worker->lock);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void asus_worker_stop(struct asus_worker *worker)
>>> +{
>>> + struct asus_work_action *action, *tmp;
>>> unsigned long flags;
>>>
>>> - spin_lock_irqsave(&led->lock, flags);
>>> - buf[4] = led->brightness;
>>> - spin_unlock_irqrestore(&led->lock, flags);
>>> + spin_lock_irqsave(&worker->lock, flags);
>>> + worker->removed = true;
>>> + list_for_each_entry_safe(action, tmp, &worker->actions, node) {
>>> + list_del(&action->node);
>>> + kfree(action);
>>> + }
>>> + spin_unlock_irqrestore(&worker->lock, flags);
>>>
>>> - ret = asus_kbd_set_report(led->hdev, buf, sizeof(buf));
>>> - if (ret < 0)
>>> - hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret);
>>> + cancel_work_sync(&worker->work);
>>> }
>>>
>>> /*
>>> @@ -760,23 +951,11 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>>> le16_to_cpu(udev->descriptor.idProduct));
>>> }
>>>
>>> - drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
>>> - sizeof(struct asus_kbd_leds),
>>> - GFP_KERNEL);
>>> - if (!drvdata->kbd_backlight)
>>> - return -ENOMEM;
>>> -
>>> - drvdata->kbd_backlight->removed = false;
>>> - drvdata->kbd_backlight->brightness = 0;
>>> - drvdata->kbd_backlight->hdev = hdev;
>>> - drvdata->kbd_backlight->listener.brightness_set = asus_kbd_backlight_set;
>>> - INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_backlight_work);
>>> - spin_lock_init(&drvdata->kbd_backlight->lock);
>>> -
>>> - ret = asus_hid_register_listener(&drvdata->kbd_backlight->listener);
>>> + drvdata->listener.brightness_set = asus_kbd_backlight_set;
>>> + ret = asus_hid_register_listener(&drvdata->listener);
>>> if (ret < 0) {
>>> - /* No need to have this still around */
>>> - devm_kfree(&hdev->dev, drvdata->kbd_backlight);
>>> + hid_err(hdev, "Unable to register kbd brightness listener: %d\n", ret);
>>> + drvdata->listener.brightness_set = NULL;
>>> }
>>>
>>> return ret;
>>> @@ -965,11 +1144,9 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>>> }
>>> }
>>>
>>> - if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
>>> - drvdata->fn_lock = true;
>>> - INIT_WORK(&drvdata->fn_lock_sync_work, asus_sync_fn_lock);
>>> - asus_kbd_set_fn_lock(hdev, true);
>>> - }
>>> + if ((drvdata->quirks & QUIRK_HID_FN_LOCK) &&
>>> + (asus_kbd_fn_lock_set(drvdata, true)))
>>> + hid_warn(hdev, "Error while setting FN lock to ON\n");
>>>
>>> if (drvdata->tp) {
>>> int ret;
>>> @@ -1004,11 +1181,9 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>>>
>>> drvdata->input = input;
>>>
>>> - if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
>>> - drvdata->fn_lock = true;
>>> - INIT_WORK(&drvdata->fn_lock_sync_work, asus_sync_fn_lock);
>>> - asus_kbd_set_fn_lock(hdev, true);
>>> - }
>>> + if ((drvdata->quirks & QUIRK_HID_FN_LOCK) &&
>>> + (asus_kbd_fn_lock_set(drvdata, true)))
>>> + hid_warn(hdev, "Error while setting FN lock to ON\n");
>>>
>>> return 0;
>>> }
>>> @@ -1171,20 +1346,16 @@ static int asus_start_multitouch(struct hid_device *hdev)
>>> static int __maybe_unused asus_resume(struct hid_device *hdev)
>>> {
>>> struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>>> - int ret = 0;
>>>
>>> - if (drvdata->kbd_backlight) {
>>> - const u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4,
>>> - drvdata->kbd_backlight->brightness };
>>> - ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
>>> - if (ret < 0) {
>>> - hid_err(hdev, "Asus failed to set keyboard backlight: %d\n", ret);
>>> - goto asus_resume_err;
>>> - }
>>> - }
>>> + /*
>>> + * If we have a backlight listener registered, restore the previous state,
>>> + * in case of error do not fail: most models restore the backlight
>>> + * automatically, and the error is non-fatal.
>>> + */
>>> + if (drvdata->listener.brightness_set)
>>> + asus_kbd_backlight_set(&drvdata->listener, drvdata->kbd_backlight_brightness);
>>>
>>> -asus_resume_err:
>>> - return ret;
>>> + return 0;
>>> }
>>>
>>> static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
>>> @@ -1294,6 +1465,12 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>> is_vendor = true;
>>> }
>>>
>>> + ret = asus_worker_create(hdev, drvdata);
>>> + if (ret) {
>>> + hid_warn(hdev, "Failed to initialize worker: %d\n", ret);
>>> + return ret;
>>> + }
>>> +
>>> ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>>> if (ret) {
>>> hid_err(hdev, "Asus hw start failed: %d\n", ret);
>>> @@ -1343,6 +1520,10 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>>
>>> return 0;
>>> err_stop_hw:
>>> + if (drvdata->listener.brightness_set)
>>> + asus_hid_unregister_listener(&drvdata->listener);
>>> +
>>> + asus_worker_stop(drvdata->worker);
>>> hid_hw_stop(hdev);
>>> return ret;
>>> }
>>> @@ -1350,21 +1531,11 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>> static void asus_remove(struct hid_device *hdev)
>>> {
>>> struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>>> - unsigned long flags;
>>> -
>>> - if (drvdata->kbd_backlight) {
>>> - asus_hid_unregister_listener(&drvdata->kbd_backlight->listener);
>>> -
>>> - spin_lock_irqsave(&drvdata->kbd_backlight->lock, flags);
>>> - drvdata->kbd_backlight->removed = true;
>>> - spin_unlock_irqrestore(&drvdata->kbd_backlight->lock, flags);
>>> -
>>> - cancel_work_sync(&drvdata->kbd_backlight->work);
>>> - }
>>>
>>> - if (drvdata->quirks & QUIRK_HID_FN_LOCK)
>>> - cancel_work_sync(&drvdata->fn_lock_sync_work);
>>> + if (drvdata->listener.brightness_set)
>>> + asus_hid_unregister_listener(&drvdata->listener);
>>>
>>> + asus_worker_stop(drvdata->worker);
>>> hid_hw_stop(hdev);
>>> }
>>>
>>> --
>>> 2.47.3
>>>
>>>
^ permalink raw reply
* Re: [PATCH 4/4] arm64: dts: qcom: Add PMIH0108 haptics device node
From: Fenglin Wu @ 2026-06-17 1:30 UTC (permalink / raw)
To: Konrad Dybcio, linux-arm-msm, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Lee Jones, Stephen Boyd,
Bjorn Andersson, Konrad Dybcio
Cc: David Collins, Subbaraman Narayanamurthy, Kamal Wadhwa, kernel,
linux-input, devicetree, linux-kernel
In-Reply-To: <6b3155ea-b583-4f82-8313-7a057fd78066@oss.qualcomm.com>
On 6/16/2026 6:27 PM, Konrad Dybcio wrote:
> On 6/16/26 12:08 PM, Fenglin Wu wrote:
>> Add haptics device node in the PMIH0108 PMIC base dtsi files, and enable
>> it on several boards according to the LRA (Linear Resonant Actuator)
>> component mounted on each of them.
>>
>> Signed-off-by: Fenglin Wu <fenglin.wu@oss.qualcomm.com>
>> ---
>> arch/arm64/boot/dts/qcom/kaanapali-mtp.dts | 7 +++++++
>> arch/arm64/boot/dts/qcom/kaanapali-qrd.dts | 7 +++++++
>> arch/arm64/boot/dts/qcom/pmih0108-kaanapali.dtsi | 9 +++++++++
>> arch/arm64/boot/dts/qcom/pmih0108.dtsi | 9 +++++++++
>> arch/arm64/boot/dts/qcom/sm8750-mtp.dts | 7 +++++++
>> arch/arm64/boot/dts/qcom/sm8750-qrd.dts | 7 +++++++
> One commit per board, please
>
>> 6 files changed, 46 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/kaanapali-mtp.dts b/arch/arm64/boot/dts/qcom/kaanapali-mtp.dts
>> index 07247dc98b70..7e3f59fc008e 100644
>> --- a/arch/arm64/boot/dts/qcom/kaanapali-mtp.dts
>> +++ b/arch/arm64/boot/dts/qcom/kaanapali-mtp.dts
>> @@ -952,6 +952,13 @@ wifi@0 {
>> };
>> };
>>
>> +&pmih0108_e1_haptics {
>> + status = "okay";
> 'status' should go last
>
>> +
>> + qcom,lra-period-us = <6667>;
>> + qcom,vmax-mv = <3600>;
> Do these properties depend on the physical characteristics on what's
> connected to the other end of the haptics driver?
Correct. They are basically the parameters of the LRA that's used on the
board. You can find details of them in the binding document.
>
> Konrad
^ permalink raw reply
* Re: [PATCH 3/4] input: misc: Add Qualcomm SPMI PMIC haptics driver
From: Fenglin Wu @ 2026-06-17 2:31 UTC (permalink / raw)
To: Konrad Dybcio, linux-arm-msm, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Lee Jones, Stephen Boyd,
Bjorn Andersson, Konrad Dybcio
Cc: David Collins, Subbaraman Narayanamurthy, Kamal Wadhwa, kernel,
linux-input, devicetree, linux-kernel
In-Reply-To: <eb693705-c0c3-427b-a924-5aa907fd65bb@oss.qualcomm.com>
>> + ret = ptn_bulk_write(h, HAP_PTN_FIFO_DIN_0_REG, &data[i], 4);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + for (; i < len; i++) {
>> + ret = ptn_write(h, HAP_PTN_FIFO_DIN_1B_REG, (u8)data[i]);
>> + if (ret)
>> + return ret;
>> + }
> So if i'm reading this right, the first loop will always write
> 4*(len//4) bytes and the second one will be entered at most once,
> to write len rem 4 bytes.. should this be an if instead?
I should put a comment for clarification. Here’s some background: FIFO
data writing supports both 4-byte bulk writes using registers
[HAP_PTN_FIFO_DIN_0_REG ... HAP_PTN_FIFO_DIN_3_REG], and 1-byte writes
using the HAP_PTN_FIFO_DIN_1B_REG register. The 4-byte bulk write is
more efficient, especially for waveform which has several Kb data, and
it helps to reduce software latency when loading effects and reduce the
delay in triggering vibration. It also helps prevent the FIFO from
running dry during data refill in FIFO-empty interrupts. Typically, we
use 4-byte writes for the initial 4-byte aligned data, and 1-byte writes
for any trailing remainder.
So it still needs a 'for' loop here since the remainder could be more
than 1 byte.
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * Configure the hardware FIFO memory boundary.
>> + * FIFO occupies addresses [0, fifo_len).
>> + */
>> +static int haptics_configure_fifo_mmap(struct qcom_haptics *h)
>> +{
>> + u32 fifo_len, fifo_units;
>> +
>> + /* Config all memory space for FIFO usage for now */
> What's the not-"for now" endgame for this?
The hardware supports more modes than the two currently supported in the
driver. One of these, called 'PAT_MEM' mode, also shares memory space
with FIFO mode. However, 'PAT_MEM' requires memory to be pre-reserved
and waveform data to be pre-loaded. The entire 8K bytes of memory can be
divided into partitions, and it is configurable, with FIFO mode always
using the first partition [0, fifo_len], where 'fifo_len' is set via the
'MMAP_FIFO_REG' register. 'PAT_MEM' mode plays waveform using data
preloaded in a memory bank defined by the registers
'PATX_MEM_START_ADDR_REG' and 'PATTERN_SPMI_PATX_LEN_REG' (they are not
defined in the driver). Since PAT_MEM is mainly intended for
hardware-triggered vibrations, such as a signal from a dedicated GPIO
triggering a short vibration with a preloaded waveform, and although it
also supports software triggers, I haven't found a suitable way to
support it well into the driver under input FF framework yet. So, I am
currently allocating the entire 8K FIFO memory for FIFO mode only. We
can adjust this later if we find a better way to incorporate 'PAT_MEM'
mode into the driver.
^ permalink raw reply
* [PATCH] Input: synaptics - disable InterTouch on ThinkPad T440p (board id 2722)
From: Raphaël Larocque @ 2026-06-17 2:56 UTC (permalink / raw)
To: linux-input; +Cc: linux-kernel, dmitry.torokhov, Raphaël Larocque
In-Reply-To: <20260616171137.19069-1-rlarocque@disroot.org>
The Lenovo ThinkPad T440p (PNP ID LEN0036, board id 2722) has a
Synaptics touchpad whose SMBus companion is not ready at boot and
takes ~200 s to appear. During this window the touchpad
and pointer are completely unresponsive on approximately 50% of
boots, making the machine unusable until the companion finally
registers.
The device is in the topbuttonpad_pnp_ids[] SMBus allowlist, so the
kernel attempts to use SMBus/RMI4 mode by default. When the companion
is not ready, psmouse_smbus_init() leaves breadcrumbs and returns
-EAGAIN, the PS/2 fallback path is taken, but the device does not
function properly until the companion appears and RMI4 takes over.
Disable SMBus InterTouch for PNP ID LEN0036 with board id 2722 so
the touchpad and TrackPoint work immediately via PS/2 from boot.
Users can still force SMBus with psmouse.synaptics_intertouch=1 if
needed.
Tested-by: Raphaël Larocque <rlarocque@disroot.org>
Signed-off-by: Raphaël Larocque <rlarocque@disroot.org>
---
drivers/input/mouse/synaptics.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index c70502e24031..982685918459 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -1837,6 +1837,16 @@ static int synaptics_setup_intertouch(struct psmouse *psmouse,
return -ENXIO;
}
+
+ /* Disable intertouch on known-broken board revisions */
+ if (psmouse_matches_pnp_id(psmouse,
+ (const char * const []){"LEN0036", NULL}) &&
+ info->board_id == 2722) {
+ psmouse_info(psmouse,
+ "Disabling intertouch for board id %d\n",
+ info->board_id);
+ return -ENXIO;
+ }
}
psmouse_info(psmouse, "Trying to set up SMBus access\n");
--
2.53.0
^ permalink raw reply related
* Re: [PATCH] Input: synaptics - disable InterTouch on ThinkPad T440p (board id 2722)
From: sashiko-bot @ 2026-06-17 3:04 UTC (permalink / raw)
To: Raphaël Larocque; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260617025619.41237-1-rlarocque@disroot.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Format specifier type mismatch in `psmouse_info` call.
--
commit 0fa38fb74cf1360727ab0551657ea6c7dcbf7b75
Author: Raphaël Larocque <rlarocque@disroot.org>
Input: synaptics - disable InterTouch on ThinkPad T440p (board id 2722)
This commit disables SMBus InterTouch for the ThinkPad T440p (board id 2722)
by matching its PNP ID and board ID. This ensures the touchpad works via
PS/2 immediately at boot, bypassing a long delay caused by the SMBus
companion device taking time to initialize.
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -1837,6 +1837,16 @@ static int synaptics_setup_intertouch(struct psmouse *psmouse,
[ ... ]
> + /* Disable intertouch on known-broken board revisions */
> + if (psmouse_matches_pnp_id(psmouse,
> + (const char * const []){"LEN0036", NULL}) &&
> + info->board_id == 2722) {
> + psmouse_info(psmouse,
> + "Disabling intertouch for board id %d\n",
> + info->board_id);
[Severity: Low]
This isn't a bug, but should the format specifier for info->board_id be %u
instead of %d?
Looking at struct synaptics_device_info, board_id is declared as a u32:
struct synaptics_device_info {
...
u32 board_id; /* Board-ID */
...
};
While 2722 easily fits in a signed integer, using %u would avoid potential
format string warnings under stricter warning levels.
> + return -ENXIO;
> + }
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617025619.41237-1-rlarocque@disroot.org?part=1
^ permalink raw reply
* [PATCH] Input: synaptics - disable InterTouch on ThinkPad T440p (board id 2722)
From: Raphaël Larocque @ 2026-06-17 3:34 UTC (permalink / raw)
To: linux-input; +Cc: linux-kernel, dmitry.torokhov, Raphaël Larocque
In-Reply-To: <20260617025619.41237-1-rlarocque@disroot.org>
The Lenovo ThinkPad T440p (PNP ID LEN0036, board id 2722) has a
Synaptics touchpad whose SMBus companion is not ready at boot and
takes ~200 s to appear. During this window the touchpad
and pointer are completely unresponsive on approximately 50% of
boots, making the machine unusable until the companion finally
registers.
The device is in the topbuttonpad_pnp_ids[] SMBus allowlist, so the
kernel attempts to use SMBus/RMI4 mode by default. When the companion
is not ready, psmouse_smbus_init() leaves breadcrumbs and returns
-EAGAIN, the PS/2 fallback path is taken, but the device does not
function properly until the companion appears and RMI4 takes over.
Disable SMBus InterTouch for PNP ID LEN0036 with board id 2722 so
the touchpad and TrackPoint work immediately via PS/2 from boot.
Users can still force SMBus with psmouse.synaptics_intertouch=1 if
needed.
Tested-by: Raphaël Larocque <rlarocque@disroot.org>
Signed-off-by: Raphaël Larocque <rlarocque@disroot.org>
---
drivers/input/mouse/synaptics.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index c70502e24031..dd11ffdd51ae 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -1837,6 +1837,16 @@ static int synaptics_setup_intertouch(struct psmouse *psmouse,
return -ENXIO;
}
+
+ /* Disable intertouch on known-broken board revisions */
+ if (psmouse_matches_pnp_id(psmouse,
+ (const char * const []){"LEN0036", NULL}) &&
+ info->board_id == 2722) {
+ psmouse_info(psmouse,
+ "Disabling intertouch for board id %u\n",
+ info->board_id);
+ return -ENXIO;
+ }
}
psmouse_info(psmouse, "Trying to set up SMBus access\n");
--
2.53.0
^ permalink raw reply related
* RE: [PATCH] Input: yealink - stop URB resubmission on completion error
From: Wang, Jie @ 2026-06-17 5:49 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Henk Vergonet, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org,
syzbot+039eab266c6321a174bd@syzkaller.appspotmail.com,
syzkaller-bugs@googlegroups.com
In-Reply-To: <ajGfjuo3MTlezkYX@google.com>
Hi Dmitry,
Thank you for the review.
> -----Original Message-----
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Sent: Wednesday, June 17, 2026 3:17 AM
> To: Wang, Jie <jie.wang@intel.com>
> Cc: Henk Vergonet <Henk.Vergonet@gmail.com>; linux-input@vger.kernel.org;
> linux-kernel@vger.kernel.org;
> syzbot+039eab266c6321a174bd@syzkaller.appspotmail.com; syzkaller-
> bugs@googlegroups.com
> Subject: Re: [PATCH] Input: yealink - stop URB resubmission on completion error
>
> Hi Jie,
>
> On Tue, Jun 16, 2026 at 11:04:40AM +0000, Jie Wang wrote:
> > urb_irq_callback() and urb_ctl_callback() resubmit each other's URBs
> > regardless of completion status. When the device returns a persistent
> > error (-EPROTO), this creates an unthrottled resubmission loop that
> > starves the CPU and triggers RCU stalls.
> >
> > Stop resubmitting on any non-zero URB status, following the standard
> > completion pattern used by other USB input drivers.
> >
> > Reported-by: syzbot+039eab266c6321a174bd@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=039eab266c6321a174bd
> > Signed-off-by: Jie Wang <jie.wang@intel.com>
>
> It would be good to have a helper like
>
> bool usb_is_permanent_error(struct urb *urb, struct device *dev);
>
> instead of having this "switch" in all the drivers. It also unclear whether we should
> not attempt to re-submit the URB on errors other than ECONNRESET, ENOENT, or
> ESHUTDOWN.
>
For the helper - how about a local yealink_urb_check_status() in v2?
A usb_is_permanent_error() in usb.h would benefit more drivers
across the tree, but that's a larger effort best done separately.
For resubmission - I'm thinking stop immediately on -ECONNRESET,
-ENOENT, -ESHUTDOWN, and for
everything else retry up to 50 consecutive errors before giving up.
Counter resets on any successful completion, so transient bus glitches
recover but a dead device won't spin the CPU forever.
Let me know if that sounds reasonable and I'll send v2.
> > ---
> > drivers/input/misc/yealink.c | 26 ++++++++++++++++++++++----
> > 1 file changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/input/misc/yealink.c
> > b/drivers/input/misc/yealink.c index 8786ed8b3565..bbfa1a9c23d1 100644
> > --- a/drivers/input/misc/yealink.c
> > +++ b/drivers/input/misc/yealink.c
> > @@ -414,9 +414,20 @@ static void urb_irq_callback(struct urb *urb)
> > struct yealink_dev *yld = urb->context;
> > int ret, status = urb->status;
> >
> > - if (status)
> > + switch (status) {
> > + case 0:
> > + break;
> > + case -ECONNRESET:
> > + case -ENOENT:
> > + case -ESHUTDOWN:
> > + dev_dbg(&yld->intf->dev, "%s - urb shutting down with
> status %d\n",
> > + __func__, status);
> > + return;
> > + default:
> > dev_err(&yld->intf->dev, "%s - urb status %d\n",
> > __func__, status);
> > + return;
> > + }
> >
> > switch (yld->irq_data->cmd) {
> > case CMD_KEYPRESS:
> > @@ -452,9 +463,20 @@ static void urb_ctl_callback(struct urb *urb)
> > struct yealink_dev *yld = urb->context;
> > int ret = 0, status = urb->status;
> >
> > - if (status)
> > + switch (status) {
> > + case 0:
> > + break;
> > + case -ECONNRESET:
> > + case -ENOENT:
> > + case -ESHUTDOWN:
> > + dev_dbg(&yld->intf->dev, "%s - urb shutting down with
> status %d\n",
> > + __func__, status);
> > + return;
> > + default:
> > dev_err(&yld->intf->dev, "%s - urb status %d\n",
> > __func__, status);
> > + return;
> > + }
> >
> > switch (yld->ctl_data->cmd) {
> > case CMD_KEYPRESS:
> > --
> > 2.34.1
>
> Thanks.
>
> --
> Dmitry
^ permalink raw reply
* Re: [PATCH 1/6] Input: mms114 - fix touch indexing for MMS134S and MMS136
From: Bryam Vargas @ 2026-06-17 6:26 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel
In-Reply-To: <ajHGfk7pCYltjOm3@google.com>
On Tue, Jun 16, 2026 at 02:58:31PM -0700, Dmitry Torokhov wrote:
> This is fixed by the patch you sent earlier, right? It's been applied so
> I did not send it out again with the series.
Yes -- between the applied reject and 1/6's event_size indexing it's
covered; the min_t I floated is redundant.
Thanks for updating the Fixes: tag.
Reviewed-by: Bryam Vargas <hexlabsecurity@proton.me>
Thanks,
Bryam
^ permalink raw reply
* Re: [PATCH v5 2/2] Input: isa1200 - new driver for Imagis ISA1200
From: Svyatoslav Ryhel @ 2026-06-17 6:52 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, devicetree, linux-kernel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Linus Walleij
In-Reply-To: <ajGk5YxRTFycFc1G@google.com>
вт, 16 черв. 2026 р. о 23:30 Dmitry Torokhov <dmitry.torokhov@gmail.com> пише:
>
> On Tue, Jun 16, 2026 at 09:45:25AM +0300, Svyatoslav Ryhel wrote:
> >
> > I have tested your code on my P895 and it works perfectly fine. Should
> > I resend with these changes or you can integrate them while picking
> > patchset?
>
> I think there was an update requested by Rob for the bindings?
>
Acknowledged, I will resend with both changes.
> >
> > Thank you for your suggestions and efforts!
>
> Thank you for your patience.
>
> --
> Dmitry
^ permalink raw reply
* [PATCH v6 0/2] input: misc: add support for Imagis ISA1200 haptic motor driver
From: Svyatoslav Ryhel @ 2026-06-17 7:05 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Linus Walleij, Svyatoslav Ryhel
Cc: linux-input, devicetree, linux-kernel
The ISA1200 is a haptic feedback unit from Imagis Technology using two
motors for haptic feedback in mobile phones. Used in many mobile devices
c. 2012 including Samsung Galxy S Advance GT-I9070 (Janice), Samsung Beam
GT-I8350 (Gavini), LG Optimus 4X P880 and LG Optimus Vu P895.
The exact datasheet for the ISA1200 is not available; all data was modeled
based on available downstream kernel sources for various devices and
fragments of information scattered across the internet.
---
Changes in v6:
- added minItems for gpios
- included changes by Dmitry Torokhov
Changes in v5:
- added supplies to private structure
- clk_on dropped
- ret > err
- added active flag to track status
- all hardware manipulations consolidated in start/stop
- dropped mutex from work
- dropped active check from isa1200_vibrator_close it was
moved to stop directly
- dropped hw maniplations from probe
- bustype set to BUS_I2C
- adjusted error strings
- fixed cancel_work_sync in isa1200_suspend
Changes in v4:
- added INPUT_FF_MEMLESS option selection
- fixed missing clock status set
- guard start/stop calls in isa1200_play_work with lock
- clamp ldo voltages to allowed range
- fixed imagis,pll-div parsing
- dropped Tested-by from schema adding commit
Changes in v3:
- added clock state tracking
- dropped level check in vibrator close
- added clkdiv clamping
- added comments regarding registers 5 and 6
Changes in v2:
- imagis,clk-div switched to accept actual divider value
- dropped DT header
- adjusted imagis,period-ns range
- initiated hctrl0 and hctrl1 values in isa1200_start
- fixed situation when PWM might return -EPROBE_DEFER to be
treated properly
- added chech a clock or PWM is available
- fixed regulator voltages check being off by 10
- added chech if state.period is not zero
- added action call to disable clock and gpios on error
- used managed version of work init
- added work cancel on suspend
- PW calls are done under mutex lock
---
Linus Walleij (1):
Input: isa1200 - new driver for Imagis ISA1200
Svyatoslav Ryhel (1):
dt-bindings: input: Document Imagis ISA1200 haptic motor driver
.../bindings/input/imagis,isa1200.yaml | 141 +++++
drivers/input/misc/Kconfig | 12 +
drivers/input/misc/Makefile | 1 +
drivers/input/misc/isa1200.c | 536 ++++++++++++++++++
4 files changed, 690 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/imagis,isa1200.yaml
create mode 100644 drivers/input/misc/isa1200.c
--
2.53.0
^ permalink raw reply
* [PATCH v6 1/2] dt-bindings: input: Document Imagis ISA1200 haptic motor driver
From: Svyatoslav Ryhel @ 2026-06-17 7:05 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Linus Walleij, Svyatoslav Ryhel
Cc: linux-input, devicetree, linux-kernel
In-Reply-To: <20260617070528.35006-1-clamor95@gmail.com>
Document the Imagis ISA1200 haptic motor driver, used primarily in mobile
handheld devices and capable of supporting up to two motors.
The exact datasheet for the ISA1200 is not available; all data was modeled
based on available downstream kernel sources for various devices and
fragments of information scattered across the internet.
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
.../bindings/input/imagis,isa1200.yaml | 141 ++++++++++++++++++
1 file changed, 141 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/imagis,isa1200.yaml
diff --git a/Documentation/devicetree/bindings/input/imagis,isa1200.yaml b/Documentation/devicetree/bindings/input/imagis,isa1200.yaml
new file mode 100644
index 000000000000..4bc8630edcdd
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/imagis,isa1200.yaml
@@ -0,0 +1,141 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/imagis,isa1200.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Imagis ISA1200 haptic motor driver
+
+maintainers:
+ - Svyatoslav Ryhel <clamor95@gmail.com>
+ - Linus Walleij <linusw@kernel.org>
+
+description:
+ The ISA1200 is a high-performance enhanced haptic motor driver designed
+ for mobile hand-held devices. It supports various voltages for both ERM
+ (Eccentric Rotating Mass) and LRA (Linear Resonant Actuator) type
+ actuators. Thanks to an embedded LDO, battery power can be used directly
+ in handheld applications.
+
+properties:
+ compatible:
+ const: imagis,isa1200
+
+ reg:
+ maxItems: 1
+
+ control-gpios:
+ description:
+ One or two GPIOs flagged as active high linked to HEN and LEN pins
+ minItems: 1
+ maxItems: 2
+
+ clocks:
+ maxItems: 1
+
+ pwms:
+ maxItems: 1
+
+ vdd-supply:
+ description:
+ Regulator for 2.4V - 5.5V power supply
+
+ vddp-supply:
+ description:
+ Regulator for 2.4V - 3.6V IO power supply
+
+ imagis,clk-div:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Divider for the external input clock/PWM
+ enum: [128, 256, 512, 1024]
+ default: 128
+
+ imagis,pll-div:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Divider for the internal PLL clock
+ minimum: 1
+ maximum: 15
+ default: 1
+
+ imagis,mode:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: |
+ Defines the motor type isa1200 drives
+ 0 - LRA (Linear Resonant Actuator)
+ 1 - ERM (Eccentric Rotating Mass)
+ enum: [0, 1]
+ default: 0
+
+ imagis,period-ns:
+ description:
+ Period of the internal PWM channel in nanoseconds.
+ minimum: 10000
+ maximum: 30000
+
+ imagis,duty-cycle-ns:
+ description:
+ Duty cycle of the external/internal PWM channel in nanoseconds,
+ defaults to 50% of the channel's period
+
+ ldo:
+ $ref: /schemas/regulator/regulator.yaml#
+ type: object
+ description:
+ Embedded LDO regulator with voltage range 2.3V - 3.8V
+ unevaluatedProperties: false
+
+ required:
+ - regulator-min-microvolt
+ - regulator-max-microvolt
+
+required:
+ - compatible
+ - reg
+ - ldo
+
+oneOf:
+ - required:
+ - clocks
+ - imagis,period-ns
+ - required:
+ - pwms
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ haptic-engine@49 {
+ compatible = "imagis,isa1200";
+ reg = <0x49>;
+
+ clocks = <&isa1200_refclk>;
+
+ control-gpios = <&gpio 22 GPIO_ACTIVE_HIGH>,
+ <&gpio 23 GPIO_ACTIVE_HIGH>;
+
+ vdd-supply = <&vdd_3v3_vbat>;
+ vddp-supply = <&vdd_2v8_vvib>;
+
+ imagis,clk-div = <256>;
+ imagis,pll-div = <2>;
+
+ imagis,mode = <0>; /* LRA_MODE */
+
+ imagis,period-ns = <13400>;
+ imagis,duty-cycle-ns = <100>;
+
+ ldo {
+ regulator-name = "vdd_vib";
+ regulator-min-microvolt = <2300000>;
+ regulator-max-microvolt = <2300000>;
+ };
+ };
+ };
--
2.53.0
^ permalink raw reply related
* [PATCH v6 2/2] Input: isa1200 - new driver for Imagis ISA1200
From: Svyatoslav Ryhel @ 2026-06-17 7:05 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Linus Walleij, Svyatoslav Ryhel
Cc: linux-input, devicetree, linux-kernel
In-Reply-To: <20260617070528.35006-1-clamor95@gmail.com>
From: Linus Walleij <linusw@kernel.org>
The ISA1200 is a haptic feedback unit from Imagis Technology using two
motors for haptic feedback in mobile phones. Used in many mobile devices
c. 2012 including Samsung Galxy S Advance GT-I9070 (Janice), Samsung Beam
GT-I8350 (Gavini), LG Optimus 4X P880 and LG Optimus Vu P895.
The exact datasheet for the ISA1200 is not available; all data was modeled
based on available downstream kernel sources for various devices and
fragments of information scattered across the internet.
Tested-by: Linus Walleij <linusw@kernel.org> # GT-I9070 Janice
Signed-off-by: Linus Walleij <linusw@kernel.org>
Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
drivers/input/misc/Kconfig | 12 +
drivers/input/misc/Makefile | 1 +
drivers/input/misc/isa1200.c | 536 +++++++++++++++++++++++++++++++++++
3 files changed, 549 insertions(+)
create mode 100644 drivers/input/misc/isa1200.c
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 1f6c57dba030..7154eaf5a60b 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -842,6 +842,18 @@ config INPUT_IQS7222
To compile this driver as a module, choose M here: the
module will be called iqs7222.
+config INPUT_ISA1200_HAPTIC
+ tristate "Imagis ISA1200 haptic feedback unit"
+ depends on I2C
+ select INPUT_FF_MEMLESS
+ select REGMAP_I2C
+ help
+ Say Y to enable support for the Imagis ISA1200 haptic
+ feedback unit.
+
+ To compile this driver as a module, choose M here: the
+ module will be called isa1200.
+
config INPUT_CMA3000
tristate "VTI CMA3000 Tri-axis accelerometer"
help
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 2281d6803fce..e9f85ca20c33 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_INPUT_IMS_PCU) += ims-pcu.o
obj-$(CONFIG_INPUT_IQS269A) += iqs269a.o
obj-$(CONFIG_INPUT_IQS626A) += iqs626a.o
obj-$(CONFIG_INPUT_IQS7222) += iqs7222.o
+obj-$(CONFIG_INPUT_ISA1200_HAPTIC) += isa1200.o
obj-$(CONFIG_INPUT_KEYSPAN_REMOTE) += keyspan_remote.o
obj-$(CONFIG_INPUT_KXTJ9) += kxtj9.o
obj-$(CONFIG_INPUT_M68K_BEEP) += m68kspkr.o
diff --git a/drivers/input/misc/isa1200.c b/drivers/input/misc/isa1200.c
new file mode 100644
index 000000000000..f8c4da4a0e08
--- /dev/null
+++ b/drivers/input/misc/isa1200.c
@@ -0,0 +1,536 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/array_size.h>
+#include <linux/bitmap.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/devm-helpers.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/units.h>
+
+/*
+ * System control (LDO regulator)
+ *
+ * LDO voltage to register mapping is linear, but it is split in two parts:
+ * 2.3V - 3.0V map to 0x08 - 0x0f; 3.1V - 3.8V map to 0x00 - 0x7
+ */
+
+#define ISA1200_SCTRL 0x00
+#define ISA1200_LDO_VOLTAGE_BASE 0x08
+#define ISA1200_LDO_VOLTAGE_STEP 100000
+#define ISA1200_LDO_VOLTAGE_2V3 23
+#define ISA1200_LDO_VOLTAGE_3V1 31
+#define ISA1200_LDO_VOLTAGE_MIN 2300000
+#define ISA1200_LDO_VOLTAGE_MAX 3800000
+
+/*
+ * The output frequency is calculated with this formula:
+ *
+ * base clock frequency
+ * fout = -----------------------------------------
+ * (128 - PWM_FREQ) * 2 * PLLDIV * PWM_PERIOD
+ *
+ * The base clock frequency is the clock frequency provided on the
+ * clock input to the chip, divided by the value in HCTRL0
+ *
+ * PWM_FREQ is configured in register HCTRL4, it is common to set this
+ * to 0 to get only two variables to calculate.
+ *
+ * PLLDIV is configured in register HCTRL3 (bits 7..4, so 0..15)
+ * PWM_PERIOD is configured in register HCTRL6
+ * Further the duty cycle can be configured in HCTRL5
+ */
+
+/*
+ * HCTRL0 configures clock or PWM input and selects the divider for
+ * the clock input.
+ */
+#define ISA1200_HCTRL0 0x30
+#define ISA1200_HCTRL0_HAP_ENABLE BIT(7)
+#define ISA1200_HCTRL0_PWM_GEN_MODE BIT(4)
+#define ISA1200_HCTRL0_PWM_INPUT_MODE BIT(3)
+#define ISA1200_HCTRL0_CLKDIV_128 128
+
+/*
+ * HCTRL1 configures the motor type and clock sourse
+ */
+#define ISA1200_HCTRL1 0x31
+#define ISA1200_HCTRL1_EXT_CLOCK BIT(7)
+#define ISA1200_HCTRL1_DAC_INVERT BIT(6)
+#define ISA1200_HCTRL1_MODE(n) (((n) & 1) << 5)
+
+/* HCTRL2 controls software reset of the chip */
+#define ISA1200_HCTRL2 0x32
+#define ISA1200_HCTRL2_SW_RESET BIT(0)
+
+/*
+ * HCTRL3 controls the PLL divisor
+ *
+ * Bits [0,1] are always set to 1 (we don't know what they are
+ * used for) and bit 4 and upward control the PLL divisor.
+ */
+#define ISA1200_HCTRL3 0x33
+#define ISA1200_HCTRL3_DEFAULT 0x03
+#define ISA1200_HCTRL3_PLLDIV(n) (((n) & 0xf) << 4)
+
+/* HCTRL4 controls the PWM frequency of external channel */
+#define ISA1200_HCTRL4 0x34
+
+/* HCTRL5 controls the PWM high duty cycle of internal channel */
+#define ISA1200_HCTRL5 0x35
+
+/* HCTRL6 controls the PWM period of internal channel */
+#define ISA1200_HCTRL6 0x36
+#define ISA1200_HCTRL6_PERIOD_SCALE 100
+
+/* The use for these registers is unknown but they exist */
+#define ISA1200_HCTRL7 0x37
+#define ISA1200_HCTRL8 0x38
+#define ISA1200_HCTRL9 0x39
+#define ISA1200_HCTRLA 0x3a
+#define ISA1200_HCTRLB 0x3b
+#define ISA1200_HCTRLC 0x3c
+#define ISA1200_HCTRLD 0x3d
+
+#define ISA1200_EN_PINS_MAX 2
+
+static const struct regulator_bulk_data isa1200_supplies[] = {
+ { .supply = "vdd" }, { .supply = "vddp" },
+};
+
+struct isa1200_config {
+ u32 ldo_voltage;
+ u32 mode;
+ u32 clkdiv;
+ u32 plldiv;
+ u32 freq;
+ u32 period;
+ u32 duty;
+};
+
+struct isa1200 {
+ struct input_dev *input;
+ struct regmap *map;
+
+ struct clk *clk;
+ struct pwm_device *pwm;
+ struct gpio_descs *enable_gpios;
+ struct regulator_bulk_data *supplies;
+
+ struct work_struct play_work;
+ struct isa1200_config config;
+
+ int level;
+ bool suspended;
+ bool active;
+};
+
+static const struct regmap_config isa1200_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = ISA1200_HCTRLD,
+};
+
+static void isa1200_start(struct isa1200 *isa)
+{
+ struct isa1200_config *config = &isa->config;
+ struct device *dev = &isa->input->dev;
+ struct pwm_state state;
+ u8 hctrl0 = 0, hctrl1 = 0;
+ DECLARE_BITMAP(values, ISA1200_EN_PINS_MAX);
+ int err;
+
+ if (!isa->active) {
+ err = regulator_bulk_enable(ARRAY_SIZE(isa1200_supplies),
+ isa->supplies);
+ if (err) {
+ dev_err(dev, "failed to enable supplies (%d)\n", err);
+ return;
+ }
+
+ err = clk_prepare_enable(isa->clk);
+ if (err) {
+ dev_err(dev, "failed to enable clock (%d)\n", err);
+ regulator_bulk_disable(ARRAY_SIZE(isa1200_supplies),
+ isa->supplies);
+ return;
+ }
+
+ bitmap_fill(values, ISA1200_EN_PINS_MAX);
+ gpiod_multi_set_value_cansleep(isa->enable_gpios, values);
+
+ usleep_range(200, 300);
+ }
+
+ regmap_write(isa->map, ISA1200_SCTRL, config->ldo_voltage);
+
+ if (isa->clk) {
+ hctrl0 = ISA1200_HCTRL0_PWM_GEN_MODE;
+ hctrl1 = ISA1200_HCTRL1_EXT_CLOCK;
+ }
+
+ if (isa->pwm) {
+ hctrl0 = ISA1200_HCTRL0_PWM_INPUT_MODE;
+ hctrl1 = 0;
+ }
+
+ hctrl0 |= __ffs(config->clkdiv / ISA1200_HCTRL0_CLKDIV_128);
+ hctrl1 |= ISA1200_HCTRL1_DAC_INVERT;
+ hctrl1 |= ISA1200_HCTRL1_MODE(config->mode);
+
+ regmap_write(isa->map, ISA1200_HCTRL0, hctrl0);
+ regmap_write(isa->map, ISA1200_HCTRL1, hctrl1);
+
+ /* Make sure to de-assert software reset */
+ regmap_write(isa->map, ISA1200_HCTRL2, 0x00);
+
+ /* PLL divisor */
+ regmap_write(isa->map, ISA1200_HCTRL3,
+ ISA1200_HCTRL3_PLLDIV(config->plldiv) |
+ ISA1200_HCTRL3_DEFAULT);
+
+ /* Frequency */
+ regmap_write(isa->map, ISA1200_HCTRL4, config->freq);
+ /* Duty cycle */
+ regmap_write(isa->map, ISA1200_HCTRL5, config->period >> 1);
+ /* Period */
+ regmap_write(isa->map, ISA1200_HCTRL6, config->period);
+
+ hctrl0 |= ISA1200_HCTRL0_HAP_ENABLE;
+ regmap_write(isa->map, ISA1200_HCTRL0, hctrl0);
+
+ if (isa->clk)
+ regmap_write(isa->map, ISA1200_HCTRL5, config->duty);
+
+ if (isa->pwm) {
+ pwm_get_state(isa->pwm, &state);
+ state.duty_cycle = config->duty;
+ state.enabled = true;
+ pwm_apply_might_sleep(isa->pwm, &state);
+ }
+
+ isa->active = true;
+}
+
+static void isa1200_stop(struct isa1200 *isa)
+{
+ struct pwm_state state;
+ DECLARE_BITMAP(values, ISA1200_EN_PINS_MAX);
+
+ if (!isa->active)
+ return;
+
+ if (isa->pwm) {
+ pwm_get_state(isa->pwm, &state);
+ state.duty_cycle = 0;
+ state.enabled = false;
+ pwm_apply_might_sleep(isa->pwm, &state);
+ }
+
+ regmap_write(isa->map, ISA1200_HCTRL0, 0x00);
+
+ bitmap_zero(values, ISA1200_EN_PINS_MAX);
+ gpiod_multi_set_value_cansleep(isa->enable_gpios, values);
+
+ clk_disable_unprepare(isa->clk);
+ regulator_bulk_disable(ARRAY_SIZE(isa1200_supplies),
+ isa->supplies);
+
+ isa->active = false;
+}
+
+static void isa1200_play_work(struct work_struct *work)
+{
+ struct isa1200 *isa = container_of(work, struct isa1200, play_work);
+ struct input_dev *input = isa->input;
+
+ scoped_guard(mutex_try, &input->mutex) {
+ if (!isa->suspended) {
+ if (isa->level)
+ isa1200_start(isa);
+ else
+ isa1200_stop(isa);
+ }
+ }
+}
+
+static int isa1200_vibrator_play_effect(struct input_dev *input, void *data,
+ struct ff_effect *effect)
+{
+ struct isa1200 *isa = input_get_drvdata(input);
+ int level;
+
+ /*
+ * TODO: we currently only support rumble.
+ * The ISA1200 can control two motors and some devices
+ * also have two motors mounted.
+ */
+ level = effect->u.rumble.strong_magnitude;
+ if (!level)
+ level = effect->u.rumble.weak_magnitude;
+
+ dev_dbg(&input->dev, "FF effect type %d level %d\n",
+ effect->type, level);
+
+ if (isa->level != level) {
+ isa->level = level;
+ if (!READ_ONCE(isa->suspended))
+ schedule_work(&isa->play_work);
+ }
+
+ return 0;
+}
+
+static void isa1200_vibrator_close(struct input_dev *input)
+{
+ struct isa1200 *isa = input_get_drvdata(input);
+
+ cancel_work_sync(&isa->play_work);
+ isa1200_stop(isa);
+ isa->level = 0;
+}
+
+static int isa1200_of_probe(struct i2c_client *client)
+{
+ struct isa1200 *isa = i2c_get_clientdata(client);
+ struct isa1200_config *config = &isa->config;
+ struct device *dev = &client->dev;
+ struct fwnode_handle *ldo_node;
+ int err;
+
+ isa->clk = devm_clk_get_optional(dev, NULL);
+ if (IS_ERR(isa->clk))
+ return dev_err_probe(dev, PTR_ERR(isa->clk),
+ "failed to get clock\n");
+
+ isa->pwm = devm_pwm_get(dev, NULL);
+ if (IS_ERR(isa->pwm)) {
+ err = PTR_ERR(isa->pwm);
+ if (err == -ENODEV || err == -EINVAL)
+ isa->pwm = NULL;
+ else
+ return dev_err_probe(dev, err, "getting PWM\n");
+ }
+
+ if (!isa->clk && !isa->pwm)
+ return dev_err_probe(dev, -EINVAL,
+ "clock or PWM are required, none were provided\n");
+
+ err = devm_regulator_bulk_get_const(dev, ARRAY_SIZE(isa1200_supplies),
+ isa1200_supplies, &isa->supplies);
+ if (err)
+ return dev_err_probe(dev, err, "failed to get supplies\n");
+
+ isa->enable_gpios = devm_gpiod_get_array_optional(dev, "control",
+ GPIOD_OUT_LOW);
+ if (IS_ERR(isa->enable_gpios))
+ return dev_err_probe(dev, PTR_ERR(isa->enable_gpios),
+ "failed to get enable gpios\n");
+
+ if (isa->enable_gpios && isa->enable_gpios->ndescs > ISA1200_EN_PINS_MAX)
+ return dev_err_probe(dev, -EINVAL, "too many enable gpios\n");
+
+ ldo_node = device_get_named_child_node(dev, "ldo");
+ if (!ldo_node)
+ return dev_err_probe(dev, -ENODEV,
+ "failed to get embedded LDO node\n");
+
+ err = fwnode_property_read_u32(ldo_node, "regulator-min-microvolt",
+ &config->ldo_voltage);
+ fwnode_handle_put(ldo_node);
+ if (err)
+ return dev_err_probe(dev, err,
+ "failed to get ldo voltage\n");
+
+ config->ldo_voltage = clamp(config->ldo_voltage,
+ ISA1200_LDO_VOLTAGE_MIN,
+ ISA1200_LDO_VOLTAGE_MAX);
+
+ config->ldo_voltage /= ISA1200_LDO_VOLTAGE_STEP;
+ if (config->ldo_voltage < ISA1200_LDO_VOLTAGE_3V1)
+ config->ldo_voltage = config->ldo_voltage -
+ ISA1200_LDO_VOLTAGE_2V3 +
+ ISA1200_LDO_VOLTAGE_BASE;
+ else
+ config->ldo_voltage -= ISA1200_LDO_VOLTAGE_3V1;
+
+ config->mode = 0; /* LRA_MODE */
+ device_property_read_u32(dev, "imagis,mode", &config->mode);
+
+ config->clkdiv = ISA1200_HCTRL0_CLKDIV_128;
+ device_property_read_u32(dev, "imagis,clk-div", &config->clkdiv);
+ if (!config->clkdiv)
+ return dev_err_probe(dev, -EINVAL, "clk-div cannot be zero\n");
+
+ config->clkdiv = clamp(config->clkdiv, ISA1200_HCTRL0_CLKDIV_128,
+ ISA1200_HCTRL0_CLKDIV_128 << 3);
+
+ err = device_property_read_u32(dev, "imagis,pll-div", &config->plldiv);
+ if (err || !config->plldiv)
+ config->plldiv = 1;
+
+ config->period = 0;
+ config->freq = 0;
+ config->duty = 0;
+
+ if (isa->clk) {
+ err = device_property_read_u32(dev, "imagis,period-ns",
+ &config->period);
+ if (err)
+ return dev_err_probe(dev, err,
+ "failed to get period\n");
+
+ /*
+ * TODO: The scale value is arbitrary, but it fits observations
+ * quite well, and the exact conversion method is unknown.
+ * The period property value returned above is the HCTRL6
+ * register value set by the vendor code, multiplied by 100.
+ */
+ config->period /= ISA1200_HCTRL6_PERIOD_SCALE;
+ config->duty = config->period >> 1;
+ }
+
+ if (isa->pwm) {
+ struct pwm_state state;
+
+ pwm_init_state(isa->pwm, &state);
+
+ if (!state.period)
+ return dev_err_probe(dev, -EINVAL,
+ "PWM period cannot be zero\n");
+
+ config->freq = div64_u64(NANO, state.period * config->clkdiv);
+ config->duty = state.period >> 1;
+
+ err = pwm_apply_might_sleep(isa->pwm, &state);
+ if (err)
+ return dev_err_probe(dev, err,
+ "failed to apply initial PWM state\n");
+ }
+
+ /*
+ * TODO: If device is using a clock, this property should return the
+ * value written to the HCTRL5 register by downstrem code. It likely
+ * needs to be converted into a meaningful duty cycle value, though
+ * unfortunately the exact conversion mechanism is unknown. If the
+ * device uses PWM, this property will return the correct duty cycle
+ * in nanoseconds.
+ */
+ device_property_read_u32(dev, "imagis,duty-cycle-ns", &config->duty);
+
+ return 0;
+}
+
+static int isa1200_probe(struct i2c_client *client)
+{
+ struct isa1200 *isa;
+ struct device *dev = &client->dev;
+ int err;
+
+ isa = devm_kzalloc(dev, sizeof(*isa), GFP_KERNEL);
+ if (!isa)
+ return -ENOMEM;
+
+ isa->input = devm_input_allocate_device(dev);
+ if (!isa->input)
+ return -ENOMEM;
+
+ i2c_set_clientdata(client, isa);
+
+ err = isa1200_of_probe(client);
+ if (err)
+ return err;
+
+ isa->map = devm_regmap_init_i2c(client, &isa1200_regmap_config);
+ if (IS_ERR(isa->map))
+ return dev_err_probe(dev, PTR_ERR(isa->map),
+ "failed to initialize register map\n");
+
+ INIT_WORK(&isa->play_work, isa1200_play_work);
+
+ isa->input->name = "isa1200-haptic";
+ isa->input->id.bustype = BUS_I2C;
+ isa->input->close = isa1200_vibrator_close;
+
+ isa->active = false;
+
+ input_set_drvdata(isa->input, isa);
+
+ /* TODO: this hardware can likely support more than rumble */
+ input_set_capability(isa->input, EV_FF, FF_RUMBLE);
+
+ err = input_ff_create_memless(isa->input, NULL,
+ isa1200_vibrator_play_effect);
+ if (err)
+ return dev_err_probe(dev, err, "failed to create FF dev\n");
+
+ err = input_register_device(isa->input);
+ if (err)
+ return dev_err_probe(dev, err, "failed to register input dev\n");
+
+ return 0;
+}
+
+static int isa1200_suspend(struct device *dev)
+{
+ struct isa1200 *isa = dev_get_drvdata(dev);
+
+ guard(mutex)(&isa->input->mutex);
+
+ if (input_device_enabled(isa->input)) {
+ WRITE_ONCE(isa->suspended, true);
+ cancel_work_sync(&isa->play_work);
+ isa1200_stop(isa);
+ }
+
+ return 0;
+}
+
+static int isa1200_resume(struct device *dev)
+{
+ struct isa1200 *isa = dev_get_drvdata(dev);
+
+ guard(mutex)(&isa->input->mutex);
+
+ if (input_device_enabled(isa->input)) {
+ WRITE_ONCE(isa->suspended, false);
+ if (isa->level)
+ schedule_work(&isa->play_work);
+ }
+
+ return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(isa1200_pm_ops, isa1200_suspend, isa1200_resume);
+
+static const struct of_device_id isa1200_of_match[] = {
+ { .compatible = "imagis,isa1200" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, isa1200_of_match);
+
+static struct i2c_driver isa1200_i2c_driver = {
+ .driver = {
+ .name = "isa1200",
+ .of_match_table = isa1200_of_match,
+ .pm = pm_sleep_ptr(&isa1200_pm_ops),
+ },
+ .probe = isa1200_probe,
+};
+module_i2c_driver(isa1200_i2c_driver);
+
+MODULE_AUTHOR("Linus Walleij <linusw@kernel.org>");
+MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
+MODULE_DESCRIPTION("Imagis ISA1200 haptic feedback unit");
+MODULE_LICENSE("GPL");
--
2.53.0
^ permalink raw reply related
* Re: [PATCH v6 2/2] Input: isa1200 - new driver for Imagis ISA1200
From: sashiko-bot @ 2026-06-17 7:18 UTC (permalink / raw)
To: Svyatoslav Ryhel; +Cc: robh, devicetree, linux-input, conor+dt, dmitry.torokhov
In-Reply-To: <20260617070528.35006-3-clamor95@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The `ff-memless` timer and input device can outlive the devres-managed driver state upon device unbind, leading to a Use-After-Free (UAF).
- [High] The worker `isa1200_play_work` silently drops force feedback events if it fails to acquire `input->mutex`, potentially leaving the vibrator stuck.
- [Medium] Concurrent access to `isa->level` across different locking contexts causes a data race that can schedule work after the device is closed.
--
commit 2ea8a9b82f2921004386e7307e4e382941aa5611
Author: Linus Walleij <linusw@kernel.org>
Input: isa1200 - new driver for Imagis ISA1200
The ISA1200 is a haptic feedback unit from Imagis Technology using two
motors for haptic feedback in mobile phones. Used in many mobile devices
c. 2012 including Samsung Galxy S Advance GT-I9070 (Janice), Samsung Beam
GT-I8350 (Gavini), LG Optimus 4X P880 and LG Optimus Vu P895.
> diff --git a/drivers/input/misc/isa1200.c b/drivers/input/misc/isa1200.c
> new file mode 100644
> index 0000000000000..f8c4da4a0e088
> --- /dev/null
> +++ b/drivers/input/misc/isa1200.c
[ ... ]
> +static void isa1200_play_work(struct work_struct *work)
> +{
> + struct isa1200 *isa = container_of(work, struct isa1200, play_work);
> + struct input_dev *input = isa->input;
> +
> + scoped_guard(mutex_try, &input->mutex) {
[Severity: High]
Can this cause force feedback events to be silently dropped?
The worker uses scoped_guard(mutex_try, &input->mutex) to acquire the lock.
However, input->mutex is frequently held by the input core for normal
operations like input_open_device, input_close_device, and ioctls
(e.g., EVIOCGABS).
If this worker runs concurrently with any of these operations, the mutex_try
will fail and the worker will exit without updating the hardware. Because
isa->level was already updated locklessly in isa1200_vibrator_play_effect(),
future identical effect updates will be skipped, which could leave the
vibrator stuck in its current state.
> + if (!isa->suspended) {
> + if (isa->level)
> + isa1200_start(isa);
> + else
> + isa1200_stop(isa);
> + }
> + }
> +}
[ ... ]
> +static void isa1200_vibrator_close(struct input_dev *input)
> +{
> + struct isa1200 *isa = input_get_drvdata(input);
> +
> + cancel_work_sync(&isa->play_work);
> + isa1200_stop(isa);
> + isa->level = 0;
[Severity: Medium]
Does this concurrent modification of isa->level create a data race that
can restart the vibrator after the device is closed?
The state variable isa->level is updated in isa1200_vibrator_play_effect()
under the input core's event_lock (timer context). Concurrently, it is reset
to 0 here in isa1200_vibrator_close() under input->mutex, without acquiring
event_lock.
Because the ff-memless timer is not explicitly stopped during close, the
timer can fire concurrently with or immediately after close. If it fires
after cancel_work_sync(&isa->play_work) completes, it can overwrite
isa->level to a non-zero value and re-schedule isa->play_work. This could
re-enable the motor indefinitely.
> +}
[ ... ]
> +static int isa1200_probe(struct i2c_client *client)
> +{
> + struct isa1200 *isa;
> + struct device *dev = &client->dev;
> + int err;
> +
> + isa = devm_kzalloc(dev, sizeof(*isa), GFP_KERNEL);
[Severity: Critical]
Is there a use-after-free risk here during device unbind?
The private isa structure is allocated with devm_kzalloc(), meaning it is
freed immediately when the device unbinds. However, the input device itself
is managed by devm_input_allocate_device(), which keeps the memory alive
as long as userspace holds an open file descriptor.
Because the ff-memless timer (ml_effect_timer) is tied to the input_dev
lifecycle, the timer can continue to fire even after unbind if an effect
was playing. When it fires, isa1200_vibrator_play_effect() retrieves the
freed isa structure via input_get_drvdata() and accesses corrupted memory
to schedule the workqueue.
Additionally, there doesn't appear to be a devm action added to explicitly
cancel isa->play_work on unbind.
> + if (!isa)
> + return -ENOMEM;
> +
> + isa->input = devm_input_allocate_device(dev);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617070528.35006-1-clamor95@gmail.com?part=2
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox