* [PATCH v2 3/7] i2c: of-prober: skip post-power-on delay if already powered on
From: Chen-Yu Tsai @ 2026-07-03 11:55 UTC (permalink / raw)
To: Matthias Brugger, AngeloGioacchino Del Regno, Benson Leung,
Tzung-Bi Shih, Dmitry Torokhov, Jiri Kosina, Andi Shyti
Cc: Chen-Yu Tsai, linux-mediatek, devicetree, linux-arm-kernel,
chrome-platform, linux-input, linux-i2c, linux-kernel
In-Reply-To: <20260703115601.1323491-1-wenst@chromium.org>
On some devices the I2C component is powered from an always-on power
rail, or the power rail has been left on by either POR defaults or
the bootloader. By the time the prober probes the device, the device
most certainly has finished initializing and can respond. There is no
need for the delay.
In such designs, the system integrators tend to work around the delay
to avoid the boot time penalty by simply omitting it from the device
tree and the component prober. This is undesired, as the device tree
is not fully describing the hardware.
Instead, check if the regulator supplies are all enabled, and skip
the post-power-on delay if that is the case.
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
drivers/i2c/i2c-core-of-prober.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c
index 6a82b03809d4..f274e260353c 100644
--- a/drivers/i2c/i2c-core-of-prober.c
+++ b/drivers/i2c/i2c-core-of-prober.c
@@ -18,6 +18,7 @@
#include <linux/regulator/consumer.h>
#include <linux/slab.h>
#include <linux/stddef.h>
+#include <linux/string_choices.h>
/*
* Some devices, such as Google Hana Chromebooks, are produced by multiple
@@ -219,19 +220,25 @@ static void i2c_of_probe_simple_put_supply(struct i2c_of_probe_simple_ctx *ctx)
static int i2c_of_probe_simple_enable_regulator(struct device *dev, struct i2c_of_probe_simple_ctx *ctx)
{
+ bool supply_was_on;
int ret;
if (!ctx->supply)
return 0;
- dev_dbg(dev, "Enabling regulator supply \"%s\"\n", ctx->opts->supply_name);
+ supply_was_on = regulator_is_enabled(ctx->supply);
+
+ dev_dbg(dev, "Enabling regulator supply \"%s\" (was %s)\n", ctx->opts->supply_name,
+ str_on_off(supply_was_on));
ret = regulator_enable(ctx->supply);
if (ret)
return ret;
- if (ctx->opts->post_power_on_delay_ms)
+ if (!supply_was_on && ctx->opts->post_power_on_delay_ms) {
+ dev_dbg(dev, "Waiting after enabling regulator\n");
msleep(ctx->opts->post_power_on_delay_ms);
+ }
return 0;
}
--
2.55.0.rc0.799.gd6f94ed593-goog
^ permalink raw reply related
* [PATCH v2 2/7] HID: i2c-hid-of: skip post-power-on delay if already powered on
From: Chen-Yu Tsai @ 2026-07-03 11:55 UTC (permalink / raw)
To: Matthias Brugger, AngeloGioacchino Del Regno, Benson Leung,
Tzung-Bi Shih, Dmitry Torokhov, Jiri Kosina, Andi Shyti
Cc: Chen-Yu Tsai, linux-mediatek, devicetree, linux-arm-kernel,
chrome-platform, linux-input, linux-i2c, linux-kernel
In-Reply-To: <20260703115601.1323491-1-wenst@chromium.org>
On some devices the HID device is powered from an always-on power rail,
or the power rail has been left on by either POR defaults or the
bootloader. By the time the driver probes, the device most certainly
has finished initializing. There is no need for the delay.
In such designs, the system integrators tend to work around the delay
to avoid the boot time penalty by simply omitting it from the device
tree. This is undesired, as the device tree is not fully describing
the hardware.
Instead, check if the regulator supplies are all enabled, and skip
the post-power-on delay if that is the case.
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
drivers/hid/i2c-hid/i2c-hid-of.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
index 59393d71ddb9..70afdfb207ac 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of.c
@@ -29,6 +29,7 @@
#include <linux/of.h>
#include <linux/pm.h>
#include <linux/regulator/consumer.h>
+#include <linux/string_choices.h>
#include "i2c-hid.h"
@@ -46,8 +47,12 @@ static int i2c_hid_of_power_up(struct i2chid_ops *ops)
{
struct i2c_hid_of *ihid_of = container_of(ops, struct i2c_hid_of, ops);
struct device *dev = &ihid_of->client->dev;
+ bool supply_was_enabled = true;
int ret;
+ for (unsigned int i = 0; i < ARRAY_SIZE(ihid_of->supplies); i++)
+ supply_was_enabled &= regulator_is_enabled(ihid_of->supplies[i].consumer);
+
ret = regulator_bulk_enable(ARRAY_SIZE(ihid_of->supplies),
ihid_of->supplies);
if (ret) {
@@ -55,7 +60,8 @@ static int i2c_hid_of_power_up(struct i2chid_ops *ops)
return ret;
}
- if (ihid_of->post_power_delay_ms)
+ dev_dbg(dev, "supply was %s.\n", str_on_off(supply_was_enabled));
+ if (!supply_was_enabled && ihid_of->post_power_delay_ms)
msleep(ihid_of->post_power_delay_ms);
gpiod_set_value_cansleep(ihid_of->reset_gpio, 0);
--
2.55.0.rc0.799.gd6f94ed593-goog
^ permalink raw reply related
* [PATCH v2 1/7] Input: elan_i2c - Wait for initialization after enabling regulator supply
From: Chen-Yu Tsai @ 2026-07-03 11:55 UTC (permalink / raw)
To: Matthias Brugger, AngeloGioacchino Del Regno, Benson Leung,
Tzung-Bi Shih, Dmitry Torokhov, Jiri Kosina, Andi Shyti
Cc: Chen-Yu Tsai, linux-mediatek, devicetree, linux-arm-kernel,
chrome-platform, linux-input, linux-i2c, linux-kernel
In-Reply-To: <20260703115601.1323491-1-wenst@chromium.org>
Elan trackpad controllers require some delay after enabling power to
the controller for the hardware and firmware to initialize:
- 2ms for hardware initialization
- 100ms for firmware initialization
Until then, the hardware will not respond to I2C transfers. This was
observed on the MT8173 Chromebooks after the regulator supply for the
trackpad was changed to "not always on".
Add proper delays after regulator_enable() calls. To avoid impacting
the boot time of existing devices that have the power rails always on,
skip the delay if the regulator supply was already enabled. In this
case the regulator is either always on, was on by default at power up,
or was left on by some other driver, such as the I2C OF component
prober. Either way the controller has had ample time to initialize.
Fixes: 6696777c6506 ("Input: add driver for Elan I2C/SMbus touchpad")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
Changes since v1:
- Delay only if the regulator was previously disabled / turned off
- Link to v1
https://lore.kernel.org/all/20241001093815.2481899-1-wenst@chromium.org/
---
drivers/input/mouse/elan_i2c_core.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index f93dd545d66b..db48d7ef8357 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -47,6 +47,8 @@
#define ETP_FWIDTH_REDUCE 90
#define ETP_FINGER_WIDTH 15
#define ETP_RETRY_COUNT 3
+/* H/W init 2 ms + F/W init 100 ms w/ round up */
+#define ETP_POWER_ON_DELAY 110
/* quirks to control the device */
#define ETP_QUIRK_QUICK_WAKEUP BIT(0)
@@ -1219,6 +1221,7 @@ static int elan_probe(struct i2c_client *client)
struct device *dev = &client->dev;
struct elan_tp_data *data;
unsigned long irqflags;
+ bool supply_was_enabled;
int error;
if (IS_ENABLED(CONFIG_MOUSE_ELAN_I2C_I2C) &&
@@ -1250,6 +1253,8 @@ static int elan_probe(struct i2c_client *client)
if (IS_ERR(data->vcc))
return dev_err_probe(dev, PTR_ERR(data->vcc), "Failed to get 'vcc' regulator\n");
+ supply_was_enabled = regulator_is_enabled(data->vcc);
+
error = regulator_enable(data->vcc);
if (error) {
dev_err(dev, "Failed to enable regulator: %d\n", error);
@@ -1263,6 +1268,9 @@ static int elan_probe(struct i2c_client *client)
return error;
}
+ if (!supply_was_enabled)
+ msleep(ETP_POWER_ON_DELAY);
+
/* Make sure there is something at this address */
error = i2c_smbus_read_byte(client);
if (error < 0) {
@@ -1406,11 +1414,16 @@ static int elan_resume(struct device *dev)
int error;
if (!device_may_wakeup(dev)) {
+ bool supply_was_enabled = regulator_is_enabled(data->vcc);
+
error = regulator_enable(data->vcc);
if (error) {
dev_err(dev, "error %d enabling regulator\n", error);
goto err;
}
+
+ if (!supply_was_enabled)
+ msleep(ETP_POWER_ON_DELAY);
}
error = elan_set_power(data, true);
--
2.55.0.rc0.799.gd6f94ed593-goog
^ permalink raw reply related
* [PATCH v2 0/7] arm64: mediatek: Chromebook trackpad supply fixes
From: Chen-Yu Tsai @ 2026-07-03 11:55 UTC (permalink / raw)
To: Matthias Brugger, AngeloGioacchino Del Regno, Benson Leung,
Tzung-Bi Shih, Dmitry Torokhov, Jiri Kosina, Andi Shyti
Cc: Chen-Yu Tsai, linux-mediatek, devicetree, linux-arm-kernel,
chrome-platform, linux-input, linux-i2c, linux-kernel
Hi everyone,
This series fixes the trackpad descriptions on some MediaTek-based
Chromebooks: either the trackpad's supply was set as always-on to
workaround missing delays in the driver, or the delay and supply
are missing from the trackpad's device node.
v1 was just the first patch [1]. It has since grown to cover multiple
drivers and devices.
Patch 1 adds the correct enable delay after enabling the supply regulator
for the Elan trackpad to initialize. Compared to v1, the delay is now
skipped if the regulator was already enabled to avoid impacting boot
time or time before the trackpad is operational.
Patch 2 applies the same logic of skipping the power on delay to the
i2c-hid-of driver.
Patch 3 applies the same logic of skipping the power on delay to the
i2c OF component prober library.
Patch 4 adds a delay between when the device node found is enabled and
when regulator_disable() is called. This gives an asynchronously probing
driver some time to increment the enable count of their regulator
reference, thus keeping the device operational and allowing the driver
to skip the initialization delay.
Patch 5 adds the correct delay for probing trackpads for Hana devices
to the ChromeOS OF component prober.
Patch 6 removes the "always-on" setting from the trackpad supply for
Elm / Hana and adds the correct delay to the second source trackpad.
This corrects the hardware description.
Patch 7 adds the supply and power on delay properties to the Synaptics
trackpad on the Spherion device. Combined with previous driver changes
this should cause no actual functional changes or delays.
Please take a look. There are no build time dependencies between any
of the patches, but the DT changes must go in after all the driver
changes land, especially the first one adding delays to the Elan
trackpad driver. Otherwise one could potentially end up with a
non-functional trackpad on the device.
Thanks
ChenYu
[1] https://lore.kernel.org/all/20241001093815.2481899-1-wenst@chromium.org/
Chen-Yu Tsai (7):
Input: elan_i2c - Wait for initialization after enabling regulator
supply
HID: i2c-hid-of: skip post-power-on delay if already powered on
i2c: of-prober: skip post-power-on delay if already powered on
i2c: of-prober: Defer regulator_disable() on successful probe in
simple helper
platform/chrome: of_hw_prober: Add delay for hana trackpads
arm64: dts: mediatek: mt8173-elm-hana: Unmark trackpad supply as
always-on
arm64: dts: mediatek: mt8192-asurada-spherion: Add Synaptics
trackpad's supply
.../boot/dts/mediatek/mt8173-elm-hana.dtsi | 8 +----
arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 1 -
.../mediatek/mt8192-asurada-spherion-r0.dts | 2 ++
drivers/hid/i2c-hid/i2c-hid-of.c | 8 ++++-
drivers/i2c/i2c-core-of-prober.c | 29 +++++++++++++++----
drivers/input/mouse/elan_i2c_core.c | 13 +++++++++
.../platform/chrome/chromeos_of_hw_prober.c | 4 +--
7 files changed, 48 insertions(+), 17 deletions(-)
--
2.55.0.rc0.799.gd6f94ed593-goog
^ permalink raw reply
* Re: [PATCH 09/10] HID: steam: Reject short reads
From: Yousef Alhouseen @ 2026-07-03 11:26 UTC (permalink / raw)
To: vi; +Cc: jikos, bentiss, linux-input, syzbot+75f3f9bff8c510602d36
In-Reply-To: <20260702222145.1863104-9-vi@endrift.com>
Hi Vicki,
I think the length comparison is reversed here.
hid_hw_raw_request() includes the report-ID byte in ret, while
steam_recv_report() strips that byte before copying, so only ret - 1
bytes are present in data. The encoded report body is data[1] + 2
bytes (command, length, and payload). A short report therefore
satisfies:
ret - 1 < data[1] + 2
or equivalently:
ret < data[1] + 3
The current data[1] > ret + 2 test will miss the usual short-read
cases. Also, when ret == 2, only data[0] was copied, so reading
data[1] is itself uninitialized despite the ret >= 2 guard.
Perhaps the check could be structured as:
if (ret > 0 && (ret < 3 || ret < data[1] + 3))
return -EPROTO;
with the diagnostic adjusted depending on whether the two-byte header
was present.
Thanks,
Yousef
On Thu, 2 Jul 2026 15:21:42 -0700, Vicki Pfau <vi@endrift.com> wrote:
> Steam Controller FEATURE reports encode the size of the message in the
> message itself. Previously we were trusting that the size reported matched
> the size we actually read, leading to a potential issue with short reads.
> Instead, we should actually verify the length of the read.
>
> Fixes: c164d6abf384 ("HID: add driver for Valve Steam Controller")
> Reported-by: syzbot+75f3f9bff8c510602d36@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=75f3f9bff8c510602d36
>
> Signed-off-by: Vicki Pfau <vi@endrift.com>
> ---
> drivers/hid/hid-steam.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 593151709cf1..e97431bc2828 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -389,6 +389,12 @@ static int steam_recv_report(struct steam_device *steam,
> hid_err(steam->hdev, "%s: error %d\n", __func__, ret);
> else
> hid_dbg(steam->hdev, "Received report %*ph\n", ret, data);
> +
> + if (ret >= 2 && data[1] > ret + 2) {
> + hid_err(steam->hdev, "%s: expected %u bytes, read %i\n",
> + __func__, data[1] + 2, ret);
> + return -EPROTO;
> + }
> return ret;
> }
>
> --
> 2.54.0
^ permalink raw reply
* Re: [PATCH v9 4/7] input: keyboard: Add driver for ASUS Transformer dock multimedia keys
From: Svyatoslav Ryhel @ 2026-07-03 8:53 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Michał Mirosław, Ion Agorria,
Svyatoslav Ryhel, Sebastian Reichel, Pavel Machek, Lee Jones,
Conor Dooley, Krzysztof Kozlowski, devicetree, linux-kernel,
linux-input, linux-leds, linux-pm
In-Reply-To: <20260625081529.22447-5-clamor95@gmail.com>
чт, 25 черв. 2026 р. о 11:16 Svyatoslav Ryhel <clamor95@gmail.com> пише:
>
> 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.
> Only Dock keyboard input events are tracked for AltGr pressing.
>
> Co-developed-by: Ion Agorria <ion@agorria.com>
> Signed-off-by: Ion Agorria <ion@agorria.com>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
> drivers/input/keyboard/Kconfig | 10 +
> drivers/input/keyboard/Makefile | 1 +
> .../input/keyboard/asus-transformer-ec-keys.c | 314 ++++++++++++++++++
> 3 files changed, 325 insertions(+)
> create mode 100644 drivers/input/keyboard/asus-transformer-ec-keys.c
>
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 9d1019ba0245..913cb4900565 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -89,6 +89,16 @@ config KEYBOARD_APPLESPI
> To compile this driver as a module, choose M here: the
> module will be called applespi.
>
> +config KEYBOARD_ASUS_TRANSFORMER_EC
> + tristate "Asus Transformer's Mobile Dock multimedia keys"
> + depends on MFD_ASUS_TRANSFORMER_EC
> + help
> + Say Y here if you want to use multimedia keys present on Asus
> + Transformer's Mobile Dock.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called asus-transformer-ec-keys.
> +
> config KEYBOARD_ATARI
> tristate "Atari keyboard"
> depends on ATARI
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index 60bb7baf802f..0d81096887ad 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_KEYBOARD_ADP5585) += adp5585-keys.o
> obj-$(CONFIG_KEYBOARD_ADP5588) += adp5588-keys.o
> obj-$(CONFIG_KEYBOARD_AMIGA) += amikbd.o
> obj-$(CONFIG_KEYBOARD_APPLESPI) += applespi.o
> +obj-$(CONFIG_KEYBOARD_ASUS_TRANSFORMER_EC) += asus-transformer-ec-keys.o
> obj-$(CONFIG_KEYBOARD_ATARI) += atakbd.o
> obj-$(CONFIG_KEYBOARD_ATKBD) += atkbd.o
> obj-$(CONFIG_KEYBOARD_BCM) += bcm-keypad.o
> diff --git a/drivers/input/keyboard/asus-transformer-ec-keys.c b/drivers/input/keyboard/asus-transformer-ec-keys.c
> new file mode 100644
> index 000000000000..53aff3ce7146
> --- /dev/null
> +++ b/drivers/input/keyboard/asus-transformer-ec-keys.c
> @@ -0,0 +1,314 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <linux/array_size.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/mfd/asus-transformer-ec.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define ASUSEC_EXT_KEY_CODES 0x20
> +
> +struct asus_ec_keys_data {
> + struct notifier_block nb;
> + struct asusec_core *ec;
> + struct input_dev *xidev;
> + struct input_handler input_handler;
> + unsigned short keymap[ASUSEC_EXT_KEY_CODES * 2];
> + const char *kbc_phys;
> + bool special_key_pressed;
> + bool special_key_mode;
> +};
> +
> +static void asus_ec_input_event(struct input_handle *handle,
> + unsigned int event_type,
> + unsigned int event_code, int value)
> +{
> + struct asus_ec_keys_data *priv = handle->handler->private;
> +
> + /* Store special key state */
> + if (event_type == EV_KEY && event_code == KEY_RIGHTALT)
> + priv->special_key_pressed = !!value;
> +}
> +
> +static int asus_ec_input_connect(struct input_handler *handler,
> + struct input_dev *dev,
> + const struct input_device_id *id)
> +{
> + struct asus_ec_keys_data *priv = handler->private;
> + struct input_handle *handle;
> + int error;
> +
> + if (!dev->phys || !strstr(dev->phys, priv->kbc_phys))
> + return -ENODEV;
> +
Hello Dmitry!
Would this approach be acceptable? Handler links strictly to asus-ec keyboard.
> + handle = kzalloc_obj(*handle);
> + if (!handle)
> + return -ENOMEM;
> +
> + handle->dev = dev;
> + handle->handler = handler;
> + handle->name = handler->name;
> +
> + error = input_register_handle(handle);
> + if (error)
> + goto err_free_handle;
> +
> + error = input_open_device(handle);
> + if (error)
> + goto err_unregister_handle;
> +
> + return 0;
> +
> + err_unregister_handle:
> + input_unregister_handle(handle);
> + err_free_handle:
> + kfree(handle);
> +
> + return error;
> +}
> +
> +static void asus_ec_input_disconnect(struct input_handle *handle)
> +{
> + input_close_device(handle);
> + input_unregister_handle(handle);
> + kfree(handle);
> +}
> +
> +static const struct input_device_id asus_ec_input_ids[] = {
> + {
> + .flags = INPUT_DEVICE_ID_MATCH_EVBIT,
> + .evbit = { BIT_MASK(EV_KEY) },
> + },
> + { }
> +};
> +
> +static const unsigned short asus_ec_dock_ext_keys[] = {
> + /* Function keys [0x00 - 0x19] */
> + [0x01] = KEY_DELETE,
> + [0x02] = KEY_F1,
> + [0x03] = KEY_F2,
> + [0x04] = KEY_F3,
> + [0x05] = KEY_F4,
> + [0x06] = KEY_F5,
> + [0x07] = KEY_F6,
> + [0x08] = KEY_F7,
> + [0x10] = KEY_F8,
> + [0x11] = KEY_F9,
> + [0x12] = KEY_F10,
> + [0x13] = KEY_F11,
> + [0x14] = KEY_F12,
> + [0x15] = KEY_MUTE,
> + [0x16] = KEY_VOLUMEDOWN,
> + [0x17] = KEY_VOLUMEUP,
> + /* Multimedia keys [0x20 - 0x39] */
> + [0x21] = KEY_SCREENLOCK,
> + [0x22] = KEY_WLAN,
> + [0x23] = KEY_BLUETOOTH,
> + [0x24] = KEY_TOUCHPAD_TOGGLE,
> + [0x25] = KEY_BRIGHTNESSDOWN,
> + [0x26] = KEY_BRIGHTNESSUP,
> + [0x27] = KEY_BRIGHTNESS_AUTO,
> + [0x28] = KEY_PRINT,
> + [0x30] = KEY_WWW,
> + [0x31] = KEY_CONFIG,
> + [0x32] = KEY_PREVIOUSSONG,
> + [0x33] = KEY_PLAYPAUSE,
> + [0x34] = KEY_NEXTSONG,
> + [0x35] = KEY_MUTE,
> + [0x36] = KEY_VOLUMEDOWN,
> + [0x37] = KEY_VOLUMEUP,
> +};
> +
> +static void asus_ec_keys_report_key(struct input_dev *dev, unsigned int code,
> + unsigned int key, bool value)
> +{
> + input_event(dev, EV_MSC, MSC_SCAN, code);
> + input_report_key(dev, key, value);
> + input_sync(dev);
> +}
> +
> +static int asus_ec_keys_process_key(struct input_dev *dev, u8 code)
> +{
> + struct asus_ec_keys_data *priv = dev_get_drvdata(dev->dev.parent);
> + unsigned int key = 0;
> +
> + if (code == 0)
> + return NOTIFY_DONE;
> +
> + /* Flip special key mode state when pressing SCREEN LOCK + R ALT */
> + if (priv->special_key_pressed && code == 1) {
> + priv->special_key_mode = !priv->special_key_mode;
> + return NOTIFY_DONE;
> + }
> +
> + /*
> + * Relocate code to second "page" if pressed state XOR's mode state
> + * This way special key will invert the current mode
> + */
> + if (priv->special_key_mode ^ priv->special_key_pressed)
> + code += ASUSEC_EXT_KEY_CODES;
> +
> + if (code < dev->keycodemax) {
> + unsigned short *map = dev->keycode;
> +
> + key = map[code];
> + }
> +
> + if (!key)
> + key = KEY_UNKNOWN;
> +
> + asus_ec_keys_report_key(dev, code, key, 1);
> + asus_ec_keys_report_key(dev, code, key, 0);
> +
> + return NOTIFY_OK;
> +}
> +
> +static int asus_ec_keys_notify(struct notifier_block *nb,
> + unsigned long action, void *data_)
> +{
> + struct asus_ec_keys_data *priv =
> + container_of(nb, struct asus_ec_keys_data, nb);
> + u8 *data = data_;
> +
> + if (action & ASUSEC_SMI_MASK)
> + return NOTIFY_DONE;
> +
> + if (action & ASUSEC_SCI_MASK)
> + return asus_ec_keys_process_key(priv->xidev, data[2]);
> +
> + return NOTIFY_DONE;
> +}
> +
> +static void asus_ec_keys_setup_keymap(struct asus_ec_keys_data *priv)
> +{
> + struct input_dev *dev = priv->xidev;
> + unsigned int i;
> +
> + BUILD_BUG_ON(ARRAY_SIZE(priv->keymap) < ARRAY_SIZE(asus_ec_dock_ext_keys));
> +
> + dev->keycode = priv->keymap;
> + dev->keycodesize = sizeof(*priv->keymap);
> + dev->keycodemax = ARRAY_SIZE(priv->keymap);
> +
> + input_set_capability(dev, EV_MSC, MSC_SCAN);
> + input_set_capability(dev, EV_KEY, KEY_UNKNOWN);
> +
> + for (i = 0; i < ARRAY_SIZE(asus_ec_dock_ext_keys); i++) {
> + unsigned int code = asus_ec_dock_ext_keys[i];
> +
> + if (!code)
> + continue;
> +
> + __set_bit(code, dev->keybit);
> + priv->keymap[i] = code;
> + }
> +}
> +
> +static int asus_ec_keys_register_handler(struct device *dev,
> + struct asus_ec_keys_data *priv)
> +{
> + struct i2c_client *parent = to_i2c_client(dev->parent);
> + int error;
> +
> + priv->input_handler.event = asus_ec_input_event;
> + priv->input_handler.connect = asus_ec_input_connect;
> + priv->input_handler.disconnect = asus_ec_input_disconnect;
> + priv->input_handler.id_table = asus_ec_input_ids;
> + priv->input_handler.passive_observer = true;
> + priv->input_handler.private = priv;
> + priv->input_handler.name = devm_kasprintf(dev, GFP_KERNEL,
> + "%s-media-handler",
> + priv->ec->name);
> + if (!priv->input_handler.name)
> + return -ENOMEM;
> +
> + priv->kbc_phys = devm_kasprintf(dev, GFP_KERNEL, "i2c-%u-%04x/serio0",
> + i2c_adapter_id(parent->adapter),
> + parent->addr);
> + if (!priv->kbc_phys)
> + return -ENOMEM;
> +
> + error = input_register_handler(&priv->input_handler);
> + if (error)
> + return error;
> +
> + return 0;
> +}
> +
> +static int asus_ec_keys_probe(struct platform_device *pdev)
> +{
> + struct i2c_client *parent = to_i2c_client(pdev->dev.parent);
> + struct asusec_core *ec = dev_get_drvdata(pdev->dev.parent);
> + struct device *dev = &pdev->dev;
> + struct asus_ec_keys_data *priv;
> + int error;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, priv);
> + priv->ec = ec;
> +
> + priv->xidev = devm_input_allocate_device(dev);
> + if (!priv->xidev)
> + return -ENOMEM;
> +
> + priv->xidev->name = devm_kasprintf(dev, GFP_KERNEL, "%s Keyboard Ext",
> + ec->model);
> + priv->xidev->phys = devm_kasprintf(dev, GFP_KERNEL, "i2c-%u-%04x",
> + i2c_adapter_id(parent->adapter),
> + parent->addr);
> +
> + if (!priv->xidev->name || !priv->xidev->phys)
> + return -ENOMEM;
> +
> + asus_ec_keys_setup_keymap(priv);
> +
> + error = input_register_device(priv->xidev);
> + if (error)
> + return dev_err_probe(dev, error,
> + "failed to register extension keys\n");
> +
> + error = asus_ec_keys_register_handler(dev, priv);
> + if (error) {
> + input_unregister_device(priv->xidev);
> + return error;
> + }
> +
> + priv->nb.notifier_call = asus_ec_keys_notify;
> +
> + error = blocking_notifier_chain_register(&ec->notify_list, &priv->nb);
> + if (error) {
> + input_unregister_device(priv->xidev);
> + input_unregister_handler(&priv->input_handler);
> + return error;
> + }
> +
> + return 0;
> +}
> +
> +static void asus_ec_keys_remove(struct platform_device *pdev)
> +{
> + struct asus_ec_keys_data *priv = platform_get_drvdata(pdev);
> + struct asusec_core *ec = priv->ec;
> +
> + blocking_notifier_chain_unregister(&ec->notify_list, &priv->nb);
> + input_unregister_handler(&priv->input_handler);
> + input_unregister_device(priv->xidev);
> +}
> +
> +static struct platform_driver asus_ec_keys_driver = {
> + .driver.name = "asus-transformer-ec-keys",
> + .probe = asus_ec_keys_probe,
> + .remove = asus_ec_keys_remove,
> +};
> +module_platform_driver(asus_ec_keys_driver);
> +
> +MODULE_ALIAS("platform:asus-transformer-ec-keys");
> +MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>");
> +MODULE_DESCRIPTION("ASUS Transformer's multimedia keys driver");
> +MODULE_LICENSE("GPL");
> --
> 2.53.0
>
^ permalink raw reply
* Re: [PATCH v2 1/3] HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA
From: sashiko-bot @ 2026-07-03 8:12 UTC (permalink / raw)
To: Even Xu; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260703075858.2780398-2-even.xu@intel.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [High] thc_rxdma_reset() fails to re-initialize the RxDMA PRD tables, leading to hardware reading shrunken buffer sizes on restart.
- [High] thc_rxdma_reset() unsafely clears PRD base addresses for all DMA channels while TX or SW DMA engines may still be running.
- [Medium] thc_rxdma_reset() ignores the return value of thc_interrupt_quiesce(dev, false) on the success path.
Pre-existing issues:
- [High] Incorrect polling condition and execution order in thc_interrupt_quiesce during unquiesce (int_quiesce = false).
--
commit b0beafd1bc7a02f7a2277c5a6d69c17c58724f01
Author: Even Xu <even.xu@intel.com>
HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA
Add a helper function thc_rxdma_reset() to do read DMA reset, it can be
called when fatal DMA error happens.
> diff --git a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
> index 6ee675e0a7384..7ceb8aeeccd37 100644
> --- a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
> +++ b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
> @@ -561,6 +561,57 @@ static int thc_wait_for_dma_pause(struct thc_device *dev, enum thc_dma_channel c
[ ... ]
> +int thc_rxdma_reset(struct thc_device *dev)
> +{
[ ... ]
> + ret = thc_wait_for_dma_pause(dev, THC_RXDMA2);
> + if (ret) {
> + dev_err(dev->dev, "Wait for RxDMA2 pause failed during RxDMA reset\n");
> + goto end;
> + }
> +
> + thc_dma_unconfigure(dev);
[Severity: High]
Could this cause memory corruption or IOMMU faults if TX or SW DMA engines
are still running?
thc_rxdma_reset() explicitly pauses RxDMA1 and RxDMA2 but does not pause
TXDMA or SWDMA. It then calls thc_dma_unconfigure(), which loops over all
DMA channels (MAX_THC_DMA_CHANNEL) and blindly overwrites their PRD base
addresses to 0x0.
If TXDMA was left running (for instance, if a prior thc_dma_write() timed
out without clearing the TXDMA start bit), setting its PRD base address to 0
while it is actively fetching could cause the hardware to fetch descriptors
from physical address 0.
> +
> + ret = thc_dma_configure(dev);
[Severity: High]
Does this leave dirty 'len' descriptors in the PRD memory layout?
thc_dma_configure() restarts the DMA engine pointers to 0, but it does not
re-initialize the PRD memory layout. The hardware updates the 'len' field
of PRD entries to indicate the actual length of received packets.
Restarting the DMA from index 0 without restoring 'len' to the maximum
buffer capacity could cause the hardware to use previously received smaller
packet sizes as the maximum capacity for new transfers, resulting in
truncated packets or DMA errors.
> + if (ret) {
> + dev_err(dev->dev, "Re-config DMA failed during RxDMA reset\n");
> + goto end;
> + }
> +
> + thc_interrupt_quiesce(dev, false);
[Severity: Medium]
Is it safe to ignore the return value of thc_interrupt_quiesce() here?
Discarding this return value on the success path could silently hide a
hardware timeout or regmap read failure, potentially leaving interrupts
disabled while the function reports success.
[Severity: High]
This is a pre-existing issue, but is the polling condition and execution
order in thc_interrupt_quiesce() correct when unquiescing?
When thc_interrupt_quiesce() is called with int_quiesce = false, it polls
the HW_STS bit expecting it to be 1 BEFORE it clears the QUIESCE_EN bit:
drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.c:thc_interrupt_quiesce() {
...
ret = regmap_read_poll_timeout(dev->thc_regmap, THC_M_PRT_CONTROL_OFFSET, ctrl,
ctrl & THC_M_PRT_CONTROL_THC_DEVINT_QUIESCE_HW_STS,
THC_REGMAP_POLLING_INTERVAL_US, THC_QUIESCE_EN_TIMEOUT_US);
...
/* Unquiesce device interrupt - Clear the quiesce bit */
if (!int_quiesce)
regmap_write_bits(dev->thc_regmap, THC_M_PRT_CONTROL_OFFSET,
THC_M_PRT_CONTROL_THC_DEVINT_QUIESCE_EN, 0);
}
Since the device is already quiesced, HW_STS is already 1, causing the poll
to return immediately. It then clears the QUIESCE_EN bit and returns without
ever waiting for HW_STS to become 0 to acknowledge the unquiesce. This could
cause the driver to proceed before interrupts are actually re-enabled.
> +
> +end:
> + mutex_unlock(&dev->thc_bus_lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(thc_rxdma_reset, "INTEL_THC");
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703075858.2780398-1-even.xu@intel.com?part=1
^ permalink raw reply
* Re: [PATCH v2 2/3] HID: Intel-thc-hid: Intel-quicki2c: Refine recover callback
From: sashiko-bot @ 2026-07-03 8:10 UTC (permalink / raw)
To: Even Xu; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260703075858.2780398-3-even.xu@intel.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Use-After-Free due to premature `cancel_work_sync` during device removal and shutdown.
Pre-existing issues:
- [High] Missing `pci_free_irq_vectors()` in probe error paths and device removal leaks IRQ resources.
- [High] The hard IRQ handler accesses hardware registers unconditionally, violating PM subsystem guidelines.
--
commit e88da9d40562f5d0be755568ae60010a7ba925df
Author: Even Xu <even.xu@intel.com>
HID: Intel-thc-hid: Intel-quicki2c: Refine recover callback
This commit refines the recover flow by offloading it to a workqueue instead
of processing it within the IRQ handler. It simplifies the operation with a
reset API and ensures interrupts are disabled during the recovery process.
> diff --git a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> index 46d3e9a019995..11e0e129b44c3 100644
> --- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> +++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> @@ -245,28 +245,28 @@ static irqreturn_t quicki2c_irq_quick_handler(int irq, void *dev_id)
> }
[Severity: High]
This is a pre-existing issue, but does quicki2c_irq_quick_handler() access
hardware registers unconditionally?
If this device shares an INTx interrupt line, a spurious interrupt from
another device could cause quicki2c_irq_quick_handler() to execute. It then
calls thc_interrupt_enable() which writes to memory-mapped registers. If the
device is in a low-power state, this hardware access can trigger a bus fault.
Should pm_runtime_get_if_active() be used to check if the device is active
before touching the hardware registers?
[ ... ]
> @@ -771,6 +771,8 @@ static void quicki2c_remove(struct pci_dev *pdev)
> if (!qcdev)
> return;
>
> + cancel_work_sync(&qcdev->recover_work);
[Severity: High]
Can this sequence lead to a use-after-free?
cancel_work_sync() is called here, but hardware interrupts are not yet
quiesced, and the state is still active. If an interrupt fires immediately
after cancel_work_sync(), quicki2c_irq_thread_handler() can execute and call:
schedule_work(&qcdev->recover_work);
This re-enqueues the work item. After quicki2c_remove() returns, devres
frees the qcdev structure. When the scheduled work later executes, it will
dereference the freed qcdev pointer.
Should the work be cancelled after interrupts are disabled and the device
state is set to QUICKI2C_DISABLED?
> quicki2c_hid_remove(qcdev);
> quicki2c_dma_deinit(qcdev);
>
> pm_runtime_get_noresume(qcdev->dev);
>
> quicki2c_dev_deinit(qcdev);
>
> pci_clear_master(pdev);
> }
[Severity: High]
This is a pre-existing issue, but does quicki2c_remove() leak IRQ resources?
The remove path unbinds the device but doesn't appear to call
pci_free_irq_vectors(). Similarly, if quicki2c_probe() fails after
pci_alloc_irq_vectors(), the error path (disable_pci_device) does not
free the vectors.
Repeated unbind/bind operations or probe deferrals will continually leak
these vectors, which can eventually exhaust system IRQ capacity.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703075858.2780398-1-even.xu@intel.com?part=2
^ permalink raw reply
* Re: [PATCH v2 3/3] HID: Intel-thc-hid: Intel-quickspi: Refine recover callback
From: sashiko-bot @ 2026-07-03 8:08 UTC (permalink / raw)
To: Even Xu; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260703075858.2780398-4-even.xu@intel.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `cancel_work_sync()` is called before interrupts are safely disabled during device teardown, and is missing entirely from the probe error path, allowing the work item to execute after the device structure is freed.
- [High] Non-fatal or spurious interrupts occurring during async recovery will prematurely re-enable hardware interrupts, racing with the DMA reset and potentially causing DMA corruption.
--
commit f745eb96a11ec6734e09736d8674beb349a74ab9
Author: Even Xu <even.xu@intel.com>
HID: Intel-thc-hid: Intel-quickspi: Refine recover callback
This commit shifts the QuickSPI recover flow from the IRQ handler to a
workqueue. It relies on thc_rxdma_reset() for recovery and keeps interrupts
disabled during the process.
> diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
[ ... ]
> @@ -337,11 +331,10 @@ static irqreturn_t quickspi_irq_thread_handler(int irq, void *dev_id)
> }
>
> end:
> - thc_interrupt_enable(qsdev->thc_hw, true);
> -
> if (err_recover)
> - if (try_recover(qsdev))
> - qsdev->state = QUICKSPI_DISABLED;
> + schedule_work(&qsdev->recover_work);
> + else
> + thc_interrupt_enable(qsdev->thc_hw, true);
[Severity: High]
If try_recover() disables device interrupts and is actively resetting DMA
engines, could a spurious or non-fatal interrupt cause
quickspi_irq_thread_handler() to evaluate err_recover == 0 and unconditionally
re-enable hardware interrupts here?
If so, a subsequent real interrupt could race with the ongoing DMA reset
since thc_rxdma_read() does not appear to hold thc_bus_lock.
>
> pm_runtime_put_autosuspend(qsdev->dev);
>
[ ... ]
> @@ -710,6 +704,8 @@ static void quickspi_remove(struct pci_dev *pdev)
> if (!qsdev)
> return;
>
> + cancel_work_sync(&qsdev->recover_work);
> +
[Severity: High]
Since quickspi_dev_deinit() disables hardware interrupts later in this function,
is there a race window here?
If a fatal error interrupt occurs after cancel_work_sync() but before
hardware interrupts are disabled, the IRQ thread handler could schedule
recover_work again. The qsdev structure would then be freed, leaving a
pending work item that dereferences freed memory.
Also, should cancel_work_sync() be called in the error paths of
quickspi_probe() after the work item is initialized and interrupts are
enabled?
> quickspi_hid_remove(qsdev);
> quickspi_dma_deinit(qsdev);
>
[ ... ]
> @@ -737,6 +733,8 @@ static void quickspi_shutdown(struct pci_dev *pdev)
> if (!qsdev)
> return;
>
> + cancel_work_sync(&qsdev->recover_work);
> +
[Severity: High]
Could this have the same race window as quickspi_remove(), where a fatal
error interrupt might reschedule recover_work before quickspi_dev_deinit()
disables interrupts?
> /* Must stop DMA before reboot to avoid DMA entering into unknown state */
> quickspi_dma_deinit(qsdev);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703075858.2780398-1-even.xu@intel.com?part=3
^ permalink raw reply
* [PATCH v2 3/3] HID: Intel-thc-hid: Intel-quickspi: Refine recover callback
From: Even Xu @ 2026-07-03 7:58 UTC (permalink / raw)
To: bentiss, jikos; +Cc: srinivas.pandruvada, linux-input, linux-kernel, Even Xu
In-Reply-To: <20260703075858.2780398-1-even.xu@intel.com>
Refine recover flow:
1. Use workqueue to handle recover flow instead of processing in irq
handler.
2. Call thc_rxdma_reset() API to simplify the recover operation.
3. Disable interrupt during whole recover flow.
4. If recover fails, disable interrupt to avoid interrupt storm.
Signed-off-by: Even Xu <even.xu@intel.com>
---
.../intel-quickspi/pci-quickspi.c | 46 +++++++++----------
.../intel-quickspi/quickspi-dev.h | 3 ++
2 files changed, 25 insertions(+), 24 deletions(-)
diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
index f669235f1883..bc9eead77562 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
@@ -252,34 +252,28 @@ static irqreturn_t quickspi_irq_quick_handler(int irq, void *dev_id)
}
/**
- * try_recover - Try to recovery THC and Device
- * @qsdev: pointer to quickspi device
+ * try_recover - Recover callback to recover THC
+ * @work: pointer to work_struct
*
- * This function is a error handler, called when fatal error happens.
- * It try to reset Touch Device and re-configure THC to recovery
+ * This function is an error handler, called when fatal error happens.
+ * It try to reset Touch Device and re-configure THC to recover
* transferring between Device and THC.
- *
- * Return: 0 if successful or error code on failed.
*/
-static int try_recover(struct quickspi_device *qsdev)
+static void try_recover(struct work_struct *work)
{
- int ret;
+ struct quickspi_device *qsdev = container_of(work, struct quickspi_device, recover_work);
- ret = reset_tic(qsdev);
- if (ret) {
- dev_err(qsdev->dev, "Reset touch device failed, ret = %d\n", ret);
- return ret;
- }
+ if (pm_runtime_resume_and_get(qsdev->dev))
+ return;
- thc_dma_unconfigure(qsdev->thc_hw);
+ thc_interrupt_enable(qsdev->thc_hw, false);
- ret = thc_dma_configure(qsdev->thc_hw);
- if (ret) {
- dev_err(qsdev->dev, "Re-configure THC DMA failed, ret = %d\n", ret);
- return ret;
- }
+ if (thc_rxdma_reset(qsdev->thc_hw))
+ qsdev->state = QUICKSPI_DISABLED;
+ else
+ thc_interrupt_enable(qsdev->thc_hw, true);
- return 0;
+ pm_runtime_put_autosuspend(qsdev->dev);
}
/**
@@ -337,11 +331,10 @@ static irqreturn_t quickspi_irq_thread_handler(int irq, void *dev_id)
}
end:
- thc_interrupt_enable(qsdev->thc_hw, true);
-
if (err_recover)
- if (try_recover(qsdev))
- qsdev->state = QUICKSPI_DISABLED;
+ schedule_work(&qsdev->recover_work);
+ else
+ thc_interrupt_enable(qsdev->thc_hw, true);
pm_runtime_put_autosuspend(qsdev->dev);
@@ -385,6 +378,7 @@ static struct quickspi_device *quickspi_dev_init(struct pci_dev *pdev, void __io
init_waitqueue_head(&qsdev->report_desc_got_wq);
init_waitqueue_head(&qsdev->get_report_cmpl_wq);
init_waitqueue_head(&qsdev->set_report_cmpl_wq);
+ INIT_WORK(&qsdev->recover_work, try_recover);
/* thc hw init */
qsdev->thc_hw = thc_dev_init(qsdev->dev, qsdev->mem_addr);
@@ -710,6 +704,8 @@ static void quickspi_remove(struct pci_dev *pdev)
if (!qsdev)
return;
+ cancel_work_sync(&qsdev->recover_work);
+
quickspi_hid_remove(qsdev);
quickspi_dma_deinit(qsdev);
@@ -737,6 +733,8 @@ static void quickspi_shutdown(struct pci_dev *pdev)
if (!qsdev)
return;
+ cancel_work_sync(&qsdev->recover_work);
+
/* Must stop DMA before reboot to avoid DMA entering into unknown state */
quickspi_dma_deinit(qsdev);
diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h
index bf5e18f5a5f4..363e589b0bde 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h
@@ -8,6 +8,7 @@
#include <linux/hid-over-spi.h>
#include <linux/sizes.h>
#include <linux/wait.h>
+#include <linux/workqueue.h>
#include "quickspi-protocol.h"
@@ -173,6 +174,8 @@ struct quickspi_device {
wait_queue_head_t set_report_cmpl_wq;
bool set_report_cmpl;
+
+ struct work_struct recover_work;
};
#endif /* _QUICKSPI_DEV_H_ */
--
2.43.0
^ permalink raw reply related
* [PATCH v2 2/3] HID: Intel-thc-hid: Intel-quicki2c: Refine recover callback
From: Even Xu @ 2026-07-03 7:58 UTC (permalink / raw)
To: bentiss, jikos; +Cc: srinivas.pandruvada, linux-input, linux-kernel, Even Xu
In-Reply-To: <20260703075858.2780398-1-even.xu@intel.com>
Refine recover flow:
1. Use workqueue to handle recover flow instead of processing in irq
handler.
2. Call thc_rxdma_reset() API to simplify the recover operation.
3. Disable interrupt during whole recover flow.
4. If recover fails, disable interrupt to avoid interrupt storm.
Signed-off-by: Even Xu <even.xu@intel.com>
---
.../intel-quicki2c/pci-quicki2c.c | 42 ++++++++++---------
.../intel-quicki2c/quicki2c-dev.h | 2 +
2 files changed, 25 insertions(+), 19 deletions(-)
diff --git a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
index 46d3e9a01999..11e0e129b44c 100644
--- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
+++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
@@ -245,28 +245,28 @@ static irqreturn_t quicki2c_irq_quick_handler(int irq, void *dev_id)
}
/**
- * try_recover - Try to recovery THC and Device
- * @qcdev: Pointer to quicki2c_device structure
+ * try_recover - Recover callback to recover THC
+ * @work: pointer to work_struct
*
* This function is an error handler, called when fatal error happens.
- * It try to reset touch device and re-configure THC to recovery
- * communication between touch device and THC.
- *
- * Return: 0 if successful or error code on failure
+ * It try to reset Touch Device and re-configure THC to recover
+ * transferring between Device and THC.
*/
-static int try_recover(struct quicki2c_device *qcdev)
+static void try_recover(struct work_struct *work)
{
- int ret;
+ struct quicki2c_device *qcdev = container_of(work, struct quicki2c_device, recover_work);
- thc_dma_unconfigure(qcdev->thc_hw);
+ if (pm_runtime_resume_and_get(qcdev->dev))
+ return;
- ret = thc_dma_configure(qcdev->thc_hw);
- if (ret) {
- dev_err(qcdev->dev, "Reconfig DMA failed\n");
- return ret;
- }
+ thc_interrupt_enable(qcdev->thc_hw, false);
- return 0;
+ if (thc_rxdma_reset(qcdev->thc_hw))
+ qcdev->state = QUICKI2C_DISABLED;
+ else
+ thc_interrupt_enable(qcdev->thc_hw, true);
+
+ pm_runtime_put_autosuspend(qcdev->dev);
}
static int handle_input_report(struct quicki2c_device *qcdev)
@@ -343,11 +343,10 @@ static irqreturn_t quicki2c_irq_thread_handler(int irq, void *dev_id)
}
exit:
- thc_interrupt_enable(qcdev->thc_hw, true);
-
if (err_recover)
- if (try_recover(qcdev))
- qcdev->state = QUICKI2C_DISABLED;
+ schedule_work(&qcdev->recover_work);
+ else
+ thc_interrupt_enable(qcdev->thc_hw, true);
pm_runtime_put_autosuspend(qcdev->dev);
@@ -386,6 +385,7 @@ static struct quicki2c_device *quicki2c_dev_init(struct pci_dev *pdev, void __io
qcdev->ddata = ddata;
init_waitqueue_head(&qcdev->reset_ack_wq);
+ INIT_WORK(&qcdev->recover_work, try_recover);
/* THC hardware init */
qcdev->thc_hw = thc_dev_init(qcdev->dev, qcdev->mem_addr);
@@ -771,6 +771,8 @@ static void quicki2c_remove(struct pci_dev *pdev)
if (!qcdev)
return;
+ cancel_work_sync(&qcdev->recover_work);
+
quicki2c_hid_remove(qcdev);
quicki2c_dma_deinit(qcdev);
@@ -796,6 +798,8 @@ static void quicki2c_shutdown(struct pci_dev *pdev)
if (!qcdev)
return;
+ cancel_work_sync(&qcdev->recover_work);
+
/* Must stop DMA before reboot to avoid DMA entering into unknown state */
quicki2c_dma_deinit(qcdev);
diff --git a/drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-dev.h b/drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-dev.h
index 61dbdece59a1..aedf85291e60 100644
--- a/drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-dev.h
+++ b/drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-dev.h
@@ -222,6 +222,8 @@ struct quicki2c_device {
wait_queue_head_t reset_ack_wq;
bool reset_ack;
+ struct work_struct recover_work;
+
u32 i2c_max_frame_size_enable;
u32 i2c_max_frame_size;
u32 i2c_int_delay_enable;
--
2.43.0
^ permalink raw reply related
* [PATCH v2 1/3] HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA
From: Even Xu @ 2026-07-03 7:58 UTC (permalink / raw)
To: bentiss, jikos; +Cc: srinivas.pandruvada, linux-input, linux-kernel, Even Xu
In-Reply-To: <20260703075858.2780398-1-even.xu@intel.com>
Add a helper function thc_rxdma_reset() to do read DMA reset, it can be
called when fatal DMA error happens.
Signed-off-by: Even Xu <even.xu@intel.com>
---
.../intel-thc-hid/intel-thc/intel-thc-dma.c | 51 +++++++++++++++++++
.../intel-thc-hid/intel-thc/intel-thc-dma.h | 1 +
2 files changed, 52 insertions(+)
diff --git a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
index 6ee675e0a738..7ceb8aeeccd3 100644
--- a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
+++ b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
@@ -561,6 +561,57 @@ static int thc_wait_for_dma_pause(struct thc_device *dev, enum thc_dma_channel c
return 0;
}
+/**
+ * thc_rxdma_reset - Reset all read DMA engines
+ *
+ * @dev: The pointer of THC private device context
+ *
+ * This is a helper function to reset RxDMA configure. It's typically used
+ * for RxDMA recovery when fatal error happens.
+ *
+ * Return: 0 if successful or error code on failure.
+ */
+int thc_rxdma_reset(struct thc_device *dev)
+{
+ int ret;
+
+ if (mutex_lock_interruptible(&dev->thc_bus_lock))
+ return -EINTR;
+
+ ret = thc_interrupt_quiesce(dev, true);
+ if (ret) {
+ dev_err(dev->dev, "Quiesce interrupt failed during RxDMA reset\n");
+ goto end;
+ }
+
+ ret = thc_wait_for_dma_pause(dev, THC_RXDMA1);
+ if (ret) {
+ dev_err(dev->dev, "Wait for RxDMA1 pause failed during RxDMA reset\n");
+ goto end;
+ }
+
+ ret = thc_wait_for_dma_pause(dev, THC_RXDMA2);
+ if (ret) {
+ dev_err(dev->dev, "Wait for RxDMA2 pause failed during RxDMA reset\n");
+ goto end;
+ }
+
+ thc_dma_unconfigure(dev);
+
+ ret = thc_dma_configure(dev);
+ if (ret) {
+ dev_err(dev->dev, "Re-config DMA failed during RxDMA reset\n");
+ goto end;
+ }
+
+ thc_interrupt_quiesce(dev, false);
+
+end:
+ mutex_unlock(&dev->thc_bus_lock);
+ return ret;
+}
+EXPORT_SYMBOL_NS_GPL(thc_rxdma_reset, "INTEL_THC");
+
static int read_dma_buffer(struct thc_device *dev,
struct thc_dma_configuration *read_config,
u8 prd_table_index, void *read_buff)
diff --git a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.h b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.h
index 541d33995baf..715423453a9d 100644
--- a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.h
+++ b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.h
@@ -145,6 +145,7 @@ int thc_dma_allocate(struct thc_device *dev);
int thc_dma_configure(struct thc_device *dev);
void thc_dma_unconfigure(struct thc_device *dev);
void thc_dma_release(struct thc_device *dev);
+int thc_rxdma_reset(struct thc_device *dev);
int thc_rxdma_read(struct thc_device *dev, enum thc_dma_channel dma_channel,
void *read_buff, size_t *read_len, int *read_finished);
int thc_swdma_read(struct thc_device *dev, void *write_buff, size_t write_len,
--
2.43.0
^ permalink raw reply related
* [PATCH v2 0/3] HID: Intel-thc-hid: Refine error recovery flow
From: Even Xu @ 2026-07-03 7:58 UTC (permalink / raw)
To: bentiss, jikos; +Cc: srinivas.pandruvada, linux-input, linux-kernel, Even Xu
This series refines the fatal error recovery flow for the Intel THC
(Touch Host Controller) subsystem, covering both the QuickI2C and
QuickSPI drivers.
Currently, when a fatal DMA error is detected in the IRQ thread handler,
the recovery is performed inline: the interrupt handler calls
try_recover() directly, which unconfigures and reconfigures the DMA
engine.
This approach has several problems:
1. Recovery runs in the IRQ thread context, which is not ideal for
potentially slow reset operations.
2. The interrupt is re-enabled before recovery completes, risking an
interrupt storm if DMA errors persist.
3. The DMA reset logic is open-coded in each protocol driver, leading
to duplication and divergence over time.
This patch series addresses all of the above:
By adding a new thc_rxdma_reset() API to the THC core layer, QuickI2C
and QuickSPI drivers can call it respectively to refine the recovery
callback.
The synchronous try_recover() call in the IRQ thread is replaced with
schedule_work(), deferring recovery to a workqueue. Within the work
function:
- The interrupt line is disabled before any DMA manipulation.
- thc_rxdma_reset() is used instead of the open-coded sequence.
- On failure the device is marked DISABLED and the interrupt remains
off, preventing an interrupt storm.
Change log:
v2:
- Use dev_err() instead of dev_err_once() so repeated failures during
recurring recovery are not silently suppressed.
- Pause both RxDMA channels via thc_wait_for_dma_pause() before calling
thc_dma_unconfigure() to ensure the DMA engines are inactive before
clearing PRD base addresses, preventing potential IOMMU faults or
memory corruption.
- Hold a runtime PM reference inside try_recover() to prevent the
device from suspending while the work accesses hardware registers.
- Add cancel_work_sync() in quicki2c_remove() and quicki2c_shutdown()
to prevent use-after-free if recovery work is still queued at teardown.
- Only re-enable the interrupt in the IRQ thread handler when no recovery
is needed; the work function handles re-enabling after successful reset,
avoiding an interrupt storm from the uncleared hardware error state.
Even Xu (3):
HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA
HID: Intel-thc-hid: Intel-quicki2c: Refine recover callback
HID: Intel-thc-hid: Intel-quickspi: Refine recover callback
.../intel-quicki2c/pci-quicki2c.c | 42 ++++++++-------
.../intel-quicki2c/quicki2c-dev.h | 2 +
.../intel-quickspi/pci-quickspi.c | 46 ++++++++---------
.../intel-quickspi/quickspi-dev.h | 3 ++
.../intel-thc-hid/intel-thc/intel-thc-dma.c | 51 +++++++++++++++++++
.../intel-thc-hid/intel-thc/intel-thc-dma.h | 1 +
6 files changed, 102 insertions(+), 43 deletions(-)
--
2.43.0
^ permalink raw reply
* Re: [PATCH] Input: maplemouse - fix NULL pointer dereference in open()
From: Dmitry Torokhov @ 2026-07-03 5:46 UTC (permalink / raw)
To: Florian Fuchs; +Cc: linux-input, linux-sh, Guenter Roeck, linux-kernel
In-Reply-To: <20260628230715.2982552-1-fuchsfl@gmail.com>
Hi Florian,
On Mon, Jun 29, 2026 at 01:07:15AM +0200, Florian Fuchs wrote:
> Commit 555c765b0cc2 ("Input: mouse - drop unnecessary calls to
> input_set_drvdata") dropped the input_set_drvdata() call in probe
> because the data appeared to be unused. However, dc_mouse_open() and
> dc_mouse_close() were using maple_get_drvdata(to_maple_dev(&dev->dev)).
> This actually retrieves driver data from the input device's embedded
> struct device. After input_set_drvdata() was removed, that lookup started
> returning NULL and opening the input device dereferences mse->mdev.
>
> Restore input_set_drvdata() and convert open() and close() to use
> input_get_drvdata() so the dependency is no longer hidden.
Thank you for the patch. While the patch itself is correct I believe the
main culprit is actually 6b3480855aad ("maple: input: fix up maple mouse driver")
that introduced incorrect use of
maple_get_drvdata() and to_maple_dev() on instance of input_dev instead
of maple_device.
I added another Fixes: tag and applied.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 4/4] dt-bindings: input: remove obsolete matrix-keymap.txt
From: Akash Sukhavasi @ 2026-07-03 5:24 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring (Arm), Lee Jones, Conor Dooley, David S. Miller,
Andrew Lunn, linux-tegra, linux-input, Mauro Carvalho Chehab,
Heiner Kallweit, Thierry Reding, linux-media, Krzysztof Kozlowski,
Jakub Kicinski, Vladimir Oltean, linux-doc, linux-kernel,
Eric Dumazet, Jonathan Hunter, Simon Horman, devicetree,
Paolo Abeni, netdev, Shuah Khan, Russell King, Jonathan Corbet
In-Reply-To: <aiSK6_n4ZnB_KRd8@google.com>
On Sat, Jun 06, 2026 at 02:03:43PM -0700, Dmitry Torokhov wrote:
> On Wed, Jun 03, 2026 at 05:26:38PM -0500, Rob Herring (Arm) wrote:
> >
> > On Wed, 03 Jun 2026 15:42:21 -0500, Akash Sukhavasi wrote:
> > > matrix-keymap.txt has been a single-line redirect to
> > > matrix-keymap.yaml since commit 639d6eda3b80 ("dt-bindings: input:
> > > Convert matrix-keymap to json-schema"), which introduced the .yaml
> > > schema and reduced the .txt to a stub in the same change. The .yaml
> > > has the same filename in the same directory, making this redirect
> > > unnecessary for discoverability.
> > >
> > > Eight instances across six files still reference matrix-keymap.txt,
> > > forcing readers through an extra hop to reach the .yaml. The stub has
> > > not been touched since June 2020. Update all references across input
> > > and mfd binding documentation to point directly to matrix-keymap.yaml
> > > and remove the stub.
> > >
> > > Signed-off-by: Akash Sukhavasi <akash.sukhavasi@gmail.com>
> > > ---
> > > v2:
> > > - Patch 4/4: corrected commit message (eight references in six files,
> > > not eight files), Sashiko review.
> > > https://sashiko.dev/#/patchset/20260529052246.4934-1-akash.sukhavasi@gmail.com?part=4
> > >
> > > v1: https://lore.kernel.org/all/20260529052246.4934-5-akash.sukhavasi@gmail.com/
> > > ---
> > > Documentation/devicetree/bindings/input/brcm,bcm-keypad.txt | 2 +-
> > > Documentation/devicetree/bindings/input/clps711x-keypad.txt | 2 +-
> > > Documentation/devicetree/bindings/input/matrix-keymap.txt | 1 -
> > > Documentation/devicetree/bindings/input/nvidia,tegra20-kbc.txt | 2 +-
> > > Documentation/devicetree/bindings/input/pxa27x-keypad.txt | 2 +-
> > > Documentation/devicetree/bindings/input/st-keyscan.txt | 2 +-
> > > Documentation/devicetree/bindings/mfd/tc3589x.txt | 6 +++---
> > > 7 files changed, 8 insertions(+), 9 deletions(-)
> > >
> >
> > Acked-by: Rob Herring (Arm) <robh@kernel.org>
> >
>
> Lee, could you please ack for MFD piece and I can take it through input?
>
> Thanks.
>
> --
> Dmitry
Friendly ping on this as well. Rob's Acked-by has been on v2 since
June 3, patch still applies cleanly.
--
Thanks,
Akash
^ permalink raw reply
* Re: [PATCH v2 2/4] dt-bindings: media: remove obsolete rc.txt
From: Akash Sukhavasi @ 2026-07-03 5:21 UTC (permalink / raw)
To: Rob Herring (Arm)
Cc: Jakub Kicinski, Jonathan Hunter, Paolo Abeni, linux-kernel,
linux-input, Heiner Kallweit, Eric Dumazet, devicetree,
Simon Horman, Mauro Carvalho Chehab, linux-media, linux-tegra,
David S. Miller, Jonathan Corbet, Shuah Khan, Conor Dooley,
Lee Jones, Andrew Lunn, Krzysztof Kozlowski, Russell King,
linux-doc, netdev, Vladimir Oltean, Dmitry Torokhov,
Thierry Reding
In-Reply-To: <178052520503.2279647.12293343206224505400.robh@kernel.org>
On Wed, Jun 03, 2026 at 05:20:05PM -0500, Rob Herring (Arm) wrote:
>
> On Wed, 03 Jun 2026 15:42:19 -0500, Akash Sukhavasi wrote:
> > rc.txt has been a single-line redirect to rc.yaml since
> > commit 7c31b9d67342 ("media: dt-bindings: media: Add YAML schemas for
> > the generic RC bindings"), which introduced the .yaml schema and
> > reduced the .txt to a stub in the same change. The .yaml has the same
> > filename in the same directory, making this redirect unnecessary
> > for discoverability.
> >
> > One file still references rc.txt, forcing readers through an extra
> > hop to reach the .yaml. The stub has not been touched since August
> > 2019. Update the reference in hix5hd2-ir.txt to point directly to
> > rc.yaml and remove the stub.
> >
> > Signed-off-by: Akash Sukhavasi <akash.sukhavasi@gmail.com>
> > ---
> > Documentation/devicetree/bindings/media/hix5hd2-ir.txt | 2 +-
> > Documentation/devicetree/bindings/media/rc.txt | 1 -
> > 2 files changed, 1 insertion(+), 2 deletions(-)
> >
>
> Acked-by: Rob Herring (Arm) <robh@kernel.org>
>
Friendly ping on this one. Rob's Acked-by has been on v2 since June 3
and the patch still applies cleanly.
--
Thanks,
Akash
^ permalink raw reply
* Re: [PATCH v7 1/4] firmware_loader: expand firmware error codes with up-to-date error
From: Marco Felsch @ 2026-07-02 23:11 UTC (permalink / raw)
To: sashiko-reviews; +Cc: dmitry.torokhov, devicetree, linux-input, conor+dt, robh
In-Reply-To: <20260702224537.0823C1F000E9@smtp.kernel.org>
On 26-07-02, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] The selftest script for firmware uploads does not inject and test the new 'firmware-duplicate' error.
> - [Low] Missing ABI documentation for the newly added sysfs error string 'firmware-duplicate'.
> --
>
> commit b451d666b20500e69051bac63ead44572b79d708
> Author: Marco Felsch <m.felsch@pengutronix.de>
>
> firmware_loader: expand firmware error codes with up-to-date error
>
> This commit adds FW_UPLOAD_ERR_DUPLICATE to allow drivers to inform the
> firmware_loader that an update is not required. It also updates the
> test_firmware module to support injecting this new error state.
>
> > diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c
> > index efc33294212fd..82c4b1fedf4cd 100644
> > --- a/drivers/base/firmware_loader/sysfs_upload.c
> > +++ b/drivers/base/firmware_loader/sysfs_upload.c
> > @@ -28,6 +28,7 @@ static const char * const fw_upload_err_str[] = {
> > [FW_UPLOAD_ERR_RW_ERROR] = "read-write-error",
> > [FW_UPLOAD_ERR_WEAROUT] = "flash-wearout",
> > [FW_UPLOAD_ERR_FW_INVALID] = "firmware-invalid",
> > + [FW_UPLOAD_ERR_DUPLICATE] = "firmware-duplicate",
>
> [Severity: Low]
> Since a new error string "firmware-duplicate" is exposed to userspace,
> should this also be documented in
> Documentation/ABI/testing/sysfs-class-firmware?
I checked the firmware-invalid which is also not documented. I could add
2nd patch to fix this if this is urgent.
>
> It looks like the new string is missing from the allowed error values
> documented for the sysfs interface.
>
> > };
> >
> > static const char *fw_upload_progress(struct device *dev,
>
> [ ... ]
>
> > diff --git a/lib/test_firmware.c b/lib/test_firmware.c
> > index 7459bba65444d..a822d9641eb4c 100644
> > --- a/lib/test_firmware.c
> > +++ b/lib/test_firmware.c
> > @@ -1134,6 +1134,7 @@ static const char * const fw_upload_err_str[] = {
> > [FW_UPLOAD_ERR_RW_ERROR] = "read-write-error",
> > [FW_UPLOAD_ERR_WEAROUT] = "flash-wearout",
> > [FW_UPLOAD_ERR_FW_INVALID] = "firmware-invalid",
> > + [FW_UPLOAD_ERR_DUPLICATE] = "firmware-duplicate",
>
> [Severity: Medium]
> Does the test script also need to be updated to exercise this new code path?
>
> While this properly adds injection support to the test module, the hardcoded
> errors list in tools/testing/selftests/firmware/fw_upload.sh does not appear
> to include "firmware-duplicate", meaning the kselftest won't actually
> inject or test this new error type.
I'm not sure how much sense it is to have a test reporting this error.
Regards,
Marco
>
> > };
> >
> > static void upload_err_inject_error(struct test_firmware_upload *tst,
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260703-v6-10-topic-touchscreen-axiom-v7-0-0d2a550a7ee8@pengutronix.de?part=1
>
--
#gernperDu
#CallMeByMyFirstName
Pengutronix e.K. | |
Steuerwalder Str. 21 | https://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
^ permalink raw reply
* Re: [PATCH v7 4/4] Input: Add TouchNetix aXiom I2C Touchscreen support
From: sashiko-bot @ 2026-07-02 22:48 UTC (permalink / raw)
To: Marco Felsch; +Cc: robh, linux-input, conor+dt, dmitry.torokhov, devicetree
In-Reply-To: <20260703-v6-10-topic-touchscreen-axiom-v7-4-0d2a550a7ee8@pengutronix.de>
Thank you for your contribution! Sashiko AI review found 18 potential issue(s) to consider:
- [High] Missing bounds checking during firmware chunk parsing allows malformed firmware to trigger integer underflow and out-of-bounds memory accesses.
- [High] Invalid cast of regmap buffer to u16* causes endian-dependent behavior and corrupts register addresses.
- [High] Unbalanced pm_runtime_put in firmware upload cleanup leads to negative PM usage counts.
- [High] Firmware-controlled allocation size allows out-of-bounds heap read when checking touch slots.
- [High] The driver prevents firmware recovery when the device boots in bootloader mode by improperly failing the probe.
- [High] fwupdate_lock is held across system calls violating kernel mutex semantics, and is double-unlocked on error paths.
- [High] Improper devm teardown order causes Use-After-Free of the input device by background firmware upload threads.
- [High] Use-after-free of ts->input in the threaded IRQ handler during config firmware updates and device removal.
- [High] The driver waits on completion primitives for hardware resets and saves without reinitializing them, causing future waits to falsely succeed immediately.
- [High] The driver passes stack-allocated buffers to I2C transfers, which violates DMA requirements and can cause memory corruption.
- [High] NULL pointer dereference in `axiom_remove_axfw_fwl_action` if `devm_add_action_or_reset` fails.
- [Medium] An operator precedence error in subtracting the preamble and postamble causes the calculated report payload length to be artificially inflated.
- [Medium] Probe error paths leak PM runtime references and fail to disable regulators.
- [Medium] I2C read errors are swallowed and falsely reported as success in polling functions.
- [Medium] Missing NULL check on `kasprintf` results in a crash or broken sysfs state when registering the firmware upload interfaces.
- [Medium] The driver lacks system sleep power management callbacks, causing the device to remain powered on during system suspend if not acting as a panel follower.
- [Medium] Dead code in `axiom_register_input_dev` prevents failing initialization when no touchslots are enabled.
- [Low] Typo in the sysfs `device_state` output violates the documented ABI.
--
commit 7ff2ad22a9b84958a9fb53d181dbd9af7a0e7b00
Author: Marco Felsch <m.felsch@pengutronix.de>
Input: Add TouchNetix aXiom I2C Touchscreen support
This patch adds initial support for the TouchNetix AX54A touchscreen
controller. It handles I2C input events, touch coordinate reporting, and
supports firmware and configuration updates via the fw_upload subsystem.
> diff --git a/drivers/input/touchscreen/touchnetix_axiom.c b/drivers/input/touchscreen/touchnetix_axiom.c
> --- /dev/null
> +++ b/drivers/input/touchscreen/touchnetix_axiom.c
[ ... ]
> +static const char *axiom_runmode_to_string(struct axiom_data *ts)
> +{
> + switch (ts->mode) {
> + case AXIOM_DISCOVERY_MODE: return "discovery";
> + case AXIOM_TCP_MODE: return "tcp";
> + case AXIOM_TCP_CFG_UPDATE_MODE: return "th2cfg-update";
> + case AXIOM_BLP_MODE: return "bootlaoder";
[Severity: Low]
Does this output "bootlaoder" instead of the documented "bootloader" state?
This typo might break userspace tools reading the device_state sysfs file.
> + default: return "unknown";
> + }
> +}
[ ... ]
> +static int axiom_u02_wait_idle(struct axiom_data *ts)
> +{
> + unsigned int reg;
> + int error, ret;
> + u16 cmd;
> +
> + if (!axiom_driver_supports_usage(ts, AXIOM_U02))
> + return -EINVAL;
> +
> + reg = axiom_usage_baseaddr(ts, AXIOM_U02);
> + reg += AXIOM_U02_REV1_COMMAND_REG;
> +
> + /*
> + * Missing regmap_raw_read_poll_timeout for now. RESP_SUCCESS means that
> + * the last command successfully completed and the device is idle.
> + */
> + error = read_poll_timeout(regmap_raw_read, ret,
> + ret || cmd == AXIOM_U02_REV1_RESP_SUCCESS,
> + 10 * USEC_PER_MSEC, 1 * USEC_PER_SEC, false,
> + ts->regmap, reg, &cmd, 2);
[Severity: Medium]
Can this swallow I2C read errors?
If regmap_raw_read fails, ret will be non-zero (an error code). The
condition evaluates to true, ending the poll successfully. The function
then incorrectly assumes commands completed successfully and returns 0.
> + if (error) {
> + dev_err(ts->dev, "Poll u02 timedout with: %#x\n", cmd);
> + return error;
> + }
> +
> + return 0;
> +}
[ ... ]
> +static int axiom_u02_swreset(struct axiom_data *ts)
> +{
> + struct axiom_u02_rev1_system_manager_msg msg = {
> + .command = cpu_to_le16(AXIOM_U02_REV1_CMD_SOFTRESET),
> + };
> + int error;
> +
> + if (!axiom_driver_supports_usage(ts, AXIOM_U02))
> + return -EINVAL;
> +
> + error = axiom_u02_send_msg(ts, &msg, false);
> + if (error)
> + return error;
> +
> + /*
> + * Downstream axcfg.py waits for 1sec without checking U01 HELLO. Tests
> + * showed that waiting for the HELLO message isn't enough therefore we
> + * need to add the additional fsleep(1sec).
> + * Touchnetix said that the boot can take up to 2sec if all self tests
> + * are enabled, so wait 2sec for the HELLO message.
> + */
> + if (!axiom_wait_for_completion_timeout(ts, &ts->boot_complete,
> + msecs_to_jiffies(2 * MSEC_PER_SEC))) {
[Severity: High]
Is reinit_completion needed here before waiting?
Since boot_complete and nvm_write are signaled via complete, future calls
to axiom_wait_for_completion_timeout will return immediately because
reinit_completion is never called to reset their internal state. This might
cause premature hardware communication.
> + dev_err(ts->dev, "Error swreset timedout\n");
> + error = -ETIMEDOUT;
> + }
> +
> + fsleep(USEC_PER_SEC);
> +
> + return error;
> +}
[ ... ]
> +static bool axiom_u42_get_touchslots(struct axiom_data *ts)
> +{
> + unsigned int bufsize;
> + unsigned int reg;
> + int error, i;
> +
> + bufsize = axiom_usage_size(ts, AXIOM_U42);
> + u8 *buf __free(kfree) = kzalloc(bufsize, GFP_KERNEL);
> + if (!buf)
> + return false;
> +
> + reg = axiom_usage_baseaddr(ts, AXIOM_U42);
> + error = regmap_raw_read(ts->regmap, reg, buf, bufsize);
> + if (error) {
> + dev_warn(ts->dev, "Failed to read u42\n");
> + return false;
> + }
> +
> + ts->enabled_slots = 0;
> + ts->num_slots = 0;
> +
> + for (i = 0; i < AXIOM_MAX_TOUCHSLOTS; i++) {
> + if (axiom_u42_touch_enabled(ts, buf, i)) {
[Severity: High]
Could this lead to an out-of-bounds heap read?
The bufsize is determined by the device via axiom_usage_size. If a
compromised or malfunctioning device reports a size smaller than 18 bytes,
the loop still iterates up to 10 slots checking fixed offsets up to index
17 in axiom_u42_touch_enabled, reading past the allocated buffer.
> + ts->enabled_slots |= BIT(i);
> + ts->num_slots++;
> + }
> + }
> +
> + return true;
> +}
[ ... ]
> +static int axiom_u34_rev1_process_report(struct axiom_data *ts,
> + const u8 *_buf, size_t bufsize)
> +{
> + unsigned int reg = axiom_usage_baseaddr(ts, AXIOM_U34);
> + struct regmap *regmap = ts->regmap;
> + u8 buf[AXIOM_PAGE_BYTE_LEN] = { };
[Severity: High]
Can this cause memory corruption on architectures lacking hardware cache
coherence?
The 256-byte buf array is allocated on the stack and passed to
regmap_raw_read, which eventually reaches the I2C core. Stack memory is
generally not DMA-safe, violating I2C transfer requirements.
> + struct device *dev = ts->dev;
> + unsigned char report_usage;
> + u16 crc_report, crc_calc;
> + unsigned int len;
> + u8 *payload;
> + int error;
> +
> + error = regmap_raw_read(regmap, reg, buf, ts->max_report_byte_len);
> + if (error)
> + return error;
[ ... ]
> + report_usage = buf[1];
> + payload = &buf[AXIOM_U34_REV1_PREAMBLE_BYTES];
> + len -= AXIOM_U34_REV1_PREAMBLE_BYTES - AXIOM_U34_REV1_POSTAMBLE_BYTES;
[Severity: Medium]
Could this calculation artificially inflate the payload length?
Mathematically, subtracting without parentheses evaluates like this:
len -= (2 - 4), which results in len += 2.
Should this have parentheses around the values being subtracted?
> +
> + switch (report_usage) {
> + case AXIOM_U01:
> + case AXIOM_U41:
[ ... ]
> +static int axiom_u41_rev2_process_report(struct axiom_data *ts,
> + const u8 *buf, size_t bufsize)
> +{
> + struct input_dev *input = ts->input;
> + unsigned char id;
> + u16 targets;
> +
> + /*
> + * The input registration can be postponed but the touchscreen FW is
> + * sending u41 reports regardless.
> + */
> + if (!input)
> + return 0;
[Severity: High]
Could this lead to a use-after-free if the input device is unregistered?
During config firmware updates (axiom_cfg_fw_write) and module removal,
the input device is freed. If the threaded IRQ handler runs concurrently
and reads ts->input before it is nulled out, it might access a freed pointer
when calling input_mt_slot and touchscreen_report_pos.
> +
> + targets = get_unaligned_le16(&buf[AXIOM_U41_REV2_TARGETSTATUS_REG]);
[ ... ]
> +static int axiom_regmap_read(void *context, const void *reg_buf, size_t reg_size,
> + void *val_buf, size_t val_size)
> +{
> + struct device *dev = context;
> + struct i2c_client *i2c = to_i2c_client(dev);
> + struct axiom_data *ts = i2c_get_clientdata(i2c);
> + struct axiom_cmd_header hdr;
> + struct i2c_msg xfer[2];
> + u16 xferlen, addr;
> + int ret;
> +
> + if (val_size > AXIOM_MAX_XFERLEN) {
> + dev_err(ts->dev, "Exceed max xferlen: %zu > %u\n",
> + val_size, AXIOM_MAX_XFERLEN);
> + return -EINVAL;
> + }
> +
> + addr = *((u16 *)reg_buf);
> + hdr.target_address = cpu_to_le16(addr);
[Severity: High]
Does this cast cause endianness issues on big-endian architectures?
Casting a byte buffer directly to a u16 pointer evaluates differently
depending on the host byte order. When cpu_to_le16 is applied afterwards,
the register address sent to the device will be corrupted on big-endian
systems. The same pattern is present in axiom_regmap_write.
> + xferlen = FIELD_PREP(AXIOM_CMD_HDR_DIR_MASK, AXIOM_CMD_HDR_READ) |
> + FIELD_PREP(AXIOM_CMD_HDR_LEN_MASK, val_size);
> + hdr.xferlen = cpu_to_le16(xferlen);
> +
> + if (!axiom_usage_supported(ts, addr))
> + return -EINVAL;
> +
> + xfer[0].addr = i2c->addr;
> + xfer[0].flags = 0;
> + xfer[0].len = sizeof(hdr);
> + xfer[0].buf = (u8 *)&hdr;
[Severity: High]
Could passing the stack-allocated hdr struct to i2c_transfer cause issues
with DMA mapping?
The I2C core expects buffers that are DMA-safe, but stack variables might
violate this on non-coherent architectures. A similar stack-buffer is passed
in axiom_regmap_write.
> +
> + xfer[1].addr = i2c->addr;
> + xfer[1].flags = I2C_M_RD;
> + xfer[1].len = val_size;
> + xfer[1].buf = val_buf;
[ ... ]
> +static enum fw_upload_err axiom_axfw_fw_prepare(struct fw_upload *fw_upload,
> + const u8 *data, u32 size)
> +{
> + struct axiom_data *ts = fw_upload->dd_handle;
> + struct axiom_firmware *afw = &ts->fw[AXIOM_FW_AXFW];
> + enum fw_upload_err ret;
> +
> + scoped_guard(mutex, &afw->lock) {
> + afw->cancel = false;
> + }
> +
> + mutex_lock(&ts->fwupdate_lock);
> +
> + ret = __axiom_axfw_fw_prepare(ts, afw, data, size);
> +
> + /*
> + * In FW_UPLOAD_ERR_NONE case the complete handler will release the
> + * lock.
> + */
> + if (ret != FW_UPLOAD_ERR_NONE)
> + mutex_unlock(&ts->fwupdate_lock);
[Severity: High]
Can this lead to a double-unlock of fwupdate_lock?
If __axiom_axfw_fw_prepare returns an error, this code unlocks the mutex.
However, the firmware upload core will subsequently call the cleanup
callback axiom_axfw_fw_cleanup, which unconditionally unlocks the mutex
again. The same issue exists in axiom_cfg_fw_prepare.
> +
> + return ret;
> +}
[ ... ]
> +static enum fw_upload_err axiom_axfw_fw_write(struct fw_upload *fw_upload,
> + const u8 *data, u32 offset,
> + u32 size, u32 *written)
> +{
> + struct axiom_data *ts = fw_upload->dd_handle;
> + struct axiom_firmware *afw = &ts->fw[AXIOM_FW_AXFW];
> + struct device *dev = ts->dev;
> + int error;
> +
> + /*
> + * According Touchnetix the IRQ pin gets reconfigured in bootloader
> + * mode. Which can cause spurious IRQs. Therefore the IRQ should be
> + * disabled on the host.
> + *
> + * See the cleanup routine for the balanced enable.
> + */
> + if (axiom_get_runmode(ts) != AXIOM_BLP_MODE)
> + axiom_disable_irq(ts);
> +
> + /* Done before cancel check due to cleanup based put */
> + error = pm_runtime_resume_and_get(ts->dev);
> + if (error)
> + return FW_UPLOAD_ERR_HW_ERROR;
[Severity: High]
Does this error path lead to an unbalanced PM runtime usage count?
If pm_runtime_resume_and_get fails, it returns an error and no reference
is acquired. The firmware upload core will then call axiom_axfw_fw_cleanup,
which unconditionally calls pm_runtime_put_sync_autosuspend, potentially
causing a refcount underflow.
> +
> + scoped_guard(mutex, &afw->lock) {
> + if (afw->cancel)
> + return FW_UPLOAD_ERR_CANCELED;
> + }
[ ... ]
> + while (size) {
> + u16 chunk_len, len;
> +
> + chunk_len = get_unaligned_be16(&data[6]);
> + len = chunk_len + sizeof(struct axiom_fw_axfw_chunk_hdr);
> +
> + /*
> + * The bootlaoder FW can handle the complete chunk incl. the
> + * header.
> + */
> + error = axiom_blp_write_chunk(ts, data, len);
> + if (error) {
> + /*
> + * Tests showed that the bootloader mode must be exited
> + * if an invalid chunk was received by the bootloader fw
> + * since all following attempts to download valid chunks
> + * will fail. Try do so via axiom_blp_reset() but tests
> + * also showed that this may fail too. So inform the
> + * user and hope that the full power-cycle helps. To get
> + * the device back into a working mode where the device
> + * accepts data again.
> + */
> + if (axiom_blp_reset(ts))
> + dev_warn(dev, "Couldn't recover device, device requires power-cycle\n");
> + return axiom_unlock_input_return_hw_error(ts);
> + }
> +
> + size -= len;
> + *written += len;
> + data += len;
[Severity: High]
Is there a bounds checking issue here that could lead to integer underflow?
The length is calculated and subtracted from size without checking if len
is actually less than or equal to size. If a malformed firmware chunk
reports a large chunk_len, size will wrap around, causing out-of-bounds
accesses on the next iteration. This also occurs in axiom_cfg_fw_write
and __axiom_cfg_fw_prepare.
> + }
[ ... ]
> +static int axiom_register_fwl(struct axiom_data *ts)
> +{
> + struct device *dev = ts->dev;
> + struct fw_upload *fwl;
> + char *fw_name;
> + int error;
> +
> + if (!IS_ENABLED(CONFIG_FW_UPLOAD)) {
> + dev_dbg(dev, "axfw and th2cfgbin update disabled\n");
> + return 0;
> + }
> +
> + mutex_init(&ts->fw[AXIOM_FW_AXFW].lock);
> + fw_name = kasprintf(GFP_KERNEL, "i2c:%s.axfw", dev_name(dev));
> + fwl = firmware_upload_register(THIS_MODULE, ts->dev, fw_name,
> + &axiom_axfw_fw_upload_ops, ts);
[Severity: Medium]
Should there be a NULL pointer check for fw_name?
If kasprintf fails under memory pressure, it returns NULL, which is then
passed directly to firmware_upload_register.
> + kfree(fw_name);
> + if (IS_ERR(fwl))
> + return dev_err_probe(dev, PTR_ERR(fwl),
> + "Failed to register firmware upload\n");
> +
> + error = devm_add_action_or_reset(dev, axiom_remove_axfw_fwl_action, ts);
> + if (error)
> + return error;
> +
> + ts->fw[AXIOM_FW_AXFW].fwl = fwl;
[Severity: High]
Could this cause a NULL pointer dereference if devm_add_action_or_reset fails?
If it fails, devm_add_action_or_reset executes axiom_remove_axfw_fwl_action
immediately. However, ts->fw[AXIOM_FW_AXFW].fwl is currently NULL because
it hasn't been assigned yet, causing firmware_upload_unregister to crash.
> +
> + mutex_init(&ts->fw[AXIOM_FW_CFG].lock);
[ ... ]
> +static int axiom_register_input_dev(struct axiom_data *ts,
> + bool update_in_process)
> +{
> + struct device *dev = ts->dev;
> + struct i2c_client *client = to_i2c_client(dev);
> + struct input_dev *input;
> + int error;
[ ... ]
> + axiom_get_touchslots(ts);
> + if (!ts->num_slots && update_in_process) {
> + input_free_device(input);
> + /*
> + * Skip input device registration but don't throw an error to
> + * not abort the update since some FW updates require a
> + * following CFG update to re-initialize the touchslot handling.
> + */
> + if (update_in_process) {
> + dev_info(dev, "No touchslots found after FW or CFG update, skip registering input device\n");
> + return 0;
> + }
> +
> + dev_err(dev, "Error firmware has no touchslots enabled\n");
> + return -EINVAL;
> + }
[Severity: Medium]
Is the -EINVAL error path dead code?
The outer if-statement requires update_in_process to be true. The inner
if-statement also checks if update_in_process is true and returns 0.
The code will never reach the dev_err and -EINVAL return.
> +
> + error = input_mt_init_slots(input, ts->num_slots, INPUT_MT_DIRECT);
[ ... ]
> +static int axiom_i2c_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct axiom_data *ts;
> + int error;
[ ... ]
> + pm_runtime_set_autosuspend_delay(dev, 10 * MSEC_PER_SEC);
> + pm_runtime_use_autosuspend(dev);
> + pm_runtime_set_active(dev);
> + pm_runtime_get_noresume(dev);
> + error = devm_pm_runtime_enable(dev);
> + if (error)
> + return dev_err_probe(dev, error, "Failed to enable pm-runtime\n");
> +
> + error = axiom_register_panel_follower(ts);
> + if (error)
> + return dev_err_probe(dev, error, "Failed to register panel follower\n");
[Severity: Medium]
Do these error returns leak the PM runtime reference and leave the power
supplies enabled?
If devm_pm_runtime_enable or axiom_register_panel_follower fail, the
function returns directly without dropping the reference acquired by
pm_runtime_get_noresume or disabling the regulators.
> +
> + error = axiom_u31_device_discover(ts);
> + /*
> + * Register the device to allow FW updates in case that the current FW
> + * doesn't support the required driver usages or if the device is in
> + * bootloader mode.
> + *
> + * The U02 usage has to be supported, since this is the usage which is
> + * used to put the device into bootloader mode!
> + *
> + * TODO: Add support for the emergency bootloader mode enter sequence.
> + * The emergency enter is done by toggling the nRESET pin 5-times in a
> + * row.
> + */
> + if (error) {
> + if (IS_ENABLED(CONFIG_FW_UPLOAD) &&
> + axiom_driver_supports_usage(ts, AXIOM_U02) &&
> + (axiom_get_runmode(ts) == AXIOM_DISCOVERY_MODE ||
> + axiom_get_runmode(ts) == AXIOM_BLP_MODE)) {
> + dev_warn(dev, "Device discovery failed, wait for user fw update\n");
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_sync_autosuspend(dev);
> + return 0;
> + }
> + pm_runtime_put_sync(dev);
> + return dev_err_probe(dev, error, "Device discovery failed\n");
> + }
[Severity: High]
Does this logic prevent firmware recovery when the device boots in bootloader
mode?
When starting in BLP mode, axiom_u31_device_discover returns -EACCES
without populating the usage table. The recovery check here evaluates
axiom_driver_supports_usage(ts, AXIOM_U02), which returns false because the
usage table is empty. This aborts the probe instead of enabling the upload
interface.
> +
> + error = axiom_register_input_dev(ts, false);
[ ... ]
> +static void axiom_i2c_remove(struct i2c_client *client)
> +{
> + struct axiom_data *ts = i2c_get_clientdata(client);
> +
> + axiom_unregister_input_dev(ts);
> +}
[Severity: High]
Could this lead to a use-after-free during unbind?
The firmware upload cleanup actions are registered via devres, meaning they
execute after axiom_i2c_remove. However, axiom_i2c_remove explicitly frees
the input device. If a background firmware upload thread is active, it will
access the freed input device.
> +
> +static int axiom_runtime_suspend(struct device *dev)
> +{
> + struct axiom_data *ts = dev_get_drvdata(dev);
> +
> + axiom_disable_irq(ts);
> + axiom_power_down_device(ts);
> +
> + return 0;
> +}
> +
> +static int axiom_runtime_resume(struct device *dev)
> +{
> + struct axiom_data *ts = dev_get_drvdata(dev);
> + int error;
> +
> + error = axiom_power_up_device(ts);
> + if (error)
> + return error;
> +
> + axiom_enable_irq(ts);
> +
> + return 0;
> +}
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(axiom_pm_ops, axiom_runtime_suspend,
> + axiom_runtime_resume, NULL);
[Severity: Medium]
Should this include system sleep power management callbacks?
DEFINE_RUNTIME_DEV_PM_OPS is used, but without SYSTEM_SLEEP_PM_OPS. If the
touchscreen is held open by userspace during a system suspend (S3), the
device will remain in RPM_ACTIVE and stay powered on.
> +
> +static const struct i2c_device_id axiom_i2c_id_table[] = {
> + { "ax54a" },
> + { },
> +};
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-v6-10-topic-touchscreen-axiom-v7-0-0d2a550a7ee8@pengutronix.de?part=4
^ permalink raw reply
* Re: [PATCH v7 1/4] firmware_loader: expand firmware error codes with up-to-date error
From: sashiko-bot @ 2026-07-02 22:45 UTC (permalink / raw)
To: Marco Felsch; +Cc: dmitry.torokhov, devicetree, linux-input, conor+dt, robh
In-Reply-To: <20260703-v6-10-topic-touchscreen-axiom-v7-1-0d2a550a7ee8@pengutronix.de>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The selftest script for firmware uploads does not inject and test the new 'firmware-duplicate' error.
- [Low] Missing ABI documentation for the newly added sysfs error string 'firmware-duplicate'.
--
commit b451d666b20500e69051bac63ead44572b79d708
Author: Marco Felsch <m.felsch@pengutronix.de>
firmware_loader: expand firmware error codes with up-to-date error
This commit adds FW_UPLOAD_ERR_DUPLICATE to allow drivers to inform the
firmware_loader that an update is not required. It also updates the
test_firmware module to support injecting this new error state.
> diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c
> index efc33294212fd..82c4b1fedf4cd 100644
> --- a/drivers/base/firmware_loader/sysfs_upload.c
> +++ b/drivers/base/firmware_loader/sysfs_upload.c
> @@ -28,6 +28,7 @@ static const char * const fw_upload_err_str[] = {
> [FW_UPLOAD_ERR_RW_ERROR] = "read-write-error",
> [FW_UPLOAD_ERR_WEAROUT] = "flash-wearout",
> [FW_UPLOAD_ERR_FW_INVALID] = "firmware-invalid",
> + [FW_UPLOAD_ERR_DUPLICATE] = "firmware-duplicate",
[Severity: Low]
Since a new error string "firmware-duplicate" is exposed to userspace,
should this also be documented in
Documentation/ABI/testing/sysfs-class-firmware?
It looks like the new string is missing from the allowed error values
documented for the sysfs interface.
> };
>
> static const char *fw_upload_progress(struct device *dev,
[ ... ]
> diff --git a/lib/test_firmware.c b/lib/test_firmware.c
> index 7459bba65444d..a822d9641eb4c 100644
> --- a/lib/test_firmware.c
> +++ b/lib/test_firmware.c
> @@ -1134,6 +1134,7 @@ static const char * const fw_upload_err_str[] = {
> [FW_UPLOAD_ERR_RW_ERROR] = "read-write-error",
> [FW_UPLOAD_ERR_WEAROUT] = "flash-wearout",
> [FW_UPLOAD_ERR_FW_INVALID] = "firmware-invalid",
> + [FW_UPLOAD_ERR_DUPLICATE] = "firmware-duplicate",
[Severity: Medium]
Does the test script also need to be updated to exercise this new code path?
While this properly adds injection support to the test module, the hardcoded
errors list in tools/testing/selftests/firmware/fw_upload.sh does not appear
to include "firmware-duplicate", meaning the kselftest won't actually
inject or test this new error type.
> };
>
> static void upload_err_inject_error(struct test_firmware_upload *tst,
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-v6-10-topic-touchscreen-axiom-v7-0-0d2a550a7ee8@pengutronix.de?part=1
^ permalink raw reply
* Re: [PATCH v6 4/4] Input: Add TouchNetix aXiom I2C Touchscreen support
From: sashiko-bot @ 2026-07-02 22:43 UTC (permalink / raw)
To: Marco Felsch; +Cc: linux-input, conor+dt, dmitry.torokhov, robh, devicetree
In-Reply-To: <20260703-v6-10-topic-touchscreen-axiom-v6-4-1aa50ba3bc5a@pengutronix.de>
Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [High] Custom regmap callbacks interpret 16-bit register addresses with incorrect endianness, leading to I2C transfer failures on little-endian hosts and corrupted addresses on big-endian hosts.
- [High] The firmware upload cleanup handler double-unlocks `fwupdate_lock` and permanently underflows the PM runtime usage counter.
- [High] Missing bounds checks when parsing firmware chunks lead to out-of-bounds memory reads and integer underflows.
- [High] Insufficient startup delay due to using a millisecond macro with `fsleep()`, causing a 1000x shorter sleep than intended.
- [High] Manual unregistration of the input device in `remove()` races with devres-managed IRQs and sysfs interfaces, leading to Use-After-Free.
- [High] The TH2CFG firmware update path replaces the input device dynamically without disabling the IRQ, causing a Use-After-Free race with active interrupt handlers.
- [High] Missing `reinit_completion()` allows late interrupts to cause spurious wakeups in subsequent operations.
- [Medium] Probe error paths leak the PM runtime usage counter and leave hardware regulators permanently enabled.
- [Medium] The allocated input device is leaked if requesting the threaded IRQ fails.
- [Medium] Arithmetic error calculating the payload length due to subtraction of preamble and postamble bytes without parentheses.
- [Medium] Unreachable error handling when the device reports 0 touch slots during initial probe leads to a logic flaw and dead code.
- [Medium] Dead code pointer check `if (!entry)` is used instead of checking if the usage entry is populated.
- [Medium] Insufficient bounds checking on device-supplied usage size leads to a heap out-of-bounds read in `axiom_u42_get_touchslots`.
--
A review of this patch has revealed the following issues:
commit ee908a9756d78a49fffc38a01178c9ab48ed3f33
Author: Marco Felsch <m.felsch@pengutronix.de>
Input: Add TouchNetix aXiom I2C Touchscreen support
This adds the initial support for the TouchNetix AX54A touchcontroller
which is part of TouchNetix's aXiom touchscreen controller family.
> diff --git a/drivers/input/touchscreen/touchnetix_axiom.c b/drivers/input/touchscreen/touchnetix_axiom.c
> --- a/drivers/input/touchscreen/touchnetix_axiom.c
> +++ b/drivers/input/touchscreen/touchnetix_axiom.c
[ ... ]
> @@ -854,16 +854,16 @@ static int axiom_u02_save_config(struct axiom_data *ts)
> msg.parameters[2] = cpu_to_le16(AXIOM_U02_REV1_PARAM2_SAVEVLTLCFG2NVM);
>
> error = axiom_u02_send_msg(ts, &msg, false);
> if (error)
> return error;
>
> /* Downstream axcfg.py waits for 2sec without checking U01 response */
> if (!axiom_wait_for_completion_timeout(ts, &ts->nvm_write,
> msecs_to_jiffies(2 * MSEC_PER_SEC))) {
[Severity: High]
Does this code risk consuming a stale completion signal? The completion
variable ts->nvm_write doesn't appear to be reinitialized. If a previous
operation timed out but completed late, could wait_for_completion_timeout()
return immediately here without actually waiting? We might need a call to
reinit_completion() before starting the wait.
> @@ -876,16 +876,16 @@ static int axiom_u02_swreset(struct axiom_data *ts)
> error = axiom_u02_send_msg(ts, &msg, false);
> if (error)
> return error;
>
> /*
> * Downstream axcfg.py waits for 1sec without checking U01 HELLO. Tests
> * showed that waiting for the HELLO message isn't enough therefore we
> * need to add the additional fsleep(1sec).
> * Touchnetix said that the boot can take up to 2sec if all self tests
> * are enabled, so wait 2sec for the HELLO message.
> */
> if (!axiom_wait_for_completion_timeout(ts, &ts->boot_complete,
> msecs_to_jiffies(2 * MSEC_PER_SEC))) {
[Severity: High]
Could we hit the same stale completion issue here for ts->boot_complete? A
late interrupt from a previous operation might cause this to return early
if reinit_completion() is missing.
[ ... ]
> @@ -998,16 +998,16 @@ static int axiom_regmap_read(void *context, const void *reg_buf, size_t reg_size
>
> if (val_size > AXIOM_MAX_XFERLEN) {
> dev_err(ts->dev, "Exceed max xferlen: %zu > %u\n",
> val_size, AXIOM_MAX_XFERLEN);
> return -EINVAL;
> }
>
> addr = *((u16 *)reg_buf);
> hdr.target_address = cpu_to_le16(addr);
[Severity: High]
Will this address casting work correctly on big-endian hosts? Since I2C
regmap sends the buffer in big-endian format by default, casting the byte
buffer directly to u16 will swap the bytes on little-endian systems. Then,
when calling cpu_to_le16(addr), a big-endian system will swap the big-endian
bytes to little-endian, completely breaking the hardware communication.
Does this code need to read the address using get_unaligned_be16() instead?
> @@ -1035,16 +1035,16 @@ static int axiom_regmap_write(void *context, const void *data, size_t count)
> val_size = count - sizeof(addr);
> if (val_size > AXIOM_MAX_XFERLEN) {
> dev_err(ts->dev, "Exceed max xferlen: %zu > %u\n",
> val_size, AXIOM_MAX_XFERLEN);
> return -EINVAL;
> }
>
> addr = *((u16 *)data);
> hdr.target_address = cpu_to_le16(addr);
[Severity: High]
Does this have the same endianness issue as axiom_regmap_read()?
[ ... ]
> @@ -1078,16 +1078,16 @@ static bool axiom_u42_get_touchslots(struct axiom_data *ts)
>
> reg = axiom_usage_baseaddr(ts, AXIOM_U42);
> error = regmap_raw_read(ts->regmap, reg, buf, bufsize);
> if (error) {
> dev_warn(ts->dev, "Failed to read u42\n");
> return false;
> }
>
> ts->enabled_slots = 0;
> ts->num_slots = 0;
>
> for (i = 0; i < AXIOM_MAX_TOUCHSLOTS; i++) {
> if (axiom_u42_touch_enabled(ts, buf, i)) {
[Severity: Medium]
Could this loop read out of bounds? The buf buffer is sized based on bufsize
which is supplied by the device. If the device advertises a U42 usage size
smaller than the maximum required index (e.g., 4 bytes), iterating 10 times
and indexing into buf might result in a heap out-of-bounds read. Do we need
to verify that bufsize is large enough for all max touch slots?
[ ... ]
> @@ -1117,16 +1117,16 @@ static int axiom_process_report(struct axiom_data *ts, unsigned char usage_num,
> const u8 *buf, size_t buflen)
> {
> struct axiom_usage_table_entry *entry = &ts->usage_table[usage_num];
>
> /* Skip processing if not in TCP mode */
> if ((axiom_get_runmode(ts) != AXIOM_TCP_MODE) &&
> (axiom_get_runmode(ts) != AXIOM_TCP_CFG_UPDATE_MODE))
> return 0;
>
> /* May happen if an unsupported usage was requested */
> if (!entry) {
> dev_info(ts->dev, "Unsupported usage U%x request\n", usage_num);
> return 0;
> }
[Severity: Medium]
Is this pointer check effective? Since entry is initialized using the address
of an element in the array (&ts->usage_table[usage_num]), it will never
be NULL. Should this check !entry->populated instead?
[ ... ]
> @@ -1158,16 +1158,16 @@ static int axiom_u34_rev1_process_report(struct axiom_data *ts,
> * The CRC16 value can be queried at the last two bytes of the report.
> * The value itself is covering the complete report excluding the CRC16
> * value at the end.
> */
> crc_report = get_unaligned_le16(&buf[len - 2]);
> crc_calc = crc16(0, buf, (len - 2));
>
> if (crc_calc != crc_report) {
> dev_err_ratelimited(dev, "CRC16 mismatch!\n");
> return -EINVAL;
> }
>
> report_usage = buf[1];
> payload = &buf[AXIOM_U34_REV1_PREAMBLE_BYTES];
> len -= AXIOM_U34_REV1_PREAMBLE_BYTES - AXIOM_U34_REV1_POSTAMBLE_BYTES;
[Severity: Medium]
Does this calculation result in an incorrect length? Due to C operator
precedence, len -= A - B calculates len -= (A - B). This evaluates to
len -= (2 - 4), effectively adding 2 to the length instead of subtracting 6.
Would len -= (AXIOM_U34_REV1_PREAMBLE_BYTES + AXIOM_U34_REV1_POSTAMBLE_BYTES)
be the intended math here?
[ ... ]
> @@ -1838,16 +1838,16 @@ static enum fw_upload_err axiom_axfw_fw_prepare(struct fw_upload *fw_upload,
> const u8 *data, u32 size)
> {
> struct axiom_data *ts = fw_upload->dd_handle;
> struct axiom_firmware *afw = &ts->fw[AXIOM_FW_AXFW];
> enum fw_upload_err ret;
>
> scoped_guard(mutex, &afw->lock) {
> afw->cancel = false;
> }
>
> mutex_lock(&ts->fwupdate_lock);
>
> ret = __axiom_axfw_fw_prepare(ts, afw, data, size);
>
> /*
> * In FW_UPLOAD_ERR_NONE case the complete handler will release the
> * lock.
> */
> if (ret != FW_UPLOAD_ERR_NONE)
> mutex_unlock(&ts->fwupdate_lock);
[Severity: High]
Does this code risk a double-unlock of ts->fwupdate_lock? The firmware
upload core unconditionally calls the cleanup callback when prepare fails.
Since axiom_axfw_fw_cleanup() also unconditionally unlocks the mutex,
unlocking it here on the error path will cause a double-unlock panic.
[ ... ]
> @@ -1934,16 +1934,16 @@ static enum fw_upload_err axiom_axfw_fw_write(struct fw_upload *fw_upload,
> error = axiom_enter_bootloader_mode(ts);
> if (error)
> return axiom_unlock_input_return_hw_error(ts);
> }
>
> while (size) {
> u16 chunk_len, len;
>
> chunk_len = get_unaligned_be16(&data[6]);
> len = chunk_len + sizeof(struct axiom_fw_axfw_chunk_hdr);
>
> /*
> * The bootlaoder FW can handle the complete chunk incl. the
> * header.
> */
> error = axiom_blp_write_chunk(ts, data, len);
> if (error) {
> /*
> * Tests showed that the bootloader mode must be exited
> * if an invalid chunk was received by the bootloader fw
> * since all following attempts to download valid chunks
> * will fail. Try do so via axiom_blp_reset() but tests
> * also showed that this may fail too. So inform the
> * user and hope that the full power-cycle helps. To get
> * the device back into a working mode where the device
> * accepts data again.
> */
> if (axiom_blp_reset(ts))
> dev_warn(dev, "Couldn't recover device, device requires power-cycle\n");
> return axiom_unlock_input_return_hw_error(ts);
> }
>
> size -= len;
[Severity: High]
Could this loop read out of bounds on the kernel heap? The chunk_len is
extracted directly from the untrusted firmware data, but there is no check
to ensure that size is large enough to contain the full chunk. If the
firmware is malformed such that len > size, size will underflow to a huge
value causing an infinite out of bounds read.
[ ... ]
> @@ -2022,16 +2022,16 @@ static void axiom_axfw_fw_handle_irq(struct axiom_data *ts)
> * because the probe() aborted due to firmware incompatibilities. Skip
> * the enable step if the IRQ is not disabled to not mess with the
> * refcount.
> */
> if (irqd_irq_disabled(data) &&
> axiom_get_runmode(ts) != AXIOM_BLP_MODE)
> axiom_enable_irq(ts);
> }
>
> static void axiom_axfw_fw_cleanup(struct fw_upload *fw_upload)
> {
> struct axiom_data *ts = fw_upload->dd_handle;
>
> mutex_unlock(&ts->fwupdate_lock);
> axiom_axfw_fw_handle_irq(ts);
> pm_runtime_mark_last_busy(ts->dev);
> pm_runtime_put_sync_autosuspend(ts->dev);
> }
[Severity: High]
Does this pm_runtime_put_sync_autosuspend() call risk an underflow? If the
firmware upload fails during the prepare callback, the core will call this
cleanup function. Since the pm_runtime_resume_and_get() is only called later
inside the write callback, dropping the reference unconditionally here will
permanently underflow the PM runtime usage counter.
[ ... ]
> @@ -2171,16 +2171,16 @@ static enum fw_upload_err __axiom_cfg_fw_prepare(struct axiom_data *ts,
> /* Skip to the first fw chunk */
> data += sizeof(struct axiom_fw_cfg_hdr);
> size -= sizeof(struct axiom_fw_cfg_hdr);
>
> /*
> * Search for u33 which contains the CRC information and perform only
> * the runtime-crc check.
> */
> while (size) {
> unsigned int chunk_len;
>
> chunk_len = axiom_cfg_fw_prepare_chunk(&chunk, data);
> if (chunk.usage_num == AXIOM_U33)
> break;
>
> data += chunk_len;
> size -= chunk_len;
[Severity: High]
Is it possible for this loop to underflow the size variable if the chunk
is malformed? There doesn't appear to be a check verifying that size is
greater than or equal to chunk_len before subtracting it.
[ ... ]
> @@ -2435,16 +2435,16 @@ static enum fw_upload_err axiom_cfg_fw_write(struct fw_upload *fw_upload,
>
> /*
> * (Re)start the device with the new config. Start the device AE
> * either via u02 SW_RESET or u02 CMD_START, the behavior is the same
> * according Touchnetix.
> */
> error = axiom_u02_swreset(ts);
> if (error) {
> dev_err(dev, "Soft reset failed\n");
> goto err_unlock;
> }
>
> error = axiom_u33_read(ts, &ts->crc[AXIOM_CRC_CUR]);
> if (error)
> goto err_unlock;
>
> if (axiom_verify_crcs(ts))
> goto err_unlock;
>
> /* Unlock before the input device gets unregistered */
> axiom_unlock_input_device(ts);
>
> error = axiom_update_input_dev(ts);
[Severity: High]
Could this dynamic input device replacement lead to a Use-After-Free? The
driver softly resets the device and calls axiom_update_input_dev(ts) which
frees the old ts->input and allocates a new one. Unlike the AXFW update,
axiom_disable_irq(ts) is never called here. A soft-reset triggers an immediate
interrupt, which would cause axiom_u41_rev2_process_report() to execute and
access ts->input without locks while it is being freed.
[ ... ]
> @@ -2776,16 +2776,16 @@ static int axiom_register_input_dev(struct axiom_data *ts,
> input_set_abs_params(input, ABS_MT_POSITION_X, 0, AXIOM_MAX_XY, 0, 0);
> input_set_abs_params(input, ABS_MT_POSITION_Y, 0, AXIOM_MAX_XY, 0, 0);
> input_set_abs_params(input, ABS_MT_DISTANCE, 0, 127, 0, 0);
> if (ts->cds_enabled)
> input_set_abs_params(input, ABS_MT_PRESSURE, 0, 127, 0, 0);
>
> touchscreen_parse_properties(input, true, &ts->prop);
>
> axiom_get_touchslots(ts);
> if (!ts->num_slots && update_in_process) {
> input_free_device(input);
> /*
> * Skip input device registration but don't throw an error to
> * not abort the update since some FW updates require a
> * following CFG update to re-initialize the touchslot handling.
> */
> if (update_in_process) {
[Severity: Medium]
Will this error path ever execute during the initial probe? When called from
probe, update_in_process is false, making the condition evaluate to false
even if ts->num_slots is 0. This skips the error block, calls
input_mt_init_slots (which returns 0 without initializing dev->mt), and
registers a potentially useless input device instead of returning an error.
[ ... ]
> @@ -2831,16 +2831,16 @@ static int axiom_register_input_dev(struct axiom_data *ts,
> * to the i2c-dev whereas the input-poller belong to the input-dev. The
> * input-dev can get unregistered during a firmware update to reflect
> * the new firmware state. Therefore the input-poller setup must be done
> * always.
> */
> if (client->irq) {
> if (!ts->irq_setup_done) {
> error = devm_request_threaded_irq(dev, client->irq,
> NULL, axiom_irq,
> IRQF_ONESHOT,
> dev_name(dev), ts);
> if (error) {
> dev_err(dev, "Failed to request IRQ\n");
> return error;
> }
[Severity: Medium]
Does this error path leak the input device? It returns immediately when
devm_request_threaded_irq() fails, but it appears to miss calling
input_free_device(input) which is done in the other error paths in this
function.
[ ... ]
> @@ -2866,16 +2866,16 @@ static int axiom_power_up_device(struct axiom_data *ts)
> int error;
>
> error = regulator_bulk_enable(ts->num_supplies, ts->supplies);
> if (error) {
> dev_err(dev, "Failed to enable power supplies\n");
> return error;
> }
>
> gpiod_set_value_cansleep(ts->reset_gpio, 1);
> fsleep(2000);
> gpiod_set_value_cansleep(ts->reset_gpio, 0);
>
> fsleep(AXIOM_STARTUP_TIME_MS);
>
> return 0;
> }
[Severity: High]
Does this use the intended sleep duration? fsleep() takes its argument in
microseconds, but it's passed AXIOM_STARTUP_TIME_MS (which is 110). This will
cause the driver to wait for 110 microseconds instead of 110 milliseconds,
which could prevent the touch controller from finishing its boot sequence
before communication is attempted.
[ ... ]
> @@ -2982,16 +2982,16 @@ static int axiom_i2c_probe(struct i2c_client *client)
> if (error)
> return error;
>
> error = axiom_power_up_device(ts);
> if (error)
> return dev_err_probe(dev, error, "Failed to power-on device\n");
>
> pm_runtime_set_autosuspend_delay(dev, 10 * MSEC_PER_SEC);
> pm_runtime_use_autosuspend(dev);
> pm_runtime_set_active(dev);
> pm_runtime_get_noresume(dev);
> error = devm_pm_runtime_enable(dev);
> if (error)
> return dev_err_probe(dev, error, "Failed to enable pm-runtime\n");
>
> error = axiom_register_panel_follower(ts);
> if (error)
> return dev_err_probe(dev, error, "Failed to register panel follower\n");
[Severity: Medium]
Do these probe error paths leak the PM runtime usage counter? Returning
directly here skips calling pm_runtime_put_sync() and leaves the hardware
regulators permanently enabled since axiom_power_down_device(ts) isn't called.
[ ... ]
> @@ -3099,14 +3099,14 @@ static int axiom_i2c_probe(struct i2c_client *client)
> if (!IS_ENABLED(CONFIG_FW_UPLOAD))
> return dev_err_probe(dev, error, "Failed to register input device\n");
>
> dev_warn(dev, "Failed to register the input device, wait for user fw update\n");
> }
>
> return 0;
> }
>
> static void axiom_i2c_remove(struct i2c_client *client)
> {
> struct axiom_data *ts = i2c_get_clientdata(client);
>
> axiom_unregister_input_dev(ts);
> }
[Severity: High]
Could manually unregistering the input device here race with devres-managed
resources? The threaded IRQ handler and sysfs firmware upload callbacks are
managed by devm, so they won't be disabled until after remove() finishes. If
an interrupt fires or a sysfs upload starts during unbind, it might access the
already-freed input device.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-v6-10-topic-touchscreen-axiom-v6-0-1aa50ba3bc5a@pengutronix.de?part=4
^ permalink raw reply
* Re: [PATCH v6 1/4] firmware_loader: expand firmware error codes with up-to-date error
From: sashiko-bot @ 2026-07-02 22:39 UTC (permalink / raw)
To: Marco Felsch; +Cc: devicetree, robh, linux-input, conor+dt, dmitry.torokhov
In-Reply-To: <20260703-v6-10-topic-touchscreen-axiom-v6-1-1aa50ba3bc5a@pengutronix.de>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The newly added "firmware-duplicate" sysfs error string is omitted from the sysfs ABI documentation and the corresponding selftest script.
--
commit 59f39aaf73939b5dbe553a0c6fe3c5d3c9ce3d78
Author: Marco Felsch <m.felsch@pengutronix.de>
firmware_loader: expand firmware error codes with up-to-date error
This commit adds FW_UPLOAD_ERR_DUPLICATE to allow drivers to signal
that a firmware update is not required. It also updates the error
strings in the core sysfs interface and the test firmware module.
> diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c
> --- a/drivers/base/firmware_loader/sysfs_upload.c
> +++ b/drivers/base/firmware_loader/sysfs_upload.c
> @@ -28,6 +28,7 @@ static const char * const fw_upload_err_str[] = {
> [FW_UPLOAD_ERR_RW_ERROR] = "read-write-error",
> [FW_UPLOAD_ERR_WEAROUT] = "flash-wearout",
> [FW_UPLOAD_ERR_FW_INVALID] = "firmware-invalid",
> + [FW_UPLOAD_ERR_DUPLICATE] = "firmware-duplicate",
[Severity: Low]
Since firmware-duplicate is now exposed to userspace, should it also
be documented in Documentation/ABI/testing/sysfs-class-firmware? The
existing documentation lists the other expected error strings but is
missing this one.
Additionally, does this require an update to the selftests? Looking at
tools/testing/selftests/firmware/fw_upload.sh, the errors list is
hardcoded:
errors="hw-error
timeout
device-busy
invalid-file-size
read-write-error
flash-wearout"
Without adding firmware-duplicate to this list, the new error injection
capability added to lib/test_firmware.c might not be exercised by the
test suite.
> };
>
> static const char *fw_upload_progress(struct device *dev,
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-v6-10-topic-touchscreen-axiom-v6-0-1aa50ba3bc5a@pengutronix.de?part=1
^ permalink raw reply
* Re: [PATCH 09/10] HID: steam: Reject short reads
From: sashiko-bot @ 2026-07-02 22:36 UTC (permalink / raw)
To: Vicki Pfau; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260702222145.1863104-9-vi@endrift.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] The newly added short-read check in `steam_recv_report` contains multiple logic errors that render it ineffective, allowing short reads to bypass validation and leak uninitialized kernel stack memory, while also erroneously rejecting valid reports.
Pre-existing issues:
- [High] Self-deadlock in haptic rumble callbacks due to recursive acquisition of `steam->report_mutex`.
--
commit 27b5eb27907342525c15b39fe076131c6198f9b3
Author: Vicki Pfau <vi@endrift.com>
HID: steam: Reject short reads
Steam Controller FEATURE reports encode the size of the message in the
message itself. Previously we were trusting that the size reported matched
the size we actually read, leading to a potential issue with short reads.
Instead, we should actually verify the length of the read.
Link: https://syzkaller.appspot.com/bug?extid=75f3f9bff8c510602d36
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 593151709cf1d..e97431bc2828c 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -389,6 +389,12 @@ static int steam_recv_report(struct steam_device *steam,
> hid_err(steam->hdev, "%s: error %d\n", __func__, ret);
> else
> hid_dbg(steam->hdev, "Received report %*ph\n", ret, data);
> +
> + if (ret >= 2 && data[1] > ret + 2) {
[Severity: High]
Does this logic effectively catch short reads, or does it contain an algebraic
error?
It appears data[1] > ret + 2 should be data[1] + 2 > ret. With the current
check, a device returning a short payload would bypass validation.
Additionally, since this check happens after ret is clamped to size earlier
in the function (ret = min(size, ret - 1);), won't this reject perfectly valid
large reports if the caller only requested a smaller buffer?
Also, the ret >= 2 gate allows 0 or 1 byte reads to return successfully. If
this occurs, callers like steam_get_serial() will read from an uninitialized
stack buffer.
For instance, in steam_get_serial():
drivers/hid/hid-steam.c:steam_get_serial() {
u8 reply[3 + STEAM_SERIAL_LEN + 1];
...
res = steam_recv_report(steam, reply, sizeof(reply));
if (res < 0)
return res;
reply[3 + reply[1]] = 0;
strscpy(steam->serial_no, reply + 3, sizeof(steam->serial_no));
}
If steam_recv_report() returns 0 or 1, reply[1] is uninitialized,
potentially causing out-of-bounds reads or leaking kernel stack memory into
steam->serial_no.
> + hid_err(steam->hdev, "%s: expected %u bytes, read %i\n",
> + __func__, data[1] + 2, ret);
> + return -EPROTO;
> + }
> return ret;
> }
[Severity: High]
This is a pre-existing issue, but does steam_haptic_rumble_cb() cause a
self-deadlock when triggering rumble?
This was introduced by the commit "HID: steam: Coalesce rumble packets"
earlier in the same series and persists through the end of the series.
In steam_haptic_rumble_cb():
drivers/hid/hid-steam.c:steam_haptic_rumble_cb() {
guard(mutex)(&steam->report_mutex);
steam_haptic_rumble(steam, 0, steam->rumble_left,
steam->rumble_right, 2, 0);
}
And steam_haptic_rumble() unconditionally locks the same mutex:
drivers/hid/hid-steam.c:steam_haptic_rumble() {
mutex_lock(&steam->report_mutex);
ret = steam_send_report(steam, report, sizeof(report));
mutex_unlock(&steam->report_mutex);
}
Since Linux mutexes are not recursive, won't this permanently hang the system
workqueue worker executing the rumble callbacks?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260702222145.1863104-1-vi@endrift.com?part=9
^ permalink raw reply
* Re: [PATCH 04/10] HID: steam: Add support for sensor events on the Steam Controller (2015)
From: sashiko-bot @ 2026-07-02 22:36 UTC (permalink / raw)
To: Vicki Pfau; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260702222145.1863104-4-vi@endrift.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [Medium] `steam_get_attributes()` incorrectly parses the device attributes payload due to a wrong buffer offset and incorrect loop bounds, misinterpreting the report header as payload data.
Pre-existing issues:
- [High] Concurrent execution of device registration due to an unprotected race between `steam_probe` and `steam_work_connect_cb` leads to double allocation and a memory leak of input devices.
- [High] If `steam_register` fails, the device remains in the global `steam_devices` list, leading to a Use-After-Free when the memory is freed by devres.
--
commit 73cdd9e51b7293cba488d76f23eb3d6d1d4ca204
Author: Vicki Pfau <vi@endrift.com>
HID: steam: Add support for sensor events on the Steam Controller (2015)
Sensor support was added for the Steam Deck previously, but Steam
Controller sensor events were never added. This adds that missing support,
bringing Steam Controller support much closer to feature parity with things
like SDL and Steam itself.
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index add9f4f27fff9..34653ad383ac0 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -468,6 +500,38 @@ static int steam_get_serial(struct steam_device *steam)
> return ret;
> }
>
> +static int steam_get_attributes(struct steam_device *steam)
> +{
> + int ret = 0;
> + u8 cmd[] = {ID_GET_ATTRIBUTES_VALUES, 0};
> + u8 reply[64] = {};
> + u8 size;
> + int i;
> + struct steam_controller_attribute *attr;
> +
> + guard(mutex)(&steam->report_mutex);
> + ret = steam_send_report(steam, cmd, sizeof(cmd));
> + if (ret < 0)
> + return ret;
> + ret = steam_recv_report(steam, reply, sizeof(reply));
> + if (ret < 0)
> + return ret;
> + if (reply[0] != ID_GET_ATTRIBUTES_VALUES || reply[1] < 2)
> + return -EIO;
> +
> + size = min(reply[1], sizeof(reply) - 2);
> + for (i = 0; i + sizeof(*attr) <= size; i += sizeof(*attr)) {
> + attr = (struct steam_controller_attribute *)&reply[i];
[Severity: Medium]
Does this code incorrectly parse the device attributes payload?
The iteration starts at index 0, which is the message ID reply[0], instead
of index 2 where the payload actually begins.
Also, the size check relies on reply[1] but misses the +2 offset for the
header.
Will this misread the header as payload data and fail to extract attributes
like ATTRIB_CONNECTION_INTERVAL_IN_US?
> + if (attr->tag == ATTRIB_CONNECTION_INTERVAL_IN_US) {
> + steam->sensor_update_rate_us = get_unaligned_le32(&attr->value);
> + hid_dbg(steam->hdev, "Sensor update rate: %uus\n",
> + steam->sensor_update_rate_us);
> + }
> + }
> +
> + return 0;
> +}
> +
[ ... ]
> @@ -839,9 +939,6 @@ static int steam_sensors_register(struct steam_device *steam)
> struct input_dev *sensors;
> int ret;
>
> - if (!(steam->quirks & STEAM_QUIRK_DECK))
> - return 0;
> -
> rcu_read_lock();
> sensors = rcu_dereference(steam->sensors);
> rcu_read_unlock();
[Severity: High]
This is a pre-existing issue, but does steam_input_register() have a race
condition that leaks memory?
Looking at steam_input_register() (and steam_sensors_register() as well), the
check for an existing input device is not protected by a lock:
rcu_read_lock();
input = rcu_dereference(steam->input);
rcu_read_unlock();
if (input) {
dbg_hid("%s: already connected\n", __func__);
return 0;
}
input = input_allocate_device();
If a wired Steam controller sends a wireless connection event
(ID_CONTROLLER_WIRELESS) immediately after its hardware is started in
steam_probe(), steam_work_connect_cb() is queued and runs concurrently with
the rest of steam_probe().
Can both threads enter steam_register() and steam_input_register(), see a
NULL pointer, and allocate/register the input device twice?
[ ... ]
> @@ -968,6 +1077,12 @@ static int steam_register(struct steam_device *steam)
> strscpy(steam->serial_no, "XXXXXXXXXX",
> sizeof(steam->serial_no));
>
> + ret = steam_get_attributes(steam);
> + if (ret < 0)
> + hid_err(steam->hdev,
> + "%s:steam_get_attributes failed with error %d\n",
> + __func__, ret);
> +
[Severity: High]
This is a pre-existing issue, but does the error handling in steam_probe()
leak the device in the global steam_devices list?
When steam_register() is called, it adds the device to the global list:
mutex_lock(&steam_devices_lock);
if (list_empty(&steam->list))
list_add(&steam->list, &steam_devices);
mutex_unlock(&steam_devices_lock);
If a subsequent step in steam_register() fails (like input_allocate_device()
failing due to memory pressure), it returns an error but leaves the device in
the list.
Then in steam_probe():
ret = steam_register(steam);
if (ret) {
hid_err(hdev,
"%s:steam_register failed with error %d\n",
__func__, ret);
goto err_hw_close;
}
The error handling path skips the err_steam_unregister label. The driver core
frees the devres-managed steam_device, leaving a dangling pointer in the
steam_devices list.
Could subsequent iterations of this list (like when writing to the lizard_mode
module parameter) dereference the freed pointer and cause a Use-After-Free?
> hid_info(steam->hdev, "Steam Controller '%s' connected",
> steam->serial_no);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260702222145.1863104-1-vi@endrift.com?part=4
^ permalink raw reply
* Re: [PATCH 08/10] HID: steam: Improve logging and other cleanup
From: sashiko-bot @ 2026-07-02 22:36 UTC (permalink / raw)
To: Vicki Pfau; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260702222145.1863104-8-vi@endrift.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Self-deadlock in rumble callbacks due to nested acquisition of `steam->report_mutex`.
- [Medium] The newly added error path in `steam_get_serial()` unconditionally prints the uninitialized stack buffer `reply`, leading to a kernel stack information leak.
Pre-existing issues:
- [High] Race condition in `steam_probe` error path with `work_connect` leads to UAF and dangling input devices.
--
commit 8cdb505892ea25a09ef1370038b435985daea2fb
Author: Vicki Pfau <vi@endrift.com>
HID: steam: Improve logging and other cleanup
This commit adds debug logging to report sending and receiving, re-indents
input report identifiers for consistent styling, and converts several mutex
lock/unlock sequences to use the guard(mutex) scoped cleanup pattern.
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 9d1fe97921011..593151709cf1d 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -481,22 +490,21 @@ static int steam_get_serial(struct steam_device *steam)
> u8 cmd[] = {ID_GET_STRING_ATTRIBUTE, sizeof(steam->serial_no), ATTRIB_STR_UNIT_SERIAL};
> u8 reply[3 + STEAM_SERIAL_LEN + 1];
>
> - mutex_lock(&steam->report_mutex);
> + guard(mutex)(&steam->report_mutex);
> ret = steam_send_report(steam, cmd, sizeof(cmd));
> if (ret < 0)
> - goto out;
> + return ret;
> ret = steam_recv_report(steam, reply, sizeof(reply));
> if (ret < 0)
> - goto out;
> + return ret;
> if (reply[0] != ID_GET_STRING_ATTRIBUTE || reply[1] < 1 ||
> reply[1] > sizeof(steam->serial_no) || reply[2] != ATTRIB_STR_UNIT_SERIAL) {
> - ret = -EIO;
> - goto out;
> + hid_err(steam->hdev, "%s: invalid reply (%*ph)\n", __func__,
> + (int)sizeof(reply), reply);
[Severity: Medium]
Could this unconditionally print the uninitialized reply stack buffer?
steam_get_serial() allocates reply on the stack without initializing it.
steam_recv_report() reads data into reply, but if the device returns a
short response, only part of reply is written.
In this error path, hid_err() uses %*ph to print the entire sizeof(reply)
bytes. Could this expose uninitialized kernel stack memory to dmesg if the
device malfunctions or returns a short response?
> + return -EIO;
> }
> reply[3 + STEAM_SERIAL_LEN] = 0;
> strscpy(steam->serial_no, reply + 3, reply[1]);
> -out:
> - mutex_unlock(&steam->report_mutex);
> return ret;
> }
I also noticed a couple of other issues in this file while reviewing this
patch.
drivers/hid/hid-steam.c:
static void steam_haptic_rumble_cb(struct work_struct *work)
{
struct steam_device *steam = container_of(work, struct steam_device,
rumble_work);
guard(mutex)(&steam->report_mutex);
steam_haptic_rumble(steam, 0, steam->rumble_left,
steam->rumble_right, 2, 0);
}
[Severity: High]
I noticed an issue introduced by the "Coalesce rumble packets" patch earlier in
this same series. Does this callback cause a self-deadlock?
Both steam_haptic_rumble_cb() and steam_coalesce_rumble_cb() acquire
steam->report_mutex via guard(mutex)(&steam->report_mutex) and then immediately
invoke steam_haptic_rumble().
Since steam_haptic_rumble() unconditionally calls
mutex_lock(&steam->report_mutex) and Linux mutexes are not recursive, won't
this result in a guaranteed self-deadlock of the workqueue thread the first
time a rumble effect is played?
drivers/hid/hid-steam.c:steam_probe() {
...
err_steam_unregister:
if (steam->connected)
steam_unregister(steam);
err_hw_close:
hid_hw_close(hdev);
err_hw_stop:
hid_hw_stop(hdev);
err_cancel_work:
cancel_work_sync(&steam->work_connect);
...
}
[Severity: High]
This is a pre-existing issue, but is there a race condition in this teardown
order?
If steam_probe() fails after requesting wireless connection status, the device
may have already triggered steam_do_connect_event(), which schedules
steam->work_connect.
In the error path above, steam_unregister() is called before the worker is
cancelled with cancel_work_sync(&steam->work_connect).
If the worker is executing concurrently, steam_unregister() may do nothing
because serial_no isn't set yet. The worker would then complete
steam_register() and register the input devices.
After cancel_work_sync() waits for the worker to finish, steam_probe()
returns, freeing the steam struct. Could this leave dangling input devices
that will trigger a use-after-free when accessed?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260702222145.1863104-1-vi@endrift.com?part=8
^ permalink raw reply
* Re: [PATCH 10/10] HID: steam: Retry send/recv reports if stale
From: sashiko-bot @ 2026-07-02 22:36 UTC (permalink / raw)
To: Vicki Pfau; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260702222145.1863104-10-vi@endrift.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Insufficient bounds checking allows uninitialized kernel stack memory to be parsed, exposed to userspace, and logged to dmesg.
--
commit 74668d65404cd2e20c8afd5929ec0da105fd2ec0
Author: Vicki Pfau <vi@endrift.com>
HID: steam: Retry send/recv reports if stale
This commit introduces a retry mechanism for sending and receiving reports to
handle stale replies. It adds steam_exchange_report() to wrap the send and
receive logic with retries, avoiding failures from firmware bugs.
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index e97431bc2828c..8d036b3412539 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> +static int steam_exchange_report(struct steam_device *steam, u8 *cmd, int csize,
> + u8 *reply, int rsize)
> +{
> + unsigned int retries = 5;
> + int ret;
> +
> + guard(mutex)(&steam->report_mutex);
> + do {
> + ret = steam_send_report(steam, cmd, csize);
> + if (ret < 0)
> + return ret;
> + ret = steam_recv_report(steam, reply, rsize);
[ ... ]
> + if (ret < 0) {
> + hid_err(steam->hdev, "%s: error reading reply (%*ph)\n",
> + __func__, csize, cmd);
> + return ret;
> + }
> + if (reply[0] == cmd[0] && reply[1] >= 1)
[Severity: High]
This is a pre-existing issue, but does this code evaluate uninitialized
stack memory if steam_recv_report() returns a short read?
steam_recv_report() has a mathematically flawed bounds check that can fail
to reject truncated packets or bypass the check entirely for 0-1 byte replies:
if (ret >= 2 && data[1] > ret + 2) {
...
return -EPROTO;
}
Because reply is uninitialized on the stack in callers like steam_get_serial(),
can a short read from a malfunctioning or malicious USB device cause us to
read uninitialized memory here?
Could it also potentially return success if the stack happens to match the
expected values?
> + break;
> + if (retries > 0)
> + continue;
> + hid_err(steam->hdev, "%s: invalid reply (%*ph)\n", __func__,
> + rsize, reply);
[Severity: High]
This is a pre-existing issue, but if steam_recv_report() returned a short read,
does this log uninitialized stack memory to dmesg since it dumps the full
rsize bytes of the reply buffer?
> + return -EPROTO;
> + } while (retries--);
> +
> + return ret;
> +}
> +
> static int steam_get_serial(struct steam_device *steam)
> {
[ ... ]
> u8 cmd[] = {ID_GET_STRING_ATTRIBUTE, sizeof(steam->serial_no), ATTRIB_STR_UNIT_SERIAL};
> u8 reply[3 + STEAM_SERIAL_LEN + 1];
>
> - guard(mutex)(&steam->report_mutex);
> - ret = steam_send_report(steam, cmd, sizeof(cmd));
> + ret = steam_exchange_report(steam, cmd, sizeof(cmd), reply, sizeof(reply));
> if (ret < 0)
> return ret;
> - ret = steam_recv_report(steam, reply, sizeof(reply));
> - if (ret < 0)
> - return ret;
> - if (reply[0] != ID_GET_STRING_ATTRIBUTE || reply[1] < 1 ||
> - reply[1] > sizeof(steam->serial_no) || reply[2] != ATTRIB_STR_UNIT_SERIAL) {
> + if (reply[1] > sizeof(steam->serial_no) || reply[2] != ATTRIB_STR_UNIT_SERIAL) {
[Severity: High]
This is a pre-existing issue, but since the length from reply[1] is not
validated against the actual bytes received, does the later call to strscpy()
leak up to 31 bytes of uninitialized kernel stack memory to userspace?
drivers/hid/hid-steam.c:steam_get_serial() {
...
reply[3 + STEAM_SERIAL_LEN] = 0;
strscpy(steam->serial_no, reply + 3, reply[1]);
return ret;
}
Can a remote or malfunctioning USB device returning short or empty HID payloads
trigger this information leak?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260702222145.1863104-1-vi@endrift.com?part=10
^ 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