* [PATCH 4/6] Input: mms114 - replace udelay with usleep_range
From: Dmitry Torokhov @ 2026-06-16 5:09 UTC (permalink / raw)
To: linux-input; +Cc: Linus Walleij, Bryam Vargas, linux-kernel
In-Reply-To: <20260616050912.1531241-1-dmitry.torokhov@gmail.com>
The driver currently uses udelay(MMS114_I2C_DELAY) (50us) to ensure a
mandatory delay between I2C transfers in __mms114_read_reg() and
mms114_write_reg().
Both functions invoke underlying I2C core operations (i2c_transfer,
i2c_master_send) which acquire mutexes and sleep. Furthermore, the
interrupt handler mms114_interrupt() is registered as a threaded IRQ
handler. Since the entire execution path is fully sleepable,
busy-waiting with udelay() for 50us unnecessarily wastes CPU cycles.
Replace udelay() with usleep_range() to allow the CPU to enter low-power
states or execute other tasks during the delay.
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/mms114.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index c2e006ac1196..c59aec8f2feb 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -116,7 +116,7 @@ static int __mms114_read_reg(struct mms114_data *data, u8 reg,
"%s: i2c transfer failed (%d)\n", __func__, error);
return error < 0 ? error : -EIO;
}
- udelay(MMS114_I2C_DELAY);
+ usleep_range(MMS114_I2C_DELAY, MMS114_I2C_DELAY + 50);
return 0;
}
@@ -148,7 +148,7 @@ static int mms114_write_reg(struct mms114_data *data, u8 reg, u8 val)
"%s: i2c send failed (%d)\n", __func__, error);
return error < 0 ? error : -EIO;
}
- udelay(MMS114_I2C_DELAY);
+ usleep_range(MMS114_I2C_DELAY, MMS114_I2C_DELAY + 50);
if (reg == MMS114_MODE_CONTROL)
data->cache_mode_control = val;
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* [PATCH 3/6] Input: mms114 - use appropriate register argument types
From: Dmitry Torokhov @ 2026-06-16 5:09 UTC (permalink / raw)
To: linux-input; +Cc: Linus Walleij, Bryam Vargas, linux-kernel
In-Reply-To: <20260616050912.1531241-1-dmitry.torokhov@gmail.com>
The MMS114 I2C touch controller uses 8-bit register addresses (0x01 to
0xF2) and 8-bit single-register data values. The helper functions
previously declared reg and val as 32-bit unsigned int, requiring
explicit bitwise masking (& 0xff) to narrow the values down to u8 before
populating the I2C transfer buffers.
Update reg and val parameters to u8 across mms114_read_reg(),
mms114_write_reg(), and __mms114_read_reg() to accurately reflect the
hardware specification and eliminate the redundant & 0xff masking.
Additionally, update the val buffer pointer in __mms114_read_reg() from
u8 * to void * to allow callers to pass data structures directly without
requiring explicit casting.
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/mms114.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index db23b51f4630..c2e006ac1196 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -87,12 +87,12 @@ struct mms114_touch {
u8 reserved[2];
} __packed;
-static int __mms114_read_reg(struct mms114_data *data, unsigned int reg,
- unsigned int len, u8 *val)
+static int __mms114_read_reg(struct mms114_data *data, u8 reg,
+ unsigned int len, void *val)
{
struct i2c_client *client = data->client;
struct i2c_msg xfer[2];
- u8 buf = reg & 0xff;
+ u8 buf = reg;
int error;
if (reg <= MMS114_MODE_CONTROL && reg + len > MMS114_MODE_CONTROL)
@@ -121,7 +121,7 @@ static int __mms114_read_reg(struct mms114_data *data, unsigned int reg,
return 0;
}
-static int mms114_read_reg(struct mms114_data *data, unsigned int reg)
+static int mms114_read_reg(struct mms114_data *data, u8 reg)
{
u8 val;
int error;
@@ -133,15 +133,14 @@ static int mms114_read_reg(struct mms114_data *data, unsigned int reg)
return error < 0 ? error : val;
}
-static int mms114_write_reg(struct mms114_data *data, unsigned int reg,
- unsigned int val)
+static int mms114_write_reg(struct mms114_data *data, u8 reg, u8 val)
{
struct i2c_client *client = data->client;
u8 buf[2];
int error;
- buf[0] = reg & 0xff;
- buf[1] = val & 0xff;
+ buf[0] = reg;
+ buf[1] = val;
error = i2c_master_send(client, buf, 2);
if (error != 2) {
@@ -242,9 +241,8 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
touch_size = packet_size / event_size;
- error = __mms114_read_reg(data, MMS114_INFORMATION, packet_size,
- (u8 *)touch);
- if (error < 0)
+ error = __mms114_read_reg(data, MMS114_INFORMATION, packet_size, touch);
+ if (error)
goto out;
for (index = 0; index < touch_size; index++) {
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* [PATCH 2/6] Input: mms114 - prefer GPL over GPL v2 for module license
From: Dmitry Torokhov @ 2026-06-16 5:09 UTC (permalink / raw)
To: linux-input; +Cc: Linus Walleij, Bryam Vargas, linux-kernel
In-Reply-To: <20260616050912.1531241-1-dmitry.torokhov@gmail.com>
As explained in commit bf7fbeeae6db ("module: Cure the MODULE_LICENSE
"GPL" vs. "GPL v2" bogosity"), "GPL" and "GPL v2" have identical
semantics in the module loader, but "GPL" is preferred to avoid
unnecessary confusion and maintain consistency across the kernel.
Change MODULE_LICENSE("GPL v2") to MODULE_LICENSE("GPL").
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/mms114.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index 53ad35d61d47..db23b51f4630 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -722,4 +722,4 @@ module_i2c_driver(mms114_driver);
/* Module information */
MODULE_AUTHOR("Joonyoung Shim <jy0922.shim@samsung.com>");
MODULE_DESCRIPTION("MELFAS mms114 Touchscreen driver");
-MODULE_LICENSE("GPL v2");
+MODULE_LICENSE("GPL");
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* [PATCH 1/6] Input: mms114 - fix touch indexing for MMS134S and MMS136
From: Dmitry Torokhov @ 2026-06-16 5:09 UTC (permalink / raw)
To: linux-input; +Cc: Linus Walleij, Bryam Vargas, linux-kernel, sashiko-bot
The MMS134S and MMS136 touch controllers have an event size of 6 bytes
rather than 8 bytes. When __mms114_read_reg() reads the touch data
packet from the device into the touch buffer, the events are packed
tightly at 6-byte intervals. However, the driver iterates through the
events using standard C array indexing (touch[index]), where each
element is sizeof(struct mms114_touch) (8 bytes) apart. As a result, any
touch events beyond the first one are read from incorrect offsets and
parsed improperly.
Fix this by explicitly calculating the byte offset for each touch event
based on the device's specific event size.
Fixes: ab108678195f ("Input: mms114 - support MMS134S")
Reported-by: sashiko-bot@kernel.org
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/mms114.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index 4d748a13408d..53ad35d61d47 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -217,7 +217,9 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
struct mms114_data *data = dev_id;
struct i2c_client *client = data->client;
struct mms114_touch touch[MMS114_MAX_TOUCH];
+ struct mms114_touch *t;
int packet_size;
+ int event_size;
int touch_size;
int index;
int error;
@@ -234,9 +236,11 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
/* MMS136 has slightly different event size */
if (data->type == TYPE_MMS134S || data->type == TYPE_MMS136)
- touch_size = packet_size / MMS136_EVENT_SIZE;
+ event_size = MMS136_EVENT_SIZE;
else
- touch_size = packet_size / MMS114_EVENT_SIZE;
+ event_size = MMS114_EVENT_SIZE;
+
+ touch_size = packet_size / event_size;
error = __mms114_read_reg(data, MMS114_INFORMATION, packet_size,
(u8 *)touch);
@@ -244,18 +248,20 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
goto out;
for (index = 0; index < touch_size; index++) {
- switch (touch[index].type) {
+ t = (struct mms114_touch *)((u8 *)touch + index * event_size);
+
+ switch (t->type) {
case MMS114_TYPE_TOUCHSCREEN:
- mms114_process_mt(data, touch + index);
+ mms114_process_mt(data, t);
break;
case MMS114_TYPE_TOUCHKEY:
- mms114_process_touchkey(data, touch + index);
+ mms114_process_touchkey(data, t);
break;
default:
dev_err(&client->dev, "Wrong touch type (%d)\n",
- touch[index].type);
+ t->type);
break;
}
}
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* Re: [PATCH v8 4/7] input: keyboard: Add driver for ASUS Transformer dock multimedia keys
From: Dmitry Torokhov @ 2026-06-16 4:26 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Lee Jones,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <20260528053203.9339-5-clamor95@gmail.com>
Hi Svyatoslav,
On Thu, May 28, 2026 at 08:32:00AM +0300, Svyatoslav Ryhel wrote:
> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>
> Add support for multimedia top button row of ASUS Transformer's Mobile
> Dock keyboard. Driver is made that function keys (F1-F12) are used by
> default which suits average Linux use better and with pressing
> ScreenLock + AltGr function keys layout is switched to multimedia keys.
> Since this only modifies codes sent by asus-ec-keys it doesn't affect
> normal keyboards at all.
I think using input handler to intercept ScreenLock + AltGr is quite
awkward. I think this also passes the original key events (unless you
make it a filter not a regular handler).
I do not see benefit for reacting to AltGr+ScreenLock on other keyboards
to activate the special mode on this one. So given the fact that you
already mange the data stream when you split it into "serio" ports,
maybe just intercept this key combo right there and create the input
device and signal input events right there?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 1/4] HID: multitouch: set INPUT_PROP_PRESSUREPAD based on Digitizer/Button Type
From: Peter Hutterer @ 2026-06-16 4:24 UTC (permalink / raw)
To: Rong Zhang
Cc: devnull+peter.hutterer.who-t.net, bentiss, dmitry.torokhov, jikos,
linux-input, linux-kernel, linux-kselftest, shuah, vadim
In-Reply-To: <fb198851093477d958eae61665e9cfda0be5b48f.camel@rong.moe>
Hi Rong,
On Tue, Jun 02, 2026 at 01:25:57AM +0800, Rong Zhang wrote:
>
> Hi all,
>
> Hopefully I'm not too late to show up here.
>
> > From: Peter Hutterer <peter.hutterer@who-t.net>
> >
> > A Digitizer/Button Type value of 1 indicates the device is a
> > pressurepad, see
> > https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/touchpad-windows-precision-touchpad-collection#device-capabilities-feature-report
> >
> > Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
> > ---
> > drivers/hid/hid-multitouch.c | 12 +++++++++++-
> > 1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> > index 179dc316b4b518d78bdc900d9fd15756c5eba83e..382e6f50c4f7e663af7d028abb8be7cb2e6e7b8e 100644
> > --- a/drivers/hid/hid-multitouch.c
> > +++ b/drivers/hid/hid-multitouch.c
> > @@ -81,6 +81,7 @@ MODULE_LICENSE("GPL");
> > #define MT_INPUTMODE_TOUCHPAD 0x03
> >
> > #define MT_BUTTONTYPE_CLICKPAD 0
> > +#define MT_BUTTONTYPE_PRESSUREPAD 1
> >
> > enum latency_mode {
> > HID_LATENCY_NORMAL = 0,
> > @@ -179,6 +180,7 @@ struct mt_device {
> > __u8 inputmode_value; /* InputMode HID feature value */
> > __u8 maxcontacts;
> > bool is_buttonpad; /* is this device a button pad? */
> > + bool is_pressurepad; /* is this device a pressurepad? */
> > bool is_haptic_touchpad; /* is this device a haptic touchpad? */
> > bool serial_maybe; /* need to check for serial protocol */
> >
> > @@ -530,8 +532,14 @@ static void mt_feature_mapping(struct hid_device *hdev,
> > }
> >
> > mt_get_feature(hdev, field->report);
> > - if (field->value[usage->usage_index] == MT_BUTTONTYPE_CLICKPAD)
> > + switch (field->value[usage->usage_index]) {
> > + case MT_BUTTONTYPE_CLICKPAD:
> > td->is_buttonpad = true;
> > + break;
> > + case MT_BUTTONTYPE_PRESSUREPAD:
> > + td->is_pressurepad = true;
> > + break;
> > + }
> >
> > break;
> > case 0xff0000c5:
> > @@ -1393,6 +1401,8 @@ static int mt_touch_input_configured(struct hid_device *hdev,
> >
> > if (td->is_buttonpad)
> > __set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
> > + if (td->is_pressurepad)
> > + __set_bit(INPUT_PROP_PRESSUREPAD, input->propbit);
>
> I noticed that this leads to dual reporting on my device.
>
> Consider previous checks:
>
> if (application == HID_DG_TOUCHPAD) {
> mt_application->mt_flags |= INPUT_MT_POINTER;
> td->inputmode_value = MT_INPUTMODE_TOUCHPAD;
> }
>
> ...
>
> /* check for clickpads */
> if ((app->mt_flags & INPUT_MT_POINTER) &&
> (app->buttons_count == 1))
> td->is_buttonpad = true;
>
> ... where `td->is_buttonpad' is set to true when a pressure pad has only
> one button, i.e., the "touchpad button integrated with digitizer" [1].
> Most (if not all) pressure pads fall into this category. As a result,
> the presence of INPUT_PROP_PRESSUREPAD is always accompanied by the
> presence of INPUT_PROP_BUTTONPAD.
Yes, this is intended, see commit
ae8966b7b5bd69b86209cc34bcca1ba9f18b68e6 which lists this in the commit
message:
```
This means:
- clickpad: INPUT_PROP_BUTTONPAD
- pressurepad: INPUT_PROP_BUTTONPAD + INPUT_PROP_PRESSUREPAD
- pressurepad with configurable haptics:
INPUT_PROP_BUTTONPAD + INPUT_PROP_PRESSUREPAD + FF_HAPTIC
```
We have to keep setting BUTTONPAD on all pressurepads because otherwise
we'd break existing userspace which relies on this.
Cheers,
Peter
^ permalink raw reply
* Re: [PATCH v5 2/2] Input: isa1200 - new driver for Imagis ISA1200
From: Dmitry Torokhov @ 2026-06-16 4:16 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: linux-input, devicetree, linux-kernel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Linus Walleij
In-Reply-To: <CAPVz0n3iCSeT3xJ2XkwZ6PYofwSLkc0gfm+iYo4xbKBkAtihcQ@mail.gmail.com>
Hi Svyatoslav,
On Mon, Jun 15, 2026 at 09:19:27AM +0300, Svyatoslav Ryhel wrote:
> чт, 28 трав. 2026 р. о 08:38 Svyatoslav Ryhel <clamor95@gmail.com> пише:
> >
> > вт, 12 трав. 2026 р. о 13:24 Svyatoslav Ryhel <clamor95@gmail.com> пише:
> > >
> > > From: Linus Walleij <linusw@kernel.org>
> > >
> > > The ISA1200 is a haptic feedback unit from Imagis Technology using two
> > > motors for haptic feedback in mobile phones. Used in many mobile devices
> > > c. 2012 including Samsung Galxy S Advance GT-I9070 (Janice), Samsung Beam
> > > GT-I8350 (Gavini), LG Optimus 4X P880 and LG Optimus Vu P895.
> > >
> > > The exact datasheet for the ISA1200 is not available; all data was modeled
> > > based on available downstream kernel sources for various devices and
> > > fragments of information scattered across the internet.
> > >
> > > Tested-by: Linus Walleij <linusw@kernel.org> # GT-I9070 Janice
> > > Signed-off-by: Linus Walleij <linusw@kernel.org>
> > > Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > ---
> > > drivers/input/misc/Kconfig | 12 +
> > > drivers/input/misc/Makefile | 1 +
> > > drivers/input/misc/isa1200.c | 524 +++++++++++++++++++++++++++++++++++
> > > 3 files changed, 537 insertions(+)
> > > create mode 100644 drivers/input/misc/isa1200.c
> > >
> >
> > Hello Dmitry! Do I need to make any further adjustments to this driver?
>
> Hello Dmitry! Do I need to make any further adjustments to this
> driver? This driver is hanging in LKML for some time already without
> responds from input maintainer. It is still relevant and I would like
> it to move forward.
There were valid sashiko comments on the patch regarding resetting
"level" to 0 and also potential racing conditions, as well as suggestion
to check number of gpios specified in the device tree.
Please see if the following works for you:
diff --git a/drivers/input/misc/isa1200.c b/drivers/input/misc/isa1200.c
index ff82252a08e1..c61adc4b605c 100644
--- a/drivers/input/misc/isa1200.c
+++ b/drivers/input/misc/isa1200.c
@@ -131,6 +131,7 @@ struct isa1200 {
struct work_struct play_work;
struct isa1200_config config;
+ bool suspended;
bool active;
int level;
};
@@ -247,17 +248,21 @@ static void isa1200_stop(struct isa1200 *isa)
isa->supplies);
isa->active = false;
- isa->level = 0;
}
static void isa1200_play_work(struct work_struct *work)
{
struct isa1200 *isa = container_of(work, struct isa1200, play_work);
-
- if (isa->level)
- isa1200_start(isa);
- else
- isa1200_stop(isa);
+ struct input_dev *input = isa->input;
+
+ scoped_guard(mutex_try, &input->mutex) {
+ if (!isa->suspended) {
+ if (isa->level)
+ isa1200_start(isa);
+ else
+ isa1200_stop(isa);
+ }
+ }
}
static int isa1200_vibrator_play_effect(struct input_dev *input, void *data,
@@ -280,7 +285,8 @@ static int isa1200_vibrator_play_effect(struct input_dev *input, void *data,
if (isa->level != level) {
isa->level = level;
- schedule_work(&isa->play_work);
+ if (!READ_ONCE(isa->suspended))
+ schedule_work(&isa->play_work);
}
return 0;
@@ -292,6 +298,7 @@ static void isa1200_vibrator_close(struct input_dev *input)
cancel_work_sync(&isa->play_work);
isa1200_stop(isa);
+ isa->level = 0;
}
static int isa1200_of_probe(struct i2c_client *client)
@@ -331,6 +338,9 @@ static int isa1200_of_probe(struct i2c_client *client)
return dev_err_probe(dev, PTR_ERR(isa->enable_gpios),
"failed to get enable gpios\n");
+ if (isa->enable_gpios && isa->enable_gpios->ndescs > ISA1200_EN_PINS_MAX)
+ return dev_err_probe(dev, -EINVAL, "too many enable gpios\n");
+
ldo_node = device_get_named_child_node(dev, "ldo");
if (!ldo_node)
return dev_err_probe(dev, -ENODEV,
@@ -479,9 +489,9 @@ static int isa1200_suspend(struct device *dev)
guard(mutex)(&isa->input->mutex);
if (input_device_enabled(isa->input)) {
+ WRITE_ONCE(isa->suspended, true);
cancel_work_sync(&isa->play_work);
- if (isa->level)
- isa1200_stop(isa);
+ isa1200_stop(isa);
}
return 0;
@@ -493,9 +503,11 @@ static int isa1200_resume(struct device *dev)
guard(mutex)(&isa->input->mutex);
- if (input_device_enabled(isa->input))
+ if (input_device_enabled(isa->input)) {
+ WRITE_ONCE(isa->suspended, false);
if (isa->level)
- isa1200_start(isa);
+ schedule_work(&isa->play_work);
+ }
return 0;
}
--
Dmitry
^ permalink raw reply related
* Re: [PATCH] Input: yealink - stop URB resubmission on completion error
From: sashiko-bot @ 2026-06-16 3:31 UTC (permalink / raw)
To: Jie Wang; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260616110440.1073059-1-jie.wang@intel.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] The URB completion callbacks return early on any non-zero URB status, preventing resubmission on transient errors (like -EPROTO or -EILSEQ) and permanently breaking the device's polling loop.
Pre-existing issues:
- [High] Concurrent accesses to `yld->ctl_data` from sysfs and the asynchronous URB polling loop cause DMA buffer corruption, and passing a coherent DMA buffer to a synchronous USB API violates DMA mappings.
- [High] Ping-pong URBs can escape `usb_kill_urb()` during `input_close()`, leading to continuous background execution and use-after-free.
--
commit 02a6ac0b5f7fd710af199370b924a27f34ca574c
Author: Jie Wang <jie.wang@intel.com>
Input: yealink - stop URB resubmission on completion error
urb_irq_callback() and urb_ctl_callback() resubmit each other's URBs
regardless of completion status. When the device returns a persistent
error (-EPROTO), this creates an unthrottled resubmission loop that
starves the CPU and triggers RCU stalls.
Stop resubmitting on any non-zero URB status, following the standard
completion pattern used by other USB input drivers.
Reported-by: syzbot+039eab266c6321a174bd@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=039eab266c6321a174bd
Signed-off-by: Jie Wang <jie.wang@intel.com>
> diff --git a/drivers/input/misc/yealink.c b/drivers/input/misc/yealink.c
> index 560f895a00cda..fcf66912f86b6 100644
> --- a/drivers/input/misc/yealink.c
> +++ b/drivers/input/misc/yealink.c
> @@ -405,9 +405,20 @@ static void urb_irq_callback(struct urb *urb)
> struct yealink_dev *yld = urb->context;
> int ret, status = urb->status;
>
> - if (status)
> + switch (status) {
> + case 0:
> + break;
> + case -ECONNRESET:
> + case -ENOENT:
> + case -ESHUTDOWN:
> + dev_dbg(&yld->intf->dev, "%s - urb shutting down with status %d\n",
> + __func__, status);
> + return;
> + default:
> dev_err(&yld->intf->dev, "%s - urb status %d\n",
> __func__, status);
> + return;
> + }
[Severity: High]
Does returning early on any non-zero status prevent resubmission on transient
errors?
If an error like -EPROTO or -EILSEQ is encountered due to electrical noise,
this early return completely drops the URB. Because the driver has no backoff
timer or error retry mechanism, a single transient error will permanently stop
the polling state machine. Sysfs updates and key presses will silently be
ignored until the device is physically replugged.
[ ... ]
> @@ -443,9 +454,20 @@ static void urb_ctl_callback(struct urb *urb)
> struct yealink_dev *yld = urb->context;
> int ret = 0, status = urb->status;
>
> - if (status)
> + switch (status) {
> + case 0:
> + break;
> + case -ECONNRESET:
> + case -ENOENT:
> + case -ESHUTDOWN:
> + dev_dbg(&yld->intf->dev, "%s - urb shutting down with status %d\n",
> + __func__, status);
> + return;
> + default:
> dev_err(&yld->intf->dev, "%s - urb status %d\n",
> __func__, status);
> + return;
> + }
[Severity: High]
Will the same issue happen here, breaking the ping-pong URB loop on transient
USB errors and permanently freezing the device state machine?
[Severity: High]
This is a pre-existing issue, but does concurrent access to the shared control
buffer from sysfs cause DMA corruption?
Looking at yealink_set_ringtone(), it modifies the shared control buffer
without locking:
drivers/input/misc/yealink.c:yealink_set_ringtone() {
...
memset(yld->ctl_data, 0, sizeof(*(yld->ctl_data)));
yld->ctl_data->cmd = CMD_RING_VOLUME;
...
}
Since yld->ctl_data is used as the dedicated DMA transfer buffer for the
asynchronous control URB which runs continuously in a background loop, modifying
it from sysfs without locking against the async loop could corrupt an active DMA
transfer.
Additionally, yealink_set_ringtone() synchronously sends this buffer:
drivers/input/misc/yealink.c:yealink_cmd() {
...
return usb_control_msg(yld->udev,
usb_sndctrlpipe(yld->udev, 0),
USB_REQ_SET_CONFIGURATION,
USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_OUT,
0x200, 3,
p, sizeof(*p),
USB_CTRL_SET_TIMEOUT);
}
Because yld->ctl_data is allocated with usb_alloc_coherent(), does this violate
the DMA API by attempting to dma_map_single() already-coherent memory?
[Severity: High]
This is a pre-existing issue, but can ping-pong URBs escape the shutdown
sequence and cause a use-after-free?
The driver attempts to stop the ping-pong URBs in input_close() by setting a
flag and killing them:
drivers/input/misc/yealink.c:input_close() {
...
yld->shutdown = 1;
usb_kill_urb(yld->urb_ctl);
usb_kill_urb(yld->urb_irq);
...
}
If urb_irq_callback() evaluates !yld->shutdown as true but is preempted:
drivers/input/misc/yealink.c:urb_irq_callback() {
...
if (!yld->shutdown) {
ret = usb_submit_urb(yld->urb_ctl, GFP_ATOMIC);
...
}
And input_close() then sets shutdown = 1, kills urb_ctl (which isn't in flight
yet), and blocks killing urb_irq. When urb_irq_callback() resumes, it submits
urb_ctl. Because usb_kill_urb(urb_ctl) already returned and cleared the reject
count, the submission succeeds.
If the device is disconnected, usb_cleanup() frees the yealink_dev structure,
but the runaway URB will dereference it upon completion. Should this use
usb_poison_urb() instead to reliably stop the loop?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616110440.1073059-1-jie.wang@intel.com?part=1
^ permalink raw reply
* [PATCH] Input: yealink - stop URB resubmission on completion error
From: Jie Wang @ 2026-06-16 11:04 UTC (permalink / raw)
To: Henk Vergonet, Dmitry Torokhov
Cc: linux-input, linux-kernel, syzbot+039eab266c6321a174bd,
syzkaller-bugs
In-Reply-To: <68bba4fb.050a0220.192772.01a4.GAE@google.com>
urb_irq_callback() and urb_ctl_callback() resubmit each other's URBs
regardless of completion status. When the device returns a persistent
error (-EPROTO), this creates an unthrottled resubmission loop that
starves the CPU and triggers RCU stalls.
Stop resubmitting on any non-zero URB status, following the standard
completion pattern used by other USB input drivers.
Reported-by: syzbot+039eab266c6321a174bd@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=039eab266c6321a174bd
Signed-off-by: Jie Wang <jie.wang@intel.com>
---
drivers/input/misc/yealink.c | 26 ++++++++++++++++++++++----
1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/drivers/input/misc/yealink.c b/drivers/input/misc/yealink.c
index 8786ed8b3565..bbfa1a9c23d1 100644
--- a/drivers/input/misc/yealink.c
+++ b/drivers/input/misc/yealink.c
@@ -414,9 +414,20 @@ static void urb_irq_callback(struct urb *urb)
struct yealink_dev *yld = urb->context;
int ret, status = urb->status;
- if (status)
+ switch (status) {
+ case 0:
+ break;
+ case -ECONNRESET:
+ case -ENOENT:
+ case -ESHUTDOWN:
+ dev_dbg(&yld->intf->dev, "%s - urb shutting down with status %d\n",
+ __func__, status);
+ return;
+ default:
dev_err(&yld->intf->dev, "%s - urb status %d\n",
__func__, status);
+ return;
+ }
switch (yld->irq_data->cmd) {
case CMD_KEYPRESS:
@@ -452,9 +463,20 @@ static void urb_ctl_callback(struct urb *urb)
struct yealink_dev *yld = urb->context;
int ret = 0, status = urb->status;
- if (status)
+ switch (status) {
+ case 0:
+ break;
+ case -ECONNRESET:
+ case -ENOENT:
+ case -ESHUTDOWN:
+ dev_dbg(&yld->intf->dev, "%s - urb shutting down with status %d\n",
+ __func__, status);
+ return;
+ default:
dev_err(&yld->intf->dev, "%s - urb status %d\n",
__func__, status);
+ return;
+ }
switch (yld->ctl_data->cmd) {
case CMD_KEYPRESS:
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v3] HID: Expose LattePanda IOTA UPS as a power_supply device
From: Andrew Maney @ 2026-06-16 2:22 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: jikos, linux-kernel, linux-input
In-Reply-To: <ah2iNcGB-xsEHC0r@beelink>
Hi, sorry for the late response. I've gotten quite busy recently.
I'll work in the kernel implementation, it will probably be easier to get going
with the cleanup code you have. In the meantime, I'll look deeper into the
HID-UPS specification so I can write a proper implementation.
Thanks,
Andrew
On Mon, Jun 1, 2026 at 11:24 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> On May 21 2026, Andrew Maney wrote:
> > On Thu, May 21, 2026 at 2:54 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
> > >
> > > On May 20 2026, Andrew Maney wrote:
> > > > This driver exposes the DFRobot LattePanda IOTA UPS board as a standard
> > > > power_supply device, allowing desktop environments and power management
> > > > tools such as UPower and systemd-logind to display battery status,
> > > > remaining capacity, and charging status without any special
> > > > configuration. It also enables automatic suspend or shutdown on low
> > > > battery and power profile configuration via any tool that supports the
> > > > standard power_supply interface.
> > > >
> > > > The UPS presents itself as an Arduino Leonardo HID device running custom
> > > > firmware (VID 0x2341, PID 0x8036). It reports status and capacity via
> > > > HID reports 0x07 and 0x0C respectively.
> > >
> > > As sashiko detected, you are using a standard Arduino Leonardo and not
> > > making any specific detections. So I was thinking that maybe we should
> > > implement that feature as a HID-BPF program on top of the generic HID
> > > handling.
> > >
> > > However, the handling of battery supplies in the HID generic core is not
> > > entirely filling all of the requirements here, but that's something I
> > > wanted to do for a couple of month but I have been swamped with other
> > > projects.
> > >
> > > Anyway, I wanted to understand why this product was using a generic
> > > Leonardo PID, and: https://wiki.dfrobot.com/dfr1247#tech_specs
> > >
> > > "it leverages the standard HID-UPS protocol to be natively recognized by
> > > Windows as a battery-powered device"
> > >
> > > So. Instead of working on a custom driver, why not simply implement (or
> > > finish the generic implementation) of HID-UPS in hid-input.c?
> > >
> > > Cheers,
> > > Benjamin
> > >
> >
> > Hi Benjamin,
> >
> > Thank you for the feedback! I'll look into the HID-UPS implementation or a
> > HID-BPF program.
> >
> > I should mention that the kernel does already detect the IOTA UPS via the
> > generic HID-UPS support, but the updates that it receives are so slow that
> > it becomes practically unusable for a device with smaller capacity batteries
> > like this. Changes can take several minutes or more to be detected. I
> > originally went with a driver that reads the HID reports directly because it
> > was providing updates far faster.
> >
> > I implemented a check in driver v4 that verifies that the device's HID reports
> > are what's expected before binding to it. In theory, that should prevent the
> > driver from binding to other Arduino Leonardo devices, but I agree that fixing
> > the generic HID-UPS implementation would be a better long-term solution.
> >
> > Which approach do you think would be more suitable for this, completing the
> > HID-UPS implementation or a HID-BPF program? I'm happy to work on either,
> > though as a first-time kernel contributor I'd appreciate any pointers
> > on where to
> > start.
>
> Honestly, if the device is compliant with HID-UPS, you should work in
> the kernel implementation. A HID-BPF would be used to fix device issues,
> but if the kernel is not correct enough you can't do much in just a
> HID-BPF.
>
> If I'm not wrong, the HID-UPS implementation we have currently is the
> hid_battery_* functions. And that implementation assumes a HID connected
> device is a separate device, not a power supply of the main machine. So
> you'll have to deal with a few new HID fields.
>
> Also, I've started the cleanup work on the hid_battery code a couple of
> months ago and never got the chance to send it. Maybe I should send it
> so you can have a better environment with testing.
>
> Cheers,
> Benjamin
>
> >
> > Thanks,
> > Andrew
> >
> > > >
> > > > The charge limit (80% or 100%) is configured via a physical DIP switch
> > > > on the UPS board and cannot be detected automatically. Userspace can
> > > > inform the driver of the configured limit via
> > > > charge_control_end_threshold.
> > > >
> > > > ---
> > > >
> > > > Changes in v3:
> > > > - Deferred power_supply registration to workqueue to avoid blocking probe
> > > > - Fixed kernel panic when instantiated via uhid by checking hid_is_usb()
> > > > before dereferencing USB-specific structures
> > > >
> > > > - Fixed ERR_PTR dereference in raw_event by only assigning ups->psu on
> > > > successful registration
> > > >
> > > > - Fixed data race on ups->charge_limit using spin_lock_irqsave()
> > > > - Removed TIME_TO_EMPTY_NOW and TIME_TO_FULL_NOW properties to avoid
> > > > spurious shutdowns
> > > >
> > > > - Changed plugged-in but not charging state from FULL to NOT_CHARGING
> > > > - Used devm_kasprintf() for a unique sysfs name in order to support
> > > > multiple devices
> > > >
> > > > - Added POWER_SUPPLY and HIDRAW dependencies to Kconfig
> > > > - Used %pe for more human-readable error messages
> > > >
> > > > Changes in v2:
> > > > - Rebased on top of the current tree
> > > > - Moved vendor and device IDs to drivers/hid/hid-ids.h
> > > > - Added Kconfig entry under HID bus support -> Special HID drivers
> > > > - Added build rule to drivers/hid/Makefile
> > > >
> > > > Signed-off-by: Andrew Maney <andrewmaney05@gmail.com>
> > > > ---
> > > > MAINTAINERS | 6 +
> > > > drivers/hid/Kconfig | 10 +
> > > > drivers/hid/Makefile | 1 +
> > > > drivers/hid/hid-ids.h | 3 +
> > > > drivers/hid/hid-lattepanda-iota-ups.c | 409 ++++++++++++++++++++++++++
> > > > 5 files changed, 429 insertions(+)
> > > > create mode 100644 drivers/hid/hid-lattepanda-iota-ups.c
> > > >
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 10e825318..d80721c2c 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -11416,6 +11416,12 @@ F: include/uapi/linux/hid*
> > > > F: samples/hid/
> > > > F: tools/testing/selftests/hid/
> > > >
> > > > +HID LATTEPANDA IOTA UPS DRIVER
> > > > +M: Andrew Maney <andrewmaney05@gmail.com>
> > > > +L: linux-input@vger.kernel.org
> > > > +S: Maintained
> > > > +F: drivers/hid/hid-lattepanda-iota-ups.c
> > > > +
> > > > HID LOGITECH DRIVERS
> > > > R: Filipe Laíns <lains@riseup.net>
> > > > L: linux-input@vger.kernel.org
> > > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > > > index ff2f580b6..21ffc2fd0 100644
> > > > --- a/drivers/hid/Kconfig
> > > > +++ b/drivers/hid/Kconfig
> > > > @@ -510,6 +510,16 @@ config HID_KYSONA
> > > > Say Y here if you have a Kysona M600 mouse
> > > > and want to be able to read its battery capacity.
> > > >
> > > > +config HID_LATTEPANDA_IOTA_UPS
> > > > + tristate "LattePanda IOTA UPS"
> > > > + depends on USB_HID && USB_HIDDEV && X86 && POWER_SUPPLY
> > > > + help
> > > > + Support for the LattePanda IOTA UPS (DFRobot, VID 0x2341 PID 0x8036).
> > > > + Exposes the battery status and capacity via the power_supply interface.
> > > > +
> > > > + To compile as a module, choose M here: the module will be
> > > > + called hid-lattepanda-iota-ups.
> > > > +
> > > > config HID_UCLOGIC
> > > > tristate "UC-Logic"
> > > > depends on USB_HID
> > > > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > > > index 0597fd6a4..d7ad3fc8f 100644
> > > > --- a/drivers/hid/Makefile
> > > > +++ b/drivers/hid/Makefile
> > > > @@ -74,6 +74,7 @@ obj-$(CONFIG_HID_KENSINGTON) += hid-kensington.o
> > > > obj-$(CONFIG_HID_KEYTOUCH) += hid-keytouch.o
> > > > obj-$(CONFIG_HID_KYE) += hid-kye.o
> > > > obj-$(CONFIG_HID_KYSONA) += hid-kysona.o
> > > > +obj-$(CONFIG_HID_LATTEPANDA_IOTA_UPS) += hid-lattepanda-iota-ups.o
> > > > obj-$(CONFIG_HID_LCPOWER) += hid-lcpower.o
> > > > obj-$(CONFIG_HID_LENOVO) += hid-lenovo.o
> > > > obj-$(CONFIG_HID_LENOVO_GO) += hid-lenovo-go.o
> > > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > > > index 4657d96fb..6ded2c943 100644
> > > > --- a/drivers/hid/hid-ids.h
> > > > +++ b/drivers/hid/hid-ids.h
> > > > @@ -859,6 +859,9 @@
> > > > #define USB_DEVICE_ID_LD_HYBRID 0x2090
> > > > #define USB_DEVICE_ID_LD_HEATCONTROL 0x20A0
> > > >
> > > > +#define USB_VENDOR_ID_LATTEPANDA_IOTA 0x2341
> > > > +#define USB_DEVICE_ID_LATTEPANDA_IOTA_UPS 0x8036
> > > > +
> > > > #define USB_VENDOR_ID_LENOVO 0x17ef
> > > > #define USB_DEVICE_ID_LENOVO_TPKBD 0x6009
> > > > #define USB_DEVICE_ID_LENOVO_CUSBKBD 0x6047
> > > > diff --git a/drivers/hid/hid-lattepanda-iota-ups.c b/drivers/hid/hid-lattepanda-iota-ups.c
> > > > new file mode 100644
> > > > index 000000000..f5d522695
> > > > --- /dev/null
> > > > +++ b/drivers/hid/hid-lattepanda-iota-ups.c
> > > > @@ -0,0 +1,409 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +#include <linux/power_supply.h>
> > > > +#include <linux/completion.h>
> > > > +#include <linux/workqueue.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/spinlock.h>
> > > > +#include <linux/hid.h>
> > > > +#include <linux/usb.h>
> > > > +#include "hid-ids.h"
> > > > +
> > > > +#define REPORT_ID_CAPACITY 0x0C
> > > > +#define REPORT_ID_STATUS 0x07
> > > > +
> > > > +#define STATUS_DISCHARGING BIT(1)
> > > > +#define STATUS_PLUGGED_IN BIT(0)
> > > > +#define STATUS_CHARGING BIT(2)
> > > > +
> > > > +MODULE_AUTHOR("Andrew Maney");
> > > > +MODULE_DESCRIPTION("LattePanda IOTA UPS power supply driver");
> > > > +MODULE_LICENSE("GPL");
> > > > +
> > > > +struct iota_ups {
> > > > + struct power_supply_desc psu_desc;
> > > > + struct power_supply *psu;
> > > > + struct hid_device *hiddev;
> > > > + spinlock_t lock; /* Protects cached HID report values */
> > > > +
> > > > + /* Cached values that are updated from HID reports */
> > > > + bool plugged_in;
> > > > + char serial[64];
> > > > + int charge_limit;
> > > > + int psu_status;
> > > > + int capacity;
> > > > +
> > > > + /*
> > > > + * Wait for both status and capacity reports before registering
> > > > + * with the power_supply core, so initial values are correct and
> > > > + * not erroneous.
> > > > + */
> > > > + struct completion got_initial_data;
> > > > + struct work_struct register_work;
> > > > + bool got_capacity;
> > > > + bool data_ready;
> > > > + bool got_status;
> > > > +};
> > > > +
> > > > +static enum power_supply_property iota_ups_properties[] = {
> > > > + POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD,
> > > > + POWER_SUPPLY_PROP_SERIAL_NUMBER,
> > > > + POWER_SUPPLY_PROP_MANUFACTURER,
> > > > + POWER_SUPPLY_PROP_MODEL_NAME,
> > > > + POWER_SUPPLY_PROP_TECHNOLOGY,
> > > > + POWER_SUPPLY_PROP_CAPACITY,
> > > > + POWER_SUPPLY_PROP_PRESENT,
> > > > + POWER_SUPPLY_PROP_ONLINE,
> > > > + POWER_SUPPLY_PROP_STATUS,
> > > > + POWER_SUPPLY_PROP_SCOPE,
> > > > +};
> > > > +
> > > > +static const struct hid_device_id iota_ups_devices[] = {
> > > > + { HID_USB_DEVICE(USB_VENDOR_ID_LATTEPANDA_IOTA,
> > > > + USB_DEVICE_ID_LATTEPANDA_IOTA_UPS) },
> > > > + { }
> > > > +};
> > > > +MODULE_DEVICE_TABLE(hid, iota_ups_devices);
> > > > +
> > > > +static int iota_ups_get_property(struct power_supply *supply,
> > > > + enum power_supply_property psp,
> > > > + union power_supply_propval *val)
> > > > +{
> > > > + struct iota_ups *ups = power_supply_get_drvdata(supply);
> > > > + unsigned long flags;
> > > > +
> > > > + spin_lock_irqsave(&ups->lock, flags);
> > > > +
> > > > + switch (psp) {
> > > > + case POWER_SUPPLY_PROP_STATUS:
> > > > + val->intval = ups->psu_status;
> > > > + break;
> > > > +
> > > > + /* Remaining capacity as a percentage from 0 to 100 */
> > > > + case POWER_SUPPLY_PROP_CAPACITY:
> > > > + val->intval = ups->capacity;
> > > > + break;
> > > > +
> > > > + /* The UPS is always present if the driver is loaded */
> > > > + case POWER_SUPPLY_PROP_PRESENT:
> > > > + val->intval = 1;
> > > > + break;
> > > > +
> > > > + /* Whether mains power is connected */
> > > > + case POWER_SUPPLY_PROP_ONLINE:
> > > > + val->intval = ups->plugged_in ? 1 : 0;
> > > > + break;
> > > > +
> > > > + /*
> > > > + * The UPS board supplies power to the IOTA and any
> > > > + * peripherals connected to it, therefore its scope
> > > > + * is system-wide.
> > > > + */
> > > > + case POWER_SUPPLY_PROP_SCOPE:
> > > > + val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
> > > > + break;
> > > > +
> > > > + /* V1.0 only accepts 18650 Li-ion cells */
> > > > + case POWER_SUPPLY_PROP_TECHNOLOGY:
> > > > + val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
> > > > + break;
> > > > +
> > > > + /* 80% or 100%, configured via a DIP switch on the UPS board */
> > > > + case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
> > > > + val->intval = ups->charge_limit;
> > > > + break;
> > > > +
> > > > + case POWER_SUPPLY_PROP_MANUFACTURER:
> > > > + val->strval = "DFRobot";
> > > > + break;
> > > > +
> > > > + case POWER_SUPPLY_PROP_MODEL_NAME:
> > > > + val->strval = "LattePanda IOTA UPS";
> > > > + break;
> > > > +
> > > > + /* Retrieved from the USB descriptor */
> > > > + case POWER_SUPPLY_PROP_SERIAL_NUMBER:
> > > > + val->strval = ups->serial;
> > > > + break;
> > > > +
> > > > + default:
> > > > + spin_unlock_irqrestore(&ups->lock, flags);
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + spin_unlock_irqrestore(&ups->lock, flags);
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int iota_ups_set_property(struct power_supply *supply,
> > > > + enum power_supply_property psp,
> > > > + const union power_supply_propval *val)
> > > > +{
> > > > + struct iota_ups *ups = power_supply_get_drvdata(supply);
> > > > +
> > > > + if (psp == POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD) {
> > > > + unsigned long flags;
> > > > +
> > > > + /*
> > > > + * V1.0 supports 80% and 100% charge limits only, which is
> > > > + * set via a DIP switch on the board. This property allows
> > > > + * userspace to inform the driver which limit is configured.
> > > > + */
> > > > + if (val->intval != 80 && val->intval != 100)
> > > > + return -EINVAL;
> > > > +
> > > > + spin_lock_irqsave(&ups->lock, flags);
> > > > + ups->charge_limit = val->intval;
> > > > + spin_unlock_irqrestore(&ups->lock, flags);
> > > > + return 0;
> > > > + }
> > > > +
> > > > + return -EINVAL;
> > > > +}
> > > > +
> > > > +static int iota_ups_property_is_writable(struct power_supply *supply,
> > > > + enum power_supply_property psp)
> > > > +{
> > > > + return psp == POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD;
> > > > +}
> > > > +
> > > > +static int iota_ups_raw_event(struct hid_device *hdev,
> > > > + struct hid_report *report,
> > > > + u8 *data, int size)
> > > > +{
> > > > + struct iota_ups *ups = hid_get_drvdata(hdev);
> > > > + unsigned long flags;
> > > > + bool changed = false;
> > > > +
> > > > + /* All of the UPS's reports are at least 2 bytes */
> > > > + if (size < 2)
> > > > + return 0;
> > > > +
> > > > + spin_lock_irqsave(&ups->lock, flags);
> > > > +
> > > > + switch (data[0]) {
> > > > + case REPORT_ID_STATUS: {
> > > > + u8 status = data[1];
> > > > + int new_status;
> > > > + bool plugged_in = !!(status & STATUS_PLUGGED_IN);
> > > > +
> > > > + /*
> > > > + * The UPS status is determined as follows:
> > > > + * Battery full:
> > > > + * UPS is plugged in
> > > > + * Battery is at full capacity
> > > > + *
> > > > + * Battery charging:
> > > > + * UPS is plugged in
> > > > + * Battery is not at full capacity
> > > > + *
> > > > + * Battery discharging:
> > > > + * UPS is not plugged in
> > > > + *
> > > > + * Battery not charging:
> > > > + * UPS is plugged in
> > > > + * UPS has halted charging for some reason
> > > > + *
> > > > + * Unknown:
> > > > + * None of the above conditions are met
> > > > + */
> > > > + if (status & STATUS_CHARGING) {
> > > > + if (ups->capacity >= ups->charge_limit)
> > > > + new_status = POWER_SUPPLY_STATUS_FULL;
> > > > + else
> > > > + new_status = POWER_SUPPLY_STATUS_CHARGING;
> > > > +
> > > > + } else if (status & STATUS_DISCHARGING) {
> > > > + new_status = POWER_SUPPLY_STATUS_DISCHARGING;
> > > > +
> > > > + } else if (plugged_in) {
> > > > + new_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> > > > +
> > > > + } else {
> > > > + new_status = POWER_SUPPLY_STATUS_UNKNOWN;
> > > > + }
> > > > +
> > > > + if (new_status != ups->psu_status ||
> > > > + plugged_in != ups->plugged_in) {
> > > > + ups->plugged_in = plugged_in;
> > > > + ups->psu_status = new_status;
> > > > + changed = true;
> > > > + }
> > > > +
> > > > + ups->got_status = true;
> > > > + break;
> > > > + }
> > > > +
> > > > + case REPORT_ID_CAPACITY: {
> > > > + int new_cap = clamp((int)data[1], 0, 100);
> > > > +
> > > > + if (new_cap != ups->capacity) {
> > > > + ups->capacity = new_cap;
> > > > + changed = true;
> > > > + }
> > > > +
> > > > + ups->got_capacity = true;
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + /*
> > > > + * Signal that the UPS is ready to be registered because we have
> > > > + * received both capacity and status reports.
> > > > + */
> > > > + if (!ups->data_ready && ups->got_status && ups->got_capacity) {
> > > > + ups->data_ready = true;
> > > > + complete(&ups->got_initial_data);
> > > > + }
> > > > +
> > > > + spin_unlock_irqrestore(&ups->lock, flags);
> > > > +
> > > > + /*
> > > > + * Notify the power_supply core outside the spinlock to avoid
> > > > + * a deadlock; power_supply_changed() may call back into
> > > > + * get_property() which acquires the same lock.
> > > > + */
> > > > + if (changed && ups->psu)
> > > > + power_supply_changed(ups->psu);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void iota_ups_register_work(struct work_struct *work)
> > > > +{
> > > > + struct iota_ups *ups = container_of(work, struct iota_ups, register_work);
> > > > + struct power_supply_config psu_config = {};
> > > > + struct power_supply *psu;
> > > > +
> > > > + /*
> > > > + * Wait for both status and capacity reports before registering.
> > > > + * The device sends reports every ~1 second, so 3 seconds is safe.
> > > > + * We wait here in order to prevent registration in an unknown
> > > > + * state, since this could cause emergency shutdowns or other
> > > > + * undesired effects.
> > > > + */
> > > > + wait_for_completion_timeout(&ups->got_initial_data,
> > > > + msecs_to_jiffies(3000));
> > > > +
> > > > + /* Configure the UPS's power supply properties */
> > > > + ups->psu_desc.name = devm_kasprintf(&ups->hiddev->dev, GFP_KERNEL,
> > > > + "lattepanda-iota-ups.%s",
> > > > + dev_name(&ups->hiddev->dev));
> > > > +
> > > > + if (!ups->psu_desc.name) {
> > > > + hid_err(ups->hiddev, "failed to allocate power supply name\n");
> > > > + return;
> > > > + }
> > > > +
> > > > + ups->psu_desc.property_is_writeable = iota_ups_property_is_writable;
> > > > + ups->psu_desc.num_properties = ARRAY_SIZE(iota_ups_properties);
> > > > + ups->psu_desc.get_property = iota_ups_get_property;
> > > > + ups->psu_desc.set_property = iota_ups_set_property;
> > > > + ups->psu_desc.properties = iota_ups_properties;
> > > > + ups->psu_desc.type = POWER_SUPPLY_TYPE_BATTERY;
> > > > + psu_config.drv_data = ups;
> > > > +
> > > > + /* Register the UPS as a power_supply device */
> > > > + psu = devm_power_supply_register(&ups->hiddev->dev, &ups->psu_desc, &psu_config);
> > > > + if (IS_ERR(psu)) {
> > > > + hid_err(ups->hiddev, "power supply registration failed: %pe\n", psu);
> > > > + return;
> > > > + }
> > > > +
> > > > + /*
> > > > + * Finally, notify the power_supply core so userspace reads the correct
> > > > + * initial state immediately after registration.
> > > > + */
> > > > + ups->psu = psu;
> > > > + power_supply_changed(ups->psu);
> > > > + hid_info(ups->hiddev, "LattePanda IOTA UPS registered as a power_supply device\n");
> > > > +}
> > > > +
> > > > +static int iota_ups_probe(struct hid_device *hdev,
> > > > + const struct hid_device_id *id)
> > > > +{
> > > > + struct iota_ups *ups;
> > > > + int ret;
> > > > +
> > > > + ups = devm_kzalloc(&hdev->dev, sizeof(*ups), GFP_KERNEL);
> > > > + if (!ups)
> > > > + return -ENOMEM;
> > > > +
> > > > + ups->hiddev = hdev;
> > > > + ups->psu_status = POWER_SUPPLY_STATUS_UNKNOWN;
> > > > +
> > > > + /* 50% is a safe default if wait_for_completion_timeout() times out. */
> > > > + ups->capacity = 50;
> > > > +
> > > > + /*
> > > > + * Default to 100% to prevent unexpected shutdowns.
> > > > + * Userspace can update this via charge_control_end_threshold.
> > > > + */
> > > > + ups->charge_limit = 100;
> > > > +
> > > > + init_completion(&ups->got_initial_data);
> > > > + spin_lock_init(&ups->lock);
> > > > + hid_set_drvdata(hdev, ups);
> > > > +
> > > > + /*
> > > > + * Retrieve the UPS's serial number from the USB descriptor. If the device is not
> > > > + * a USB device, we can use the unique device identifier as the serial number.
> > > > + */
> > > > + if (hid_is_usb(hdev)) {
> > > > + struct usb_device *udev = to_usb_device(hdev->dev.parent->parent);
> > > > +
> > > > + if (udev->serial)
> > > > + strscpy(ups->serial, udev->serial, sizeof(ups->serial));
> > > > + else
> > > > + strscpy(ups->serial, "Unknown", sizeof(ups->serial));
> > > > + } else {
> > > > + if (*hdev->uniq)
> > > > + strscpy(ups->serial, hdev->uniq, sizeof(ups->serial));
> > > > + else
> > > > + strscpy(ups->serial, "Unknown", sizeof(ups->serial));
> > > > + }
> > > > +
> > > > + ret = hid_parse(hdev);
> > > > + if (ret) {
> > > > + hid_err(hdev, "HID parse failed: %pe\n", ERR_PTR(ret));
> > > > + return ret;
> > > > + }
> > > > +
> > > > + ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> > > > + if (ret) {
> > > > + hid_err(hdev, "HID hw start failed: %pe\n", ERR_PTR(ret));
> > > > + return ret;
> > > > + }
> > > > +
> > > > + ret = hid_hw_open(hdev);
> > > > + if (ret) {
> > > > + hid_err(hdev, "HID hw open failed: %pe\n", ERR_PTR(ret));
> > > > + goto err_stop;
> > > > + }
> > > > +
> > > > + /* Probe for the UPS in a worker queue so we don't halt the enumeration thread */
> > > > + INIT_WORK(&ups->register_work, iota_ups_register_work);
> > > > + schedule_work(&ups->register_work);
> > > > + return 0;
> > > > +
> > > > +err_stop:
> > > > + hid_hw_stop(hdev);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static void iota_ups_remove(struct hid_device *hdev)
> > > > +{
> > > > + struct iota_ups *ups = hid_get_drvdata(hdev);
> > > > +
> > > > + cancel_work_sync(&ups->register_work);
> > > > + hid_hw_close(hdev);
> > > > + hid_hw_stop(hdev);
> > > > +}
> > > > +
> > > > +static struct hid_driver iota_ups_driver = {
> > > > + .name = "lattepanda-iota-ups",
> > > > + .id_table = iota_ups_devices,
> > > > + .probe = iota_ups_probe,
> > > > + .remove = iota_ups_remove,
> > > > + .raw_event = iota_ups_raw_event,
> > > > +};
> > > > +module_hid_driver(iota_ups_driver);
> > > > --
> > > > 2.54.0
> > > >
> > > >
> >
^ permalink raw reply
* Re: [PATCH v4 1/5] HID: asus: refactor the two workqueues and init sequence
From: Denis Benato @ 2026-06-16 1:00 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: linux-kernel, linux-input, Benjamin Tissoires, Jiri Kosina,
Luke D . Jones, Mateusz Schyboll, Denis Benato, Connor Belli,
sahiko-bot
In-Reply-To: <CAGwozwHC8ni6BCeEigEt0NQL+Ys2CqLP4_tVSpfA_zF3x52y-A@mail.gmail.com>
On 6/15/26 23:49, Antheas Kapenekakis wrote:
> On Mon, 15 Jun 2026 at 18:51, Denis Benato <denis.benato@linux.dev> wrote:
>> Multiple issues have been found within the hid-asus driver:
>> - unchecked size in asus_raw_event()
>> - unclean teardown of asus_probe on failure
>> - possible use-after-free in asus_probe
>> - multiple workqueue used for jobs where one was enough
>> - sleeping calls in atomic context
>> - packets of incorrect size being sent to the keyboard controller
>>
>> Join the two workqueues into one reusing the stopping mechanism
>> of the brightness workqueue, use the joined workqueue to also
>> move the asus_wmi_send_event() sleeping call away from atomic
>> context and add a size check in asus_raw_event().
>>
>> Fixes: f631011e36b8 ("HID: hid-asus: Implement fn lock for Asus ProArt P16")
>> Fixes: 1489a34e97ef ("HID: asus: Implement Fn+F5 fan control key handler")
>> Fixes: b34b5945a769 ("HID: asus: listen to the asus-wmi brightness device instead of creating one")
> I'm not sure b34b5945a769 caused an issue, perhaps drop it from fixes.
> Does f631011e36b8 do a WMI from a hid interrupt? If not, perhaps it
> does not need to be changed
Maybe I confused some tags.
> But moreso, this patch is too large and will take a bit to review. Can
> you send it separately after the rest of the series goes through
> unless users report warnings?
I agree on it being large. I tried doing it by pieces, but got a bunch of compile
errors.
I ended up deciding trying it when after a few hours I had to modify code
I did not want to modify (just yet) just for the thing to compile.
Probably result of frustration more than technical limitation...
I am fine with waiting and will be unavailable for a few days,
in the meanwhile I hope in some input from hid maintainers
too.
> Antheas
>
>> Reported-by: sahiko-bot@kernel.org
>> Closes: https://lore.kernel.org/all/20260613154732.60A4B1F000E9@smtp.kernel.org/
>> Signed-off-by: Denis Benato <denis.benato@linux.dev>
>> ---
>> drivers/hid/hid-asus.c | 393 +++++++++++++++++++++++++++++------------
>> 1 file changed, 282 insertions(+), 111 deletions(-)
>>
>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>> index d34d74df3dc0..05ca6665e0a4 100644
>> --- a/drivers/hid/hid-asus.c
>> +++ b/drivers/hid/hid-asus.c
>> @@ -109,11 +109,36 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>>
>> #define TRKID_SGN ((TRKID_MAX + 1) >> 1)
>>
>> -struct asus_kbd_leds {
>> - struct asus_hid_listener listener;
>> +enum asus_work_action_type {
>> + FN_LOCK_SYNC,
>> + BRIGHTNESS_SET,
>> + WMI_FAN,
>> +};
>> +
>> +struct hid_raw_event_data {
>> + u8 report_data[FEATURE_KBD_REPORT_SIZE];
>> + size_t report_size;
>> +};
>> +
>> +struct asus_work_action {
>> + struct list_head node;
>> + enum asus_work_action_type type;
>> + union {
>> + /* Data for BRIGHTNESS_SET */
>> + unsigned int brightness;
>> +
>> + /* Data for FN_LOCK_SYNC */
>> + bool fn_lock;
>> +
>> + /* Data for WMI_FAN */
>> + struct hid_raw_event_data fan_hid_data;
>> + } data;
>> +};
>> +
>> +struct asus_worker {
>> struct hid_device *hdev;
>> struct work_struct work;
>> - unsigned int brightness;
>> + struct list_head actions;
>> spinlock_t lock;
>> bool removed;
>> };
>> @@ -133,7 +158,8 @@ struct asus_drvdata {
>> struct hid_device *hdev;
>> struct input_dev *input;
>> struct input_dev *tp_kbd_input;
>> - struct asus_kbd_leds *kbd_backlight;
>> + struct asus_worker *worker;
>> + unsigned int kbd_backlight_brightness;
>> const struct asus_touchpad_info *tp;
>> struct power_supply *battery;
>> struct power_supply_desc battery_desc;
>> @@ -141,7 +167,7 @@ struct asus_drvdata {
>> int battery_stat;
>> bool battery_in_query;
>> unsigned long battery_next_query;
>> - struct work_struct fn_lock_sync_work;
>> + struct asus_hid_listener listener;
>> bool fn_lock;
>> };
>>
>> @@ -211,6 +237,29 @@ static const u8 asus_report_id_init[] = {
>> FEATURE_KBD_LED_REPORT_ID2
>> };
>>
>> +/*
>> + * Send events to asus-wmi driver for handling special keys
>> + */
>> +static int asus_wmi_send_event(struct asus_drvdata *drvdata, u8 code)
>> +{
>> + int err;
>> + u32 retval;
>> +
>> + err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
>> + ASUS_WMI_METHODID_NOTIF, code, &retval);
>> + if (err) {
>> + pr_warn("Failed to notify asus-wmi: %d\n", err);
>> + return err;
>> + }
>> +
>> + if (retval != 0) {
>> + pr_warn("Failed to notify asus-wmi (retval): 0x%x\n", retval);
>> + return -EIO;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static void asus_report_contact_down(struct asus_drvdata *drvdat,
>> int toolType, u8 *data)
>> {
>> @@ -331,25 +380,71 @@ static int asus_e1239t_event(struct asus_drvdata *drvdat, u8 *data, int size)
>> }
>>
>> /*
>> - * Send events to asus-wmi driver for handling special keys
>> + * Used in atomic contexts to schedule work involving sleeps operations or
>> + * asus-wmi interactions.
>> + *
>> + * Caller is responsible to store relevant data in the structure to carry out
>> + * the required action.
>> + *
>> + * This function must be called while the spin lock protecting the workqueue
>> + * is already being held.
>> */
>> -static int asus_wmi_send_event(struct asus_drvdata *drvdata, u8 code)
>> +static void asus_worker_schedule(struct asus_worker *worker, struct asus_work_action *action)
>> {
>> - int err;
>> - u32 retval;
>> -
>> - err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
>> - ASUS_WMI_METHODID_NOTIF, code, &retval);
>> - if (err) {
>> - pr_warn("Failed to notify asus-wmi: %d\n", err);
>> - return err;
>> + if (worker->removed) {
>> + kfree(action);
>> + return;
>> }
>>
>> - if (retval != 0) {
>> - pr_warn("Failed to notify asus-wmi (retval): 0x%x\n", retval);
>> - return -EIO;
>> + list_add_tail(&action->node, &worker->actions);
>> + schedule_work(&worker->work);
>> +}
>> +
>> +static int asus_kbd_fn_lock_set(struct asus_drvdata *drvdata, bool enabled)
>> +{
>> + struct asus_work_action *action;
>> + unsigned long flags;
>> +
>> + action = kzalloc_obj(struct asus_work_action);
>> + if (!action)
>> + return -ENOMEM;
>> +
>> + drvdata->fn_lock = enabled;
>> + action->type = FN_LOCK_SYNC;
>> + action->data.fn_lock = drvdata->fn_lock;
>> + INIT_LIST_HEAD(&action->node);
>> +
>> + spin_lock_irqsave(&drvdata->worker->lock, flags);
>> + asus_worker_schedule(drvdata->worker, action);
>> + spin_unlock_irqrestore(&drvdata->worker->lock, flags);
>> +
>> + return 0;
>> +}
>> +
>> +static int asus_kbd_wmi_fan_send(struct asus_drvdata *drvdata, u8 *report_data,
>> + size_t report_size)
>> +{
>> + struct asus_work_action *action;
>> + unsigned long flags;
>> +
>> + if (report_size > FEATURE_KBD_REPORT_SIZE) {
>> + hid_err(drvdata->hdev, "Invalid report size for fan event: %zu\n", report_size);
>> + return -EINVAL;
>> }
>>
>> + action = kzalloc_obj(struct asus_work_action);
>> + if (!action)
>> + return -ENOMEM;
>> +
>> + action->type = WMI_FAN;
>> + action->data.fan_hid_data.report_size = report_size;
>> + memcpy(action->data.fan_hid_data.report_data, report_data, report_size);
>> + INIT_LIST_HEAD(&action->node);
>> +
>> + spin_lock_irqsave(&drvdata->worker->lock, flags);
>> + asus_worker_schedule(drvdata->worker, action);
>> + spin_unlock_irqrestore(&drvdata->worker->lock, flags);
>> +
>> return 0;
>> }
>>
>> @@ -357,6 +452,7 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
>> struct hid_usage *usage, __s32 value)
>> {
>> struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>> + int ret;
>>
>> if ((usage->hid & HID_USAGE_PAGE) == HID_UP_ASUSVENDOR &&
>> (usage->hid & HID_USAGE) != 0x00 &&
>> @@ -375,8 +471,11 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
>> return !asus_hid_event(ASUS_EV_BRTTOGGLE);
>> case KEY_FN_ESC:
>> if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
>> - drvdata->fn_lock = !drvdata->fn_lock;
>> - schedule_work(&drvdata->fn_lock_sync_work);
>> + ret = asus_kbd_fn_lock_set(drvdata, !drvdata->fn_lock);
>> + if (ret) {
>> + hid_err(hdev, "Error while toggling FN lock: %d\n", ret);
>> + return ret;
>> + }
>> }
>> break;
>> }
>> @@ -389,6 +488,12 @@ static int asus_raw_event(struct hid_device *hdev,
>> struct hid_report *report, u8 *data, int size)
>> {
>> struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>> + int ret;
>> +
>> + if (size < 2) {
>> + hid_dbg(hdev, "Unexpected keyboard report size %d\n", size);
>> + return 0;
>> + }
>>
>> if (drvdata->battery && data[0] == BATTERY_REPORT_ID)
>> return asus_report_battery(drvdata, data, size);
>> @@ -414,19 +519,13 @@ static int asus_raw_event(struct hid_device *hdev,
>> * pass to userspace so it can implement its own fan control.
>> */
>> if (data[1] == ASUS_FAN_CTRL_KEY_CODE) {
>> - int ret = asus_wmi_send_event(drvdata, ASUS_FAN_CTRL_KEY_CODE);
>> + ret = asus_kbd_wmi_fan_send(drvdata, data, size);
>>
>> - if (ret == 0) {
>> - /* Successfully handled by asus-wmi, block event */
>> + /* if execution deferred successfully block event */
>> + if (ret == 0)
>> return -1;
>> - }
>>
>> - /*
>> - * Warn if asus-wmi failed (but not if it's unavailable).
>> - * Let the event reach userspace in all failure cases.
>> - */
>> - if (ret != -ENODEV)
>> - hid_warn(hdev, "Failed to notify asus-wmi: %d\n", ret);
>> + return ret;
>> }
>>
>> /*
>> @@ -569,59 +668,151 @@ static int asus_kbd_disable_oobe(struct hid_device *hdev)
>> return 0;
>> }
>>
>> -static int asus_kbd_set_fn_lock(struct hid_device *hdev, bool enabled)
>> +static void asus_kbd_set_fn_lock(struct hid_device *hdev, bool enabled)
>> {
>> - u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xd0, 0x4e, !!enabled };
>> + const u8 buf[FEATURE_KBD_REPORT_SIZE] = { FEATURE_KBD_REPORT_ID, 0xd0, 0x4e, !!enabled };
>> + int ret;
>>
>> - return asus_kbd_set_report(hdev, buf, sizeof(buf));
>> + ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
>> + if (ret < 0)
>> + hid_err(hdev, "Asus failed to set fn lock: %d\n", ret);
>> }
>>
>> -static void asus_sync_fn_lock(struct work_struct *work)
>> +static void asus_kbd_set_brightness(struct hid_device *hdev, u8 brightness)
>> {
>> - struct asus_drvdata *drvdata =
>> - container_of(work, struct asus_drvdata, fn_lock_sync_work);
>> + const u8 buf[FEATURE_KBD_REPORT_SIZE] = {
>> + FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, brightness
>> + };
>> + int ret;
>>
>> - asus_kbd_set_fn_lock(drvdata->hdev, drvdata->fn_lock);
>> + ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
>> + if (ret < 0)
>> + hid_err(hdev, "Asus failed to set keyboard backlight: %d\n", ret);
>> }
>>
>> -static void asus_schedule_work(struct asus_kbd_leds *led)
>> +static void asus_kbd_wmi_fan(struct hid_device *hdev, struct hid_raw_event_data *data)
>> {
>> + struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>> + int ret;
>> +
>> + ret = asus_wmi_send_event(drvdata, ASUS_FAN_CTRL_KEY_CODE);
>> +
>> + /*
>> + * Warn if asus-wmi failed (but not if it's unavailable).
>> + * Let the event reach userspace in all failure cases.
>> + */
>> + switch (ret) {
>> + case -ENODEV:
>> + break;
>> + case 0:
>> + return;
>> + default:
>> + hid_warn(hdev, "Failed to notify asus-wmi: %d\n", ret);
>> + break;
>> + }
>> +
>> + /* Fallback: pass the raw event to the HID core */
>> + hid_report_raw_event(hdev, HID_INPUT_REPORT,
>> + data->report_data, data->report_size,
>> + data->report_size, 1);
>> +}
>> +
>> +static void asus_kbd_backlight_set(struct asus_hid_listener *listener, int brightness)
>> +{
>> + struct asus_drvdata *drvdata = container_of(listener, struct asus_drvdata, listener);
>> + struct asus_worker *worker = drvdata->worker;
>> + struct asus_work_action *action;
>> unsigned long flags;
>>
>> - spin_lock_irqsave(&led->lock, flags);
>> - if (!led->removed)
>> - schedule_work(&led->work);
>> - spin_unlock_irqrestore(&led->lock, flags);
>> + drvdata->kbd_backlight_brightness = brightness;
>> +
>> + action = kzalloc_obj(struct asus_work_action);
>> + if (!action) {
>> + hid_warn(drvdata->hdev, "Failed to allocate memory for backlight action\n");
>> + return;
>> + }
>> +
>> + action->type = BRIGHTNESS_SET;
>> + action->data.brightness = brightness;
>> + INIT_LIST_HEAD(&action->node);
>> +
>> + spin_lock_irqsave(&worker->lock, flags);
>> + asus_worker_schedule(worker, action);
>> + spin_unlock_irqrestore(&worker->lock, flags);
>> }
>>
>> -static void asus_kbd_backlight_set(struct asus_hid_listener *listener,
>> - int brightness)
>> +static void asus_work(struct work_struct *work)
>> {
>> - struct asus_kbd_leds *led = container_of(listener, struct asus_kbd_leds,
>> - listener);
>> + struct asus_worker *worker = container_of(work, struct asus_worker, work);
>> + struct asus_work_action *action = NULL;
>> unsigned long flags;
>>
>> - spin_lock_irqsave(&led->lock, flags);
>> - led->brightness = brightness;
>> - spin_unlock_irqrestore(&led->lock, flags);
>> + /* Save the action to be performed and clear the flag */
>> + spin_lock_irqsave(&worker->lock, flags);
>> + if (!list_empty(&worker->actions)) {
>> + action = list_first_entry(&worker->actions,
>> + struct asus_work_action, node);
>> + list_del(&action->node);
>> + }
>> + spin_unlock_irqrestore(&worker->lock, flags);
>> +
>> + if (!action)
>> + return;
>> +
>> + switch (action->type) {
>> + case BRIGHTNESS_SET:
>> + asus_kbd_set_brightness(worker->hdev, action->data.brightness);
>> + break;
>> + case FN_LOCK_SYNC:
>> + asus_kbd_set_fn_lock(worker->hdev, action->data.fn_lock);
>> + break;
>> + case WMI_FAN:
>> + asus_kbd_wmi_fan(worker->hdev, &action->data.fan_hid_data);
>> + break;
>> + default:
>> + hid_err(worker->hdev, "Invalid action type: %d\n", action->type);
>> + break;
>> + }
>> +
>> + kfree(action);
>>
>> - asus_schedule_work(led);
>> + /* Re-schedule if there are more pending actions */
>> + spin_lock_irqsave(&worker->lock, flags);
>> + if (!list_empty(&worker->actions))
>> + schedule_work(&worker->work);
>> + spin_unlock_irqrestore(&worker->lock, flags);
>> }
>>
>> -static void asus_kbd_backlight_work(struct work_struct *work)
>> +static int asus_worker_create(struct hid_device *hdev, struct asus_drvdata *drvdata)
>> {
>> - struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds, work);
>> - u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, 0x00 };
>> - int ret;
>> + drvdata->worker = devm_kzalloc(&hdev->dev, sizeof(struct asus_worker), GFP_KERNEL);
>> + if (!drvdata->worker)
>> + return -ENOMEM;
>> +
>> + drvdata->worker->removed = false;
>> + drvdata->worker->hdev = hdev;
>> + INIT_LIST_HEAD(&drvdata->worker->actions);
>> +
>> + INIT_WORK(&drvdata->worker->work, asus_work);
>> + spin_lock_init(&drvdata->worker->lock);
>> +
>> + return 0;
>> +}
>> +
>> +static void asus_worker_stop(struct asus_worker *worker)
>> +{
>> + struct asus_work_action *action, *tmp;
>> unsigned long flags;
>>
>> - spin_lock_irqsave(&led->lock, flags);
>> - buf[4] = led->brightness;
>> - spin_unlock_irqrestore(&led->lock, flags);
>> + spin_lock_irqsave(&worker->lock, flags);
>> + worker->removed = true;
>> + list_for_each_entry_safe(action, tmp, &worker->actions, node) {
>> + list_del(&action->node);
>> + kfree(action);
>> + }
>> + spin_unlock_irqrestore(&worker->lock, flags);
>>
>> - ret = asus_kbd_set_report(led->hdev, buf, sizeof(buf));
>> - if (ret < 0)
>> - hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret);
>> + cancel_work_sync(&worker->work);
>> }
>>
>> /*
>> @@ -760,23 +951,11 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>> le16_to_cpu(udev->descriptor.idProduct));
>> }
>>
>> - drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
>> - sizeof(struct asus_kbd_leds),
>> - GFP_KERNEL);
>> - if (!drvdata->kbd_backlight)
>> - return -ENOMEM;
>> -
>> - drvdata->kbd_backlight->removed = false;
>> - drvdata->kbd_backlight->brightness = 0;
>> - drvdata->kbd_backlight->hdev = hdev;
>> - drvdata->kbd_backlight->listener.brightness_set = asus_kbd_backlight_set;
>> - INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_backlight_work);
>> - spin_lock_init(&drvdata->kbd_backlight->lock);
>> -
>> - ret = asus_hid_register_listener(&drvdata->kbd_backlight->listener);
>> + drvdata->listener.brightness_set = asus_kbd_backlight_set;
>> + ret = asus_hid_register_listener(&drvdata->listener);
>> if (ret < 0) {
>> - /* No need to have this still around */
>> - devm_kfree(&hdev->dev, drvdata->kbd_backlight);
>> + hid_err(hdev, "Unable to register kbd brightness listener: %d\n", ret);
>> + drvdata->listener.brightness_set = NULL;
>> }
>>
>> return ret;
>> @@ -965,11 +1144,9 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>> }
>> }
>>
>> - if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
>> - drvdata->fn_lock = true;
>> - INIT_WORK(&drvdata->fn_lock_sync_work, asus_sync_fn_lock);
>> - asus_kbd_set_fn_lock(hdev, true);
>> - }
>> + if ((drvdata->quirks & QUIRK_HID_FN_LOCK) &&
>> + (asus_kbd_fn_lock_set(drvdata, true)))
>> + hid_warn(hdev, "Error while setting FN lock to ON\n");
>>
>> if (drvdata->tp) {
>> int ret;
>> @@ -1004,11 +1181,9 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>>
>> drvdata->input = input;
>>
>> - if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
>> - drvdata->fn_lock = true;
>> - INIT_WORK(&drvdata->fn_lock_sync_work, asus_sync_fn_lock);
>> - asus_kbd_set_fn_lock(hdev, true);
>> - }
>> + if ((drvdata->quirks & QUIRK_HID_FN_LOCK) &&
>> + (asus_kbd_fn_lock_set(drvdata, true)))
>> + hid_warn(hdev, "Error while setting FN lock to ON\n");
>>
>> return 0;
>> }
>> @@ -1171,20 +1346,16 @@ static int asus_start_multitouch(struct hid_device *hdev)
>> static int __maybe_unused asus_resume(struct hid_device *hdev)
>> {
>> struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>> - int ret = 0;
>>
>> - if (drvdata->kbd_backlight) {
>> - const u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4,
>> - drvdata->kbd_backlight->brightness };
>> - ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
>> - if (ret < 0) {
>> - hid_err(hdev, "Asus failed to set keyboard backlight: %d\n", ret);
>> - goto asus_resume_err;
>> - }
>> - }
>> + /*
>> + * If we have a backlight listener registered, restore the previous state,
>> + * in case of error do not fail: most models restore the backlight
>> + * automatically, and the error is non-fatal.
>> + */
>> + if (drvdata->listener.brightness_set)
>> + asus_kbd_backlight_set(&drvdata->listener, drvdata->kbd_backlight_brightness);
>>
>> -asus_resume_err:
>> - return ret;
>> + return 0;
>> }
>>
>> static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
>> @@ -1294,6 +1465,12 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> is_vendor = true;
>> }
>>
>> + ret = asus_worker_create(hdev, drvdata);
>> + if (ret) {
>> + hid_warn(hdev, "Failed to initialize worker: %d\n", ret);
>> + return ret;
>> + }
>> +
>> ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>> if (ret) {
>> hid_err(hdev, "Asus hw start failed: %d\n", ret);
>> @@ -1343,6 +1520,10 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>
>> return 0;
>> err_stop_hw:
>> + if (drvdata->listener.brightness_set)
>> + asus_hid_unregister_listener(&drvdata->listener);
>> +
>> + asus_worker_stop(drvdata->worker);
>> hid_hw_stop(hdev);
>> return ret;
>> }
>> @@ -1350,21 +1531,11 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> static void asus_remove(struct hid_device *hdev)
>> {
>> struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>> - unsigned long flags;
>> -
>> - if (drvdata->kbd_backlight) {
>> - asus_hid_unregister_listener(&drvdata->kbd_backlight->listener);
>> -
>> - spin_lock_irqsave(&drvdata->kbd_backlight->lock, flags);
>> - drvdata->kbd_backlight->removed = true;
>> - spin_unlock_irqrestore(&drvdata->kbd_backlight->lock, flags);
>> -
>> - cancel_work_sync(&drvdata->kbd_backlight->work);
>> - }
>>
>> - if (drvdata->quirks & QUIRK_HID_FN_LOCK)
>> - cancel_work_sync(&drvdata->fn_lock_sync_work);
>> + if (drvdata->listener.brightness_set)
>> + asus_hid_unregister_listener(&drvdata->listener);
>>
>> + asus_worker_stop(drvdata->worker);
>> hid_hw_stop(hdev);
>> }
>>
>> --
>> 2.47.3
>>
>>
^ permalink raw reply
* Re: [PATCH v4 4/5] HID: asus: add support for xgm led
From: Antheas Kapenekakis @ 2026-06-15 21:55 UTC (permalink / raw)
To: Denis Benato
Cc: linux-kernel, linux-input, Benjamin Tissoires, Jiri Kosina,
Luke D . Jones, Mateusz Schyboll, Denis Benato, Connor Belli
In-Reply-To: <20260615165058.3845-5-denis.benato@linux.dev>
On Mon, 15 Jun 2026 at 18:51, Denis Benato <denis.benato@linux.dev> wrote:
>
> XG mobile stations have very bright leds behind the fan that can be
> turned either ON or OFF: add a cled interface to allow controlling the
> brightness of those red leds.
>
> Signed-off-by: Denis Benato <denis.benato@linux.dev>
> ---
> drivers/hid/hid-asus.c | 91 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 91 insertions(+)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 6896730efafc..e68a6d93369d 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -51,6 +51,8 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> #define FEATURE_KBD_LED_REPORT_ID1 0x5d
> #define FEATURE_KBD_LED_REPORT_ID2 0x5e
>
> +#define ROG_XGM_REPORT_SIZE 300
> +
> #define ROG_ALLY_REPORT_SIZE 64
> #define ROG_ALLY_X_MIN_MCU 313
> #define ROG_ALLY_MIN_MCU 319
> @@ -143,6 +145,11 @@ struct asus_worker {
> bool removed;
> };
>
> +struct asus_xgm_led {
> + struct led_classdev cdev;
> + struct hid_device *hdev;
> +};
> +
> struct asus_touchpad_info {
> int max_x;
> int max_y;
> @@ -169,6 +176,7 @@ struct asus_drvdata {
> unsigned long battery_next_query;
> struct asus_hid_listener listener;
> bool fn_lock;
> + struct asus_xgm_led *xgm_led;
> };
>
> static int asus_report_battery(struct asus_drvdata *, u8 *, int);
> @@ -1119,6 +1127,26 @@ static int asus_battery_probe(struct hid_device *hdev)
> return ret;
> }
>
> +static int asus_xgm_led_set(struct led_classdev *led_cdev, enum led_brightness value)
> +{
> + const u8 buf[ROG_XGM_REPORT_SIZE] = {
> + FEATURE_KBD_LED_REPORT_ID2, 0xC5, (value) ? 0x50 : 0x00
> + };
> + struct asus_xgm_led *xgm = container_of(led_cdev, struct asus_xgm_led, cdev);
> + int ret;
> +
> + ret = asus_kbd_set_report(xgm->hdev, buf, ROG_XGM_REPORT_SIZE);
> + if (ret < 0) {
> + hid_err(xgm->hdev, "Unable to set XG mobile led state: %d\n", ret);
> + return ret;
> + } else if (ret != ROG_XGM_REPORT_SIZE) {
> + hid_err(xgm->hdev, "Unexpected partial transfer to XG mobile: %d\n", ret);
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
> {
> struct input_dev *input = hi->input;
> @@ -1343,9 +1371,52 @@ static int asus_start_multitouch(struct hid_device *hdev)
> return 0;
> }
>
> +static int asus_xgm_init(struct hid_device *hdev, struct asus_drvdata *drvdata)
> +{
> + const char *name;
> + int ret;
> +
> + drvdata->xgm_led = devm_kzalloc(&hdev->dev, sizeof(*drvdata->xgm_led), GFP_KERNEL);
> + if (drvdata->xgm_led == NULL)
> + return -ENOMEM;
> +
> + name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "asus:xgm-%s:led",
> + strlen(hdev->uniq) ? hdev->uniq : dev_name(&hdev->dev));
> +
> + if (name == NULL) {
> + ret = -ENOMEM;
> + goto asus_xgm_init_err;
> + }
> +
> + drvdata->xgm_led->hdev = hdev;
> + drvdata->xgm_led->cdev.name = name;
> + drvdata->xgm_led->cdev.brightness = 1;
> + drvdata->xgm_led->cdev.max_brightness = 1;
> + drvdata->xgm_led->cdev.brightness_set_blocking = asus_xgm_led_set;
> +
> + /* LED state is arbitrary on boot, set a default */
> + ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
> + if (ret) {
> + hid_err(hdev, "Asus failed to set xgm led: %d\n", ret);
> + goto asus_xgm_init_err;
> + }
> +
> + ret = devm_led_classdev_register(&hdev->dev, &drvdata->xgm_led->cdev);
> + if (ret) {
> + hid_err(hdev, "Asus failed to register xgm led: %d\n", ret);
> + goto asus_xgm_init_err;
> + }
> +
> + return 0;
> +asus_xgm_init_err:
> + drvdata->xgm_led = NULL;
> + return ret;
> +}
> +
> static int __maybe_unused asus_resume(struct hid_device *hdev)
> {
> struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> + int ret;
>
> /*
> * If we have a backlight listener registered, restore the previous state,
> @@ -1355,7 +1426,17 @@ static int __maybe_unused asus_resume(struct hid_device *hdev)
> if (drvdata->listener.brightness_set)
> asus_kbd_backlight_set(&drvdata->listener, drvdata->kbd_backlight_brightness);
>
> + if (drvdata->xgm_led) {
> + ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
> + if (ret) {
> + hid_err(hdev, "Asus failed to restore xgm brightness: %d\n", ret);
> + goto asus_resume_err;
> + }
> + }
> +
> return 0;
> +asus_resume_err:
> + return ret;
> }
>
> static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
> @@ -1484,6 +1565,16 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
> }
> }
>
> + if (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID) &&
> + ((hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2022) ||
> + (hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2023))) {
> + ret = asus_xgm_init(hdev, drvdata);
> + if (ret) {
> + hid_err(hdev, "Failed to initialize xg mobile: %d\n", ret);
> + goto err_stop_hw;
> + }
> + }
> +
reset_resume has a special meaning and perhaps should not have been
used for this driver at all. Check if resume works and if it does use
that instead.
Other than that and the refactor patch I'd rather see separately, the
other 4 patches look fine to me.
Reviewed-by: Antheas Kapenekakis <lkml@antheas.dev>
Best,
Antheas
> /* Laptops keyboard backlight is always at 0x5a */
> if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
> (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID)) &&
> --
> 2.47.3
>
>
^ permalink raw reply
* Re: [PATCH v4 2/5] HID: asus: remove unnecessary OOM message
From: Antheas Kapenekakis @ 2026-06-15 21:52 UTC (permalink / raw)
To: Denis Benato
Cc: linux-kernel, linux-input, Benjamin Tissoires, Jiri Kosina,
Luke D . Jones, Mateusz Schyboll, Denis Benato, Connor Belli
In-Reply-To: <20260615165058.3845-3-denis.benato@linux.dev>
On Mon, 15 Jun 2026 at 18:51, Denis Benato <denis.benato@linux.dev> wrote:
>
> If devm_kzalloc fails an allocation error is already being reported:
> there is no need for the driver to repeat it, moreover if this were
> new code checkpatch.pl would throw a warning.
Perhaps something more of:
HID: asus: remove extraneous allocation warning
If devm_kzalloc fails an allocation error is already being reported.
Therefore, there is no need for the driver to repeat it. For new code,
this behavior is disincentivized and checkpatch.pl reports a warning.
Reviewed-by: Antheas Kapenekakis <lkml@antheas.dev>
> Signed-off-by: Denis Benato <denis.benato@linux.dev>
> ---
> drivers/hid/hid-asus.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 05ca6665e0a4..bbf964b12c16 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -1377,10 +1377,8 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
> int ret;
>
> drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
> - if (drvdata == NULL) {
> - hid_err(hdev, "Can't alloc Asus descriptor\n");
> + if (drvdata == NULL)
> return -ENOMEM;
> - }
>
> hid_set_drvdata(hdev, drvdata);
>
> --
> 2.47.3
>
>
^ permalink raw reply
* Re: [PATCH v4 1/5] HID: asus: refactor the two workqueues and init sequence
From: Antheas Kapenekakis @ 2026-06-15 21:49 UTC (permalink / raw)
To: Denis Benato
Cc: linux-kernel, linux-input, Benjamin Tissoires, Jiri Kosina,
Luke D . Jones, Mateusz Schyboll, Denis Benato, Connor Belli,
sahiko-bot
In-Reply-To: <20260615165058.3845-2-denis.benato@linux.dev>
On Mon, 15 Jun 2026 at 18:51, Denis Benato <denis.benato@linux.dev> wrote:
>
> Multiple issues have been found within the hid-asus driver:
> - unchecked size in asus_raw_event()
> - unclean teardown of asus_probe on failure
> - possible use-after-free in asus_probe
> - multiple workqueue used for jobs where one was enough
> - sleeping calls in atomic context
> - packets of incorrect size being sent to the keyboard controller
>
> Join the two workqueues into one reusing the stopping mechanism
> of the brightness workqueue, use the joined workqueue to also
> move the asus_wmi_send_event() sleeping call away from atomic
> context and add a size check in asus_raw_event().
>
> Fixes: f631011e36b8 ("HID: hid-asus: Implement fn lock for Asus ProArt P16")
> Fixes: 1489a34e97ef ("HID: asus: Implement Fn+F5 fan control key handler")
> Fixes: b34b5945a769 ("HID: asus: listen to the asus-wmi brightness device instead of creating one")
I'm not sure b34b5945a769 caused an issue, perhaps drop it from fixes.
Does f631011e36b8 do a WMI from a hid interrupt? If not, perhaps it
does not need to be changed
But moreso, this patch is too large and will take a bit to review. Can
you send it separately after the rest of the series goes through
unless users report warnings?
Antheas
> Reported-by: sahiko-bot@kernel.org
> Closes: https://lore.kernel.org/all/20260613154732.60A4B1F000E9@smtp.kernel.org/
> Signed-off-by: Denis Benato <denis.benato@linux.dev>
> ---
> drivers/hid/hid-asus.c | 393 +++++++++++++++++++++++++++++------------
> 1 file changed, 282 insertions(+), 111 deletions(-)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index d34d74df3dc0..05ca6665e0a4 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -109,11 +109,36 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>
> #define TRKID_SGN ((TRKID_MAX + 1) >> 1)
>
> -struct asus_kbd_leds {
> - struct asus_hid_listener listener;
> +enum asus_work_action_type {
> + FN_LOCK_SYNC,
> + BRIGHTNESS_SET,
> + WMI_FAN,
> +};
> +
> +struct hid_raw_event_data {
> + u8 report_data[FEATURE_KBD_REPORT_SIZE];
> + size_t report_size;
> +};
> +
> +struct asus_work_action {
> + struct list_head node;
> + enum asus_work_action_type type;
> + union {
> + /* Data for BRIGHTNESS_SET */
> + unsigned int brightness;
> +
> + /* Data for FN_LOCK_SYNC */
> + bool fn_lock;
> +
> + /* Data for WMI_FAN */
> + struct hid_raw_event_data fan_hid_data;
> + } data;
> +};
> +
> +struct asus_worker {
> struct hid_device *hdev;
> struct work_struct work;
> - unsigned int brightness;
> + struct list_head actions;
> spinlock_t lock;
> bool removed;
> };
> @@ -133,7 +158,8 @@ struct asus_drvdata {
> struct hid_device *hdev;
> struct input_dev *input;
> struct input_dev *tp_kbd_input;
> - struct asus_kbd_leds *kbd_backlight;
> + struct asus_worker *worker;
> + unsigned int kbd_backlight_brightness;
> const struct asus_touchpad_info *tp;
> struct power_supply *battery;
> struct power_supply_desc battery_desc;
> @@ -141,7 +167,7 @@ struct asus_drvdata {
> int battery_stat;
> bool battery_in_query;
> unsigned long battery_next_query;
> - struct work_struct fn_lock_sync_work;
> + struct asus_hid_listener listener;
> bool fn_lock;
> };
>
> @@ -211,6 +237,29 @@ static const u8 asus_report_id_init[] = {
> FEATURE_KBD_LED_REPORT_ID2
> };
>
> +/*
> + * Send events to asus-wmi driver for handling special keys
> + */
> +static int asus_wmi_send_event(struct asus_drvdata *drvdata, u8 code)
> +{
> + int err;
> + u32 retval;
> +
> + err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
> + ASUS_WMI_METHODID_NOTIF, code, &retval);
> + if (err) {
> + pr_warn("Failed to notify asus-wmi: %d\n", err);
> + return err;
> + }
> +
> + if (retval != 0) {
> + pr_warn("Failed to notify asus-wmi (retval): 0x%x\n", retval);
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> static void asus_report_contact_down(struct asus_drvdata *drvdat,
> int toolType, u8 *data)
> {
> @@ -331,25 +380,71 @@ static int asus_e1239t_event(struct asus_drvdata *drvdat, u8 *data, int size)
> }
>
> /*
> - * Send events to asus-wmi driver for handling special keys
> + * Used in atomic contexts to schedule work involving sleeps operations or
> + * asus-wmi interactions.
> + *
> + * Caller is responsible to store relevant data in the structure to carry out
> + * the required action.
> + *
> + * This function must be called while the spin lock protecting the workqueue
> + * is already being held.
> */
> -static int asus_wmi_send_event(struct asus_drvdata *drvdata, u8 code)
> +static void asus_worker_schedule(struct asus_worker *worker, struct asus_work_action *action)
> {
> - int err;
> - u32 retval;
> -
> - err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
> - ASUS_WMI_METHODID_NOTIF, code, &retval);
> - if (err) {
> - pr_warn("Failed to notify asus-wmi: %d\n", err);
> - return err;
> + if (worker->removed) {
> + kfree(action);
> + return;
> }
>
> - if (retval != 0) {
> - pr_warn("Failed to notify asus-wmi (retval): 0x%x\n", retval);
> - return -EIO;
> + list_add_tail(&action->node, &worker->actions);
> + schedule_work(&worker->work);
> +}
> +
> +static int asus_kbd_fn_lock_set(struct asus_drvdata *drvdata, bool enabled)
> +{
> + struct asus_work_action *action;
> + unsigned long flags;
> +
> + action = kzalloc_obj(struct asus_work_action);
> + if (!action)
> + return -ENOMEM;
> +
> + drvdata->fn_lock = enabled;
> + action->type = FN_LOCK_SYNC;
> + action->data.fn_lock = drvdata->fn_lock;
> + INIT_LIST_HEAD(&action->node);
> +
> + spin_lock_irqsave(&drvdata->worker->lock, flags);
> + asus_worker_schedule(drvdata->worker, action);
> + spin_unlock_irqrestore(&drvdata->worker->lock, flags);
> +
> + return 0;
> +}
> +
> +static int asus_kbd_wmi_fan_send(struct asus_drvdata *drvdata, u8 *report_data,
> + size_t report_size)
> +{
> + struct asus_work_action *action;
> + unsigned long flags;
> +
> + if (report_size > FEATURE_KBD_REPORT_SIZE) {
> + hid_err(drvdata->hdev, "Invalid report size for fan event: %zu\n", report_size);
> + return -EINVAL;
> }
>
> + action = kzalloc_obj(struct asus_work_action);
> + if (!action)
> + return -ENOMEM;
> +
> + action->type = WMI_FAN;
> + action->data.fan_hid_data.report_size = report_size;
> + memcpy(action->data.fan_hid_data.report_data, report_data, report_size);
> + INIT_LIST_HEAD(&action->node);
> +
> + spin_lock_irqsave(&drvdata->worker->lock, flags);
> + asus_worker_schedule(drvdata->worker, action);
> + spin_unlock_irqrestore(&drvdata->worker->lock, flags);
> +
> return 0;
> }
>
> @@ -357,6 +452,7 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
> struct hid_usage *usage, __s32 value)
> {
> struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> + int ret;
>
> if ((usage->hid & HID_USAGE_PAGE) == HID_UP_ASUSVENDOR &&
> (usage->hid & HID_USAGE) != 0x00 &&
> @@ -375,8 +471,11 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
> return !asus_hid_event(ASUS_EV_BRTTOGGLE);
> case KEY_FN_ESC:
> if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
> - drvdata->fn_lock = !drvdata->fn_lock;
> - schedule_work(&drvdata->fn_lock_sync_work);
> + ret = asus_kbd_fn_lock_set(drvdata, !drvdata->fn_lock);
> + if (ret) {
> + hid_err(hdev, "Error while toggling FN lock: %d\n", ret);
> + return ret;
> + }
> }
> break;
> }
> @@ -389,6 +488,12 @@ static int asus_raw_event(struct hid_device *hdev,
> struct hid_report *report, u8 *data, int size)
> {
> struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> + int ret;
> +
> + if (size < 2) {
> + hid_dbg(hdev, "Unexpected keyboard report size %d\n", size);
> + return 0;
> + }
>
> if (drvdata->battery && data[0] == BATTERY_REPORT_ID)
> return asus_report_battery(drvdata, data, size);
> @@ -414,19 +519,13 @@ static int asus_raw_event(struct hid_device *hdev,
> * pass to userspace so it can implement its own fan control.
> */
> if (data[1] == ASUS_FAN_CTRL_KEY_CODE) {
> - int ret = asus_wmi_send_event(drvdata, ASUS_FAN_CTRL_KEY_CODE);
> + ret = asus_kbd_wmi_fan_send(drvdata, data, size);
>
> - if (ret == 0) {
> - /* Successfully handled by asus-wmi, block event */
> + /* if execution deferred successfully block event */
> + if (ret == 0)
> return -1;
> - }
>
> - /*
> - * Warn if asus-wmi failed (but not if it's unavailable).
> - * Let the event reach userspace in all failure cases.
> - */
> - if (ret != -ENODEV)
> - hid_warn(hdev, "Failed to notify asus-wmi: %d\n", ret);
> + return ret;
> }
>
> /*
> @@ -569,59 +668,151 @@ static int asus_kbd_disable_oobe(struct hid_device *hdev)
> return 0;
> }
>
> -static int asus_kbd_set_fn_lock(struct hid_device *hdev, bool enabled)
> +static void asus_kbd_set_fn_lock(struct hid_device *hdev, bool enabled)
> {
> - u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xd0, 0x4e, !!enabled };
> + const u8 buf[FEATURE_KBD_REPORT_SIZE] = { FEATURE_KBD_REPORT_ID, 0xd0, 0x4e, !!enabled };
> + int ret;
>
> - return asus_kbd_set_report(hdev, buf, sizeof(buf));
> + ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
> + if (ret < 0)
> + hid_err(hdev, "Asus failed to set fn lock: %d\n", ret);
> }
>
> -static void asus_sync_fn_lock(struct work_struct *work)
> +static void asus_kbd_set_brightness(struct hid_device *hdev, u8 brightness)
> {
> - struct asus_drvdata *drvdata =
> - container_of(work, struct asus_drvdata, fn_lock_sync_work);
> + const u8 buf[FEATURE_KBD_REPORT_SIZE] = {
> + FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, brightness
> + };
> + int ret;
>
> - asus_kbd_set_fn_lock(drvdata->hdev, drvdata->fn_lock);
> + ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
> + if (ret < 0)
> + hid_err(hdev, "Asus failed to set keyboard backlight: %d\n", ret);
> }
>
> -static void asus_schedule_work(struct asus_kbd_leds *led)
> +static void asus_kbd_wmi_fan(struct hid_device *hdev, struct hid_raw_event_data *data)
> {
> + struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> + int ret;
> +
> + ret = asus_wmi_send_event(drvdata, ASUS_FAN_CTRL_KEY_CODE);
> +
> + /*
> + * Warn if asus-wmi failed (but not if it's unavailable).
> + * Let the event reach userspace in all failure cases.
> + */
> + switch (ret) {
> + case -ENODEV:
> + break;
> + case 0:
> + return;
> + default:
> + hid_warn(hdev, "Failed to notify asus-wmi: %d\n", ret);
> + break;
> + }
> +
> + /* Fallback: pass the raw event to the HID core */
> + hid_report_raw_event(hdev, HID_INPUT_REPORT,
> + data->report_data, data->report_size,
> + data->report_size, 1);
> +}
> +
> +static void asus_kbd_backlight_set(struct asus_hid_listener *listener, int brightness)
> +{
> + struct asus_drvdata *drvdata = container_of(listener, struct asus_drvdata, listener);
> + struct asus_worker *worker = drvdata->worker;
> + struct asus_work_action *action;
> unsigned long flags;
>
> - spin_lock_irqsave(&led->lock, flags);
> - if (!led->removed)
> - schedule_work(&led->work);
> - spin_unlock_irqrestore(&led->lock, flags);
> + drvdata->kbd_backlight_brightness = brightness;
> +
> + action = kzalloc_obj(struct asus_work_action);
> + if (!action) {
> + hid_warn(drvdata->hdev, "Failed to allocate memory for backlight action\n");
> + return;
> + }
> +
> + action->type = BRIGHTNESS_SET;
> + action->data.brightness = brightness;
> + INIT_LIST_HEAD(&action->node);
> +
> + spin_lock_irqsave(&worker->lock, flags);
> + asus_worker_schedule(worker, action);
> + spin_unlock_irqrestore(&worker->lock, flags);
> }
>
> -static void asus_kbd_backlight_set(struct asus_hid_listener *listener,
> - int brightness)
> +static void asus_work(struct work_struct *work)
> {
> - struct asus_kbd_leds *led = container_of(listener, struct asus_kbd_leds,
> - listener);
> + struct asus_worker *worker = container_of(work, struct asus_worker, work);
> + struct asus_work_action *action = NULL;
> unsigned long flags;
>
> - spin_lock_irqsave(&led->lock, flags);
> - led->brightness = brightness;
> - spin_unlock_irqrestore(&led->lock, flags);
> + /* Save the action to be performed and clear the flag */
> + spin_lock_irqsave(&worker->lock, flags);
> + if (!list_empty(&worker->actions)) {
> + action = list_first_entry(&worker->actions,
> + struct asus_work_action, node);
> + list_del(&action->node);
> + }
> + spin_unlock_irqrestore(&worker->lock, flags);
> +
> + if (!action)
> + return;
> +
> + switch (action->type) {
> + case BRIGHTNESS_SET:
> + asus_kbd_set_brightness(worker->hdev, action->data.brightness);
> + break;
> + case FN_LOCK_SYNC:
> + asus_kbd_set_fn_lock(worker->hdev, action->data.fn_lock);
> + break;
> + case WMI_FAN:
> + asus_kbd_wmi_fan(worker->hdev, &action->data.fan_hid_data);
> + break;
> + default:
> + hid_err(worker->hdev, "Invalid action type: %d\n", action->type);
> + break;
> + }
> +
> + kfree(action);
>
> - asus_schedule_work(led);
> + /* Re-schedule if there are more pending actions */
> + spin_lock_irqsave(&worker->lock, flags);
> + if (!list_empty(&worker->actions))
> + schedule_work(&worker->work);
> + spin_unlock_irqrestore(&worker->lock, flags);
> }
>
> -static void asus_kbd_backlight_work(struct work_struct *work)
> +static int asus_worker_create(struct hid_device *hdev, struct asus_drvdata *drvdata)
> {
> - struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds, work);
> - u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, 0x00 };
> - int ret;
> + drvdata->worker = devm_kzalloc(&hdev->dev, sizeof(struct asus_worker), GFP_KERNEL);
> + if (!drvdata->worker)
> + return -ENOMEM;
> +
> + drvdata->worker->removed = false;
> + drvdata->worker->hdev = hdev;
> + INIT_LIST_HEAD(&drvdata->worker->actions);
> +
> + INIT_WORK(&drvdata->worker->work, asus_work);
> + spin_lock_init(&drvdata->worker->lock);
> +
> + return 0;
> +}
> +
> +static void asus_worker_stop(struct asus_worker *worker)
> +{
> + struct asus_work_action *action, *tmp;
> unsigned long flags;
>
> - spin_lock_irqsave(&led->lock, flags);
> - buf[4] = led->brightness;
> - spin_unlock_irqrestore(&led->lock, flags);
> + spin_lock_irqsave(&worker->lock, flags);
> + worker->removed = true;
> + list_for_each_entry_safe(action, tmp, &worker->actions, node) {
> + list_del(&action->node);
> + kfree(action);
> + }
> + spin_unlock_irqrestore(&worker->lock, flags);
>
> - ret = asus_kbd_set_report(led->hdev, buf, sizeof(buf));
> - if (ret < 0)
> - hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret);
> + cancel_work_sync(&worker->work);
> }
>
> /*
> @@ -760,23 +951,11 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
> le16_to_cpu(udev->descriptor.idProduct));
> }
>
> - drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
> - sizeof(struct asus_kbd_leds),
> - GFP_KERNEL);
> - if (!drvdata->kbd_backlight)
> - return -ENOMEM;
> -
> - drvdata->kbd_backlight->removed = false;
> - drvdata->kbd_backlight->brightness = 0;
> - drvdata->kbd_backlight->hdev = hdev;
> - drvdata->kbd_backlight->listener.brightness_set = asus_kbd_backlight_set;
> - INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_backlight_work);
> - spin_lock_init(&drvdata->kbd_backlight->lock);
> -
> - ret = asus_hid_register_listener(&drvdata->kbd_backlight->listener);
> + drvdata->listener.brightness_set = asus_kbd_backlight_set;
> + ret = asus_hid_register_listener(&drvdata->listener);
> if (ret < 0) {
> - /* No need to have this still around */
> - devm_kfree(&hdev->dev, drvdata->kbd_backlight);
> + hid_err(hdev, "Unable to register kbd brightness listener: %d\n", ret);
> + drvdata->listener.brightness_set = NULL;
> }
>
> return ret;
> @@ -965,11 +1144,9 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
> }
> }
>
> - if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
> - drvdata->fn_lock = true;
> - INIT_WORK(&drvdata->fn_lock_sync_work, asus_sync_fn_lock);
> - asus_kbd_set_fn_lock(hdev, true);
> - }
> + if ((drvdata->quirks & QUIRK_HID_FN_LOCK) &&
> + (asus_kbd_fn_lock_set(drvdata, true)))
> + hid_warn(hdev, "Error while setting FN lock to ON\n");
>
> if (drvdata->tp) {
> int ret;
> @@ -1004,11 +1181,9 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>
> drvdata->input = input;
>
> - if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
> - drvdata->fn_lock = true;
> - INIT_WORK(&drvdata->fn_lock_sync_work, asus_sync_fn_lock);
> - asus_kbd_set_fn_lock(hdev, true);
> - }
> + if ((drvdata->quirks & QUIRK_HID_FN_LOCK) &&
> + (asus_kbd_fn_lock_set(drvdata, true)))
> + hid_warn(hdev, "Error while setting FN lock to ON\n");
>
> return 0;
> }
> @@ -1171,20 +1346,16 @@ static int asus_start_multitouch(struct hid_device *hdev)
> static int __maybe_unused asus_resume(struct hid_device *hdev)
> {
> struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> - int ret = 0;
>
> - if (drvdata->kbd_backlight) {
> - const u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4,
> - drvdata->kbd_backlight->brightness };
> - ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
> - if (ret < 0) {
> - hid_err(hdev, "Asus failed to set keyboard backlight: %d\n", ret);
> - goto asus_resume_err;
> - }
> - }
> + /*
> + * If we have a backlight listener registered, restore the previous state,
> + * in case of error do not fail: most models restore the backlight
> + * automatically, and the error is non-fatal.
> + */
> + if (drvdata->listener.brightness_set)
> + asus_kbd_backlight_set(&drvdata->listener, drvdata->kbd_backlight_brightness);
>
> -asus_resume_err:
> - return ret;
> + return 0;
> }
>
> static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
> @@ -1294,6 +1465,12 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
> is_vendor = true;
> }
>
> + ret = asus_worker_create(hdev, drvdata);
> + if (ret) {
> + hid_warn(hdev, "Failed to initialize worker: %d\n", ret);
> + return ret;
> + }
> +
> ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> if (ret) {
> hid_err(hdev, "Asus hw start failed: %d\n", ret);
> @@ -1343,6 +1520,10 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>
> return 0;
> err_stop_hw:
> + if (drvdata->listener.brightness_set)
> + asus_hid_unregister_listener(&drvdata->listener);
> +
> + asus_worker_stop(drvdata->worker);
> hid_hw_stop(hdev);
> return ret;
> }
> @@ -1350,21 +1531,11 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
> static void asus_remove(struct hid_device *hdev)
> {
> struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> - unsigned long flags;
> -
> - if (drvdata->kbd_backlight) {
> - asus_hid_unregister_listener(&drvdata->kbd_backlight->listener);
> -
> - spin_lock_irqsave(&drvdata->kbd_backlight->lock, flags);
> - drvdata->kbd_backlight->removed = true;
> - spin_unlock_irqrestore(&drvdata->kbd_backlight->lock, flags);
> -
> - cancel_work_sync(&drvdata->kbd_backlight->work);
> - }
>
> - if (drvdata->quirks & QUIRK_HID_FN_LOCK)
> - cancel_work_sync(&drvdata->fn_lock_sync_work);
> + if (drvdata->listener.brightness_set)
> + asus_hid_unregister_listener(&drvdata->listener);
>
> + asus_worker_stop(drvdata->worker);
> hid_hw_stop(hdev);
> }
>
> --
> 2.47.3
>
>
^ permalink raw reply
* Re: [PATCH v4 3/5] HID: asus: fix a off-by-one issue making a check stricter that it needs to be
From: Antheas Kapenekakis @ 2026-06-15 21:44 UTC (permalink / raw)
To: Denis Benato
Cc: linux-kernel, linux-input, Benjamin Tissoires, Jiri Kosina,
Luke D . Jones, Mateusz Schyboll, Denis Benato, Connor Belli
In-Reply-To: <20260615165058.3845-4-denis.benato@linux.dev>
On Mon, 15 Jun 2026 at 18:51, Denis Benato <denis.benato@linux.dev> wrote:
>
> In mcu_parse_version_string() a size validation for response is stricter
> that it needs to be: relax the check by one byte.
"making a check stricter that it needs to be"
what check, you might need to reword the title
> Fixes: ("hid-asus: check ROG Ally MCU version and warn")
> Signed-off-by: Denis Benato <denis.benato@linux.dev>
> ---
> drivers/hid/hid-asus.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index bbf964b12c16..6896730efafc 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -835,7 +835,7 @@ static int mcu_parse_version_string(const u8 *response, size_t response_size)
> dots++;
> }
>
> - if (dots != 2 || p >= end || (p + 3) >= end)
> + if (dots != 2 || end - p < 3)
> return -EINVAL;
>
> memcpy(buf, p, 3);
> --
> 2.47.3
>
>
^ permalink raw reply
* Re: [PATCH v4 4/5] HID: asus: add support for xgm led
From: Denis Benato @ 2026-06-15 20:59 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260615170045.AF91E1F000E9@smtp.kernel.org>
On 6/15/26 19:00, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Propagating cosmetic LED restore errors during system resume causes the entire device resume to fail.
> - [High] Failing to initialize the auxiliary XG Mobile LED aborts the entire HID device probe.
> - [High] Manual call to `hid_hw_stop()` while using `devm_led_classdev_register()` leads to a use-after-teardown bug during device removal and probe error paths.
> --
>
> commit 26f82fa14c4bebc817bb5d79a1cdcdc34bb503d3
> Author: Denis Benato <denis.benato@linux.dev>
>
> HID: asus: add support for xgm led
>
> XG mobile stations have very bright leds behind the fan that can be
> turned either ON or OFF: add a cled interface to allow controlling the
> brightness of those red leds.
>
>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>> index 6896730efafca..e68a6d93369d8 100644
>> --- a/drivers/hid/hid-asus.c
>> +++ b/drivers/hid/hid-asus.c
> [ ... ]
>> @@ -1343,9 +1371,52 @@ static int asus_start_multitouch(struct hid_device *hdev)
>> return 0;
>> }
>>
>> +static int asus_xgm_init(struct hid_device *hdev, struct asus_drvdata *drvdata)
>> +{
> [ ... ]
>> + drvdata->xgm_led->cdev.brightness_set_blocking = asus_xgm_led_set;
>> +
>> + /* LED state is arbitrary on boot, set a default */
>> + ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
>> + if (ret) {
>> + hid_err(hdev, "Asus failed to set xgm led: %d\n", ret);
>> + goto asus_xgm_init_err;
>> + }
>> +
>> + ret = devm_led_classdev_register(&hdev->dev, &drvdata->xgm_led->cdev);
> [Severity: High]
> Because this LED is registered using devm_led_classdev_register(), its
> unregistration happens during the devres cleanup phase. This phase runs after
> the driver's remove callback (or probe error path) finishes.
>
> If the probe error path or asus_remove() manually calls hid_hw_stop(hdev),
> the HID transport is shut down. During the subsequent devres cleanup,
> led_classdev_unregister() is called and sets the LED to LED_OFF. This invokes
> asus_xgm_led_set(), which calls hid_hw_raw_request() on a stopped interface.
>
> Could this lead to a use-after-teardown bug?
>
>> + if (ret) {
>> + hid_err(hdev, "Asus failed to register xgm led: %d\n", ret);
>> + goto asus_xgm_init_err;
>> + }
>> +
>> + return 0;
>> +asus_xgm_init_err:
>> + drvdata->xgm_led = NULL;
>> + return ret;
>> +}
>> +
>> static int __maybe_unused asus_resume(struct hid_device *hdev)
>> {
>> struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>> + int ret;
>>
>> /*
>> * If we have a backlight listener registered, restore the previous state,
>> @@ -1355,7 +1426,17 @@ static int __maybe_unused asus_resume(struct hid_device *hdev)
>> if (drvdata->listener.brightness_set)
>> asus_kbd_backlight_set(&drvdata->listener, drvdata->kbd_backlight_brightness);
>>
>> + if (drvdata->xgm_led) {
>> + ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
>> + if (ret) {
>> + hid_err(hdev, "Asus failed to restore xgm brightness: %d\n", ret);
>> + goto asus_resume_err;
> [Severity: High]
> If asus_xgm_led_set() fails to restore the LED state (e.g., due to an I/O
> error or timeout), this returns the error code directly to the PM core.
> A failed resume callback causes the PM subsystem to treat the device resume as
> failed, potentially leaving primary features like the keyboard and touchpad
> unresponsive.
>
> Would it be better for the failure of a cosmetic LED restore to log a warning
> instead of aborting the entire device resume?
No: xg mobile device controllers are just that: not an input device: if it fails
there it's right to tear down the whole thing.
>> + }
>> + }
>> +
>> return 0;
>> +asus_resume_err:
>> + return ret;
>> }
>>
>> static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
> [ ... ]
>> @@ -1484,6 +1565,16 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> }
>> }
>>
>> + if (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID) &&
>> + ((hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2022) ||
>> + (hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2023))) {
>> + ret = asus_xgm_init(hdev, drvdata);
>> + if (ret) {
>> + hid_err(hdev, "Failed to initialize xg mobile: %d\n", ret);
>> + goto err_stop_hw;
> [Severity: High]
> If asus_xgm_init() fails, the code jumps to err_stop_hw, failing the entire
> device probe.
>
> Does this make a non-critical aesthetic feature a hard dependency for the
> entire device? Might it be safer to log the failure rather than preventing
> the keyboard and touchpad from working?
Ditto. Won't resolve these as it would only make the code more confusing.
>> + }
>> + }
>> +
>> /* Laptops keyboard backlight is always at 0x5a */
>> if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
>> (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID)) &&
^ permalink raw reply
* Re: [PATCH] Input: goodix - clamp the device-reported contact count
From: Bryam Vargas @ 2026-06-15 18:42 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel
In-Reply-To: <ai8WppWIAsSKXwFk@google.com>
On Sun, Jun 14, 2026 at 02:02:27PM -0700, Dmitry Torokhov wrote:
> Should we drop the report if is has bogus data in it?
We already do, for the per-interrupt count. goodix_ts_read_input_report()
drops a report whose reported touch count is out of range:
touch_num = data[0] & 0x0f;
if (touch_num > ts->max_touch_num)
return -EPROTO;
The only gap this patch closes is that ts->max_touch_num itself could be
larger than what point_data[] holds: it was taken straight from the 4-bit
config nibble (0..15), while the buffer is sized for GOODIX_MAX_CONTACTS
(10) -- 2 + 9*10 = 92 bytes. A config advertising 11..15 made that -EPROTO
check accept an out-of-range touch_num and overflow the stack buffer.
Clamping max_touch_num to GOODIX_MAX_CONTACTS at config-read time makes
that existing per-report drop a safe bound: a report claiming more than 10
contacts is now dropped with -EPROTO, and 10 is exactly point_data[]'s
capacity, so no legitimate report is lost (the driver maximum is 10, and
the other max_touch_num assignments already use GOODIX_MAX_CONTACTS).
If you'd rather flag a bogus config than silently clamp it, I'm happy to
add a dev_warn_ratelimited() in a v2 -- just say the word.
Thanks,
Bryam
^ permalink raw reply
* Re: [PATCH v3 2/2] Input: apple_z2 - bound the device-reported finger count
From: sashiko-bot @ 2026-06-15 18:32 UTC (permalink / raw)
To: Bryam Vargas via B4 Relay; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260615-b4-disp-05943ee1-v3-2-bbfe1b651613@proton.me>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [High] Concurrent execution of apple_z2_read_packet() from the probe thread and threaded IRQ handler causes buffer corruption.
- [High] Missing length check for `fw->size` before accessing the firmware header in `apple_z2_upload_firmware()`.
- [Medium] The timeout error path in `apple_z2_boot()` returns early without disabling the IRQ or asserting the reset GPIO.
- [Low] Missing input_set_drvdata() before device registration.
--
commit 647d8f6a84132a3fd0648807a009baeb1d458863
Author: Bryam Vargas <hexlabsecurity@proton.me>
Input: apple_z2 - bound the device-reported finger count
This commit adds bounds checking to apple_z2_parse_touches() to ensure the
reported finger count does not exceed the receive buffer. It drops packets
that claim more fingers than can fit, preventing an out-of-bounds read that
could leak adjacent kernel memory.
Closes: https://lore.kernel.org/all/20260613215358.329921F000E9@smtp.kernel.org/
> diff --git a/drivers/input/touchscreen/apple_z2.c b/drivers/input/touchscreen/apple_z2.c
> index ff9ff97be185a..4e2a6967436e8 100644
> --- a/drivers/input/touchscreen/apple_z2.c
> +++ b/drivers/input/touchscreen/apple_z2.c
[ ... ]
[Severity: High]
This isn't a bug introduced by this patch, but does concurrent execution of
apple_z2_read_packet() from the probe thread and threaded IRQ handler cause
buffer corruption?
In apple_z2_upload_firmware():
z2->booted = true;
apple_z2_read_packet(z2);
If the device asserts the IRQ line here, the unmasked IRQ handler
apple_z2_irq() wakes up, observes booted is true, and also calls
apple_z2_read_packet():
else
apple_z2_read_packet(z2);
Could this result in a data race where SPI DMA transfers corrupt the receive
buffer while apple_z2_parse_touches() is executing?
[Severity: High]
This is a pre-existing issue, but does apple_z2_upload_firmware() miss a
length check for fw->size before accessing the firmware header?
fw_hdr = (const struct apple_z2_fw_hdr *)fw->data;
if (le32_to_cpu(fw_hdr->magic) != APPLE_Z2_FW_MAGIC ||
le32_to_cpu(fw_hdr->version) != 1) {
If the firmware file is truncated or empty, will this cause an out-of-bounds
read when the header fields are accessed?
[Severity: Medium]
This isn't a bug introduced by this patch, but I noticed the timeout error
path in apple_z2_boot() returns early without cleaning up:
if (!wait_for_completion_timeout(&z2->boot_irq, msecs_to_jiffies(20)))
return -ETIMEDOUT;
Does this leave the hardware out of reset and create an unbalanced IRQ enable
state? The subsequent error path for apple_z2_upload_firmware() correctly
performs these cleanup steps by disabling the IRQ and asserting the reset
GPIO.
[Severity: Low]
This is a pre-existing issue, but the input device is registered in
apple_z2_probe() without prior initialization of driver data:
error = input_register_device(z2->input_dev);
Is it intentional to omit input_set_drvdata() before registration here? The
subsystem guidelines state that all private driver data must be initialized
before registration to avoid visibility issues.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-b4-disp-05943ee1-v3-0-bbfe1b651613@proton.me?part=2
^ permalink raw reply
* Re: [PATCH v3 1/2] Input: apple_z2 - bound the device-reported packet length
From: sashiko-bot @ 2026-06-15 18:32 UTC (permalink / raw)
To: Bryam Vargas via B4 Relay; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260615-b4-disp-05943ee1-v3-1-bbfe1b651613@proton.me>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] Skipping `spi_read()` on oversized packets leaves the device's buffer undrained, causing an infinite interrupt storm.
Pre-existing issues:
- [High] Unprotected concurrent calls to `apple_z2_read_packet()` from both the probe/resume thread and IRQ context lead to SPI buffer corruption.
--
commit 9b2ec52954ce23198e826455d8fa38261e0ca8d8
Author: Bryam Vargas <hexlabsecurity@proton.me>
Input: apple_z2 - bound the device-reported packet length
apple_z2_read_packet() takes a 16-bit length from the touch controller's
interrupt-data reply and uses it as the size of the second SPI transfer into
the fixed-size rx_buf. Reject any packet whose length exceeds the receive
buffer to prevent out-of-bounds writes.
Fixes: 471a92f8a21a ("Input: apple_z2 - add a driver for Apple Z2 touchscreens")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
Reviewed-by: Sasha Finkelstein <k@chaosmail.tech>
> diff --git a/drivers/input/touchscreen/apple_z2.c b/drivers/input/touchscreen/apple_z2.c
> index 271ababf0ad55..ff9ff97be185a 100644
> --- a/drivers/input/touchscreen/apple_z2.c
> +++ b/drivers/input/touchscreen/apple_z2.c
[ ... ]
> @@ -147,6 +148,8 @@ static int apple_z2_read_packet(struct apple_z2 *z2)
> return 0;
[Severity: High]
This is a pre-existing issue, but does this function need synchronization to
prevent SPI buffer corruption?
If apple_z2_upload_firmware() sets z2->booted = true and synchronously calls
apple_z2_read_packet(), and a concurrent interrupt triggers apple_z2_irq(),
they can both invoke apple_z2_read_packet() at the same time.
Won't this cause simultaneous unsynchronized modifications to the shared
z2->tx_buf, z2->rx_buf, and z2->index_parity variables?
>
> pkt_len = (get_unaligned_le16(z2->rx_buf + 1) + 8) & 0xfffffffc;
> + if (pkt_len > APPLE_Z2_MAX_PACKET)
> + return -EMSGSIZE;
[Severity: High]
Will returning early here cause an interrupt storm?
By skipping spi_read(), the device's internal buffer is not drained and the
hardware interrupt line will likely remain asserted.
Since apple_z2_irq() ignores the error returned by apple_z2_read_packet() and
unilaterally returns IRQ_HANDLED:
drivers/input/touchscreen/apple_z2.c:apple_z2_irq() {
...
else
apple_z2_read_packet(z2);
return IRQ_HANDLED;
}
Could this immediately unmask the interrupt and endlessly loop through this
same failure path?
>
> error = spi_read(z2->spidev, z2->rx_buf, pkt_len);
> if (error)
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-b4-disp-05943ee1-v3-0-bbfe1b651613@proton.me?part=1
^ permalink raw reply
* [PATCH v3 2/2] Input: apple_z2 - bound the device-reported finger count
From: Bryam Vargas via B4 Relay @ 2026-06-15 18:20 UTC (permalink / raw)
To: Sasha Finkelstein, Dmitry Torokhov
Cc: linux-arm-kernel, linux-kernel, Neal Gompa, asahi, Sven Peter,
linux-input, Janne Grunau
In-Reply-To: <20260615-b4-disp-05943ee1-v3-0-bbfe1b651613@proton.me>
From: Bryam Vargas <hexlabsecurity@proton.me>
apple_z2_parse_touches() takes the finger count from the touch
controller's report and loops over that many fixed-size finger records
without checking the count against the length of the report:
nfingers = msg[APPLE_Z2_NUM_FINGERS_OFFSET];
fingers = (struct apple_z2_finger *)(msg + APPLE_Z2_FINGERS_OFFSET);
for (i = 0; i < nfingers; i++)
/* read fingers[i] ... */
msg points into the fixed 4000-byte rx_buf and nfingers is a single
device-supplied byte, so it can be as large as 255. A malicious,
malfunctioning or counterfeit controller (or an interposer on the SPI
bus) can report a large finger count in a short packet, making the loop
read up to 255 * sizeof(struct apple_z2_finger) bytes starting 24 bytes
into msg -- past the end of the receive buffer. This is a
controller-driven heap out-of-bounds read, and the finger fields that
are read (position, pressure, touch and tool dimensions) are forwarded
to userspace as input events, leaking adjacent kernel memory.
Drop any packet that claims more fingers than it can hold, warning once
per rate-limit interval, rather than reporting a truncated set of
fingers from an inconsistent report. A short zero-finger ("all lifted")
report is still passed through so the lifted state reaches userspace.
The 4000-byte receive buffer holds at most 132 finger records, so this
only ever drops counts that could not have been parsed in bounds.
Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/all/20260613215358.329921F000E9@smtp.kernel.org/
Fixes: 471a92f8a21a ("Input: apple_z2 - add a driver for Apple Z2 touchscreens")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
drivers/input/touchscreen/apple_z2.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/input/touchscreen/apple_z2.c b/drivers/input/touchscreen/apple_z2.c
index ff9ff97be185..4e2a6967436e 100644
--- a/drivers/input/touchscreen/apple_z2.c
+++ b/drivers/input/touchscreen/apple_z2.c
@@ -93,6 +93,17 @@ static void apple_z2_parse_touches(struct apple_z2 *z2,
return;
nfingers = msg[APPLE_Z2_NUM_FINGERS_OFFSET];
fingers = (struct apple_z2_finger *)(msg + APPLE_Z2_FINGERS_OFFSET);
+ /*
+ * A malicious or malfunctioning controller can report more fingers
+ * than the packet actually carries; drop the packet rather than read
+ * finger records past the end of the receive buffer.
+ */
+ if (nfingers && msg_len < APPLE_Z2_FINGERS_OFFSET + nfingers * sizeof(*fingers)) {
+ dev_warn_ratelimited(&z2->spidev->dev,
+ "discarding packet: %d fingers exceed packet length\n",
+ nfingers);
+ return;
+ }
for (i = 0; i < nfingers; i++) {
slot = input_mt_get_slot_by_key(z2->input_dev, fingers[i].finger);
if (slot < 0) {
--
2.43.0
^ permalink raw reply related
* [PATCH v3 1/2] Input: apple_z2 - bound the device-reported packet length
From: Bryam Vargas via B4 Relay @ 2026-06-15 18:20 UTC (permalink / raw)
To: Sasha Finkelstein, Dmitry Torokhov
Cc: linux-arm-kernel, linux-kernel, Neal Gompa, asahi, Sven Peter,
linux-input, Janne Grunau
In-Reply-To: <20260615-b4-disp-05943ee1-v3-0-bbfe1b651613@proton.me>
From: Bryam Vargas <hexlabsecurity@proton.me>
apple_z2_read_packet() takes a 16-bit length from the touch controller's
interrupt-data reply (rx_buf[1..2]) and, only rounded down to a multiple of
four, uses it as the size of the second SPI transfer into the fixed-size
rx_buf:
pkt_len = (get_unaligned_le16(z2->rx_buf + 1) + 8) & 0xfffffffc;
error = spi_read(z2->spidev, z2->rx_buf, pkt_len);
rx_buf is a fixed 4000-byte buffer, but pkt_len is fully controlled by the
device and is never checked against it, so a malicious, malfunctioning or
counterfeit controller (or an interposer on the SPI bus) that reports a
large length makes spi_read() write up to 65540 device-supplied bytes into
the 4000-byte buffer -- a controller-driven heap out-of-bounds write of up
to about 61 KiB. The recently added reply-type check only validates
rx_buf[0], not the length.
Reject any packet whose length exceeds the receive buffer.
Fixes: 471a92f8a21a ("Input: apple_z2 - add a driver for Apple Z2 touchscreens")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
Reviewed-by: Sasha Finkelstein <k@chaosmail.tech>
---
drivers/input/touchscreen/apple_z2.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/apple_z2.c b/drivers/input/touchscreen/apple_z2.c
index 271ababf0ad5..ff9ff97be185 100644
--- a/drivers/input/touchscreen/apple_z2.c
+++ b/drivers/input/touchscreen/apple_z2.c
@@ -22,6 +22,7 @@
#define APPLE_Z2_TOUCH_MOVED 4
#define APPLE_Z2_CMD_READ_INTERRUPT_DATA 0xEB
#define APPLE_Z2_REPLY_INTERRUPT_DATA 0xE1
+#define APPLE_Z2_MAX_PACKET 4000
#define APPLE_Z2_HBPP_CMD_BLOB 0x3001
#define APPLE_Z2_FW_MAGIC 0x5746325A
#define LOAD_COMMAND_INIT_PAYLOAD 0
@@ -147,6 +148,8 @@ static int apple_z2_read_packet(struct apple_z2 *z2)
return 0;
pkt_len = (get_unaligned_le16(z2->rx_buf + 1) + 8) & 0xfffffffc;
+ if (pkt_len > APPLE_Z2_MAX_PACKET)
+ return -EMSGSIZE;
error = spi_read(z2->spidev, z2->rx_buf, pkt_len);
if (error)
@@ -363,7 +366,7 @@ static int apple_z2_probe(struct spi_device *spi)
if (!z2->tx_buf)
return -ENOMEM;
/* 4096 will end up being rounded up to 8192 due to devres header */
- z2->rx_buf = devm_kzalloc(dev, 4000, GFP_KERNEL);
+ z2->rx_buf = devm_kzalloc(dev, APPLE_Z2_MAX_PACKET, GFP_KERNEL);
if (!z2->rx_buf)
return -ENOMEM;
--
2.43.0
^ permalink raw reply related
* [PATCH v3 0/2] Input: apple_z2 - bound device-reported packet length and finger count
From: Bryam Vargas via B4 Relay @ 2026-06-15 18:20 UTC (permalink / raw)
To: Sasha Finkelstein, Dmitry Torokhov
Cc: linux-arm-kernel, linux-kernel, Neal Gompa, asahi, Sven Peter,
linux-input, Janne Grunau
The Apple Z2 touch controller is across an SPI bus and its reports are not
trusted: a malicious, malfunctioning or counterfeit controller (or an
interposer on the bus) can put arbitrary values in the report header. Two
such device-supplied fields are used as sizes without being bounded against
the fixed 4000-byte receive buffer, giving the device a heap out-of-bounds
write and a heap out-of-bounds read. This series bounds both.
Patch 1 bounds the 16-bit packet length before the second SPI transfer
(spi_read() into the 4000-byte rx_buf) -- the OOB write.
Patch 2 bounds the device-reported finger count in apple_z2_parse_touches()
-- the OOB read that is then forwarded to userspace as input events.
The two are paired because, as Dmitry noted on v2 of patch 2, the finger
count alone is not enough: msg_len is itself derived from the device length,
so patch 1 is what makes the patch 2 bound meaningful. Patch 1 was posted
standalone first and carries Sasha Finkelstein's Reviewed-by; it is unchanged
here.
This v3 series supersedes the two separate postings:
- packet length (standalone): https://lore.kernel.org/all/20260613-b4-disp-b1926f1a-v1-1-3a277b7c0cfa@proton.me/
- finger count v1: https://lore.kernel.org/all/20260613-b4-disp-f0148c89-v1-1-868a48b2a187@proton.me/
- finger count v2: https://lore.kernel.org/all/20260613-b4-disp-4ebcbd68-v2-1-0161acfbd688@proton.me/
Changes since the v2 finger-count patch (Dmitry Torokhov review,
https://lore.kernel.org/all/ai8U0QiwpBH8qNS3@google.com/):
- Patch 2: instead of clamping the finger count to what the packet holds
(v2 used min_t and reported the fingers that fit), drop the whole packet
when the device claims more fingers than it can hold, with a
dev_warn_ratelimited(), per Dmitry's "I'd drop such packet" suggestion.
A legitimate zero-finger ("all lifted") report (nfingers == 0) is still
passed through to input_mt_sync_frame()/input_sync(), so lifting all
fingers is not lost. (Dropped Joshua Peisach's v2 Reviewed-by, as the
fix shape changed materially from clamp to drop.)
- Patch 2 bound uses the multiplication form
(msg_len < FINGERS_OFFSET + nfingers * sizeof(*fingers)) rather than a
(msg_len - FINGERS_OFFSET) subtraction, so there is no unsigned underflow
when msg_len < FINGERS_OFFSET (which would otherwise re-open the read).
- Patch 1 folded into the series unchanged (Reviewed-by Sasha Finkelstein).
Note on patch 1 (Sasha Finkelstein, on the standalone thread): rejecting an
over-long packet desyncs the SPI datastream and the device then needs a reset
before it operates again. That is the intended outcome for a controller that
is already misbehaving or hostile -- a recoverable desync is preferable to a
~61 KiB heap overflow -- so the reject is kept.
Verification (in-kernel KASAN, CONFIG_KASAN=y kasan.fault=panic, plus userspace
ASan models of the exact parse arithmetic on x86_64 and i386):
Patch 1 (packet length): reported length -> pkt_len 65540:
Arm A (no patch): KASAN slab-out-of-bounds / ASan heap-buffer-overflow WRITE,
both ABIs.
Arm B (patch): rejected, clean. Arm C (benign): clean.
Patch 2 (finger count, drop shape): nfingers 255 in a short packet (msg_len 19):
Arm A (no patch): BUG: KASAN: slab-out-of-bounds, Read of size 2, 1 byte to
the right of the allocated 4000-byte region (cache kmalloc-4k) -> panic;
ASan heap-buffer-overflow READ, both ABIs.
Arm B (patch): packet dropped, zero reads, clean, not synced.
Arm C (benign 3 fingers, msg_len 114): parsed in-bounds, clean, synced.
Arm D (zero-finger all-lifted, nfingers 0, msg_len 19): clean, synced.
Reproducers and full logs available on request.
---
Bryam Vargas (2):
Input: apple_z2 - bound the device-reported packet length
Input: apple_z2 - bound the device-reported finger count
drivers/input/touchscreen/apple_z2.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
---
base-commit: 8e65320d91cdc3b241d4b94855c88459b91abf66
change-id: 20260615-b4-disp-05943ee1-9dc5cce670fe
Best regards,
--
Bryam Vargas <hexlabsecurity@proton.me>
^ permalink raw reply
* Re: [PATCH v6 1/7] dt-bindings: mfd: mt6397: Add MT6392 PMIC
From: Luca Leonardo Scorcia @ 2026-06-15 17:09 UTC (permalink / raw)
To: Conor Dooley
Cc: linux-mediatek, Fabien Parent, Val Packett, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Sen Chu,
Sean Wang, Macpaul Lin, Lee Jones, Matthias Brugger,
AngeloGioacchino Del Regno, Linus Walleij, Julien Massot,
Louis-Alexis Eyraud, Akari Tsuyukusa, Chen Zhong, linux-input,
devicetree, linux-kernel, linux-pm, linux-arm-kernel, linux-gpio
In-Reply-To: <20260615-palatable-aerobics-3091229b6ada@spud>
Hi,
yes, sorry about that, series v6 has been superseded by v7 (I replied
to the thread and marked it as archived in patchwork, please let me
know if I have to do something else to mark it as obsolete).
Sashiko was correct, the regulators node is required for this device.
Sashiko also has suggestions for v7, a few pre existing issues and a
few nits here and there but some are actual improvements. One bit that
caught my eye is the use of the modeset register in the
mt6392_ldo_get_mode function. I have to double check that with the
data sheet and the android kernel sources. Not sure if I can do that
before next week though.
Is there any way I can trigger a Sashiko review before sending patches
to the ML?
Thank you,
Luca
^ permalink raw reply
* Re: [PATCH v4 1/5] HID: asus: refactor the two workqueues and init sequence
From: sashiko-bot @ 2026-06-15 17:03 UTC (permalink / raw)
To: Denis Benato; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260615165058.3845-2-denis.benato@linux.dev>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Sleeping in atomic context due to implicit GFP_KERNEL allocations in HID event handlers.
- [High] Data race on shared `hid_report` object when calling `hid_report_raw_event()` asynchronously from the workqueue.
- [High] Use-After-Free due to un-canceled workqueue items if `hid_hw_start()` fails during probe.
--
commit 29efc59c7a1d251ec0726f58d50488da03f9ba12
Author: Denis Benato <denis.benato@linux.dev>
HID: asus: refactor the two workqueues and init sequence
Join the two workqueues into one reusing the stopping mechanism
of the brightness workqueue, use the joined workqueue to also
move the asus_wmi_send_event() sleeping call away from atomic
context and add a size check in asus_raw_event().
Link: https://lore.kernel.org/all/20260613154732.60A4B1F000E9@smtp.kernel.org/
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -331,25 +380,71 @@ static int asus_e1239t_event(struct asus_drvdata *drvdat, u8 *data, int size)
[ ... ]
> +static int asus_kbd_fn_lock_set(struct asus_drvdata *drvdata, bool enabled)
> +{
> + struct asus_work_action *action;
> + unsigned long flags;
> +
> + action = kzalloc_obj(struct asus_work_action);
[Severity: High]
Does this allocation sleep in an atomic context?
Because kzalloc_obj() implicitly uses default_gfp() which resolves to
GFP_KERNEL, this introduces a blocking allocation. Since
asus_kbd_fn_lock_set() is called directly from the raw HID event handler
asus_event() which runs in an atomic context, this might trigger kernel
panics or deadlocks under load.
> + if (!action)
> + return -ENOMEM;
[ ... ]
> +static int asus_kbd_wmi_fan_send(struct asus_drvdata *drvdata, u8 *report_data,
> + size_t report_size)
> +{
> + struct asus_work_action *action;
> + unsigned long flags;
> +
> + if (report_size > FEATURE_KBD_REPORT_SIZE) {
> + hid_err(drvdata->hdev, "Invalid report size for fan event: %zu\n", report_size);
> + return -EINVAL;
> }
>
> + action = kzalloc_obj(struct asus_work_action);
[Severity: High]
Similarly, does this kzalloc_obj() introduce a blocking allocation in an
atomic context?
asus_kbd_wmi_fan_send() is invoked from the asus_raw_event() handler, which
also executes in an atomic context.
> + if (!action)
> + return -ENOMEM;
[ ... ]
> @@ -569,59 +668,151 @@ static int asus_kbd_disable_oobe(struct hid_device *hdev)
[ ... ]
> +static void asus_kbd_wmi_fan(struct hid_device *hdev, struct hid_raw_event_data *data)
> +{
> + struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> + int ret;
> +
> + ret = asus_wmi_send_event(drvdata, ASUS_FAN_CTRL_KEY_CODE);
[ ... ]
> + /* Fallback: pass the raw event to the HID core */
> + hid_report_raw_event(hdev, HID_INPUT_REPORT,
> + data->report_data, data->report_size,
> + data->report_size, 1);
[Severity: High]
Can this data race with native HID IRQ delivery?
If notifying the WMI system of a fan key event fails, the deferred work
handler asus_kbd_wmi_fan() falls back to injecting the raw event into the
HID core using hid_report_raw_event().
However, hid_report_raw_event() parses the report and mutates the shared
report->field state arrays without taking any locks. Because the workqueue
bypasses the hid->driver_input_lock semaphore used by the native IRQ path,
a concurrent hardware interrupt targeting the same report ID could race with
this workqueue. This might lead to unsynchronized reads and writes to
field->value and field->new_value, potentially corrupting the core HID
report state.
> +}
[ ... ]
> @@ -1294,6 +1465,12 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
[ ... ]
> + ret = asus_worker_create(hdev, drvdata);
> + if (ret) {
> + hid_warn(hdev, "Failed to initialize worker: %d\n", ret);
> + return ret;
> + }
> +
> ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> if (ret) {
> hid_err(hdev, "Asus hw start failed: %d\n", ret);
> return ret;
> }
[Severity: High]
Could this leak a pending work item and cause a use-after-free if
hid_hw_start() fails?
During driver probe, asus_worker_create() creates a devres-managed worker
struct. Next, hid_hw_start() connects the device, invoking
asus_input_configured(), which synchronously schedules a FN_LOCK_SYNC
work item on the global workqueue.
If hid_hw_start() subsequently fails (e.g., due to an input registration
error later in the sequence), asus_probe() captures the error and returns
it directly, bypassing the err_stop_hw label. Consequently, it fails to
call asus_worker_stop() or cancel_work_sync().
The devres framework will automatically free the worker memory, but the
scheduled work item remains active. When the system workqueue eventually
executes the pending work, it will dereference the freed memory.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615165058.3845-1-denis.benato@linux.dev?part=1
^ permalink raw reply
* Re: [PATCH v4 4/5] HID: asus: add support for xgm led
From: sashiko-bot @ 2026-06-15 17:00 UTC (permalink / raw)
To: Denis Benato; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260615165058.3845-5-denis.benato@linux.dev>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Propagating cosmetic LED restore errors during system resume causes the entire device resume to fail.
- [High] Failing to initialize the auxiliary XG Mobile LED aborts the entire HID device probe.
- [High] Manual call to `hid_hw_stop()` while using `devm_led_classdev_register()` leads to a use-after-teardown bug during device removal and probe error paths.
--
commit 26f82fa14c4bebc817bb5d79a1cdcdc34bb503d3
Author: Denis Benato <denis.benato@linux.dev>
HID: asus: add support for xgm led
XG mobile stations have very bright leds behind the fan that can be
turned either ON or OFF: add a cled interface to allow controlling the
brightness of those red leds.
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 6896730efafca..e68a6d93369d8 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -1343,9 +1371,52 @@ static int asus_start_multitouch(struct hid_device *hdev)
> return 0;
> }
>
> +static int asus_xgm_init(struct hid_device *hdev, struct asus_drvdata *drvdata)
> +{
[ ... ]
> + drvdata->xgm_led->cdev.brightness_set_blocking = asus_xgm_led_set;
> +
> + /* LED state is arbitrary on boot, set a default */
> + ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
> + if (ret) {
> + hid_err(hdev, "Asus failed to set xgm led: %d\n", ret);
> + goto asus_xgm_init_err;
> + }
> +
> + ret = devm_led_classdev_register(&hdev->dev, &drvdata->xgm_led->cdev);
[Severity: High]
Because this LED is registered using devm_led_classdev_register(), its
unregistration happens during the devres cleanup phase. This phase runs after
the driver's remove callback (or probe error path) finishes.
If the probe error path or asus_remove() manually calls hid_hw_stop(hdev),
the HID transport is shut down. During the subsequent devres cleanup,
led_classdev_unregister() is called and sets the LED to LED_OFF. This invokes
asus_xgm_led_set(), which calls hid_hw_raw_request() on a stopped interface.
Could this lead to a use-after-teardown bug?
> + if (ret) {
> + hid_err(hdev, "Asus failed to register xgm led: %d\n", ret);
> + goto asus_xgm_init_err;
> + }
> +
> + return 0;
> +asus_xgm_init_err:
> + drvdata->xgm_led = NULL;
> + return ret;
> +}
> +
> static int __maybe_unused asus_resume(struct hid_device *hdev)
> {
> struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> + int ret;
>
> /*
> * If we have a backlight listener registered, restore the previous state,
> @@ -1355,7 +1426,17 @@ static int __maybe_unused asus_resume(struct hid_device *hdev)
> if (drvdata->listener.brightness_set)
> asus_kbd_backlight_set(&drvdata->listener, drvdata->kbd_backlight_brightness);
>
> + if (drvdata->xgm_led) {
> + ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
> + if (ret) {
> + hid_err(hdev, "Asus failed to restore xgm brightness: %d\n", ret);
> + goto asus_resume_err;
[Severity: High]
If asus_xgm_led_set() fails to restore the LED state (e.g., due to an I/O
error or timeout), this returns the error code directly to the PM core.
A failed resume callback causes the PM subsystem to treat the device resume as
failed, potentially leaving primary features like the keyboard and touchpad
unresponsive.
Would it be better for the failure of a cosmetic LED restore to log a warning
instead of aborting the entire device resume?
> + }
> + }
> +
> return 0;
> +asus_resume_err:
> + return ret;
> }
>
> static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
[ ... ]
> @@ -1484,6 +1565,16 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
> }
> }
>
> + if (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID) &&
> + ((hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2022) ||
> + (hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2023))) {
> + ret = asus_xgm_init(hdev, drvdata);
> + if (ret) {
> + hid_err(hdev, "Failed to initialize xg mobile: %d\n", ret);
> + goto err_stop_hw;
[Severity: High]
If asus_xgm_init() fails, the code jumps to err_stop_hw, failing the entire
device probe.
Does this make a non-critical aesthetic feature a hard dependency for the
entire device? Might it be safer to log the failure rather than preventing
the keyboard and touchpad from working?
> + }
> + }
> +
> /* Laptops keyboard backlight is always at 0x5a */
> if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
> (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID)) &&
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615165058.3845-1-denis.benato@linux.dev?part=4
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox