* Re: [PATCH v2] HID: appletb-kbd: add option to switch default layer on double pressing fn key
From: sashiko-bot @ 2026-05-26 16:51 UTC (permalink / raw)
To: Aditya Garg; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260526154954.29002-1-gargaditya08@proton.me>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues:
- [Medium] Dynamic mutation of a read-only global module parameter from a per-device atomic event handler without synchronization.
- [Medium] Incorrect double-press detection during early system uptime on 32-bit systems due to jiffies wrap-around with a 0-initialized timestamp.
- [Low] Intervening non-Fn key presses do not reset the double-press detection timer.
Pre-existing issues:
- [High] Calling sleeping functions (`hid_hw_power` and `hid_hw_request`) from an atomic context causes a kernel panic.
- [High] A race condition during `probe()` allows incoming HID events to trigger a NULL pointer dereference.
--
commit 9461d5707b93702971195b71e23ddea06392c720
Author: Aditya Garg <gargaditya08@proton.me>
HID: appletb-kbd: add option to switch default layer on double pressing fn key
This patch enables a user to switch the default layer from media to fn
keys and vice-versa upon double pressing the fn key. This behaviour can
be configured using the double_press_switch_time module parameter.
> diff --git a/drivers/hid/hid-appletb-kbd.c b/drivers/hid/hid-appletb-kbd.c
> index 462010a758993..34bfc595949dc 100644
> --- a/drivers/hid/hid-appletb-kbd.c
> +++ b/drivers/hid/hid-appletb-kbd.c
[ ... ]
> @@ -243,6 +250,18 @@ static int appletb_kbd_hid_event(struct hid_device *hdev, struct hid_field *fiel
> return kbd->current_mode == APPLETB_KBD_MODE_OFF;
> }
>
[Severity: High]
This is a pre-existing issue, but does a race condition during probe() allow
incoming HID events to trigger a NULL pointer dereference?
If hid_hw_start() and hid_hw_open() are called before hid_set_drvdata() is
executed, an incoming keyboard event might invoke appletb_kbd_hid_event()
before probe() finishes. This would call hid_get_drvdata(hdev) returning
NULL, which is then passed to reset_inactivity_timer() and unconditionally
dereferenced.
> +static u8 appletb_switch_mode(u8 mode)
> +{
[ ... ]
> @@ -250,15 +269,42 @@ static void appletb_kbd_inp_event(struct input_handle *handle, unsigned int type
>
> reset_inactivity_timer(kbd);
>
> - if (type == EV_KEY && code == KEY_FN && appletb_tb_fn_toggle &&
> - (kbd->current_mode == APPLETB_KBD_MODE_SPCL ||
> - kbd->current_mode == APPLETB_KBD_MODE_FN)) {
> + if (type == EV_KEY && code == KEY_FN &&
> + (kbd->current_mode == APPLETB_KBD_MODE_SPCL ||
> + kbd->current_mode == APPLETB_KBD_MODE_FN)) {
> +
[Severity: Low]
Is it possible that intervening non-Fn key presses should reset the
double-press detection timer?
If a user presses Fn, types another key, and then presses Fn again within
the delay timeout, the driver might erroneously interpret this as a
double-press and switch the layer.
> if (value == 1) {
> - kbd->saved_mode = kbd->current_mode;
> - appletb_kbd_set_mode(kbd, kbd->current_mode == APPLETB_KBD_MODE_SPCL
> - ? APPLETB_KBD_MODE_FN : APPLETB_KBD_MODE_SPCL);
> + if (appletb_tb_double_press_switch_layers > 0) {
> + unsigned long now = jiffies;
> +
> + if (time_before(now, kbd->last_fn_press +
> + msecs_to_jiffies(appletb_tb_double_press_switch_layers))) {
[Severity: Medium]
Could this double-press detection trigger incorrectly during early system
uptime on 32-bit systems?
Since last_fn_press is zero-initialized and jiffies starts at a large
negative value, time_before() might evaluate to true for the first 5
minutes of uptime.
This would cause any single press of the Fn key to erroneously trigger a
double-press layer switch.
> +
> + appletb_tb_def_mode =
> + appletb_switch_mode(
> + appletb_tb_def_mode);
[Severity: Medium]
Is it safe to dynamically mutate the appletb_tb_def_mode module parameter
from a per-device atomic event handler without synchronization?
Since appletb_tb_def_mode is a global variable exported as read-only,
modifying it dynamically could violate its sysfs contract and introduce a
data race if multiple keyboards are used concurrently.
This would also improperly alter the default mode for all current and future
connected devices.
> +
> + appletb_kbd_set_mode(kbd,
> + appletb_tb_def_mode);
[Severity: High]
This is a pre-existing issue, but does calling appletb_kbd_set_mode() from
this atomic context cause a kernel panic?
The appletb_kbd_inp_event handler is invoked by the input subsystem with
the dev->event_lock spinlock held and interrupts disabled.
appletb_kbd_set_mode() calls hid_hw_power() and hid_hw_request(). For USB
HID devices, hid_hw_power() may sleep and will warn in atomic context,
potentially leading to a scheduling while atomic panic.
> +
> + kbd->saved_mode = appletb_tb_def_mode;
> + kbd->last_fn_press = 0;
> +
> + return;
> + }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260526154954.29002-1-gargaditya08@proton.me?part=1
^ permalink raw reply
* Re: [PATCH v4 00/36] HID: iio: basic clean up and introduce devm_ API for HID sensors
From: Jonathan Cameron @ 2026-05-26 15:56 UTC (permalink / raw)
To: Sanjay Chitroda
Cc: jikos, srinivas.pandruvada, dlechner, nuno.sa, andy, sakari.ailus,
linux-input, linux-iio, linux-kernel
In-Reply-To: <20260524192059.1193716-1-sanjayembedded@gmail.com>
On Mon, 25 May 2026 00:50:23 +0530
Sanjay Chitroda <sanjayembeddedse@gmail.com> wrote:
> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
>
> Key highlights:
> - 0000-0024: General cleanup and kernel few coding style fixes across HID IIO drivers
> - 0025: Remove unused iio_dev argument from HID IIO sensor helper
> - 0026: Introduce devm_hid_sensor_setup_trigger() device-managed API
> - 0027-0036: Convert HID IIO sensor drivers to use the new devm-based trigger setup
>
> changes in v4:
> - Extend the series to cover remaining HID IIO drivers with devm API usage
> - Reorder patches to place cleanup and warning fix at beginning and,
> devm-related changes toward the end based on feedback from David
> - v3 series -> https://lore.kernel.org/all/20260509101040.791404-1-sanjayembedded@gmail.com/
> changes in v3:
> - Added cleanup and prepratory changes before adding devm_ API
> conversion based on self review: 0002, 0004, 0006, 0007 and 0008
> - Address andy's review comment on commit message and coding style
> - v2 series -> https://lore.kernel.org/all/20260429175918.2541914-1-sanjayembedded@gmail.com/
> changes in v2:
> - Following input from Jonathan and Andy, squash initial patch v1
> series in single change as individual change should not break anything
> - Add devm API support and two driver using the same
> - v1 series -> https://lore.kernel.org/all/20260428071613.1134053-1-sanjayembedded@gmail.com/
>
> Testing:
> - Compiled with W=1 for each patch in series
> - Build-tested on QEMU x86_64
>
> P.S.
> - Sashiko reported an issue in a different driver and noted that it is not
> introduced by this series. I have taken this feedback into account and
> will address the actual issue in a separate series focus on that driver.
> - Once this series is merged into the IIO tree, a number of HID IIO
> drivers will become available to fully converted to devm API usage.
> - The changes are organized across drivers to keep similar modifications
> grouped together for consistency, making the series easier to review,
> rather than grouping all changes per driver.
>
The series is mostly fine, though not sure what happened with how you sent
it as I have it in seperate threads of 10 patches at a time. If you are having
email troubles with big series, use b4 to send them instead (instructions on kernel.org)
My main concern is this is a lot of churn on a critical driver. I'd like therefore
to be able to see the full benefit of those devm patches rather than get us
part way as this does.
Jonathan
> Thanks,
> Sanjay Chitroda
>
> Sanjay Chitroda (36):
> iio: hid-sensors: add missing blank line after declarations
> iio: gyro: hid-sensor-gyro-3d: use u32 instead of unsigned
> iio: accel: hid-sensor-accel-3d: use u32 instead of unsigned
> iio: light: hid-sensor-als: use u32 instead of unsigned
> iio: light: hid-sensor-prox: use u32 instead of unsigned
> iio: orientation: hid-sensor-incl-3d: use u32 instead of unsigned
> iio: orientation: hid-sensor-rotation: use u32 instead of unsigned
> iio: pressure: hid-sensor-press: use u32 instead of unsigned
> iio: humidity: hid-sensor-humidity: align parenthesis for readability
> iio: gyro: hid-sensor-gyro-3d: align parenthesis for readability
> iio: magnetometer: hid-sensor-magn-3d: align parenthesis for
> readability
> iio: humidity: hid-sensor-humidity: use common device for devres
> iio: position: hid-sensor-custom-intel-hinge: use common device for
> devres
> iio: temperature: hid-sensor-temperature: use common device for devres
> iio: humidity: hid-sensor-humidity: use local struct device
> iio: gyro: hid-sensor-gyro-3d: use local struct device
> iio: accel: hid-sensor-accel-3d: use local struct device
> iio: light: hid-sensor-als: use local struct device
> iio: light: hid-sensor-prox: use local struct device
> iio: magnetometer: hid-sensor-magn-3d: use local struct device
> iio: orientation: hid-sensor-incl-3d: use local struct device
> iio: orientation: hid-sensor-rotation: use local struct device
> iio: position: hid-sensor-custom-intel-hinge: use local struct device
> iio: pressure: hid-sensor-press: use local struct device
> iio: hid-sensors: remove unused iio_dev argument
> iio: hid-sensors: introduce device managed API
> iio: gyro: hid-sensor-gyro-3d: drop hid_sensor_remove_trigger() using
> devm API
> iio: humidity: hid-sensor-humidity: drop hid_sensor_remove_trigger()
> using devm API
> iio: light: hid-sensor-prox: drop hid_sensor_remove_trigger() using
> devm API
> iio: light: hid-sensor-als: drop hid_sensor_remove_trigger() using
> devm API
> iio: magnetometer: hid-sensor-magn-3d: drop
> hid_sensor_remove_trigger() using devm API
> iio: orientation: hid-sensor-incl-3d: drop hid_sensor_remove_trigger()
> using devm API
> iio: orientation: hid-sensor-rotation: drop
> hid_sensor_remove_trigger() using devm API
> iio: position: hid-sensor-custom-intel-hinge: drop
> hid_sensor_remove_trigger() using devm API
> iio: pressure: hid-sensor-press: drop hid_sensor_remove_trigger()
> using devm API
> iio: temperature: hid-sensor-temperature: drop
> hid_sensor_remove_trigger() using devm API
>
> drivers/iio/accel/hid-sensor-accel-3d.c | 30 ++---
> .../common/hid-sensors/hid-sensor-trigger.c | 24 +++-
> .../common/hid-sensors/hid-sensor-trigger.h | 5 +-
> drivers/iio/gyro/hid-sensor-gyro-3d.c | 96 ++++++++-------
> drivers/iio/humidity/hid-sensor-humidity.c | 61 +++++-----
> drivers/iio/light/hid-sensor-als.c | 31 +++--
> drivers/iio/light/hid-sensor-prox.c | 30 ++---
> drivers/iio/magnetometer/hid-sensor-magn-3d.c | 112 +++++++++---------
> drivers/iio/orientation/hid-sensor-incl-3d.c | 36 +++---
> drivers/iio/orientation/hid-sensor-rotation.c | 38 +++---
> .../position/hid-sensor-custom-intel-hinge.c | 27 ++---
> drivers/iio/pressure/hid-sensor-press.c | 36 +++---
> .../iio/temperature/hid-sensor-temperature.c | 15 +--
> 13 files changed, 264 insertions(+), 277 deletions(-)
>
>
> base-commit: 08297ca8422541dde6c8b7e6b1d68bd4aa4568ef
^ permalink raw reply
* Re: [PATCH v4 35/36] iio: pressure: hid-sensor-press: drop hid_sensor_remove_trigger() using devm API
From: Jonathan Cameron @ 2026-05-26 15:54 UTC (permalink / raw)
To: Sanjay Chitroda
Cc: Christophe JAILLET, jikos, srinivas.pandruvada, dlechner, nuno.sa,
andy, sakari.ailus, linux-input, linux-iio, linux-kernel
In-Reply-To: <39A229F0-436E-4AAC-82A4-9B04733E9EE3@gmail.com>
On Mon, 25 May 2026 22:35:50 +0530
Sanjay Chitroda <sanjayembeddedse@gmail.com> wrote:
> On 25 May 2026 3:39:11 pm IST, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
> >Le 24/05/2026 à 21:29, Sanjay Chitroda a écrit :
> >> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> >>
> >> Use devm_hid_sensor_setup_trigger() to automatically release resource
> >> during fail, unbind or removal of driver using devres framework.
> >>
> >> This simplify the setup, remove goto, avoid manual resource cleanup in
> >> teardown path.
> >>
> >> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> >> ---
> >> drivers/iio/pressure/hid-sensor-press.c | 10 +++-------
> >> 1 file changed, 3 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/iio/pressure/hid-sensor-press.c b/drivers/iio/pressure/hid-sensor-press.c
> >> index fee7dcb86801..8438498c9aeb 100644
> >> --- a/drivers/iio/pressure/hid-sensor-press.c
> >> +++ b/drivers/iio/pressure/hid-sensor-press.c
> >> @@ -291,8 +291,8 @@ static int hid_press_probe(struct platform_device *pdev)
> >> atomic_set(&press_state->common_attributes.data_ready, 0);
> >> - ret = hid_sensor_setup_trigger(indio_dev, name,
> >> - &press_state->common_attributes);
> >> + ret = devm_hid_sensor_setup_trigger(dev, indio_dev, name,
> >> + &press_state->common_attributes);
> >> if (ret) {
> >> dev_err(dev, "trigger setup failed\n");
> >> return ret;
> >> @@ -301,7 +301,7 @@ static int hid_press_probe(struct platform_device *pdev)
> >> ret = iio_device_register(indio_dev);
> >
> >Hi,
> >
> >Looks like devm_iio_device_register() would now make sense and simplify things even further.
> >
> >CJ
>
> Hi CJ,
>
> Yes, as mentioned in cover letter most of HID IIO drivers have potential and will converted to devm based driver gradually.
This is putting a lot of churn and many patches on a critical driver
(given this device is in a lot of laptops). As such I think it's fine
to do prep for devm in separate changes, but the final per driver change
should be one per driver to go fully devm rather than doing it in smaller
chunks. I obviously want to see this well tested, and don't want to have
to keep asking people to test 'the next' set of changes.
Jonathan
>
> Welcoming all suggestions and especially if someone can validate and have Tested-by tag.
>
> Thank, Sanjay
>
> >
> >> if (ret) {
> >> dev_err(dev, "device register failed\n");
> >> - goto error_remove_trigger;
> >> + return ret;
> >> }
> >> press_state->callbacks.send_event = press_proc_event;
> >> @@ -318,8 +318,6 @@ static int hid_press_probe(struct platform_device *pdev)
> >> error_iio_unreg:
> >> iio_device_unregister(indio_dev);
> >> -error_remove_trigger:
> >> - hid_sensor_remove_trigger(&press_state->common_attributes);
> >> return ret;
> >> }
> >> @@ -328,11 +326,9 @@ static void hid_press_remove(struct platform_device *pdev)
> >> {
> >> struct hid_sensor_hub_device *hsdev = dev_get_platdata(&pdev->dev);
> >> struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> >> - struct press_state *press_state = iio_priv(indio_dev);
> >> sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_PRESSURE);
> >> iio_device_unregister(indio_dev);
> >> - hid_sensor_remove_trigger(&press_state->common_attributes);
> >> }
> >> static const struct platform_device_id hid_press_ids[] = {
> >
^ permalink raw reply
* [PATCH v2] HID: appletb-kbd: add option to switch default layer on double pressing fn key
From: Aditya Garg @ 2026-05-26 15:50 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Linux Input Mailing List, Linux Kernel Mailing List, Aditya Garg
In-Reply-To: <20260521132447.19241-1-gargaditya08@proton.me>
From: Aditya Garg <gargaditya08@proton.me>
This patch enables a user to switch the default layer from media to fn
keys and vice-versa upon double pressing the fn key. This behaviour can
be configured using the double_press_switch_time module parameter whose
value depicts the time in milliseconds within which the fn key must be
pressed again to switch the default layer. If set to 0, it simply
disables this behaviour.
Signed-off-by: Aditya Garg <gargaditya08@proton.me>
---
v2: added a check to ensure negative fn switch times are ignored.
drivers/hid/hid-appletb-kbd.c | 60 +++++++++++++++++++++++++++++++----
1 file changed, 53 insertions(+), 7 deletions(-)
diff --git a/drivers/hid/hid-appletb-kbd.c b/drivers/hid/hid-appletb-kbd.c
index 462010a75899..34bfc595949d 100644
--- a/drivers/hid/hid-appletb-kbd.c
+++ b/drivers/hid/hid-appletb-kbd.c
@@ -56,6 +56,12 @@ static int appletb_tb_idle_timeout = 15;
module_param_named(idle_timeout, appletb_tb_idle_timeout, int, 0644);
MODULE_PARM_DESC(idle_timeout, "Idle timeout in sec");
+static int appletb_tb_double_press_switch_layers;
+module_param_named(double_press_switch_time,
+ appletb_tb_double_press_switch_layers, int, 0644);
+MODULE_PARM_DESC(double_press_switch_time, "Time in ms within which if fn key is double "
+ "pressed will switch layers");
+
struct appletb_kbd {
struct hid_field *mode_field;
struct input_handler inp_handler;
@@ -68,6 +74,7 @@ struct appletb_kbd {
bool has_turned_off;
u8 saved_mode;
u8 current_mode;
+ unsigned long last_fn_press;
};
static const struct key_entry appletb_kbd_keymap[] = {
@@ -243,6 +250,18 @@ static int appletb_kbd_hid_event(struct hid_device *hdev, struct hid_field *fiel
return kbd->current_mode == APPLETB_KBD_MODE_OFF;
}
+static u8 appletb_switch_mode(u8 mode)
+{
+ switch (mode) {
+ case APPLETB_KBD_MODE_SPCL:
+ return APPLETB_KBD_MODE_FN;
+ case APPLETB_KBD_MODE_FN:
+ return APPLETB_KBD_MODE_SPCL;
+ default:
+ return mode;
+ }
+}
+
static void appletb_kbd_inp_event(struct input_handle *handle, unsigned int type,
unsigned int code, int value)
{
@@ -250,15 +269,42 @@ static void appletb_kbd_inp_event(struct input_handle *handle, unsigned int type
reset_inactivity_timer(kbd);
- if (type == EV_KEY && code == KEY_FN && appletb_tb_fn_toggle &&
- (kbd->current_mode == APPLETB_KBD_MODE_SPCL ||
- kbd->current_mode == APPLETB_KBD_MODE_FN)) {
+ if (type == EV_KEY && code == KEY_FN &&
+ (kbd->current_mode == APPLETB_KBD_MODE_SPCL ||
+ kbd->current_mode == APPLETB_KBD_MODE_FN)) {
+
if (value == 1) {
- kbd->saved_mode = kbd->current_mode;
- appletb_kbd_set_mode(kbd, kbd->current_mode == APPLETB_KBD_MODE_SPCL
- ? APPLETB_KBD_MODE_FN : APPLETB_KBD_MODE_SPCL);
+ if (appletb_tb_double_press_switch_layers > 0) {
+ unsigned long now = jiffies;
+
+ if (time_before(now, kbd->last_fn_press +
+ msecs_to_jiffies(appletb_tb_double_press_switch_layers))) {
+
+ appletb_tb_def_mode =
+ appletb_switch_mode(
+ appletb_tb_def_mode);
+
+ appletb_kbd_set_mode(kbd,
+ appletb_tb_def_mode);
+
+ kbd->saved_mode = appletb_tb_def_mode;
+ kbd->last_fn_press = 0;
+
+ return;
+ }
+
+ kbd->last_fn_press = now;
+ }
+ if (appletb_tb_fn_toggle) {
+ kbd->saved_mode = kbd->current_mode;
+
+ appletb_kbd_set_mode(kbd,
+ appletb_switch_mode(kbd->current_mode));
+ }
+
} else if (value == 0) {
- if (kbd->saved_mode != kbd->current_mode)
+ if (appletb_tb_fn_toggle &&
+ kbd->saved_mode != kbd->current_mode)
appletb_kbd_set_mode(kbd, kbd->saved_mode);
}
}
Range-diff against v1:
1: 5c9d8c8ba8c0 ! 1: 4f6a818b98b9 HID: appletb-kbd: add option to switch default layer on double pressing fn key
@@ drivers/hid/hid-appletb-kbd.c: static void appletb_kbd_inp_event(struct input_ha
- kbd->saved_mode = kbd->current_mode;
- appletb_kbd_set_mode(kbd, kbd->current_mode == APPLETB_KBD_MODE_SPCL
- ? APPLETB_KBD_MODE_FN : APPLETB_KBD_MODE_SPCL);
-+ if (appletb_tb_double_press_switch_layers) {
++ if (appletb_tb_double_press_switch_layers > 0) {
+ unsigned long now = jiffies;
+
+ if (time_before(now, kbd->last_fn_press +
--
2.53.0
^ permalink raw reply related
* Re: [PATCH v4 02/36] iio: gyro: hid-sensor-gyro-3d: use u32 instead of unsigned
From: Jonathan Cameron @ 2026-05-26 15:45 UTC (permalink / raw)
To: Sanjay Chitroda
Cc: jikos, srinivas.pandruvada, dlechner, nuno.sa, andy, sakari.ailus,
linux-input, linux-iio, linux-kernel
In-Reply-To: <20260524192059.1193716-3-sanjayembedded@gmail.com>
On Mon, 25 May 2026 00:50:25 +0530
Sanjay Chitroda <sanjayembeddedse@gmail.com> wrote:
> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
>
> Prefer 'u32' instead of bare 'unsigned' variable to improve code
> clarity and consistency with kernel style.
I think it is relevant that the definition of the callbacks is:
struct hid_sensor_hub_callbacks {
...
int (*capture_sample)(struct hid_sensor_hub_device *hsdev,
u32 usage_id, size_t raw_len, char *raw_data,
void *priv);
int (*send_event)(struct hid_sensor_hub_device *hsdev, u32 usage_id,
void *priv);
};
So in all cases we have a u32. Same applies to other calls the usage_id is passed to.
>
> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> ---
> Changes in v4:
> - Use 'u32' instead of 'unsigned int' as driver is already uses that and respective prototype has same data type with input from David and Andy
> - Link to v3 -> https://lore.kernel.org/all/20260509101040.791404-5-sanjayembedded@gmail.com/
> ---
> drivers/iio/gyro/hid-sensor-gyro-3d.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/gyro/hid-sensor-gyro-3d.c b/drivers/iio/gyro/hid-sensor-gyro-3d.c
> index c340cc899a7c..020812639947 100644
> --- a/drivers/iio/gyro/hid-sensor-gyro-3d.c
> +++ b/drivers/iio/gyro/hid-sensor-gyro-3d.c
> @@ -187,7 +187,7 @@ static const struct iio_info gyro_3d_info = {
>
> /* Callback handler to send event after all samples are received and captured */
> static int gyro_3d_proc_event(struct hid_sensor_hub_device *hsdev,
> - unsigned usage_id,
> + u32 usage_id,
> void *priv)
> {
> struct iio_dev *indio_dev = platform_get_drvdata(priv);
> @@ -209,7 +209,7 @@ static int gyro_3d_proc_event(struct hid_sensor_hub_device *hsdev,
>
> /* Capture samples in local storage */
> static int gyro_3d_capture_sample(struct hid_sensor_hub_device *hsdev,
> - unsigned usage_id,
> + u32 usage_id,
> size_t raw_len, char *raw_data,
> void *priv)
> {
> @@ -244,7 +244,7 @@ static int gyro_3d_capture_sample(struct hid_sensor_hub_device *hsdev,
> static int gyro_3d_parse_report(struct platform_device *pdev,
> struct hid_sensor_hub_device *hsdev,
> struct iio_chan_spec *channels,
> - unsigned usage_id,
> + u32 usage_id,
> struct gyro_3d_state *st)
> {
> int ret;
^ permalink raw reply
* [old REGRESSION] brightness keys on HP Elitebook 840 G3
From: Marijn Schouten @ 2026-05-26 12:29 UTC (permalink / raw)
To: linux-input
LS (not subscribed please CC)
The last kernel version that the brightness keys on my HP Elitebook
840 G3 worked on was 4.19.208, and they did not work from 5.4.150 on.
I was not able to test any intermediate versions.
I'm currently on 5.15.208. I've been using sysfs as a workaround:
"echo 700 > /sys/class/backlight/intel_backlight/brightness".
I get this failing dmesg output when pressing brightness down and brightness up:
$ dmesg |tail -n 8
[ 682.502592] atkbd serio0: Unknown key pressed (translated set 2,
code 0x92 on isa0060/serio0).
[ 682.502614] atkbd serio0: Use 'setkeycodes e012 <keycode>' to make it known.
[ 682.584414] atkbd serio0: Unknown key released (translated set 2,
code 0x92 on isa0060/serio0).
[ 682.584435] atkbd serio0: Use 'setkeycodes e012 <keycode>' to make it known.
[ 683.334480] atkbd serio0: Unknown key pressed (translated set 2,
code 0x97 on isa0060/serio0).
[ 683.334501] atkbd serio0: Use 'setkeycodes e017 <keycode>' to make it known.
[ 683.449715] atkbd serio0: Unknown key released (translated set 2,
code 0x97 on isa0060/serio0).
[ 683.449737] atkbd serio0: Use 'setkeycodes e017 <keycode>' to make it known.
Should this be fixed in drivers/input/keyboard/atkbd.c? I tried:
--- drivers/input/keyboard/atkbd.c.orig 2026-05-26 13:34:12.275821222 +0200
+++ drivers/input/keyboard/atkbd.c 2026-05-26 13:40:52.309840298 +0200
@@ -1162,6 +1162,12 @@
atkbd->keycode[scancode] = KEY_HANJA;
__set_bit(scancode, atkbd->force_release_mask);
+ /* HP brightness keys mapping for translated mode */
+ if (atkbd->translated) {
+ atkbd->keycode[0x112] = KEY_BRIGHTNESSDOWN;
+ atkbd->keycode[0x117] = KEY_BRIGHTNESSUP;
+ }
+
/*
* Perform additional fixups
*/
but it did not work...
With kind regards,
Marijn (not subscribed please CC)
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: hid-over-i2c: Add reset-gpios property
From: Hans de Goede @ 2026-05-26 8:55 UTC (permalink / raw)
To: Pradyot Kumar Nayak, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Abel Vesa, Dmitry Baryshkov,
Benjamin Tissoires, Jiri Kosina
Cc: linux-arm-msm, linux-input, Konrad Dybcio, devicetree,
linux-kernel,
20260519-glymur-mahua-common-nodes-v2-1-9f1b89a3f398
In-Reply-To: <20260523-arm64-dts-glymur-crd-add-reset-gpio-to-touchscreen-v2-1-5c333051e5bb@oss.qualcomm.com>
Hi,
On 22-May-26 21:55, Pradyot Kumar Nayak wrote:
> Document reset-gpios property for devices that explicitly require
> the reset line to be de-asserted during power up.
>
> Signed-off-by: Pradyot Kumar Nayak <pradyot.nayak@oss.qualcomm.com>
> ---
> Documentation/devicetree/bindings/input/hid-over-i2c.yaml | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
> index 138caad96a29..fe0ec4fce673 100644
> --- a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
> +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
> @@ -50,6 +50,10 @@ properties:
> description: Time required by the device after enabling its regulators
> or powering it on, before it is ready for communication.
>
> + reset-gpios:
> + maxItems: 1
> + description: GPIO line used to reset the device.
> +
For I2C HID devices which need a reset, we typically also
need reset timings, trying to do I2C transfers to the device directly
after the reset has been de-asserted typically will not work and
some touchscreen-model specific delay is necessary.
So far in the DT world this has been handled by touchscreen
vendor specific bindings:
Documentation/devicetree/bindings/input/elan,ekth6915.yaml
Documentation/devicetree/bindings/input/focaltech,ft8112.yaml
Documentation/devicetree/bindings/input/ilitek,ili9882t.yaml
Which are all 3 handled by:
drivers/hid/i2c-hid/i2c-hid-of-elan.c
Which has per compatible match-data with the reset timings.
You should probably extend drivers/hid/i2c-hid/i2c-hid-of-elan.c
with support for the CRD touchscreen and either add a new
compatible to one of the existing 3 bindings; or if the touchscreen
controller IC is from a vendor without a binding yet maybe add
a new bindings file?
There also is:
Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
which is handled by:
drivers/hid/i2c-hid/i2c-hid-of-goodix.c
and is special in its own way.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: hid-over-i2c: Add reset-gpios property
From: Dmitry Baryshkov @ 2026-05-25 19:22 UTC (permalink / raw)
To: Pradyot Kumar Nayak
Cc: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Abel Vesa, Benjamin Tissoires, Jiri Kosina,
linux-arm-msm, linux-input, Konrad Dybcio, devicetree,
linux-kernel,
20260519-glymur-mahua-common-nodes-v2-1-9f1b89a3f398
In-Reply-To: <20260523-arm64-dts-glymur-crd-add-reset-gpio-to-touchscreen-v2-1-5c333051e5bb@oss.qualcomm.com>
On Sat, May 23, 2026 at 01:25:37AM +0530, Pradyot Kumar Nayak wrote:
> Document reset-gpios property for devices that explicitly require
> the reset line to be de-asserted during power up.
>
> Signed-off-by: Pradyot Kumar Nayak <pradyot.nayak@oss.qualcomm.com>
> ---
> Documentation/devicetree/bindings/input/hid-over-i2c.yaml | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
> index 138caad96a29..fe0ec4fce673 100644
> --- a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
> +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
> @@ -50,6 +50,10 @@ properties:
> description: Time required by the device after enabling its regulators
> or powering it on, before it is ready for communication.
>
> + reset-gpios:
> + maxItems: 1
> + description: GPIO line used to reset the device.
> +
If you are extending the schema, where are the driver changes?
> touchscreen-inverted-x: true
>
> touchscreen-inverted-y: true
>
> --
> 2.43.0
>
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH] HID: logitech-hidpp: sync wheel multiplier on wheel mode changes
From: Lauri Saurus @ 2026-05-25 18:29 UTC (permalink / raw)
To: sashiko-reviews; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260518201000.BBD71C2BCB7@smtp.kernel.org>
> > +static int hidpp20_hires_wheel_raw_event(struct hidpp_device *hidpp,
> > + u8 *data, int size)
> > +{
> > + if (hidpp->hires_wheel_feature_index == 0xff)
> > + return 0;
> > +
> > + if (size < 5)
> > + return 0;
> > +
> > + if (data[0] != REPORT_ID_HIDPP_LONG ||
> > + data[2] != hidpp->hires_wheel_feature_index)
> > + return 0;
>
> Does this code need to handle VERY_LONG reports as well?
>
> Userspace tools or newer devices might transmit FAP commands using
> REPORT_ID_HIDPP_VERY_LONG (0x12). If the request is a VERY_LONG report, the
> response will also be VERY_LONG, and this check would drop it, failing to
> update the multiplier.
I agree, it could be possible some hosts could send the command as VERY_LONG report for newer devices which support that. I will change this to support VERY_LONG reports also.
>
> > +
> > + if ((data[3] & 0xf0) == CMD_HIRES_WHEEL_SET_WHEEL_MODE) {
> > + u8 mode = data[4];
> > + bool hires = (mode & 0x02) != 0;
>
> Does the HID++ 2.0 protocol guarantee that SET command responses will echo
> the requested mode in the payload?
>
> In the FAP protocol, responses to SET commands often return an empty
> (zero-filled) payload. If the device returns a zero payload, data[4] would
> be 0x00. The code would then evaluate hires = false and forcefully set the
> multiplier to 1, even if high-resolution mode was successfully requested.
As per the "Protocol HID++2.0 essential features" section in https://lekensteyn.nl/files/logitech/logitech_hidpp_2.0_specification_draft_2012-06-04.pdf:
"
All functions must respect the following rule:
All parameters in a request must always be repeated in the response:
Any parameter that is fully supported must be repeated "as is".
Any parameter that is only "partially supported", must be returned as supported.
The following examples should help to better understand these requirement:
A read command where an address (i.e., memory address, register address, etc.) is given as parameter must return the address and the data in the response (and not just the address).
A write command which sets a collection of bits or bit fields, should return the same value where all unsupported bits and bit fields have been masked to their default values (usually 0).
A command such as open-lock, erase-memory, etc. should return its parameters unchanged.
Note that there is no requirement to implement partial support. Each feature designer is free to decide if "partially correct" parameters
should return and error or be "partially supported." However, if partial support is implemented, then the parameters must be returned
as supported.
"
Based on the aforementioned specification, all parameters must be echoed in the response. If the SetWheelMode would not support setting hires parameter (partial support), then hires parameter would be returned as default value 0 (low res) in the response. In that case, it is correct to set the multiplier to 1, since setting hires was not supported.
Unless there are verified cases of HID++ capable mouses behaving as described in the review comment, or knowledge of newer version of the specification to have changed aforementioned rule, I would assume all devices to work according to the specification.
>
> > + int new_multiplier = (hires && hidpp->hires_wheel_multiplier > 0)
> > + ? hidpp->hires_wheel_multiplier : 1;
> > + hidpp->vertical_wheel_counter.wheel_multiplier = new_multiplier;
>
> Is there a potential data race on wheel_multiplier and
> hires_wheel_multiplier here?
>
> Looking at hi_res_scroll_enable(), it writes to these variables from
> process context via the workqueue. Concurrently, this event handler reads
> and writes to them from the HID raw event delivery path in interrupt context.
> There does not appear to be any synchronization protecting these shared
> variables across contexts.
>
> > + return 1;
> > + }
> > +
> > + return 0;
> > +}
>
I agree that in theory there is a possible race condition. However, wheel_multiplier was already written to from hi_res_scroll_enable, and read from the HID event handler before this patch. Not sure if this patch should address this issue or not.
^ permalink raw reply
* Re: [PATCH v4 35/36] iio: pressure: hid-sensor-press: drop hid_sensor_remove_trigger() using devm API
From: Sanjay Chitroda @ 2026-05-25 17:05 UTC (permalink / raw)
To: Christophe JAILLET, jikos, jic23, srinivas.pandruvada
Cc: dlechner, nuno.sa, andy, sakari.ailus, linux-input, linux-iio,
linux-kernel
In-Reply-To: <0a8ebbd2-5de6-48d2-9218-f7b816eb3455@wanadoo.fr>
On 25 May 2026 3:39:11 pm IST, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
>Le 24/05/2026 à 21:29, Sanjay Chitroda a écrit :
>> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
>>
>> Use devm_hid_sensor_setup_trigger() to automatically release resource
>> during fail, unbind or removal of driver using devres framework.
>>
>> This simplify the setup, remove goto, avoid manual resource cleanup in
>> teardown path.
>>
>> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
>> ---
>> drivers/iio/pressure/hid-sensor-press.c | 10 +++-------
>> 1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/iio/pressure/hid-sensor-press.c b/drivers/iio/pressure/hid-sensor-press.c
>> index fee7dcb86801..8438498c9aeb 100644
>> --- a/drivers/iio/pressure/hid-sensor-press.c
>> +++ b/drivers/iio/pressure/hid-sensor-press.c
>> @@ -291,8 +291,8 @@ static int hid_press_probe(struct platform_device *pdev)
>> atomic_set(&press_state->common_attributes.data_ready, 0);
>> - ret = hid_sensor_setup_trigger(indio_dev, name,
>> - &press_state->common_attributes);
>> + ret = devm_hid_sensor_setup_trigger(dev, indio_dev, name,
>> + &press_state->common_attributes);
>> if (ret) {
>> dev_err(dev, "trigger setup failed\n");
>> return ret;
>> @@ -301,7 +301,7 @@ static int hid_press_probe(struct platform_device *pdev)
>> ret = iio_device_register(indio_dev);
>
>Hi,
>
>Looks like devm_iio_device_register() would now make sense and simplify things even further.
>
>CJ
Hi CJ,
Yes, as mentioned in cover letter most of HID IIO drivers have potential and will converted to devm based driver gradually.
Welcoming all suggestions and especially if someone can validate and have Tested-by tag.
Thank, Sanjay
>
>> if (ret) {
>> dev_err(dev, "device register failed\n");
>> - goto error_remove_trigger;
>> + return ret;
>> }
>> press_state->callbacks.send_event = press_proc_event;
>> @@ -318,8 +318,6 @@ static int hid_press_probe(struct platform_device *pdev)
>> error_iio_unreg:
>> iio_device_unregister(indio_dev);
>> -error_remove_trigger:
>> - hid_sensor_remove_trigger(&press_state->common_attributes);
>> return ret;
>> }
>> @@ -328,11 +326,9 @@ static void hid_press_remove(struct platform_device *pdev)
>> {
>> struct hid_sensor_hub_device *hsdev = dev_get_platdata(&pdev->dev);
>> struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> - struct press_state *press_state = iio_priv(indio_dev);
>> sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_PRESSURE);
>> iio_device_unregister(indio_dev);
>> - hid_sensor_remove_trigger(&press_state->common_attributes);
>> }
>> static const struct platform_device_id hid_press_ids[] = {
>
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: hid-over-i2c: Add reset-gpios property
From: Conor Dooley @ 2026-05-25 16:24 UTC (permalink / raw)
To: Pradyot Kumar Nayak
Cc: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Abel Vesa, Dmitry Baryshkov, Benjamin Tissoires,
Jiri Kosina, linux-arm-msm, linux-input, Konrad Dybcio,
devicetree, linux-kernel,
20260519-glymur-mahua-common-nodes-v2-1-9f1b89a3f398
In-Reply-To: <20260523-arm64-dts-glymur-crd-add-reset-gpio-to-touchscreen-v2-1-5c333051e5bb@oss.qualcomm.com>
[-- Attachment #1: Type: text/plain, Size: 1289 bytes --]
On Sat, May 23, 2026 at 01:25:37AM +0530, Pradyot Kumar Nayak wrote:
> Document reset-gpios property for devices that explicitly require
> the reset line to be de-asserted during power up.
If there are devices that require this, it seems short-sighted to not
add their compatibles to hid-over-i2c.yaml and make the property
required for them.
pw-bot: changes-requested
Cheers,
Conor.
>
> Signed-off-by: Pradyot Kumar Nayak <pradyot.nayak@oss.qualcomm.com>
> ---
> Documentation/devicetree/bindings/input/hid-over-i2c.yaml | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
> index 138caad96a29..fe0ec4fce673 100644
> --- a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
> +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
> @@ -50,6 +50,10 @@ properties:
> description: Time required by the device after enabling its regulators
> or powering it on, before it is ready for communication.
>
> + reset-gpios:
> + maxItems: 1
> + description: GPIO line used to reset the device.
> +
> touchscreen-inverted-x: true
>
> touchscreen-inverted-y: true
>
> --
> 2.43.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] Input: ims-pcu - fix use-after-free in probe error path
From: sashiko-bot @ 2026-05-25 16:06 UTC (permalink / raw)
To: Zhian Liang; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260525151410.42750-1-liangzhan5dev@gmail.com>
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues:
- [Low] The patch introduces a useless `goto` statement immediately preceding its target label.
Pre-existing issues:
- [High] A malicious USB device can trigger a Double Free and Use-After-Free in the probe error path by spoofing the data interface class.
- [High] The driver blindly trusts the CDC Union descriptor's `bMasterInterface0` field, allowing a malicious device to corrupt other drivers' drvdata.
- [High] An infinite loop can occur in `ims_pcu_get_cdc_union_desc` if a USB descriptor has a `bLength` of 0.
- [High] The driver frees the `urb_in_buf` DMA coherent buffer using an incorrect size parameter.
- [High] The driver assumes the first endpoint of the control interface is an Interrupt IN endpoint without verifying its type or direction.
--
commit a298fe95c1b14555659ce23349aaf55e3bb035b5
Author: Zhian Liang <liangzhan5dev@gmail.com>
Input: ims-pcu - fix use-after-free in probe error path
This commit addresses a use-after-free in the probe error path by clearing
the control interface data to NULL before freeing the pcu structure. This
prevents the disconnect handler from accessing the freed pointer if the
device is removed.
> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 4c022a36dbe84..fce3232ebf072 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -2063,6 +2063,10 @@ static int ims_pcu_probe(struct usb_interface *intf,
> ims_pcu_buffers_free(pcu);
[Severity: High]
This is a pre-existing issue, but does this function properly free the
urb_in_buf?
Looking at ims_pcu_buffers_free(), it seems to use max_out_size instead of
max_in_size:
ims_pcu_buffers_free() {
...
usb_free_coherent(pcu->udev, pcu->max_out_size,
pcu->urb_in_buf, pcu->read_dma);
...
}
Could a device with mismatched IN and OUT endpoint descriptors cause an
improper free size and corrupt the DMA allocator?
> err_unclaim_intf:
> usb_driver_release_interface(&ims_pcu_driver, pcu->data_intf);
[Severity: High]
This is a pre-existing issue, but can a device spoofing the data interface
class trigger a double free here?
If a device presents a data interface with bInterfaceClass == USB_CLASS_COMM,
ims_pcu_disconnect() might fail to skip it. When
usb_driver_release_interface() is called on the data interface during a
probe failure, it synchronously unbinds the interface, triggering
ims_pcu_disconnect().
Could ims_pcu_disconnect() then incorrectly assume it is handling the
control interface and free the pcu structure, leading to a use-after-free
read on pcu->ctrl_intf and a double free on pcu in this error path?
> + goto err_clear_intfdata;
[Severity: Low]
Is this goto statement necessary? Execution would naturally fall through to
the err_clear_intfdata label on the very next line, making this instruction
redundant.
> +err_clear_intfdata:
> + if (pcu->ctrl_intf)
> + usb_set_intfdata(pcu->ctrl_intf, NULL);
[Severity: High]
This is a pre-existing issue, but does the driver safely assign
pcu->ctrl_intf?
In ims_pcu_parse_cdc_data(), pcu->ctrl_intf is assigned using the
bMasterInterface0 field from the CDC Union descriptor. If a device sets
bMasterInterface0 to point to a different, already-bound interface, could
the probe function unconditionally overwrite that victim interface's drvdata?
If the probe subsequently fails, this error path would clear the victim's
drvdata to NULL, which might cause the victim driver to crash when
disconnected.
> err_free_mem:
> kfree(pcu);
> return error;
[Severity: High]
This is a pre-existing issue, but could there be an infinite loop during
descriptor parsing?
If a device provides a CDC union descriptor with a bLength of 0, the size
check in ims_pcu_get_cdc_union_desc() would pass since 0 is not greater
than the remaining length. The loop would then subtract 0 from buflen and
add 0 to buf, causing it to never advance or terminate. Would this hang
the USB enumeration thread?
[Severity: High]
This is a pre-existing issue, but does ims_pcu_parse_cdc_data() verify the
endpoint direction and type before using it?
The code assigns pcu->ep_ctrl to &alt->endpoint[0].desc and passes its
address to usb_fill_int_urb(). If a device exposes a Bulk or OUT endpoint
at this index instead, could submitting it as an Interrupt IN URB lead to
unhandled behavior or warnings in host controller drivers?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260525151410.42750-1-liangzhan5dev@gmail.com?part=1
^ permalink raw reply
* [PATCH] Input: ims-pcu - fix use-after-free in probe error path
From: Zhian Liang @ 2026-05-25 15:14 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, stable, Zhian Liang
If the driver fails during init (e.g. in ims_pcu_init_application_mode),the error path frees the pcu struct without clearing the interface data.
If the device is disconnected while in this state, the disconnect handler will retrieve the stale pointer from
usb_get_intfdata() and trigger a use-after-free
Fix this by setting the interface data to NULL in the probe before freeing the pcu struct.
Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
Cc: <stable@vger.kernel.org>
Signed-off-by: Zhian Liang <liangzhan5dev@gmail.com>
---
drivers/input/misc/ims-pcu.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 4c022a36dbe8..fce3232ebf07 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -2063,6 +2063,10 @@ static int ims_pcu_probe(struct usb_interface *intf,
ims_pcu_buffers_free(pcu);
err_unclaim_intf:
usb_driver_release_interface(&ims_pcu_driver, pcu->data_intf);
+ goto err_clear_intfdata;
+err_clear_intfdata:
+ if (pcu->ctrl_intf)
+ usb_set_intfdata(pcu->ctrl_intf, NULL);
err_free_mem:
kfree(pcu);
return error;
--
2.34.1
^ permalink raw reply related
* Re: [syzbot] [fs?] [usb?] [input?] INFO: rcu detected stall in __fsnotify_parent (3)
From: Jan Kara @ 2026-05-25 14:47 UTC (permalink / raw)
To: syzbot
Cc: amir73il, jack, linux-fsdevel, linux-input, linux-kernel,
linux-usb, syzkaller-bugs
In-Reply-To: <6a0cce0d.170a0220.3cbd30.0138.GAE@google.com>
Hello!
On Tue 19-05-26 13:54:37, syzbot wrote:
> syzbot found the following issue on:
>
> HEAD commit: 6916d5703ddf Merge tag 'drm-fixes-2026-05-16' of https://g..
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=13437cc8580000
> kernel config: https://syzkaller.appspot.com/x/.config?x=59da38148f3a3d24
> dashboard link: https://syzkaller.appspot.com/bug?extid=27ce6cc06c1311c152f5
> compiler: gcc (Debian 14.2.0-19) 14.2.0, GNU ld (GNU Binutils for Debian) 2.44
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=123e8596580000
>
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/637181391a5c/disk-6916d570.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/bfbefc8a3671/vmlinux-6916d570.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/a419831b32a6/bzImage-6916d570.xz
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+27ce6cc06c1311c152f5@syzkaller.appspotmail.com
>
> rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
> rcu: Tasks blocked on level-0 rcu_node (CPUs 0-1): P4963/1:b..l
>
> rcu: (detected by 1, t=10506 jiffies, g=15565, q=1900 ncpus=2)
> task:acpid state:R
> running task stack:22856 pid:4963 tgid:4963 ppid:1 task_flags:0x400040 flags:0x00080001
FWIW all the reproducer does is some games with USB, no signs of fsnotify.
Bisection has failed, the problem was hit only once more than a week ago.
I'm going to sign this off to some random fluctuation.
Honza
> Call Trace:
> <TASK>
> context_switch kernel/sched/core.c:5388 [inline]
> __schedule+0x1295/0x67a0 kernel/sched/core.c:7189
> preempt_schedule_irq+0x50/0x90 kernel/sched/core.c:7513
> irqentry_exit_to_kernel_mode_preempt include/linux/irq-entry-common.h:476 [inline]
> irqentry_exit_to_kernel_mode include/linux/irq-entry-common.h:547 [inline]
> irqentry_exit+0x205/0x7e0 kernel/entry/common.c:164
> asm_sysvec_apic_timer_interrupt+0x1a/0x20 arch/x86/include/asm/idtentry.h:697
> RIP: 0010:lock_acquire+0x5e/0x370 kernel/locking/lockdep.c:5872
> Code: 05 3b a2 26 12 83 f8 07 0f 87 d9 02 00 00 48 0f a3 05 46 41 f3 0e 0f 82 a4 02 00 00 8b 35 ce 74 f3 0e 85 f6 0f 85 bf 00 00 00 <48> 8b 44 24 30 65 48 2b 05 dd a1 26 12 0f 85 ed 02 00 00 48 83 c4
> RSP: 0018:ffffc9000342fb30 EFLAGS: 00000206
>
> RAX: 0000000000000046 RBX: 0000000000000000 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: ffffffff8defcf06 RDI: ffffffff8c1c3a00
> RBP: ffffffff8e7e57a0 R08: 0000000086db7919 R09: 0000000000000007
> R10: 0000000000000200 R11: 0000000000000000 R12: 0000000000000002
> R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> rcu_lock_acquire include/linux/rcupdate.h:300 [inline]
> rcu_read_lock include/linux/rcupdate.h:838 [inline]
> dput.part.0+0x4c/0x570 fs/dcache.c:971
> dput+0x1f/0x30 fs/dcache.c:968
> __fsnotify_parent+0x694/0xca0 fs/notify/fsnotify.c:242
> fsnotify_parent include/linux/fsnotify.h:96 [inline]
> fsnotify_path include/linux/fsnotify.h:113 [inline]
> fsnotify_file include/linux/fsnotify.h:127 [inline]
> fsnotify_file include/linux/fsnotify.h:116 [inline]
> fsnotify_access include/linux/fsnotify.h:425 [inline]
> vfs_read+0x486/0xb30 fs/read_write.c:578
> ksys_read+0x1f8/0x250 fs/read_write.c:717
> do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
> do_syscall_64+0x10b/0xf80 arch/x86/entry/syscall_64.c:94
> entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7feac7fb3407
> RSP: 002b:00007ffc6dd51e00 EFLAGS: 00000202
> ORIG_RAX: 0000000000000000
> RAX: ffffffffffffffda RBX: 00007feac7f29780 RCX: 00007feac7fb3407
> RDX: 0000000000000018 RSI: 00007ffc6dd51e50 RDI: 000000000000000b
> RBP: 00007ffc6dd51e50 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000202 R12: 0000556e2f8031e4
> R13: 0000556e4a9f2380 R14: 0000000000000007 R15: 000000000000000b
> </TASK>
> rcu: rcu_preempt kthread starved for 4298 jiffies! g15565 f0x0 RCU_GP_WAIT_FQS(5) ->state=0x0 ->cpu=1
> rcu: Unless rcu_preempt kthread gets sufficient CPU time, OOM is now expected behavior.
> rcu: RCU grace-period kthread stack dump:
> task:rcu_preempt state:R
> running task stack:27992 pid:16 tgid:16 ppid:2 task_flags:0x208040 flags:0x00080000
> Call Trace:
> <TASK>
> context_switch kernel/sched/core.c:5388 [inline]
> __schedule+0x1295/0x67a0 kernel/sched/core.c:7189
> __schedule_loop kernel/sched/core.c:7268 [inline]
> schedule+0xdd/0x390 kernel/sched/core.c:7283
> schedule_timeout+0x127/0x280 kernel/time/sleep_timeout.c:99
> rcu_gp_fqs_loop+0x1a9/0x900 kernel/rcu/tree.c:2095
> rcu_gp_kthread+0x179/0x230 kernel/rcu/tree.c:2297
> kthread+0x370/0x450 kernel/kthread.c:436
> ret_from_fork+0x72b/0xd50 arch/x86/kernel/process.c:158
> ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
> </TASK>
> rcu: Stack dump where RCU GP kthread last ran:
> CPU: 1 UID: 0 PID: 5978 Comm: syz.1.18 Not tainted syzkaller #0 PREEMPT(full)
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/18/2026
> RIP: 0010:write_comp_data+0x40/0x90 kernel/kcov.c:246
> Code: 02 12 a9 00 01 ff 00 74 1b f6 c4 01 74 07 a9 00 00 ff 00 74 05 c3 cc cc cc cc 8b 87 d4 16 00 00 85 c0 74 f1 8b 87 b0 16 00 00 <83> f8 03 75 e6 48 8b 87 b8 16 00 00 8b bf b4 16 00 00 48 8b 30 48
> RSP: 0018:ffffc900024af648 EFLAGS: 00000246
>
> RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffffff81fc2bcd
> RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff88805a5e0000
> RBP: ffff8880b8443320 R08: 0000000000000005 R09: 0000000000000000
> R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000003
> R13: ffffed1017088665 R14: 0000000000000001 R15: ffff8880b853c5c0
> FS: 0000000000000000(0000) GS:ffff88812446d000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f8c6e1b56e8 CR3: 0000000059a89000 CR4: 00000000003526f0
> Call Trace:
> <TASK>
> csd_lock_wait kernel/smp.c:342 [inline]
> smp_call_function_many_cond+0x5ad/0x1700 kernel/smp.c:892
> on_each_cpu_cond_mask+0x40/0x90 kernel/smp.c:1057
> __flush_tlb_multi arch/x86/include/asm/paravirt.h:46 [inline]
> flush_tlb_multi arch/x86/mm/tlb.c:1361 [inline]
> flush_tlb_mm_range+0x45f/0x16f0 arch/x86/mm/tlb.c:1451
> tlb_flush arch/x86/include/asm/tlb.h:23 [inline]
> tlb_flush_mmu_tlbonly include/asm-generic/tlb.h:509 [inline]
> tlb_flush_mmu_tlbonly include/asm-generic/tlb.h:499 [inline]
> tlb_flush_mmu mm/mmu_gather.c:423 [inline]
> tlb_finish_mmu+0x3fe/0x810 mm/mmu_gather.c:549
> exit_mmap+0x454/0xa10 mm/mmap.c:1313
> __mmput+0x12a/0x410 kernel/fork.c:1178
> mmput+0x67/0x80 kernel/fork.c:1201
> exit_mm kernel/exit.c:582 [inline]
> do_exit+0x8b2/0x2af0 kernel/exit.c:964
> do_group_exit+0xd5/0x2a0 kernel/exit.c:1119
> get_signal+0x20ff/0x2210 kernel/signal.c:3037
> arch_do_signal_or_restart+0x91/0x7e0 arch/x86/kernel/signal.c:337
> __exit_to_user_mode_loop kernel/entry/common.c:64 [inline]
> exit_to_user_mode_loop+0x8b/0x4f0 kernel/entry/common.c:98
> __exit_to_user_mode_prepare include/linux/irq-entry-common.h:207 [inline]
> syscall_exit_to_user_mode_prepare include/linux/irq-entry-common.h:238 [inline]
> syscall_exit_to_user_mode include/linux/entry-common.h:318 [inline]
> do_syscall_64+0x706/0xf80 arch/x86/entry/syscall_64.c:100
> entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7f7a9eb5d68e
> Code: Unable to access opcode bytes at 0x7f7a9eb5d664.
> RSP: 002b:00007fffdcda5708 EFLAGS: 00000246
> ORIG_RAX: 00000000000000e6
> RAX: 0000000000000000 RBX: 000055558fc36500 RCX: 00007f7a9eb5d68e
> RDX: 00007fffdcda5760 RSI: 0000000000000000 RDI: 0000000000000000
> RBP: 0000000000000001 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 00007fffdcda58b0
> R13: 00007f7a9ee15fac R14: 000000000002bbe6 R15: 00007f7a9ee15fa0
> </TASK>
>
>
> ---
> 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.
>
> syzbot will keep track of this issue. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
>
> If the report is already addressed, let syzbot know by replying with:
> #syz fix: exact-commit-title
>
> If you want syzbot to run the reproducer, reply with:
> #syz test: git://repo/address.git branch-or-commit-hash
> If you attach or paste a git patch, syzbot will apply it before testing.
>
> If you want to overwrite report's subsystems, reply with:
> #syz set subsystems: new-subsystem
> (See the list of subsystem names on the web dashboard)
>
> If the report is a duplicate of another one, reply with:
> #syz dup: exact-subject-of-another-report
>
> If you want to undo deduplication, reply with:
> #syz undup
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH v5 02/11] dt-bindings: hwmon: Add Apple System Management Controller hwmon schema
From: Guenter Roeck @ 2026-05-25 13:33 UTC (permalink / raw)
To: James Calligeros
Cc: Sven Peter, Janne Grunau, Alyssa Rosenzweig, Neal Gompa,
Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Alexandre Belloni, Jean Delvare, Dmitry Torokhov, Jonathan Corbet,
asahi, linux-arm-kernel, devicetree, linux-kernel, linux-rtc,
linux-hwmon, linux-input, linux-doc
In-Reply-To: <20251112-macsmc-subdevs-v5-2-728e4b91fe81@gmail.com>
On Wed, Nov 12, 2025 at 09:16:48PM +1000, James Calligeros wrote:
> Apple Silicon devices integrate a vast array of sensors, monitoring
> current, power, temperature, and voltage across almost every part of
> the system. The sensors themselves are all connected to the System
> Management Controller (SMC). The SMC firmware exposes the data
> reported by these sensors via its standard FourCC-based key-value
> API. The SMC is also responsible for monitoring and controlling any
> fans connected to the system, exposing them in the same way.
>
> For reasons known only to Apple, each device exposes its sensors with
> an almost totally unique set of keys. This is true even for devices
> which share an SoC. An M1 Mac mini, for example, will report its core
> temperatures on different keys to an M1 MacBook Pro. Worse still, the
> SMC does not provide a way to enumerate the available keys at runtime,
> nor do the keys follow any sort of reasonable or consistent naming
> rules that could be used to deduce their purpose. We must therefore
> know which keys are present on any given device, and which function
> they serve, ahead of time.
>
> Add a schema so that we can describe the available sensors for a given
> Apple Silicon device in the Devicetree.
>
> Reviewed-by: Neal Gompa <neal@gompa.dev>
> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
> ---
> .../bindings/hwmon/apple,smc-hwmon.yaml | 86 +++++++++++++++++++++++++
> .../bindings/mfd/apple,smc.yaml | 36 +++++++++++
I see that the rest of this series is going to be applied. This patch
touches bindings in mfd, which I can not apply.
With the assumption that the patch will be applied through some other tree,
presumably mfd:
Acked-by: Guenter Roeck <linux@roeck-us.net>
Guenter
> MAINTAINERS | 1 +
> 3 files changed, 123 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/hwmon/apple,smc-hwmon.yaml b/Documentation/devicetree/bindings/hwmon/apple,smc-hwmon.yaml
> new file mode 100644
> index 000000000000..2eec317bc4b3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/apple,smc-hwmon.yaml
> @@ -0,0 +1,86 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/apple,smc-hwmon.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple SMC Hardware Monitoring
> +
> +description:
> + Apple's System Management Controller (SMC) exposes a vast array of
> + hardware monitoring sensors, including temperature probes, current and
> + voltage sense, power meters, and fan speeds. It also provides endpoints
> + to manually control the speed of each fan individually. Each Apple
> + Silicon device exposes a different set of endpoints via SMC keys. This
> + is true even when two machines share an SoC. The CPU core temperature
> + sensor keys on an M1 Mac mini are different to those on an M1 MacBook
> + Pro, for example.
> +
> +maintainers:
> + - James Calligeros <jcalligeros99@gmail.com>
> +
> +$defs:
> + sensor:
> + type: object
> +
> + properties:
> + apple,key-id:
> + $ref: /schemas/types.yaml#/definitions/string
> + pattern: "^[A-Za-z0-9]{4}$"
> + description: The SMC FourCC key of the desired sensor.
> + Must match the node's suffix.
> +
> + label:
> + description: Human-readable name for the sensor
> +
> + required:
> + - apple,key-id
> +
> +properties:
> + compatible:
> + const: apple,smc-hwmon
> +
> +patternProperties:
> + "^current-[A-Za-z0-9]{4}$":
> + $ref: "#/$defs/sensor"
> + unevaluatedProperties: false
> +
> + "^fan-[A-Za-z0-9]{4}$":
> + $ref: "#/$defs/sensor"
> + unevaluatedProperties: false
> +
> + properties:
> + apple,fan-minimum:
> + $ref: /schemas/types.yaml#/definitions/string
> + pattern: "^[A-Za-z0-9]{4}$"
> + description: SMC key containing the fan's minimum speed
> +
> + apple,fan-maximum:
> + $ref: /schemas/types.yaml#/definitions/string
> + pattern: "^[A-Za-z0-9]{4}$"
> + description: SMC key containing the fan's maximum speed
> +
> + apple,fan-target:
> + $ref: /schemas/types.yaml#/definitions/string
> + pattern: "^[A-Za-z0-9]{4}$"
> + description: Writeable endpoint for setting desired fan speed
> +
> + apple,fan-mode:
> + $ref: /schemas/types.yaml#/definitions/string
> + pattern: "^[A-Za-z0-9]{4}$"
> + description: Writeable key to enable/disable manual fan control
> +
> +
> + "^power-[A-Za-z0-9]{4}$":
> + $ref: "#/$defs/sensor"
> + unevaluatedProperties: false
> +
> + "^temperature-[A-Za-z0-9]{4}$":
> + $ref: "#/$defs/sensor"
> + unevaluatedProperties: false
> +
> + "^voltage-[A-Za-z0-9]{4}$":
> + $ref: "#/$defs/sensor"
> + unevaluatedProperties: false
> +
> +additionalProperties: false
> diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> index 0410e712c900..34ce048619f5 100644
> --- a/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> @@ -49,6 +49,9 @@ properties:
> rtc:
> $ref: /schemas/rtc/apple,smc-rtc.yaml
>
> + hwmon:
> + $ref: /schemas/hwmon/apple,smc-hwmon.yaml
> +
> additionalProperties: false
>
> required:
> @@ -89,5 +92,38 @@ examples:
> nvmem-cells = <&rtc_offset>;
> nvmem-cell-names = "rtc_offset";
> };
> +
> + hwmon {
> + compatible = "apple,smc-hwmon";
> +
> + current-ID0R {
> + apple,key-id = "ID0R";
> + label = "AC Input Current";
> + };
> +
> + fan-F0Ac {
> + apple,key-id = "F0Ac";
> + apple,fan-minimum = "F0Mn";
> + apple,fan-maximum = "F0Mx";
> + apple,fan-target = "F0Tg";
> + apple,fan-mode = "F0Md";
> + label = "Fan 1";
> + };
> +
> + power-PSTR {
> + apple,key-id = "PSTR";
> + label = "Total System Power";
> + };
> +
> + temperature-TW0P {
> + apple,key-id = "TW0P";
> + label = "WiFi/BT Module Temperature";
> + };
> +
> + voltage-VD0R {
> + apple,key-id = "VD0R";
> + label = "AC Input Voltage";
> + };
> + };
> };
> };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 51942a9a9b43..6e5e219c5fe6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2422,6 +2422,7 @@ F: Documentation/devicetree/bindings/cpufreq/apple,cluster-cpufreq.yaml
> F: Documentation/devicetree/bindings/dma/apple,admac.yaml
> F: Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
> F: Documentation/devicetree/bindings/gpu/apple,agx.yaml
> +F: Documentation/devicetree/bindings/hwmon/apple,smc-hwmon.yaml
> F: Documentation/devicetree/bindings/i2c/apple,i2c.yaml
> F: Documentation/devicetree/bindings/input/touchscreen/apple,z2-multitouch.yaml
> F: Documentation/devicetree/bindings/interrupt-controller/apple,*
^ permalink raw reply
* Re: [PATCH v4 35/36] iio: pressure: hid-sensor-press: drop hid_sensor_remove_trigger() using devm API
From: Christophe JAILLET @ 2026-05-25 10:09 UTC (permalink / raw)
To: Sanjay Chitroda, jikos, jic23, srinivas.pandruvada
Cc: dlechner, nuno.sa, andy, sakari.ailus, linux-input, linux-iio,
linux-kernel
In-Reply-To: <20260524192943.1196773-6-sanjayembedded@gmail.com>
Le 24/05/2026 à 21:29, Sanjay Chitroda a écrit :
> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
>
> Use devm_hid_sensor_setup_trigger() to automatically release resource
> during fail, unbind or removal of driver using devres framework.
>
> This simplify the setup, remove goto, avoid manual resource cleanup in
> teardown path.
>
> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> ---
> drivers/iio/pressure/hid-sensor-press.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iio/pressure/hid-sensor-press.c b/drivers/iio/pressure/hid-sensor-press.c
> index fee7dcb86801..8438498c9aeb 100644
> --- a/drivers/iio/pressure/hid-sensor-press.c
> +++ b/drivers/iio/pressure/hid-sensor-press.c
> @@ -291,8 +291,8 @@ static int hid_press_probe(struct platform_device *pdev)
>
> atomic_set(&press_state->common_attributes.data_ready, 0);
>
> - ret = hid_sensor_setup_trigger(indio_dev, name,
> - &press_state->common_attributes);
> + ret = devm_hid_sensor_setup_trigger(dev, indio_dev, name,
> + &press_state->common_attributes);
> if (ret) {
> dev_err(dev, "trigger setup failed\n");
> return ret;
> @@ -301,7 +301,7 @@ static int hid_press_probe(struct platform_device *pdev)
> ret = iio_device_register(indio_dev);
Hi,
Looks like devm_iio_device_register() would now make sense and simplify
things even further.
CJ
> if (ret) {
> dev_err(dev, "device register failed\n");
> - goto error_remove_trigger;
> + return ret;
> }
>
> press_state->callbacks.send_event = press_proc_event;
> @@ -318,8 +318,6 @@ static int hid_press_probe(struct platform_device *pdev)
>
> error_iio_unreg:
> iio_device_unregister(indio_dev);
> -error_remove_trigger:
> - hid_sensor_remove_trigger(&press_state->common_attributes);
> return ret;
> }
>
> @@ -328,11 +326,9 @@ static void hid_press_remove(struct platform_device *pdev)
> {
> struct hid_sensor_hub_device *hsdev = dev_get_platdata(&pdev->dev);
> struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> - struct press_state *press_state = iio_priv(indio_dev);
>
> sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_PRESSURE);
> iio_device_unregister(indio_dev);
> - hid_sensor_remove_trigger(&press_state->common_attributes);
> }
>
> static const struct platform_device_id hid_press_ids[] = {
^ permalink raw reply
* Re: [PATCH 02/10] [v3] input: gpio-keys: make legacy gpiolib optional
From: Linus Walleij @ 2026-05-25 8:57 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-gpio, 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,
Bartosz Golaszewski, Dmitry Torokhov, Lee Jones, Pavel Machek,
Matti Vaittinen, 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 Wed, May 20, 2026 at 8:38 PM Arnd Bergmann <arnd@kernel.org> 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>
Reviewed-by: Linus Walleij <linusw@kernel.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v9 4/4] HID: hid-msi: Add Rumble Intensity Attributes
From: sashiko-bot @ 2026-05-25 4:17 UTC (permalink / raw)
To: Derek J. Clark; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260525020543.519082-5-derekjohn.clark@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Mixing goto-based error handling with scope-based cleanup (__free/guard) in claw_hw_output_report().
--
commit a152767539ab279fa2750588503cc5a77a8522da
Author: Derek J. Clark <derekjohn.clark@gmail.com>
HID: hid-msi: Add Rumble Intensity Attributes
Adds intensity adjustment for the left and right rumble motors. It introduces
new sysfs attributes for reading and writing rumble intensity.
> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 96710d80543d4..f5c54cd62f3bb 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> @@ -930,6 +962,162 @@ static ssize_t button_mapping_options_show(struct device *dev,
[ ... ]
> +static ssize_t rumble_intensity_left_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
[ ... ]
> + guard(mutex)(&drvdata->rom_mutex);
> + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
> + (u8 *)&report, sizeof(report), 25);
[Severity: High]
This is a pre-existing issue, but does claw_hw_output_report() mix goto-based
error handling with scope-based cleanup?
Looking at claw_hw_output_report(), it defines dmabuf with __free(kfree)
and uses guard(mutex), but also relies on explicit goto err_clear_state
jumps for error handling:
unsigned char *dmabuf __free(kfree) = NULL;
...
if (ret < 0)
goto err_clear_state;
...
err_clear_state:
According to the cleanup guidelines, functions should either convert all
resources to scope-based cleanup, or convert none of them to avoid confusing
ownership semantics. Could this function be updated to strictly use one
approach?
> + if (ret)
> + return ret;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260525020543.519082-1-derekjohn.clark@gmail.com?part=4
^ permalink raw reply
* Re: [PATCH v9 3/4] HID: hid-msi: Add RGB control interface
From: sashiko-bot @ 2026-05-25 3:41 UTC (permalink / raw)
To: Derek J. Clark; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260525020543.519082-4-derekjohn.clark@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Tearing down the `rgb_queue` delayed work before unregistering the sysfs attributes in `claw_remove()` allows the work to be re-armed, leading to a Use-After-Free.
--
commit e775f842567caa50d4c5a9bd87b14e6d6c8ffff6
Author: Derek J. Clark <derekjohn.clark@gmail.com>
Subject: HID: hid-msi: Add RGB control interface
This commit adds an RGB control interface for MSI Claw devices. It supports
configuring up to 8 sequential frames of RGB zone data across 9 zones,
mirroring effects available in the official Windows driver.
> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 5f20102eae69d..96710d80543d4 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> @@ -1043,11 +1675,21 @@ static void claw_remove(struct hid_device *hdev)
> gp_registered = smp_load_acquire(&drvdata->gp_registered);
> /* Pairs with smp_load_acquire in attribute show/store functions */
> smp_store_release(&drvdata->gp_registered, false);
> + /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
> + rgb_registered = smp_load_acquire(&drvdata->rgb_registered);
> + /* Pairs with smp_load_acquire in attribute show/store functions */
> + smp_store_release(&drvdata->rgb_registered, false);
> }
>
> if (gp_registered)
> device_remove_group(&hdev->dev, &claw_gamepad_attr_group);
>
> + cancel_delayed_work_sync(&drvdata->rgb_queue);
[Severity: High]
Could this lead to a use-after-free if the rgb_queue delayed work is re-armed
while claw_remove() is running?
If a sysfs writer (like effect_store()) reads rgb_registered as true under
registration_lock and gets preempted, claw_remove() can clear rgb_registered,
call cancel_delayed_work_sync(), and then block in device_remove_group()
waiting for the sysfs writer to drop its active kernfs reference.
The sysfs writer could then resume, call mod_delayed_work() to re-arm the
workqueue, and finish:
sysfs writer
scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
if (!smp_load_acquire(&drvdata->rgb_registered))
return -ENODEV;
}
// Preempted here
claw_remove()
smp_store_release(&drvdata->rgb_registered, false);
cancel_delayed_work_sync(&drvdata->rgb_queue);
device_remove_group(drvdata->led_mc.led_cdev.dev, &claw_rgb_attr_group);
// Blocks waiting for sysfs writer
sysfs writer (resumes)
mod_delayed_work(system_wq, &drvdata->rgb_queue, msecs_to_jiffies(50));
// Completes
claw_remove()
// Unblocks and completes
After claw_remove() finishes, drvdata is freed by devres. When the re-armed
delayed work executes 50ms later, it would dereference the freed drvdata,
causing a use-after-free crash.
Should cancel_delayed_work_sync() be called after the sysfs attributes and LED
class device are fully unregistered?
> + if (rgb_registered) {
> + device_remove_group(drvdata->led_mc.led_cdev.dev, &claw_rgb_attr_group);
> + led_classdev_multicolor_unregister(&drvdata->led_mc);
> + }
> +
> hid_hw_close(hdev);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260525020543.519082-1-derekjohn.clark@gmail.com?part=3
^ permalink raw reply
* Re: [PATCH v9 2/4] HID: hid-msi: Add M-key mapping attributes
From: Derek John Clark @ 2026-05-25 3:39 UTC (permalink / raw)
To: sashiko-reviews; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260525031507.088AC1F000E9@smtp.kernel.org>
On Sun, May 24, 2026 at 8:15 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] Typographical error checking `gamepad_registered` instead of `gp_registered` causes newly added sysfs attributes to unconditionally return `-ENODEV`.
> - [High] Dangling asynchronous ACK from `SYNC_TO_ROM` can prematurely complete subsequent commands, corrupting the device state machine.
> - [Medium] Spurious or late `READ_PROFILE_ACK` packets incorrectly abort subsequent profile reads with `-EINVAL`.
> - [Medium] If `claw_hw_output_report()` fails in `claw_mkey_show()`, the function exits without clearing the `profile_pending` state.
> --
>
> commit fe7de95a59af02c44e638aba1fcc1763f3b5354f
> Author: Derek J. Clark <derekjohn.clark@gmail.com>
>
> HID: hid-msi: Add M-key mapping attributes
>
> Adds attributes that allow for remapping the M-keys with up to 5 values when in
> macro mode. Mapped events will fire from gamepad, keyboard, or mouse evdevs.
>
> > diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> > index 3a176f44f2c43..5f20102eae69d 100644
> > --- a/drivers/hid/hid-msi.c
> > +++ b/drivers/hid/hid-msi.c
> [ ... ]
> > @@ -87,24 +249,47 @@ struct claw_command_report {
> > u8 data[59];
> > } __packed;
> >
> [ ... ]
> > struct claw_drvdata {
> > /* MCU General Variables */
> > + enum claw_profile_ack_pending profile_pending;
> > struct completion send_cmd_complete;
> [ ... ]
> > /* Gamepad Variables */
> > enum claw_mkeys_function_index mkeys_function;
> > enum claw_gamepad_mode_index gamepad_mode;
> > + u8 m1_codes[CLAW_KEYS_MAX];
> > + u8 m2_codes[CLAW_KEYS_MAX];
> > + bool gamepad_registered;
> > spinlock_t mode_lock; /* Lock for mode data read/write */
> > + const u16 *bmap_addr;
> > + bool bmap_support;
> > bool gp_registered;
> > };
> [ ... ]
> > @@ -165,9 +383,20 @@ static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *repor
> > complete(&drvdata->send_cmd_complete);
> > }
> >
> > + break;
> > + case CLAW_COMMAND_TYPE_READ_PROFILE_ACK:
> > + ret = claw_profile_event(drvdata, cmd_rep);
> > + if (drvdata->waiting_for_ack &&
> > + drvdata->waiting_cmd == CLAW_COMMAND_TYPE_READ_PROFILE) {
> > + drvdata->cmd_status = ret;
> > + drvdata->waiting_for_ack = false;
> > + complete(&drvdata->send_cmd_complete);
> > + }
>
> [Severity: Medium]
> If a spurious or late READ_PROFILE_ACK packet arrives, claw_profile_event()
> will return -EINVAL. Does this unconditionally pass the error to cmd_status
> and prematurely complete the wait for a valid, currently pending READ_PROFILE
> command?
This is a limitation of the hardware. In real life usage this issue is
unlikely to occur, and if it does it will just cause a timeout of the
next command which can be re-sent.
> > +
> > + break;
> > case CLAW_COMMAND_TYPE_ACK:
> [ ... ]
> > @@ -442,6 +671,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)
> > +{
> > + struct hid_device *hdev = to_hid_device(dev);
> > + struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> > + struct claw_mkey_report report = { {0x01, cpu_to_be16(drvdata->bmap_addr[mkey])},
> > + 0x07, 0x04, 0x00, {0xff, 0xff, 0xff, 0xff, 0xff} };
> > + char **raw_keys __free(argv_free) = NULL;
> > + int ret, key_count, i;
> > +
> > + 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]
> Does this check the newly added gamepad_registered field instead of the
> already existing gp_registered field?
Mistake in rebase, will fix.
> Since gamepad_registered is never set to true, it looks like this will
> unconditionally return -ENODEV, rendering the sysfs attributes inaccessible.
>
> > +
> > + raw_keys = argv_split(GFP_KERNEL, buf, &key_count);
> [ ... ]
> > +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]
> Since this command uses a timeout of 0, could the asynchronous ACK from
> SYNC_TO_ROM arrive while the driver is waiting for a subsequent command
> (like another WRITE_PROFILE_DATA)?
>
This is also a limitation of the hardware and unlikely to occur.
> If the generic ACK arrives after the driver starts waiting for the next
> command, it seems it could falsely complete the next command prematurely
> and break the lock-step protocol between host and MCU.
>
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int claw_mkey_show(struct device *dev, char *buf, enum claw_key_index m_key)
> > +{
> > + struct hid_device *hdev = to_hid_device(dev);
> > + struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> > + struct claw_mkey_report report = { {0x01, cpu_to_be16(drvdata->bmap_addr[m_key])}, 0x07 };
> > + int i, ret, count = 0;
> > + const char *name;
> > + u8 *codes;
> > +
> > + 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;
> > + }
> > +
> > + codes = (m_key == CLAW_KEY_M1) ? drvdata->m1_codes : drvdata->m2_codes;
> > +
> > + guard(mutex)(&drvdata->profile_mutex);
> > + scoped_guard(spinlock_irqsave, &drvdata->profile_lock)
> > + drvdata->profile_pending = (m_key == CLAW_KEY_M1) ? CLAW_M1_PENDING
> > + : CLAW_M2_PENDING;
> > +
> > + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_PROFILE,
> > + (u8 *)&report, sizeof(report), 25);
> > + if (ret)
> > + return ret;
>
> [Severity: Medium]
> If claw_hw_output_report() fails or times out, this returns immediately.
> Does this leave drvdata->profile_pending set to a pending state?
>
> If the state is left dangling after the mutex is released, could a subsequent
> late or spurious profile packet be incorrectly accepted and parsed into the
> local codes arrays?
>
Subsequent profile events will overwrite profile_pending before
sending their data. This isn't a problem.
- Derek
> > +
> > + for (i = 0; i < CLAW_KEYS_MAX; i++) {
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260525020543.519082-1-derekjohn.clark@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH v9 2/4] HID: hid-msi: Add M-key mapping attributes
From: sashiko-bot @ 2026-05-25 3:15 UTC (permalink / raw)
To: Derek J. Clark; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260525020543.519082-3-derekjohn.clark@gmail.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Typographical error checking `gamepad_registered` instead of `gp_registered` causes newly added sysfs attributes to unconditionally return `-ENODEV`.
- [High] Dangling asynchronous ACK from `SYNC_TO_ROM` can prematurely complete subsequent commands, corrupting the device state machine.
- [Medium] Spurious or late `READ_PROFILE_ACK` packets incorrectly abort subsequent profile reads with `-EINVAL`.
- [Medium] If `claw_hw_output_report()` fails in `claw_mkey_show()`, the function exits without clearing the `profile_pending` state.
--
commit fe7de95a59af02c44e638aba1fcc1763f3b5354f
Author: Derek J. Clark <derekjohn.clark@gmail.com>
HID: hid-msi: Add M-key mapping attributes
Adds attributes that allow for remapping the M-keys with up to 5 values when in
macro mode. Mapped events will fire from gamepad, keyboard, or mouse evdevs.
> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 3a176f44f2c43..5f20102eae69d 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> @@ -87,24 +249,47 @@ struct claw_command_report {
> u8 data[59];
> } __packed;
>
[ ... ]
> struct claw_drvdata {
> /* MCU General Variables */
> + enum claw_profile_ack_pending profile_pending;
> struct completion send_cmd_complete;
[ ... ]
> /* Gamepad Variables */
> enum claw_mkeys_function_index mkeys_function;
> enum claw_gamepad_mode_index gamepad_mode;
> + u8 m1_codes[CLAW_KEYS_MAX];
> + u8 m2_codes[CLAW_KEYS_MAX];
> + bool gamepad_registered;
> spinlock_t mode_lock; /* Lock for mode data read/write */
> + const u16 *bmap_addr;
> + bool bmap_support;
> bool gp_registered;
> };
[ ... ]
> @@ -165,9 +383,20 @@ static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *repor
> complete(&drvdata->send_cmd_complete);
> }
>
> + break;
> + case CLAW_COMMAND_TYPE_READ_PROFILE_ACK:
> + ret = claw_profile_event(drvdata, cmd_rep);
> + if (drvdata->waiting_for_ack &&
> + drvdata->waiting_cmd == CLAW_COMMAND_TYPE_READ_PROFILE) {
> + drvdata->cmd_status = ret;
> + drvdata->waiting_for_ack = false;
> + complete(&drvdata->send_cmd_complete);
> + }
[Severity: Medium]
If a spurious or late READ_PROFILE_ACK packet arrives, claw_profile_event()
will return -EINVAL. Does this unconditionally pass the error to cmd_status
and prematurely complete the wait for a valid, currently pending READ_PROFILE
command?
> +
> + break;
> case CLAW_COMMAND_TYPE_ACK:
[ ... ]
> @@ -442,6 +671,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)
> +{
> + struct hid_device *hdev = to_hid_device(dev);
> + struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> + struct claw_mkey_report report = { {0x01, cpu_to_be16(drvdata->bmap_addr[mkey])},
> + 0x07, 0x04, 0x00, {0xff, 0xff, 0xff, 0xff, 0xff} };
> + char **raw_keys __free(argv_free) = NULL;
> + int ret, key_count, i;
> +
> + 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]
Does this check the newly added gamepad_registered field instead of the
already existing gp_registered field?
Since gamepad_registered is never set to true, it looks like this will
unconditionally return -ENODEV, rendering the sysfs attributes inaccessible.
> +
> + raw_keys = argv_split(GFP_KERNEL, buf, &key_count);
[ ... ]
> +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]
Since this command uses a timeout of 0, could the asynchronous ACK from
SYNC_TO_ROM arrive while the driver is waiting for a subsequent command
(like another WRITE_PROFILE_DATA)?
If the generic ACK arrives after the driver starts waiting for the next
command, it seems it could falsely complete the next command prematurely
and break the lock-step protocol between host and MCU.
> + }
> +
> + return ret;
> +}
> +
> +static int claw_mkey_show(struct device *dev, char *buf, enum claw_key_index m_key)
> +{
> + struct hid_device *hdev = to_hid_device(dev);
> + struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> + struct claw_mkey_report report = { {0x01, cpu_to_be16(drvdata->bmap_addr[m_key])}, 0x07 };
> + int i, ret, count = 0;
> + const char *name;
> + u8 *codes;
> +
> + 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;
> + }
> +
> + codes = (m_key == CLAW_KEY_M1) ? drvdata->m1_codes : drvdata->m2_codes;
> +
> + guard(mutex)(&drvdata->profile_mutex);
> + scoped_guard(spinlock_irqsave, &drvdata->profile_lock)
> + drvdata->profile_pending = (m_key == CLAW_KEY_M1) ? CLAW_M1_PENDING
> + : CLAW_M2_PENDING;
> +
> + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_PROFILE,
> + (u8 *)&report, sizeof(report), 25);
> + if (ret)
> + return ret;
[Severity: Medium]
If claw_hw_output_report() fails or times out, this returns immediately.
Does this leave drvdata->profile_pending set to a pending state?
If the state is left dangling after the mutex is released, could a subsequent
late or spurious profile packet be incorrectly accepted and parsed into the
local codes arrays?
> +
> + for (i = 0; i < CLAW_KEYS_MAX; i++) {
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260525020543.519082-1-derekjohn.clark@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH v9 1/4] HID: hid-msi: Add MSI Claw configuration driver
From: Derek J. Clark @ 2026-05-25 3:11 UTC (permalink / raw)
To: sashiko-reviews, sashiko-bot; +Cc: dmitry.torokhov, linux-input, bentiss, jikos
In-Reply-To: <20260525023712.E1C131F000E9@smtp.kernel.org>
On May 25, 2026 11:37:12 AM GMT+09:00, sashiko-bot@kernel.org wrote:
>Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>- [Medium] Mixing goto-based error handling with scope-based cleanups (__free, guard) in claw_hw_output_report.
>--
>
>commit 279964e3a8cf0a3835903d6bad7357f0ca42f523
>Author: Derek J. Clark <derekjohn.clark@gmail.com>
>
>HID: hid-msi: Add MSI Claw configuration driver
>
>This commit adds a configuration HID driver for the MSI Claw series of
>handheld PCs. It introduces the initial driver outline and attributes for
>changing gamepad mode, M-key behavior, and resetting the device. It also
>adds a completion bypass when the timeout is 0 to handle USB disconnects
>gracefully.
>
>> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
>> new file mode 100644
>> index 0000000000000..3a176f44f2c43
>> --- /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;
>
>[ ... ]
>
>> + guard(mutex)(&drvdata->cfg_mutex);
>
>[ ... ]
>
>> + ret = hid_hw_output_report(hdev, dmabuf, CLAW_PACKET_SIZE);
>> + if (ret < 0)
>> + goto err_clear_state;
>
>[Severity: Medium]
Hi robot,
Interesting that this same issue was Severity: Low in v8...
>Is it intended to mix scope-based cleanups with goto statements in this
>function? The subsystem guidelines recommend either converting all resources
>that need a goto cleanup to scope-based cleanups, or converting none of them.
>Mixing __free() and guard() declarations with goto-based error handling in the
>same function can create confusing ownership semantics.
>
The goto's don't return early or break in/out of guarded sections. It simply skips waiting for the completion if there was an error sending the payload. Using this goto removes the need for 8 lines of duplicated code and the use of cmd_lock in 2 additional places. Unless Jiri or Benjamin specifically ask for it I don't intend to change this.
- Derek
>> +
>> + 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;
>> +}
>
^ permalink raw reply
* Re: [PATCH] HID: hid-lenovo-go: cancel cfg_setup work in hid_go_cfg_remove()
From: Derek J. Clark @ 2026-05-25 3:09 UTC (permalink / raw)
To: Manish Khadka, linux-input
Cc: Mark Pearson, Jiri Kosina, Benjamin Tissoires, linux-kernel
In-Reply-To: <20260515153607.76175-1-maskmemanish@gmail.com>
On May 15, 2026 8:36:07 AM PDT, Manish Khadka <maskmemanish@gmail.com> wrote:
>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);
Looks good.
Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
^ permalink raw reply
* Re: [PATCH v9 1/4] HID: hid-msi: Add MSI Claw configuration driver
From: sashiko-bot @ 2026-05-25 2:37 UTC (permalink / raw)
To: Derek J. Clark; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260525020543.519082-2-derekjohn.clark@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Mixing goto-based error handling with scope-based cleanups (__free, guard) in claw_hw_output_report.
--
commit 279964e3a8cf0a3835903d6bad7357f0ca42f523
Author: Derek J. Clark <derekjohn.clark@gmail.com>
HID: hid-msi: Add MSI Claw configuration driver
This commit adds a configuration HID driver for the MSI Claw series of
handheld PCs. It introduces the initial driver outline and attributes for
changing gamepad mode, M-key behavior, and resetting the device. It also
adds a completion bypass when the timeout is 0 to handle USB disconnects
gracefully.
> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> new file mode 100644
> index 0000000000000..3a176f44f2c43
> --- /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;
[ ... ]
> + guard(mutex)(&drvdata->cfg_mutex);
[ ... ]
> + ret = hid_hw_output_report(hdev, dmabuf, CLAW_PACKET_SIZE);
> + if (ret < 0)
> + goto err_clear_state;
[Severity: Medium]
Is it intended to mix scope-based cleanups with goto statements in this
function? The subsystem guidelines recommend either converting all resources
that need a goto cleanup to scope-based cleanups, or converting none of them.
Mixing __free() and guard() declarations with goto-based error handling in the
same function can create 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;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260525020543.519082-1-derekjohn.clark@gmail.com?part=1
^ permalink raw reply
* [PATCH v9 4/4] HID: hid-msi: Add Rumble Intensity Attributes
From: Derek J. Clark @ 2026-05-25 2:05 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Pierre-Loup A . Griffais, Denis Benato, Zhouwang Huang,
Derek J . Clark, linux-input, linux-doc, linux-kernel
In-Reply-To: <20260525020543.519082-1-derekjohn.clark@gmail.com>
Adds intensity adjustment for the left and right rumble motors.
Claude was used during the reverse-engineering data gathering for this
feature done by Zhouwang Huang. As the code had already been affected,
I used Claude to create the initial framing for the feature, then did
manual cleanup of the _show and _store functions afterwards to fix bugs
and keep the coding style consistent. Claude was also used as an initial
reviewer of this patch.
Assisted-by: Claude:claude-sonnet-4-6
Co-developed-by: Zhouwang Huang <honjow311@gmail.com>
Signed-off-by: Zhouwang Huang <honjow311@gmail.com>
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
v7:
- Match on write address for rumble reports to prevent late ACK
from causing synchronization errors.
- Use spinlock for read/write profile_pending.
- Use smp_[store_release|load_acquire] pattern for checking
gamepad_registered to avoid possible races during teardown.
- Use struct for rumble reports.
v6:
- Make all timeouts 25ms to ensure at least 2 jiffies in a 100Hz
config.
- Add spinlock_irqsave for read/write access on rumble_intensity
variables.
- Gate all attribute show/store functions with gamepad_registered.
v5:
- Remove mkey related changes.
v2:
- Use pending_profile and sync to rom mutexes.
---
drivers/hid/hid-msi.c | 202 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 201 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
index 96710d80543d4..f5c54cd62f3bb 100644
--- a/drivers/hid/hid-msi.c
+++ b/drivers/hid/hid-msi.c
@@ -79,6 +79,8 @@ enum claw_profile_ack_pending {
CLAW_M1_PENDING,
CLAW_M2_PENDING,
CLAW_RGB_PENDING,
+ CLAW_RUMBLE_LEFT_PENDING,
+ CLAW_RUMBLE_RIGHT_PENDING,
};
enum claw_key_index {
@@ -266,6 +268,11 @@ static const u16 button_mapping_addr_new[] = {
static const u16 rgb_addr_old = 0x01fa;
static const u16 rgb_addr_new = 0x024a;
+static const u16 rumble_addr[] = {
+ 0x0022, /* left */
+ 0x0023, /* right */
+};
+
struct claw_command_report {
u8 report_id;
u8 padding[2];
@@ -308,6 +315,12 @@ struct claw_rgb_report {
struct rgb_frame zone_data;
} __packed;
+struct claw_rumble_report {
+ struct claw_profile_report;
+ u8 padding;
+ u8 intensity;
+} __packed;
+
struct claw_drvdata {
/* MCU General Variables */
enum claw_profile_ack_pending profile_pending;
@@ -332,8 +345,12 @@ struct claw_drvdata {
enum claw_gamepad_mode_index gamepad_mode;
u8 m1_codes[CLAW_KEYS_MAX];
u8 m2_codes[CLAW_KEYS_MAX];
- spinlock_t mode_lock; /* Lock for mode data read/write */
+ u8 rumble_intensity_right;
+ u8 rumble_intensity_left;
const u16 *bmap_addr;
+ spinlock_t rumble_lock; /* lock for rumble_intensity read/write */
+ spinlock_t mode_lock; /* Lock for mode data read/write */
+ bool rumble_support;
bool gp_registered;
bool bmap_support;
@@ -382,6 +399,7 @@ static int claw_gamepad_mode_event(struct claw_drvdata *drvdata,
static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_report *cmd_rep)
{
enum claw_profile_ack_pending profile;
+ struct claw_rumble_report *rumble;
struct claw_mkey_report *mkeys;
struct claw_rgb_report *frame;
u16 rgb_addr, read_addr;
@@ -431,6 +449,20 @@ static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_
}
break;
+ case CLAW_RUMBLE_LEFT_PENDING:
+ rumble = (struct claw_rumble_report *)cmd_rep->data;
+ if (be16_to_cpu(rumble->read_addr) != rumble_addr[0])
+ return -EINVAL;
+ scoped_guard(spinlock, &drvdata->rumble_lock)
+ drvdata->rumble_intensity_left = rumble->intensity;
+ break;
+ case CLAW_RUMBLE_RIGHT_PENDING:
+ rumble = (struct claw_rumble_report *)cmd_rep->data;
+ if (be16_to_cpu(rumble->read_addr) != rumble_addr[1])
+ return -EINVAL;
+ scoped_guard(spinlock, &drvdata->rumble_lock)
+ drvdata->rumble_intensity_right = rumble->intensity;
+ break;
default:
dev_dbg(&drvdata->hdev->dev,
"Got profile event without changes pending from command: %x\n",
@@ -930,6 +962,162 @@ static ssize_t button_mapping_options_show(struct device *dev,
}
static DEVICE_ATTR_RO(button_mapping_options);
+static ssize_t rumble_intensity_left_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct claw_rumble_report report = { {0x01, cpu_to_be16(rumble_addr[0])}, 0x01 };
+ struct hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ u8 val;
+ 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->gp_registered))
+ return -ENODEV;
+ }
+
+ ret = kstrtou8(buf, 10, &val);
+ if (ret)
+ return ret;
+
+ if (val > 100)
+ return -EINVAL;
+
+ report.intensity = val;
+
+ 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);
+ if (ret)
+ return ret;
+
+ return count;
+}
+
+static ssize_t rumble_intensity_left_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct claw_rumble_report report = { {0x01, cpu_to_be16(rumble_addr[0])}, 0x01 };
+ struct hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ int ret;
+ u8 val;
+
+ 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->gp_registered))
+ return -ENODEV;
+ }
+
+ guard(mutex)(&drvdata->profile_mutex);
+ scoped_guard(spinlock_irqsave, &drvdata->profile_lock)
+ drvdata->profile_pending = CLAW_RUMBLE_LEFT_PENDING;
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_PROFILE,
+ (u8 *)&report, sizeof(report), 25);
+ if (ret)
+ return ret;
+
+ scoped_guard(spinlock_irqsave, &drvdata->rumble_lock)
+ val = drvdata->rumble_intensity_left;
+
+ return sysfs_emit(buf, "%u\n", val);
+}
+static DEVICE_ATTR_RW(rumble_intensity_left);
+
+static ssize_t rumble_intensity_right_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct claw_rumble_report report = { {0x01, cpu_to_be16(rumble_addr[1])}, 0x01 };
+ struct hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ u8 val;
+ 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->gp_registered))
+ return -ENODEV;
+ }
+
+ ret = kstrtou8(buf, 10, &val);
+ if (ret)
+ return ret;
+
+ if (val > 100)
+ return -EINVAL;
+
+ report.intensity = val;
+
+ 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);
+ if (ret)
+ return ret;
+
+ return count;
+}
+
+static ssize_t rumble_intensity_right_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct claw_rumble_report report = { {0x01, cpu_to_be16(rumble_addr[1])}, 0x01 };
+ struct hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ int ret;
+ u8 val;
+
+ 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->gp_registered))
+ return -ENODEV;
+ }
+
+ guard(mutex)(&drvdata->profile_mutex);
+ scoped_guard(spinlock_irqsave, &drvdata->profile_lock)
+ drvdata->profile_pending = CLAW_RUMBLE_RIGHT_PENDING;
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_PROFILE,
+ (u8 *)&report, sizeof(report), 25);
+ if (ret)
+ return ret;
+
+ scoped_guard(spinlock_irqsave, &drvdata->rumble_lock)
+ val = drvdata->rumble_intensity_right;
+
+ return sysfs_emit(buf, "%u\n", val);
+}
+static DEVICE_ATTR_RW(rumble_intensity_right);
+
+static ssize_t rumble_intensity_range_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ return sysfs_emit(buf, "0-100\n");
+}
+static DEVICE_ATTR_RO(rumble_intensity_range);
+
static umode_t claw_gamepad_attr_is_visible(struct kobject *kobj, struct attribute *attr,
int n)
{
@@ -950,6 +1138,12 @@ static umode_t claw_gamepad_attr_is_visible(struct kobject *kobj, struct attribu
attr == &dev_attr_reset.attr)
return attr->mode;
+ /* Hide rumble attrs if not supported */
+ if (attr == &dev_attr_rumble_intensity_left.attr ||
+ attr == &dev_attr_rumble_intensity_right.attr ||
+ attr == &dev_attr_rumble_intensity_range.attr)
+ return drvdata->rumble_support ? attr->mode : 0;
+
/* Hide button mapping attrs if it isn't supported */
return drvdata->bmap_support ? attr->mode : 0;
}
@@ -963,6 +1157,9 @@ static struct attribute *claw_gamepad_attrs[] = {
&dev_attr_mkeys_function.attr,
&dev_attr_mkeys_function_index.attr,
&dev_attr_reset.attr,
+ &dev_attr_rumble_intensity_left.attr,
+ &dev_attr_rumble_intensity_right.attr,
+ &dev_attr_rumble_intensity_range.attr,
NULL,
};
@@ -1542,6 +1739,7 @@ static void claw_features_supported(struct claw_drvdata *drvdata)
drvdata->bmap_support = true;
if (minor >= 0x66) {
drvdata->bmap_addr = button_mapping_addr_new;
+ drvdata->rumble_support = true;
drvdata->rgb_addr = rgb_addr_new;
} else {
drvdata->bmap_addr = button_mapping_addr_old;
@@ -1553,6 +1751,7 @@ static void claw_features_supported(struct claw_drvdata *drvdata)
if ((major == 0x02 && minor >= 0x17) || major >= 0x03) {
drvdata->bmap_support = true;
drvdata->bmap_addr = button_mapping_addr_new;
+ drvdata->rumble_support = true;
drvdata->rgb_addr = rgb_addr_new;
return;
}
@@ -1602,6 +1801,7 @@ static int claw_probe(struct hid_device *hdev, u8 ep)
spin_lock_init(&drvdata->mode_lock);
spin_lock_init(&drvdata->profile_lock);
spin_lock_init(&drvdata->frame_lock);
+ spin_lock_init(&drvdata->rumble_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);
--
2.53.0
^ permalink raw reply related
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