* Re: [PATCH v8 1/4] HID: hid-msi: Add MSI Claw configuration driver
From: sashiko-bot @ 2026-05-22 2:33 UTC (permalink / raw)
To: Derek J. Clark; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260522015518.1111290-2-derekjohn.clark@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Sysfs attributes remain active during and after device removal, leading to a Time-of-Check to Time-of-Use (TOCTOU) race condition and Use-After-Free of HID transport structures.
- [High] Calling the sleeping function `hid_hw_close()` while holding a spinlock in `claw_remove()`, causing a kernel panic.
- [Low] Mixing `goto` error handling with scope-based cleanups (`__free()` and `guard()`) in the same function.
--
commit 76e865e5c59094dfdc13a67a7954da1cc82124bc
Author: Derek J. Clark <derekjohn.clark@gmail.com>
HID: hid-msi: Add MSI Claw configuration driver
This commit adds a new HID configuration driver for the MSI Claw handheld PC.
It introduces the initial driver structure, including sysfs attributes to
control the gamepad mode, M-key behavior, and device reset commands.
> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> new file mode 100644
> index 0000000000000..40b16253abbb3
> --- /dev/null
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> +static int claw_hw_output_report(struct hid_device *hdev, u8 index, u8 *data,
> + size_t len, unsigned int timeout)
> +{
> + unsigned char *dmabuf __free(kfree) = NULL;
> + u8 header[] = { CLAW_OUTPUT_REPORT_ID, 0, 0, 0x3c, index };
> + struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> + size_t header_size = ARRAY_SIZE(header);
> + int ret;
> +
> + if (header_size + len > CLAW_PACKET_SIZE)
> + return -EINVAL;
> +
> + /* We can't use a devm_alloc reusable buffer without side effects during suspend */
> + dmabuf = kzalloc(CLAW_PACKET_SIZE, GFP_KERNEL);
> + if (!dmabuf)
> + return -ENOMEM;
> +
> + memcpy(dmabuf, header, header_size);
> + if (data && len)
> + memcpy(dmabuf + header_size, data, len);
> +
> + guard(mutex)(&drvdata->cfg_mutex);
> + if (timeout) {
> + reinit_completion(&drvdata->send_cmd_complete);
> + scoped_guard(spinlock_irqsave, &drvdata->cmd_lock) {
> + drvdata->waiting_cmd = index;
> + drvdata->waiting_for_ack = true;
> + drvdata->cmd_status = -ETIMEDOUT;
> + }
> + }
> +
> + dev_dbg(&hdev->dev, "Send data as raw output report: [%*ph]\n",
> + CLAW_PACKET_SIZE, dmabuf);
> +
> + ret = hid_hw_output_report(hdev, dmabuf, CLAW_PACKET_SIZE);
> + if (ret < 0)
> + goto err_clear_state;
[Severity: Low]
Is it okay to mix goto error labels with scope-based cleanups in the same
function?
According to the cleanup subsystem guidelines, functions should either fully
rely on scope-based cleanups or use manual cleanups entirely to avoid
confusing ownership semantics.
> +
> + ret = ret == CLAW_PACKET_SIZE ? 0 : -EIO;
> + if (ret)
> + goto err_clear_state;
> +
> + if (timeout) {
> + ret = wait_for_completion_interruptible_timeout(&drvdata->send_cmd_complete,
> + msecs_to_jiffies(timeout));
> +
> + dev_dbg(&hdev->dev, "Remaining timeout: %u\n", ret);
> + ret = ret > 0 ? drvdata->cmd_status : ret ?: -EBUSY;
> + }
> +
> +err_clear_state:
> + if (timeout) {
> + guard(spinlock_irqsave)(&drvdata->cmd_lock);
> + drvdata->waiting_cmd = CLAW_COMMAND_TYPE_NONE;
> + drvdata->waiting_for_ack = false;
> + }
> +
> + return ret;
> +}
[ ... ]
> +static ssize_t gamepad_mode_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct hid_device *hdev = to_hid_device(dev);
> + struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> + int i, ret = -EINVAL;
> + u8 data[2];
> +
> + scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
> + /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
> + if (!smp_load_acquire(&drvdata->gamepad_registered))
> + return -ENODEV;
> + }
[Severity: High]
Can this lead to a use-after-free during device disconnect?
If a sysfs thread calls gamepad_mode_store() and gets preempted right after
releasing registration_lock here, a concurrent device disconnect could
trigger msi_remove() which clears gamepad_registered.
msi_remove() will then call hid_hw_stop(), destroying the HID transport
structures. When the sysfs thread resumes and continues execution here:
> +
> + for (i = 0; i < ARRAY_SIZE(claw_gamepad_mode_text); i++) {
> + if (claw_gamepad_mode_text[i] && sysfs_streq(buf, claw_gamepad_mode_text[i])) {
> + ret = i;
> + break;
> + }
> + }
> + if (ret < 0)
> + return ret;
> +
> + data[0] = ret;
> + scoped_guard(spinlock_irqsave, &drvdata->mode_lock)
> + data[1] = drvdata->mkeys_function;
> +
> + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SWITCH_MODE, data, ARRAY_SIZE(data), 0);
It will call claw_hw_output_report() which delegates to hid_hw_output_report()
and accesses the now-destroyed transport structures.
Since devm_device_add_group() is used to register the sysfs attributes, the
sysfs group remains active until after msi_remove() finishes completely.
Should the sysfs group be removed synchronously before hid_hw_stop() is
called to prevent this race?
> + if (ret)
> + return ret;
> +
> + return count;
> +}
[ ... ]
> +static void claw_remove(struct hid_device *hdev)
> +{
> + struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +
> + if (!drvdata)
> + return;
> +
> + cancel_delayed_work_sync(&drvdata->cfg_resume);
> + cancel_delayed_work_sync(&drvdata->cfg_setup);
> +
> + guard(spinlock_irqsave)(&drvdata->registration_lock);
> + /* Pairs with smp_load_acquire in attribute show/store functions */
> + smp_store_release(&drvdata->gamepad_registered, false);
> +
> + hid_hw_close(hdev);
[Severity: High]
Does this call a sleeping function while holding a spinlock?
The guard() macro acquires registration_lock and holds it until the end of
the function. While this spinlock is held and interrupts are disabled,
hid_hw_close() is called, which attempts to acquire the sleeping mutex
ll_open_lock.
I noticed this is fixed in a later patch in the series by changing guard()
to scoped_guard(), but doesn't this break bisectability by introducing a
kernel panic in this commit?
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260522015518.1111290-1-derekjohn.clark@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH v8 2/4] HID: hid-msi: Add M-key mapping attributes
From: sashiko-bot @ 2026-05-22 3:27 UTC (permalink / raw)
To: Derek J. Clark; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260522015518.1111290-3-derekjohn.clark@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Command synchronization cross-talk due to unconsumed generic ACKs from `SYNC_TO_ROM`
- [Medium] TOCTOU race in `claw_profile_event` unconditionally overwrites `profile_pending`
- [Low] Invalid anonymous struct declaration in `struct claw_mkey_report`
--
commit b8dd002e01cbcbf34fdd54df7543cbafb1207a47
Author: Derek J. Clark <derekjohn.clark@gmail.com>
HID: hid-msi: Add M-key mapping attributes
This commit adds attributes that allow for remapping the M-keys with up to 5
values when in macro mode. It adds support for mappings from the gamepad,
keyboard, or mouse event devices.
> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 40b16253abbb3..b9901869075f6 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> @@ -87,25 +249,47 @@ struct claw_command_report {
> u8 report_id;
> u8 padding[2];
> u8 header_tail;
> u8 cmd;
> u8 data[59];
> } __packed;
>
> +struct claw_profile_report {
> + u8 profile;
> + __be16 read_addr;
> +} __packed;
> +
> +struct claw_mkey_report {
> + struct claw_profile_report;
[Severity: Low]
Does this act as a forward declaration rather than an embedded field? Under
standard C11 and GNU C, a struct with a tag cannot be an anonymous embedded
member without specific compiler extensions. Will this cause a compilation
error when accessing fields like mkeys->read_addr later?
> + u8 padding_0;
> + u8 padding_1;
> + u8 padding_2;
> + u8 codes[5];
> +} __packed;
[ ... ]
> @@ -137,6 +321,39 @@ static int claw_gamepad_mode_event(struct claw_drvdata *drvdata,
> return 0;
> }
>
> +static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_report *cmd_rep)
> +{
> + enum claw_profile_ack_pending profile;
> + struct claw_mkey_report *mkeys;
> + u8 *codes, key;
> + int i;
> +
> + scoped_guard(spinlock, &drvdata->profile_lock)
> + profile = drvdata->profile_pending;
> +
> + switch (profile) {
> + case CLAW_M1_PENDING:
> + case CLAW_M2_PENDING:
[ ... ]
> + default:
> + dev_dbg(&drvdata->hdev->dev,
> + "Got profile event without changes pending from command: %x\n",
> + cmd_rep->cmd);
> + return -EINVAL;
> + }
> + scoped_guard(spinlock, &drvdata->profile_lock)
> + drvdata->profile_pending = CLAW_NO_PENDING;
[Severity: Medium]
Could this unconditionally overwrite a new state if a timeout occurs? If a
command times out, the waiting thread might drop profile_mutex, allowing a
new thread to enter claw_mkey_show() and set profile_pending for a new
operation. If the delayed ACK from the first command arrives at that moment,
will this clear the new thread's state and cause its operation to fail?
> +
> + return 0;
> +}
[ ... ]
> @@ -442,6 +670,177 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
[ ... ]
> +static int claw_mkey_store(struct device *dev, const char *buf, u8 mkey)
> +{
[ ... ]
> +set_buttons:
> + scoped_guard(mutex, &drvdata->rom_mutex) {
> + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
> + (u8 *)&report, sizeof(report), 25);
> + if (ret)
> + return ret;
> + /* MCU will not send ACK until the USB transaction completes. ACK is sent
> + * immediately after and will hit the stale state machine, before the next
> + * command re-arms the state machine. Timeout 0 ensures no deadlock waiting
> + * for ACK that ill never come.
> + */
> + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 0);
[Severity: High]
Can this lead to command synchronization cross-talk? Since timeout is 0,
the driver won't wait for the ACK. If a user rapidly issues another sysfs
write, the next operation could take rom_mutex and send its own command.
Because generic ACKs appear to lack command IDs, will claw_raw_event()
blindly consume the orphaned ACK from this command and prematurely complete
the new command's wait before the MCU has actually processed it?
> + }
> +
> + return ret;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260522015518.1111290-1-derekjohn.clark@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH v8 3/4] HID: hid-msi: Add RGB control interface
From: sashiko-bot @ 2026-05-22 4:00 UTC (permalink / raw)
To: Derek J. Clark; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260522015518.1111290-4-derekjohn.clark@gmail.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Explicitly unregistering a devm-managed LED class device causes a double unregister and list corruption upon device teardown.
- [High] Calling sleepable functions that allocate memory with GFP_KERNEL or take sleeping locks while holding a spinlock.
- [High] Race condition between sysfs/LED attributes and `claw_remove()` allows `rgb_queue` to be scheduled after cancellation.
- [High] Missing cancellation of the `rgb_queue` delayed work during system suspend can cause USB I/O to a suspended device.
--
commit 34d0916b058e394f024a63889da5c0fbe434c516
Author: Derek J. Clark <derekjohn.clark@gmail.com>
HID: hid-msi: Add RGB control interface
Adds RGB control interface for MSI Claw devices. The MSI Claw uses a
fairly unique RGB interface with 9 total zones, supporting up to 8
sequential frames of RGB zone data. The driver mirrors the effects
available in Windows while keeping effect names consistent with Lenovo
drivers.
> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index b9901869075f6..0b875603dbb32 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> +static ssize_t effect_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct led_classdev_mc *led_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
> + struct claw_drvdata *drvdata = container_of(led_mc, struct claw_drvdata, led_mc);
> + int ret;
> +
> + scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
> + /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
> + if (!smp_load_acquire(&drvdata->rgb_registered))
> + return -ENODEV;
> + }
> +
> + ret = sysfs_match_string(claw_rgb_effect_text, buf);
> + if (ret < 0)
> + return ret;
> +
> + drvdata->rgb_effect = ret;
> + mod_delayed_work(system_wq, &drvdata->rgb_queue, msecs_to_jiffies(50));
[Severity: High]
Can mod_delayed_work() here race with claw_remove()?
If claw_remove() runs concurrently, it acquires the registration_lock, sets
rgb_registered to false, drops the lock, and calls
cancel_delayed_work_sync(&drvdata->rgb_queue).
It then calls device_remove_group(), which blocks until this sysfs store
completes. Once device_remove_group() returns, devres frees drvdata.
However, since this sysfs store function already checked rgb_registered and
dropped the lock, it will continue executing and call mod_delayed_work(),
re-arming the workqueue. Will this lead to the delayed work executing on
freed memory?
> +
> + return count;
> +}
[ ... ]
> +static void cfg_setup_fn(struct work_struct *work)
> +{
[ ... ]
> +try_gamepad:
> + scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
> + /* Pairs with smp_store_release from below */
> + if (!smp_load_acquire(&drvdata->gamepad_registered) && gamepad_ready) {
> + ret = devm_device_add_group(&drvdata->hdev->dev, &claw_gamepad_attr_group);
[Severity: High]
Does devm_device_add_group() sleep?
It allocates memory using GFP_KERNEL inside devres_alloc() and acquires
the sleeping kernfs_mutex during sysfs group creation. Calling this inside a
spinlock block with interrupts disabled will trigger a scheduling while atomic
panic. Can this be called outside the spinlock?
> + if (ret) {
> + dev_err(&drvdata->hdev->dev,
> + "Failed to create gamepad attrs: %d\n", ret);
> + goto try_rgb;
> + }
> +
> + /* Pairs with smp_load_acquire in attribute show/store functions */
> + smp_store_release(&drvdata->gamepad_registered, true);
> + }
> }
> - scoped_guard(spinlock_irqsave, &drvdata->registration_lock)
> - /* Pairs with smp_load_acquire in attribute show/store functions */
> - smp_store_release(&drvdata->gamepad_registered, true);
>
> - kobject_uevent(&drvdata->hdev->dev.kobj, KOBJ_CHANGE);
> +try_rgb:
> + /* Add and enable RGB interface once we have the device state */
> + scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
> + /* Pairs with smp_store_release from below */
> + if (!smp_load_acquire(&drvdata->rgb_registered) && rgb_ready) {
> + ret = devm_led_classdev_multicolor_register(&drvdata->hdev->dev,
> + &drvdata->led_mc);
[Severity: High]
Does devm_led_classdev_multicolor_register() also sleep?
Similar to devm_device_add_group(), this registers a device which takes
sleeping locks and allocates memory with GFP_KERNEL. Should this be called
outside of the spinlock block?
> + if (ret) {
> + dev_err(&drvdata->hdev->dev,
> + "Failed to create led device: %d\n", ret);
> + goto update_kobjects;
> + }
> +
> + ret = device_add_group(drvdata->led_mc.led_cdev.dev, &claw_rgb_attr_group);
> + if (ret) {
> + dev_err(&drvdata->hdev->dev,
> + "Failed to create RGB attrs: %d\n", ret);
> + led_classdev_unregister(&drvdata->led_mc.led_cdev);
[Severity: High]
Does this manual unregister cause a double-free on device teardown?
Since devm_led_classdev_multicolor_register() attaches a devres cleanup
action, manually calling led_classdev_unregister() here leaves the devres
action queued. When the driver is unbound, devres will execute
led_classdev_unregister() a second time on the same LED class device pointer.
Will this result in list corruption and use-after-free?
> + goto update_kobjects;
> + }
> +
> + /* Pairs with smp_load_acquire in attribute show/store functions */
> + smp_store_release(&drvdata->rgb_registered, true);
> + }
> + }
[ ... ]
> @@ -971,9 +1593,11 @@ static int claw_probe(struct hid_device *hdev, u8 ep)
> spin_lock_init(&drvdata->cmd_lock);
> spin_lock_init(&drvdata->mode_lock);
> spin_lock_init(&drvdata->profile_lock);
> + spin_lock_init(&drvdata->frame_lock);
> init_completion(&drvdata->send_cmd_complete);
> INIT_DELAYED_WORK(&drvdata->cfg_resume, &cfg_resume_fn);
> INIT_DELAYED_WORK(&drvdata->cfg_setup, &cfg_setup_fn);
> + INIT_DELAYED_WORK(&drvdata->rgb_queue, &claw_rgb_queue_fn);
[Severity: High]
Is the rgb_queue delayed work properly cancelled during system suspend?
In claw_suspend(), the driver cancels cfg_resume and cfg_setup, but it does
not appear to cancel the newly introduced rgb_queue delayed work. Because
rgb_queue is queued on system_wq (which is not freezable), could it execute
after the device has been suspended? This might lead to USB I/O errors and
possible hardware lockups.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260522015518.1111290-1-derekjohn.clark@gmail.com?part=3
^ permalink raw reply
* Re: [PATCH 02/10] [v3] input: gpio-keys: make legacy gpiolib optional
From: Matti Vaittinen @ 2026-05-22 4:55 UTC (permalink / raw)
To: Arnd Bergmann, linux-gpio
Cc: linux-kernel, Arnd Bergmann, Christian Lamparter, Johannes Berg,
Aaro Koskinen, Andreas Kemnade, Kevin Hilman, Roger Quadros,
Tony Lindgren, Thomas Bogendoerfer, John Paul Adrian Glaubitz,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Linus Walleij, Bartosz Golaszewski,
Dmitry Torokhov, Lee Jones, Pavel Machek, Florian Fainelli,
Jonas Gorski, Andrew Lunn, Vladimir Oltean, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-wireless,
linux-omap, linux-arm-kernel, linux-mips, linux-sh, linux-input,
linux-leds, netdev
In-Reply-To: <20260520183815.2510387-3-arnd@kernel.org>
On 20/05/2026 21:38, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> Most users of gpio-keys and gpio-keys-polled use modern gpiolib
> interfaces, but there are still number of ancient sh, arm32 and x86
> machines that have never been converted.
>
> Add an #ifdef block for the parts of the driver that are only used on
> those legacy machines.
>
> The two Rohm PMIC drivers use a gpio-keys device without an actual GPIO,
> passing an IRQ number instead. In order to keep this working both with
> and with CONFIG_GPIOLIB_LEGACY, change the gpio-keys driver to ignore
> the gpio number if an IRQ is passed.
>
> Link: https://lore.kernel.org/all/b3c94552-c104-42e3-be15-7e8362e8039e@gmail.com/
> Link: https://lore.kernel.org/all/afJXG4_rtaj3l2Dk@google.com/
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v3: resend
> v2: skip the fake GPIO number passing from mfd
>
> The removal of the arm platforms using this is not yet going to happen
> for 7.2, and Dmitry's changes for the Rohm drivers have not yet
> made it into linux-next as of 2026-05-20, so for the moment I
> would still like to see this patch get merged, even if we are
> closing in on completely removing the legacy gpio support in
> the gpio_keys driver, so we can make CONFIG_GPIOLIB_LEGACY
> default-disabled sooner.
I am (still) all fine with this, even though I like Dmitry's set. I
suppose you already have a plan for merging this, but I still have to
ask - why the MFD changes aren't in own patch? I feel it would have
simplified merging, backporting, reviewing and reverting if needed.
Well, other than that:
Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
Yours,
-- Matti
---
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply
* [PATCH 1/2] HID: lenovo: Add support for ThinkPad X13 Folio keyboard
From: Vishnu @ 2026-05-22 5:06 UTC (permalink / raw)
To: jikos, bentiss, derekjohn.clark, mpearson-lenovo
Cc: vsankar, linux-input, linux-kernel, Vishnu Sankar
From: Vishnu Sankar <vishnuocv@gmail.com>
Add USB ID support for the ThinkPad X13 detachable keyboard.
The Keyboard uses the same HID raw event protocol as the ThinkPad
X12 Gen 2. The functionality stays the same with X12 Gen 2 Keyboards.
Also declare KEY_PERFORMANCE capability in lenovo_input_configured()
for X13 detachable, allowing userspace to discover the key via evdev
capability bits.
Signed-off-by: Vishnu Sankar <vishnuocv@gmail.com>
---
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-lenovo.c | 21 +++++++++++++++++----
2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 4657d96fb083..064be2333a00 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -874,6 +874,7 @@
#define USB_DEVICE_ID_LENOVO_X1_TAB3 0x60b5
#define USB_DEVICE_ID_LENOVO_X12_TAB 0x60fe
#define USB_DEVICE_ID_LENOVO_X12_TAB2 0x61ae
+#define USB_DEVICE_ID_LENOVO_X13_TAB 0x62af
#define USB_DEVICE_ID_LENOVO_YOGABOOK9I 0x6161
#define USB_DEVICE_ID_LENOVO_OPTICAL_USB_MOUSE_600E 0x600e
#define USB_DEVICE_ID_LENOVO_PIXART_USB_MOUSE_608D 0x608d
diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index a6b73e03c16b..35b00619f045 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -504,6 +504,7 @@ static int lenovo_input_mapping(struct hid_device *hdev,
usage, bit, max);
case USB_DEVICE_ID_LENOVO_X12_TAB:
case USB_DEVICE_ID_LENOVO_X12_TAB2:
+ case USB_DEVICE_ID_LENOVO_X13_TAB:
case USB_DEVICE_ID_LENOVO_X1_TAB:
case USB_DEVICE_ID_LENOVO_X1_TAB2:
case USB_DEVICE_ID_LENOVO_X1_TAB3:
@@ -620,6 +621,7 @@ static ssize_t attr_fn_lock_store(struct device *dev,
break;
case USB_DEVICE_ID_LENOVO_X12_TAB:
case USB_DEVICE_ID_LENOVO_X12_TAB2:
+ case USB_DEVICE_ID_LENOVO_X13_TAB:
case USB_DEVICE_ID_LENOVO_TP10UBKBD:
case USB_DEVICE_ID_LENOVO_X1_TAB:
case USB_DEVICE_ID_LENOVO_X1_TAB2:
@@ -791,9 +793,10 @@ static int lenovo_raw_event(struct hid_device *hdev,
* Lenovo TP X12 Tab KBD's Fn+XX is HID raw data defined. Report ID is 0x03
* e.g.: Raw data received for MIC mute is 0x00020003.
*/
- if (unlikely((hdev->product == USB_DEVICE_ID_LENOVO_X12_TAB
- || hdev->product == USB_DEVICE_ID_LENOVO_X12_TAB2)
- && size >= 3 && report->id == 0x03))
+ if (unlikely((hdev->product == USB_DEVICE_ID_LENOVO_X12_TAB ||
+ hdev->product == USB_DEVICE_ID_LENOVO_X12_TAB2 ||
+ hdev->product == USB_DEVICE_ID_LENOVO_X13_TAB) &&
+ size >= 3 && report->id == 0x03))
return lenovo_raw_event_TP_X12_tab(hdev, le32_to_cpu(*(__le32 *)data));
return 0;
@@ -877,6 +880,7 @@ static int lenovo_event(struct hid_device *hdev, struct hid_field *field,
return lenovo_event_cptkbd(hdev, field, usage, value);
case USB_DEVICE_ID_LENOVO_X12_TAB:
case USB_DEVICE_ID_LENOVO_X12_TAB2:
+ case USB_DEVICE_ID_LENOVO_X13_TAB:
case USB_DEVICE_ID_LENOVO_TP10UBKBD:
case USB_DEVICE_ID_LENOVO_X1_TAB:
case USB_DEVICE_ID_LENOVO_X1_TAB2:
@@ -1161,6 +1165,7 @@ static int lenovo_led_brightness_set(struct led_classdev *led_cdev,
break;
case USB_DEVICE_ID_LENOVO_X12_TAB:
case USB_DEVICE_ID_LENOVO_X12_TAB2:
+ case USB_DEVICE_ID_LENOVO_X13_TAB:
case USB_DEVICE_ID_LENOVO_TP10UBKBD:
case USB_DEVICE_ID_LENOVO_X1_TAB:
case USB_DEVICE_ID_LENOVO_X1_TAB2:
@@ -1355,7 +1360,8 @@ static int lenovo_probe_tp10ubkbd(struct hid_device *hdev)
*/
data->fn_lock = !(hdev->product == USB_DEVICE_ID_LENOVO_X12_TAB ||
- hdev->product == USB_DEVICE_ID_LENOVO_X12_TAB2);
+ hdev->product == USB_DEVICE_ID_LENOVO_X12_TAB2 ||
+ hdev->product == USB_DEVICE_ID_LENOVO_X13_TAB);
lenovo_led_set_tp10ubkbd(hdev, TP10UBKBD_FN_LOCK_LED, data->fn_lock);
@@ -1402,6 +1408,7 @@ static int lenovo_probe(struct hid_device *hdev,
break;
case USB_DEVICE_ID_LENOVO_X12_TAB:
case USB_DEVICE_ID_LENOVO_X12_TAB2:
+ case USB_DEVICE_ID_LENOVO_X13_TAB:
case USB_DEVICE_ID_LENOVO_TP10UBKBD:
case USB_DEVICE_ID_LENOVO_X1_TAB:
case USB_DEVICE_ID_LENOVO_X1_TAB2:
@@ -1490,6 +1497,7 @@ static void lenovo_remove(struct hid_device *hdev)
break;
case USB_DEVICE_ID_LENOVO_X12_TAB:
case USB_DEVICE_ID_LENOVO_X12_TAB2:
+ case USB_DEVICE_ID_LENOVO_X13_TAB:
case USB_DEVICE_ID_LENOVO_TP10UBKBD:
case USB_DEVICE_ID_LENOVO_X1_TAB:
case USB_DEVICE_ID_LENOVO_X1_TAB2:
@@ -1517,6 +1525,9 @@ static int lenovo_input_configured(struct hid_device *hdev,
hi->input->propbit);
}
break;
+ case USB_DEVICE_ID_LENOVO_X13_TAB:
+ input_set_capability(hi->input, EV_KEY, KEY_PERFORMANCE);
+ break;
}
return 0;
@@ -1551,6 +1562,8 @@ static const struct hid_device_id lenovo_devices[] = {
USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_X12_TAB) },
{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_X12_TAB2) },
+ { HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
+ USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_X13_TAB) },
{ HID_DEVICE(BUS_I2C, HID_GROUP_GENERIC,
USB_VENDOR_ID_ITE, I2C_DEVICE_ID_ITE_LENOVO_YOGA_SLIM_7X_KEYBOARD) },
{ }
--
2.54.0
^ permalink raw reply related
* [PATCH 2/2] HID: lenovo: Use KEY_PERFORMANCE capability for ThinkPad X12 Tab Gen 2
From: Vishnu @ 2026-05-22 5:06 UTC (permalink / raw)
To: jikos, bentiss, derekjohn.clark, mpearson-lenovo
Cc: vsankar, linux-input, linux-kernel, Vishnu Sankar
In-Reply-To: <20260522050632.8079-1-vishnuocv@gmail.com>
From: Vishnu Sankar <vishnuocv@gmail.com>
The X12 Tab Gen 2 emits KEY_PERFORMANCE via Fn+F8 through the raw
event handler but never declared the capability via
input_set_capability(). This prevents userspace tools from
discovering the key through evdev capability bits.
Signed-off-by: Vishnu Sankar <vishnuocv@gmail.com>
---
drivers/hid/hid-lenovo.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 35b00619f045..3492ba8d0fb0 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -1525,6 +1525,7 @@ static int lenovo_input_configured(struct hid_device *hdev,
hi->input->propbit);
}
break;
+ case USB_DEVICE_ID_LENOVO_X12_TAB2:
case USB_DEVICE_ID_LENOVO_X13_TAB:
input_set_capability(hi->input, EV_KEY, KEY_PERFORMANCE);
break;
--
2.54.0
^ permalink raw reply related
* Recall: [PATCH] HID: remove duplicate hid_warn_ratelimited definition
From: Liu Kai @ 2026-05-22 6:04 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Liu Kai would like to recall the message, "[PATCH] HID: remove duplicate hid_warn_ratelimited definition".
^ permalink raw reply
* Re: [PATCH 2/2] HID: lenovo: Use KEY_PERFORMANCE capability for ThinkPad X12 Tab Gen 2
From: sashiko-bot @ 2026-05-22 6:05 UTC (permalink / raw)
Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260522050632.8079-2-vishnuocv@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [Low] The patch unconditionally adds the KEY_PERFORMANCE capability to all input nodes exposed by the device, causing capability pollution on unrelated interfaces.
Pre-existing issues:
- [High] The raw event handler emits keys like KEY_RFKILL, KEY_BOOKMARKS, and KEY_KBDILLUMTOGGLE, but their capabilities are never declared, causing the input subsystem to silently drop these events.
- [High] The raw event handler contains a loop that unconditionally fires key events on the first input node it encounters, ignoring capability routing and causing events to be dropped.
--
commit 1b40b44e417a26a21650fec78e6a2ea55f535004
Author: Vishnu Sankar <vishnuocv@gmail.com>
HID: lenovo: Use KEY_PERFORMANCE capability for ThinkPad X12 Tab Gen 2
This commit adds the KEY_PERFORMANCE capability for the ThinkPad X12 Tab Gen 2
and X13 Tab devices during input configuration. This allows userspace tools to
discover the key emitted via the raw event handler.
> diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> index 35b00619f0456..3492ba8d0fb02 100644
> --- a/drivers/hid/hid-lenovo.c
> +++ b/drivers/hid/hid-lenovo.c
> @@ -1525,6 +1525,7 @@ static int lenovo_input_configured(struct hid_device *hdev,
> hi->input->propbit);
> }
> break;
> + case USB_DEVICE_ID_LENOVO_X12_TAB2:
> case USB_DEVICE_ID_LENOVO_X13_TAB:
> input_set_capability(hi->input, EV_KEY, KEY_PERFORMANCE);
[Severity: Low]
Does this unconditionally add the KEY_PERFORMANCE capability to all input
nodes exposed by the device?
Since lenovo_input_configured() is invoked for every hid_input interface,
this regression might advertise the capability on unrelated interfaces.
Could we filter this by checking the application usage or verifying it is
the primary keyboard node?
[Severity: High]
This isn't a bug introduced by this patch, but I noticed the raw event
handler also emits keys like KEY_RFKILL, KEY_PICKUP_PHONE, KEY_BOOKMARKS,
and KEY_KBDILLUMTOGGLE.
Should we also declare the capabilities for these keys here?
Since they are generated from raw reports rather than standard HID
descriptors, they appear to be missing from the capability bits, which would
cause the input subsystem to silently drop these events.
[Severity: High]
This is a pre-existing issue, but looking at how the raw events are handled
in lenovo_raw_event_TP_X12_tab(), is it possible the events are firing on
the wrong input node?
The handler loops over hdev->inputs but returns immediately after reporting
the key event on the first node it encounters. If the first device lacks the
specific key capabilities, the input core might discard the event.
Should the loop instead locate the specific input node that possesses the
corresponding key capability before dispatching?
> break;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260522050632.8079-1-vishnuocv@gmail.com?part=2
^ permalink raw reply
* Recall: [PATCH] HID: remove duplicate hid_warn_ratelimited definition
From: Liu Kai @ 2026-05-22 6:11 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Liu Kai would like to recall the message, "[PATCH] HID: remove duplicate hid_warn_ratelimited definition".
^ permalink raw reply
* Re: [syzbot] [input?] possible deadlock in tasklet_action_common (2)
From: Sebastian Andrzej Siewior @ 2026-05-22 6:39 UTC (permalink / raw)
To: Hillf Danton
Cc: syzbot, dmitry.torokhov, linux-input, Tetsuo Handa, linux-kernel,
syzkaller-bugs
In-Reply-To: <20260521223516.914-1-hdanton@sina.com>
On 2026-05-22 06:35:14 [+0800], Hillf Danton wrote:
> input_inject_event() is invoked in the brightness_set callback [1] for
> example.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/input/input-leds.c#n142
>
> > Now, input_inject_event() does spin_lock_irqsave() and alloc_skb() does
> > local_bh_disable()/ local_bh_enable(). On !RT this is not legal. On RT
>
> Can you please specify why that is illegal on !RT?
So if you do
spin_lock_irq();
local_bh_disable();
then the
local_bh_enable();
has lockdep_assert_irqs_enabled() which will yell. Then there is also
this local_irq_enable() which will enable interrupts before the unlock
which bad, again. Also, should softirqs been raised within this section,
the this enable will invoke the required callback which again, enable
interrupts.
> > it is okay but then local_bh_enable() here should not invoke any
> > softirqs because none were raised within the section (alloc_skb()).
>
> On RT spinlock is replaced with mutex, and softirq can be raised in the
> irq that could come any moment after spin_lock_irqsave().
That is true on the other hand. That means having raised another tasklet
could lead to the backtrace. But it would have been two different locks,
not blocking on each other.
Sebastian
^ permalink raw reply
* Re: [syzbot] [input?] possible deadlock in tasklet_action_common (2)
From: Hillf Danton @ 2026-05-22 7:21 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: syzbot, dmitry.torokhov, linux-input, Tetsuo Handa, linux-kernel,
syzkaller-bugs
In-Reply-To: <20260522063938.ewKum8vW@linutronix.de>
On Fri, 22 May 2026 08:39:38 +0200 Sebastian Andrzej Siewior wrote:
> On 2026-05-22 06:35:14 [+0800], Hillf Danton wrote:
> > input_inject_event() is invoked in the brightness_set callback [1] for
> > example.
> >
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/input/input-leds.c#n142
> >
> > > Now, input_inject_event() does spin_lock_irqsave() and alloc_skb() does
> > > local_bh_disable()/ local_bh_enable(). On !RT this is not legal. On RT
> >
> > Can you please specify why that is illegal on !RT?
>
> So if you do
> spin_lock_irq();
> local_bh_disable();
>
> then the
> local_bh_enable();
>
> has lockdep_assert_irqs_enabled() which will yell. Then there is also
Ah got it, thanks.
> this local_irq_enable() which will enable interrupts before the unlock
> which bad, again. Also, should softirqs been raised within this section,
> the this enable will invoke the required callback which again, enable
> interrupts.
>
> > > it is okay but then local_bh_enable() here should not invoke any
> > > softirqs because none were raised within the section (alloc_skb()).
> >
> > On RT spinlock is replaced with mutex, and softirq can be raised in the
> > irq that could come any moment after spin_lock_irqsave().
>
> That is true on the other hand. That means having raised another tasklet
> could lead to the backtrace. But it would have been two different locks,
> not blocking on each other.
>
The last question, by two different locks, do you mean that the
tasklet_sync_callback.cb_lock is per cpu?
^ permalink raw reply
* [PATCH] HID: core: Fix bsize printk specifier in hid_report_raw_event()
From: yaolu @ 2026-05-22 7:27 UTC (permalink / raw)
To: jikos, bentiss; +Cc: linux-input, linux-kernel, Lu Yao, k2ci
From: Lu Yao <yaolu@kylinos.cn>
Fix the following warning during build time:
drivers/hid/hid-core.c:2053:43: error: format ‘%ld’ expects argument of type ‘long int’, but argument 5 has type ‘size_t’ {aka ‘unsigned int’} [-Werror=format=]
drivers/hid/hid-core.c:2075:43: error: format ‘%ld’ expects argument of type ‘long int’, but argument 5 has type ‘size_t’ {aka ‘unsigned int’} [-Werror=format=]
Fixes: 2c85c61d1332 ("HID: pass the buffer size to hid_report_raw_event")
Reported-by: k2ci <kernel-bot@kylinos.cn>
Signed-off-by: Lu Yao <yaolu@kylinos.cn>
---
drivers/hid/hid-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index b3596851c719..c21c58d7bf02 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2050,7 +2050,7 @@ int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 *
return 0;
if (unlikely(bsize < csize)) {
- hid_warn_ratelimited(hid, "Event data for report %d is incorrect (%d vs %ld)\n",
+ hid_warn_ratelimited(hid, "Event data for report %d is incorrect (%d vs %u)\n",
report->id, csize, bsize);
return -EINVAL;
}
@@ -2072,7 +2072,7 @@ int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 *
rsize = max_buffer_size;
if (bsize < rsize) {
- hid_warn_ratelimited(hid, "Event data for report %d was too short (%d vs %ld)\n",
+ hid_warn_ratelimited(hid, "Event data for report %d was too short (%d vs %u)\n",
report->id, rsize, bsize);
return -EINVAL;
}
--
2.25.1
^ permalink raw reply related
* Re: [PATCH] HID: core: Fix bsize printk specifier in hid_report_raw_event()
From: sashiko-bot @ 2026-05-22 7:31 UTC (permalink / raw)
To: yaolu; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260522072718.186707-1-yaolu@kylinos.cn>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Incorrect format specifier for the `size_t` variable `bsize`. The patch replaces `%ld` with `%u` to fix a warning on 32-bit architectures (where `size_t` is `unsigned int`), but this introduces a new warning on 64-bit architectures (where `size_t` is `unsigned long`). The correct format specifier for `size_t` is `%zu`.
--
commit d6a5fb9c76c9718771a50e1f79f29b3df496f869
Author: Lu Yao <yaolu@kylinos.cn>
HID: core: Fix bsize printk specifier in hid_report_raw_event()
This commit attempts to fix a build warning on 32-bit architectures by
changing the format specifier for a size_t variable from %ld to %u.
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index b3596851c7191..c21c58d7bf02b 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -2050,7 +2050,7 @@ int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 *
> return 0;
>
> if (unlikely(bsize < csize)) {
> - hid_warn_ratelimited(hid, "Event data for report %d is incorrect (%d vs %ld)\n",
> + hid_warn_ratelimited(hid, "Event data for report %d is incorrect (%d vs %u)\n",
[Severity: Low]
Will this cause a new format string warning on 64-bit architectures? bsize is
of type size_t, which is an unsigned long on 64-bit systems. Should this use
the %zu format specifier instead to be portable across both 32-bit and 64-bit
architectures?
> report->id, csize, bsize);
> return -EINVAL;
> }
[ ... ]
> @@ -2072,7 +2072,7 @@ int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 *
> rsize = max_buffer_size;
>
> if (bsize < rsize) {
> - hid_warn_ratelimited(hid, "Event data for report %d was too short (%d vs %ld)\n",
> + hid_warn_ratelimited(hid, "Event data for report %d was too short (%d vs %u)\n",
[Severity: Low]
Similarly here, should this also use %zu for the size_t variable bsize?
> report->id, rsize, bsize);
> return -EINVAL;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260522072718.186707-1-yaolu@kylinos.cn?part=1
^ permalink raw reply
* Re: [syzbot] [input?] possible deadlock in tasklet_action_common (2)
From: Sebastian Andrzej Siewior @ 2026-05-22 7:35 UTC (permalink / raw)
To: Hillf Danton
Cc: syzbot, dmitry.torokhov, linux-input, Tetsuo Handa, linux-kernel,
syzkaller-bugs
In-Reply-To: <20260522072144.934-1-hdanton@sina.com>
On 2026-05-22 15:21:43 [+0800], Hillf Danton wrote:
> > > On RT spinlock is replaced with mutex, and softirq can be raised in the
> > > irq that could come any moment after spin_lock_irqsave().
> >
> > That is true on the other hand. That means having raised another tasklet
> > could lead to the backtrace. But it would have been two different locks,
> > not blocking on each other.
> >
> The last question, by two different locks, do you mean that the
> tasklet_sync_callback.cb_lock is per cpu?
Yes, it is but this does not matter. kbd_led_trigger_activate() does
tasklet_disable(&keyboard_tasklet) so you can't have kbd_bh() running
and led_set_brightness() which would kick the softirq again. Not from
kbd_led_trigger_activate().
Even if another component would raise a softirq in that window, you
could run tasklets, yes, but never kbd_bh(). Not in this window.
So the kbd_led_trigger_activate() -> input_inject_event() needs to be a
different event device than kbd_propagate_led_state() -> event. I don't
see how you could unfold this.
I *think* lockdep observed all this possible interactions and sketched
this possibility but it does not know about tasklet_disable().
I do have an idea how to avoid alloc_skb() invoking softirqs if someone
else raised them while alloc_skb() was preempted. This could avoid this
kind of splat.
Sebastian
^ permalink raw reply
* Re: [PATCH 02/10] [v3] input: gpio-keys: make legacy gpiolib optional
From: Arnd Bergmann @ 2026-05-22 8:28 UTC (permalink / raw)
To: Matti Vaittinen, Arnd Bergmann, open list:GPIO SUBSYSTEM
Cc: linux-kernel, Christian Lamparter, Johannes Berg, Aaro Koskinen,
Andreas Kemnade, Kevin Hilman, Roger Quadros, Tony Lindgren,
Thomas Bogendoerfer, John Paul Adrian Glaubitz, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
Linus Walleij, Bartosz Golaszewski, Dmitry Torokhov, Lee Jones,
Pavel Machek, Florian Fainelli, Jonas Gorski, Andrew Lunn,
Vladimir Oltean, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, linux-wireless, Linux-OMAP, linux-arm-kernel,
linux-mips, linux-sh, linux-input, linux-leds, Netdev
In-Reply-To: <b79d8ad9-f36e-4769-9dc2-58baefe23000@gmail.com>
On Fri, May 22, 2026, at 06:55, Matti Vaittinen wrote:
> On 20/05/2026 21:38, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>>
>> The two Rohm PMIC drivers use a gpio-keys device without an actual GPIO,
>> passing an IRQ number instead. In order to keep this working both with
>> and with CONFIG_GPIOLIB_LEGACY, change the gpio-keys driver to ignore
>> the gpio number if an IRQ is passed.
>>
>
> I am (still) all fine with this, even though I like Dmitry's set. I
> suppose you already have a plan for merging this, but I still have to
> ask - why the MFD changes aren't in own patch? I feel it would have
> simplified merging, backporting, reviewing and reverting if needed.
Splitting it out would break bisection: The gpio-leds change without
the mfd change causes a build failure when assigning the .gpio
field, and the reverse causes a runtime failure when .gpio=0 is
a valid line.
It would be possible to only do the gpio-led driver patch without
the header file change first. This would let us apply the last
patch in the series without regression, but it would risk having
not catching other drivers that incorrectly set the .gpio field
while CONFIG_GPIOLIB_LEGACY is disabled.
Arnd
^ permalink raw reply
* Re: [PATCH] HID: core: Fix bsize printk specifier in hid_report_raw_event()
From: Benjamin Tissoires @ 2026-05-22 8:46 UTC (permalink / raw)
To: yaolu; +Cc: jikos, linux-input, linux-kernel, k2ci
In-Reply-To: <20260522072718.186707-1-yaolu@kylinos.cn>
On May 22 2026, yaolu@kylinos.cn wrote:
> From: Lu Yao <yaolu@kylinos.cn>
>
> Fix the following warning during build time:
>
> drivers/hid/hid-core.c:2053:43: error: format ‘%ld’ expects argument of type ‘long int’, but argument 5 has type ‘size_t’ {aka ‘unsigned int’} [-Werror=format=]
> drivers/hid/hid-core.c:2075:43: error: format ‘%ld’ expects argument of type ‘long int’, but argument 5 has type ‘size_t’ {aka ‘unsigned int’} [-Werror=format=]
>
> Fixes: 2c85c61d1332 ("HID: pass the buffer size to hid_report_raw_event")
> Reported-by: k2ci <kernel-bot@kylinos.cn>
> Signed-off-by: Lu Yao <yaolu@kylinos.cn>
In addition to shashiko comments about %zu, it's already in linus' tree:
4d3a2a466b8d ("HID: core: Fix size_t specifier in hid_report_raw_event()")
Cheers,
Benjamin
> ---
> drivers/hid/hid-core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index b3596851c719..c21c58d7bf02 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -2050,7 +2050,7 @@ int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 *
> return 0;
>
> if (unlikely(bsize < csize)) {
> - hid_warn_ratelimited(hid, "Event data for report %d is incorrect (%d vs %ld)\n",
> + hid_warn_ratelimited(hid, "Event data for report %d is incorrect (%d vs %u)\n",
> report->id, csize, bsize);
> return -EINVAL;
> }
> @@ -2072,7 +2072,7 @@ int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 *
> rsize = max_buffer_size;
>
> if (bsize < rsize) {
> - hid_warn_ratelimited(hid, "Event data for report %d was too short (%d vs %ld)\n",
> + hid_warn_ratelimited(hid, "Event data for report %d was too short (%d vs %u)\n",
> report->id, rsize, bsize);
> return -EINVAL;
> }
> --
> 2.25.1
>
>
^ permalink raw reply
* [PATCH] Input: ims-pcu - fix usb_free_coherent() size in ims_pcu_buffers_alloc()
From: Thomas Fourier @ 2026-05-22 8:54 UTC (permalink / raw)
Cc: Thomas Fourier, stable, Dmitry Torokhov, Dan Carpenter,
Seungjin Bae, Sanghoon Choi, Kees Cook, linux-input, linux-kernel
The input buffer size is pcu->max_in_size, but pcu->max_out_size is
passed to usb_free_coherent().
Change size to match the allocation size.
Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
Cc: <stable@vger.kernel.org>
Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>
---
drivers/input/misc/ims-pcu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 4c022a36dbe8..7a1cb9333f53 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -1624,7 +1624,7 @@ static void ims_pcu_buffers_free(struct ims_pcu *pcu)
usb_kill_urb(pcu->urb_in);
usb_free_urb(pcu->urb_in);
- usb_free_coherent(pcu->udev, pcu->max_out_size,
+ usb_free_coherent(pcu->udev, pcu->max_in_size,
pcu->urb_in_buf, pcu->read_dma);
kfree(pcu->urb_out_buf);
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] Input: ims-pcu - fix usb_free_coherent() size in ims_pcu_buffers_alloc()
From: sashiko-bot @ 2026-05-22 10:55 UTC (permalink / raw)
To: Thomas Fourier; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260522085412.45430-2-fourier.thomas@gmail.com>
Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
New issues:
- [Low] Commit message inaccuracy regarding the fixed function.
Pre-existing issues:
- [High] Failure to release explicitly claimed secondary data interface on disconnect leaves the interface permanently claimed.
- [High] Memory leak of firmware object in `ims_pcu_process_async_firmware()`.
- [High] Missing interface verification in `ims_pcu_disconnect()` leads to Use-After-Free and Double-Free vulnerabilities.
- [High] Race condition in sysfs `reset_device` store callback due to missing mutex.
- [High] Type confusion vulnerability in CDC union descriptor parsing.
- [Medium] Missing endpoint type validation for the control interface.
- [Medium] Logic error in `ims_pcu_reset_packet()` breaks the parser state machine.
- [Medium] Out-of-bounds read and information leak due to missing response length checks.
- [Medium] Violation of DMA mapping rules by performing DMA on an embedded structure.
- [Low] Out-of-bounds read in `ims_pcu_irq()` debug logging.
--
commit b02b6acbca140f2c98e04a2bf7bf5d8b5b24ae41
Author: Thomas Fourier <fourier.thomas@gmail.com>
Input: ims-pcu - fix usb_free_coherent() size in ims_pcu_buffers_alloc()
This commit changes the size passed to usb_free_coherent() in
ims_pcu_buffers_free() from max_out_size to max_in_size, properly matching
the input buffer's allocation size.
> Input: ims-pcu - fix usb_free_coherent() size in ims_pcu_buffers_alloc()
>
> The input buffer size is pcu->max_in_size, but pcu->max_out_size is
> passed to usb_free_coherent().
>
> Change size to match the allocation size.
[Severity: Low]
Is the function name in the subject line accurate? This modifies
ims_pcu_buffers_free(), but the subject mentions ims_pcu_buffers_alloc().
The error handling path in ims_pcu_buffers_alloc() appears to already
correctly use pcu->max_in_size.
>
> Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>
> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 4c022a36dbe84..7a1cb9333f53c 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -1624,7 +1624,7 @@ static void ims_pcu_buffers_free(struct ims_pcu *pcu)
> usb_kill_urb(pcu->urb_in);
> usb_free_urb(pcu->urb_in);
>
> - usb_free_coherent(pcu->udev, pcu->max_out_size,
> + usb_free_coherent(pcu->udev, pcu->max_in_size,
> pcu->urb_in_buf, pcu->read_dma);
>
> kfree(pcu->urb_out_buf);
[Severity: High]
This is a pre-existing issue, but does ims_pcu_disconnect() leak the claimed
secondary data interface?
drivers/input/misc/ims-pcu.c:ims_pcu_disconnect() {
[ ... ]
if (alt->desc.bInterfaceClass != USB_CLASS_COMM)
return;
ims_pcu_stop_io(pcu);
[ ... ]
ims_pcu_buffers_free(pcu);
kfree(pcu);
}
During ims_pcu_probe(), the driver claims the secondary data interface using
usb_driver_claim_interface(), but ims_pcu_disconnect() never calls
usb_driver_release_interface() for pcu->data_intf when the primary control
interface is unbound. Does this leave the data interface permanently claimed,
causing subsequent binds to fail with -EBUSY?
[Severity: High]
This is a pre-existing issue, but can this logic in ims_pcu_disconnect() lead
to a use-after-free or double-free?
drivers/input/misc/ims-pcu.c:ims_pcu_disconnect() {
[ ... ]
/*
* See if we are dealing with control or data interface. The cleanup
* happens when we unbind primary (control) interface.
*/
if (alt->desc.bInterfaceClass != USB_CLASS_COMM)
return;
[ ... ]
kfree(pcu);
}
If a malicious USB device creates a secondary data interface with the
USB_CLASS_COMM class, and the user unbinds this malicious data interface via
sysfs, does ims_pcu_disconnect() bypass the return statement and prematurely
free the pcu structure while the primary interface is still active?
Should it verify if (intf != pcu->ctrl_intf) instead?
[Severity: High]
This is a pre-existing issue, but does this code cause a type confusion
regression during probe?
In ims_pcu_parse_cdc_data(), the driver looks up the control interface using
union_desc->bMasterInterface0 from the untrusted CDC union descriptor, and
assigns it to pcu->ctrl_intf without validating that pcu->ctrl_intf matches
the interface currently being probed.
drivers/input/misc/ims-pcu.c:ims_pcu_probe() {
[ ... ]
error = ims_pcu_parse_cdc_data(intf, pcu);
if (error)
goto err_free_mem;
[ ... ]
usb_set_intfdata(pcu->ctrl_intf, pcu);
[ ... ]
If a malicious device points bMasterInterface0 to an interface owned by a
completely different driver, does usb_set_intfdata() overwrite the other
driver's private data pointer, potentially leading to memory corruption?
[Severity: High]
This is a pre-existing issue, but does this error path leak the firmware
object?
If ihex_validate_fw(fw) returns an error in ims_pcu_process_async_firmware(),
the function jumps to the out label, which bypasses the release_firmware(fw)
call. Since fw is a valid pointer when ihex_validate_fw() is called, does this
directly leak the firmware object memory?
[Severity: High]
This is a pre-existing issue, but is there a race condition in the sysfs
reset_device store callback?
The ims_pcu_reset_device() sysfs callback directly calls
ims_pcu_execute_command() without acquiring pcu->cmd_mutex. This function
manipulates shared variables like pcu->expected_response, pcu->cmd_done, and
pcu->cmd_buf. If triggered concurrently with other commands (such as LED
brightness updates which properly lock the mutex), does this cause data races
and corruption of the shared command buffer?
[Severity: Medium]
This is a pre-existing issue, but does the driver validate the endpoint type
for the control interface?
In ims_pcu_parse_cdc_data(), the driver extracts the first endpoint:
pcu->ep_ctrl = &alt->endpoint[0].desc
Does it fail to verify that it is an IN interrupt endpoint? If a malicious
device provides a different endpoint type (e.g., bulk OUT), which is then
passed to usb_fill_int_urb() and usb_submit_urb(), does this trigger a
BOGUS urb xfer kernel warning?
[Severity: Medium]
This is a pre-existing issue, but is there a logic error in
ims_pcu_reset_packet() that breaks the parser state machine?
ims_pcu_reset_packet() is intended to reset the protocol state machine to wait
for the next packet, but it sets pcu->have_stx = true.
Does this cause the state machine to bypass the wait for the next STX byte
after the first packet is processed, and immediately accept garbage bytes? When
the actual STX byte later arrives, does the driver spam the kernel log with
Unexpected STX at byte X, discarding old data warnings?
[Severity: Medium]
This is a pre-existing issue, but does this code lead to out-of-bounds reads
and information leaks?
Functions like ims_pcu_report_events() and ims_pcu_get_info() process data
directly from the response buffers (e.g., pcu->read_buf[3] or pcu->cmd_buf)
without checking pcu->read_pos or pcu->cmd_buf_len to ensure the device
actually sent enough data.
If a device sends a shortened response, does the driver parse stale,
uninitialized data remaining in the buffer from previous commands?
[Severity: Medium]
This is a pre-existing issue, but does this code violate DMA mapping rules?
In ims_pcu_line_setup(), pcu->cmd_buf is passed to usb_control_msg() for a DMA
transfer:
drivers/input/misc/ims-pcu.c:ims_pcu_line_setup() {
[ ... ]
struct usb_cdc_line_coding *line = (void *)pcu->cmd_buf;
[ ... ]
error = usb_control_msg(pcu->udev, usb_sndctrlpipe(pcu->udev, 0),
USB_CDC_REQ_SET_LINE_CODING,
USB_TYPE_CLASS | USB_RECIP_INTERFACE,
0, interface->desc.bInterfaceNumber,
line, sizeof(struct usb_cdc_line_coding),
5000);
[ ... ]
Since pcu->cmd_buf is an array embedded directly inside the struct ims_pcu
allocation, and embedded fields are not guaranteed to be strictly cacheline
aligned, can cache maintenance operations on this buffer corrupt adjacent
fields in the struct ims_pcu on architectures without hardware cache coherency?
[Severity: Low]
This is a pre-existing issue, but is there an out-of-bounds read in the
ims_pcu_irq() debug logging?
In ims_pcu_irq(), the debug logging unconditionally prints data from
pcu->urb_in_buf up to urb->actual_length bytes. If the interrupt fired for
pcu->urb_ctrl, the actual transferred data resides in pcu->urb_ctrl_buf.
Does this result in printing unrelated stale data, and if urb->actual_length
from the control URB exceeds pcu->max_in_size, does it read out of bounds up
to the format limit?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260522085412.45430-2-fourier.thomas@gmail.com?part=1
^ permalink raw reply
* [PATCH 2/5] HID: multitouch: Fix stale MT slots when contact count drops to zero
From: Dave Carey @ 2026-05-22 11:15 UTC (permalink / raw)
To: linux-input; +Cc: jikos, bentiss, Dave Carey
In-Reply-To: <20260515175253.873796-1-carvsdriver@gmail.com>
The INGENIC 17EF:6161 touchscreen (Lenovo Yoga Book 9 14IAH10) reports
HID_DG_CONTACTCOUNT=0 in the frame immediately following the last finger
lift rather than omitting the frame entirely. In mt_touch_report() the
existing code only updates num_expected when contact_count is non-zero,
so a zero contact count on the first packet of a new frame leaves
num_expected at its previous value (e.g. 2 for a two-finger gesture).
The sync check "num_received >= num_expected" then evaluates "0 >= 2"
and never fires, preventing INPUT_MT_DROP_UNUSED from releasing the
stale slots. Those slots remain active in the kernel MT layer until the
next touch, at which point they are released in a batch alongside the
new contact — causing the userspace event consumer to miss the intervening
finger-up sequence and corrupt its gesture session state.
Fix by resetting num_expected to 0 when contact_count is zero and
num_received is still 0 (i.e., this is the first and only packet of the
frame, not a continuation packet in a multi-packet sequence). With
num_expected=0 the sync check "0 >= 0" fires immediately, calling
input_mt_sync_frame() which drops the stale slots via INPUT_MT_DROP_UNUSED.
The num_received==0 guard is critical: continuation packets in a
multi-packet frame arrive after at least one contact has already been
processed (num_received>0), so they are correctly excluded from this
path and the existing multi-packet logic is unaffected.
Signed-off-by: Dave Carey <carvsdriver@gmail.com>
Tested-by: Dave Carey <carvsdriver@gmail.com>
---
v2:
- Restructured contact_count block per Benjamin Tissoires' review:
replace three-branch if/else-if/else-if with a cleaner two-branch
form, dropping the outer if (contact_count >= 0) wrapper
- Add prev_scantime != scantime guard to the zero-contact sentinel case
drivers/hid/hid-multitouch.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index ec04dbafb..f1a2b3c4d5 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1321,21 +1321,18 @@ static void mt_touch_report(struct hid_device *hid,
* Includes multi-packet support where subsequent
* packets are sent with zero contactcount.
*/
- if (contact_count >= 0) {
+ if (contact_count > 0)
+ app->num_expected = contact_count;
+ else if (app->num_received == 0 && app->prev_scantime != scantime) {
/*
+ * New multi-report frame:
+ *
* For Win8 PTPs the first packet (td->num_received == 0) may
* have a contactcount of 0 if there only is a button event.
- * We double check that this is not a continuation packet
- * of a possible multi-packet frame be checking that the
- * timestamp has changed.
+ *
+ * Some other devices use a sentinel frame with 0 to release all contacts
*/
- if ((app->quirks & MT_QUIRK_WIN8_PTP_BUTTONS) &&
- app->num_received == 0 &&
- app->prev_scantime != scantime)
- app->num_expected = contact_count;
- /* A non 0 contact count always indicates a first packet */
- else if (contact_count)
- app->num_expected = contact_count;
+ app->num_expected = 0;
}
app->prev_scantime = scantime;
--
2.53.0
^ permalink raw reply related
* [syzbot] Monthly input report (May 2026)
From: syzbot @ 2026-05-22 12:32 UTC (permalink / raw)
To: linux-input, linux-kernel, syzkaller-bugs
Hello input maintainers/developers,
This is a 31-day syzbot report for the input subsystem.
All related reports/information can be found at:
https://syzkaller.appspot.com/upstream/s/input
During the period, 2 new issues were detected and 0 were fixed.
In total, 21 issues are still open and 63 have already been fixed.
There are also 2 low-priority issues.
Some of the still happening issues:
Ref Crashes Repro Title
<1> 184 Yes WARNING in cm109_input_open/usb_submit_urb (3)
https://syzkaller.appspot.com/bug?extid=ac0f9c4cc1e034160492
<2> 99 No KASAN: slab-use-after-free Read in report_descriptor_read
https://syzkaller.appspot.com/bug?extid=bc537ca7a0efe33988eb
<3> 22 Yes INFO: task hung in __input_unregister_device (5)
https://syzkaller.appspot.com/bug?extid=78e2288f58b881ed3c45
<4> 18 Yes INFO: task hung in console_callback (6)
https://syzkaller.appspot.com/bug?extid=6027421afa74a2ba440d
<5> 1983 No possible deadlock in evdev_pass_values (2)
https://syzkaller.appspot.com/bug?extid=13d3cb2a3dc61e6092f5
<6> 621 Yes KASAN: slab-out-of-bounds Read in mcp2221_raw_event (2)
https://syzkaller.appspot.com/bug?extid=1018672fe70298606e5f
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
To disable reminders for individual bugs, reply with the following command:
#syz set <Ref> no-reminders
To change bug's subsystems, reply with:
#syz set <Ref> subsystems: new-subsystem
You may send multiple commands in a single email message.
^ permalink raw reply
* Re: [PATCH 02/10] [v3] input: gpio-keys: make legacy gpiolib optional
From: Matti Vaittinen @ 2026-05-22 12:45 UTC (permalink / raw)
To: Arnd Bergmann, Arnd Bergmann, open list:GPIO SUBSYSTEM
Cc: linux-kernel, Christian Lamparter, Johannes Berg, Aaro Koskinen,
Andreas Kemnade, Kevin Hilman, Roger Quadros, Tony Lindgren,
Thomas Bogendoerfer, John Paul Adrian Glaubitz, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
Linus Walleij, Bartosz Golaszewski, Dmitry Torokhov, Lee Jones,
Pavel Machek, Florian Fainelli, Jonas Gorski, Andrew Lunn,
Vladimir Oltean, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, linux-wireless, Linux-OMAP, linux-arm-kernel,
linux-mips, linux-sh, linux-input, linux-leds, Netdev
In-Reply-To: <681f328c-bc8a-4410-b876-f8c5f116325d@app.fastmail.com>
On 22/05/2026 11:28, Arnd Bergmann wrote:
> On Fri, May 22, 2026, at 06:55, Matti Vaittinen wrote:
>> On 20/05/2026 21:38, Arnd Bergmann wrote:
>>> From: Arnd Bergmann <arnd@arndb.de>
>>>
>>> The two Rohm PMIC drivers use a gpio-keys device without an actual GPIO,
>>> passing an IRQ number instead. In order to keep this working both with
>>> and with CONFIG_GPIOLIB_LEGACY, change the gpio-keys driver to ignore
>>> the gpio number if an IRQ is passed.
>>>
>>
>> I am (still) all fine with this, even though I like Dmitry's set. I
>> suppose you already have a plan for merging this, but I still have to
>> ask - why the MFD changes aren't in own patch? I feel it would have
>> simplified merging, backporting, reviewing and reverting if needed.
>
> Splitting it out would break bisection: The gpio-leds change without
> the mfd change causes a build failure when assigning the .gpio
> field, and the reverse causes a runtime failure when .gpio=0 is
> a valid line.
Ah, yes. Thanks for explanation. (I feel a bit [a lot] stupid, as I
somehow missed the problem of .gpio = 0 being valid. That even though I
had it explicitly set to -1 myself :rolleyes: ).
No further questions ;)
Yours,
-- Matti
--
---
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply
* Re: [PATCH 00/11] HID: storing pointers in 'hid_device_id::driver_data'
From: Pawel Zalewski @ 2026-05-22 14:18 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Ping Cheng, Jason Gerecke, linux-kernel, linux-input,
Christian A. Ehrhardt, Christian A. Ehrhardt
In-Reply-To: <ag8bARsGNVqA8xbQ@beelink>
> I would agree with sashiko that the series introduces an anti-pattern by
> allowing .driver_data to be an arbitrary pointer. The reason is that we
> can dynamically set driver_data through the kernel command line, and
> when it's used as a pointer, bad things will happen.
I have not considered the command line override ! This indeed would not
be a good case for having a pointer at all in there - in the current
form of the codebase.
The series does not introduce this anti-pattern - this is already the
current status quo in the codebase and the problem predates the patch
series itself. The series makes the problem more visible and validates
the existing status quo - agreed on this.
> I would think we should fix the 2 offenders to enforce not using a
> direct pointer but an offset in a table of pointers.
>
> How does that sound for you?
That could work, probably via a named enum (so that the idx is bounded
and validated to avoid out of bounds memory access) with its items mapped
into a table of const pointers. That way the command line override still
works as before and my goals set out in the series are met as well.
Perhaps an alternative solution would be to sanitize the user-input instead in
the 'hid-core::new_id_store' function, such that only 'driver_data' values
that match an existing entry in the driver's 'id_table' are accepted, this
is how it is done currently in the PCI subsystem in 'pci-driver::new_id_store',
so it would be consistent as well.
The bonus here is that all other drivers in the HID subsystem benefit from
this aproach as opposed to just the two current offenders, as providing
'driver_data' from the command line is now mandatory and must match the
existing table entry - which is what we want ?
So something among the lines of:
@hid-core::new_id_store
```
struct hid_driver *hdrv = to_hid_driver(drv);
const struct hid_device_id *ids = hdrv->id_table;
(...)
/* Only accept driver_data values that match an existing id_table
entry */
if (ids) {
ret = -EINVAL;
while (ids->vendor || ids->product) {
if (driver_data == ids->driver_data) {
ret = 0;
break;
}
ids++;
}
if (ret) /* No match */
return ret;
}
```
Would that approach work for you ? Do help in what is the correct
termination for the while loop - for this demo I went with 'ids->vendor'
and 'ids->product' fields as common-sense would indicate that quirks need
to be device-specific by definition but that was without any actual research.
The downside is that 'new_id' drivers that actually use a pointer for the
'driver_data' will be rejected - which would be actually covered by the
solution that you have proposed in the first place. To think of it the
input sanitizer could work additionaly in tandem with the enum-based
solution if this fact is an issue, so it might not be an alternative
actually but a complement for it.
In the meantime I think that patches 1-8 can be reviewed regardless
without any changes (as they predate patch 9 that adds the union and the
commit messages does not mention anything about it).
Best regards,
Pawel
^ permalink raw reply
* [PATCH] Input: synaptics - add quirk for Daffodil DC253D touchpad
From: Ashwin Gundarapu @ 2026-05-22 15:22 UTC (permalink / raw)
To: linux-input; +Cc: linux-kernel, dmitrytorokhov, ubuntu-kernel, hasan6l
From ee16efae3c4371d37467c49c0cae6af808e1b625 Mon Sep 17 00:00:00 2001
From: Ashwin Gundarapu <linuxuser509@zohomail.in>
Date: Fri, 22 May 2026 20:48:25 +0530
Subject: [PATCH] Input: synaptics - add quirk for Daffodil DC253D touchpad
On the Daffodil DC253D laptop, the touchpad is incorrectly detected as
a clickpad, causing the right button to be missing. Add a DMI quirk
to clear the CLICKPAD bit for this device.
Link: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2153983
Signed-off-by: Ashwin Gundarapu <linuxuser509@zohomail.in>
---
drivers/input/mouse/synaptics.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 26071128f43a..4ca85fb4ab70 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -534,9 +534,16 @@ static int synaptics_invert_y(int y)
* Apply quirk(s) if the hardware matches
*/
static void synaptics_apply_quirks(struct psmouse *psmouse,
- struct synaptics_device_info *info)
+ struct synaptics_device_info *info)
{
- int i;
+ int i;
+
+ /* Daffodil DC253D has a physical right button */
+ if (dmi_match(DMI_SYS_VENDOR, "Daffodil Computers Ltd.") &&
+ dmi_match(DMI_PRODUCT_NAME, "DC253D")) {
+ info->ext_cap_0c &= ~BIT(20); /* Clear CLICKPAD bit */
+ psmouse_info(psmouse, "Force right button on Daffodil DC253D\n");
+ }
for (i = 0; min_max_pnpid_table[i].pnp_ids; i++) {
if (!psmouse_matches_pnp_id(psmouse,
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] Input: synaptics - add quirk for Daffodil DC253D touchpad
From: sashiko-bot @ 2026-05-22 15:43 UTC (permalink / raw)
To: Ashwin Gundarapu; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <19e5047ead1.ce62bdb39017.6022546617731203250@zohomail.in>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The newly added DMI quirk is bypassed if the touchpad initializes in SMBus (InterTouch) mode.
- [Medium] Using exact string matching with dmi_match() instead of a dmi_system_id table.
- [Low] Spaces are used instead of tabs for indentation.
--
commit 49d6473335088b14bbb30544a1f0fe866d10ddb5
Author: Ashwin Gundarapu <linuxuser509@zohomail.in>
Input: synaptics - add quirk for Daffodil DC253D touchpad
This commit adds a DMI quirk to clear the CLICKPAD capability bit for the
Daffodil DC253D touchpad. It aims to fix an issue where the device is
incorrectly detected as a clickpad, causing the physical right button to be
missing.
Link: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2153983
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 26071128f43a7..4ca85fb4ab70d 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -534,9 +534,16 @@ static int synaptics_invert_y(int y)
> * Apply quirk(s) if the hardware matches
> */
> static void synaptics_apply_quirks(struct psmouse *psmouse,
[Severity: High]
Does this logic get bypassed if the touchpad initializes in SMBus
(InterTouch) mode?
It looks like synaptics_apply_quirks() is only called from the PS/2 fallback
initialization path (synaptics_init_ps2()). During device initialization in
synaptics_init(), if InterTouch (SMBus) is supported, the setup is routed to
synaptics_setup_intertouch().
This SMBus path directly evaluates SYN_CAP_CLICKPAD(info->ext_cap_0c) to
configure the .buttonpad property for the RMI4 driver without ever running
synaptics_apply_quirks(). This would cause the intended fix to fail on modern
systems using InterTouch.
> - struct synaptics_device_info *info)
> + struct synaptics_device_info *info)
> {
> - int i;
> + int i;
[Severity: Low]
This isn't a bug, but it looks like spaces were used for indentation here and
in the modified function arguments instead of tabs. The Linux kernel coding
style mandates the use of 8-character tabs for indentation.
> +
> + /* Daffodil DC253D has a physical right button */
> + if (dmi_match(DMI_SYS_VENDOR, "Daffodil Computers Ltd.") &&
> + dmi_match(DMI_PRODUCT_NAME, "DC253D")) {
[Severity: Medium]
Will this exact string matching fail if the DMI strings contain trailing
whitespace or padding?
The dmi_match() function uses exact string matching. DMI strings from OEM BIOS
vendors frequently include trailing whitespace or padding, which would cause
this match to fail and the quirk to be silently ignored.
The standard and more robust approach used throughout the kernel and within
synaptics.c is to use dmi_check_system() with a struct dmi_system_id table.
This utilizes DMI_MATCH() to perform substring matching, safely ignoring any
trailing spaces.
> + info->ext_cap_0c &= ~BIT(20); /* Clear CLICKPAD bit */
> + psmouse_info(psmouse, "Force right button on Daffodil DC253D\n");
> + }
--
Sashiko AI review · https://sashiko.dev/#/patchset/19e5047ead1.ce62bdb39017.6022546617731203250@zohomail.in?part=1
^ permalink raw reply
* Re: [PATCH v4 0/6] iio: hid-sensor: standardize scan_type initialization
From: Jonathan Cameron @ 2026-05-22 16:51 UTC (permalink / raw)
To: Natália Salvino André
Cc: andy, dlechner, jikos, nuno.sa, srinivas.pandruvada, linux-iio,
linux-input, Zhang, Lixu
In-Reply-To: <20260519234752.23911-1-natalia.andre@ime.usp.br>
On Tue, 19 May 2026 20:40:42 -0300
Natália Salvino André <natalia.andre@ime.usp.br> wrote:
> This series refactors the HID sensor drivers to standardize the
> initialization of the iio_chan_spec scan_type structure using compound
> literals.
>
> This change improves code readability and ensures that all fields of
> the scan_type structure are properly zero-initialized, allowing the
> removal of local helper functions.
>
> Additionally, the channel initialization loops for hid-sensor-accel-3d
> and hid-sensor-gyro-3d were cleaned up to iterate directly over the
> scan indices, eliminating redundant index-offset logic.
+CC Lixu
Please make sure to add to your +CC anyone who has been involve in earlier
versions of the series.
I'm going to queue this now to get some build coverage. Still fine
to add tags though or indeed drop it if any problems.
Applied to the testing branch of iio.git
thanks,
Jonathan
>
> ---
>
> Changes in v4:
>
> - Fixed a bug in the sensor_hub_input_get_attribute_info()
> call where passing the raw loop index 'ch' broke the HID report
> field lookup in hid-sensor-accel-3d. Restored the proper HID
> usage ID offset calculation using HID_USAGE_SENSOR_ACCEL_X_AXIS + ch
> - Cleaned up the channel initialization loop to iterate
> directly over the scan indices in hid-sensor-gyro-3d for consistency
>
> Changes in v3:
>
> - Dropped the global helper function implementation from
> hid-sensor-attributes.c following maintainer feedback.
> - Shifted to local refactoring inside the drivers using compound
> literals and the macros BYTES_TO_BITS() and BITS_PER_TYPE()
> - Refactored the channel initialization loop in hid-sensor-accel-3d to
> iterate directly over the scan enums instead of using 0-based
> index offsets
> - v3 link: https://lore.kernel.org/linux-iio/89a2e6775e3c922a4848a4b4730aab8d32097454.camel@linux.intel.com/T/#t
>
> Changes in v2:
>
> - Modified the helper function to use the BITS_PER_BYTE macro instead of
> magic numbers for bit calculations
> - Updated the scan_type structure field assignment from '.sign'
> to '.format' based on recent subsystem renaming feedback
>
> Natália Salvino André (6):
> iio: accel: HID: hid-sensor-accel-3d: Refactor channel initialization
> iio: gyro: HID: hid-sensor-gyro-3d: Refactor channel initialization
> iio: light: HID: hid-sensor-als: Refactor channel initialization
> iio: light: HID: hid-sensor-prox: Refactor channel initialization
> iio: magnetometer: HID: hid-sensor-magn-3d: Refactor channel
> initialization
> iio: pressure: HID: hid-sensor-press: Refactor channel initialization
>
> drivers/iio/accel/hid-sensor-accel-3d.c | 27 +++++++------------
> drivers/iio/gyro/hid-sensor-gyro-3d.c | 27 +++++++------------
> drivers/iio/light/hid-sensor-als.c | 18 +++++--------
> drivers/iio/light/hid-sensor-prox.c | 19 +++++--------
> drivers/iio/magnetometer/hid-sensor-magn-3d.c | 20 +++++---------
> drivers/iio/pressure/hid-sensor-press.c | 19 +++++--------
> 6 files changed, 42 insertions(+), 88 deletions(-)
>
^ 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