* Re: [PATCH 4/6] Input: mms114 - replace udelay with usleep_range
From: sashiko-bot @ 2026-06-16 5:20 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260616050912.1531241-4-dmitry.torokhov@gmail.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [High] Race condition in `mms114_suspend` releases touches before disabling the hardware IRQ, potentially leaving touches stuck active.
- [High] Failure in `mms114_resume()` leads to unbalanced `disable_irq()` and `regulator_disable()`, causing kernel warnings and state corruption.
- [High] Copy-paste error in `mms114_setup_regs` writes the X resolution to the Y resolution register, breaking touchscreen coordinates mapping.
- [High] Hardware data bytes are read directly into a struct containing C bitfields, leading to endian-dependent memory layout corruption.
--
commit 4b0151507f5071f8f96d0af3ccffe9ae30a17c92
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Input: mms114 - replace udelay with usleep_range
The driver currently uses udelay(MMS114_I2C_DELAY) (50us) to ensure a
mandatory delay between I2C transfers in __mms114_read_reg() and
mms114_write_reg().
Both functions invoke underlying I2C core operations (i2c_transfer,
i2c_master_send) which acquire mutexes and sleep. Furthermore, the
interrupt handler mms114_interrupt() is registered as a threaded IRQ
handler. Since the entire execution path is fully sleepable,
busy-waiting with udelay() for 50us unnecessarily wastes CPU cycles.
Replace udelay() with usleep_range() to allow the CPU to enter low-power
states or execute other tasks during the delay.
> diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> index c2e006ac11960..c59aec8f2febe 100644
> --- a/drivers/input/touchscreen/mms114.c
> +++ b/drivers/input/touchscreen/mms114.c
> @@ -116,7 +116,7 @@ static int __mms114_read_reg(struct mms114_data *data, u8 reg,
> "%s: i2c transfer failed (%d)\n", __func__, error);
> return error < 0 ? error : -EIO;
> }
> - udelay(MMS114_I2C_DELAY);
> + usleep_range(MMS114_I2C_DELAY, MMS114_I2C_DELAY + 50);
>
> return 0;
> }
[ ... ]
[Severity: High]
This is a pre-existing issue, but I noticed a potential race condition in the
suspend path.
drivers/input/touchscreen/mms114.c:mms114_suspend() {
...
/* Release all touch */
for (id = 0; id < MMS114_MAX_TOUCH; id++) {
input_mt_slot(input_dev, id);
input_mt_report_slot_inactive(input_dev);
}
input_mt_report_pointer_emulation(input_dev, true);
input_sync(input_dev);
guard(mutex)(&input_dev->mutex);
if (input_device_enabled(input_dev))
mms114_stop(data);
...
}
Because mms114_stop() disables the hardware IRQ after input_sync() has reported
the release events, is it possible for a hardware interrupt to fire between
the synthetic release and the actual IRQ disable?
Since the interrupt handler runs as a concurrent kernel thread and does not
share a lock with this early part of the suspend path, the touch could be
reported as active again right before the system sleeps. Would this leave the
touch stuck active indefinitely upon resume?
[Severity: High]
This isn't a bug introduced by this patch, but the resume error handling
might lead to unbalanced resource disabling.
drivers/input/touchscreen/mms114.c:mms114_resume() {
...
if (input_device_enabled(input_dev)) {
error = mms114_start(data);
if (error)
return error;
}
...
}
If mms114_start() fails (e.g. due to I2C communication failure), it disables
the regulators and returns an error without enabling the IRQ. However, the
input device remains logically open.
If userspace subsequently closes the device, or another suspend cycle occurs,
mms114_stop() will be called unconditionally:
drivers/input/touchscreen/mms114.c:mms114_stop() {
...
disable_irq(client->irq);
error = regulator_disable(data->io_reg);
...
}
Will this unconditionally call disable_irq() and regulator_disable() on
resources that are already disabled, triggering warnings and corrupting
regulator reference counts?
[Severity: High]
This is a pre-existing issue, but I spotted a potential copy-paste error
during resolution configuration.
drivers/input/touchscreen/mms114.c:mms114_setup_regs() {
...
val = props->max_x & 0xff;
error = mms114_write_reg(data, MMS114_X_RESOLUTION, val);
if (error < 0)
return error;
val = props->max_x & 0xff;
error = mms114_write_reg(data, MMS114_Y_RESOLUTION, val);
...
}
Should the second register write use props->max_y instead of props->max_x?
Writing the X resolution to MMS114_Y_RESOLUTION might truncate the hardware's
internal Y resolution, breaking touch tracking and coordinate mapping on the
Y axis for screens where the dimensions differ.
[Severity: High]
This isn't a bug introduced by this patch, but there might be an endianness
problem with the touch data structure.
drivers/input/touchscreen/mms114.c:
...
struct mms114_touch {
u8 id:4, reserved_bit4:1, type:2, pressed:1;
u8 x_hi:4, y_hi:4;
u8 x_lo;
u8 y_lo;
u8 width;
u8 strength;
u8 reserved[2];
} __packed;
...
This struct is used to map raw byte stream data from the I2C hardware. Since
the memory layout of C bitfields is compiler and endian-dependent, will these
bitfields be reversed on big-endian architectures?
This could result in incorrect parsing of touch coordinates, IDs, and event
types.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616050912.1531241-1-dmitry.torokhov@gmail.com?part=4
^ permalink raw reply
* [PATCH 4/4] Input: sur40 - fix V4L2 video device lifetime
From: Dmitry Torokhov @ 2026-06-16 5:12 UTC (permalink / raw)
To: Hans Verkuil, linux-input; +Cc: linux-kernel, sashiko-bot, stable
In-Reply-To: <20260616051235.1549517-1-dmitry.torokhov@gmail.com>
sur40_disconnect() synchronously frees the sur40_state structure (kfree(sur40))
while userspace might still hold an open file descriptor to the V4L2 video
device node. When userspace later accesses or closes the lingering file
descriptor, the V4L2 core invokes file operations (such as vb2_fop_release)
that dereference the already freed sur40 memory, resulting in a use-after-free
vulnerability.
Fix this by implementing a V4L2 release callback (sur40_video_release) in
sur40_video_device to clean up V4L2 components and free the sur40 structure
only when the last video file descriptor is closed.
Additionally, update the sur40_probe() error path to call video_unregister_device()
and return inline if input initialization fails after video device registration
succeeded, allowing the V4L2 release callback to manage cleanup.
Also, call v4l2_device_disconnect() in sur40_disconnect() to safely dissociate
the V4L2 device from the parent USB device during unplug.
Reported-by: sashiko-bot@kernel.org
Cc: stable@vger.kernel.org
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/sur40.c | 27 ++++++++++++++++-----------
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index 1ad68131e3a6..2f0efee23d1e 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -806,8 +806,10 @@ static int sur40_probe(struct usb_interface *interface,
}
error = sur40_init_input(sur40);
- if (error)
- goto err_unreg_video;
+ if (error) {
+ video_unregister_device(&sur40->vdev);
+ return error;
+ }
/* we can register the device now, as it is ready */
usb_set_intfdata(interface, sur40);
@@ -815,8 +817,6 @@ static int sur40_probe(struct usb_interface *interface,
return 0;
-err_unreg_video:
- video_unregister_device(&sur40->vdev);
err_free_ctrl:
v4l2_ctrl_handler_free(&sur40->hdl);
err_unreg_v4l2:
@@ -835,13 +835,8 @@ static void sur40_disconnect(struct usb_interface *interface)
struct sur40_state *sur40 = usb_get_intfdata(interface);
input_unregister_device(sur40->input);
-
- v4l2_ctrl_handler_free(&sur40->hdl);
video_unregister_device(&sur40->vdev);
- v4l2_device_unregister(&sur40->v4l2);
-
- kfree(sur40->bulk_in_buffer);
- kfree(sur40);
+ v4l2_device_disconnect(&sur40->v4l2);
usb_set_intfdata(interface, NULL);
dev_dbg(&interface->dev, "%s is now disconnected\n", DRIVER_DESC);
@@ -1176,11 +1171,21 @@ static const struct v4l2_ioctl_ops sur40_video_ioctl_ops = {
.vidioc_streamoff = vb2_ioctl_streamoff,
};
+static void sur40_video_release(struct video_device *vdev)
+{
+ struct sur40_state *sur40 = video_get_drvdata(vdev);
+
+ v4l2_device_unregister(&sur40->v4l2);
+ v4l2_ctrl_handler_free(&sur40->hdl);
+ kfree(sur40->bulk_in_buffer);
+ kfree(sur40);
+}
+
static const struct video_device sur40_video_device = {
.name = DRIVER_LONG,
.fops = &sur40_video_fops,
.ioctl_ops = &sur40_video_ioctl_ops,
- .release = video_device_release_empty,
+ .release = sur40_video_release,
.device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_TOUCH |
V4L2_CAP_READWRITE | V4L2_CAP_STREAMING,
};
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* [PATCH 3/4] Input: sur40 - factor out and move input device initialization
From: Dmitry Torokhov @ 2026-06-16 5:12 UTC (permalink / raw)
To: Hans Verkuil, linux-input; +Cc: linux-kernel, sashiko-bot, stable
In-Reply-To: <20260616051235.1549517-1-dmitry.torokhov@gmail.com>
The input device allocation, setup, and registration in sur40_probe() is
quite verbose. Factor it out into a helper function sur40_init_input() to
improve readability.
Additionally, call this helper at the very end of sur40_probe() instead of
allocating the input device early. This ensures all video components are fully
initialized before the input device is registered (which starts polling),
and simplifies the early probe error paths since we don't have to carry and
free the input device if probe fails during early V4L2 setup.
Reported-by: sashiko-bot@kernel.org
Cc: stable@vger.kernel.org
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/sur40.c | 91 +++++++++++++++++--------------
1 file changed, 50 insertions(+), 41 deletions(-)
diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index e9089b0c3e2f..1ad68131e3a6 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -647,6 +647,53 @@ static int sur40_input_setup_events(struct input_dev *input_dev)
return 0;
}
+static int sur40_init_input(struct sur40_state *sur40)
+{
+ struct input_dev *input;
+ int error;
+
+ input = input_allocate_device();
+ if (!input)
+ return -ENOMEM;
+
+ /* Set up regular input device structure */
+ input->name = DRIVER_LONG;
+ usb_to_input_id(sur40->usbdev, &input->id);
+ usb_make_path(sur40->usbdev, sur40->phys, sizeof(sur40->phys));
+ strlcat(sur40->phys, "/input0", sizeof(sur40->phys));
+ input->phys = sur40->phys;
+ input->dev.parent = sur40->dev;
+
+ input->open = sur40_open;
+ input->close = sur40_close;
+
+ error = sur40_input_setup_events(input);
+ if (error)
+ goto err_free_input;
+
+ input_set_drvdata(input, sur40);
+ error = input_setup_polling(input, sur40_poll);
+ if (error) {
+ dev_err(sur40->dev, "failed to set up polling\n");
+ goto err_free_input;
+ }
+
+ input_set_poll_interval(input, POLL_INTERVAL);
+
+ error = input_register_device(input);
+ if (error) {
+ dev_err(sur40->dev, "Unable to register polled input device.\n");
+ goto err_free_input;
+ }
+
+ sur40->input = input;
+ return 0;
+
+err_free_input:
+ input_free_device(input);
+ return error;
+}
+
/* Check candidate USB interface. */
static int sur40_probe(struct usb_interface *interface,
const struct usb_device_id *id)
@@ -655,7 +702,6 @@ static int sur40_probe(struct usb_interface *interface,
struct sur40_state *sur40;
struct usb_host_interface *iface_desc;
struct usb_endpoint_descriptor *endpoint;
- struct input_dev *input;
int error;
/* Check if we really have the right interface. */
@@ -676,44 +722,13 @@ static int sur40_probe(struct usb_interface *interface,
if (!sur40)
return -ENOMEM;
- input = input_allocate_device();
- if (!input) {
- error = -ENOMEM;
- goto err_free_dev;
- }
-
/* initialize locks/lists */
INIT_LIST_HEAD(&sur40->buf_list);
spin_lock_init(&sur40->qlock);
mutex_init(&sur40->lock);
- /* Set up regular input device structure */
- input->name = DRIVER_LONG;
- usb_to_input_id(usbdev, &input->id);
- usb_make_path(usbdev, sur40->phys, sizeof(sur40->phys));
- strlcat(sur40->phys, "/input0", sizeof(sur40->phys));
- input->phys = sur40->phys;
- input->dev.parent = &interface->dev;
-
- input->open = sur40_open;
- input->close = sur40_close;
-
- error = sur40_input_setup_events(input);
- if (error)
- goto err_free_input;
-
- input_set_drvdata(input, sur40);
- error = input_setup_polling(input, sur40_poll);
- if (error) {
- dev_err(&interface->dev, "failed to set up polling");
- goto err_free_input;
- }
-
- input_set_poll_interval(input, POLL_INTERVAL);
-
sur40->usbdev = usbdev;
sur40->dev = &interface->dev;
- sur40->input = input;
/* use the bulk-in endpoint tested above */
sur40->bulk_in_size = usb_endpoint_maxp(endpoint);
@@ -722,7 +737,7 @@ static int sur40_probe(struct usb_interface *interface,
if (!sur40->bulk_in_buffer) {
dev_err(&interface->dev, "Unable to allocate input buffer.");
error = -ENOMEM;
- goto err_free_input;
+ goto err_free_dev;
}
/* register the video master device */
@@ -790,13 +805,9 @@ static int sur40_probe(struct usb_interface *interface,
goto err_free_ctrl;
}
- /* register the polled input device */
- error = input_register_device(input);
- if (error) {
- dev_err(&interface->dev,
- "Unable to register polled input device.");
+ error = sur40_init_input(sur40);
+ if (error)
goto err_unreg_video;
- }
/* we can register the device now, as it is ready */
usb_set_intfdata(interface, sur40);
@@ -812,8 +823,6 @@ static int sur40_probe(struct usb_interface *interface,
v4l2_device_unregister(&sur40->v4l2);
err_free_buffer:
kfree(sur40->bulk_in_buffer);
-err_free_input:
- input_free_device(input);
err_free_dev:
kfree(sur40);
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* [PATCH 2/4] Input: sur40 - fix V4L error path cleanup
From: Dmitry Torokhov @ 2026-06-16 5:12 UTC (permalink / raw)
To: Hans Verkuil, linux-input; +Cc: linux-kernel, sashiko-bot, stable
In-Reply-To: <20260616051235.1549517-1-dmitry.torokhov@gmail.com>
In sur40_probe(), if video_register_device() fails, the error path jumps to
err_unreg_video. This incorrectly attempts to unregister a video device
that was never successfully registered, and fails to free the V4L2 control
handler (v4l2_ctrl_handler_free) that was initialized immediately prior.
Fix this by introducing an err_free_ctrl label to properly free the V4L2
control handler and bypass video_unregister_device() when video device
registration fails.
Reported-by: sashiko-bot@kernel.org
Cc: stable@vger.kernel.org
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/sur40.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index 8639ec3ad703..e9089b0c3e2f 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -787,7 +787,7 @@ static int sur40_probe(struct usb_interface *interface,
if (error) {
dev_err(&interface->dev,
"Unable to register video subdevice.");
- goto err_unreg_video;
+ goto err_free_ctrl;
}
/* register the polled input device */
@@ -806,6 +806,8 @@ static int sur40_probe(struct usb_interface *interface,
err_unreg_video:
video_unregister_device(&sur40->vdev);
+err_free_ctrl:
+ v4l2_ctrl_handler_free(&sur40->hdl);
err_unreg_v4l2:
v4l2_device_unregister(&sur40->v4l2);
err_free_buffer:
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* [PATCH 1/4] Input: sur40 - fix input device registration ordering
From: Dmitry Torokhov @ 2026-06-16 5:12 UTC (permalink / raw)
To: Hans Verkuil, linux-input; +Cc: linux-kernel, sashiko-bot, stable
In sur40_probe(), input_register_device() was previously called early before
the V4L2 video device and vb2_queue components were fully initialized. If
userspace opened the input device immediately upon registration, sur40_open()
would trigger and start the sur40_poll() worker thread. This worker thread
invokes sur40_process_video() and accesses the uninitialized vb2_queue
structure, leading to a data race and potential system crash.
Furthermore, if V4L2 or video registration failed after input_register_device()
succeeded, the error path fell through to calling input_free_device() on a
successfully registered device instead of input_unregister_device(), corrupting
input core state.
Move input_register_device() to the very end of sur40_probe(). This ensures
the V4L2 and video queue structures are fully initialized before polling can
start, and naturally resolves the error path bug since input_free_device()
is now only called when input registration has not yet occurred.
To maintain strict LIFO (Last-In, First-Out) teardown ordering, also move
input_unregister_device() to the very beginning of sur40_disconnect(). This
guarantees that the input polling worker thread is stopped before V4L2
video components or control handlers are unregistered.
Reported-by: sashiko-bot@kernel.org
Cc: stable@vger.kernel.org
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/sur40.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index fe63d53d56db..8639ec3ad703 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -725,21 +725,13 @@ static int sur40_probe(struct usb_interface *interface,
goto err_free_input;
}
- /* register the polled input device */
- error = input_register_device(input);
- if (error) {
- dev_err(&interface->dev,
- "Unable to register polled input device.");
- goto err_free_buffer;
- }
-
/* register the video master device */
snprintf(sur40->v4l2.name, sizeof(sur40->v4l2.name), "%s", DRIVER_LONG);
error = v4l2_device_register(sur40->dev, &sur40->v4l2);
if (error) {
dev_err(&interface->dev,
"Unable to register video master device.");
- goto err_unreg_v4l2;
+ goto err_free_buffer;
}
/* initialize the lock and subdevice */
@@ -798,6 +790,14 @@ static int sur40_probe(struct usb_interface *interface,
goto err_unreg_video;
}
+ /* register the polled input device */
+ error = input_register_device(input);
+ if (error) {
+ dev_err(&interface->dev,
+ "Unable to register polled input device.");
+ goto err_unreg_video;
+ }
+
/* we can register the device now, as it is ready */
usb_set_intfdata(interface, sur40);
dev_dbg(&interface->dev, "%s is now attached\n", DRIVER_DESC);
@@ -823,11 +823,12 @@ static void sur40_disconnect(struct usb_interface *interface)
{
struct sur40_state *sur40 = usb_get_intfdata(interface);
+ input_unregister_device(sur40->input);
+
v4l2_ctrl_handler_free(&sur40->hdl);
video_unregister_device(&sur40->vdev);
v4l2_device_unregister(&sur40->v4l2);
- input_unregister_device(sur40->input);
kfree(sur40->bulk_in_buffer);
kfree(sur40);
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* [PATCH 6/6] Input: mms114 - refactor chip variant handling using descriptors
From: Dmitry Torokhov @ 2026-06-16 5:09 UTC (permalink / raw)
To: linux-input; +Cc: Linus Walleij, Bryam Vargas, linux-kernel
In-Reply-To: <20260616050912.1531241-1-dmitry.torokhov@gmail.com>
Instead of using an enum and conditional switch/if statements throughout
the driver to handle differences between chip variants (MMS114, MMS134S,
MMS136, MMS152, MMS345L), introduce a variant-specific descriptor
structure that encapsulates variant-specific properties (name, event
size, presence of configuration registers) and callbacks (such as
get_version). Define descriptors for each supported chip and associate
them with the matching entries in the OF and I2C device ID tables.
This eliminates the need for variant checks in the driver logic, making
it easier to support new chip variants in the future.
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/mms114.c | 204 ++++++++++++++++-------------
1 file changed, 112 insertions(+), 92 deletions(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index bf01eee0560a..006dded17eb8 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -52,21 +52,13 @@
#define MMS114_TYPE_TOUCHSCREEN 1
#define MMS114_TYPE_TOUCHKEY 2
-enum mms_type {
- TYPE_MMS114 = 114,
- TYPE_MMS134S = 134,
- TYPE_MMS136 = 136,
- TYPE_MMS152 = 152,
- TYPE_MMS345L = 345,
-};
-
struct mms114_data {
+ const struct mms_chip *chip;
struct i2c_client *client;
struct input_dev *input_dev;
struct regulator *core_reg;
struct regulator *io_reg;
struct touchscreen_properties props;
- enum mms_type type;
unsigned int contact_threshold;
unsigned int moving_threshold;
@@ -77,6 +69,13 @@ struct mms114_data {
u8 cache_mode_control;
};
+struct mms_chip {
+ const char *name;
+ int event_size;
+ bool has_config_regs;
+ int (*get_version)(struct mms114_data *data);
+};
+
struct mms114_touch {
u8 id:4, reserved_bit4:1, type:2, pressed:1;
u8 x_hi:4, y_hi:4;
@@ -156,6 +155,91 @@ static int mms114_write_reg(struct mms114_data *data, u8 reg, u8 val)
return 0;
}
+static int mms114_get_version(struct mms114_data *data)
+{
+ struct device *dev = &data->client->dev;
+ u8 buf[6];
+ int error;
+
+ error = __mms114_read_reg(data, MMS114_TSP_REV, 6, buf);
+ if (error)
+ return error;
+
+ dev_info(dev, "TSP Rev: 0x%x, HW Rev: 0x%x, Firmware Ver: 0x%x\n",
+ buf[0], buf[1], buf[3]);
+ return 0;
+}
+
+static int mms152_get_version(struct mms114_data *data)
+{
+ struct device *dev = &data->client->dev;
+ u8 buf[3];
+ int group;
+ int error;
+
+ error = __mms114_read_reg(data, MMS152_FW_REV, 3, buf);
+ if (error)
+ return error;
+
+ group = i2c_smbus_read_byte_data(data->client, MMS152_COMPAT_GROUP);
+ if (group < 0)
+ return group;
+
+ dev_info(dev, "TSP FW Rev: bootloader 0x%x / core 0x%x / config 0x%x, Compat group: %c\n",
+ buf[0], buf[1], buf[2], group);
+ return 0;
+}
+
+static int mms345l_get_version(struct mms114_data *data)
+{
+ struct device *dev = &data->client->dev;
+ u8 buf[3];
+ int error;
+
+ error = __mms114_read_reg(data, MMS152_FW_REV, 3, buf);
+ if (error)
+ return error;
+
+ dev_info(dev, "TSP FW Rev: bootloader 0x%x / core 0x%x / config 0x%x\n",
+ buf[0], buf[1], buf[2]);
+ return 0;
+}
+
+static const struct mms_chip mms114_descriptor = {
+ .name = "MMS114",
+ .event_size = MMS114_EVENT_SIZE,
+ .has_config_regs = true,
+ .get_version = mms114_get_version,
+};
+
+static const struct mms_chip mms134s_descriptor = {
+ .name = "MMS134S",
+ .event_size = MMS136_EVENT_SIZE,
+ .has_config_regs = true,
+ .get_version = mms114_get_version,
+};
+
+static const struct mms_chip mms136_descriptor = {
+ .name = "MMS136",
+ .event_size = MMS136_EVENT_SIZE,
+ .has_config_regs = true,
+ .get_version = mms114_get_version,
+};
+
+static const struct mms_chip mms152_descriptor = {
+ .name = "MMS152",
+ .event_size = MMS114_EVENT_SIZE,
+ .has_config_regs = false,
+ .get_version = mms152_get_version,
+};
+
+static const struct mms_chip mms345l_descriptor = {
+ .name = "MMS345L",
+ .event_size = MMS114_EVENT_SIZE,
+ .has_config_regs = false,
+ .get_version = mms345l_get_version,
+};
+
static void mms114_process_mt(struct mms114_data *data, struct mms114_touch *touch)
{
struct i2c_client *client = data->client;
@@ -217,8 +301,8 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
struct i2c_client *client = data->client;
struct mms114_touch touch[MMS114_MAX_TOUCH];
struct mms114_touch *t;
+ int event_size = data->chip->event_size;
int packet_size;
- int event_size;
int touch_size;
int index;
int error;
@@ -233,12 +317,6 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
goto out;
}
- /* MMS136 has slightly different event size */
- if (data->type == TYPE_MMS134S || data->type == TYPE_MMS136)
- event_size = MMS136_EVENT_SIZE;
- else
- event_size = MMS114_EVENT_SIZE;
-
touch_size = packet_size / event_size;
error = __mms114_read_reg(data, MMS114_INFORMATION, packet_size, touch);
@@ -288,64 +366,17 @@ static int mms114_set_active(struct mms114_data *data, bool active)
return mms114_write_reg(data, MMS114_MODE_CONTROL, val);
}
-static int mms114_get_version(struct mms114_data *data)
-{
- struct device *dev = &data->client->dev;
- u8 buf[6];
- int group;
- int error;
-
- switch (data->type) {
- case TYPE_MMS345L:
- error = __mms114_read_reg(data, MMS152_FW_REV, 3, buf);
- if (error)
- return error;
-
- dev_info(dev, "TSP FW Rev: bootloader 0x%x / core 0x%x / config 0x%x\n",
- buf[0], buf[1], buf[2]);
- break;
-
- case TYPE_MMS152:
- error = __mms114_read_reg(data, MMS152_FW_REV, 3, buf);
- if (error)
- return error;
-
- group = i2c_smbus_read_byte_data(data->client, MMS152_COMPAT_GROUP);
- if (group < 0)
- return group;
-
- dev_info(dev, "TSP FW Rev: bootloader 0x%x / core 0x%x / config 0x%x, Compat group: %c\n",
- buf[0], buf[1], buf[2], group);
- break;
-
- case TYPE_MMS114:
- case TYPE_MMS134S:
- case TYPE_MMS136:
- error = __mms114_read_reg(data, MMS114_TSP_REV, 6, buf);
- if (error)
- return error;
-
- dev_info(dev, "TSP Rev: 0x%x, HW Rev: 0x%x, Firmware Ver: 0x%x\n",
- buf[0], buf[1], buf[3]);
- break;
- }
-
- return 0;
-}
-
static int mms114_setup_regs(struct mms114_data *data)
{
const struct touchscreen_properties *props = &data->props;
int val;
int error;
- error = mms114_get_version(data);
- if (error < 0)
+ error = data->chip->get_version(data);
+ if (error)
return error;
- /* MMS114, MMS134S and MMS136 have configuration and power on registers */
- if (data->type != TYPE_MMS114 && data->type != TYPE_MMS134S &&
- data->type != TYPE_MMS136)
+ if (!data->chip->has_config_regs)
return 0;
error = mms114_set_active(data, true);
@@ -481,7 +512,6 @@ static int mms114_probe(struct i2c_client *client)
{
struct mms114_data *data;
struct input_dev *input_dev;
- const void *match_data;
int error;
int i;
@@ -501,12 +531,10 @@ static int mms114_probe(struct i2c_client *client)
data->client = client;
data->input_dev = input_dev;
- match_data = device_get_match_data(&client->dev);
- if (!match_data)
+ data->chip = i2c_get_match_data(client);
+ if (!data->chip)
return -EINVAL;
- data->type = (enum mms_type)match_data;
-
data->num_keycodes = device_property_count_u32(&client->dev,
"linux,keycodes");
if (data->num_keycodes == -EINVAL) {
@@ -563,8 +591,7 @@ static int mms114_probe(struct i2c_client *client)
0, data->props.max_y, 0, 0);
}
- if (data->type == TYPE_MMS114 || data->type == TYPE_MMS134S ||
- data->type == TYPE_MMS136) {
+ if (data->chip->has_config_regs) {
/*
* The firmware handles movement and pressure fuzz, so
* don't duplicate that in software.
@@ -579,8 +606,8 @@ static int mms114_probe(struct i2c_client *client)
}
input_dev->name = devm_kasprintf(&client->dev, GFP_KERNEL,
- "MELFAS MMS%d Touchscreen",
- data->type);
+ "MELFAS %s Touchscreen",
+ data->chip->name);
if (!input_dev->name)
return -ENOMEM;
@@ -676,29 +703,22 @@ static int mms114_resume(struct device *dev)
static DEFINE_SIMPLE_DEV_PM_OPS(mms114_pm_ops, mms114_suspend, mms114_resume);
static const struct i2c_device_id mms114_id[] = {
- { .name = "mms114" },
+ { .name = "mms114", .driver_data = (kernel_ulong_t)&mms114_descriptor },
+ { .name = "mms134s", .driver_data = (kernel_ulong_t)&mms134s_descriptor },
+ { .name = "mms136", .driver_data = (kernel_ulong_t)&mms136_descriptor },
+ { .name = "mms152", .driver_data = (kernel_ulong_t)&mms152_descriptor },
+ { .name = "mms345l", .driver_data = (kernel_ulong_t)&mms345l_descriptor },
{ }
};
MODULE_DEVICE_TABLE(i2c, mms114_id);
#ifdef CONFIG_OF
static const struct of_device_id mms114_dt_match[] = {
- {
- .compatible = "melfas,mms114",
- .data = (void *)TYPE_MMS114,
- }, {
- .compatible = "melfas,mms134s",
- .data = (void *)TYPE_MMS134S,
- }, {
- .compatible = "melfas,mms136",
- .data = (void *)TYPE_MMS136,
- }, {
- .compatible = "melfas,mms152",
- .data = (void *)TYPE_MMS152,
- }, {
- .compatible = "melfas,mms345l",
- .data = (void *)TYPE_MMS345L,
- },
+ { .compatible = "melfas,mms114", .data = &mms114_descriptor },
+ { .compatible = "melfas,mms134s", .data = &mms134s_descriptor },
+ { .compatible = "melfas,mms136", .data = &mms136_descriptor },
+ { .compatible = "melfas,mms152", .data = &mms152_descriptor },
+ { .compatible = "melfas,mms345l", .data = &mms345l_descriptor },
{ }
};
MODULE_DEVICE_TABLE(of, mms114_dt_match);
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* [PATCH 5/6] Input: mms114 - replace BUG() and fix alignment
From: Dmitry Torokhov @ 2026-06-16 5:09 UTC (permalink / raw)
To: linux-input; +Cc: Linus Walleij, Bryam Vargas, linux-kernel
In-Reply-To: <20260616050912.1531241-1-dmitry.torokhov@gmail.com>
Avoid crashing the kernel with BUG() when userspace attempts to read
the write-only mode control register. Instead, trigger a warning with
WARN_ON() and return -EINVAL to allow the kernel to recover.
Additionally, fix parameter alignment to match the open parenthesis
in several functions to conform to the kernel coding style.
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/mms114.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index c59aec8f2feb..bf01eee0560a 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -95,8 +95,8 @@ static int __mms114_read_reg(struct mms114_data *data, u8 reg,
u8 buf = reg;
int error;
- if (reg <= MMS114_MODE_CONTROL && reg + len > MMS114_MODE_CONTROL)
- BUG();
+ if (WARN_ON(reg <= MMS114_MODE_CONTROL && reg + len > MMS114_MODE_CONTROL))
+ return -EINVAL;
/* Write register */
xfer[0].addr = client->addr;
@@ -310,8 +310,7 @@ static int mms114_get_version(struct mms114_data *data)
if (error)
return error;
- group = i2c_smbus_read_byte_data(data->client,
- MMS152_COMPAT_GROUP);
+ group = i2c_smbus_read_byte_data(data->client, MMS152_COMPAT_GROUP);
if (group < 0)
return group;
@@ -371,14 +370,14 @@ static int mms114_setup_regs(struct mms114_data *data)
if (data->contact_threshold) {
error = mms114_write_reg(data, MMS114_CONTACT_THRESHOLD,
- data->contact_threshold);
+ data->contact_threshold);
if (error < 0)
return error;
}
if (data->moving_threshold) {
error = mms114_write_reg(data, MMS114_MOVING_THRESHOLD,
- data->moving_threshold);
+ data->moving_threshold);
if (error < 0)
return error;
}
@@ -464,9 +463,9 @@ static int mms114_parse_legacy_bindings(struct mms114_data *data)
}
device_property_read_u32(dev, "contact-threshold",
- &data->contact_threshold);
+ &data->contact_threshold);
device_property_read_u32(dev, "moving-threshold",
- &data->moving_threshold);
+ &data->moving_threshold);
if (device_property_read_bool(dev, "x-invert"))
props->invert_x = true;
@@ -519,7 +518,7 @@ static int mms114_probe(struct i2c_client *client)
return data->num_keycodes;
} else if (data->num_keycodes > MMS114_MAX_TOUCHKEYS) {
dev_warn(&client->dev,
- "Found %d linux,keycodes but max is %d, ignoring the rest\n",
+ "Found %d linux,keycodes but max is %d, ignoring the rest\n",
data->num_keycodes, MMS114_MAX_TOUCHKEYS);
data->num_keycodes = MMS114_MAX_TOUCHKEYS;
}
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* [PATCH 4/6] Input: mms114 - replace udelay with usleep_range
From: Dmitry Torokhov @ 2026-06-16 5:09 UTC (permalink / raw)
To: linux-input; +Cc: Linus Walleij, Bryam Vargas, linux-kernel
In-Reply-To: <20260616050912.1531241-1-dmitry.torokhov@gmail.com>
The driver currently uses udelay(MMS114_I2C_DELAY) (50us) to ensure a
mandatory delay between I2C transfers in __mms114_read_reg() and
mms114_write_reg().
Both functions invoke underlying I2C core operations (i2c_transfer,
i2c_master_send) which acquire mutexes and sleep. Furthermore, the
interrupt handler mms114_interrupt() is registered as a threaded IRQ
handler. Since the entire execution path is fully sleepable,
busy-waiting with udelay() for 50us unnecessarily wastes CPU cycles.
Replace udelay() with usleep_range() to allow the CPU to enter low-power
states or execute other tasks during the delay.
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/mms114.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index c2e006ac1196..c59aec8f2feb 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -116,7 +116,7 @@ static int __mms114_read_reg(struct mms114_data *data, u8 reg,
"%s: i2c transfer failed (%d)\n", __func__, error);
return error < 0 ? error : -EIO;
}
- udelay(MMS114_I2C_DELAY);
+ usleep_range(MMS114_I2C_DELAY, MMS114_I2C_DELAY + 50);
return 0;
}
@@ -148,7 +148,7 @@ static int mms114_write_reg(struct mms114_data *data, u8 reg, u8 val)
"%s: i2c send failed (%d)\n", __func__, error);
return error < 0 ? error : -EIO;
}
- udelay(MMS114_I2C_DELAY);
+ usleep_range(MMS114_I2C_DELAY, MMS114_I2C_DELAY + 50);
if (reg == MMS114_MODE_CONTROL)
data->cache_mode_control = val;
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* [PATCH 3/6] Input: mms114 - use appropriate register argument types
From: Dmitry Torokhov @ 2026-06-16 5:09 UTC (permalink / raw)
To: linux-input; +Cc: Linus Walleij, Bryam Vargas, linux-kernel
In-Reply-To: <20260616050912.1531241-1-dmitry.torokhov@gmail.com>
The MMS114 I2C touch controller uses 8-bit register addresses (0x01 to
0xF2) and 8-bit single-register data values. The helper functions
previously declared reg and val as 32-bit unsigned int, requiring
explicit bitwise masking (& 0xff) to narrow the values down to u8 before
populating the I2C transfer buffers.
Update reg and val parameters to u8 across mms114_read_reg(),
mms114_write_reg(), and __mms114_read_reg() to accurately reflect the
hardware specification and eliminate the redundant & 0xff masking.
Additionally, update the val buffer pointer in __mms114_read_reg() from
u8 * to void * to allow callers to pass data structures directly without
requiring explicit casting.
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/mms114.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index db23b51f4630..c2e006ac1196 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -87,12 +87,12 @@ struct mms114_touch {
u8 reserved[2];
} __packed;
-static int __mms114_read_reg(struct mms114_data *data, unsigned int reg,
- unsigned int len, u8 *val)
+static int __mms114_read_reg(struct mms114_data *data, u8 reg,
+ unsigned int len, void *val)
{
struct i2c_client *client = data->client;
struct i2c_msg xfer[2];
- u8 buf = reg & 0xff;
+ u8 buf = reg;
int error;
if (reg <= MMS114_MODE_CONTROL && reg + len > MMS114_MODE_CONTROL)
@@ -121,7 +121,7 @@ static int __mms114_read_reg(struct mms114_data *data, unsigned int reg,
return 0;
}
-static int mms114_read_reg(struct mms114_data *data, unsigned int reg)
+static int mms114_read_reg(struct mms114_data *data, u8 reg)
{
u8 val;
int error;
@@ -133,15 +133,14 @@ static int mms114_read_reg(struct mms114_data *data, unsigned int reg)
return error < 0 ? error : val;
}
-static int mms114_write_reg(struct mms114_data *data, unsigned int reg,
- unsigned int val)
+static int mms114_write_reg(struct mms114_data *data, u8 reg, u8 val)
{
struct i2c_client *client = data->client;
u8 buf[2];
int error;
- buf[0] = reg & 0xff;
- buf[1] = val & 0xff;
+ buf[0] = reg;
+ buf[1] = val;
error = i2c_master_send(client, buf, 2);
if (error != 2) {
@@ -242,9 +241,8 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
touch_size = packet_size / event_size;
- error = __mms114_read_reg(data, MMS114_INFORMATION, packet_size,
- (u8 *)touch);
- if (error < 0)
+ error = __mms114_read_reg(data, MMS114_INFORMATION, packet_size, touch);
+ if (error)
goto out;
for (index = 0; index < touch_size; index++) {
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* [PATCH 2/6] Input: mms114 - prefer GPL over GPL v2 for module license
From: Dmitry Torokhov @ 2026-06-16 5:09 UTC (permalink / raw)
To: linux-input; +Cc: Linus Walleij, Bryam Vargas, linux-kernel
In-Reply-To: <20260616050912.1531241-1-dmitry.torokhov@gmail.com>
As explained in commit bf7fbeeae6db ("module: Cure the MODULE_LICENSE
"GPL" vs. "GPL v2" bogosity"), "GPL" and "GPL v2" have identical
semantics in the module loader, but "GPL" is preferred to avoid
unnecessary confusion and maintain consistency across the kernel.
Change MODULE_LICENSE("GPL v2") to MODULE_LICENSE("GPL").
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/mms114.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index 53ad35d61d47..db23b51f4630 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -722,4 +722,4 @@ module_i2c_driver(mms114_driver);
/* Module information */
MODULE_AUTHOR("Joonyoung Shim <jy0922.shim@samsung.com>");
MODULE_DESCRIPTION("MELFAS mms114 Touchscreen driver");
-MODULE_LICENSE("GPL v2");
+MODULE_LICENSE("GPL");
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* [PATCH 1/6] Input: mms114 - fix touch indexing for MMS134S and MMS136
From: Dmitry Torokhov @ 2026-06-16 5:09 UTC (permalink / raw)
To: linux-input; +Cc: Linus Walleij, Bryam Vargas, linux-kernel, sashiko-bot
The MMS134S and MMS136 touch controllers have an event size of 6 bytes
rather than 8 bytes. When __mms114_read_reg() reads the touch data
packet from the device into the touch buffer, the events are packed
tightly at 6-byte intervals. However, the driver iterates through the
events using standard C array indexing (touch[index]), where each
element is sizeof(struct mms114_touch) (8 bytes) apart. As a result, any
touch events beyond the first one are read from incorrect offsets and
parsed improperly.
Fix this by explicitly calculating the byte offset for each touch event
based on the device's specific event size.
Fixes: ab108678195f ("Input: mms114 - support MMS134S")
Reported-by: sashiko-bot@kernel.org
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/mms114.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index 4d748a13408d..53ad35d61d47 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -217,7 +217,9 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
struct mms114_data *data = dev_id;
struct i2c_client *client = data->client;
struct mms114_touch touch[MMS114_MAX_TOUCH];
+ struct mms114_touch *t;
int packet_size;
+ int event_size;
int touch_size;
int index;
int error;
@@ -234,9 +236,11 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
/* MMS136 has slightly different event size */
if (data->type == TYPE_MMS134S || data->type == TYPE_MMS136)
- touch_size = packet_size / MMS136_EVENT_SIZE;
+ event_size = MMS136_EVENT_SIZE;
else
- touch_size = packet_size / MMS114_EVENT_SIZE;
+ event_size = MMS114_EVENT_SIZE;
+
+ touch_size = packet_size / event_size;
error = __mms114_read_reg(data, MMS114_INFORMATION, packet_size,
(u8 *)touch);
@@ -244,18 +248,20 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
goto out;
for (index = 0; index < touch_size; index++) {
- switch (touch[index].type) {
+ t = (struct mms114_touch *)((u8 *)touch + index * event_size);
+
+ switch (t->type) {
case MMS114_TYPE_TOUCHSCREEN:
- mms114_process_mt(data, touch + index);
+ mms114_process_mt(data, t);
break;
case MMS114_TYPE_TOUCHKEY:
- mms114_process_touchkey(data, touch + index);
+ mms114_process_touchkey(data, t);
break;
default:
dev_err(&client->dev, "Wrong touch type (%d)\n",
- touch[index].type);
+ t->type);
break;
}
}
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* Re: [PATCH v8 4/7] input: keyboard: Add driver for ASUS Transformer dock multimedia keys
From: Dmitry Torokhov @ 2026-06-16 4:26 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: <20260528053203.9339-5-clamor95@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?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 1/4] HID: multitouch: set INPUT_PROP_PRESSUREPAD based on Digitizer/Button Type
From: Peter Hutterer @ 2026-06-16 4:24 UTC (permalink / raw)
To: Rong Zhang
Cc: devnull+peter.hutterer.who-t.net, bentiss, dmitry.torokhov, jikos,
linux-input, linux-kernel, linux-kselftest, shuah, vadim
In-Reply-To: <fb198851093477d958eae61665e9cfda0be5b48f.camel@rong.moe>
Hi Rong,
On Tue, Jun 02, 2026 at 01:25:57AM +0800, Rong Zhang wrote:
>
> Hi all,
>
> Hopefully I'm not too late to show up here.
>
> > From: Peter Hutterer <peter.hutterer@who-t.net>
> >
> > A Digitizer/Button Type value of 1 indicates the device is a
> > pressurepad, see
> > https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/touchpad-windows-precision-touchpad-collection#device-capabilities-feature-report
> >
> > Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
> > ---
> > drivers/hid/hid-multitouch.c | 12 +++++++++++-
> > 1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> > index 179dc316b4b518d78bdc900d9fd15756c5eba83e..382e6f50c4f7e663af7d028abb8be7cb2e6e7b8e 100644
> > --- a/drivers/hid/hid-multitouch.c
> > +++ b/drivers/hid/hid-multitouch.c
> > @@ -81,6 +81,7 @@ MODULE_LICENSE("GPL");
> > #define MT_INPUTMODE_TOUCHPAD 0x03
> >
> > #define MT_BUTTONTYPE_CLICKPAD 0
> > +#define MT_BUTTONTYPE_PRESSUREPAD 1
> >
> > enum latency_mode {
> > HID_LATENCY_NORMAL = 0,
> > @@ -179,6 +180,7 @@ struct mt_device {
> > __u8 inputmode_value; /* InputMode HID feature value */
> > __u8 maxcontacts;
> > bool is_buttonpad; /* is this device a button pad? */
> > + bool is_pressurepad; /* is this device a pressurepad? */
> > bool is_haptic_touchpad; /* is this device a haptic touchpad? */
> > bool serial_maybe; /* need to check for serial protocol */
> >
> > @@ -530,8 +532,14 @@ static void mt_feature_mapping(struct hid_device *hdev,
> > }
> >
> > mt_get_feature(hdev, field->report);
> > - if (field->value[usage->usage_index] == MT_BUTTONTYPE_CLICKPAD)
> > + switch (field->value[usage->usage_index]) {
> > + case MT_BUTTONTYPE_CLICKPAD:
> > td->is_buttonpad = true;
> > + break;
> > + case MT_BUTTONTYPE_PRESSUREPAD:
> > + td->is_pressurepad = true;
> > + break;
> > + }
> >
> > break;
> > case 0xff0000c5:
> > @@ -1393,6 +1401,8 @@ static int mt_touch_input_configured(struct hid_device *hdev,
> >
> > if (td->is_buttonpad)
> > __set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
> > + if (td->is_pressurepad)
> > + __set_bit(INPUT_PROP_PRESSUREPAD, input->propbit);
>
> I noticed that this leads to dual reporting on my device.
>
> Consider previous checks:
>
> if (application == HID_DG_TOUCHPAD) {
> mt_application->mt_flags |= INPUT_MT_POINTER;
> td->inputmode_value = MT_INPUTMODE_TOUCHPAD;
> }
>
> ...
>
> /* check for clickpads */
> if ((app->mt_flags & INPUT_MT_POINTER) &&
> (app->buttons_count == 1))
> td->is_buttonpad = true;
>
> ... where `td->is_buttonpad' is set to true when a pressure pad has only
> one button, i.e., the "touchpad button integrated with digitizer" [1].
> Most (if not all) pressure pads fall into this category. As a result,
> the presence of INPUT_PROP_PRESSUREPAD is always accompanied by the
> presence of INPUT_PROP_BUTTONPAD.
Yes, this is intended, see commit
ae8966b7b5bd69b86209cc34bcca1ba9f18b68e6 which lists this in the commit
message:
```
This means:
- clickpad: INPUT_PROP_BUTTONPAD
- pressurepad: INPUT_PROP_BUTTONPAD + INPUT_PROP_PRESSUREPAD
- pressurepad with configurable haptics:
INPUT_PROP_BUTTONPAD + INPUT_PROP_PRESSUREPAD + FF_HAPTIC
```
We have to keep setting BUTTONPAD on all pressurepads because otherwise
we'd break existing userspace which relies on this.
Cheers,
Peter
^ permalink raw reply
* Re: [PATCH v5 2/2] Input: isa1200 - new driver for Imagis ISA1200
From: Dmitry Torokhov @ 2026-06-16 4:16 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: linux-input, devicetree, linux-kernel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Linus Walleij
In-Reply-To: <CAPVz0n3iCSeT3xJ2XkwZ6PYofwSLkc0gfm+iYo4xbKBkAtihcQ@mail.gmail.com>
Hi Svyatoslav,
On Mon, Jun 15, 2026 at 09:19:27AM +0300, Svyatoslav Ryhel wrote:
> чт, 28 трав. 2026 р. о 08:38 Svyatoslav Ryhel <clamor95@gmail.com> пише:
> >
> > вт, 12 трав. 2026 р. о 13:24 Svyatoslav Ryhel <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 | 524 +++++++++++++++++++++++++++++++++++
> > > 3 files changed, 537 insertions(+)
> > > create mode 100644 drivers/input/misc/isa1200.c
> > >
> >
> > Hello Dmitry! Do I need to make any further adjustments to this driver?
>
> Hello Dmitry! Do I need to make any further adjustments to this
> driver? This driver is hanging in LKML for some time already without
> responds from input maintainer. It is still relevant and I would like
> it to move forward.
There were valid sashiko comments on the patch regarding resetting
"level" to 0 and also potential racing conditions, as well as suggestion
to check number of gpios specified in the device tree.
Please see if the following works for you:
diff --git a/drivers/input/misc/isa1200.c b/drivers/input/misc/isa1200.c
index ff82252a08e1..c61adc4b605c 100644
--- a/drivers/input/misc/isa1200.c
+++ b/drivers/input/misc/isa1200.c
@@ -131,6 +131,7 @@ struct isa1200 {
struct work_struct play_work;
struct isa1200_config config;
+ bool suspended;
bool active;
int level;
};
@@ -247,17 +248,21 @@ static void isa1200_stop(struct isa1200 *isa)
isa->supplies);
isa->active = false;
- isa->level = 0;
}
static void isa1200_play_work(struct work_struct *work)
{
struct isa1200 *isa = container_of(work, struct isa1200, play_work);
-
- if (isa->level)
- isa1200_start(isa);
- else
- isa1200_stop(isa);
+ 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,
@@ -280,7 +285,8 @@ static int isa1200_vibrator_play_effect(struct input_dev *input, void *data,
if (isa->level != level) {
isa->level = level;
- schedule_work(&isa->play_work);
+ if (!READ_ONCE(isa->suspended))
+ schedule_work(&isa->play_work);
}
return 0;
@@ -292,6 +298,7 @@ static void isa1200_vibrator_close(struct input_dev *input)
cancel_work_sync(&isa->play_work);
isa1200_stop(isa);
+ isa->level = 0;
}
static int isa1200_of_probe(struct i2c_client *client)
@@ -331,6 +338,9 @@ static int isa1200_of_probe(struct i2c_client *client)
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,
@@ -479,9 +489,9 @@ static int isa1200_suspend(struct device *dev)
guard(mutex)(&isa->input->mutex);
if (input_device_enabled(isa->input)) {
+ WRITE_ONCE(isa->suspended, true);
cancel_work_sync(&isa->play_work);
- if (isa->level)
- isa1200_stop(isa);
+ isa1200_stop(isa);
}
return 0;
@@ -493,9 +503,11 @@ static int isa1200_resume(struct device *dev)
guard(mutex)(&isa->input->mutex);
- if (input_device_enabled(isa->input))
+ if (input_device_enabled(isa->input)) {
+ WRITE_ONCE(isa->suspended, false);
if (isa->level)
- isa1200_start(isa);
+ schedule_work(&isa->play_work);
+ }
return 0;
}
--
Dmitry
^ permalink raw reply related
* Re: [PATCH] Input: yealink - stop URB resubmission on completion error
From: sashiko-bot @ 2026-06-16 3:31 UTC (permalink / raw)
To: Jie Wang; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260616110440.1073059-1-jie.wang@intel.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] The URB completion callbacks return early on any non-zero URB status, preventing resubmission on transient errors (like -EPROTO or -EILSEQ) and permanently breaking the device's polling loop.
Pre-existing issues:
- [High] Concurrent accesses to `yld->ctl_data` from sysfs and the asynchronous URB polling loop cause DMA buffer corruption, and passing a coherent DMA buffer to a synchronous USB API violates DMA mappings.
- [High] Ping-pong URBs can escape `usb_kill_urb()` during `input_close()`, leading to continuous background execution and use-after-free.
--
commit 02a6ac0b5f7fd710af199370b924a27f34ca574c
Author: Jie Wang <jie.wang@intel.com>
Input: yealink - stop URB resubmission on completion error
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>
> diff --git a/drivers/input/misc/yealink.c b/drivers/input/misc/yealink.c
> index 560f895a00cda..fcf66912f86b6 100644
> --- a/drivers/input/misc/yealink.c
> +++ b/drivers/input/misc/yealink.c
> @@ -405,9 +405,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;
> + }
[Severity: High]
Does returning early on any non-zero status prevent resubmission on transient
errors?
If an error like -EPROTO or -EILSEQ is encountered due to electrical noise,
this early return completely drops the URB. Because the driver has no backoff
timer or error retry mechanism, a single transient error will permanently stop
the polling state machine. Sysfs updates and key presses will silently be
ignored until the device is physically replugged.
[ ... ]
> @@ -443,9 +454,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;
> + }
[Severity: High]
Will the same issue happen here, breaking the ping-pong URB loop on transient
USB errors and permanently freezing the device state machine?
[Severity: High]
This is a pre-existing issue, but does concurrent access to the shared control
buffer from sysfs cause DMA corruption?
Looking at yealink_set_ringtone(), it modifies the shared control buffer
without locking:
drivers/input/misc/yealink.c:yealink_set_ringtone() {
...
memset(yld->ctl_data, 0, sizeof(*(yld->ctl_data)));
yld->ctl_data->cmd = CMD_RING_VOLUME;
...
}
Since yld->ctl_data is used as the dedicated DMA transfer buffer for the
asynchronous control URB which runs continuously in a background loop, modifying
it from sysfs without locking against the async loop could corrupt an active DMA
transfer.
Additionally, yealink_set_ringtone() synchronously sends this buffer:
drivers/input/misc/yealink.c:yealink_cmd() {
...
return usb_control_msg(yld->udev,
usb_sndctrlpipe(yld->udev, 0),
USB_REQ_SET_CONFIGURATION,
USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_OUT,
0x200, 3,
p, sizeof(*p),
USB_CTRL_SET_TIMEOUT);
}
Because yld->ctl_data is allocated with usb_alloc_coherent(), does this violate
the DMA API by attempting to dma_map_single() already-coherent memory?
[Severity: High]
This is a pre-existing issue, but can ping-pong URBs escape the shutdown
sequence and cause a use-after-free?
The driver attempts to stop the ping-pong URBs in input_close() by setting a
flag and killing them:
drivers/input/misc/yealink.c:input_close() {
...
yld->shutdown = 1;
usb_kill_urb(yld->urb_ctl);
usb_kill_urb(yld->urb_irq);
...
}
If urb_irq_callback() evaluates !yld->shutdown as true but is preempted:
drivers/input/misc/yealink.c:urb_irq_callback() {
...
if (!yld->shutdown) {
ret = usb_submit_urb(yld->urb_ctl, GFP_ATOMIC);
...
}
And input_close() then sets shutdown = 1, kills urb_ctl (which isn't in flight
yet), and blocks killing urb_irq. When urb_irq_callback() resumes, it submits
urb_ctl. Because usb_kill_urb(urb_ctl) already returned and cleared the reject
count, the submission succeeds.
If the device is disconnected, usb_cleanup() frees the yealink_dev structure,
but the runaway URB will dereference it upon completion. Should this use
usb_poison_urb() instead to reliably stop the loop?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616110440.1073059-1-jie.wang@intel.com?part=1
^ permalink raw reply
* [PATCH] Input: yealink - stop URB resubmission on completion error
From: Jie Wang @ 2026-06-16 11:04 UTC (permalink / raw)
To: Henk Vergonet, Dmitry Torokhov
Cc: linux-input, linux-kernel, syzbot+039eab266c6321a174bd,
syzkaller-bugs
In-Reply-To: <68bba4fb.050a0220.192772.01a4.GAE@google.com>
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>
---
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
^ permalink raw reply related
* Re: [PATCH v3] HID: Expose LattePanda IOTA UPS as a power_supply device
From: Andrew Maney @ 2026-06-16 2:22 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: jikos, linux-kernel, linux-input
In-Reply-To: <ah2iNcGB-xsEHC0r@beelink>
Hi, sorry for the late response. I've gotten quite busy recently.
I'll work in the kernel implementation, it will probably be easier to get going
with the cleanup code you have. In the meantime, I'll look deeper into the
HID-UPS specification so I can write a proper implementation.
Thanks,
Andrew
On Mon, Jun 1, 2026 at 11:24 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> On May 21 2026, Andrew Maney wrote:
> > On Thu, May 21, 2026 at 2:54 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
> > >
> > > On May 20 2026, Andrew Maney wrote:
> > > > This driver exposes the DFRobot LattePanda IOTA UPS board as a standard
> > > > power_supply device, allowing desktop environments and power management
> > > > tools such as UPower and systemd-logind to display battery status,
> > > > remaining capacity, and charging status without any special
> > > > configuration. It also enables automatic suspend or shutdown on low
> > > > battery and power profile configuration via any tool that supports the
> > > > standard power_supply interface.
> > > >
> > > > The UPS presents itself as an Arduino Leonardo HID device running custom
> > > > firmware (VID 0x2341, PID 0x8036). It reports status and capacity via
> > > > HID reports 0x07 and 0x0C respectively.
> > >
> > > As sashiko detected, you are using a standard Arduino Leonardo and not
> > > making any specific detections. So I was thinking that maybe we should
> > > implement that feature as a HID-BPF program on top of the generic HID
> > > handling.
> > >
> > > However, the handling of battery supplies in the HID generic core is not
> > > entirely filling all of the requirements here, but that's something I
> > > wanted to do for a couple of month but I have been swamped with other
> > > projects.
> > >
> > > Anyway, I wanted to understand why this product was using a generic
> > > Leonardo PID, and: https://wiki.dfrobot.com/dfr1247#tech_specs
> > >
> > > "it leverages the standard HID-UPS protocol to be natively recognized by
> > > Windows as a battery-powered device"
> > >
> > > So. Instead of working on a custom driver, why not simply implement (or
> > > finish the generic implementation) of HID-UPS in hid-input.c?
> > >
> > > Cheers,
> > > Benjamin
> > >
> >
> > Hi Benjamin,
> >
> > Thank you for the feedback! I'll look into the HID-UPS implementation or a
> > HID-BPF program.
> >
> > I should mention that the kernel does already detect the IOTA UPS via the
> > generic HID-UPS support, but the updates that it receives are so slow that
> > it becomes practically unusable for a device with smaller capacity batteries
> > like this. Changes can take several minutes or more to be detected. I
> > originally went with a driver that reads the HID reports directly because it
> > was providing updates far faster.
> >
> > I implemented a check in driver v4 that verifies that the device's HID reports
> > are what's expected before binding to it. In theory, that should prevent the
> > driver from binding to other Arduino Leonardo devices, but I agree that fixing
> > the generic HID-UPS implementation would be a better long-term solution.
> >
> > Which approach do you think would be more suitable for this, completing the
> > HID-UPS implementation or a HID-BPF program? I'm happy to work on either,
> > though as a first-time kernel contributor I'd appreciate any pointers
> > on where to
> > start.
>
> Honestly, if the device is compliant with HID-UPS, you should work in
> the kernel implementation. A HID-BPF would be used to fix device issues,
> but if the kernel is not correct enough you can't do much in just a
> HID-BPF.
>
> If I'm not wrong, the HID-UPS implementation we have currently is the
> hid_battery_* functions. And that implementation assumes a HID connected
> device is a separate device, not a power supply of the main machine. So
> you'll have to deal with a few new HID fields.
>
> Also, I've started the cleanup work on the hid_battery code a couple of
> months ago and never got the chance to send it. Maybe I should send it
> so you can have a better environment with testing.
>
> Cheers,
> Benjamin
>
> >
> > Thanks,
> > Andrew
> >
> > > >
> > > > The charge limit (80% or 100%) is configured via a physical DIP switch
> > > > on the UPS board and cannot be detected automatically. Userspace can
> > > > inform the driver of the configured limit via
> > > > charge_control_end_threshold.
> > > >
> > > > ---
> > > >
> > > > Changes in v3:
> > > > - Deferred power_supply registration to workqueue to avoid blocking probe
> > > > - Fixed kernel panic when instantiated via uhid by checking hid_is_usb()
> > > > before dereferencing USB-specific structures
> > > >
> > > > - Fixed ERR_PTR dereference in raw_event by only assigning ups->psu on
> > > > successful registration
> > > >
> > > > - Fixed data race on ups->charge_limit using spin_lock_irqsave()
> > > > - Removed TIME_TO_EMPTY_NOW and TIME_TO_FULL_NOW properties to avoid
> > > > spurious shutdowns
> > > >
> > > > - Changed plugged-in but not charging state from FULL to NOT_CHARGING
> > > > - Used devm_kasprintf() for a unique sysfs name in order to support
> > > > multiple devices
> > > >
> > > > - Added POWER_SUPPLY and HIDRAW dependencies to Kconfig
> > > > - Used %pe for more human-readable error messages
> > > >
> > > > Changes in v2:
> > > > - Rebased on top of the current tree
> > > > - Moved vendor and device IDs to drivers/hid/hid-ids.h
> > > > - Added Kconfig entry under HID bus support -> Special HID drivers
> > > > - Added build rule to drivers/hid/Makefile
> > > >
> > > > Signed-off-by: Andrew Maney <andrewmaney05@gmail.com>
> > > > ---
> > > > MAINTAINERS | 6 +
> > > > drivers/hid/Kconfig | 10 +
> > > > drivers/hid/Makefile | 1 +
> > > > drivers/hid/hid-ids.h | 3 +
> > > > drivers/hid/hid-lattepanda-iota-ups.c | 409 ++++++++++++++++++++++++++
> > > > 5 files changed, 429 insertions(+)
> > > > create mode 100644 drivers/hid/hid-lattepanda-iota-ups.c
> > > >
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 10e825318..d80721c2c 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -11416,6 +11416,12 @@ F: include/uapi/linux/hid*
> > > > F: samples/hid/
> > > > F: tools/testing/selftests/hid/
> > > >
> > > > +HID LATTEPANDA IOTA UPS DRIVER
> > > > +M: Andrew Maney <andrewmaney05@gmail.com>
> > > > +L: linux-input@vger.kernel.org
> > > > +S: Maintained
> > > > +F: drivers/hid/hid-lattepanda-iota-ups.c
> > > > +
> > > > HID LOGITECH DRIVERS
> > > > R: Filipe Laíns <lains@riseup.net>
> > > > L: linux-input@vger.kernel.org
> > > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > > > index ff2f580b6..21ffc2fd0 100644
> > > > --- a/drivers/hid/Kconfig
> > > > +++ b/drivers/hid/Kconfig
> > > > @@ -510,6 +510,16 @@ config HID_KYSONA
> > > > Say Y here if you have a Kysona M600 mouse
> > > > and want to be able to read its battery capacity.
> > > >
> > > > +config HID_LATTEPANDA_IOTA_UPS
> > > > + tristate "LattePanda IOTA UPS"
> > > > + depends on USB_HID && USB_HIDDEV && X86 && POWER_SUPPLY
> > > > + help
> > > > + Support for the LattePanda IOTA UPS (DFRobot, VID 0x2341 PID 0x8036).
> > > > + Exposes the battery status and capacity via the power_supply interface.
> > > > +
> > > > + To compile as a module, choose M here: the module will be
> > > > + called hid-lattepanda-iota-ups.
> > > > +
> > > > config HID_UCLOGIC
> > > > tristate "UC-Logic"
> > > > depends on USB_HID
> > > > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > > > index 0597fd6a4..d7ad3fc8f 100644
> > > > --- a/drivers/hid/Makefile
> > > > +++ b/drivers/hid/Makefile
> > > > @@ -74,6 +74,7 @@ obj-$(CONFIG_HID_KENSINGTON) += hid-kensington.o
> > > > obj-$(CONFIG_HID_KEYTOUCH) += hid-keytouch.o
> > > > obj-$(CONFIG_HID_KYE) += hid-kye.o
> > > > obj-$(CONFIG_HID_KYSONA) += hid-kysona.o
> > > > +obj-$(CONFIG_HID_LATTEPANDA_IOTA_UPS) += hid-lattepanda-iota-ups.o
> > > > obj-$(CONFIG_HID_LCPOWER) += hid-lcpower.o
> > > > obj-$(CONFIG_HID_LENOVO) += hid-lenovo.o
> > > > obj-$(CONFIG_HID_LENOVO_GO) += hid-lenovo-go.o
> > > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > > > index 4657d96fb..6ded2c943 100644
> > > > --- a/drivers/hid/hid-ids.h
> > > > +++ b/drivers/hid/hid-ids.h
> > > > @@ -859,6 +859,9 @@
> > > > #define USB_DEVICE_ID_LD_HYBRID 0x2090
> > > > #define USB_DEVICE_ID_LD_HEATCONTROL 0x20A0
> > > >
> > > > +#define USB_VENDOR_ID_LATTEPANDA_IOTA 0x2341
> > > > +#define USB_DEVICE_ID_LATTEPANDA_IOTA_UPS 0x8036
> > > > +
> > > > #define USB_VENDOR_ID_LENOVO 0x17ef
> > > > #define USB_DEVICE_ID_LENOVO_TPKBD 0x6009
> > > > #define USB_DEVICE_ID_LENOVO_CUSBKBD 0x6047
> > > > diff --git a/drivers/hid/hid-lattepanda-iota-ups.c b/drivers/hid/hid-lattepanda-iota-ups.c
> > > > new file mode 100644
> > > > index 000000000..f5d522695
> > > > --- /dev/null
> > > > +++ b/drivers/hid/hid-lattepanda-iota-ups.c
> > > > @@ -0,0 +1,409 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +#include <linux/power_supply.h>
> > > > +#include <linux/completion.h>
> > > > +#include <linux/workqueue.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/spinlock.h>
> > > > +#include <linux/hid.h>
> > > > +#include <linux/usb.h>
> > > > +#include "hid-ids.h"
> > > > +
> > > > +#define REPORT_ID_CAPACITY 0x0C
> > > > +#define REPORT_ID_STATUS 0x07
> > > > +
> > > > +#define STATUS_DISCHARGING BIT(1)
> > > > +#define STATUS_PLUGGED_IN BIT(0)
> > > > +#define STATUS_CHARGING BIT(2)
> > > > +
> > > > +MODULE_AUTHOR("Andrew Maney");
> > > > +MODULE_DESCRIPTION("LattePanda IOTA UPS power supply driver");
> > > > +MODULE_LICENSE("GPL");
> > > > +
> > > > +struct iota_ups {
> > > > + struct power_supply_desc psu_desc;
> > > > + struct power_supply *psu;
> > > > + struct hid_device *hiddev;
> > > > + spinlock_t lock; /* Protects cached HID report values */
> > > > +
> > > > + /* Cached values that are updated from HID reports */
> > > > + bool plugged_in;
> > > > + char serial[64];
> > > > + int charge_limit;
> > > > + int psu_status;
> > > > + int capacity;
> > > > +
> > > > + /*
> > > > + * Wait for both status and capacity reports before registering
> > > > + * with the power_supply core, so initial values are correct and
> > > > + * not erroneous.
> > > > + */
> > > > + struct completion got_initial_data;
> > > > + struct work_struct register_work;
> > > > + bool got_capacity;
> > > > + bool data_ready;
> > > > + bool got_status;
> > > > +};
> > > > +
> > > > +static enum power_supply_property iota_ups_properties[] = {
> > > > + POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD,
> > > > + POWER_SUPPLY_PROP_SERIAL_NUMBER,
> > > > + POWER_SUPPLY_PROP_MANUFACTURER,
> > > > + POWER_SUPPLY_PROP_MODEL_NAME,
> > > > + POWER_SUPPLY_PROP_TECHNOLOGY,
> > > > + POWER_SUPPLY_PROP_CAPACITY,
> > > > + POWER_SUPPLY_PROP_PRESENT,
> > > > + POWER_SUPPLY_PROP_ONLINE,
> > > > + POWER_SUPPLY_PROP_STATUS,
> > > > + POWER_SUPPLY_PROP_SCOPE,
> > > > +};
> > > > +
> > > > +static const struct hid_device_id iota_ups_devices[] = {
> > > > + { HID_USB_DEVICE(USB_VENDOR_ID_LATTEPANDA_IOTA,
> > > > + USB_DEVICE_ID_LATTEPANDA_IOTA_UPS) },
> > > > + { }
> > > > +};
> > > > +MODULE_DEVICE_TABLE(hid, iota_ups_devices);
> > > > +
> > > > +static int iota_ups_get_property(struct power_supply *supply,
> > > > + enum power_supply_property psp,
> > > > + union power_supply_propval *val)
> > > > +{
> > > > + struct iota_ups *ups = power_supply_get_drvdata(supply);
> > > > + unsigned long flags;
> > > > +
> > > > + spin_lock_irqsave(&ups->lock, flags);
> > > > +
> > > > + switch (psp) {
> > > > + case POWER_SUPPLY_PROP_STATUS:
> > > > + val->intval = ups->psu_status;
> > > > + break;
> > > > +
> > > > + /* Remaining capacity as a percentage from 0 to 100 */
> > > > + case POWER_SUPPLY_PROP_CAPACITY:
> > > > + val->intval = ups->capacity;
> > > > + break;
> > > > +
> > > > + /* The UPS is always present if the driver is loaded */
> > > > + case POWER_SUPPLY_PROP_PRESENT:
> > > > + val->intval = 1;
> > > > + break;
> > > > +
> > > > + /* Whether mains power is connected */
> > > > + case POWER_SUPPLY_PROP_ONLINE:
> > > > + val->intval = ups->plugged_in ? 1 : 0;
> > > > + break;
> > > > +
> > > > + /*
> > > > + * The UPS board supplies power to the IOTA and any
> > > > + * peripherals connected to it, therefore its scope
> > > > + * is system-wide.
> > > > + */
> > > > + case POWER_SUPPLY_PROP_SCOPE:
> > > > + val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
> > > > + break;
> > > > +
> > > > + /* V1.0 only accepts 18650 Li-ion cells */
> > > > + case POWER_SUPPLY_PROP_TECHNOLOGY:
> > > > + val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
> > > > + break;
> > > > +
> > > > + /* 80% or 100%, configured via a DIP switch on the UPS board */
> > > > + case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
> > > > + val->intval = ups->charge_limit;
> > > > + break;
> > > > +
> > > > + case POWER_SUPPLY_PROP_MANUFACTURER:
> > > > + val->strval = "DFRobot";
> > > > + break;
> > > > +
> > > > + case POWER_SUPPLY_PROP_MODEL_NAME:
> > > > + val->strval = "LattePanda IOTA UPS";
> > > > + break;
> > > > +
> > > > + /* Retrieved from the USB descriptor */
> > > > + case POWER_SUPPLY_PROP_SERIAL_NUMBER:
> > > > + val->strval = ups->serial;
> > > > + break;
> > > > +
> > > > + default:
> > > > + spin_unlock_irqrestore(&ups->lock, flags);
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + spin_unlock_irqrestore(&ups->lock, flags);
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int iota_ups_set_property(struct power_supply *supply,
> > > > + enum power_supply_property psp,
> > > > + const union power_supply_propval *val)
> > > > +{
> > > > + struct iota_ups *ups = power_supply_get_drvdata(supply);
> > > > +
> > > > + if (psp == POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD) {
> > > > + unsigned long flags;
> > > > +
> > > > + /*
> > > > + * V1.0 supports 80% and 100% charge limits only, which is
> > > > + * set via a DIP switch on the board. This property allows
> > > > + * userspace to inform the driver which limit is configured.
> > > > + */
> > > > + if (val->intval != 80 && val->intval != 100)
> > > > + return -EINVAL;
> > > > +
> > > > + spin_lock_irqsave(&ups->lock, flags);
> > > > + ups->charge_limit = val->intval;
> > > > + spin_unlock_irqrestore(&ups->lock, flags);
> > > > + return 0;
> > > > + }
> > > > +
> > > > + return -EINVAL;
> > > > +}
> > > > +
> > > > +static int iota_ups_property_is_writable(struct power_supply *supply,
> > > > + enum power_supply_property psp)
> > > > +{
> > > > + return psp == POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD;
> > > > +}
> > > > +
> > > > +static int iota_ups_raw_event(struct hid_device *hdev,
> > > > + struct hid_report *report,
> > > > + u8 *data, int size)
> > > > +{
> > > > + struct iota_ups *ups = hid_get_drvdata(hdev);
> > > > + unsigned long flags;
> > > > + bool changed = false;
> > > > +
> > > > + /* All of the UPS's reports are at least 2 bytes */
> > > > + if (size < 2)
> > > > + return 0;
> > > > +
> > > > + spin_lock_irqsave(&ups->lock, flags);
> > > > +
> > > > + switch (data[0]) {
> > > > + case REPORT_ID_STATUS: {
> > > > + u8 status = data[1];
> > > > + int new_status;
> > > > + bool plugged_in = !!(status & STATUS_PLUGGED_IN);
> > > > +
> > > > + /*
> > > > + * The UPS status is determined as follows:
> > > > + * Battery full:
> > > > + * UPS is plugged in
> > > > + * Battery is at full capacity
> > > > + *
> > > > + * Battery charging:
> > > > + * UPS is plugged in
> > > > + * Battery is not at full capacity
> > > > + *
> > > > + * Battery discharging:
> > > > + * UPS is not plugged in
> > > > + *
> > > > + * Battery not charging:
> > > > + * UPS is plugged in
> > > > + * UPS has halted charging for some reason
> > > > + *
> > > > + * Unknown:
> > > > + * None of the above conditions are met
> > > > + */
> > > > + if (status & STATUS_CHARGING) {
> > > > + if (ups->capacity >= ups->charge_limit)
> > > > + new_status = POWER_SUPPLY_STATUS_FULL;
> > > > + else
> > > > + new_status = POWER_SUPPLY_STATUS_CHARGING;
> > > > +
> > > > + } else if (status & STATUS_DISCHARGING) {
> > > > + new_status = POWER_SUPPLY_STATUS_DISCHARGING;
> > > > +
> > > > + } else if (plugged_in) {
> > > > + new_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> > > > +
> > > > + } else {
> > > > + new_status = POWER_SUPPLY_STATUS_UNKNOWN;
> > > > + }
> > > > +
> > > > + if (new_status != ups->psu_status ||
> > > > + plugged_in != ups->plugged_in) {
> > > > + ups->plugged_in = plugged_in;
> > > > + ups->psu_status = new_status;
> > > > + changed = true;
> > > > + }
> > > > +
> > > > + ups->got_status = true;
> > > > + break;
> > > > + }
> > > > +
> > > > + case REPORT_ID_CAPACITY: {
> > > > + int new_cap = clamp((int)data[1], 0, 100);
> > > > +
> > > > + if (new_cap != ups->capacity) {
> > > > + ups->capacity = new_cap;
> > > > + changed = true;
> > > > + }
> > > > +
> > > > + ups->got_capacity = true;
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + /*
> > > > + * Signal that the UPS is ready to be registered because we have
> > > > + * received both capacity and status reports.
> > > > + */
> > > > + if (!ups->data_ready && ups->got_status && ups->got_capacity) {
> > > > + ups->data_ready = true;
> > > > + complete(&ups->got_initial_data);
> > > > + }
> > > > +
> > > > + spin_unlock_irqrestore(&ups->lock, flags);
> > > > +
> > > > + /*
> > > > + * Notify the power_supply core outside the spinlock to avoid
> > > > + * a deadlock; power_supply_changed() may call back into
> > > > + * get_property() which acquires the same lock.
> > > > + */
> > > > + if (changed && ups->psu)
> > > > + power_supply_changed(ups->psu);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void iota_ups_register_work(struct work_struct *work)
> > > > +{
> > > > + struct iota_ups *ups = container_of(work, struct iota_ups, register_work);
> > > > + struct power_supply_config psu_config = {};
> > > > + struct power_supply *psu;
> > > > +
> > > > + /*
> > > > + * Wait for both status and capacity reports before registering.
> > > > + * The device sends reports every ~1 second, so 3 seconds is safe.
> > > > + * We wait here in order to prevent registration in an unknown
> > > > + * state, since this could cause emergency shutdowns or other
> > > > + * undesired effects.
> > > > + */
> > > > + wait_for_completion_timeout(&ups->got_initial_data,
> > > > + msecs_to_jiffies(3000));
> > > > +
> > > > + /* Configure the UPS's power supply properties */
> > > > + ups->psu_desc.name = devm_kasprintf(&ups->hiddev->dev, GFP_KERNEL,
> > > > + "lattepanda-iota-ups.%s",
> > > > + dev_name(&ups->hiddev->dev));
> > > > +
> > > > + if (!ups->psu_desc.name) {
> > > > + hid_err(ups->hiddev, "failed to allocate power supply name\n");
> > > > + return;
> > > > + }
> > > > +
> > > > + ups->psu_desc.property_is_writeable = iota_ups_property_is_writable;
> > > > + ups->psu_desc.num_properties = ARRAY_SIZE(iota_ups_properties);
> > > > + ups->psu_desc.get_property = iota_ups_get_property;
> > > > + ups->psu_desc.set_property = iota_ups_set_property;
> > > > + ups->psu_desc.properties = iota_ups_properties;
> > > > + ups->psu_desc.type = POWER_SUPPLY_TYPE_BATTERY;
> > > > + psu_config.drv_data = ups;
> > > > +
> > > > + /* Register the UPS as a power_supply device */
> > > > + psu = devm_power_supply_register(&ups->hiddev->dev, &ups->psu_desc, &psu_config);
> > > > + if (IS_ERR(psu)) {
> > > > + hid_err(ups->hiddev, "power supply registration failed: %pe\n", psu);
> > > > + return;
> > > > + }
> > > > +
> > > > + /*
> > > > + * Finally, notify the power_supply core so userspace reads the correct
> > > > + * initial state immediately after registration.
> > > > + */
> > > > + ups->psu = psu;
> > > > + power_supply_changed(ups->psu);
> > > > + hid_info(ups->hiddev, "LattePanda IOTA UPS registered as a power_supply device\n");
> > > > +}
> > > > +
> > > > +static int iota_ups_probe(struct hid_device *hdev,
> > > > + const struct hid_device_id *id)
> > > > +{
> > > > + struct iota_ups *ups;
> > > > + int ret;
> > > > +
> > > > + ups = devm_kzalloc(&hdev->dev, sizeof(*ups), GFP_KERNEL);
> > > > + if (!ups)
> > > > + return -ENOMEM;
> > > > +
> > > > + ups->hiddev = hdev;
> > > > + ups->psu_status = POWER_SUPPLY_STATUS_UNKNOWN;
> > > > +
> > > > + /* 50% is a safe default if wait_for_completion_timeout() times out. */
> > > > + ups->capacity = 50;
> > > > +
> > > > + /*
> > > > + * Default to 100% to prevent unexpected shutdowns.
> > > > + * Userspace can update this via charge_control_end_threshold.
> > > > + */
> > > > + ups->charge_limit = 100;
> > > > +
> > > > + init_completion(&ups->got_initial_data);
> > > > + spin_lock_init(&ups->lock);
> > > > + hid_set_drvdata(hdev, ups);
> > > > +
> > > > + /*
> > > > + * Retrieve the UPS's serial number from the USB descriptor. If the device is not
> > > > + * a USB device, we can use the unique device identifier as the serial number.
> > > > + */
> > > > + if (hid_is_usb(hdev)) {
> > > > + struct usb_device *udev = to_usb_device(hdev->dev.parent->parent);
> > > > +
> > > > + if (udev->serial)
> > > > + strscpy(ups->serial, udev->serial, sizeof(ups->serial));
> > > > + else
> > > > + strscpy(ups->serial, "Unknown", sizeof(ups->serial));
> > > > + } else {
> > > > + if (*hdev->uniq)
> > > > + strscpy(ups->serial, hdev->uniq, sizeof(ups->serial));
> > > > + else
> > > > + strscpy(ups->serial, "Unknown", sizeof(ups->serial));
> > > > + }
> > > > +
> > > > + ret = hid_parse(hdev);
> > > > + if (ret) {
> > > > + hid_err(hdev, "HID parse failed: %pe\n", ERR_PTR(ret));
> > > > + return ret;
> > > > + }
> > > > +
> > > > + ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> > > > + if (ret) {
> > > > + hid_err(hdev, "HID hw start failed: %pe\n", ERR_PTR(ret));
> > > > + return ret;
> > > > + }
> > > > +
> > > > + ret = hid_hw_open(hdev);
> > > > + if (ret) {
> > > > + hid_err(hdev, "HID hw open failed: %pe\n", ERR_PTR(ret));
> > > > + goto err_stop;
> > > > + }
> > > > +
> > > > + /* Probe for the UPS in a worker queue so we don't halt the enumeration thread */
> > > > + INIT_WORK(&ups->register_work, iota_ups_register_work);
> > > > + schedule_work(&ups->register_work);
> > > > + return 0;
> > > > +
> > > > +err_stop:
> > > > + hid_hw_stop(hdev);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static void iota_ups_remove(struct hid_device *hdev)
> > > > +{
> > > > + struct iota_ups *ups = hid_get_drvdata(hdev);
> > > > +
> > > > + cancel_work_sync(&ups->register_work);
> > > > + hid_hw_close(hdev);
> > > > + hid_hw_stop(hdev);
> > > > +}
> > > > +
> > > > +static struct hid_driver iota_ups_driver = {
> > > > + .name = "lattepanda-iota-ups",
> > > > + .id_table = iota_ups_devices,
> > > > + .probe = iota_ups_probe,
> > > > + .remove = iota_ups_remove,
> > > > + .raw_event = iota_ups_raw_event,
> > > > +};
> > > > +module_hid_driver(iota_ups_driver);
> > > > --
> > > > 2.54.0
> > > >
> > > >
> >
^ permalink raw reply
* Re: [PATCH v4 1/5] HID: asus: refactor the two workqueues and init sequence
From: Denis Benato @ 2026-06-16 1:00 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: linux-kernel, linux-input, Benjamin Tissoires, Jiri Kosina,
Luke D . Jones, Mateusz Schyboll, Denis Benato, Connor Belli,
sahiko-bot
In-Reply-To: <CAGwozwHC8ni6BCeEigEt0NQL+Ys2CqLP4_tVSpfA_zF3x52y-A@mail.gmail.com>
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.
> 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 v4 4/5] HID: asus: add support for xgm led
From: Antheas Kapenekakis @ 2026-06-15 21:55 UTC (permalink / raw)
To: Denis Benato
Cc: linux-kernel, linux-input, Benjamin Tissoires, Jiri Kosina,
Luke D . Jones, Mateusz Schyboll, Denis Benato, Connor Belli
In-Reply-To: <20260615165058.3845-5-denis.benato@linux.dev>
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.
Other than that and the refactor patch I'd rather see separately, the
other 4 patches look fine to me.
Reviewed-by: Antheas Kapenekakis <lkml@antheas.dev>
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 2/5] HID: asus: remove unnecessary OOM message
From: Antheas Kapenekakis @ 2026-06-15 21:52 UTC (permalink / raw)
To: Denis Benato
Cc: linux-kernel, linux-input, Benjamin Tissoires, Jiri Kosina,
Luke D . Jones, Mateusz Schyboll, Denis Benato, Connor Belli
In-Reply-To: <20260615165058.3845-3-denis.benato@linux.dev>
On Mon, 15 Jun 2026 at 18:51, Denis Benato <denis.benato@linux.dev> wrote:
>
> If devm_kzalloc fails an allocation error is already being reported:
> there is no need for the driver to repeat it, moreover if this were
> new code checkpatch.pl would throw a warning.
Perhaps something more of:
HID: asus: remove extraneous allocation warning
If devm_kzalloc fails an allocation error is already being reported.
Therefore, there is no need for the driver to repeat it. For new code,
this behavior is disincentivized and checkpatch.pl reports a warning.
Reviewed-by: Antheas Kapenekakis <lkml@antheas.dev>
> Signed-off-by: Denis Benato <denis.benato@linux.dev>
> ---
> drivers/hid/hid-asus.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 05ca6665e0a4..bbf964b12c16 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -1377,10 +1377,8 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
> int ret;
>
> drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
> - if (drvdata == NULL) {
> - hid_err(hdev, "Can't alloc Asus descriptor\n");
> + if (drvdata == NULL)
> return -ENOMEM;
> - }
>
> hid_set_drvdata(hdev, drvdata);
>
> --
> 2.47.3
>
>
^ permalink raw reply
* Re: [PATCH v4 1/5] HID: asus: refactor the two workqueues and init sequence
From: Antheas Kapenekakis @ 2026-06-15 21:49 UTC (permalink / raw)
To: Denis Benato
Cc: linux-kernel, linux-input, Benjamin Tissoires, Jiri Kosina,
Luke D . Jones, Mateusz Schyboll, Denis Benato, Connor Belli,
sahiko-bot
In-Reply-To: <20260615165058.3845-2-denis.benato@linux.dev>
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
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?
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 v4 3/5] HID: asus: fix a off-by-one issue making a check stricter that it needs to be
From: Antheas Kapenekakis @ 2026-06-15 21:44 UTC (permalink / raw)
To: Denis Benato
Cc: linux-kernel, linux-input, Benjamin Tissoires, Jiri Kosina,
Luke D . Jones, Mateusz Schyboll, Denis Benato, Connor Belli
In-Reply-To: <20260615165058.3845-4-denis.benato@linux.dev>
On Mon, 15 Jun 2026 at 18:51, Denis Benato <denis.benato@linux.dev> wrote:
>
> In mcu_parse_version_string() a size validation for response is stricter
> that it needs to be: relax the check by one byte.
"making a check stricter that it needs to be"
what check, you might need to reword the title
> Fixes: ("hid-asus: check ROG Ally MCU version and warn")
> Signed-off-by: Denis Benato <denis.benato@linux.dev>
> ---
> drivers/hid/hid-asus.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index bbf964b12c16..6896730efafc 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -835,7 +835,7 @@ static int mcu_parse_version_string(const u8 *response, size_t response_size)
> dots++;
> }
>
> - if (dots != 2 || p >= end || (p + 3) >= end)
> + if (dots != 2 || end - p < 3)
> return -EINVAL;
>
> memcpy(buf, p, 3);
> --
> 2.47.3
>
>
^ permalink raw reply
* Re: [PATCH v4 4/5] HID: asus: add support for xgm led
From: Denis Benato @ 2026-06-15 20:59 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260615170045.AF91E1F000E9@smtp.kernel.org>
On 6/15/26 19:00, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Propagating cosmetic LED restore errors during system resume causes the entire device resume to fail.
> - [High] Failing to initialize the auxiliary XG Mobile LED aborts the entire HID device probe.
> - [High] Manual call to `hid_hw_stop()` while using `devm_led_classdev_register()` leads to a use-after-teardown bug during device removal and probe error paths.
> --
>
> commit 26f82fa14c4bebc817bb5d79a1cdcdc34bb503d3
> Author: Denis Benato <denis.benato@linux.dev>
>
> HID: asus: add support for xgm led
>
> 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.
>
>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>> index 6896730efafca..e68a6d93369d8 100644
>> --- a/drivers/hid/hid-asus.c
>> +++ b/drivers/hid/hid-asus.c
> [ ... ]
>> @@ -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)
>> +{
> [ ... ]
>> + 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);
> [Severity: High]
> Because this LED is registered using devm_led_classdev_register(), its
> unregistration happens during the devres cleanup phase. This phase runs after
> the driver's remove callback (or probe error path) finishes.
>
> If the probe error path or asus_remove() manually calls hid_hw_stop(hdev),
> the HID transport is shut down. During the subsequent devres cleanup,
> led_classdev_unregister() is called and sets the LED to LED_OFF. This invokes
> asus_xgm_led_set(), which calls hid_hw_raw_request() on a stopped interface.
>
> Could this lead to a use-after-teardown bug?
>
>> + 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;
> [Severity: High]
> If asus_xgm_led_set() fails to restore the LED state (e.g., due to an I/O
> error or timeout), this returns the error code directly to the PM core.
> A failed resume callback causes the PM subsystem to treat the device resume as
> failed, potentially leaving primary features like the keyboard and touchpad
> unresponsive.
>
> Would it be better for the failure of a cosmetic LED restore to log a warning
> instead of aborting the entire device resume?
No: xg mobile device controllers are just that: not an input device: if it fails
there it's right to tear down the whole thing.
>> + }
>> + }
>> +
>> 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;
> [Severity: High]
> If asus_xgm_init() fails, the code jumps to err_stop_hw, failing the entire
> device probe.
>
> Does this make a non-critical aesthetic feature a hard dependency for the
> entire device? Might it be safer to log the failure rather than preventing
> the keyboard and touchpad from working?
Ditto. Won't resolve these as it would only make the code more confusing.
>> + }
>> + }
>> +
>> /* 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)) &&
^ permalink raw reply
* Re: [PATCH] Input: goodix - clamp the device-reported contact count
From: Bryam Vargas @ 2026-06-15 18:42 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel
In-Reply-To: <ai8WppWIAsSKXwFk@google.com>
On Sun, Jun 14, 2026 at 02:02:27PM -0700, Dmitry Torokhov wrote:
> Should we drop the report if is has bogus data in it?
We already do, for the per-interrupt count. goodix_ts_read_input_report()
drops a report whose reported touch count is out of range:
touch_num = data[0] & 0x0f;
if (touch_num > ts->max_touch_num)
return -EPROTO;
The only gap this patch closes is that ts->max_touch_num itself could be
larger than what point_data[] holds: it was taken straight from the 4-bit
config nibble (0..15), while the buffer is sized for GOODIX_MAX_CONTACTS
(10) -- 2 + 9*10 = 92 bytes. A config advertising 11..15 made that -EPROTO
check accept an out-of-range touch_num and overflow the stack buffer.
Clamping max_touch_num to GOODIX_MAX_CONTACTS at config-read time makes
that existing per-report drop a safe bound: a report claiming more than 10
contacts is now dropped with -EPROTO, and 10 is exactly point_data[]'s
capacity, so no legitimate report is lost (the driver maximum is 10, and
the other max_touch_num assignments already use GOODIX_MAX_CONTACTS).
If you'd rather flag a bogus config than silently clamp it, I'm happy to
add a dev_warn_ratelimited() in a v2 -- just say the word.
Thanks,
Bryam
^ permalink raw reply
* Re: [PATCH v3 2/2] Input: apple_z2 - bound the device-reported finger count
From: sashiko-bot @ 2026-06-15 18:32 UTC (permalink / raw)
To: Bryam Vargas via B4 Relay; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260615-b4-disp-05943ee1-v3-2-bbfe1b651613@proton.me>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [High] Concurrent execution of apple_z2_read_packet() from the probe thread and threaded IRQ handler causes buffer corruption.
- [High] Missing length check for `fw->size` before accessing the firmware header in `apple_z2_upload_firmware()`.
- [Medium] The timeout error path in `apple_z2_boot()` returns early without disabling the IRQ or asserting the reset GPIO.
- [Low] Missing input_set_drvdata() before device registration.
--
commit 647d8f6a84132a3fd0648807a009baeb1d458863
Author: Bryam Vargas <hexlabsecurity@proton.me>
Input: apple_z2 - bound the device-reported finger count
This commit adds bounds checking to apple_z2_parse_touches() to ensure the
reported finger count does not exceed the receive buffer. It drops packets
that claim more fingers than can fit, preventing an out-of-bounds read that
could leak adjacent kernel memory.
Closes: https://lore.kernel.org/all/20260613215358.329921F000E9@smtp.kernel.org/
> diff --git a/drivers/input/touchscreen/apple_z2.c b/drivers/input/touchscreen/apple_z2.c
> index ff9ff97be185a..4e2a6967436e8 100644
> --- a/drivers/input/touchscreen/apple_z2.c
> +++ b/drivers/input/touchscreen/apple_z2.c
[ ... ]
[Severity: High]
This isn't a bug introduced by this patch, but does concurrent execution of
apple_z2_read_packet() from the probe thread and threaded IRQ handler cause
buffer corruption?
In apple_z2_upload_firmware():
z2->booted = true;
apple_z2_read_packet(z2);
If the device asserts the IRQ line here, the unmasked IRQ handler
apple_z2_irq() wakes up, observes booted is true, and also calls
apple_z2_read_packet():
else
apple_z2_read_packet(z2);
Could this result in a data race where SPI DMA transfers corrupt the receive
buffer while apple_z2_parse_touches() is executing?
[Severity: High]
This is a pre-existing issue, but does apple_z2_upload_firmware() miss a
length check for fw->size before accessing the firmware header?
fw_hdr = (const struct apple_z2_fw_hdr *)fw->data;
if (le32_to_cpu(fw_hdr->magic) != APPLE_Z2_FW_MAGIC ||
le32_to_cpu(fw_hdr->version) != 1) {
If the firmware file is truncated or empty, will this cause an out-of-bounds
read when the header fields are accessed?
[Severity: Medium]
This isn't a bug introduced by this patch, but I noticed the timeout error
path in apple_z2_boot() returns early without cleaning up:
if (!wait_for_completion_timeout(&z2->boot_irq, msecs_to_jiffies(20)))
return -ETIMEDOUT;
Does this leave the hardware out of reset and create an unbalanced IRQ enable
state? The subsequent error path for apple_z2_upload_firmware() correctly
performs these cleanup steps by disabling the IRQ and asserting the reset
GPIO.
[Severity: Low]
This is a pre-existing issue, but the input device is registered in
apple_z2_probe() without prior initialization of driver data:
error = input_register_device(z2->input_dev);
Is it intentional to omit input_set_drvdata() before registration here? The
subsystem guidelines state that all private driver data must be initialized
before registration to avoid visibility issues.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-b4-disp-05943ee1-v3-0-bbfe1b651613@proton.me?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