* [PATCH v2 0/3] dt-bindings: Convert TI TPS65217 to DT schema
From: Eduard Bostina @ 2026-07-18 21:56 UTC (permalink / raw)
To: Aaro Koskinen, Andreas Kemnade, Conor Dooley, Daniel Thompson,
devicetree, Dmitry Torokhov, dri-devel, Eduard Bostina,
Jingoo Han, Kevin Hilman, Krzysztof Kozlowski, Lee Jones,
linux-input, linux-kernel, linux-leds, linux-omap, Pavel Machek,
Rob Herring, Roger Quadros, Tony Lindgren
Cc: daniel.baluta, simona.toaca, goledhruva, m-chawdhry
This series converts the Texas Instruments TPS65217 PMIC bindings to DT
schema.
Changes in v2:
- Set 'additionalProperties' to false and documented all child nodes
('regulators', 'charger', 'pwrbutton').
- Documented the 'interrupts', 'interrupt-controller', '#interrupt-cells'
and 'ti,pmic-shutdown-controller' properties.
- Dropped 'isink-en' from the schema. It is not read by any driver, so it
is now removed from am335x-guardian.dts instead.
- Added the power button conversion and referenced it from
the leds schema.
Eduard Bostina (3):
ARM: dts: ti: am335x-guardian: Drop unused isink-en property
dt-bindings: input: Convert TI TPS65217 power button to DT schema
dt-bindings: leds: backlight: Convert TPS65217 to DT schema
.../bindings/input/ti,tps65217-pwrbutton.yaml | 42 ++++++
.../bindings/input/tps65218-pwrbutton.txt | 30 -----
.../bindings/leds/backlight/ti,tps65217.yaml | 123 ++++++++++++++++++
.../leds/backlight/tps65217-backlight.txt | 27 ----
arch/arm/boot/dts/ti/omap/am335x-guardian.dts | 3 -
5 files changed, 165 insertions(+), 60 deletions(-)
create mode 100644 Documentation/devicetree/bindings/input/ti,tps65217-pwrbutton.yaml
delete mode 100644 Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
create mode 100644 Documentation/devicetree/bindings/leds/backlight/ti,tps65217.yaml
delete mode 100644 Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt
--
2.43.0
^ permalink raw reply
* Re: [PATCH] Input: gunze: replace deprecated APIs and fix warning style
From: sashiko-bot @ 2026-07-18 19:12 UTC (permalink / raw)
To: Bivash Kumar Singh; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260718190040.10613-1-bivashraj750@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Replacing simple_strtoul() with kstrtoul() breaks packet parsing by unconditionally returning an error, dropping all touch events.
--
--- Patch [1]: [PATCH] Input: gunze: replace deprecated APIs and fix warning style ---
commit c6cb2e487b7cdb132629587d8da300e20e580c31
Author: Bivash Kumar Singh <bivashraj750@gmail.com>
Input: gunze: replace deprecated APIs and fix warning style
This commit replaces deprecated simple_strtoul() calls with kstrtoul() and
adds an early return on parsing failures. It also updates the printk warning
to use dev_warn() with the serio device.
> diff --git a/drivers/input/touchscreen/gunze.c b/drivers/input/touchscreen/gunze.c
> index 2baeb4f3b941c..38450cabdd113 100644
> --- a/drivers/input/touchscreen/gunze.c
> +++ b/drivers/input/touchscreen/gunze.c
> @@ -41,15 +41,19 @@ struct gunze {
> static void gunze_process_packet(struct gunze *gunze)
> {
[ ... ]
> - input_report_abs(dev, ABS_X, simple_strtoul(gunze->data + 1, NULL, 10));
> - input_report_abs(dev, ABS_Y, 1024 - simple_strtoul(gunze->data + 6, NULL, 10));
> + if (kstrtoul(gunze->data + 1, 10, &x) || kstrtoul(gunze->data + 6, 10, &y))
> + return;
[Severity: High]
Does this replacement break packet parsing by unconditionally returning an
error?
The kstrtoul() API strictly requires the parsed string to be NUL-terminated
and contain no trailing non-digit characters other than an optional newline.
Because the input data is a comma-separated string like "T1234,5678" and the
buffer is not NUL-terminated, kstrtoul() will encounter the comma when parsing
the X coordinate and return -EINVAL. It will also fail when parsing the Y
coordinate due to the missing NUL terminator.
This early return will unconditionally trigger on every valid touch event,
causing all packets to be dropped and rendering the touchscreen unusable.
> +
> + input_report_abs(dev, ABS_X, x);
> + input_report_abs(dev, ABS_Y, 1024 - y);
> input_report_key(dev, BTN_TOUCH, gunze->data[0] == 'T');
> input_sync(dev);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260718190040.10613-1-bivashraj750@gmail.com?part=1
^ permalink raw reply
* [PATCH] Input: gunze: replace deprecated APIs and fix warning style
From: Bivash Kumar Singh @ 2026-07-18 19:00 UTC (permalink / raw)
To: linux-input; +Cc: dmitry.torokhov, Bivash Kumar Singh
Replace two uses of simple_strtoul() with kstrtoul() as the former
is deprecated. kstrtoul() also properly handles parse errors, so
add an early return if parsing fails.
Replace printk(KERN_WARNING) with dev_warn() using the serio device,
which is the correct logging style for driver code and removes the
redundant 'gunze.c:' filename prefix.
Signed-off-by: Bivash Kumar Singh <bivashraj750@gmail.com>
---
drivers/input/touchscreen/gunze.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/input/touchscreen/gunze.c b/drivers/input/touchscreen/gunze.c
index 2baeb4f3b941..38450cabdd11 100644
--- a/drivers/input/touchscreen/gunze.c
+++ b/drivers/input/touchscreen/gunze.c
@@ -41,15 +41,19 @@ struct gunze {
static void gunze_process_packet(struct gunze *gunze)
{
struct input_dev *dev = gunze->dev;
+ unsigned long x, y;
if (gunze->idx != GUNZE_MAX_LENGTH || gunze->data[5] != ',' ||
(gunze->data[0] != 'T' && gunze->data[0] != 'R')) {
- printk(KERN_WARNING "gunze.c: bad packet: >%.*s<\n", GUNZE_MAX_LENGTH, gunze->data);
+ dev_warn(&gunze->serio->dev, "bad packet: >%.*s<\n", GUNZE_MAX_LENGTH, gunze->data);
return;
}
- input_report_abs(dev, ABS_X, simple_strtoul(gunze->data + 1, NULL, 10));
- input_report_abs(dev, ABS_Y, 1024 - simple_strtoul(gunze->data + 6, NULL, 10));
+ if (kstrtoul(gunze->data + 1, 10, &x) || kstrtoul(gunze->data + 6, 10, &y))
+ return;
+
+ input_report_abs(dev, ABS_X, x);
+ input_report_abs(dev, ABS_Y, 1024 - y);
input_report_key(dev, BTN_TOUCH, gunze->data[0] == 'T');
input_sync(dev);
}
--
2.53.0
^ permalink raw reply related
* [PATCH] HID: apple: fix endianness of keyboard backlight reports
From: Andre Eikmeyer @ 2026-07-18 15:12 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-input, Paul Pawlowski, Aun-Ali Zaidi, Aditya Garg,
Andre Eikmeyer, Sashiko AI review
The raw reports used by the T2 butterfly keyboard backlight contain
little-endian 16-bit fields. They are currently read and written as native
u16 values, leaving the wire format dependent on host endianness.
We should describe these fields as __le16 and convert them explicitly when
reading the backlight limits and writing the brightness and transition
rate.
Reported-by: Sashiko AI review <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/r/20260718123641.DA2FF1F000E9@smtp.kernel.org
Signed-off-by: Andre Eikmeyer <dev@deq.rocks>
---
drivers/hid/hid-apple.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index bf7dd0fbf249..473b6722938e 100644
--- a/drivers/hid/hid-apple.c
+++ b/drivers/hid/hid-apple.c
@@ -94,14 +94,16 @@ struct apple_sc_backlight {
struct apple_backlight_config_report {
u8 report_id;
u8 version;
- u16 backlight_off, backlight_on_min, backlight_on_max;
+ __le16 backlight_off;
+ __le16 backlight_on_min;
+ __le16 backlight_on_max;
};
struct apple_backlight_set_report {
u8 report_id;
u8 version;
- u16 backlight;
- u16 rate;
+ __le16 backlight;
+ __le16 rate;
};
struct apple_magic_backlight {
@@ -803,8 +805,8 @@ static int apple_backlight_set(struct hid_device *hdev, u16 value, u16 rate)
rep->report_id = 0xB0;
rep->version = 1;
- rep->backlight = value;
- rep->rate = rate;
+ rep->backlight = cpu_to_le16(value);
+ rep->rate = cpu_to_le16(rate);
ret = hid_hw_raw_request(hdev, 0xB0u, (u8 *) rep, sizeof(*rep),
HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
@@ -848,7 +850,9 @@ static int apple_backlight_init(struct hid_device *hdev)
}
hid_dbg(hdev, "backlight config: off=%u, on_min=%u, on_max=%u\n",
- rep->backlight_off, rep->backlight_on_min, rep->backlight_on_max);
+ le16_to_cpu(rep->backlight_off),
+ le16_to_cpu(rep->backlight_on_min),
+ le16_to_cpu(rep->backlight_on_max));
asc->backlight = devm_kzalloc(&hdev->dev, sizeof(*asc->backlight), GFP_KERNEL);
if (!asc->backlight) {
@@ -858,7 +862,7 @@ static int apple_backlight_init(struct hid_device *hdev)
asc->backlight->hdev = hdev;
asc->backlight->cdev.name = "apple::kbd_backlight";
- asc->backlight->cdev.max_brightness = rep->backlight_on_max;
+ asc->backlight->cdev.max_brightness = le16_to_cpu(rep->backlight_on_max);
asc->backlight->cdev.brightness_set_blocking = apple_backlight_led_set;
asc->backlight->cdev.flags = LED_CORE_SUSPENDRESUME;
--
2.55.0
^ permalink raw reply related
* Re: [PATCH 2/2] HID: apple: use the standard keyboard backlight LED name
From: Andre Eikmeyer @ 2026-07-18 15:00 UTC (permalink / raw)
To: sashiko-reviews
Cc: Andre Eikmeyer, linux-input, jikos, bentiss, gargaditya08,
dmitry.torokhov
In-Reply-To: <20260718123641.DA2FF1F000E9@smtp.kernel.org>
Thanks. This is a valid pre-existing issue.
The multi-byte fields in these raw HID reports are little-endian. The
backlight configuration values should be converted with le16_to_cpu(), and
the brightness and transition rate should be written with cpu_to_le16().
This predates the LED naming change, so I will address it in a separate
patch rather than mix it into this one.
^ permalink raw reply
* Re: [PATCH 1/2] HID: apple: preserve keyboard backlight across T2 resume
From: Andre Eikmeyer @ 2026-07-18 15:00 UTC (permalink / raw)
To: sashiko-reviews
Cc: Andre Eikmeyer, linux-input, jikos, bentiss, gargaditya08,
dmitry.torokhov
In-Reply-To: <20260718123558.0ABF41F000E9@smtp.kernel.org>
Thanks for pointing this out.
LED_CORE_SUSPENDRESUME was needed with the older apple-bce suspend
implementation for which the original fix was tested. I maintain the
apple-bce source consumed by T2Linux as well as its split replacement,
t2bce.
Both current implementations now suspend the VHCI deterministically and
re-enumerate its devices after resume. The transport takes the keyboard
device offline during suspend, so its backlight does not remain powered
when LED_CORE_SUSPENDRESUME is removed. On resume, however, the flag makes
the LED core issue a synchronous request through the stale HID device
before re-enumeration completes, resulting in -ENODEV.
The new behavior was tested on a MacBookPro15,1 with a butterfly keyboard
and a MacBookAir9,1 with a Magic Keyboard. In both cases the backlight is
off during suspend and restored correctly after resume.
^ permalink raw reply
* Re: [PATCH] Input: novatek-nvt-ts - support being panel follower
From: sashiko-bot @ 2026-07-18 14:03 UTC (permalink / raw)
To: Gianluca Boiano; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260718135316.785467-1-morf3089@gmail.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Unconditional dereference of `data->input` in `on_novatek_panel_unpreparing` causes a kernel crash if initialization failed.
- [High] Devres ordering inversion causes a Use-After-Free of the input device and invalid IRQ access during driver unbind.
- [High] Late `devm_*` allocations in a retryable panel callback cause permanent `-EBUSY` errors on initialization retries.
- [High] Independent userspace `open()` defeats the panel follower power sequencing, unconditionally powering the touchscreen.
- [Medium] Premature assignment of `data->input` without rollback on error leaves a dangling pointer that breaks initialization retries.
--
--- Patch [1]: [PATCH] Input: novatek-nvt-ts - support being panel follower ---
commit 13c26acf74fd7e860a0a8efe083b34d9bd7e6937
Author: Alexey Minnekhanov <alexeymin@minlexx.ru>
Input: novatek-nvt-ts - support being panel follower
This commit adds support for the novatek-nvt-ts driver to operate in panel
follower mode, where the touchscreen and panel are powered together in sync.
It splits the input device registration out of the probe function, deferring
it until the panel prepares via callbacks.
> diff --git a/drivers/input/touchscreen/novatek-nvt-ts.c b/drivers/input/touchscreen/novatek-nvt-ts.c
> index 0f771f6819528..8b290f75a8e48 100644
> --- a/drivers/input/touchscreen/novatek-nvt-ts.c
> +++ b/drivers/input/touchscreen/novatek-nvt-ts.c
[ ... ]
> @@ -145,6 +156,9 @@ static int nvt_ts_start(struct input_dev *dev)
> struct nvt_ts_data *data = input_get_drvdata(dev);
> int error;
>
> + if (data->running)
> + return 0;
> +
> error = regulator_bulk_enable(ARRAY_SIZE(data->regulators), data->regulators);
[Severity: High]
Does this bypass the panel follower power sequencing if userspace opens the
device independently?
Once the panel is prepared and the input device is registered, userspace can
open the input node (e.g. /dev/input/eventX) even if the panel has since been
powered off.
When that happens, input_dev->open() calls nvt_ts_start(), which will
unconditionally enable the regulators and IRQ without checking if
data->is_panel_follower restricts power control.
Should this check if the panel is currently prepared before forcing the
hardware on?
[ ... ]
> @@ -292,10 +305,11 @@ static int nvt_ts_probe(struct i2c_client *client)
> data->input = input;
> input_set_drvdata(input, data);
>
> - error = devm_request_threaded_irq(dev, client->irq, NULL, nvt_ts_irq,
> + error = devm_request_threaded_irq(dev, data->client->irq, NULL,
> + nvt_ts_irq,
> IRQF_ONESHOT | IRQF_NO_AUTOEN |
> nvt_ts_irq_type[irq_type],
> - client->name, data);
> + data->client->name, data);
> if (error) {
> dev_err(dev, "failed to request irq: %d\n", error);
> return error;
> }
[Severity: Medium]
Does an error here leave data->input as a dangling pointer?
Because data->input = input was already assigned above, if
devm_request_threaded_irq() or input_register_device() fails, the function
returns an error but leaves data->input populated.
When the panel retries initialization in on_novatek_panel_prepared(), it checks
if (!data->input). Since it's no longer NULL, it will skip initialization and
attempt to interact with an unregistered or broken input device.
Should data->input be set back to NULL in these error paths, or assigned
only after everything succeeds?
[ ... ]
> @@ -310,6 +324,76 @@ static int nvt_ts_probe(struct i2c_client *client)
> return 0;
> }
>
> +static int on_novatek_panel_prepared(struct drm_panel_follower *follower)
> +{
> + struct nvt_ts_data *data = container_of(follower, struct nvt_ts_data, panel_follower);
> + int ret;
> +
> + /* Is this the first power on? */
> + if (!data->input) {
> + ret = nvt_ts_initial_power_on_and_register_inputdev(data);
> + if (ret)
> + return ret;
[Severity: High]
Does calling devm allocation functions from a retryable callback cause permanent
errors on failure?
If nvt_ts_initial_power_on_and_register_inputdev() fails partway through
(e.g., an I2C timeout), it will return an error, but any devm resources it
already acquired (like devm_gpiod_get) will remain attached to the device.
When the panel power-on is retried later, it will execute again and attempt
to acquire the same GPIO and regulators, which will fail with -EBUSY.
Should these devm allocations be moved back to the probe function, or cleaned
up manually on error?
> + }
> +
> + guard(mutex)(&data->input->mutex);
> +
> + if (input_device_enabled(data->input))
> + nvt_ts_start(data->input);
> +
> + return 0;
> +}
> +
> +static int on_novatek_panel_unpreparing(struct drm_panel_follower *follower)
> +{
> + struct nvt_ts_data *data = container_of(follower, struct nvt_ts_data, panel_follower);
> +
> + guard(mutex)(&data->input->mutex);
[Severity: Critical]
Is it possible for data->input to be NULL here?
If nvt_ts_initial_power_on_and_register_inputdev() fails during
on_novatek_panel_prepared() (for instance, due to an I2C error), data->input
will remain NULL.
Since the follower is still registered, the DRM framework will eventually call
on_novatek_panel_unpreparing(), and this guard(mutex) will dereference the NULL
pointer.
Should this check if data->input is valid before attempting to lock its mutex?
[Severity: High]
Could the devm lifecycle ordering cause a use-after-free here during unbind?
When nvt_ts_probe() calls devm_drm_panel_add_follower(), it registers the
follower cleanup action. Later, on_novatek_panel_prepared() calls
nvt_ts_initial_power_on_and_register_inputdev(), which allocates the input
device via devm_input_allocate_device() and the IRQ via
devm_request_threaded_irq().
Because devres cleans up in LIFO order, on unbind:
1. The IRQ and input device are freed first.
2. The follower is removed, which invokes on_novatek_panel_unpreparing().
This means on_novatek_panel_unpreparing() will try to lock the mutex of an
already freed input device, and nvt_ts_stop() will attempt to disable an
already freed IRQ.
> +
> + if (input_device_enabled(data->input))
> + nvt_ts_stop(data->input);
> +
> + return 0;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260718135316.785467-1-morf3089@gmail.com?part=1
^ permalink raw reply
* [PATCH] Input: novatek-nvt-ts - support being panel follower
From: Gianluca Boiano @ 2026-07-18 13:53 UTC (permalink / raw)
To: hansg, dmitry.torokhov
Cc: alexeymin, linux-input, linux-kernel, Gianluca Boiano
From: Alexey Minnekhanov <alexeymin@minlexx.ru>
Sometimes Novatek touchscreen is paired together with Novatek panel,
and they both need to be powered together in sync.
Add support for nvt-ts driver to operate in panel follower mode, in
which touchscreen isn't in full control of its lifecycle. This is
managed using callbacks from panel, to get informed when the panel is
powered on and off. From there we can match the nvt-ts device's power
state to that of the panel.
Without this probing touchscreen before panel sometimes randomly fails
with I2C read errors.
Split some code from probe function to a separate
initial_power_on_and_register_inputdev function, which is called once
from panel_prepared callback, or from probe function, depending on
mode.
No Kconfig dependency on DRM is needed: drm_panel.h provides inline
stubs when CONFIG_DRM_PANEL is off, so the driver remains usable as a
standalone I2C touchscreen without DRM.
In panel-follower mode, nvt_ts_stop() can be called twice for a single
nvt_ts_start(): first from on_novatek_panel_unpreparing() when the
panel powers off, and again from input_close_device() when userspace
(e.g. systemd) closes the evdev file descriptor. The symmetric problem
exists for nvt_ts_start() on panel re-prepare. Guard both with a
'running' flag to make them idempotent. The flag is safe without
additional locking as all call paths already hold input->mutex.
Signed-off-by: Alexey Minnekhanov <alexeymin@minlexx.ru>
Co-developed-by: Gianluca Boiano <morf3089@gmail.com>
Signed-off-by: Gianluca Boiano <morf3089@gmail.com>
---
drivers/input/touchscreen/novatek-nvt-ts.c | 122 +++++++++++++++++----
1 file changed, 103 insertions(+), 19 deletions(-)
diff --git a/drivers/input/touchscreen/novatek-nvt-ts.c b/drivers/input/touchscreen/novatek-nvt-ts.c
index 0f771f681952..8b290f75a8e4 100644
--- a/drivers/input/touchscreen/novatek-nvt-ts.c
+++ b/drivers/input/touchscreen/novatek-nvt-ts.c
@@ -6,6 +6,7 @@
* Copyright (c) 2023 Hans de Goede <hdegoede@redhat.com>
*/
+#include <drm/drm_panel.h>
#include <linux/delay.h>
#include <linux/gpio/consumer.h>
#include <linux/interrupt.h>
@@ -59,6 +60,13 @@ struct nvt_ts_data {
struct touchscreen_properties prop;
int max_touches;
u8 buf[NVT_TS_TOUCH_SIZE * NVT_TS_MAX_TOUCHES];
+ /*
+ * Sometimes Novatek touchscreen is paired together with Novatek panel,
+ * and they need to be powered together in sync.
+ */
+ struct drm_panel_follower panel_follower;
+ bool is_panel_follower;
+ bool running;
};
static int nvt_ts_read_data(struct i2c_client *client, u8 reg, u8 *data, int count)
@@ -95,6 +103,9 @@ static irqreturn_t nvt_ts_irq(int irq, void *dev_id)
bool active;
u8 *touch;
+ if (!data->input)
+ return IRQ_HANDLED;
+
error = nvt_ts_read_data(data->client, NVT_TS_TOUCH_START, data->buf,
data->max_touches * NVT_TS_TOUCH_SIZE);
if (error)
@@ -145,6 +156,9 @@ static int nvt_ts_start(struct input_dev *dev)
struct nvt_ts_data *data = input_get_drvdata(dev);
int error;
+ if (data->running)
+ return 0;
+
error = regulator_bulk_enable(ARRAY_SIZE(data->regulators), data->regulators);
if (error) {
dev_err(&data->client->dev, "failed to enable regulators\n");
@@ -154,6 +168,7 @@ static int nvt_ts_start(struct input_dev *dev)
enable_irq(data->client->irq);
gpiod_set_value_cansleep(data->reset_gpio, 0);
+ data->running = true;
return 0;
}
@@ -161,15 +176,23 @@ static void nvt_ts_stop(struct input_dev *dev)
{
struct nvt_ts_data *data = input_get_drvdata(dev);
+ if (!data->running)
+ return;
+
disable_irq(data->client->irq);
gpiod_set_value_cansleep(data->reset_gpio, 1);
regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
+
+ data->running = false;
}
static int nvt_ts_suspend(struct device *dev)
{
struct nvt_ts_data *data = i2c_get_clientdata(to_i2c_client(dev));
+ if (data->is_panel_follower)
+ return 0;
+
guard(mutex)(&data->input->mutex);
if (input_device_enabled(data->input))
@@ -182,6 +205,9 @@ static int nvt_ts_resume(struct device *dev)
{
struct nvt_ts_data *data = i2c_get_clientdata(to_i2c_client(dev));
+ if (data->is_panel_follower)
+ return 0;
+
guard(mutex)(&data->input->mutex);
if (input_device_enabled(data->input))
@@ -192,30 +218,17 @@ static int nvt_ts_resume(struct device *dev)
static DEFINE_SIMPLE_DEV_PM_OPS(nvt_ts_pm_ops, nvt_ts_suspend, nvt_ts_resume);
-static int nvt_ts_probe(struct i2c_client *client)
+static int nvt_ts_initial_power_on_and_register_inputdev(struct nvt_ts_data *data)
{
- struct device *dev = &client->dev;
+ struct device *dev = &data->client->dev;
int error, width, height, irq_type;
- struct nvt_ts_data *data;
const struct nvt_ts_i2c_chip_data *chip;
struct input_dev *input;
- if (!client->irq) {
- dev_err(dev, "Error no irq specified\n");
- return -EINVAL;
- }
-
- data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
- if (!data)
- return -ENOMEM;
-
- chip = device_get_match_data(&client->dev);
+ chip = device_get_match_data(dev);
if (!chip)
return -EINVAL;
- data->client = client;
- i2c_set_clientdata(client, data);
-
/*
* VCC is the analog voltage supply
* IOVCC is the digital voltage supply
@@ -275,7 +288,7 @@ static int nvt_ts_probe(struct i2c_client *client)
if (!input)
return -ENOMEM;
- input->name = client->name;
+ input->name = data->client->name;
input->id.bustype = BUS_I2C;
input->open = nvt_ts_start;
input->close = nvt_ts_stop;
@@ -292,10 +305,11 @@ static int nvt_ts_probe(struct i2c_client *client)
data->input = input;
input_set_drvdata(input, data);
- error = devm_request_threaded_irq(dev, client->irq, NULL, nvt_ts_irq,
+ error = devm_request_threaded_irq(dev, data->client->irq, NULL,
+ nvt_ts_irq,
IRQF_ONESHOT | IRQF_NO_AUTOEN |
nvt_ts_irq_type[irq_type],
- client->name, data);
+ data->client->name, data);
if (error) {
dev_err(dev, "failed to request irq: %d\n", error);
return error;
@@ -310,6 +324,76 @@ static int nvt_ts_probe(struct i2c_client *client)
return 0;
}
+static int on_novatek_panel_prepared(struct drm_panel_follower *follower)
+{
+ struct nvt_ts_data *data = container_of(follower, struct nvt_ts_data, panel_follower);
+ int ret;
+
+ /* Is this the first power on? */
+ if (!data->input) {
+ ret = nvt_ts_initial_power_on_and_register_inputdev(data);
+ if (ret)
+ return ret;
+ }
+
+ guard(mutex)(&data->input->mutex);
+
+ if (input_device_enabled(data->input))
+ nvt_ts_start(data->input);
+
+ return 0;
+}
+
+static int on_novatek_panel_unpreparing(struct drm_panel_follower *follower)
+{
+ struct nvt_ts_data *data = container_of(follower, struct nvt_ts_data, panel_follower);
+
+ guard(mutex)(&data->input->mutex);
+
+ if (input_device_enabled(data->input))
+ nvt_ts_stop(data->input);
+
+ return 0;
+}
+
+static const struct drm_panel_follower_funcs nvt_ts_follower_funcs = {
+ .panel_prepared = on_novatek_panel_prepared,
+ .panel_unpreparing = on_novatek_panel_unpreparing,
+};
+
+static int nvt_ts_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct nvt_ts_data *data;
+
+ if (!client->irq) {
+ dev_err(dev, "Error no irq specified\n");
+ return -EINVAL;
+ }
+
+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->client = client;
+ i2c_set_clientdata(client, data);
+
+ /* Check if "panel = <&...>" is set in DT */
+ if (drm_is_panel_follower(dev)) {
+ data->is_panel_follower = true;
+ data->panel_follower.funcs = &nvt_ts_follower_funcs;
+ /*
+ * In this mode, we defer everything to the panel callbacks.
+ * The initial power on and input device registration will
+ * happen in on_novatek_panel_prepared().
+ */
+ return devm_drm_panel_add_follower(dev, &data->panel_follower);
+ }
+
+ return nvt_ts_initial_power_on_and_register_inputdev(data);
+}
+
+
static const struct nvt_ts_i2c_chip_data nvt_nt11205_ts_data = {
.chip_id = 0x05,
};
--
2.55.0
^ permalink raw reply related
* Re: [PATCH 2/2] HID: apple: use the standard keyboard backlight LED name
From: sashiko-bot @ 2026-07-18 12:36 UTC (permalink / raw)
To: Andre Eikmeyer; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260718121527.15924-2-dev@deq.rocks>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Missing endianness conversion for USB HID report u16 fields.
--
--- Patch [2]: [PATCH 2/2] HID: apple: use the standard keyboard backlight LED name ---
commit fe5b7459cd5f35bce672b51255250e2d12f88cf4
Author: Andre Eikmeyer <dev@deq.rocks>
HID: apple: use the standard keyboard backlight LED name
This commit changes the exposed LED name for the T2-attached butterfly
keyboard backlight from apple::kbd_backlight to the standard
:white:kbd_backlight naming convention. This allows userspace to discover
and handle both the butterfly models and the Magic Keyboards using the same
name without special casing.
> diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
> --- a/drivers/hid/hid-apple.c
> +++ b/drivers/hid/hid-apple.c
> @@ -860,8 +860,8 @@ static int apple_backlight_init(struct hid_device *hdev)
> }
>
> asc->backlight->hdev = hdev;
> - asc->backlight->cdev.name = "apple::kbd_backlight";
> + asc->backlight->cdev.name = ":white:" LED_FUNCTION_KBD_BACKLIGHT;
> asc->backlight->cdev.max_brightness = rep->backlight_on_max;
[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but does
this code lack endianness conversion for the USB HID report fields?
According to the USB HID standard, multi-byte fields are little-endian.
Since rep->backlight_on_max is a u16 read directly from the raw device
report in apple_backlight_init(), could this result in byte-swapped values
on big-endian architectures and cause an out-of-bounds max_brightness?
A similar issue seems to be present in apple_backlight_set() where native
u16 values for rep->backlight and rep->rate are written into the report
struct without CPU to little-endian conversion before transmission.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260718121527.15924-1-dev@deq.rocks?part=2
^ permalink raw reply
* Re: [PATCH 1/2] HID: apple: preserve keyboard backlight across T2 resume
From: sashiko-bot @ 2026-07-18 12:35 UTC (permalink / raw)
To: Andre Eikmeyer; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260718121527.15924-1-dev@deq.rocks>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removing the `LED_CORE_SUSPENDRESUME` flag reverts a previous power-management fix, causing the keyboard backlight to remain powered on during system suspend.
--
--- Patch [1]: [PATCH 1/2] HID: apple: preserve keyboard backlight across T2 resume ---
commit 2758811c67b214fbd3316fec3f21127f8166f490
Author: Andre Eikmeyer <dev@deq.rocks>
HID: apple: preserve keyboard backlight across T2 resume
This commit fixes an issue where the T2 virtual USB host controller
re-enumerates the internal keyboard on resume, resulting in the backlight
starting off. It caches the brightness during removal and restores it
during the subsequent probe.
Fixes: 1f95a6cd5ad7 ("HID: apple: ensure the keyboard backlight is off if suspending")
Cc: stable@vger.kernel.org
> diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
> --- a/drivers/hid/hid-apple.c
> +++ b/drivers/hid/hid-apple.c
[ ... ]
> @@ -860,13 +864,20 @@ static int apple_backlight_init(struct hid_device *hdev)
> asc->backlight->cdev.name = "apple::kbd_backlight";
> asc->backlight->cdev.max_brightness = rep->backlight_on_max;
> asc->backlight->cdev.brightness_set_blocking = apple_backlight_led_set;
> - asc->backlight->cdev.flags = LED_CORE_SUSPENDRESUME;
> + /* VHCI re-enumeration restores the cached brightness in the next probe. */
[Severity: High]
Does removing the LED_CORE_SUSPENDRESUME flag cause the backlight to remain
on during system suspend?
The T2 butterfly keyboard hardware does not automatically power off its
backlight when suspending. When this flag is removed, the LED subsystem
bypasses led_classdev_suspend(), leaving the hardware illuminated and
draining the battery.
The new caching mechanism in apple_remove() successfully tracks the user
brightness preference, as led_classdev_suspend() only alters the physical
hardware state without modifying the logical led_cdev->brightness.
Could the device initialization issue have been caused by the hardcoded
apple_backlight_set(hdev, 0, 0) during probe, rather than the presence of
this suspend flag?
>
> - ret = apple_backlight_set(hdev, 0, 0);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260718121527.15924-1-dev@deq.rocks?part=1
^ permalink raw reply
* [PATCH] Input: i8042 - add Xiaomi Book Pro 14 to dumbkbd quirk list
From: Andrew Zhou @ 2026-07-18 12:24 UTC (permalink / raw)
To: linux-input; +Cc: linux-kernel, dmitry.torokhov, Andrew Zhou
The Xiaomi Book Pro 14's built-in keyboard doesn't work by default.
Add the device to the quirk table with SERIO_QUIRK_DUMBKBD so the keyboard
works out of the box without needing i8042.dumbkbd on the kernel command line.
Tested on a Xiaomi Book Pro 14.
Signed-off-by: Andrew Zhou <zhoulol888@gmail.com>
---
drivers/input/serio/i8042-acpipnpio.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/input/serio/i8042-acpipnpio.h b/drivers/input/serio/i8042-acpipnpio.h
index 412f82d7a..6b18014e2 100644
--- a/drivers/input/serio/i8042-acpipnpio.h
+++ b/drivers/input/serio/i8042-acpipnpio.h
@@ -1430,6 +1430,18 @@ static const struct dmi_system_id i8042_dmi_quirk_table[] __initconst = {
},
.driver_data = (void *)(SERIO_QUIRK_DIRECT)
},
+ /*
+ * Some laptops have a broken keyboard controller that reports
+ * incorrect data. SERIO_QUIRK_DUMBKBD bypasses the intelligent
+ * probing and forces a basic PS/2 keyboard assumption.
+ */
+ {
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "XIAOMI"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Xiaomi Book Pro 14"),
+ },
+ .driver_data = (void *)(SERIO_QUIRK_DUMBKBD)
+ },
{ }
};
--
2.54.0
^ permalink raw reply related
* [PATCH 1/2] HID: apple: preserve keyboard backlight across T2 resume
From: Andre Eikmeyer @ 2026-07-18 12:15 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-input, Aditya Garg, Andre Eikmeyer, stable
The T2 virtual USB host controller re-enumerates the internal keyboard
after system resume. The butterfly keyboard backlight currently uses
LED_CORE_SUSPENDRESUME, so the LED core sends a blocking request to the
old HID device while it is disappearing. That request fails with -ENODEV
and the newly probed device starts with its backlight off.
To fix this, we cache the requested brightness when the old HID device is
removed and restore it when the replacement is probed. We let
re-enumeration handle restoration instead of issuing a request through the
stale device.
Fixes: 1f95a6cd5ad7 ("HID: apple: ensure the keyboard backlight is off if suspending")
Cc: stable@vger.kernel.org
Signed-off-by: Andre Eikmeyer <dev@deq.rocks>
---
drivers/hid/hid-apple.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index bf7dd0fbf249..a5f232cc2b66 100644
--- a/drivers/hid/hid-apple.c
+++ b/drivers/hid/hid-apple.c
@@ -91,6 +91,9 @@ struct apple_sc_backlight {
struct hid_device *hdev;
};
+/* T2 VHCI re-enumerates the internal keyboard across system resume. */
+static int apple_backlight_resume_brightness = -1;
+
struct apple_backlight_config_report {
u8 report_id;
u8 version;
@@ -825,6 +828,7 @@ static int apple_backlight_led_set(struct led_classdev *led_cdev,
static int apple_backlight_init(struct hid_device *hdev)
{
int ret;
+ int brightness;
struct apple_sc *asc = hid_get_drvdata(hdev);
struct apple_backlight_config_report *rep;
@@ -860,13 +864,20 @@ static int apple_backlight_init(struct hid_device *hdev)
asc->backlight->cdev.name = "apple::kbd_backlight";
asc->backlight->cdev.max_brightness = rep->backlight_on_max;
asc->backlight->cdev.brightness_set_blocking = apple_backlight_led_set;
- asc->backlight->cdev.flags = LED_CORE_SUSPENDRESUME;
+ /* VHCI re-enumeration restores the cached brightness in the next probe. */
+
+ brightness = READ_ONCE(apple_backlight_resume_brightness);
+ if (brightness < 0)
+ brightness = LED_OFF;
+ else
+ brightness = min_t(int, brightness, rep->backlight_on_max);
- ret = apple_backlight_set(hdev, 0, 0);
+ ret = apple_backlight_set(hdev, brightness, 0);
if (ret < 0) {
hid_err(hdev, "backlight set request failed: %d\n", ret);
goto cleanup_and_exit;
}
+ asc->backlight->cdev.brightness = brightness;
ret = devm_led_classdev_register(&hdev->dev, &asc->backlight->cdev);
@@ -999,6 +1010,9 @@ static void apple_remove(struct hid_device *hdev)
if (asc->quirks & APPLE_RDESC_BATTERY)
timer_delete_sync(&asc->battery_timer);
+ if (asc->backlight)
+ WRITE_ONCE(apple_backlight_resume_brightness,
+ asc->backlight->cdev.brightness);
hid_hw_stop(hdev);
}
--
2.55.0
^ permalink raw reply related
* [PATCH 2/2] HID: apple: use the standard keyboard backlight LED name
From: Andre Eikmeyer @ 2026-07-18 12:15 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, Aditya Garg, Andre Eikmeyer
In-Reply-To: <20260718121527.15924-1-dev@deq.rocks>
The T2-attached butterfly keyboard backlight is exposed as
apple::kbd_backlight. This leaves the color field empty and gives
userspace a model-specific name for the same white keyboard-backlight
function exposed by Magic Keyboards.
Magic Keyboard backlight support was added later and already follows the
current LED naming convention. As a result, userspace has to handle two
different names for the same function.
We should use :white:kbd_backlight for both implementations. This follows
the LED color and function naming convention and lets userspace discover
either keyboard generation without a special case for the butterfly
models.
Signed-off-by: Andre Eikmeyer <dev@deq.rocks>
---
drivers/hid/hid-apple.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index a5f232cc2b66..0f30cf4670ce 100644
--- a/drivers/hid/hid-apple.c
+++ b/drivers/hid/hid-apple.c
@@ -861,7 +861,7 @@ static int apple_backlight_init(struct hid_device *hdev)
}
asc->backlight->hdev = hdev;
- asc->backlight->cdev.name = "apple::kbd_backlight";
+ asc->backlight->cdev.name = ":white:" LED_FUNCTION_KBD_BACKLIGHT;
asc->backlight->cdev.max_brightness = rep->backlight_on_max;
asc->backlight->cdev.brightness_set_blocking = apple_backlight_led_set;
/* VHCI re-enumeration restores the cached brightness in the next probe. */
--
2.55.0
^ permalink raw reply related
* Re: [PATCH] Input: cs40l50-vibra - validate custom data from user space
From: sashiko-bot @ 2026-07-18 7:54 UTC (permalink / raw)
To: HyeongJun An; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260718074032.1864861-1-sammiee5311@gmail.com>
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
Pre-existing issues:
- [High] Updating an existing OWT effect leaks DSP memory and desynchronizes indices.
- [High] Out-of-bounds slab read in cs40l50_upload_owt due to passing byte count instead of element count to regmap_bulk_write.
- [High] The driver allocates devm resources against the parent device rather than its own child device, leaking resources across unbind cycles.
- [High] Local DoS via unbounded synchronous sleeping in ordered workqueue during playback.
- [Medium] Unhandled odd custom_len values lead to truncated OWT data lengths.
- [Medium] Memory structures are permanently leaked if hardware cannot be resumed during worker execution.
--
--- Patch [1]: [PATCH] Input: cs40l50-vibra - validate custom data from user space ---
commit a72a6893fcb7d7f39ce7369bea556bb585afa8b7
Author: HyeongJun An <sammiee5311@gmail.com>
Input: cs40l50-vibra - validate custom data from user space
This commit adds validation for custom data lengths to prevent zero or one-byte
allocations. It also updates the bank value to a 32-bit unsigned integer to
avoid bypassing upper bounds checks with negative values.
> diff --git a/drivers/input/misc/cs40l50-vibra.c b/drivers/input/misc/cs40l50-vibra.c
> index 996d6c38cca43..7ef4534fea2fb 100644
> --- a/drivers/input/misc/cs40l50-vibra.c
> +++ b/drivers/input/misc/cs40l50-vibra.c
[ ... ]
> @@ -326,6 +326,12 @@ static int cs40l50_add(struct input_dev *dev, struct ff_effect *effect,
> return -EINVAL;
> }
>
> + if (periodic->custom_len < CS40L50_OWT_CUSTOM_DATA_SIZE) {
[Severity: Medium]
This is a pre-existing issue, but would this check ensure `custom_len` is even?
Since `custom_len` represents the number of 16-bit words, an odd value results
in a byte length that is not a multiple of 4. Later in `cs40l50_upload_owt()`:
cs40l50_upload_owt() {
...
header.data_words = len / sizeof(u32);
...
}
Could this integer division truncate the unaligned payload length before it is
sent to the DSP?
> + dev_err(vib->dev, "Invalid custom data length (%u)\n",
> + periodic->custom_len);
> + return -EINVAL;
> + }
> +
> work_data.custom_data = memdup_array_user(effect->u.periodic.custom_data,
[Severity: High]
This is a pre-existing issue, but is it possible that updating an existing OWT
effect leaks DSP memory and desynchronizes indices?
When an existing OWT effect is updated, it seems `cs40l50_add_worker()` pushes
a new waveform to the DSP without deleting the old one:
cs40l50_add_worker() {
...
effect = cs40l50_find_effect(work_data->effect->id, &vib->effect_head);
...
}
Might this leak DSP memory until it permanently errors with `-ENOSPC`?
Additionally, `cs40l50_effect_index_set()` computes the new index by counting
OWT elements in `vib->effect_head`:
cs40l50_effect_index_set() {
...
list_for_each_entry(owt_effect, &vib->effect_head, list)
if (owt_effect->type == CS40L50_WVFRM_BANK_OWT)
effect->index++;
...
}
Since `CS40L50_EFFECTS_MAX` is 1, this count is always 1 during an update.
However, the DSP increments its internal index for each push. Will this cause
the software index to permanently desynchronize from the DSP after multiple
updates, resulting in the wrong effect being played?
[Severity: High]
This is a pre-existing issue, but could there be an out-of-bounds read in
`cs40l50_upload_owt()`?
cs40l50_upload_owt() {
...
error = regmap_bulk_write(vib->regmap, vib->dsp.owt_base_reg +
(offset * sizeof(u32)), new_owt_effect_data,
sizeof(header) + len);
...
}
It appears `sizeof(header) + len` is passed as the `val_count` argument to
`regmap_bulk_write()`. Since the device's regmap configuration sets
`val_bits = 32`, `regmap_bulk_write()` interprets `val_count` as the number of
elements, not bytes.
Would this multiply the count by 4, reading 4x the allocated bytes from
`new_owt_effect_data` and sending arbitrary kernel memory to the DSP?
[Severity: Medium]
This is a pre-existing issue, but are memory structures leaked if the hardware
cannot be resumed?
In `cs40l50_stop_worker()`:
cs40l50_stop_worker() {
...
if (pm_runtime_resume_and_get(vib->dev) < 0)
return;
...
}
Would this early return bypass the `kfree(work_data)` call, leaking the
dynamically allocated work structure?
Similarly, in `cs40l50_erase_worker()`:
cs40l50_erase_worker() {
...
error = pm_runtime_resume_and_get(vib->dev);
if (error)
goto err_exit;
...
}
Can this failure path skip removing the effect from `vib->effect_head` and
`kfree(erase_effect)`, leaving the effect state permanently stranded?
[Severity: High]
This is a pre-existing issue, but are devm resources allocated against the wrong
device?
cs40l50_vibra_probe() {
...
vib = devm_kzalloc(pdev->dev.parent, sizeof(*vib), GFP_KERNEL);
...
}
Because `devm_kzalloc()`, `devm_input_allocate_device()`, and
`devm_add_action_or_reset()` are bound to `pdev->dev.parent` (the parent MFD
device), will these resources be leaked across unbind cycles when the child
driver is unbound via sysfs?
[Severity: High]
This is a pre-existing issue, but could there be an unbounded synchronous sleep
in the workqueue during playback?
When a playback event is triggered, `work_data->count` comes from the
user-controlled event `value`:
cs40l50_start_worker() {
...
while (--work_data->count >= 0) {
vib->dsp.write(vib->dev, vib->regmap, start_effect->index);
usleep_range(work_data->effect->replay.length,
work_data->effect->replay.length + 100);
}
...
}
Could a user pass `count = 0x7FFFFFFF`, causing the worker to sleep for up to
4.4 years?
Since `vib->vib_wq` is an ordered workqueue, would this completely block all
other work items?
If another process attempts to add, erase, or close the device descriptor, it
will call `flush_work()`:
cs40l50_add() {
...
flush_work(&work_data.work);
...
}
Will this cause the process to hang in `TASK_UNINTERRUPTIBLE` indefinitely if
the workqueue is blocked?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260718074032.1864861-1-sammiee5311@gmail.com?part=1
^ permalink raw reply
* [PATCH] Input: cs40l50-vibra - validate custom data from user space
From: HyeongJun An @ 2026-07-18 7:40 UTC (permalink / raw)
To: James Ogletree, Fred Treven, Ben Bright, Dmitry Torokhov
Cc: patches, linux-input, linux-kernel, stable, HyeongJun An
cs40l50_add() copies the custom data of an FF_PERIODIC/FF_CUSTOM effect
straight from the ff_effect the user passed to EVIOCSFF, without
requiring it to hold anything:
work_data.custom_data = memdup_array_user(periodic->custom_data,
periodic->custom_len,
sizeof(s16));
work_data.custom_len = periodic->custom_len;
The driver then reads two words out of that buffer: custom_data[0] as the
waveform bank in cs40l50_effect_bank_set(), and custom_data[1] as the
index within the bank in cs40l50_effect_index_set(). Neither read is
covered by a length check, and custom_len is fully user controlled:
- custom_len == 0 makes memdup_array_user() call memdup_user() with a
length of zero, which returns ZERO_SIZE_PTR rather than an error, so
custom_data[0] dereferences it.
- custom_len == 1 allocates two bytes. A bank of ROM or RAM keeps
effect->type out of the OWT case, and custom_data[1] is then read one
word past the allocation.
The bank value itself is also mishandled. It is masked with
CS40L50_CUSTOM_DATA_MASK (0xffff) but stored in an s16, so a
custom_data[0] of 0x8000 or above wraps to a negative value that passes
the "bank_type >= CS40L50_WVFRM_BANK_NUM" test.
cs40l50_effect_index_set() indexes vib->dsp.banks[] with it before the
switch statement's default case gets a chance to reject it:
base_index = vib->dsp.banks[effect->type].base_index;
max_index = vib->dsp.banks[effect->type].max_index;
Require the two words the driver reads to be present, and hold the masked
bank in a u32 so the existing upper-bound test covers the whole range.
The da7280 haptic driver already range checks custom_len this way.
Fixes: c38fe1bb5d21 ("Input: cs40l50 - Add support for the CS40L50 haptic driver")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: HyeongJun An <sammiee5311@gmail.com>
---
drivers/input/misc/cs40l50-vibra.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/input/misc/cs40l50-vibra.c b/drivers/input/misc/cs40l50-vibra.c
index 996d6c38cca4..7ef4534fea2f 100644
--- a/drivers/input/misc/cs40l50-vibra.c
+++ b/drivers/input/misc/cs40l50-vibra.c
@@ -139,10 +139,10 @@ static struct cs40l50_effect *cs40l50_find_effect(int id, struct list_head *effe
static int cs40l50_effect_bank_set(struct cs40l50_work *work_data,
struct cs40l50_effect *effect)
{
- s16 bank_type = work_data->custom_data[0] & CS40L50_CUSTOM_DATA_MASK;
+ u32 bank_type = work_data->custom_data[0] & CS40L50_CUSTOM_DATA_MASK;
if (bank_type >= CS40L50_WVFRM_BANK_NUM) {
- dev_err(work_data->vib->dev, "Invalid bank (%d)\n", bank_type);
+ dev_err(work_data->vib->dev, "Invalid bank (%u)\n", bank_type);
return -EINVAL;
}
@@ -326,6 +326,12 @@ static int cs40l50_add(struct input_dev *dev, struct ff_effect *effect,
return -EINVAL;
}
+ if (periodic->custom_len < CS40L50_OWT_CUSTOM_DATA_SIZE) {
+ dev_err(vib->dev, "Invalid custom data length (%u)\n",
+ periodic->custom_len);
+ return -EINVAL;
+ }
+
work_data.custom_data = memdup_array_user(effect->u.periodic.custom_data,
effect->u.periodic.custom_len,
sizeof(s16));
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 2/4] HID: corsair-void: fix out-of-bounds read in corsair_void_raw_event
From: Stuart Hayhurst @ 2026-07-17 22:55 UTC (permalink / raw)
To: Jiale Yao, Jiri Kosina, Benjamin Tissoires, linux-input,
linux-kernel, stuart.a.hayhurst
In-Reply-To: <20260714131611.269929-2-yaojiale02@163.com>
Hi,
Looking through this, I had a couple thoughts:
- It should send a warning for unexpected sizes, since most likely
it's a format we haven't seen yet
- 5 is a bit of a magic number, especially as some packets of other
sizes pass through, but we don't do anything with them (yet?)
- AFAIK returning 0 indicates everything was OK, when actually we
bailed out. By convention, shouldn't this return 1 on failure?
The patch I submitted a few weeks ago handled these:
https://lore.kernel.org/all/20260630010656.626157-3-stuart.a.hayhurst@gmail.com/
Stuart
^ permalink raw reply
* Re: [PATCH v4 0/3] input: misc: Add an initial driver for haptics inside Qcom PMIH010x PMIC
From: Jeff Johnson @ 2026-07-17 18:52 UTC (permalink / raw)
To: Fenglin Wu, linux-arm-msm, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Lee Jones, Stephen Boyd,
Bjorn Andersson, Konrad Dybcio
Cc: David Collins, Subbaraman Narayanamurthy, Kamal Wadhwa,
linux-input, devicetree, linux-kernel, Krzysztof Kozlowski
In-Reply-To: <20260717-qcom-spmi-haptics-v4-0-b0fe0ed30849@oss.qualcomm.com>
On 7/17/2026 12:28 AM, Fenglin Wu wrote:
> Dependencies:
> - [patch 2/3] depends on [patch 1/3] and they should be applied together
???
it is normal for later patches in a series to depend upon earlier patches, but
that doesn't normally mean they need to be applied together, just that the
later patch needs to be applied after the earlier patch, which is normal.
you'd only need them to be applied together if they have cross dependencies.
But then that would be bad because it would mean you can't bisect the changes.
So this dependencies statement doesn't really make any sense.
/jeff
^ permalink raw reply
* Re: [PATCH v4 1/2] dt-bindings: input: focaltech,ft8112: Add focaltech,ft3d81 compatible
From: Dmitry Torokhov @ 2026-07-17 16:54 UTC (permalink / raw)
To: Pradyot Kumar Nayak
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Daniel Peng,
Bjorn Andersson, Konrad Dybcio, Abel Vesa, Dmitry Baryshkov,
linux-arm-msm, linux-input, devicetree, linux-kernel,
Konrad Dybcio, Krzysztof Kozlowski
In-Reply-To: <20260717-add_focaltech_ft3d81_touchscreen_support-v4-1-5dd091e25801@oss.qualcomm.com>
On Fri, Jul 17, 2026 at 05:28:34PM +0530, Pradyot Kumar Nayak wrote:
> The Focaltech ft3d81 is fully compatible with the ft8112 i.e.
> it uses the same I2C-HID protocol and the same power-on/reset sequencing,
> DT nodes for boards carrying an ft3d81,can therefore bind to the existing
> ft8112 driver without any additional changes.
>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
> Signed-off-by: Pradyot Kumar Nayak <pradyot.nayak@oss.qualcomm.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v4 3/3] input: misc: Add Qualcomm SPMI PMIC haptics driver
From: Dmitry Torokhov @ 2026-07-17 16:51 UTC (permalink / raw)
To: Fenglin Wu
Cc: linux-arm-msm, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Lee Jones, Stephen Boyd, Bjorn Andersson, Konrad Dybcio,
David Collins, Subbaraman Narayanamurthy, Kamal Wadhwa, kernel,
linux-input, devicetree, linux-kernel
In-Reply-To: <20260717-qcom-spmi-haptics-v4-3-b0fe0ed30849@oss.qualcomm.com>
Hi Fenglin,
On Fri, Jul 17, 2026 at 12:29:01AM -0700, Fenglin Wu wrote:
> Add an initial driver for the Qualcomm PMIH0108 PMIC haptics module,
> named as HAP530_HV. This module supports several play modes, including
> DIRECT_PLAY, FIFO, PAT_MEM, and SWR, each with distinct data sourcing
> and hardware data handling logic. Currently, the driver provides support
> for two play modes using the input force-feedback framework: FF_CONSTANT
> effect for DIRECT_PLAY mode and FF_PERIODIC effect with FF_CUSTOM
> waveform for FIFO mode.
>
> Assisted-by: Claude:claude-4-8-opus
> Signed-off-by: Fenglin Wu <fenglin.wu@oss.qualcomm.com>
> ---
> drivers/input/misc/Kconfig | 11 +
> drivers/input/misc/Makefile | 1 +
> drivers/input/misc/qcom-spmi-haptics.c | 1178 ++++++++++++++++++++++++++++++++
> 3 files changed, 1190 insertions(+)
>
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 1f6c57dba030..4f40940973e4 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -236,6 +236,17 @@ config INPUT_PMIC8XXX_PWRKEY
> To compile this driver as a module, choose M here: the
> module will be called pmic8xxx-pwrkey.
>
> +config INPUT_QCOM_SPMI_HAPTICS
> + tristate "Qualcomm SPMI PMIC haptics support"
> + depends on MFD_SPMI_PMIC
> + help
> + Say Y to enable support for the Qualcomm PMIH0108 SPMI PMIC haptics
> + module. Supports DIRECT_PLAY, FIFO streaming play modes via the
> + Linux input force-feedback framework.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called qcom-spmi-haptics.
> +
> config INPUT_SPARCSPKR
> tristate "SPARC Speaker support"
> depends on PCI && SPARC64
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 2281d6803fce..c5c9aa139a11 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -69,6 +69,7 @@ obj-$(CONFIG_INPUT_PMIC8XXX_PWRKEY) += pmic8xxx-pwrkey.o
> obj-$(CONFIG_INPUT_POWERMATE) += powermate.o
> obj-$(CONFIG_INPUT_PWM_BEEPER) += pwm-beeper.o
> obj-$(CONFIG_INPUT_PWM_VIBRA) += pwm-vibra.o
> +obj-$(CONFIG_INPUT_QCOM_SPMI_HAPTICS) += qcom-spmi-haptics.o
> obj-$(CONFIG_INPUT_QNAP_MCU) += qnap-mcu-input.o
> obj-$(CONFIG_INPUT_RAVE_SP_PWRBUTTON) += rave-sp-pwrbutton.o
> obj-$(CONFIG_INPUT_RB532_BUTTON) += rb532_button.o
> diff --git a/drivers/input/misc/qcom-spmi-haptics.c b/drivers/input/misc/qcom-spmi-haptics.c
> new file mode 100644
> index 000000000000..235f77e0229c
> --- /dev/null
> +++ b/drivers/input/misc/qcom-spmi-haptics.c
> @@ -0,0 +1,1178 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/device.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/list.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/uaccess.h>
> +#include <linux/workqueue.h>
> +
> +/* HAP_CFG register offsets, bit fields, value constants */
> +#define HAP_CFG_INT_RT_STS_REG 0x10
> +#define FIFO_EMPTY_BIT BIT(1)
> +#define HAP_CFG_EN_CTL_REG 0x46
> +#define HAPTICS_EN_BIT BIT(7)
> +#define HAP_CFG_VMAX_REG 0x48
> +#define VMAX_STEP_MV 50
> +#define VMAX_MV_MAX 10000
> +#define HAP_CFG_SPMI_PLAY_REG 0x4C
> +#define PLAY_EN_BIT BIT(7)
> +#define PAT_SRC_MASK GENMASK(2, 0)
> +#define PAT_SRC_FIFO 0
> +#define PAT_SRC_DIRECT_PLAY 1
> +#define HAP_CFG_TLRA_OL_HIGH_REG 0x5C
> +#define TLRA_OL_MSB_MASK GENMASK(3, 0)
> +#define TLRA_STEP_US 5
> +#define TLRA_US_MAX 20475
> +#define HAP_CFG_TLRA_OL_LOW_REG 0x5D
> +#define HAP_CFG_DRV_DUTY_CFG_REG 0x60
> +#define ADT_DRV_DUTY_EN_BIT BIT(7)
> +#define ADT_BRK_DUTY_EN_BIT BIT(6)
> +#define DRV_DUTY_MASK GENMASK(5, 3)
> +#define AUTORES_DRV_DUTY_62P5 2
> +#define BRK_DUTY_MASK GENMASK(2, 0)
> +#define AUTORES_BRK_DUTY_62P5 5
> +#define HAP_CFG_ZX_WIND_CFG_REG 0x62
> +#define ZX_DEBOUNCE_MASK GENMASK(6, 4)
> +#define AUTORES_ZX_DEBOUNCE 3
> +#define ZX_WIN_HEIGHT_MASK GENMASK(2, 0)
> +#define AUTORES_ZX_WIN_HEIGHT 2
> +#define HAP_CFG_AUTORES_CFG_REG 0x63
> +#define AUTORES_EN_BIT BIT(7)
> +#define AUTORES_EN_DLY_MASK GENMASK(6, 2)
> +#define AUTORES_EN_DLY_CYCLES 10
> +#define AUTORES_ERR_WIN_MASK GENMASK(1, 0)
> +#define AUTORES_ERR_WIN_25PCT 1
> +#define HAP_CFG_FAULT_CLR_REG 0x66
> +#define ZX_TO_FAULT_CLR_BIT BIT(4)
> +#define SC_CLR_BIT BIT(2)
> +#define AUTO_RES_ERR_CLR_BIT BIT(1)
> +#define HPWR_RDY_FAULT_CLR_BIT BIT(0)
> +#define FAULT_CLR_ALL (ZX_TO_FAULT_CLR_BIT | SC_CLR_BIT | \
> + AUTO_RES_ERR_CLR_BIT | HPWR_RDY_FAULT_CLR_BIT)
> +#define HAP_CFG_RAMP_DN_CFG2_REG 0x86
> +#define AUTORES_PRE_HIZ_DLY_10US 1
> +
> +/* HAP_PTN register offsets, bit fields, value constants */
> +#define HAP_PTN_REVISION2_REG 0x01
> +#define HAP_PTN_FIFO_DIN_0_REG 0x20
> +#define HAP_PTN_FIFO_PLAY_RATE_REG 0x24
> +#define FIFO_PLAY_RATE_MASK GENMASK(3, 0)
> +#define HAP_PTN_DIRECT_PLAY_REG 0x26
> +#define HAP_PTN_FIFO_EMPTY_CFG_REG 0x2A
> +#define FIFO_THRESH_LSB 64
> +#define HAP_PTN_FIFO_DIN_1B_REG 0x2C
> +#define HAP_PTN_MEM_OP_ACCESS_REG 0x2D
> +#define MEM_FLUSH_RELOAD_BIT BIT(0)
> +#define HAP_PTN_MMAP_FIFO_REG 0xA0
> +#define MMAP_FIFO_EXIST_BIT BIT(7)
> +#define MMAP_FIFO_LEN_MASK GENMASK(6, 0)
> +#define HAP_PTN_PATX_PLAY_CFG_REG 0xA2
> +
> +#define HAP530_MEM_TOTAL_BYTES 8192
> +#define FIFO_EMPTY_THRESH 280
> +#define FIFO_INIT_FILL 320
> +
> +#define HAPTICS_AUTOSUSPEND_MS 1000
> +
> +/*
> + * FF_CUSTOM data layout (custom_data[] of type s16):
> + * [0] = play rate (PLAY_RATE_*)
> + * [1] = vmax in mV (0 = use device default from qcom,vmax-microvolt)
> + * [2..N-1] = signed 8-bit PCM samples packed one per s16 (lower byte used)
> + */
> +#define CUSTOM_DATA_RATE_IDX 0
> +#define CUSTOM_DATA_VMAX_IDX 1
> +#define CUSTOM_DATA_SAMPLE_START 2
> +#define CUSTOM_DATA_MAX_LEN (48 * 1024)
> +
> +#define HAPTICS_MAX_EFFECTS 8
> +
> +enum qcom_haptics_mode {
> + HAPTICS_MODE_NONE,
> + HAPTICS_DIRECT_PLAY,
> + HAPTICS_FIFO,
> +};
> +
> +enum qcom_haptics_play_rate {
> + PLAY_RATE_T_LRA = 0,
> + PLAY_RATE_T_LRA_DIV_2 = 1,
> + PLAY_RATE_T_LRA_DIV_4 = 2,
> + PLAY_RATE_T_LRA_DIV_8 = 3,
> + PLAY_RATE_T_LRA_X_2 = 4,
> + PLAY_RATE_T_LRA_X_4 = 5,
> + PLAY_RATE_T_LRA_X_8 = 6,
> + PLAY_RATE_RESERVED = 7,
> + PLAY_RATE_F_8KHZ = 8,
> + PLAY_RATE_F_16KHZ = 9,
> + PLAY_RATE_F_24KHZ = 10,
> + PLAY_RATE_F_32KHZ = 11,
> + PLAY_RATE_F_44P1KHZ = 12,
> + PLAY_RATE_F_48KHZ = 13,
> + PLAY_RATE_MAX = PLAY_RATE_F_48KHZ,
> +};
> +
> +/**
> + * struct qcom_haptics_effect: A haptics effect
> + * @mode: haptics HW play mode
> + * @vmax_mv: peak voltage of the haptics output waveform
> + * @length_us: vibration play duration
> + * @amplitude: DIRECT_PLAY mode output waveform amplitude
> + * @play_rate: FIFO mode play rate
> + * @fifo_data: 8-bit data samples consumed in FIFO mode
> + * @data_len: length of the FIFO data samples
> + */
> +struct qcom_haptics_effect {
> + enum qcom_haptics_mode mode;
> + u32 vmax_mv;
> + u32 length_us;
> +
> + u8 amplitude;
> +
> + enum qcom_haptics_play_rate play_rate;
> + s8 *fifo_data;
> + u32 data_len;
> +};
> +
> +/**
> + * struct haptics_play_req: A haptics play request
> + * @node: list node of the request
> + * @effect_id: effect index of the request
> + * @play: flag of starting or stopping the play
> + */
> +struct haptics_play_req {
> + struct list_head node;
> + int effect_id;
> + bool play;
> +};
> +
> +/**
> + * struct qcom_haptics
> + * @dev: underlying SPMI device
> + * @regmap: regmap for SPMI register access
> + * @input: input device exposing the FF interface
> + * @cfg_base: base address of the CFG peripheral
> + * @ptn_base: base address of the PTN peripheral
> + * @t_lra_us: LRA resonance period in microseconds
> + * @vmax_mv: maximum actuator drive voltage in millivolts
> + * @fifo_len: programmed HW FIFO depth in bytes
> + * @gain: playback gain scaler
> + * @play_work: delayed work that plays the queued requests
> + * @play_lock: mutex lock to serialize playbacks
> + * @play_queue_lock: spinlock protecting @play_queue
> + * @play_queue: The list of pending start/stop requests
> + * @active_effect_id: index into @effects[] currently under play
> + * @active_mode: mode of the effect currently armed in hardware
> + * @fifo_empty_irq: IRQ number for the FIFO-empty interrupt
> + * @pm_ref_held: true while a pm_runtime_get is held
> + * @irq_enabled: true if fifo_empty_irq is enabled
> + * @fifo_lock: mutex protecting the FIFO streaming data
> + * @fifo_data: pointer of the data buffer for FIFO streaming
> + * @data_len: length of the data buffer for current effect
> + * @data_written: number of samples written to the hardware FIFO
> + * @effects: table of the effects
> + */
> +struct qcom_haptics {
> + struct device *dev;
> + struct regmap *regmap;
> + struct input_dev *input;
> +
> + u32 cfg_base;
> + u32 ptn_base;
> + u32 t_lra_us;
> + u32 vmax_mv;
> + u32 fifo_len;
> + atomic_t gain;
> +
> + struct delayed_work play_work;
> + struct mutex play_lock; /* mutex used to serialize playbacks */
> +
> + spinlock_t play_queue_lock; /* protects play_queue */
> + struct list_head play_queue;
> +
> + int active_effect_id;
> + enum qcom_haptics_mode active_mode;
> +
> + int fifo_empty_irq;
> + bool pm_ref_held;
> + bool irq_enabled;
> +
> + struct mutex fifo_lock; /* protect the FIFO data during play */
> + const s8 *fifo_data;
> + u32 data_len;
> + u32 data_written;
> +
> + struct qcom_haptics_effect effects[HAPTICS_MAX_EFFECTS];
> +};
> +
> +static int cfg_write(struct qcom_haptics *h, u32 off, u32 val)
> +{
> + return regmap_write(h->regmap, h->cfg_base + off, val);
> +}
> +
> +static int cfg_update_bits(struct qcom_haptics *h, u32 off, u32 mask, u32 val)
> +{
> + return regmap_update_bits(h->regmap, h->cfg_base + off, mask, val);
> +}
> +
> +static int ptn_write(struct qcom_haptics *h, u32 off, u32 val)
> +{
> + return regmap_write(h->regmap, h->ptn_base + off, val);
> +}
> +
> +static int ptn_update_bits(struct qcom_haptics *h, u32 off, u32 mask, u32 val)
> +{
> + return regmap_update_bits(h->regmap, h->ptn_base + off, mask, val);
> +}
> +
> +static int ptn_bulk_write(struct qcom_haptics *h, u32 off,
> + const void *buf, size_t count)
> +{
> + return regmap_bulk_write(h->regmap, h->ptn_base + off, buf, count);
> +}
> +
> +static int haptics_clear_faults(struct qcom_haptics *h)
> +{
> + return cfg_write(h, HAP_CFG_FAULT_CLR_REG, FAULT_CLR_ALL);
> +}
> +
> +static int haptics_set_vmax(struct qcom_haptics *h, u32 vmax_mv)
> +{
> + return cfg_write(h, HAP_CFG_VMAX_REG, vmax_mv / VMAX_STEP_MV);
> +}
> +
> +static int haptics_config_lra_period(struct qcom_haptics *h)
> +{
> + u32 tmp = h->t_lra_us / TLRA_STEP_US;
> + int ret;
Please use "error" or "err" for such variables that hold error codes.
> +
> + ret = cfg_write(h, HAP_CFG_TLRA_OL_HIGH_REG, (tmp >> 8) & TLRA_OL_MSB_MASK);
> + if (ret)
> + return ret;
> +
> + return cfg_write(h, HAP_CFG_TLRA_OL_LOW_REG, tmp & 0xFF);
Explicit error handling and "return 0;" in cases where there are
multiple failure points.
> +}
> +
> +static int haptics_enable_module(struct qcom_haptics *h, bool enable)
> +{
> + return cfg_update_bits(h, HAP_CFG_EN_CTL_REG, HAPTICS_EN_BIT,
> + enable ? HAPTICS_EN_BIT : 0);
> +}
> +
> +static int haptics_configure_autores(struct qcom_haptics *h)
> +{
> + int ret;
> +
> + /* AUTORES_CFG: enable, 10-cycle delay, 25% error window */
> + ret = cfg_write(h, HAP_CFG_AUTORES_CFG_REG,
> + AUTORES_EN_BIT |
> + FIELD_PREP(AUTORES_EN_DLY_MASK, AUTORES_EN_DLY_CYCLES) |
> + FIELD_PREP(AUTORES_ERR_WIN_MASK, AUTORES_ERR_WIN_25PCT));
> + if (ret)
> + return ret;
> +
> + /* DRV_DUTY: adaptive drive/brake duty cycles at 62.5% */
> + ret = cfg_write(h, HAP_CFG_DRV_DUTY_CFG_REG,
> + ADT_DRV_DUTY_EN_BIT | ADT_BRK_DUTY_EN_BIT |
> + FIELD_PREP(DRV_DUTY_MASK, AUTORES_DRV_DUTY_62P5) |
> + FIELD_PREP(BRK_DUTY_MASK, AUTORES_BRK_DUTY_62P5));
> + if (ret)
> + return ret;
> +
> + /* Pre-HIZ delay: 10 µs */
> + ret = cfg_write(h, HAP_CFG_RAMP_DN_CFG2_REG, AUTORES_PRE_HIZ_DLY_10US);
> + if (ret)
> + return ret;
> +
> + /* Zero-cross window: debounce 3, no hysteresis, height 2 */
> + return cfg_write(h, HAP_CFG_ZX_WIND_CFG_REG,
> + FIELD_PREP(ZX_DEBOUNCE_MASK, AUTORES_ZX_DEBOUNCE) |
> + FIELD_PREP(ZX_WIN_HEIGHT_MASK, AUTORES_ZX_WIN_HEIGHT));
> +}
> +
> +static int haptics_write_fifo_chunk(struct qcom_haptics *h,
> + const s8 *data, u32 len)
> +{
> + u32 bulk_len = ALIGN_DOWN(len, 4);
> + int i, ret;
> +
> + /*
> + * FIFO data writing supports both 4-byte bulk writes using registers
> + * [HAP_PTN_FIFO_DIN_0_REG ... HAP_PTN_FIFO_DIN_3_REG], and 1-byte writes
> + * using the HAP_PTN_FIFO_DIN_1B_REG register. The 4-byte bulk write is more
> + * efficient, so use 4-byte writes for the initial 4-byte aligned data,
> + * and 1-byte writes for any trailing remainder.
> + */
> + for (i = 0; i < bulk_len; i += 4) {
> + ret = ptn_bulk_write(h, HAP_PTN_FIFO_DIN_0_REG, &data[i], 4);
> + if (ret)
> + return ret;
> + }
> +
> + for (; i < len; i++) {
> + ret = ptn_write(h, HAP_PTN_FIFO_DIN_1B_REG, (u8)data[i]);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int haptics_configure_fifo_mmap(struct qcom_haptics *h)
> +{
> + u32 fifo_len, fifo_units;
> +
> + /* Config all memory space for FIFO usage for now */
> + fifo_len = HAP530_MEM_TOTAL_BYTES;
> + fifo_len = ALIGN_DOWN(fifo_len, 64);
> + fifo_units = fifo_len / 64;
> + h->fifo_len = fifo_len;
> +
> + return ptn_write(h, HAP_PTN_MMAP_FIFO_REG,
> + MMAP_FIFO_EXIST_BIT |
> + FIELD_PREP(MMAP_FIFO_LEN_MASK, fifo_units - 1));
> +}
> +
> +static u32 haptics_gain_scaled_vmax(struct qcom_haptics *h, u32 vmax_mv)
> +{
> + u16 gain = atomic_read(&h->gain);
> + u32 v = mult_frac(vmax_mv, gain, 0xFFFF);
> +
> + return max_t(u32, v, VMAX_STEP_MV);
> +}
> +
> +static void haptics_fifo_irq_enable(struct qcom_haptics *h, bool enable)
> +{
> + if (h->irq_enabled == enable)
> + return;
Should t you know if given code runs with interrupts disabled or
enabled? I believe this tracking and the wrapper should be removed.
> +
> + if (enable)
> + enable_irq(h->fifo_empty_irq);
> + else
> + disable_irq(h->fifo_empty_irq);
> +
> + h->irq_enabled = enable;
> +}
> +
> +static int haptics_enqueue_play_req(struct qcom_haptics *h, int effect_id, bool play)
> +{
> + struct haptics_play_req *req;
> +
> + guard(spinlock_irqsave)(&h->play_queue_lock);
> +
> + /*
> + * Coalesce with an already queued request for
> + * the same effect instead of appending.
> + */
> + list_for_each_entry(req, &h->play_queue, node) {
> + if (req->effect_id == effect_id) {
> + req->play = play;
> + return 0;
> + }
> + }
> +
> + req = kmalloc_obj(*req, GFP_ATOMIC);
> + if (!req)
> + return -ENOMEM;
> +
> + req->effect_id = effect_id;
> + req->play = play;
> +
> + list_add_tail(&req->node, &h->play_queue);
> +
> + return 0;
> +}
> +
> +static struct haptics_play_req *haptics_dequeue_play_req(struct qcom_haptics *h)
> +{
> + struct haptics_play_req *req = NULL;
> +
> + guard(spinlock_irqsave)(&h->play_queue_lock);
> +
> + if (!list_empty(&h->play_queue)) {
> + req = list_first_entry(&h->play_queue, struct haptics_play_req, node);
> + list_del(&req->node);
> + }
> +
> + return req;
> +}
> +
> +static bool haptics_queue_pending(struct qcom_haptics *h)
> +{
> + guard(spinlock_irqsave)(&h->play_queue_lock);
> +
> + return !list_empty(&h->play_queue);
I strongly dislike this kind of functions since their result is
obsolete the moment they return. Sometimes it is OK, but usually it
isn't. In this driver I think it is fine, but I would much rather inline
this into the caller and added comment. We can also schedule work
holding this spinlock I think...
> +}
> +
> +static void haptics_queue_remove_effect(struct qcom_haptics *h, int effect_id)
> +{
> + struct haptics_play_req *req, *tmp;
> +
> + guard(spinlock_irqsave)(&h->play_queue_lock);
> +
> + list_for_each_entry_safe(req, tmp, &h->play_queue, node) {
> + if (req->effect_id == effect_id) {
> + list_del(&req->node);
> + kfree(req);
> + }
> + }
> +}
> +
> +static void haptics_queue_flush(struct qcom_haptics *h)
> +{
> + struct haptics_play_req *req, *tmp;
> +
> + guard(spinlock_irqsave)(&h->play_queue_lock);
> +
> + list_for_each_entry_safe(req, tmp, &h->play_queue, node) {
> + list_del(&req->node);
> + kfree(req);
> + }
> +}
> +
> +static int haptics_start_direct_play(struct qcom_haptics *h, int effect_id)
> +{
> + struct qcom_haptics_effect *eff = &h->effects[effect_id];
> + int ret;
> +
> + ret = haptics_clear_faults(h);
> + if (ret)
> + return ret;
> +
> + /* Enable auto-resonance for DIRECT_PLAY mode */
> + ret = cfg_update_bits(h, HAP_CFG_AUTORES_CFG_REG,
> + AUTORES_EN_BIT, AUTORES_EN_BIT);
> + if (ret)
> + return ret;
> +
> + ret = haptics_set_vmax(h, haptics_gain_scaled_vmax(h, h->vmax_mv));
> + if (ret)
> + return ret;
> +
> + ret = ptn_write(h, HAP_PTN_DIRECT_PLAY_REG, eff->amplitude);
> + if (ret)
> + return ret;
> +
> + return cfg_write(h, HAP_CFG_SPMI_PLAY_REG,
> + PLAY_EN_BIT | FIELD_PREP(PAT_SRC_MASK, PAT_SRC_DIRECT_PLAY));
> +}
> +
> +static int haptics_start_fifo(struct qcom_haptics *h, int effect_id)
> +{
> + struct qcom_haptics_effect *eff = &h->effects[effect_id];
> + u32 vmax = eff->vmax_mv ? eff->vmax_mv : h->vmax_mv;
> + u32 init_len;
> + bool data_done;
> + int ret;
> +
> + if (!eff->fifo_data || !eff->data_len)
> + return -EINVAL;
> +
> + ret = haptics_clear_faults(h);
> + if (ret)
> + return ret;
> +
> + /* Disable auto-resonance for FIFO mode */
> + ret = cfg_update_bits(h, HAP_CFG_AUTORES_CFG_REG, AUTORES_EN_BIT, 0);
> + if (ret)
> + return ret;
> +
> + ret = haptics_set_vmax(h, haptics_gain_scaled_vmax(h, vmax));
> + if (ret)
> + return ret;
> +
> + ret = ptn_update_bits(h, HAP_PTN_FIFO_PLAY_RATE_REG,
> + FIFO_PLAY_RATE_MASK,
> + FIELD_PREP(FIFO_PLAY_RATE_MASK, eff->play_rate));
> + if (ret)
> + return ret;
> +
> + /* Flush FIFO before loading new data */
> + ret = ptn_write(h, HAP_PTN_MEM_OP_ACCESS_REG, MEM_FLUSH_RELOAD_BIT);
> + if (ret)
> + return ret;
> + ret = ptn_write(h, HAP_PTN_MEM_OP_ACCESS_REG, 0);
> + if (ret)
> + return ret;
> +
> + guard(mutex)(&h->fifo_lock);
> +
> + /* Write the initial chunk and initialise streaming state */
> + init_len = min_t(u32, eff->data_len, FIFO_INIT_FILL);
> + ret = haptics_write_fifo_chunk(h, eff->fifo_data, init_len);
> + if (ret)
> + return ret;
> +
> + h->fifo_data = eff->fifo_data;
> + h->data_len = eff->data_len;
> + h->data_written = init_len;
> +
> + /*
> + * Set empty threshold. When threshold > 0 the hardware fires the
> + * FIFO-empty interrupt when occupancy drops below the threshold,
> + * allowing the driver to refill.
> + */
> + data_done = (h->data_written >= h->data_len);
> + if (!data_done) {
> + ret = ptn_write(h, HAP_PTN_FIFO_EMPTY_CFG_REG,
> + FIFO_EMPTY_THRESH / FIFO_THRESH_LSB);
> + if (ret) {
> + dev_err(h->dev, "set FIFO empty threshold failed, ret=%d\n", ret);
> + h->fifo_data = NULL;
> + return ret;
> + }
> +
> + haptics_fifo_irq_enable(h, true);
> + }
> +
> + ret = cfg_write(h, HAP_CFG_SPMI_PLAY_REG,
> + PLAY_EN_BIT | FIELD_PREP(PAT_SRC_MASK, PAT_SRC_FIFO));
> + if (ret) {
> + dev_err(h->dev, "trigger FIFO play failed, ret=%d\n", ret);
> + ptn_write(h, HAP_PTN_FIFO_EMPTY_CFG_REG, 0);
> + /*
> + * HW play never started since this SPMI write itself failed,
> + * and the FIFO empty IRQ has never fired, so disabling the
> + * IRQ synchronously here can't deadlock against 'fifo_lock'.
> + */
> + haptics_fifo_irq_enable(h, false);
> + h->fifo_data = NULL;
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * haptics_fifo_empty_irq: Threaded IRQ handler for the FIFO-empty interrupt.
> + *
> + * While a FIFO play is in progress the hardware fires this interrupt when
> + * the number of samples in the FIFO drops below the programmed threshold.
> + * The handler refills the FIFO from the effect's data buffer. When all
> + * samples have been written the threshold is set to zero. The HW would
> + * stop the play automatically after all of the samples in FIFO memory are
> + * played out.
> + */
> +static irqreturn_t haptics_fifo_empty_irq(int irq, void *dev_id)
> +{
> + struct qcom_haptics *h = dev_id;
> + u32 sts, to_write;
> + int ret;
> +
> + ret = regmap_read(h->regmap,
> + h->cfg_base + HAP_CFG_INT_RT_STS_REG, &sts);
> + if (ret || !(sts & FIFO_EMPTY_BIT))
> + return IRQ_HANDLED;
> +
> + guard(mutex)(&h->fifo_lock);
> +
> + if (!h->fifo_data)
> + return IRQ_HANDLED;
> +
> + /* Refill: write the next chunk */
> + to_write = min_t(u32, h->data_len - h->data_written,
> + h->fifo_len - FIFO_EMPTY_THRESH);
> + ret = haptics_write_fifo_chunk(h, &h->fifo_data[h->data_written], to_write);
> + if (ret) {
> + dev_err(h->dev, "refill FIFO samples failed, ret=%d\n", ret);
> + /*
> + * If data refilling is failed,stop the HW play and disable the
> + * IRQ to prevent the FIFO empty IRQ being fired continuously.
> + */
Is recovery possible after this?
> + cfg_write(h, HAP_CFG_SPMI_PLAY_REG, 0);
> + disable_irq_nosync(h->fifo_empty_irq);
> + h->irq_enabled = false;
> + return IRQ_HANDLED;
> + }
> +
> + h->data_written += to_write;
> +
> + /* Disable the interrupt after all the data is queued */
> + if (h->data_written >= h->data_len)
> + ptn_write(h, HAP_PTN_FIFO_EMPTY_CFG_REG, 0);
> +
> + return IRQ_HANDLED;
> +}
> +
> +/*
> + * haptics_stop_locked: stop play in HW and put runtime PM
> + * @h: haptics device
> + * @put_noidle: a flag to put noidle
> + *
> + * Stop HW play, clear the FIFO data if the active effect was FIFO-mode, and
> + * put runtime PM into either noidle or autosuspend based on put_noidle flag.
> + *
> + * Must be called with play_lock held.
Maybe add lockdep_assert_held() instead of comments/naming?
> + */
> +static void haptics_stop_locked(struct qcom_haptics *h, bool put_noidle)
> +{
> + cfg_write(h, HAP_CFG_SPMI_PLAY_REG, 0);
> +
> + if (h->active_mode == HAPTICS_FIFO) {
> + ptn_write(h, HAP_PTN_FIFO_EMPTY_CFG_REG, 0);
> + haptics_fifo_irq_enable(h, false);
> + scoped_guard(mutex, &h->fifo_lock) {
> + h->fifo_data = NULL;
> + }
> + }
> +
> + h->active_effect_id = -1;
> + h->active_mode = HAPTICS_MODE_NONE;
> +
> + if (h->pm_ref_held) {
> + if (put_noidle)
> + pm_runtime_put_noidle(h->dev);
> + else
> + pm_runtime_put_autosuspend(h->dev);
> +
> + h->pm_ref_held = false;
> + }
> +}
> +
> +/*
> + * haptics_start_locked: start to play an effect
> + * @h: haptics device
> + * @effect_id: the index of the effect
> + *
> + * If an effect is currently active, stop it 1st. Acquires a PM ref
> + * if not already held, and then trigger the play based on the
> + * play mode. Update active_effect_id/active_mode on success.
> + *
> + * Must be called with play_lock held.
> + */
> +static int haptics_start_locked(struct qcom_haptics *h, int effect_id)
> +{
> + int ret;
> +
> + if (h->active_effect_id != -1)
> + haptics_stop_locked(h, false);
> +
> + if (!h->pm_ref_held) {
> + ret = pm_runtime_resume_and_get(h->dev);
> + if (ret < 0) {
> + dev_err(h->dev, "failed to resume device: %d\n", ret);
> + return ret;
> + }
> +
> + h->pm_ref_held = true;
> + }
> +
> + switch (h->effects[effect_id].mode) {
> + case HAPTICS_DIRECT_PLAY:
> + ret = haptics_start_direct_play(h, effect_id);
> + break;
> + case HAPTICS_FIFO:
> + ret = haptics_start_fifo(h, effect_id);
> + break;
> + default:
> + ret = -EINVAL;
> + }
> +
> + if (ret) {
> + dev_err(h->dev, "failed to start effect %d: %d\n", effect_id, ret);
> + if (h->pm_ref_held) {
> + pm_runtime_put_autosuspend(h->dev);
> + h->pm_ref_held = false;
> + }
> +
> + return ret;
> + }
> +
> + h->active_effect_id = effect_id;
> + h->active_mode = h->effects[effect_id].mode;
> +
> + return 0;
> +}
> +
> +static void haptics_play_work(struct work_struct *work)
> +{
> + struct qcom_haptics *h = container_of(to_delayed_work(work),
> + struct qcom_haptics, play_work);
> + struct haptics_play_req *req = haptics_dequeue_play_req(h);
> + bool rearmed = false;
> + u32 length_us;
> + int ret;
> +
> + guard(mutex)(&h->play_lock);
> +
> + /* No valid request available */
> + if (req == NULL)
if (!req)
> + return;
> +
> + if (req->play) {
> + ret = haptics_start_locked(h, req->effect_id);
> + if (ret) {
> + dev_err(h->dev, "play haptics failed, ret=%d\n", ret);
> + } else {
> + /*
> + * Re-arm the work to stop the play or advance
> + * to next play after the play duration
> + */
> + length_us = h->effects[req->effect_id].length_us;
> + if (length_us) {
> + schedule_delayed_work(&h->play_work,
> + usecs_to_jiffies(length_us));
> + rearmed = true;
> + }
> + }
> + } else {
> + if (req->effect_id != h->active_effect_id)
> + dev_warn(h->dev, "effect %d is not under playing\n", req->effect_id);
> + else
> + haptics_stop_locked(h, false);
> + }
> +
> + kfree(req);
> +
> + /*
> + * Handle the remaining requests if doesn't need to wait
> + */
> + if (!rearmed && haptics_queue_pending(h))
> + schedule_delayed_work(&h->play_work, 0);
> +}
> +
> +static void haptics_clear_effect(struct qcom_haptics *h,
> + struct qcom_haptics_effect *effect)
> +{
> + guard(mutex)(&h->fifo_lock);
> +
> + if (h->fifo_data == effect->fifo_data)
> + h->fifo_data = NULL;
> +
> + kvfree(effect->fifo_data);
> + effect->fifo_data = NULL;
> + effect->data_len = 0;
> +}
> +
> +/*
> + * haptics_fifo_length_us: Calculate the play duration of a FIFO effect.
> + * @h: haptics device pointer
> + * @rate: FIFO data play rate
> + * @data_len: FFO data length
> + *
> + * Each FIFO sample is played out over one play-rate period, so the total
> + * duration is the number of samples times that period:
> + *
> + * length_us = (clk_base * data_len * multiplier) / divider
> + *
> + * For T_LRA-based rates the period derives from the LRA resonance period
> + * (h->t_lra_us): DIV_2/4/8 shorten it, X_2/4/8 lengthen it. For the
> + * kHz-based rates the period is 1000 us / freq_khz.
> + */
> +static u32 haptics_fifo_length_us(struct qcom_haptics *h,
> + enum qcom_haptics_play_rate rate,
> + u32 data_len)
> +{
> + u32 clk_base = 1000, multiplier = 1, divider = 1;
> +
> + switch (rate) {
> + case PLAY_RATE_T_LRA:
> + clk_base = h->t_lra_us;
> + break;
> + case PLAY_RATE_T_LRA_DIV_2:
> + clk_base = h->t_lra_us;
> + divider = 2;
> + break;
> + case PLAY_RATE_T_LRA_DIV_4:
> + clk_base = h->t_lra_us;
> + divider = 4;
> + break;
> + case PLAY_RATE_T_LRA_DIV_8:
> + clk_base = h->t_lra_us;
> + divider = 8;
> + break;
> + case PLAY_RATE_T_LRA_X_2:
> + clk_base = h->t_lra_us;
> + multiplier = 2;
> + break;
> + case PLAY_RATE_T_LRA_X_4:
> + clk_base = h->t_lra_us;
> + multiplier = 4;
> + break;
> + case PLAY_RATE_T_LRA_X_8:
> + clk_base = h->t_lra_us;
> + multiplier = 8;
> + break;
> + case PLAY_RATE_F_8KHZ:
> + divider = 8;
> + break;
> + case PLAY_RATE_F_16KHZ:
> + divider = 16;
> + break;
> + case PLAY_RATE_F_24KHZ:
> + divider = 24;
> + break;
> + case PLAY_RATE_F_32KHZ:
> + divider = 32;
> + break;
> + case PLAY_RATE_F_44P1KHZ:
> + clk_base *= 10;
> + divider = 441;
> + break;
> + case PLAY_RATE_F_48KHZ:
> + divider = 48;
> + break;
> + default:
> + /* Unexpected rate: fall back to the resonance period. */
> + clk_base = h->t_lra_us;
> + break;
> + }
> +
> + return div_u64((u64)clk_base * data_len * multiplier, divider);
> +}
> +
> +static int haptics_upload_effect(struct input_dev *dev,
> + struct ff_effect *effect,
> + struct ff_effect *old)
> +{
> + struct qcom_haptics *h = input_get_drvdata(dev);
> + struct qcom_haptics_effect *priv;
> + int id = effect->id;
> + u32 data_len, level;
> + s8 *fifo;
> +
> + s16 *buf __free(kvfree) = NULL;
> +
> + guard(mutex)(&h->play_lock);
> +
> + if (id < 0 || id >= HAPTICS_MAX_EFFECTS)
> + return -EINVAL;
> +
> + if (id == h->active_effect_id) {
> + dev_err(h->dev, "effect %d is under playing\n", id);
> + return -EBUSY;
> + }
> +
> + priv = &h->effects[id];
> +
> + switch (effect->type) {
> + case FF_CONSTANT:
> + haptics_clear_effect(h, priv);
> + level = effect->u.constant.level <= 0 ? 0 : effect->u.constant.level;
> + priv->amplitude = (u8)mult_frac(level, 255, 0x7FFF);
> + priv->length_us = effect->replay.length * USEC_PER_MSEC;
> + priv->mode = HAPTICS_DIRECT_PLAY;
> + return 0;
> +
> + case FF_PERIODIC:
> + if (effect->u.periodic.waveform != FF_CUSTOM)
> + return -EINVAL;
> + /*
> + * Minimum 3 elements: play-rate code + vmax + at least one sample.
> + * Limit the maximum data length to ~48K so that it can at least
> + * handle ~1s vibration at the fast (48K) play rate.
> + */
> + if (effect->u.periodic.custom_len < 3 ||
> + effect->u.periodic.custom_len > CUSTOM_DATA_MAX_LEN + 2)
> + return -EINVAL;
> +
> + buf = vmemdup_array_user(effect->u.periodic.custom_data,
> + effect->u.periodic.custom_len,
> + sizeof(s16));
> + if (IS_ERR(buf))
> + return PTR_ERR(no_free_ptr(buf));
> +
> + if (buf[CUSTOM_DATA_RATE_IDX] < 0 ||
> + buf[CUSTOM_DATA_RATE_IDX] > PLAY_RATE_MAX ||
> + buf[CUSTOM_DATA_RATE_IDX] == PLAY_RATE_RESERVED)
> + return -EINVAL;
> +
> + data_len = effect->u.periodic.custom_len - CUSTOM_DATA_SAMPLE_START;
> +
> + fifo = kvmalloc(data_len, GFP_KERNEL);
> + if (!fifo)
> + return -ENOMEM;
> +
> + /* Pack: one s8 sample per s16 slot (lower byte) */
> + for (int i = 0; i < data_len; i++)
> + fifo[i] = (s8)buf[CUSTOM_DATA_SAMPLE_START + i];
> +
> + haptics_clear_effect(h, priv);
> +
> + scoped_guard(mutex, &h->fifo_lock) {
> + priv->fifo_data = fifo;
> + priv->data_len = data_len;
> + }
> +
> + priv->play_rate = (u8)buf[CUSTOM_DATA_RATE_IDX];
> + priv->vmax_mv = (u32)clamp_val(buf[CUSTOM_DATA_VMAX_IDX], 0, VMAX_MV_MAX);
> + priv->length_us = haptics_fifo_length_us(h, priv->play_rate, data_len);
> + priv->mode = HAPTICS_FIFO;
> +
> + return 0;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int haptics_playback(struct input_dev *dev, int effect_id, int val)
> +{
> + struct qcom_haptics *h = input_get_drvdata(dev);
> + int ret;
> +
> + ret = haptics_enqueue_play_req(h, effect_id, val > 0);
> + if (ret)
> + return ret;
> +
> + if (val > 0)
> + /*
> + * Queue the play. If a duration re-arm is already pending this
> + * is a no-op, so the new play waits for the current effect to
> + * finish before the worker dequeues it.
> + */
> + schedule_delayed_work(&h->play_work, 0);
> + else
> + /*
> + * Run the worker now, cancelling any pending duration re-arm,
> + * so an explicit stop takes effect immediately.
> + */
> + mod_delayed_work(system_percpu_wq, &h->play_work, 0);
> +
> + return 0;
> +}
> +
> +static int haptics_erase(struct input_dev *dev, int effect_id)
> +{
> + struct qcom_haptics *h = input_get_drvdata(dev);
> + struct qcom_haptics_effect *priv = &h->effects[effect_id];
> +
> + guard(mutex)(&h->play_lock);
> +
> + haptics_queue_remove_effect(h, effect_id);
> +
> + if (h->active_effect_id == effect_id)
> + haptics_stop_locked(h, false);
> +
> + haptics_clear_effect(h, priv);
> + priv->mode = HAPTICS_MODE_NONE;
> +
> + return 0;
> +}
> +
> +static void haptics_set_gain(struct input_dev *dev, u16 gain)
> +{
> + struct qcom_haptics *h = input_get_drvdata(dev);
> +
> + atomic_set(&h->gain, gain);
You need to adjust currently played effects here: new gain should take
effect immediately.
> +}
> +
> +static int qcom_haptics_probe(struct platform_device *pdev)
> +{
> + struct qcom_haptics *h;
> + struct input_dev *input;
> + struct ff_device *ff;
> + u32 regs[2], vmax_uv;
> + int ret, irq;
> +
> + h = devm_kzalloc(&pdev->dev, sizeof(*h), GFP_KERNEL);
> + if (!h)
> + return -ENOMEM;
> +
> + h->dev = &pdev->dev;
> +
> + h->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> + if (!h->regmap)
> + return dev_err_probe(h->dev, -ENODEV, "no regmap from parent\n");
> +
> + ret = device_property_read_u32_array(h->dev, "reg", regs, ARRAY_SIZE(regs));
> + if (ret)
> + return dev_err_probe(h->dev, ret, "failed to read 'reg' property\n");
> +
> + h->cfg_base = regs[0];
> + h->ptn_base = regs[1];
> +
> + ret = device_property_read_u32(h->dev, "qcom,lra-period-us", &h->t_lra_us);
> + if (ret)
> + return dev_err_probe(h->dev, ret, "missing qcom,lra-period-us\n");
> +
> + h->t_lra_us = clamp(h->t_lra_us, (u32)TLRA_STEP_US, (u32)TLRA_US_MAX);
> +
> + ret = device_property_read_u32(h->dev, "qcom,vmax-microvolt", &vmax_uv);
> + if (ret)
> + return dev_err_probe(h->dev, ret, "missing qcom,vmax-microvolt\n");
> +
> + h->vmax_mv = clamp(vmax_uv / 1000, (u32)VMAX_STEP_MV, (u32)VMAX_MV_MAX);
> +
> + ret = haptics_config_lra_period(h);
> + if (ret)
> + return ret;
> +
> + ret = haptics_configure_autores(h);
> + if (ret)
> + return ret;
> +
> + ret = haptics_set_vmax(h, h->vmax_mv);
> + if (ret)
> + return ret;
> +
> + ret = haptics_configure_fifo_mmap(h);
> + if (ret)
> + return ret;
> +
> + mutex_init(&h->play_lock);
> + mutex_init(&h->fifo_lock);
> + spin_lock_init(&h->play_queue_lock);
> + INIT_LIST_HEAD(&h->play_queue);
> + INIT_DELAYED_WORK(&h->play_work, haptics_play_work);
> + atomic_set(&h->gain, 0xFFFF);
> + h->active_effect_id = -1;
> +
> + irq = platform_get_irq_byname(pdev, "fifo-empty");
> + if (irq < 0)
> + return dev_err_probe(h->dev, irq, "failed to get fifo-empty IRQ\n");
> +
> + ret = devm_request_threaded_irq(h->dev, irq, NULL,
> + haptics_fifo_empty_irq,
> + IRQF_ONESHOT | IRQF_NO_AUTOEN,
> + "qcom-haptics-fifo-empty", h);
> + if (ret)
> + return dev_err_probe(h->dev, ret, "failed to request fifo-empty IRQ\n");
> +
> + h->fifo_empty_irq = irq;
> + platform_set_drvdata(pdev, h);
> +
> + pm_runtime_use_autosuspend(h->dev);
> + pm_runtime_set_autosuspend_delay(h->dev, HAPTICS_AUTOSUSPEND_MS);
> + ret = devm_pm_runtime_enable(h->dev);
> + if (ret)
> + return dev_err_probe(h->dev, ret, "enable runtime PM failed\n");
> +
> + input = devm_input_allocate_device(h->dev);
> + if (!input)
> + return -ENOMEM;
> +
> + input->name = "qcom-spmi-haptics";
Set bus type? BUS_HOST?
> + input_set_drvdata(input, h);
> + h->input = input;
> +
> + input_set_capability(input, EV_FF, FF_CONSTANT);
> + input_set_capability(input, EV_FF, FF_PERIODIC);
> + input_set_capability(input, EV_FF, FF_CUSTOM);
> + input_set_capability(input, EV_FF, FF_GAIN);
> +
> + ret = input_ff_create(input, HAPTICS_MAX_EFFECTS);
> + if (ret)
> + return ret;
> +
> + ff = input->ff;
> + ff->upload = haptics_upload_effect;
> + ff->playback = haptics_playback;
> + ff->erase = haptics_erase;
> + ff->set_gain = haptics_set_gain;
> +
> + ret = input_register_device(input);
> + if (ret) {
> + input_ff_destroy(input);
This is harmless but not needed: FF portion will be freed automatically.
> + return dev_err_probe(h->dev, ret, "failed to register input device\n");
> + }
> +
> + return 0;
> +}
> +
> +static void qcom_haptics_remove(struct platform_device *pdev)
> +{
> + struct qcom_haptics *h = platform_get_drvdata(pdev);
> +
> + /*
> + * Unregister the input device explicitly at the beginning
> + * to avoid the input device being used after the resources
> + * are freed.
> + */
> + input_unregister_device(h->input);
> + disable_delayed_work_sync(&h->play_work);
> + scoped_guard(mutex, &h->play_lock) {
> + haptics_queue_flush(h);
> + haptics_stop_locked(h, true);
> + }
> +
> + haptics_enable_module(h, false);
> +
> + scoped_guard(mutex, &h->fifo_lock) {
> + for (int i = 0; i < HAPTICS_MAX_EFFECTS; i++) {
> + kvfree(h->effects[i].fifo_data);
> + h->effects[i].fifo_data = NULL;
> + }
> + }
Most of this is better suited to close() method of input device.
> +}
> +
> +static int qcom_haptics_runtime_suspend(struct device *dev)
> +{
> + struct qcom_haptics *h = dev_get_drvdata(dev);
> +
> + return haptics_enable_module(h, false);
> +}
> +
> +static int qcom_haptics_runtime_resume(struct device *dev)
> +{
> + struct qcom_haptics *h = dev_get_drvdata(dev);
> +
> + return haptics_enable_module(h, true);
> +}
> +
> +static int qcom_haptics_suspend(struct device *dev)
> +{
> + struct qcom_haptics *h = dev_get_drvdata(dev);
> + int ret;
> +
> + disable_delayed_work_sync(&h->play_work);
> + scoped_guard(mutex, &h->play_lock) {
> + haptics_queue_flush(h);
> + haptics_stop_locked(h, true);
> + }
> +
> + ret = pm_runtime_force_suspend(dev);
> + if (ret)
> + enable_delayed_work(&h->play_work);
> +
> + return ret;
> +}
> +
> +static int qcom_haptics_resume(struct device *dev)
> +{
> + struct qcom_haptics *h = dev_get_drvdata(dev);
> +
> + enable_delayed_work(&h->play_work);
> +
> + return pm_runtime_force_resume(dev);
> +}
> +
> +static const struct dev_pm_ops qcom_haptics_pm_ops = {
> + SYSTEM_SLEEP_PM_OPS(qcom_haptics_suspend, qcom_haptics_resume)
> + RUNTIME_PM_OPS(qcom_haptics_runtime_suspend, qcom_haptics_runtime_resume,
> + NULL)
> +};
> +
> +static const struct of_device_id qcom_haptics_of_match[] = {
> + { .compatible = "qcom,spmi-haptics" },
> + {}
{ }
> +};
> +MODULE_DEVICE_TABLE(of, qcom_haptics_of_match);
> +
> +static struct platform_driver qcom_haptics_driver = {
> + .probe = qcom_haptics_probe,
> + .remove = qcom_haptics_remove,
> + .driver = {
> + .name = "qcom-spmi-haptics",
> + .of_match_table = qcom_haptics_of_match,
> + .pm = pm_ptr(&qcom_haptics_pm_ops),
> + },
> +};
> +module_platform_driver(qcom_haptics_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm SPMI PMIC Haptics driver");
> +MODULE_LICENSE("GPL");
>
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH v4 2/2] arm64: dts: qcom: glymur-crd: Add FocalTech ft3d81 touchscreen support
From: Pradyot Kumar Nayak @ 2026-07-17 11:58 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Daniel Peng, Bjorn Andersson, Konrad Dybcio, Abel Vesa,
Dmitry Baryshkov
Cc: linux-arm-msm, linux-input, devicetree, linux-kernel,
Konrad Dybcio, Pradyot Kumar Nayak
In-Reply-To: <20260717-add_focaltech_ft3d81_touchscreen_support-v4-0-5dd091e25801@oss.qualcomm.com>
The touchscreen module on Glymur/Mahua CRDs is different from
the one used on Hamoa CRDs and requires the reset-gpios to be wired to
the device. Without this in place the reset line will remain
permanently asserted during resume leaving the device offline and causing
all I2C transactions to fail with -ENXIO.
i2c_hid_of 3-0038: failed to change power setting.
i2c_hid_of 3-0038: PM: dpm_run_callback():
i2c_hid_core_pm_resume [i2c_hid] returns -6
i2c_hid_of 3-0038: PM: failed to resume async: error -6
The touchscreen on Glymur/Mahua-CRD is a focaltech ft3d81,
which is hardware-compatible with the ft8112.
we have added the required change in DT.
Fixes: e6bf559f7eb9 ("arm64: dts: qcom: glymur-crd: Enable keyboard, trackpad and touchscreen")
Fixes: f64ef325f1d9 ("arm64: dts: glymur-crd: Add reset GPIO to touchscreen node")
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Pradyot Kumar Nayak <pradyot.nayak@oss.qualcomm.com>
---
arch/arm64/boot/dts/qcom/glymur-crd.dtsi | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/glymur-crd.dtsi b/arch/arm64/boot/dts/qcom/glymur-crd.dtsi
index b314c6dd423c..4ae8834b4110 100644
--- a/arch/arm64/boot/dts/qcom/glymur-crd.dtsi
+++ b/arch/arm64/boot/dts/qcom/glymur-crd.dtsi
@@ -679,14 +679,13 @@ &i2c8 {
status = "okay";
touchscreen@38 {
- compatible = "hid-over-i2c";
+ compatible = "focaltech,ft3d81", "focaltech,ft8112";
reg = <0x38>;
- hid-descr-addr = <0x1>;
interrupts-extended = <&tlmm 51 IRQ_TYPE_LEVEL_LOW>;
- vdd-supply = <&vreg_misc_3p3>;
- vddl-supply = <&vreg_l15b_e0_1p8>;
+ vcc33-supply = <&vreg_misc_3p3>;
+ vccio-supply = <&vreg_l15b_e0_1p8>;
reset-gpios = <&tlmm 48 GPIO_ACTIVE_LOW>;
--
2.43.0
^ permalink raw reply related
* [PATCH v4 1/2] dt-bindings: input: focaltech,ft8112: Add focaltech,ft3d81 compatible
From: Pradyot Kumar Nayak @ 2026-07-17 11:58 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Daniel Peng, Bjorn Andersson, Konrad Dybcio, Abel Vesa,
Dmitry Baryshkov
Cc: linux-arm-msm, linux-input, devicetree, linux-kernel,
Konrad Dybcio, Krzysztof Kozlowski, Pradyot Kumar Nayak
In-Reply-To: <20260717-add_focaltech_ft3d81_touchscreen_support-v4-0-5dd091e25801@oss.qualcomm.com>
The Focaltech ft3d81 is fully compatible with the ft8112 i.e.
it uses the same I2C-HID protocol and the same power-on/reset sequencing,
DT nodes for boards carrying an ft3d81,can therefore bind to the existing
ft8112 driver without any additional changes.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Signed-off-by: Pradyot Kumar Nayak <pradyot.nayak@oss.qualcomm.com>
---
Documentation/devicetree/bindings/input/focaltech,ft8112.yaml | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/input/focaltech,ft8112.yaml b/Documentation/devicetree/bindings/input/focaltech,ft8112.yaml
index 197f30b14d45..5ffa1246aba1 100644
--- a/Documentation/devicetree/bindings/input/focaltech,ft8112.yaml
+++ b/Documentation/devicetree/bindings/input/focaltech,ft8112.yaml
@@ -18,8 +18,13 @@ allOf:
properties:
compatible:
- enum:
- - focaltech,ft8112
+ oneOf:
+ - items:
+ - enum:
+ - focaltech,ft3d81
+ - const: focaltech,ft8112
+ - enum:
+ - focaltech,ft8112
reg:
maxItems: 1
--
2.43.0
^ permalink raw reply related
* [PATCH v4 0/2] cover: arm64: dts: qcom: glymur-crd: Add touchscreen node for ft3d81
From: Pradyot Kumar Nayak @ 2026-07-17 11:58 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Daniel Peng, Bjorn Andersson, Konrad Dybcio, Abel Vesa,
Dmitry Baryshkov
Cc: linux-arm-msm, linux-input, devicetree, linux-kernel,
Konrad Dybcio, Krzysztof Kozlowski, Pradyot Kumar Nayak
The touchscreen module on Glymur/Mahua CRDs is a ft3d81 chip which, while
hardware-compatible with the ft8112 (same I2C-HID protocol, same
power-on/reset sequencing), requires the reset-gpios line to be
explicitly wired up in the DT. Without it the reset line remains
permanently asserted after a suspend/resume cycle and all subsequent
I2C transactions fail with -ENXIO.
V2 incorrectly used the generic hid-over-i2c binding. V3
takes a cleaner approach: extend the chip-specific focaltech,ft8112
binding to cover the ft3d81 (they share the same driver) and switch
the glymur-crd node to that binding. This removes the dependency on
the hid-over-i2c properties (hid-descr-addr) and aligns the supply
property names (vcc33-supply / vccio-supply) with the binding schema.
V3 was not picked up; instead V1 was accidentally merged by the maintainer.
V4 corrects this by superseding the partial V1 fix, and moves the DT change
from glymur-crd.dts to glymur-crd.dtsi where the shared board nodes belong.
v1: https://lore.kernel.org/linux-arm-msm/20260522-arm64-dts-glymur-crd-add-reset-gpio-to-touchscreen-v1-1-c7653924acdc@oss.qualcomm.com/
Changes in v1:
- Initial submission: add reset-gpios to touchscreen node in glymur-crd.dtsi
v2: https://lore.kernel.org/linux-arm-msm/20260523-arm64-dts-glymur-crd-add-reset-gpio-to-touchscreen-v2-0-5c333051e5bb@oss.qualcomm.com/
Changes in v2:
- Add dt-bindings patch to document reset-gpios in hid-over-i2c schema
(flagged by dtbs_check on v1)
- No change to the DTS patch itself
v3: https://lore.kernel.org/linux-arm-msm/20260603-arm64-dts-glymur-crd-add-reset-gpio-to-v3-0-3453ef577bcf@oss.qualcomm.com/
Changes in v3:
- Drop prerequisite-message-id; series is now self-contained
- Patch 1 (new): add focaltech,ft3d81 to the focaltech,ft8112
binding compatible enum
- Patch 2: Added focaltech,ft3d81 DT support which is hardware-compatible with the ft8112
Changes in v4:
- v1 was accidentally merged by the maintainer instead of v3; raising
v4 to correct this
- Move DT change from glymur-crd.dts to glymur-crd.dtsi as the
touchscreen node belongs to the shared board DTSI
---
Pradyot Kumar Nayak (2):
dt-bindings: input: focaltech,ft8112: Add focaltech,ft3d81 compatible
arm64: dts: qcom: glymur-crd: Add FocalTech ft3d81 touchscreen support
Documentation/devicetree/bindings/input/focaltech,ft8112.yaml | 9 +++++++--
arch/arm64/boot/dts/qcom/glymur-crd.dtsi | 7 +++----
2 files changed, 10 insertions(+), 6 deletions(-)
---
base-commit: 1a1757b76427f6201bfe0bf1bea9f7574f332a93
change-id: 20260717-add_focaltech_ft3d81_touchscreen_support-263409a84b78
Best regards,
--
Pradyot Kumar Nayak <pradyot.nayak@oss.qualcomm.com>
^ permalink raw reply
* Re: [PATCH] HID: amd_sfh: clear hid_sensor_hubs entry on probe failure
From: Basavaraj Natikar @ 2026-07-17 10:37 UTC (permalink / raw)
To: Chen Changcheng, basavaraj.natikar, jikos, bentiss
Cc: linux-input, linux-kernel
In-Reply-To: <20260715094622.115062-1-chenchangcheng@kylinos.cn>
On 7/15/2026 3:16 PM, Chen Changcheng wrote:
> [You don't often get email from chenchangcheng@kylinos.cn. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> In amdtp_hid_probe(), the newly allocated HID device is stored in
> cli_data->hid_sensor_hubs[cur_hid_dev] before calling hid_add_device().
> If hid_add_device() fails, the error path frees the HID device and its
> driver_data but does not clear the array entry, leaving a dangling
> pointer.
>
> When the caller (amd_sfh_hid_client_init or
> amd_sfh1_1_hid_client_init) detects the probe failure, it jumps to its
> cleanup label, which unconditionally calls amd_sfh_hid_client_deinit()
> and subsequently amdtp_hid_remove(). The latter iterates over all
> hid_sensor_hubs[] entries and, upon encountering the non-NULL but freed
> pointer, performs a use-after-free read followed by double-free of both
> the HID device and its driver_data.
>
> Clear the array entry in the error path of amdtp_hid_probe() so that
> amdtp_hid_remove() skips the failed entry.
>
> Signed-off-by: Chen Changcheng <chenchangcheng@kylinos.cn>
> ---
> drivers/hid/amd-sfh-hid/amd_sfh_hid.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_hid.c b/drivers/hid/amd-sfh-hid/amd_sfh_hid.c
> index b04f675d49b0..9eaa2785a9ac 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_hid.c
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_hid.c
> @@ -162,6 +162,7 @@ int amdtp_hid_probe(u32 cur_hid_dev, struct amdtp_cl_data *cli_data)
> return 0;
>
> err_hid_device:
> + cli_data->hid_sensor_hubs[cur_hid_dev] = NULL;
Acked-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
Thanks,
--
Basavaraj
> kfree(hid_data);
> err_hid_data:
> hid_destroy_device(hid);
> --
> 2.25.1
>
^ permalink raw reply
* Re: [PATCH v2 1/4] HID: amd_sfh: Remove redundant dev_err()
From: Basavaraj Natikar @ 2026-07-17 9:56 UTC (permalink / raw)
To: Pan Chuang, Basavaraj Natikar, Jiri Kosina, Benjamin Tissoires,
open list:AMD SENSOR FUSION HUB DRIVER, open list
In-Reply-To: <20260716075203.593012-2-panchuang@vivo.com>
On 7/16/2026 1:21 PM, Pan Chuang wrote:
> [You don't often get email from panchuang@vivo.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Since commit 55b48e23f5c4 ("genirq/devres: Add error handling in
> devm_request_*_irq()"), devm_request_irq() automatically logs
> detailed error messages on failure. Remove the now-redundant
> driver-specific dev_err() calls.
>
> Signed-off-by: Pan Chuang <panchuang@vivo.com>
> ---
> drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> index 4b81cebdc335..a2acd3ca20e2 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> @@ -128,11 +128,8 @@ int amd_sfh_irq_init_v2(struct amd_mp2_dev *privdata)
>
> rc = devm_request_irq(&privdata->pdev->dev, privdata->pdev->irq,
> amd_sfh_irq_handler, 0, DRIVER_NAME, privdata);
> - if (rc) {
> - dev_err(&privdata->pdev->dev, "failed to request irq %d err=%d\n",
> - privdata->pdev->irq, rc);
> + if (rc)
> return rc;
> - }
Please change the function to return the call directly then
return devm_request_irq(&privdata->pdev->dev, privdata->pdev->irq,
amd_sfh_irq_handler, 0, DRIVER_NAME, privdata);
With that change:
Acked-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
Thanks,
--
Basavaraj
^ permalink raw reply
* Re: [PATCH v2] HID: intel-thc-hid: intel-quickspi: validate report size before copy
From: sashiko-bot @ 2026-07-17 9:37 UTC (permalink / raw)
To: HyeongJun An; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260717091622.1744196-1-sammiee5311@gmail.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Unbounded memcpy in `quickspi_get_report()` causes a heap buffer overflow if the device responds with a report larger than the caller-allocated buffer.
- [High] The receive buffer `qsdev->input_buf` can be overflowed during an RXDMA interrupt because the message size is bounds-checked against a 4K-aligned limit instead of the actual buffer capacity.
- [High] Unbounded memcpy in `quickspi_handle_input_data()` causes a heap buffer overflow of `qsdev->report_buf` on the receive path.
- [High] Concurrent ioctl requests lead to data corruption in the shared DMA buffer `qsdev->report_buf`.
--
--- Patch [1]: [PATCH v2] HID: intel-thc-hid: intel-quickspi: validate report size before copy ---
commit 13f5051992cd9276077cab1966153d3866079f49
Author: HyeongJun An <sammiee5311@gmail.com>
HID: intel-thc-hid: intel-quickspi: validate report size before copy
write_cmd_to_txdma() builds an output report in qsdev->report_buf, a heap
buffer allocated in quickspi_alloc_report_buf() to the device-descriptor
derived max_report_len (a few hundred bytes for a touch controller). It
copies the caller-supplied report into that buffer:
[ ... ]
> 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 4ae2e1718b30c..da5ecfcd0fbf8 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> @@ -555,7 +555,14 @@ static int quickspi_alloc_report_buf(struct quickspi_device *qsdev)
> max_report_len = max(le16_to_cpu(qsdev->dev_desc.max_output_len),
> le16_to_cpu(qsdev->dev_desc.max_input_len));
>
> - qsdev->report_buf = devm_kzalloc(qsdev->dev, max_report_len, GFP_KERNEL);
> + /*
> + * write_cmd_to_txdma() writes the output report header ahead of the
> + * content in this buffer, so it has to hold both.
> + */
> + qsdev->report_buf_size = HIDSPI_OUTPUT_REPORT_SIZE(max_report_len);
> +
> + qsdev->report_buf = devm_kzalloc(qsdev->dev, qsdev->report_buf_size,
> + GFP_KERNEL);
[Severity: High]
This is a pre-existing issue, but does the allocation for qsdev->input_buf
earlier in quickspi_alloc_report_buf() need to be 4K-aligned to prevent a heap
overflow during an RX DMA interrupt?
In thc_rxdma_read(), read_dma_buffer() bounds-checks the message length
against read_config->max_packet_size, which is 4K-aligned via
thc_dma_set_max_packet_sizes().
If a hardware device sends a DMA packet larger than max_input_len but
smaller than or equal to ALIGN(max_input_len, SZ_4K), it would pass the
bounds check in read_dma_buffer():
intel-thc/intel-thc-dma.c:read_dma_buffer() {
...
if (mes_len > read_config->max_packet_size) {
...
}
and copy up to 4096 bytes into qsdev->input_buf, which was allocated strictly
to the unaligned max_input_len.
> if (!qsdev->report_buf)
> return -ENOMEM;
>
[ ... ]
> diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
> index cb19057f1191b..db6054843e777 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
> @@ -30,6 +30,9 @@ static int write_cmd_to_txdma(struct quickspi_device *qsdev,
>
> write_buf = (struct output_report *)qsdev->report_buf;
>
> + if (HIDSPI_OUTPUT_REPORT_SIZE(report_buf_len) > qsdev->report_buf_size)
> + return -EINVAL;
> +
> write_buf->output_hdr.report_type = report_type;
> write_buf->output_hdr.content_len = cpu_to_le16(report_buf_len);
> write_buf->output_hdr.content_id = report_id;
>
> if (report_buf && report_buf_len > 0)
> memcpy(write_buf->content, report_buf, report_buf_len);
[Severity: High]
This is a pre-existing issue, but can concurrent ioctl requests cause data
corruption in the shared DMA buffer qsdev->report_buf?
Multiple userspace processes can concurrently call HIDIOCSFEATURE or
HIDIOCGFEATURE. The HID core routines hold a read-lock, allowing parallel
execution in the driver callbacks.
Does write_cmd_to_txdma() modify qsdev->report_buf and trigger DMA writes
without mutual exclusion against concurrent TX or RX requests like
quickspi_handle_input_data()?
quickspi-protocol.c:quickspi_handle_input_data() {
...
memcpy(qsdev->report_buf, input_report, qsdev->report_len);
...
}
[Severity: High]
This is a pre-existing issue, but can the receive path also overflow
qsdev->report_buf?
While this patch secures the output path, quickspi_handle_input_data()
parses input_len from the payload and executes an unbounded memcpy:
quickspi-protocol.c:quickspi_handle_input_data() {
...
memcpy(qsdev->report_buf, input_report, qsdev->report_len);
...
}
If a compromised device sends a GET_FEATURE_RESPONSE with a length exceeding
qsdev->report_buf_size, will this cause a heap buffer overflow?
[Severity: Critical]
This isn't a bug introduced by this patch, but is there an unbounded memcpy
in quickspi_get_report() that could cause a heap buffer overflow?
When a userspace process calls ioctl(HIDIOCGFEATURE) with a length parameter
smaller than the actual hardware report size, quickspi_hid_raw_request()
discards the bounds-checking len parameter:
quickspi-hid.c:quickspi_hid_raw_request() {
...
ret = quickspi_get_report(qsdev, rtype, reportnum, buf);
...
}
Then quickspi_get_report() copies qsdev->report_len bytes into the
caller's buffer without validating against the buffer's true capacity:
quickspi-protocol.c:quickspi_get_report() {
...
memcpy(buf, qsdev->report_buf, qsdev->report_len);
...
}
Could this lead to memory corruption if the device responds with a report
larger than the caller-allocated buffer?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717091622.1744196-1-sammiee5311@gmail.com?part=1
^ 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