* Re: [PATCH v3 2/6] iio: gyro: HID: hid-sensor-gyro-3d: Refactor channel initialization
From: srinivas pandruvada @ 2026-05-15 16:48 UTC (permalink / raw)
To: Natália Salvino André, andy, dlechner, jic23, jikos,
nuno.sa
Cc: Pietro Di Consolo Gregorio, linux-iio, linux-input
In-Reply-To: <20260512012302.20883-3-natalia.andre@ime.usp.br>
On Mon, 2026-05-11 at 22:16 -0300, Natália Salvino André wrote:
> Replace the local gyro_3d_adjust_channel_bit_mask() function with a
> compound literal for scan_type initialization to improve code
> readability.
>
> Signed-off-by: Natália Salvino André <natalia.andre@ime.usp.br>
> Co-developed-by: Pietro Di Consolo Gregorio <pietro.gregorio@usp.br>
> Signed-off-by: Pietro Di Consolo Gregorio <pietro.gregorio@usp.br>
> ---
> drivers/iio/gyro/hid-sensor-gyro-3d.c | 24 +++++++++---------------
> 1 file changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/iio/gyro/hid-sensor-gyro-3d.c
> b/drivers/iio/gyro/hid-sensor-gyro-3d.c
> index c340cc899a7c..4cbe4d93a0a1 100644
> --- a/drivers/iio/gyro/hid-sensor-gyro-3d.c
> +++ b/drivers/iio/gyro/hid-sensor-gyro-3d.c
> @@ -3,6 +3,7 @@
> * HID Sensors Driver
> * Copyright (c) 2012, Intel Corporation.
> */
> +#include <linux/bitops.h>
> #include <linux/device.h>
> #include <linux/platform_device.h>
> #include <linux/module.h>
> @@ -82,17 +83,6 @@ static const struct iio_chan_spec
> gyro_3d_channels[] = {
> IIO_CHAN_SOFT_TIMESTAMP(CHANNEL_SCAN_INDEX_TIMESTAMP)
> };
>
> -/* Adjust channel real bits based on report descriptor */
> -static void gyro_3d_adjust_channel_bit_mask(struct iio_chan_spec
> *channels,
> - int channel, int
> size)
> -{
> - channels[channel].scan_type.sign = 's';
> - /* Real storage bits will change based on the report desc.
> */
> - channels[channel].scan_type.realbits = size * 8;
> - /* Maximum size of a sample to capture is u32 */
> - channels[channel].scan_type.storagebits = sizeof(u32) * 8;
> -}
> -
> /* Channel read_raw handler */
> static int gyro_3d_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> @@ -251,16 +241,20 @@ static int gyro_3d_parse_report(struct
> platform_device *pdev,
> int i;
>
> for (i = 0; i <= CHANNEL_SCAN_INDEX_Z; ++i) {
> + unsigned int ch = CHANNEL_SCAN_INDEX_X + i;
> +
When you do code change more than what you described, then you should
add to commit description that you are also doing this at the same
time.
Thanks,
Srinivas
> ret = sensor_hub_input_get_attribute_info(hsdev,
> HID_INPUT_REPORT,
> usage_id,
> HID_USAGE_SENSOR_ANGL_VELOCITY_X_AXI
> S + i,
> - &st->gyro[CHANNEL_SCAN_INDEX_X +
> i]);
> + &st->gyro[ch]);
> if (ret < 0)
> break;
> - gyro_3d_adjust_channel_bit_mask(channels,
> - CHANNEL_SCAN_INDEX_X + i,
> - st->gyro[CHANNEL_SCAN_INDEX_X +
> i].size);
> + channels[ch].scan_type = (struct iio_scan_type) {
> + .format = 's',
> + .realbits = BYTES_TO_BITS(st-
> >gyro[ch].size),
> + .storagebits = BITS_PER_TYPE(u32),
> + };
> }
> dev_dbg(&pdev->dev, "gyro_3d %x:%x, %x:%x, %x:%x\n",
> st->gyro[0].index,
^ permalink raw reply
* [PATCH v2] HID: letsketch: fix UAF on inrange_timer at driver unbind
From: Manish Khadka @ 2026-05-15 16:42 UTC (permalink / raw)
To: linux-input; +Cc: Hans de Goede, Jiri Kosina, Benjamin Tissoires, linux-kernel
In-Reply-To: <20260515155838.B4247C2BCB3@smtp.kernel.org>
letsketch_driver does not provide a .remove callback, but
letsketch_probe() arms a per-device timer:
timer_setup(&data->inrange_timer, letsketch_inrange_timeout, 0);
The timer is re-armed from letsketch_raw_event() with a 100 ms
timeout on every pen-in-range report, and its callback dereferences
data->input_tablet to deliver a synthetic BTN_TOOL_PEN release.
letsketch_data is allocated with devm_kzalloc(), and its input_dev
fields are devm-allocated via letsketch_setup_input_tablet(). On
device unbind (USB unplug or rmmod), the HID core runs its default
teardown and devm cleanup frees both letsketch_data and the input
devices. Because no .remove callback exists, nothing drains the
timer first: if raw_event armed it within ~100 ms of the unbind,
the pending timer fires on freed memory. This is a UAF read of
data and of data->input_tablet, followed by input_report_key() /
input_sync() into the freed input_dev.
The same problem can occur on the probe error path: if
hid_hw_start() enabled I/O on an always-poll-quirk device and then
failed, raw_event may have armed the timer before devm releases
data.
Fix by adding a .remove callback that calls hid_hw_stop() first.
hid_hw_stop() synchronously kills the URBs that deliver raw_event(),
so once it returns no path can re-arm the timer. timer_shutdown_sync()
then drains any in-flight callback and permanently disables further
mod_timer() calls. Apply the same timer_shutdown_sync() in the probe
error path so the timer is guaranteed not to outlive data.
Fixes: 33a5c2793451 ("HID: Add new Letsketch tablet driver")
Cc: stable@vger.kernel.org
Signed-off-by: Manish Khadka <maskmemanish@gmail.com>
---
v1 -> v2:
- Drop the spinlock_t and bool removing fields and their uses
(and the <linux/cleanup.h> include): hid_hw_stop() synchronously
kills URBs, so once it returns no raw_event can re-arm the timer
and an explicit gate is unnecessary.
- Use timer_shutdown_sync() in place of timer_delete_sync(), which
also makes the probe error path safe against a stray mod_timer()
that an always-poll-quirk hid_hw_start() failure could leave
behind.
- Drain the timer in the probe error path via a unified
err_shutdown_timer label.
Thanks to Sashiko AI review for both points.
drivers/hid/hid-letsketch.c | 36 +++++++++++++++++++++++++++++++++---
1 file changed, 33 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-letsketch.c b/drivers/hid/hid-letsketch.c
index 11e21f988723..b52e93a91ae5 100644
--- a/drivers/hid/hid-letsketch.c
+++ b/drivers/hid/hid-letsketch.c
@@ -296,13 +296,42 @@ static int letsketch_probe(struct hid_device *hdev, const struct hid_device_id *
ret = letsketch_setup_input_tablet(data);
if (ret)
- return ret;
+ goto err_shutdown_timer;
ret = letsketch_setup_input_tablet_pad(data);
if (ret)
- return ret;
+ goto err_shutdown_timer;
+
+ ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+ if (ret)
+ goto err_shutdown_timer;
- return hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+ return 0;
+
+err_shutdown_timer:
+ /*
+ * Drain any pending callback and permanently disable the timer
+ * before devm releases data: if hid_hw_start() enabled I/O on an
+ * always-poll-quirk device and then failed, raw_event may have
+ * armed the timer already.
+ */
+ timer_shutdown_sync(&data->inrange_timer);
+ return ret;
+}
+
+static void letsketch_remove(struct hid_device *hdev)
+{
+ struct letsketch_data *data = hid_get_drvdata(hdev);
+
+ /*
+ * hid_hw_stop() synchronously kills the URBs that deliver
+ * raw_event(), so once it returns no path can re-arm
+ * inrange_timer. timer_shutdown_sync() then drains any
+ * in-flight callback and permanently disables further
+ * mod_timer() calls before devm releases data.
+ */
+ hid_hw_stop(hdev);
+ timer_shutdown_sync(&data->inrange_timer);
}
static const struct hid_device_id letsketch_devices[] = {
@@ -315,6 +344,7 @@ static struct hid_driver letsketch_driver = {
.name = "letsketch",
.id_table = letsketch_devices,
.probe = letsketch_probe,
+ .remove = letsketch_remove,
.raw_event = letsketch_raw_event,
};
module_hid_driver(letsketch_driver);
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] HID: hid-lenovo-go: cancel cfg_setup work in hid_go_cfg_remove()
From: sashiko-bot @ 2026-05-15 16:18 UTC (permalink / raw)
To: Manish Khadka; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260515153607.76175-1-maskmemanish@gmail.com>
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] AB-BA deadlock between device removal and sysfs operations.
- [High] Global state corruption due to static per-device structs.
- [High] Use-After-Free window due to mixing devm registration with explicit teardown.
- [High] Resource leaks on probe error paths.
- [Medium] Unconditional success return in `mcu_property_out()` masks timeout errors.
- [Medium] Probe arbitrarily fails due to incorrect `schedule_delayed_work()` boolean logic.
- [Low] Commit message mechanism explanation is factually incorrect.
--
commit 8001481b81b1932a98238ba0e2de3079e8807d91
Author: Manish Khadka <maskmemanish@gmail.com>
HID: hid-lenovo-go: cancel cfg_setup work in hid_go_cfg_remove()
This commit drains the delayed go_cfg_setup workqueue at the start of
hid_go_cfg_remove() to prevent it from dereferencing a stale device pointer
after teardown. It places the cancellation before the mutex acquisition to
avoid deadlocks.
Cc: stable@vger.kernel.org
> If the device is unbound within the 2 ms scheduling delay (a probe
> failure rolling back via remove, or a fast rmmod after probe), the
> work fires after hid_set_drvdata(NULL) has cleared the back pointer,
> leaving cfg_setup() with a NULL or stale drvdata.hdev.
This isn't a bug, but the explanation of the mechanism seems slightly
inaccurate.
Does hid_set_drvdata(hdev, NULL) only clear hdev->dev.driver_data rather
than the global drvdata.hdev variable?
> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index d4d26c7833563..ef69869f0a009 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -2408,6 +2408,13 @@ static int hid_go_cfg_probe(struct hid_device *hdev,
>
> static void hid_go_cfg_remove(struct hid_device *hdev)
> {
> + /*
> + * cfg_setup is scheduled from hid_go_cfg_probe() with a 2 ms delay;
> + * drain it here before tearing down so the workqueue cannot run
> + * after hid_set_drvdata(NULL) and dereference a stale drvdata.hdev.
> + */
> + cancel_delayed_work_sync(&drvdata.go_cfg_setup);
The driver uses static global structures for device state (drvdata) and
LED attributes.
If multiple Legion Go controllers are connected, or a device is rapidly
re-bound, does re-initializing an active mutex or delayed workqueue, and
re-registering an already-linked LED struct corrupt internal kernel linked
lists and lock wait queues?
> +
> guard(mutex)(&drvdata.cfg_mutex);
> sysfs_remove_groups(&hdev->dev.kobj, top_level_attr_groups);
In hid_go_cfg_remove(), guard(mutex)(&drvdata.cfg_mutex) acquires the
mutex before calling sysfs_remove_groups(). sysfs_remove_groups() drains
kernfs active references, blocking until any currently executing sysfs
operations finish.
If the driver's sysfs callbacks acquire drvdata.cfg_mutex via
mcu_property_out(), can this cause an AB-BA deadlock if a sysfs operation
runs concurrently with device removal?
> hid_hw_close(hdev);
> hid_hw_stop(hdev);
The RGB LED class device and RGB sysfs attributes appear to be registered
using devm_* APIs, which clean up resources after the driver's .remove()
callback finishes.
If hid_go_cfg_remove() explicitly stops and closes the hardware here,
could userspace accessing the LED brightness or RGB sysfs files in the
window between hardware teardown and devres cleanup cause a use-after-free
when the sysfs callbacks call hid_hw_output_report()?
> hid_set_drvdata(hdev, NULL);
> }
Looking at hid_go_cfg_probe(), if a step after sysfs_create_groups() fails,
the function returns an error without calling sysfs_remove_groups().
Does this leave dangling sysfs files that could be accessed later?
Also, if hid_go_cfg_probe() returns an error, does hid_go_probe() leak the
hardware open references by failing to call hid_hw_stop() and
hid_hw_close()?
In hid_go_cfg_probe(), the driver checks ret = schedule_delayed_work(...)
and fails the probe if !ret.
Since schedule_delayed_work() returns false (0) when the work is already
pending, and the driver uses a single global workqueue structure, will a
second device fail to probe if it connects before the first device's work
fires?
In mcu_property_out(), wait_for_completion_interruptible_timeout() is used
to wait for hardware responses. If a timeout occurs, ret is set to -EBUSY.
Because the function ends with an unconditional return 0, does this drop
the error code and mask communication failures?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260515153607.76175-1-maskmemanish@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH v2 0/2] HID: lenovo-go: clean up USB assumption
From: Derek J. Clark @ 2026-05-15 16:11 UTC (permalink / raw)
To: Louis Clinckx, Mark Pearson, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel
In-Reply-To: <cover.1778701376.git.clinckx.louis@gmail.com>
On May 15, 2026 7:57:38 AM PDT, Louis Clinckx <clinckx.louis@gmail.com> wrote:
>These drivers' id_tables only match HID_USB_DEVICE() entries and the
>code freely assumes a USB transport (to_usb_interface() on hdev->dev.parent,
>endpoint access in raw_event). Make the assumption explicit at probe and
>drop a NULL check that the compiler-level definition of to_usb_interface()
>makes unreachable.
>
>No functional change for the supported (USB) devices.
>
>Louis Clinckx (2):
> HID: lenovo-go: reject non-USB transports in probe
> HID: lenovo-go: drop dead NULL check on to_usb_interface()
>
> drivers/hid/hid-lenovo-go-s.c | 11 ++++++-----
> drivers/hid/hid-lenovo-go.c | 6 +++---
> 2 files changed, 9 insertions(+), 8 deletions(-)
>
>---
>Changes since v1:
> - Resubmitted on linux-input as a quality fix per Benjamin's review of v1
> (sent to security@kernel.org; the HID_USB_DEVICE() id_table already
> filters BUS_USB, so no exploitable path).
> - Added patch 2 to remove the dead NULL check, at Derek's suggestion.
>
>base-commit: 50897c955902c93ae71c38698abb910525ebdc89
>--
>2.39.5
>
Thanks for the quick turnaround on this. I've already tested these changes, so for the series:
Reviewed-by: Derek J. Clark <derekjoh.clark@gmail.com>
Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>
Thanks,
Derek
^ permalink raw reply
* Re: [PATCH] HID: appleir: fix UAF on pending key_up_timer in remove()
From: sashiko-bot @ 2026-05-15 16:02 UTC (permalink / raw)
To: Manish Khadka; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260515153450.76146-1-maskmemanish@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Unprotected access to `appleir->input_dev` in the `flatbattery` event path leads to a Use-After-Free during device teardown.
- [Medium] The teardown path uses `timer_delete_sync()` instead of the mandated `timer_shutdown_sync()` API.
--
commit 85e03ec2fd55b34b6351049c19c0b16cf580e59e
Author: Manish Khadka <maskmemanish@gmail.com>
Subject: HID: appleir: fix UAF on pending key_up_timer in remove()
This commit introduces a removing flag to prevent a use-after-free during
device teardown. It checks this flag in the event handler to avoid arming
the timer or accessing the input device after it has been freed by
hid_hw_stop().
> diff --git a/drivers/hid/hid-appleir.c b/drivers/hid/hid-appleir.c
> index 5e8ced7bc05a3..0e51f1e166531 100644
> --- a/drivers/hid/hid-appleir.c
> +++ b/drivers/hid/hid-appleir.c
[ ... ]
> @@ -229,12 +234,17 @@ static int appleir_raw_event(struct hid_device *hid, struct hid_report *report,
> appleir->prev_key_idx = 0;
>
> if (!memcmp(data, keyrepeat, sizeof(keyrepeat))) {
> - key_down(hid, appleir, appleir->current_key);
> - /*
> - * Remote doesn't do key up, either pull them up, in the test
> - * above, or here set a timer which pulls them up after 1/8 s
> - */
> - mod_timer(&appleir->key_up_timer, jiffies + HZ / 8);
> + spin_lock_irqsave(&appleir->lock, flags);
> + if (!appleir->removing) {
> + key_down(hid, appleir, appleir->current_key);
> + /*
> + * Remote doesn't do key up, either pull them up, in
> + * the test above, or here set a timer which pulls them
> + * up after 1/8 s
> + */
> + mod_timer(&appleir->key_up_timer, jiffies + HZ / 8);
> + }
> + spin_unlock_irqrestore(&appleir->lock, flags);
> goto out;
> }
Does this leave the flatbattery branch unprotected?
If a flat battery packet is received during the teardown window, execution
skips these guarded blocks and falls straight through to battery_flat()
which follows immediately after this hunk.
Looking at battery_flat(), it seems to unconditionally execute
dev_err(&appleir->input_dev->dev, ...), which dereferences the input_dev
pointer. Can this result in a use-after-free if it fires concurrently with
teardown?
[ ... ]
> @@ -318,8 +328,20 @@ static int appleir_probe(struct hid_device *hid, const struct hid_device_id *id)
> static void appleir_remove(struct hid_device *hid)
> {
> struct appleir *appleir = hid_get_drvdata(hid);
> - hid_hw_stop(hid);
> + unsigned long flags;
> +
> + /*
> + * Mark the driver as tearing down so that any concurrent raw_event
> + * (e.g. from a USB URB completion that hid_hw_stop() has not yet
> + * killed) and the key_up_timer softirq stop touching input_dev
> + * before hid_hw_stop() frees it via hidinput_disconnect().
> + */
> + spin_lock_irqsave(&appleir->lock, flags);
> + appleir->removing = true;
> + spin_unlock_irqrestore(&appleir->lock, flags);
> +
> timer_delete_sync(&appleir->key_up_timer);
Should this teardown path use timer_shutdown_sync() instead?
The timer subsystem guidelines mandate using timer_shutdown_sync() in
teardown paths to permanently prevent the timer from being re-armed.
> + hid_hw_stop(hid);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260515153450.76146-1-maskmemanish@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH] HID: letsketch: fix UAF on inrange_timer at driver unbind
From: sashiko-bot @ 2026-05-15 15:58 UTC (permalink / raw)
To: Manish Khadka; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260515153551.76162-1-maskmemanish@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing timer cleanup on the probe error path leads to a Use-After-Free.
- [Medium] Unnecessary spinlock and state flag introduced instead of using `timer_shutdown_sync()`.
--
commit 2d88d7acbcf7e811559cfcb0df7e33f55178711b
Author: Manish Khadka <maskmemanish@gmail.com>
HID: letsketch: fix UAF on inrange_timer at driver unbind
This commit adds a remove callback to the letsketch driver to clean up
the inrange_timer during device unbind. It introduces a spinlock and a
removing flag to prevent the timer from being re-armed by in-flight
events while teardown is occurring.
> diff --git a/drivers/hid/hid-letsketch.c b/drivers/hid/hid-letsketch.c
> index 11e21f9887232..0dc9496d05f80 100644
> --- a/drivers/hid/hid-letsketch.c
> +++ b/drivers/hid/hid-letsketch.c
[ ... ]
> @@ -305,6 +315,23 @@ static int letsketch_probe(struct hid_device *hdev, const struct hid_device_id *
> return hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> }
While not introduced by this patch, if hid_hw_start() fails and returns an
error, does this leave a pending timer that will fire on freed memory?
Looking at hid_hw_start(), it enables hardware I/O before attempting to
connect. If the connection fails, it stops I/O and returns an error:
hid_hw_start() {
hdev->ll_driver->start(hdev);
if (hid_connect(hdev, connect_mask)) {
hdev->ll_driver->stop(hdev);
return error;
}
}
During the window where I/O is active, an incoming event could trigger
letsketch_raw_event() and arm the timer.
When letsketch_probe() directly returns this error, devres immediately frees
the letsketch_data structure without draining the timer.
> +static void letsketch_remove(struct hid_device *hdev)
> +{
> + struct letsketch_data *data = hid_get_drvdata(hdev);
> +
> + /*
> + * Block raw_event from arming inrange_timer during teardown so
> + * timer_delete_sync() below cannot race with a fresh mod_timer()
> + * issued from a URB completion handler still in flight while
> + * hid_hw_stop() is running.
> + */
> + scoped_guard(spinlock_irqsave, &data->lock)
> + data->removing = true;
> +
> + timer_delete_sync(&data->inrange_timer);
> + hid_hw_stop(hdev);
> +}
Could this synchronization be achieved without adding a new spinlock and state
flag by using timer_shutdown_sync() instead?
The timer_shutdown_sync() API permanently disables the timer, ensuring that
any subsequent mod_timer() calls are silent no-ops.
Alternatively, if hid_hw_stop() were called before cleaning up the timer, it
would wait for all URBs to complete, which structurally guarantees no new
events can arrive to re-arm the timer.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260515153551.76162-1-maskmemanish@gmail.com?part=1
^ permalink raw reply
* [PATCH] HID: hid-lenovo-go: cancel cfg_setup work in hid_go_cfg_remove()
From: Manish Khadka @ 2026-05-15 15:36 UTC (permalink / raw)
To: linux-input
Cc: Derek J . Clark, Mark Pearson, Jiri Kosina, Benjamin Tissoires,
linux-kernel
hid_go_cfg_probe() initialises drvdata.go_cfg_setup and schedules it
to run 2 ms later:
INIT_DELAYED_WORK(&drvdata.go_cfg_setup, &cfg_setup);
schedule_delayed_work(&drvdata.go_cfg_setup, msecs_to_jiffies(2));
cfg_setup() dereferences drvdata.hdev to issue MCU command requests.
hid_go_cfg_remove() tears down sysfs and stops the HID device, ending
with hid_set_drvdata(hdev, NULL), but never drains the delayed work.
If the device is unbound within the 2 ms scheduling delay (a probe
failure rolling back via remove, or a fast rmmod after probe), the
work fires after hid_set_drvdata(NULL) has cleared the back pointer,
leaving cfg_setup() with a NULL or stale drvdata.hdev.
Mirror the sibling driver hid-lenovo-go-s.c, whose hid_gos_cfg_remove()
already calls cancel_delayed_work_sync() on its analogous work, and
drain go_cfg_setup at the top of hid_go_cfg_remove(). The cancel
must come before guard(mutex)(&drvdata.cfg_mutex) because cfg_setup()
acquires that mutex; reversing the order would deadlock.
Fixes: d69ccfcbc955 ("HID: hid-lenovo-go: Add Lenovo Legion Go Series HID Driver")
Cc: stable@vger.kernel.org
Signed-off-by: Manish Khadka <maskmemanish@gmail.com>
---
drivers/hid/hid-lenovo-go.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
index d4d26c783356..ef69869f0a00 100644
--- a/drivers/hid/hid-lenovo-go.c
+++ b/drivers/hid/hid-lenovo-go.c
@@ -2408,6 +2408,13 @@ static int hid_go_cfg_probe(struct hid_device *hdev,
static void hid_go_cfg_remove(struct hid_device *hdev)
{
+ /*
+ * cfg_setup is scheduled from hid_go_cfg_probe() with a 2 ms delay;
+ * drain it here before tearing down so the workqueue cannot run
+ * after hid_set_drvdata(NULL) and dereference a stale drvdata.hdev.
+ */
+ cancel_delayed_work_sync(&drvdata.go_cfg_setup);
+
guard(mutex)(&drvdata.cfg_mutex);
sysfs_remove_groups(&hdev->dev.kobj, top_level_attr_groups);
hid_hw_close(hdev);
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v2 2/2] HID: lenovo-go: drop dead NULL check on to_usb_interface()
From: sashiko-bot @ 2026-05-15 15:36 UTC (permalink / raw)
To: Louis Clinckx; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <66162775a1a939891fde1a96660825df6f3188ca.1778701376.git.clinckx.louis@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Resource Leak and Use-After-Free due to Asymmetric Device Cleanup
--
commit 7509c2865ff8f5400d44c180711298e07d9096ba
Author: Louis Clinckx <clinckx.louis@gmail.com>
HID: lenovo-go: drop dead NULL check on to_usb_interface()
to_usb_interface() is a container_of_const() macro: it performs
pointer arithmetic and never returns NULL. The if (!intf) and if
(intf) tests in get_endpoint_address() can never fire. Remove them
in both drivers.
[ ... ]
> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index 3fa1fe83f7e5e..e0c9d5ec9451b 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -641,9 +641,6 @@ static int get_endpoint_address(struct hid_device *hdev)
> struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
> struct usb_host_endpoint *ep;
>
> - if (!intf)
> - return -ENODEV;
> -
> ep = intf->cur_altsetting->endpoint;
> if (!ep)
> return -ENODEV;
This isn't a regression introduced by your patch, but I noticed an issue with
how -ENODEV is handled by the driver.
If get_endpoint_address() returns -ENODEV (e.g., when the USB interface has no
endpoints and ep is NULL), hid_go_probe() successfully binds the device:
hid_go_probe() {
...
ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
...
ret = hid_hw_open(hdev);
...
ep = get_endpoint_address(hdev);
if (ep != GO_GP_INTF_IN) {
dev_dbg(&hdev->dev, "Started interface %x as generic HID device\n", ep);
return 0;
}
...
}
However, hid_go_remove() skips the cleanup if ep is -ENODEV:
hid_go_remove() {
int ep = get_endpoint_address(hdev);
if (ep <= 0)
return;
...
}
Does this asymmetric cleanup cause a use-after-free or leak of the input devices
and HID parser buffers since hid_hw_close() and hid_hw_stop() are skipped?
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1778701376.git.clinckx.louis@gmail.com?part=2
^ permalink raw reply
* [PATCH] HID: letsketch: fix UAF on inrange_timer at driver unbind
From: Manish Khadka @ 2026-05-15 15:35 UTC (permalink / raw)
To: linux-input; +Cc: Hans de Goede, Jiri Kosina, Benjamin Tissoires, linux-kernel
letsketch_driver does not provide a .remove callback, but
letsketch_probe() arms a per-device timer:
timer_setup(&data->inrange_timer, letsketch_inrange_timeout, 0);
The timer is re-armed from letsketch_raw_event() with a 100 ms
timeout on every pen-in-range report, and its callback dereferences
data->input_tablet to deliver a synthetic BTN_TOOL_PEN release:
static void letsketch_inrange_timeout(struct timer_list *t)
{
struct letsketch_data *data =
timer_container_of(data, t, inrange_timer);
struct input_dev *input = data->input_tablet;
input_report_key(input, BTN_TOOL_PEN, 0);
input_sync(input);
}
letsketch_data is allocated with devm_kzalloc(), and its input_dev
fields are devm-allocated via letsketch_setup_input_tablet(). On
device unbind (USB unplug or rmmod), the HID core runs its default
teardown and devm cleanup frees both letsketch_data and the input
devices. Because no .remove callback exists, nothing drains the
timer first: if raw_event armed it within ~100 ms of the unbind, the
pending timer fires on freed memory. This is a UAF read of data and
of data->input_tablet, followed by input_report_key() / input_sync()
into the freed input_dev.
Fix by adding a .remove callback that drains the timer before
hid_hw_stop(). timer_delete_sync() on its own is not sufficient: a
URB completion still in flight during hid_hw_stop() can call
mod_timer() and re-arm the timer after the drain.
Introduce a spinlock and a 'removing' flag on letsketch_data.
letsketch_remove() sets the flag under the lock, then drains the
timer, then stops the hardware. letsketch_raw_event() now arms
mod_timer() under the same lock and skips arming if the flag is set,
so no path can re-arm the timer after remove() has drained it.
Fixes: 33a5c2793451 ("HID: Add new Letsketch tablet driver")
Cc: stable@vger.kernel.org
Signed-off-by: Manish Khadka <maskmemanish@gmail.com>
---
drivers/hid/hid-letsketch.c | 32 ++++++++++++++++++++++++++++++--
1 file changed, 30 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-letsketch.c b/drivers/hid/hid-letsketch.c
index 11e21f988723..0dc9496d05f8 100644
--- a/drivers/hid/hid-letsketch.c
+++ b/drivers/hid/hid-letsketch.c
@@ -36,6 +36,7 @@
*/
#include <linux/device.h>
#include <linux/input.h>
+#include <linux/cleanup.h>
#include <linux/hid.h>
#include <linux/module.h>
#include <linux/timer.h>
@@ -62,6 +63,8 @@ struct letsketch_data {
struct input_dev *input_tablet;
struct input_dev *input_tablet_pad;
struct timer_list inrange_timer;
+ spinlock_t lock; /* serialises arming inrange_timer vs. teardown */
+ bool removing; /* set during teardown; gates mod_timer */
};
static int letsketch_open(struct input_dev *dev)
@@ -189,9 +192,15 @@ static int letsketch_raw_event(struct hid_device *hdev,
get_unaligned_le16(raw_data + 6));
/*
* There is no out of range event, so use a timer for this
- * when in range we get an event approx. every 8 ms.
+ * when in range we get an event approx. every 8 ms. Skip
+ * arming if the driver is being torn down so the timer
+ * cannot outlive devm-freed data after letsketch_remove().
*/
- mod_timer(&data->inrange_timer, jiffies + msecs_to_jiffies(100));
+ scoped_guard(spinlock_irqsave, &data->lock) {
+ if (!data->removing)
+ mod_timer(&data->inrange_timer,
+ jiffies + msecs_to_jiffies(100));
+ }
break;
case 0xe0: /* Pad data */
input = data->input_tablet_pad;
@@ -291,6 +300,7 @@ static int letsketch_probe(struct hid_device *hdev, const struct hid_device_id *
return -ENOMEM;
data->hdev = hdev;
+ spin_lock_init(&data->lock);
timer_setup(&data->inrange_timer, letsketch_inrange_timeout, 0);
hid_set_drvdata(hdev, data);
@@ -305,6 +315,23 @@ static int letsketch_probe(struct hid_device *hdev, const struct hid_device_id *
return hid_hw_start(hdev, HID_CONNECT_HIDRAW);
}
+static void letsketch_remove(struct hid_device *hdev)
+{
+ struct letsketch_data *data = hid_get_drvdata(hdev);
+
+ /*
+ * Block raw_event from arming inrange_timer during teardown so
+ * timer_delete_sync() below cannot race with a fresh mod_timer()
+ * issued from a URB completion handler still in flight while
+ * hid_hw_stop() is running.
+ */
+ scoped_guard(spinlock_irqsave, &data->lock)
+ data->removing = true;
+
+ timer_delete_sync(&data->inrange_timer);
+ hid_hw_stop(hdev);
+}
+
static const struct hid_device_id letsketch_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_LETSKETCH, USB_DEVICE_ID_WP9620N) },
{ }
@@ -315,6 +342,7 @@ static struct hid_driver letsketch_driver = {
.name = "letsketch",
.id_table = letsketch_devices,
.probe = letsketch_probe,
+ .remove = letsketch_remove,
.raw_event = letsketch_raw_event,
};
module_hid_driver(letsketch_driver);
--
2.43.0
^ permalink raw reply related
* [PATCH] HID: appleir: fix UAF on pending key_up_timer in remove()
From: Manish Khadka @ 2026-05-15 15:34 UTC (permalink / raw)
To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, linux-kernel
appleir_remove() runs hid_hw_stop() before timer_delete_sync().
hid_hw_stop() synchronously unregisters the HID input device via
hid_disconnect() -> hidinput_disconnect() -> input_unregister_device(),
which drops the last reference and frees the underlying input_dev when
no userspace handle holds it open.
key_up_tick() reads appleir->input_dev and calls input_report_key() /
input_sync() on it. The timer is armed from appleir_raw_event() with
a HZ/8 (~125 ms) timeout on every keydown and key-repeat report. If a
key was pressed shortly before the device is disconnected, the timer
can fire after hid_hw_stop() has freed input_dev but before
timer_delete_sync() drains it:
CPU0 CPU1
appleir_raw_event()
mod_timer(...) <- timer armed for 125 ms
...
USB disconnect
appleir_remove()
hid_hw_stop()
hid_disconnect()
hidinput_disconnect()
input_unregister_device() <- input_dev freed
key_up_tick()
key_up()
input_report_key(
appleir->input_dev, ...)
^^^^^^^^^ UAF
timer_delete_sync() <- too late
A simple reorder is not sufficient. Putting timer_delete_sync() first
still leaves a window where a USB URB completion (raw_event) running
during hid_hw_stop() can call mod_timer() and re-arm the timer, which
then fires after hidinput_disconnect() has freed input_dev.
Introduce a 'removing' flag on struct appleir, gated by the existing
spinlock. appleir_remove() sets the flag under the lock and then
drains the timer; appleir_raw_event() and key_up_tick() bail out
early if the flag is set, so no path can arm or run the timer after
remove() has drained it.
The keyrepeat branch of appleir_raw_event() previously called
key_down() and mod_timer() without holding the spinlock; take it now
so the flag check is well-defined. This incidentally closes a
pre-existing read-side race on appleir->current_key in that branch.
This bug is structurally a sibling of commit 4db2af929279 ("HID:
appletb-kbd: fix UAF in inactivity-timer cleanup path") and has been
present since the driver was introduced.
Fixes: 9a4a5574ce42 ("HID: appleir: add support for Apple ir devices")
Cc: stable@vger.kernel.org
Signed-off-by: Manish Khadka <maskmemanish@gmail.com>
---
drivers/hid/hid-appleir.c | 40 ++++++++++++++++++++++++++++++---------
1 file changed, 31 insertions(+), 9 deletions(-)
diff --git a/drivers/hid/hid-appleir.c b/drivers/hid/hid-appleir.c
index 5e8ced7bc05a..0e51f1e16653 100644
--- a/drivers/hid/hid-appleir.c
+++ b/drivers/hid/hid-appleir.c
@@ -109,9 +109,10 @@ struct appleir {
struct hid_device *hid;
unsigned short keymap[ARRAY_SIZE(appleir_key_table)];
struct timer_list key_up_timer; /* timer for key up */
- spinlock_t lock; /* protects .current_key */
+ spinlock_t lock; /* protects .current_key, .removing */
int current_key; /* the currently pressed key */
int prev_key_idx; /* key index in a 2 packets message */
+ bool removing; /* set during teardown; gates input_dev access */
};
static int get_key(int data)
@@ -172,7 +173,7 @@ static void key_up_tick(struct timer_list *t)
unsigned long flags;
spin_lock_irqsave(&appleir->lock, flags);
- if (appleir->current_key) {
+ if (!appleir->removing && appleir->current_key) {
key_up(hid, appleir, appleir->current_key);
appleir->current_key = 0;
}
@@ -195,6 +196,10 @@ static int appleir_raw_event(struct hid_device *hid, struct hid_report *report,
int index;
spin_lock_irqsave(&appleir->lock, flags);
+ if (appleir->removing) {
+ spin_unlock_irqrestore(&appleir->lock, flags);
+ goto out;
+ }
/*
* If we already have a key down, take it up before marking
* this one down
@@ -229,12 +234,17 @@ static int appleir_raw_event(struct hid_device *hid, struct hid_report *report,
appleir->prev_key_idx = 0;
if (!memcmp(data, keyrepeat, sizeof(keyrepeat))) {
- key_down(hid, appleir, appleir->current_key);
- /*
- * Remote doesn't do key up, either pull them up, in the test
- * above, or here set a timer which pulls them up after 1/8 s
- */
- mod_timer(&appleir->key_up_timer, jiffies + HZ / 8);
+ spin_lock_irqsave(&appleir->lock, flags);
+ if (!appleir->removing) {
+ key_down(hid, appleir, appleir->current_key);
+ /*
+ * Remote doesn't do key up, either pull them up, in
+ * the test above, or here set a timer which pulls them
+ * up after 1/8 s
+ */
+ mod_timer(&appleir->key_up_timer, jiffies + HZ / 8);
+ }
+ spin_unlock_irqrestore(&appleir->lock, flags);
goto out;
}
@@ -318,8 +328,20 @@ static int appleir_probe(struct hid_device *hid, const struct hid_device_id *id)
static void appleir_remove(struct hid_device *hid)
{
struct appleir *appleir = hid_get_drvdata(hid);
- hid_hw_stop(hid);
+ unsigned long flags;
+
+ /*
+ * Mark the driver as tearing down so that any concurrent raw_event
+ * (e.g. from a USB URB completion that hid_hw_stop() has not yet
+ * killed) and the key_up_timer softirq stop touching input_dev
+ * before hid_hw_stop() frees it via hidinput_disconnect().
+ */
+ spin_lock_irqsave(&appleir->lock, flags);
+ appleir->removing = true;
+ spin_unlock_irqrestore(&appleir->lock, flags);
+
timer_delete_sync(&appleir->key_up_timer);
+ hid_hw_stop(hid);
}
static const struct hid_device_id appleir_devices[] = {
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v2 1/2] HID: lenovo-go: reject non-USB transports in probe
From: sashiko-bot @ 2026-05-15 15:20 UTC (permalink / raw)
To: Louis Clinckx; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <e8f92f07a397f39680c72ca69a5ada9882ff7212.1778701376.git.clinckx.louis@gmail.com>
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] This problem wasn't introduced by this patch, but both drivers use a static global variable (`drvdata`) to store device state rather than allocating it per-device.
- [High] This problem wasn't introduced by this patch, but multicolor LED configuration structs (`go_cdev_rgb` and `gos_cdev_rgb`) are declared as static global variables but are passed to `devm_led_classdev_multicolor_register()`.
- [High] This problem wasn't introduced by this patch, but in `hid-lenovo-go.c`, the removal function `hid_go_cfg_remove()` fails to cancel the delayed workqueue `go_cfg_setup`.
- [High] This problem wasn't introduced by this patch, but probe failures do not correctly unwind started hardware states and sysfs groups.
- [High] This problem wasn't introduced by this patch, but a NULL pointer is passed to `memcpy` in `mcu_property_out()` when performing `GET_*` property requests.
- [High] This problem wasn't introduced by this patch, but completion synchronization is broken in `mcu_property_out()` due to incorrect placement of `reinit_completion()`.
--
commit 7a455849d3633575795dff10836095e31a4bbca3
Author: Louis Clinckx <clinckx.louis@gmail.com>
HID: lenovo-go: reject non-USB transports in probe
These drivers only match HID_USB_DEVICE() entries and assume the
underlying bus is USB. Make that explicit at probe by rejecting any
non-USB hdev.
> diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
> index 01c7bdd4fbe04..abf477e680428 100644
> --- a/drivers/hid/hid-lenovo-go-s.c
> +++ b/drivers/hid/hid-lenovo-go-s.c
> @@ -1432,6 +1432,9 @@ static int hid_gos_probe(struct hid_device *hdev,
> {
> int ret, ep;
>
> + if (!hid_is_usb(hdev))
> + return -EINVAL;
> +
[ ... ]
> ret = hid_gos_cfg_probe(hdev, id);
> if (ret)
> dev_err_probe(&hdev->dev, ret, "Failed to start configuration interface");
This isn't a regression from this patch, but do these probe failures
leave the hardware state active?
If hid_gos_cfg_probe() fails, the code returns the error immediately without
calling hid_hw_close(hdev) and hid_hw_stop(hdev).
Additionally, does sysfs_create_groups() need to be rolled back if later
steps in the config probe fail?
> dev_dbg(&hdev->dev, "Started interface %x as Go S configuration interface\n", ep);
> return ret;
> }
[ ... ]
> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index d4d26c7833563..3fa1fe83f7e5e 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -2419,6 +2419,9 @@ static int hid_go_probe(struct hid_device *hdev, const struct hid_device_id *id)
> {
> int ret, ep;
>
> + if (!hid_is_usb(hdev))
> + return -EINVAL;
> +
[ ... ]
> ret = hid_go_cfg_probe(hdev, id);
> if (ret)
> dev_err_probe(&hdev->dev, ret, "Failed to start configuration interface\n");
This isn't a regression from this patch, but does this early return
bypass hid_hw_close(hdev) and hid_hw_stop(hdev) as well?
> dev_dbg(&hdev->dev, "Started Legion Go HID Device: %x\n", ep);
>
> return ret;
> }
[ ... ]
This isn't a regression from this patch, but does the use of a static
global drvdata struct cause data races when multiple devices are connected?
Since drvdata is shared, functions like mutex_init() and init_completion()
called during probe() would re-execute on the same memory space. Could this
cause type confusion over drvdata.hdev and memory corruption from concurrent
sysfs accesses or input events?
This isn't a regression from this patch, but do the static global
go_cdev_rgb and gos_cdev_rgb structs corrupt the kernel's global leds_list?
The led framework embeds a struct list_head into the led_classdev structure.
If devm_led_classdev_multicolor_register() is called for multiple devices
using the same static struct, it would double-add the list node.
Would unbinding one device then unlink the shared struct, leading to a
use-after-free when the other device is later accessed or removed?
This isn't a regression from this patch, but does hid_go_cfg_remove()
need to cancel the go_cfg_setup delayed workqueue?
If the controller is rapidly unplugged after hid_go_cfg_probe() schedules
drvdata.go_cfg_setup, hid_go_cfg_remove() will tear down the device.
Without calling cancel_delayed_work_sync(), could the workqueue execute
and dereference the freed hdev pointer in mcu_property_out()?
This isn't a regression from this patch, but does mcu_property_out() pass
a NULL pointer to memcpy() during get property commands?
Callers intentionally pass NULL as the data pointer and 0 as len for these
commands. Since passing NULL to memcpy() is undefined behavior even when
len is 0, could this trigger sanitizer violations?
This isn't a regression from this patch, but is the completion
synchronization in mcu_property_out() susceptible to spontaneous hardware
events?
Because reinit_completion() is called at the end of mcu_property_out(), if
hid_go_raw_event() invokes complete() due to spontaneous events or multiple
responses, the completion object could be left in a signaled state.
Would the next call to wait_for_completion_interruptible_timeout() then
return instantly and provide stale or uninitialized memory to user-space?
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1778701376.git.clinckx.louis@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH v3 07/11] HID: Use trace_call__##name() at guarded tracepoint call sites
From: srinivas pandruvada @ 2026-05-15 15:09 UTC (permalink / raw)
To: Vineeth Pillai (Google), Jiri Kosina, Benjamin Tissoires
Cc: linux-input, Steven Rostedt, linux-trace-kernel, Peter Zijlstra
In-Reply-To: <20260515135941.2238861-1-vineeth@bitbyteword.org>
On Fri, 2026-05-15 at 09:59 -0400, Vineeth Pillai (Google) wrote:
> From: Vineeth Pillai <vineeth@bitbyteword.org>
>
> Replace trace_foo() with the new trace_call__foo() at sites already
> guarded by trace_foo_enabled(), avoiding a redundant
> static_branch_unlikely() re-evaluation inside the tracepoint.
> trace_call__foo() calls the tracepoint callbacks directly without
> utilizing the static branch again.
>
> Original v2 series:
> https://lore.kernel.org/linux-trace-kernel/20260323160052.17528-1-vineeth@bitbyteword.org/
>
> Parts of the original v2 series have already been merged in mainline.
> This patch is being reposted as a follow-up cleanup for the remaining
> unmerged pieces.
>
> Suggested-by: Steven Rostedt <rostedt@goodmis.org>
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Vineeth Pillai (Google) <vineeth@bitbyteword.org>
> Assisted-by: Claude:claude-sonnet-4-6
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> drivers/hid/intel-ish-hid/ipc/pci-ish.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> index ed3405c05e73..8d36ae96a3ee 100644
> --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> @@ -110,7 +110,7 @@ void ish_event_tracer(struct ishtp_device *dev,
> const char *format, ...)
> vsnprintf(tmp_buf, sizeof(tmp_buf), format, args);
> va_end(args);
>
> - trace_ishtp_dump(tmp_buf);
> + trace_call__ishtp_dump(tmp_buf);
> }
> }
>
^ permalink raw reply
* [PATCH v2 2/2] HID: lenovo-go: drop dead NULL check on to_usb_interface()
From: Louis Clinckx @ 2026-05-15 14:57 UTC (permalink / raw)
To: Derek J . Clark, Mark Pearson, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel
In-Reply-To: <cover.1778701376.git.clinckx.louis@gmail.com>
to_usb_interface() is a container_of_const() macro: it performs
pointer arithmetic and never returns NULL. The if (!intf) and if
(intf) tests in get_endpoint_address() can never fire. Remove them
in both drivers.
No functional change.
Suggested-by: Derek J. Clark <derekjohn.clark@gmail.com>
Signed-off-by: Louis Clinckx <clinckx.louis@gmail.com>
---
drivers/hid/hid-lenovo-go-s.c | 8 +++-----
drivers/hid/hid-lenovo-go.c | 3 ---
2 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
index abf477e68..f44aeb2c1 100644
--- a/drivers/hid/hid-lenovo-go-s.c
+++ b/drivers/hid/hid-lenovo-go-s.c
@@ -382,11 +382,9 @@ static int get_endpoint_address(struct hid_device *hdev)
struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
struct usb_host_endpoint *ep;
- if (intf) {
- ep = intf->cur_altsetting->endpoint;
- if (ep)
- return ep->desc.bEndpointAddress;
- }
+ ep = intf->cur_altsetting->endpoint;
+ if (ep)
+ return ep->desc.bEndpointAddress;
return -ENODEV;
}
diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
index 3fa1fe83f..e0c9d5ec9 100644
--- a/drivers/hid/hid-lenovo-go.c
+++ b/drivers/hid/hid-lenovo-go.c
@@ -641,9 +641,6 @@ static int get_endpoint_address(struct hid_device *hdev)
struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
struct usb_host_endpoint *ep;
- if (!intf)
- return -ENODEV;
-
ep = intf->cur_altsetting->endpoint;
if (!ep)
return -ENODEV;
--
2.39.5
^ permalink raw reply related
* [PATCH v2 1/2] HID: lenovo-go: reject non-USB transports in probe
From: Louis Clinckx @ 2026-05-15 14:57 UTC (permalink / raw)
To: Derek J . Clark, Mark Pearson, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel
In-Reply-To: <cover.1778701376.git.clinckx.louis@gmail.com>
These drivers only match HID_USB_DEVICE() entries and assume the
underlying bus is USB. Make that explicit at probe by rejecting any
non-USB hdev, following the pattern used by other HID drivers.
Signed-off-by: Louis Clinckx <clinckx.louis@gmail.com>
---
drivers/hid/hid-lenovo-go-s.c | 3 +++
drivers/hid/hid-lenovo-go.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
index 01c7bdd4f..abf477e68 100644
--- a/drivers/hid/hid-lenovo-go-s.c
+++ b/drivers/hid/hid-lenovo-go-s.c
@@ -1432,6 +1432,9 @@ static int hid_gos_probe(struct hid_device *hdev,
{
int ret, ep;
+ if (!hid_is_usb(hdev))
+ return -EINVAL;
+
ret = hid_parse(hdev);
if (ret) {
hid_err(hdev, "Parse failed\n");
diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
index d4d26c783..3fa1fe83f 100644
--- a/drivers/hid/hid-lenovo-go.c
+++ b/drivers/hid/hid-lenovo-go.c
@@ -2419,6 +2419,9 @@ static int hid_go_probe(struct hid_device *hdev, const struct hid_device_id *id)
{
int ret, ep;
+ if (!hid_is_usb(hdev))
+ return -EINVAL;
+
hdev->quirks |= HID_QUIRK_INPUT_PER_APP | HID_QUIRK_MULTI_INPUT;
ret = hid_parse(hdev);
--
2.39.5
^ permalink raw reply related
* [PATCH v2 0/2] HID: lenovo-go: clean up USB assumption
From: Louis Clinckx @ 2026-05-15 14:57 UTC (permalink / raw)
To: Derek J . Clark, Mark Pearson, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel
These drivers' id_tables only match HID_USB_DEVICE() entries and the
code freely assumes a USB transport (to_usb_interface() on hdev->dev.parent,
endpoint access in raw_event). Make the assumption explicit at probe and
drop a NULL check that the compiler-level definition of to_usb_interface()
makes unreachable.
No functional change for the supported (USB) devices.
Louis Clinckx (2):
HID: lenovo-go: reject non-USB transports in probe
HID: lenovo-go: drop dead NULL check on to_usb_interface()
drivers/hid/hid-lenovo-go-s.c | 11 ++++++-----
drivers/hid/hid-lenovo-go.c | 6 +++---
2 files changed, 9 insertions(+), 8 deletions(-)
---
Changes since v1:
- Resubmitted on linux-input as a quality fix per Benjamin's review of v1
(sent to security@kernel.org; the HID_USB_DEVICE() id_table already
filters BUS_USB, so no exploitable path).
- Added patch 2 to remove the dead NULL check, at Derek's suggestion.
base-commit: 50897c955902c93ae71c38698abb910525ebdc89
--
2.39.5
^ permalink raw reply
* [PATCH v3 07/11] HID: Use trace_call__##name() at guarded tracepoint call sites
From: Vineeth Pillai (Google) @ 2026-05-15 13:59 UTC (permalink / raw)
To: Srinivas Pandruvada, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, Steven Rostedt, linux-trace-kernel, Vineeth Pillai,
Peter Zijlstra
From: Vineeth Pillai <vineeth@bitbyteword.org>
Replace trace_foo() with the new trace_call__foo() at sites already
guarded by trace_foo_enabled(), avoiding a redundant
static_branch_unlikely() re-evaluation inside the tracepoint.
trace_call__foo() calls the tracepoint callbacks directly without
utilizing the static branch again.
Original v2 series:
https://lore.kernel.org/linux-trace-kernel/20260323160052.17528-1-vineeth@bitbyteword.org/
Parts of the original v2 series have already been merged in mainline.
This patch is being reposted as a follow-up cleanup for the remaining
unmerged pieces.
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Vineeth Pillai (Google) <vineeth@bitbyteword.org>
Assisted-by: Claude:claude-sonnet-4-6
---
drivers/hid/intel-ish-hid/ipc/pci-ish.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
index ed3405c05e73..8d36ae96a3ee 100644
--- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
+++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
@@ -110,7 +110,7 @@ void ish_event_tracer(struct ishtp_device *dev, const char *format, ...)
vsnprintf(tmp_buf, sizeof(tmp_buf), format, args);
va_end(args);
- trace_ishtp_dump(tmp_buf);
+ trace_call__ishtp_dump(tmp_buf);
}
}
--
2.54.0
^ permalink raw reply related
* Re: [PATCH RFC 3/4] Input: gpio-keys - add regulator to gpio_keys
From: Griffin Kroah-Hartman @ 2026-05-15 11:41 UTC (permalink / raw)
To: Mark Brown
Cc: Dmitry Torokhov, Liam Girdwood, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Luca Weiss,
linux-input, linux-kernel, linux-arm-msm, devicetree
In-Reply-To: <af3oqSshbXrUYMnz@sirena.co.uk>
On 5/8/26 15:44, Mark Brown wrote:
> On Fri, May 08, 2026 at 02:53:15PM +0200, Griffin Kroah-Hartman wrote:
>
>> + if (fwnode_property_present(child, "vdd-supply")) {
>> + button->regulator = devm_fwnode_regulator_get_optional(dev, child, "vdd");
>> + if (IS_ERR(button->regulator)) {
>
> As well as the issue I mentioned on a prior thread with this assigning a
> non-physical "vdd" name to the single supply that these components can
> have (which has had issues in the past
Our Hall Effect sensor IC does have a named "vdd" pin, but we are
totally open to changing this to power-supply or whatever best follows
the standard.
> painful) the fact that this is fwnode means that this opens up support
> for using this with ACPI which is very problematic given that ACPI has a
> strong model of how regulators should work which is that they should not
> be OS visible at all.
Would it be more appropriate to drop the devm_fwnode_regulator_get() and
replace it with a type-casted devm_of_regulator_get()?
> That probably needs more addressing in the prior regulator patch, that
> needs a bit more motivation and discussion about the issues with trying
> to do a regulator interface firmware neutrally.
Thank you for your response and feedback!
^ permalink raw reply
* Re: [PATCH 5/6] tools/build: selftests: Allow versioning LLVM lld
From: James Clark @ 2026-05-15 8:25 UTC (permalink / raw)
To: sashiko-reviews; +Cc: dmitry.torokhov, bpf, linux-input, linux-perf-users
In-Reply-To: <20260514184543.C6400C2BCB3@smtp.kernel.org>
On 14/05/2026 7:45 pm, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Using `-fuse-ld=ld.lld` breaks compilation of BPF selftests with GCC.
> --
>
> commit 18e1876d18fad631b521da817483ba47293a9020
> Author: James Clark <james.clark@linaro.org>
>
> tools/build: selftests: Allow versioning LLVM lld
>
> This commit updates Makefile.include to add an LLD variable, matching the
> rest of the LLVM toolchain. It modifies the BPF selftests Makefile to use
> this new LLD variable instead of hardcoding lld.
>
>> diff --git a/tools/scripts/Makefile.include b/tools/scripts/Makefile.include
>> index 380ad84ac51e6..5c2d505cba62b 100644
>> --- a/tools/scripts/Makefile.include
>> +++ b/tools/scripts/Makefile.include
> [ ... ]
>> @@ -91,6 +92,7 @@ else
>> # Some tools still require Clang, LLC and/or LLVM utils
>> $(call allow-override,CLANG,clang)
>> $(call allow-override,LLC,llc)
>> + $(call allow-override,LLD,ld.lld)
>
> Could setting LLD to ld.lld when not using LLVM cause issues for BPF
> selftests compilation with GCC?
>
$(LLD) is only used in one place, and it's used with $(CLANG) so this is
impossible.
>> $(call allow-override,LLVM_CONFIG,llvm-config)
>> $(call allow-override,LLVM_OBJCOPY,llvm-objcopy)
>> $(call allow-override,LLVM_STRIP,llvm-strip)
>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
>> index 6ef6872adbc3e..44ba829e5d4dd 100644
>> --- a/tools/testing/selftests/bpf/Makefile
>> +++ b/tools/testing/selftests/bpf/Makefile
>> @@ -245,9 +245,9 @@ $(OUTPUT)/%:%.c
>>
>> # LLVM's ld.lld doesn't support all the architectures, so use it only on x86
>> ifeq ($(SRCARCH),$(filter $(SRCARCH),x86 riscv))
>> -LLD := lld
>> +USE_LD := $(LLD)
>> else
>> -LLD := $(shell command -v $(LD))
>> +USE_LD := $(shell command -v $(LD))
>> endif
>>
>> # Filter out -static for liburandom_read.so and its dependent targets so that static builds
>> @@ -258,7 +258,7 @@ $(OUTPUT)/liburandom_read.so: urandom_read_lib1.c urandom_read_lib2.c liburandom
>> $(filter-out -static,$(CFLAGS) $(LDFLAGS)) \
>> $(filter %.c,$^) $(filter-out -static,$(LDLIBS)) \
>> -Wno-unused-command-line-argument \
>> - -fuse-ld=$(LLD) -Wl,-znoseparate-code -Wl,--build-id=sha1 \
>> + -fuse-ld=$(USE_LD) -Wl,-znoseparate-code -Wl,--build-id=sha1 \
>
> When LLVM=1 is not set, CC defaults to gcc. The Makefile uses
> $(CC) ... -fuse-ld=$(USE_LD) to link liburandom_read.so.
>
> Because USE_LD evaluates to ld.lld, gcc receives -fuse-ld=ld.lld.
>
> However, GCC's command-line option parser is strictly hardcoded to accept
> only specific linker names (lld, bfd, gold, mold). It does not recognize
> -fuse-ld=ld.lld or absolute paths, and fails with an error like:
>
> gcc: error: unrecognized command-line option '-fuse-ld=ld.lld'; did you mean '-fuse-ld=lld'?
>
> Does this regression break the default GCC build of BPF selftests on x86
> and riscv architectures?
>
Same as above. $(CC) is not used with $(LLD). You'd probably want to use
$(LD) in a $(CC) build command anyway.
^ permalink raw reply
* Re: [PATCH] HID: lg-g15: Add support for the Logitech G710/G710+ gaming keyboards
From: Hans de Goede @ 2026-05-15 7:52 UTC (permalink / raw)
To: Xavier Bestel, Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel
In-Reply-To: <094db7d8-eb0c-4103-86fb-beb11580d53b@kernel.org>
Hi,
On 6-Apr-26 20:50, Hans de Goede wrote:
> Hi Xavier,
>
> Thank you for your patch, it is always nice to have people helping
> with improving Linux support for devices like this.
>
> On 2-Apr-26 09:52, Xavier Bestel wrote:
>> Add support for the Logitech G710 and G710+ (USB ID 046d:c24d) gaming
>> keyboards to the hid-lg-g15 driver.
>>
>> These keyboards have 6 G-keys (G1-G6), M-keys (M1-M3, MR), a game mode
>> toggle, and two independent backlights: one for the main keyboard and one
>> for the WASD keys, each with a physical button to cycle brightness through
>> 5 levels (0-4).
>>
>> Key implementation details:
>>
>> - G-keys and M-keys are reported via HID input report 0x03 (4 bytes)
>> using KEY_MACRO1-6, KEY_MACRO_PRESET1-3 and KEY_MACRO_RECORD_START.
>>
>> - The WASD backlight LED is registered as
>> "g15::kbd_zoned_backlight-wasd" rather than with a
>> "::kbd_backlight" suffix, because UPower currently only supports a
>> single kbd_backlight LED per device. This follows the nomenclature
>> from Documentation/leds/leds-class.rst
>
> Actually that nomenclature is something which was suggested in the past
> but has never been agreed upon.
>
> Since the discussion about how to name the LED class devices for
> multi-zone keyboard backlights keeps coming up I've submitted a patch
> now to document how these should be named:
>
> https://lore.kernel.org/linux-leds/20260406174638.320135-1-johannes.goede@oss.qualcomm.com/
>
> Since the new userspace API for this needs to be agreed upon first, that
> patch should be accepted upstream first, before we can move forward with
> the G710 support from this patch.
My v2 patch with the LED naming scheme has been accepted into the
leds maintainer git tree now, so this is no longer a blocker for this
patch:
https://lore.kernel.org/linux-leds/20260504145434.12746-1-johannes.goede@oss.qualcomm.com/
Regards,
Hans
^ permalink raw reply
* Re: [PATCH v2] Input: elan_i2c - prevent division by zero on invalid device parameters
From: sashiko-bot @ 2026-05-15 7:10 UTC (permalink / raw)
To: Ranjan Kumar; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260515065235.1913403-1-kumarranja@chromium.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Setting `width_x` and `width_y` to 1 as a fallback introduces a severe arithmetic underflow when calculating touch area sizes.
--
commit 592cc1df694c917c489ca1a6dcea8bd8b7a8d985
Author: Ranjan Kumar <kumarranja@chromium.org>
Input: elan_i2c - prevent division by zero on invalid device parameters
The Elan I2C touchpad driver queries the device for its physical dimensions
and trace counts to calculate the device resolution and width. If the device
provides invalid zero values, this commit adds checks to fall back to safe
default values of 1 to bypass the division and prevent a kernel panic.
> diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> index fee1796da3d08..32b2849ab36a7 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -425,8 +425,17 @@ static int elan_query_device_parameters(struct elan_tp_data *data)
> if (error)
> return error;
> }
> - data->width_x = data->max_x / x_traces;
> - data->width_y = data->max_y / y_traces;
> +
> + if (unlikely(x_traces == 0 || y_traces == 0)) {
> + dev_warn(&client->dev,
> + "invalid trace numbers: x=%u, y=%u\n",
> + x_traces, y_traces);
> + data->width_x = 1;
> + data->width_y = 1;
Does setting these fallback values to 1 introduce an arithmetic underflow
regression when calculating touch area sizes?
In elan_report_absolute() and elan_report_contact(), the touch area sizes are
calculated using an expression like:
area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE);
Since ETP_FWIDTH_REDUCE is 90 and data->width_x is an unsigned int, setting
the width to 1 causes the subtraction to underflow, resulting in a massive
unsigned integer.
When passed to input_report_abs(), this value is cast back to a negative
number, violating the 0 minimum boundary configured for ABS_MT_TOUCH_MAJOR.
Could this confuse userspace input handlers and trigger unwanted behaviors?
To prevent this underflow while maintaining the firmware update fallback path,
should the fallback value be at least ETP_FWIDTH_REDUCE (90) instead of 1?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260515065235.1913403-1-kumarranja@chromium.org?part=1
^ permalink raw reply
* Re: [PATCH v2 2/7] dt-bindings: crypto: Add x1e80100 inline crypto
From: Krzysztof Kozlowski @ 2026-05-15 7:04 UTC (permalink / raw)
To: Harrison Vanderbyl
Cc: andersson, konradybcio, robh, krzk+dt, conor+dt, Herbert Xu,
davem, neil.armstrong, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, jikos, bentiss, luzmaximilian, hansg,
ilpo.jarvinen, Douglas Anderson, Jessica Zhang, linux-arm-msm,
devicetree, linux-kernel, linux-crypto, dri-devel, linux-input,
platform-driver-x86
In-Reply-To: <14cd42e3d3af4b2591c9dd8dffde11ef18666751.1778822464.git.harrison.vanderbyl@gmail.com>
On Fri, May 15, 2026 at 03:41:47PM +1000, Harrison Vanderbyl wrote:
> Add compatibility string for the x1e80100/x1p42100
> inline crypto engine.
>
> Signed-off-by: Harrison Vanderbyl <harrison.vanderbyl@gmail.com>
This was posted, please do not duplicate work.
https://lore.kernel.org/all/eggp3un5ufbw2mjamxmpfccq3cs2luxabpa2sucofydzoak4vg@hy7mx3rtqfko/
Do not attach (thread) your patchsets to some other threads (unrelated
or older versions). This buries them deep in the mailbox and might
interfere with applying entire sets. See also:
https://elixir.bootlin.com/linux/v6.16-rc2/source/Documentation/process/submitting-patches.rst#L830
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 1/7] dt-bindings: arm: qcom: Add Microsoft Surface Pro 12in
From: Krzysztof Kozlowski @ 2026-05-15 7:03 UTC (permalink / raw)
To: Harrison Vanderbyl
Cc: andersson, konradybcio, robh, krzk+dt, conor+dt, Herbert Xu,
davem, neil.armstrong, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, jikos, bentiss, luzmaximilian, hansg,
ilpo.jarvinen, Douglas Anderson, Jessica Zhang, linux-arm-msm,
devicetree, linux-kernel, linux-crypto, dri-devel, linux-input,
platform-driver-x86
In-Reply-To: <e54aa6c1e190b0e26d58504c5ea1b05fd09d64d3.1778822464.git.harrison.vanderbyl@gmail.com>
On Fri, May 15, 2026 at 03:41:46PM +1000, Harrison Vanderbyl wrote:
> Document the compatible string for the Microsoft Surface Pro
> 12-inch, 1st Edition with Snapdragon, based on the Qualcomm X1P42100
> SoC.
>
> Signed-off-by: Harrison Vanderbyl <harrison.vanderbyl@gmail.com>
> ---
> Documentation/devicetree/bindings/arm/qcom.yaml | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/qcom.yaml b/Documentation/devicetree/bindings/arm/qcom.yaml
> index b4943123d2e4..aaa9a129908a 100644
> --- a/Documentation/devicetree/bindings/arm/qcom.yaml
> +++ b/Documentation/devicetree/bindings/arm/qcom.yaml
> @@ -1168,6 +1168,10 @@ properties:
> - const: microsoft,denali
> - const: qcom,x1e80100
>
> + - items:
> + - const: microsoft,surface-pro-12in
Why isn't this part of the other enum with all devices?
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH v2] Input: elan_i2c - prevent division by zero on invalid device parameters
From: Ranjan Kumar @ 2026-05-15 6:52 UTC (permalink / raw)
To: dmitry.torokhov; +Cc: bleung, bentiss, linux-input, linux-kernel, Ranjan Kumar
In-Reply-To: <agTlxHSZqSI8cnjK@google.com>
The Elan I2C touchpad driver queries the device for its physical
dimensions and trace counts to calculate the device resolution and width.
However, if the device firmware or device tree provides invalid zero
values for x_traces, y_traces, x_mm, or y_mm, it results in a fatal
division-by-zero exception leading to a kernel panic during device probe.
Add sanity checks to ensure these physical parameters are non-zero
before performing the division. If invalid values are detected, log a
warning and fall back to safe default values (1) to safely bypass the
division.
This prevents the kernel panic while allowing the probe to complete
successfully. Completing the probe ensures the sysfs nodes are created,
keeping the firmware update path intact so a recovery firmware can be
flashed to the device.
Fixes: 6696777c6506 ("Input: add driver for Elan I2C/SMbus touchpad")
Fixes: e3a9a1290688 ("Input: elan_i2c - do not query the info if they are provided")
Signed-off-by: Ranjan Kumar <kumarranja@chromium.org>
---
Changes in v2:
- Changed error handling from aborting probe with -EINVAL to logging a
warning and falling back to default values (1).
- Bypassing the division instead of aborting preserves the sysfs
interface for firmware updates and keeps the pointer functional.
drivers/input/mouse/elan_i2c_core.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index fee1796da3d0..32b2849ab36a 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -425,8 +425,17 @@ static int elan_query_device_parameters(struct elan_tp_data *data)
if (error)
return error;
}
- data->width_x = data->max_x / x_traces;
- data->width_y = data->max_y / y_traces;
+
+ if (unlikely(x_traces == 0 || y_traces == 0)) {
+ dev_warn(&client->dev,
+ "invalid trace numbers: x=%u, y=%u\n",
+ x_traces, y_traces);
+ data->width_x = 1;
+ data->width_y = 1;
+ } else {
+ data->width_x = data->max_x / x_traces;
+ data->width_y = data->max_y / y_traces;
+ }
if (device_property_read_u32(&client->dev,
"touchscreen-x-mm", &x_mm) ||
@@ -440,8 +449,16 @@ static int elan_query_device_parameters(struct elan_tp_data *data)
data->x_res = elan_convert_resolution(hw_x_res, data->pattern);
data->y_res = elan_convert_resolution(hw_y_res, data->pattern);
} else {
- data->x_res = (data->max_x + 1) / x_mm;
- data->y_res = (data->max_y + 1) / y_mm;
+ if (unlikely(x_mm == 0 || y_mm == 0)) {
+ dev_warn(&client->dev,
+ "invalid physical dimensions: x_mm=%u, y_mm=%u\n",
+ x_mm, y_mm);
+ data->x_res = 1;
+ data->y_res = 1;
+ } else {
+ data->x_res = (data->max_x + 1) / x_mm;
+ data->y_res = (data->max_y + 1) / y_mm;
+ }
}
if (device_property_read_bool(&client->dev, "elan,clickpad"))
--
2.54.0.563.g4f69b47b94-goog
^ permalink raw reply related
* Re: [PATCH v2 7/7] arm64: dts: qcom: Add Microsoft Surface Pro 12in
From: sashiko-bot @ 2026-05-15 6:56 UTC (permalink / raw)
To: Harrison Vanderbyl
Cc: krzk+dt, devicetree, robh, dmitry.torokhov, linux-input, conor+dt
In-Reply-To: <8ac29ee38ba80a3fbde8bfe43b74b9b936b31cb1.1778822464.git.harrison.vanderbyl@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Device Tree and Driver mismatch for WSA8845 speaker reset GPIO
- [Low] Incorrect indentation in `&pm8550_gpios` node
--
commit 0876a51b5a0549c867accb3d391daa4e0f3c28b3
Author: Harrison Vanderbyl <harrison.vanderbyl@gmail.com>
arm64: dts: qcom: Add Microsoft Surface Pro 12in
This commit introduces the initial device tree for the Microsoft Surface Pro
12in, adding support for various peripherals including UFS, touchscreen,
USB, audio, Wi-Fi, and Bluetooth.
> diff --git a/arch/arm64/boot/dts/qcom/x1p42100-microsoft-sp12in.dts b/arch/arm64/boot/dts/qcom/x1p42100-microsoft-sp12in.dts
> new file mode 100644
> index 0000000000000..32b8df2497913
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/x1p42100-microsoft-sp12in.dts
[ ... ]
> +&pm8550_gpios {
> + vol_up_n_default: vol-up-n-state {
> + pins = "gpio8";
> + function = "normal";
> + power-source = <1>;
> + bias-pull-up;
> + input-enable;
> + };
> +
> + vol_down_n_default: vol-down-n-state {
This isn't a bug, but should these subnodes be indented with a hardware tab
instead of spaces to match standard device tree coding guidelines?
[ ... ]
> +&swr0 {
> + pinctrl-0 = <&wsa_swr_active>, <&spkr_01_sd_n_active>;
> + pinctrl-names = "default";
> +
> + status = "okay";
> +
> + /* WSA8845, Left speaker */
> + left_spkr: speaker@0,0 {
> + compatible = "sdw20217020400";
> + reg = <0 0>;
> + reset-gpios = <&lpass_tlmm 12 GPIO_ACTIVE_LOW>;
Will this prevent the speakers from enumerating on the SoundWire bus?
Looking at the WSA884x codec driver in sound/soc/codecs/wsa884x.c, it
requests this GPIO by the name "powerdown":
devm_gpiod_get_optional(dev, "powerdown", ...)
Because the driver looks for the powerdown-gpios property, it will fail to
acquire the GPIO named reset-gpios here. Concurrently, the pinctrl state
spkr_01_sd_n_active drives this pin output-low, asserting the active-low
reset.
Since the driver never releases the reset, the speakers will stay powered
down and audio playback will fail. Should this property be renamed to
powerdown-gpios?
> + #sound-dai-cells = <0>;
> + sound-name-prefix = "SpkrLeft";
> + vdd-1p8-supply = <&vreg_l15b_1p8>;
> + vdd-io-supply = <&vreg_l12b_1p2>;
> + qcom,port-mapping = <1 2 3 7 10 13>;
> + };
> +
> + /* WSA8845, Right speaker */
> + right_spkr: speaker@0,1 {
> + compatible = "sdw20217020400";
> + reg = <0 1>;
> + reset-gpios = <&lpass_tlmm 12 GPIO_ACTIVE_LOW>;
Does this speaker node have the same naming issue where it should use
powerdown-gpios instead of reset-gpios?
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1778822464.git.harrison.vanderbyl@gmail.com?part=7
^ permalink raw reply
* Re: [PATCH v2 6/7] arm64: dts: qcom: hamoa: Add inline crypto for UFS
From: sashiko-bot @ 2026-05-15 6:24 UTC (permalink / raw)
To: Harrison Vanderbyl
Cc: devicetree, dmitry.torokhov, krzk+dt, conor+dt, linux-input, robh
In-Reply-To: <30c12b79c6cc481afb13ac93630c5a16bc856ae4.1778822464.git.harrison.vanderbyl@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The mapped register size for the `ice` (Inline Crypto Engine) node is too small, which will cause a kernel panic when Hardware Key Manager (HWKM) features are used.
--
commit d49e015ad1235b81d3dc9cb4256b44b206a03560
Author: Harrison Vanderbyl <harrison.vanderbyl@gmail.com>
arm64: dts: qcom: hamoa: Add inline crypto for UFS
This commit adds the Inline Crypto Engine node and wires it to ufs_mem to
enable UFS storage encryption on x1e80100 and derivative SOCs.
> diff --git a/arch/arm64/boot/dts/qcom/hamoa.dtsi b/arch/arm64/boot/dts/qcom/hamoa.dtsi
> --- a/arch/arm64/boot/dts/qcom/hamoa.dtsi
> +++ b/arch/arm64/boot/dts/qcom/hamoa.dtsi
[ ... ]
> @@ -3997,6 +3999,14 @@ opp-300000000 {
> };
> };
>
> + ice: crypto@1d88000 {
> + compatible = "qcom,x1e80100-inline-crypto-engine",
> + "qcom,inline-crypto-engine";
> + reg = <0x0 0x01d88000 0x0 0x8000>;
Will this mapped size be large enough when hardware-wrapped keys are enabled
via the Hardware Key Manager (HWKM)?
When the qcom_ice driver initializes HWKM, it accesses registers at offsets
that appear to exceed the 0x8000 length provided here. For example, it accesses
QCOM_ICE_REG_HWKM_TZ_KM_STATUS at 0x9004, and QCOM_ICE_REG_HWKM_BANK0_BBAC_4
at 0xD010.
Could reading or writing to these offsets cause a synchronous external abort
and crash the kernel since they fall outside the mapped memory region?
Should the size be expanded to cover the full HWKM address space, such as
0x18000, similar to the configuration on other HWKM-capable SoCs?
> +
> + clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
> + };
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1778822464.git.harrison.vanderbyl@gmail.com?part=6
^ 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