* Re: [PATCH v2 4/7] i2c: of-prober: Defer regulator_disable() on successful probe in simple helper
From: sashiko-bot @ 2026-07-03 12:08 UTC (permalink / raw)
To: Chen-Yu Tsai; +Cc: dmitry.torokhov, devicetree, linux-input, conor+dt, robh
In-Reply-To: <20260703115601.1323491-5-wenst@chromium.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The prober unconditionally sleeps for 100ms during the cleanup path even when no matching I2C component is found, unnecessarily delaying system boot.
--
commit 772a8f5b00b1078ef6740a0746df36699760f2f2
Author: Chen-Yu Tsai <wenst@chromium.org>
i2c: of-prober: Defer regulator_disable() on successful probe in simple helper
This patch defers regulator disabling in the I2C OF component prober by adding
a 100ms delay. The delay allows asynchronously probing drivers to catch and
increase the regulator enable count, avoiding hardware re-initialization.
> diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c
> index f274e260353cd..17495125467ed 100644
> --- a/drivers/i2c/i2c-core-of-prober.c
> +++ b/drivers/i2c/i2c-core-of-prober.c
> @@ -243,11 +243,23 @@ static int i2c_of_probe_simple_enable_regulator(struct device *dev, struct i2c_o
> return 0;
> }
>
> -static void i2c_of_probe_simple_disable_regulator(struct device *dev, struct i2c_of_probe_simple_ctx *ctx)
> +static void i2c_of_probe_simple_disable_regulator(struct device *dev,
> + struct i2c_of_probe_simple_ctx *ctx,
> + bool defer_disable)
> {
> if (!ctx->supply)
> return;
>
> + /*
> + * Wait a bit of time for async drivers to probe and increase the
> + * regulator enable count. This allows the drivers to check and
> + * skip waiting for re-initialization.
> + */
> + if (defer_disable) {
> + dev_dbg(dev, "Deferring regulator disable\n");
> + msleep(100);
> + }
> +
[ ... ]
> @@ -409,7 +421,7 @@ void i2c_of_probe_simple_cleanup(struct device *dev, void *data)
> i2c_of_probe_simple_disable_gpio(dev, ctx);
> i2c_of_probe_simple_put_gpiod(ctx);
>
> - i2c_of_probe_simple_disable_regulator(dev, ctx);
> + i2c_of_probe_simple_disable_regulator(dev, ctx, true);
[Severity: Medium]
Does this unconditionally sleep for 100ms even when no matching I2C component
is found?
If i2c_of_probe_component() iterates over potential components but finds none,
it calls ops->cleanup(dev, ctx), which invokes i2c_of_probe_simple_cleanup().
Because defer_disable is hardcoded to true here,
i2c_of_probe_simple_disable_regulator() will execute the 100ms sleep.
Could this result in an unnecessary 100ms boot delay on systems missing the
probed component, since no async driver will ever probe in this failure path?
> i2c_of_probe_simple_put_supply(ctx);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703115601.1323491-1-wenst@chromium.org?part=4
^ permalink raw reply
* Re: [PATCH v2 2/7] HID: i2c-hid-of: skip post-power-on delay if already powered on
From: sashiko-bot @ 2026-07-03 12:07 UTC (permalink / raw)
To: Chen-Yu Tsai; +Cc: devicetree, robh, conor+dt, linux-input, dmitry.torokhov
In-Reply-To: <20260703115601.1323491-3-wenst@chromium.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Bitwise AND assignment (`&=`) between a `bool` and the `int` return value of `regulator_is_enabled()` causes pseudo-random evaluation based on the least significant bit (LSB) of the integer, potentially skipping mandatory delays.
- [High] Skipping the post-power-on delay based solely on the current state of `regulator_is_enabled()` introduces a race condition for devices sharing a power rail.
--
commit 14324865d2e3b5ccaf90d6ff025e907befb17313
Author: Chen-Yu Tsai <wenst@chromium.org>
HID: i2c-hid-of: skip post-power-on delay if already powered on
This patch checks if the regulator supplies for an i2c-hid device are
already enabled during probe. If all supplies are already on, it skips
the post-power-on delay to save boot time.
> diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
> --- a/drivers/hid/i2c-hid/i2c-hid-of.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-of.c
> @@ -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);
[Severity: High]
Could this bitwise operation cause a regression by inadvertently evaluating
the least significant bit of the return value?
The function regulator_is_enabled() returns an int (positive state, 0, or
a negative error). In C, performing a bitwise AND (&=) between a bool
(promoted to integer 1) and an int isolates the least significant bit.
For example, if regulator_is_enabled() returns a negative error code like
-ENODEV (-19), the least significant bit is 1, causing supply_was_enabled
to evaluate to true despite the error. If it returns an even positive
state like 2, the least significant bit is 0, making it false.
[ ... ]
> if (ret) {
> dev_warn(dev, "Failed to enable supplies: %d\n", ret);
> 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);
[Severity: High]
Is it safe to skip this delay based solely on whether the regulator is
currently enabled?
If two devices (e.g. a touchscreen and a trackpad) share the same
non-always-on regulator and probe asynchronously:
Device 1 starts probing, calls regulator_bulk_enable(), and begins
waiting its post-power delay.
Device 2 concurrently probes in i2c_hid_of_power_up(), sees
regulator_is_enabled() is true, and skips its post_power_delay_ms here.
Device 2 would then immediately issue I2C transactions to a newly powered
device that might not have finished initializing, potentially resulting
in NACKs and a probe regression.
Does the regulator framework provide a way to ensure the rail has been on
for the required duration, rather than just currently enabled?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703115601.1323491-1-wenst@chromium.org?part=2
^ permalink raw reply
* Re: [PATCH v2 1/7] Input: elan_i2c - Wait for initialization after enabling regulator supply
From: sashiko-bot @ 2026-07-03 12:06 UTC (permalink / raw)
To: Chen-Yu Tsai; +Cc: conor+dt, devicetree, linux-input, robh, dmitry.torokhov
In-Reply-To: <20260703115601.1323491-2-wenst@chromium.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The return value of `regulator_is_enabled()` is implicitly cast to `bool`, causing negative error codes to evaluate as true and incorrectly bypassing the required power-on delay.
- [High] Skipping the power-on delay for shared regulators is susceptible to unsafe power domain assumptions and TOCTOU races, leading to bypassed hardware initialization.
--
commit 62ef124436c7ca613cd0f8d922094f42cdd2a0b8
Author: Chen-Yu Tsai <wenst@chromium.org>
Input: elan_i2c - Wait for initialization after enabling regulator supply
This commit adds proper delays after enabling the regulator supply for Elan
trackpad controllers, which require time for hardware and firmware to
initialize. It attempts to avoid boot time impact by skipping the delay if
the regulator was already enabled.
> diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> index fee1796da3d08..110c662cef96f 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
[ ... ]
> @@ -1187,6 +1189,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) &&
> @@ -1218,6 +1221,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);
[Severity: High]
In elan_probe(), regulator_is_enabled() returns an int, which can include
negative error codes like -EPROBE_DEFER or -EREMOTEIO. Will assigning this to
the bool supply_was_enabled cause negative error codes to evaluate as true?
If this happens, it looks like it would incorrectly bypass the power-on delay
and lead to a permanent -ENXIO probe failure later rather than allowing a
proper deferral.
> +
> error = regulator_enable(data->vcc);
> if (error) {
> dev_err(dev, "Failed to enable regulator: %d\n", error);
> @@ -1231,6 +1236,9 @@ static int elan_probe(struct i2c_client *client)
> return error;
> }
>
> + if (!supply_was_enabled)
> + msleep(ETP_POWER_ON_DELAY);
[Severity: High]
Is it safe to assume that a regulator that is already enabled has been powered
on for a sufficient amount of time?
If the trackpad shares a regulator with another device (like a touchscreen)
that just finished its own probe milliseconds earlier, the regulator would
already be enabled, but the trackpad hardware wouldn't be fully initialized.
Could this cause elan_probe() to skip the required hardware initialization
delay and fail the subsequent i2c_smbus_read_byte() check?
[ ... ]
> @@ -1374,11 +1382,16 @@ static int elan_resume(struct device *dev)
> int error;
>
> if (!device_may_wakeup(dev)) {
> + bool supply_was_enabled = regulator_is_enabled(data->vcc);
[Severity: High]
In elan_resume(), could the same boolean cast issue happen if
regulator_is_enabled() encounters a transient failure during system resume,
causing the resume delay to be skipped?
> +
> 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);
[Severity: High]
Could the same shared-regulator timing race occur during the system resume
path, skipping the initialization delay if another device on the same shared
power domain resumed just before the trackpad?
> }
>
> error = elan_set_power(data, true);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703115601.1323491-1-wenst@chromium.org?part=1
^ permalink raw reply
* Re: [PATCH v2 3/7] i2c: of-prober: skip post-power-on delay if already powered on
From: sashiko-bot @ 2026-07-03 12:05 UTC (permalink / raw)
To: Chen-Yu Tsai; +Cc: conor+dt, dmitry.torokhov, robh, linux-input, devicetree
In-Reply-To: <20260703115601.1323491-4-wenst@chromium.org>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] The integer return value of `regulator_is_enabled()` (which can be a negative error code) is implicitly cast to or bitwise-ANDed with `bool` variables, leading to incorrect evaluations that inadvertently skip required hardware power-on delays.
- [Medium] Shared regulator logic flaw and TOCTOU race allow skipping required power-on delays.
Pre-existing issues:
- [High] Optional regulator supplies missing from the device tree incorrectly cause the prober to fail, contradicting the intended optional behavior.
--
commit 7072efff03a9502c14f9b2eb5a3863213e3992ef
Author: Chen-Yu Tsai <wenst@chromium.org>
i2c: of-prober: skip post-power-on delay if already powered on
This commit modifies the I2C OF prober to check if regulator supplies are
already enabled before applying a post-power-on delay. If the supply is
already active, it skips the delay to avoid unnecessary boot time penalties.
> diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c
> index 6a82b03809d4f..f274e260353cd 100644
> --- a/drivers/i2c/i2c-core-of-prober.c
> +++ b/drivers/i2c/i2c-core-of-prober.c
[ ... ]
> @@ -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);
[Severity: High]
Could this introduce a regression by skipping required hardware power-on
delays if regulator_is_enabled() fails?
The integer return value of regulator_is_enabled() can be a negative error
code, but it is implicitly cast to the bool variable supply_was_on without
error checking. Any non-zero integer, including negative values, becomes true.
If regulator_is_enabled() returns an error, the code will consider the supply
as already on, causing the device to skip the mandatory post-power-on delay
and fail to probe because it has not finished initializing.
I noticed similar patterns in other drivers:
- drivers/hid/i2c-hid/i2c-hid-of.c:i2c_hid_of_power_up()
- drivers/input/mouse/elan_i2c_core.c:elan_probe()
- drivers/input/mouse/elan_i2c_core.c:elan_resume()
[Severity: Medium]
Is there a potential regression here regarding a shared regulator TOCTOU race
condition?
If two devices share a regulator and probe concurrently, Driver A might enable
the regulator. When Driver B checks regulator_is_enabled(), it returns true.
Driver B will then skip its required power-on delay and attempt to communicate
with the device.
If Driver A enabled the regulator less than the required delay time ago,
Driver B will access its device before the hardware has initialized, leading
to probe failure or I2C communication errors.
> +
> + 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;
> }
[Severity: High]
This is a pre-existing issue, but I noticed optional regulator supplies missing
from the device tree incorrectly cause the prober to fail, contradicting the
intended optional behavior.
drivers/i2c/i2c-core-of-prober.c:i2c_of_probe_simple_get_supply() {
...
supply = of_regulator_get_optional(dev, node, supply_name);
if (IS_ERR(supply)) {
return dev_err_probe(dev, PTR_ERR(supply),
"Failed to get regulator supply \"%s\" from %pOF\n",
supply_name, node);
}
...
}
of_regulator_get_optional() returns -ENODEV when the supply is not in the
device tree. The function checks IS_ERR(supply) and propagates the error,
failing the probe unconditionally on hardware platforms where the device tree
omits the optional regulator.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703115601.1323491-1-wenst@chromium.org?part=3
^ permalink raw reply
* [PATCH v2 7/7] arm64: dts: mediatek: mt8192-asurada-spherion: Add Synaptics trackpad's supply
From: Chen-Yu Tsai @ 2026-07-03 11:56 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,
stable+noautosel
In-Reply-To: <20260703115601.1323491-1-wenst@chromium.org>
The Synaptics trackpad, like the Elan trackpad option, is fed from the
system 3.3V power rail. Add it to the trackpad device node.
Also add the correct post-power-on delay, even though in practice it is
not required. The Synaptics trackpad requires 100ms after power-on (or
deasserting the reset, whichever comes later) to fully initialize. The
power is always on and the reset pin is not routed out, so the
implementation could try skipping the delay.
Cc: <stable+noautosel@kernel.org> # Without driver changes only lengthens probe time
Fixes: 925ebc0cd55c ("arm64: dts: mt8192-asurada-spherion: Add Synaptics trackpad support")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
I think this shouldn't be backported, as backporting it without the
driver enhancements just delays the trackpad probing with no real
gains.
---
arch/arm64/boot/dts/mediatek/mt8192-asurada-spherion-r0.dts | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/mediatek/mt8192-asurada-spherion-r0.dts b/arch/arm64/boot/dts/mediatek/mt8192-asurada-spherion-r0.dts
index 163960f58db5..147a8e9a3a71 100644
--- a/arch/arm64/boot/dts/mediatek/mt8192-asurada-spherion-r0.dts
+++ b/arch/arm64/boot/dts/mediatek/mt8192-asurada-spherion-r0.dts
@@ -90,6 +90,8 @@ trackpad@2c {
hid-descr-addr = <0x20>;
interrupts-extended = <&pio 15 IRQ_TYPE_LEVEL_LOW>;
wakeup-source;
+ vdd-supply = <&pp3300_u>;
+ post-power-on-delay-ms = <100>;
status = "fail-needs-probe";
};
};
--
2.55.0.rc0.799.gd6f94ed593-goog
^ permalink raw reply related
* [PATCH v2 6/7] arm64: dts: mediatek: mt8173-elm-hana: Unmark trackpad supply as always-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>
Up until now, the MT8173 elm/hana device tree has set the dedicated
regulator supplying the trackpad as always-on, simply because the Elan
driver was missing proper delays. As a result the delay for the
Synaptics trackpad was also omitted, as it was not strictly required
under such a model and delayed the availability of the trackpad to the
user.
The Elan driver recently gained proper delays after power up, with
opportunistic skipping of the delay when the regulator was originally
on. The I2C HID driver gained similar opportunistic delay skipping.
So has the I2C OF component prober library.
Now fix the device tree to have the regulator not be always on, and
let the I2C HID device have the correct post-power-on delay time.
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi | 8 +-------
arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 1 -
2 files changed, 1 insertion(+), 8 deletions(-)
diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi
index 1004eb8ea52c..b9e311fcd9a0 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi
@@ -62,13 +62,7 @@ trackpad2: trackpad@2c {
pinctrl-0 = <&trackpad_irq>;
reg = <0x2c>;
hid-descr-addr = <0x0020>;
- /*
- * The trackpad needs a post-power-on delay of 100ms,
- * but at time of writing, the power supply for it on
- * this board is always on. The delay is therefore not
- * added to avoid impacting the readiness of the
- * trackpad.
- */
+ post-power-on-delay-ms = <100>;
vdd-supply = <&mt6397_vgp6_reg>;
wakeup-source;
status = "fail-needs-probe";
diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
index a0573bc359fb..6b9f47f515c7 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
@@ -1093,7 +1093,6 @@ mt6397_vgp6_reg: ldo_vgp6 {
regulator-min-microvolt = <3300000>;
regulator-max-microvolt = <3300000>;
regulator-enable-ramp-delay = <218>;
- regulator-always-on;
};
mt6397_vibr_reg: ldo_vibr {
--
2.55.0.rc0.799.gd6f94ed593-goog
^ permalink raw reply related
* [PATCH v2 5/7] platform/chrome: of_hw_prober: Add delay for hana trackpads
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>
Up until now, the MT8173 elm/hana device tree has set the dedicated
regulator supplying the trackpad as always-on, simply because the Elan
driver was missing proper delays. As a result the delay for the
Synaptics trackpad was also omitted, as it was not strictly required
under such a model and delayed the availability of the trackpad to the
user.
The Elan driver recently gained proper delays after power-up, with
adaptive skipping of the delay if the regulator was originally
on. The I2C HID driver and I2C OF component prober library gained
similar adaptive delay skipping. The device tree will be fixed to have
the regulator not be always on, and proper post-power-on delay time
added to the I2C HID device.
Also add the post-power-on delay to the ChromeOS OF component prober,
so that if the regulator is off at the time of probing, the prober knows
to wait for the hardware to initialize.
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
drivers/platform/chrome/chromeos_of_hw_prober.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/platform/chrome/chromeos_of_hw_prober.c b/drivers/platform/chrome/chromeos_of_hw_prober.c
index 8562a0e89dc6..54d8941617e2 100644
--- a/drivers/platform/chrome/chromeos_of_hw_prober.c
+++ b/drivers/platform/chrome/chromeos_of_hw_prober.c
@@ -70,10 +70,8 @@ static const struct chromeos_i2c_probe_data chromeos_i2c_probe_hana_trackpad = {
/*
* ELAN trackpad needs 2 ms for H/W init and 100 ms for F/W init.
* Synaptics trackpad needs 100 ms.
- * However, the regulator is set to "always-on", presumably to
- * avoid this delay. The ELAN driver is also missing delays.
*/
- .post_power_on_delay_ms = 0,
+ .post_power_on_delay_ms = 110,
},
};
--
2.55.0.rc0.799.gd6f94ed593-goog
^ permalink raw reply related
* [PATCH v2 4/7] i2c: of-prober: Defer regulator_disable() on successful probe in simple helper
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>
When a I2C component is found, it's device node is immediately enabled.
This triggers device creation and driver binding. The prober will hold
the regulator enable reference across this part. If the driver probes
synchronously, then it happens within this window. On the other hand,
if the driver probes asynchronously, there is high chance that it
happens after the prober's cleanup function was called, in which case
the regulator would have been disabled when the driver's probe function
is called. This would then require the driver to wait 100 ms for the
hardware to reinitialize, even if the probe function was just a split
second late and the regulator was disabled a few milliseconds ago.
Recently, some of the drivers for the component that are targeted by the
I2C OF component prober gained the ability to skip waiting for hardware
initialization if the regulator was left enabled. This happens when the
PMIC has them on by default, or if the component prober left them on
after probing the component.
Wait a bit of time before dropping the enable refcount on our end so
that the actual driver has the opportunity to catch and increase the
refcount on their end. The 100 ms delay was arbitrarily chosen.
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
drivers/i2c/i2c-core-of-prober.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c
index f274e260353c..17495125467e 100644
--- a/drivers/i2c/i2c-core-of-prober.c
+++ b/drivers/i2c/i2c-core-of-prober.c
@@ -243,11 +243,23 @@ static int i2c_of_probe_simple_enable_regulator(struct device *dev, struct i2c_o
return 0;
}
-static void i2c_of_probe_simple_disable_regulator(struct device *dev, struct i2c_of_probe_simple_ctx *ctx)
+static void i2c_of_probe_simple_disable_regulator(struct device *dev,
+ struct i2c_of_probe_simple_ctx *ctx,
+ bool defer_disable)
{
if (!ctx->supply)
return;
+ /*
+ * Wait a bit of time for async drivers to probe and increase the
+ * regulator enable count. This allows the drivers to check and
+ * skip waiting for re-initialization.
+ */
+ if (defer_disable) {
+ dev_dbg(dev, "Deferring regulator disable\n");
+ msleep(100);
+ }
+
dev_dbg(dev, "Disabling regulator supply \"%s\"\n", ctx->opts->supply_name);
regulator_disable(ctx->supply);
@@ -364,7 +376,7 @@ int i2c_of_probe_simple_enable(struct device *dev, struct device_node *bus_node,
return 0;
out_disable_regulator:
- i2c_of_probe_simple_disable_regulator(dev, ctx);
+ i2c_of_probe_simple_disable_regulator(dev, ctx, false);
out_put_gpiod:
i2c_of_probe_simple_put_gpiod(ctx);
out_put_supply:
@@ -409,7 +421,7 @@ void i2c_of_probe_simple_cleanup(struct device *dev, void *data)
i2c_of_probe_simple_disable_gpio(dev, ctx);
i2c_of_probe_simple_put_gpiod(ctx);
- i2c_of_probe_simple_disable_regulator(dev, ctx);
+ i2c_of_probe_simple_disable_regulator(dev, ctx, true);
i2c_of_probe_simple_put_supply(ctx);
}
EXPORT_SYMBOL_NS_GPL(i2c_of_probe_simple_cleanup, "I2C_OF_PROBER");
--
2.55.0.rc0.799.gd6f94ed593-goog
^ permalink raw reply related
* [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
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