* [PATCH v2 2/2] Input: atmel_mxt_ts: Allow per-board config via DT compatible
2026-05-23 15:23 [PATCH v2 1/2] Input: atmel_mxt_ts: Fix async config use-after-free Hendrik Noack
@ 2026-05-23 15:23 ` Hendrik Noack
2026-05-23 16:18 ` sashiko-bot
2026-05-23 16:01 ` [PATCH v2 1/2] Input: atmel_mxt_ts: Fix async config use-after-free sashiko-bot
1 sibling, 1 reply; 4+ messages in thread
From: Hendrik Noack @ 2026-05-23 15:23 UTC (permalink / raw)
To: Nick Dyer, Dmitry Torokhov; +Cc: Hendrik Noack, linux-input, linux-kernel
The Atmel maXTouch config is board-dependent: different panels and layouts
require different configs even when the same controller and firmware is
used.
Extend the driver to support per-board config files keyed by the device
tree compatible string. For DT-based systems, the driver now looks up the
primary compatible of the board and searches for a config file named:
atmel/maxtouch,<compatible>.cfg
If such a file exists, it is used as the board-specific config. If the
system is not DT-based or if the per-board file is not present, the driver
reverts to the original behaviour and searches for a config file named:
maxtouch.cfg
Signed-off-by: Hendrik Noack <hendrik-noack@gmx.de>
---
Changes in v2:
- fix leak of fw_name (thanks sashiko.dev)
- adopt to added patch
---
drivers/input/touchscreen/atmel_mxt_ts.c | 68 ++++++++++++++++++++++--
1 file changed, 63 insertions(+), 5 deletions(-)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 07690c3bff06..a9e9f151d042 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -36,7 +36,9 @@
/* Firmware files */
#define MXT_FW_NAME "maxtouch.fw"
-#define MXT_CFG_NAME "maxtouch.cfg"
+#define MXT_CFG_FOLDER "atmel"
+#define MXT_CFG_NAME "maxtouch"
+#define MXT_CFG_EXTENSION ".cfg"
#define MXT_CFG_MAGIC "OBP_RAW V1"
/* Registers */
@@ -2233,6 +2235,64 @@ static void mxt_config_cb(const struct firmware *cfg, void *ctx)
complete(&data->config_completion);
}
+static int mxt_invoke_config_loader(struct mxt_data *data, bool device_specific);
+
+static void mxt_board_config_cb(const struct firmware *cfg, void *ctx)
+{
+ if (!cfg) {
+ struct mxt_data *data = ctx;
+ int error;
+
+ error = mxt_invoke_config_loader(data, false);
+ /* request_firmware_nowait() succeeded and mxt_config_cb() will be called at end */
+ if (!error)
+ return;
+
+ dev_err(&data->client->dev, "Failed to invoke general config loader: %d\n", error);
+ }
+
+ mxt_config_cb(cfg, ctx);
+}
+
+static int mxt_invoke_config_loader(struct mxt_data *data, bool device_specific)
+{
+ struct device_node *root;
+ char *board_type = NULL;
+ char *fw_name;
+ void (*cb)(const struct firmware *fw, void *context);
+ int error;
+
+ root = of_find_node_by_path("/");
+ if (device_specific && root) {
+ const char *tmp;
+
+ if (!of_property_read_string_index(root, "compatible", 0, &tmp))
+ board_type = kstrdup(tmp, GFP_KERNEL);
+
+ of_node_put(root);
+ }
+
+ if (board_type) {
+ /* get rid of '/' in the compatible string to be able to find the FW */
+ strreplace(board_type, '/', '-');
+
+ fw_name = kasprintf(GFP_KERNEL, "%s/%s.%s%s", MXT_CFG_FOLDER, MXT_CFG_NAME,
+ board_type, MXT_CFG_EXTENSION);
+ cb = mxt_board_config_cb;
+ kfree(board_type);
+ } else {
+ fw_name = kasprintf(GFP_KERNEL, "%s%s", MXT_CFG_NAME, MXT_CFG_EXTENSION);
+ cb = mxt_config_cb;
+ }
+ if (!fw_name)
+ return -ENOMEM;
+
+ error = request_firmware_nowait(THIS_MODULE, true, fw_name, &data->client->dev, GFP_KERNEL,
+ data, cb);
+ kfree(fw_name);
+ return error;
+}
+
static int mxt_initialize(struct mxt_data *data)
{
struct i2c_client *client = data->client;
@@ -2288,11 +2348,9 @@ static int mxt_initialize(struct mxt_data *data)
reinit_completion(&data->config_completion);
}
- error = request_firmware_nowait(THIS_MODULE, true, MXT_CFG_NAME,
- &client->dev, GFP_KERNEL, data,
- mxt_config_cb);
+ error = mxt_invoke_config_loader(data, true);
if (error) {
- dev_err(&client->dev, "Failed to invoke firmware loader: %d\n",
+ dev_err(&client->dev, "Failed to invoke config loader: %d\n",
error);
complete(&data->config_completion);
return error;
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v2 1/2] Input: atmel_mxt_ts: Fix async config use-after-free
2026-05-23 15:23 [PATCH v2 1/2] Input: atmel_mxt_ts: Fix async config use-after-free Hendrik Noack
2026-05-23 15:23 ` [PATCH v2 2/2] Input: atmel_mxt_ts: Allow per-board config via DT compatible Hendrik Noack
@ 2026-05-23 16:01 ` sashiko-bot
1 sibling, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-05-23 16:01 UTC (permalink / raw)
To: Hendrik Noack; +Cc: dmitry.torokhov, linux-input
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues:
- [Critical] The newly added `config_lock` mutex in `struct mxt_data` is used without being initialized.
- [High] If `mxt_initialize()` aborts due to `shutting_down == true`, it consumes the completion but fails to restore it, leading to a permanent hang.
- [High] The `config_lock` mutex is held across a potentially unbounded wait, defeating the purpose of the `shutting_down` flag.
- [Medium] A time-of-check to time-of-use (TOCTOU) race allows asynchronous firmware loading to start after device removal has begun.
Pre-existing issues:
- [High] Concurrent sysfs writes to the `update_fw` attribute can trigger a double free of the input device.
--
commit a6627582981aef52fbfe146ee37db19f67d402dd
Author: Hendrik Noack <hendrik-noack@gmx.de>
Input: atmel_mxt_ts: Fix async config use-after-free
This commit attempts to fix a use-after-free by adding a mutex and a
completion to serialize the asynchronous firmware loading and the device
removal process.
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index a9e86ad7ed5ef..07690c3bff06e 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
[ ... ]
> @@ -2271,12 +2279,22 @@ static int mxt_initialize(struct mxt_data *data)
> if (error)
> return error;
>
> + scoped_guard(mutex, &data->config_lock) {
> + wait_for_completion(&data->config_completion);
[Severity: High]
Is it safe to hold the config_lock mutex across this wait_for_completion()
call?
Since the lock is held while waiting for a potentially long firmware load,
and mxt_remove() needs this same lock to set shutting_down to true, would
this cause mxt_remove() to block indefinitely, defeating the early shutdown
check?
> +
> + if (data->shutting_down)
> + return -EBUSY;
[Severity: High]
If shutting_down is true, the wait_for_completion() above has already
decremented the completion's counter.
Returning -EBUSY here without calling complete(&data->config_completion)
leaves the completion consumed. When mxt_remove() later calls
wait_for_completion(), will it hang forever waiting for a completion that
is never restored?
> +
> + reinit_completion(&data->config_completion);
> + }
> +
> error = request_firmware_nowait(THIS_MODULE, true, MXT_CFG_NAME,
> &client->dev, GFP_KERNEL, data,
> mxt_config_cb);
[Severity: Medium]
Because request_firmware_nowait() is called after the config_lock is
released, is there a race condition here?
If mxt_remove() runs immediately after the lock is dropped, it will acquire
the lock and set shutting_down to true, but request_firmware_nowait() will
still be invoked, starting a new asynchronous load while the device is
actively being removed.
[ ... ]
> @@ -3237,6 +3255,9 @@ static int mxt_probe(struct i2c_client *client)
> init_completion(&data->bl_completion);
> init_completion(&data->reset_completion);
> init_completion(&data->crc_completion);
> + init_completion(&data->config_completion);
> + complete(&data->config_completion);
> + data->shutting_down = false;
[Severity: Critical]
Does the config_lock mutex need to be initialized here?
The mxt_data structure is allocated with devm_kzalloc(), which
zero-initializes the memory. Using an uninitialized mutex with
scoped_guard() will cause a NULL pointer dereference when contention
occurs. Should there be a call to mutex_init(&data->config_lock)?
[ ... ]
> @@ -3342,6 +3363,12 @@ static void mxt_remove(struct i2c_client *client)
> {
> struct mxt_data *data = i2c_get_clientdata(client);
>
> + scoped_guard(mutex, &data->config_lock) {
> + data->shutting_down = true;
> + }
> +
> + wait_for_completion(&data->config_completion);
> +
> disable_irq(data->irq);
> mxt_free_input_device(data);
[Severity: High]
This is a pre-existing issue, but looking at mxt_free_input_device(), it
checks if data->input_dev is non-NULL and then unregisters it.
If concurrent sysfs writes to the update_fw attribute occur, multiple
threads can enter mxt_load_fw() and call mxt_free_input_device() at the
same time. Since sysfs doesn't serialize these writes and there is no
internal locking, could both threads pass the NULL check before either
sets it to NULL, resulting in a use-after-free and double-free of the
input device?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260523152332.86864-1-hendrik-noack@gmx.de?part=1
^ permalink raw reply [flat|nested] 4+ messages in thread