* [PATCH] Input: xpad - add support for Azeron devices
From: Roberts Kursitis @ 2026-06-19 12:19 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Roberts Kursitis
Add USB Vendor and Product IDs for the Azeron devices (Cyro,
Cyborg, Classic/Compact, Cyro Lefty, Cyborg II and Keyzen).
These devices share a firmware XInput implementation and
expose the standard xbox 360 interface signature. Note that
0x16d0 is a shared vendor block (MCS Electronics / pid.codes
allocations), but is bound here as the XPAD_XBOX360_VENDOR
macro only binds interfaces presenting the strict xbox 360
signature.
Note: Devices may reject the LED magic-packet with -EPIPE.
Signed-off-by: Roberts Kursitis <roberts.kursitis@azeron.eu>
---
drivers/input/joystick/xpad.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index feb8f368f..82019cbe2 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -292,6 +292,14 @@ static const struct xpad_device {
{ 0x1689, 0xfd00, "Razer Onza Tournament Edition", 0, XTYPE_XBOX360 },
{ 0x1689, 0xfd01, "Razer Onza Classic Edition", 0, XTYPE_XBOX360 },
{ 0x1689, 0xfe00, "Razer Sabertooth", 0, XTYPE_XBOX360 },
+ { 0x162e, 0xbeef, "Joytech Neo-Se Take2", 0, XTYPE_XBOX360 },
+ { 0x16d0, 0x1103, "Azeron Cyro", 0, XTYPE_XBOX360 },
+ { 0x16d0, 0x113c, "Azeron Cyborg", 0, XTYPE_XBOX360 },
+ { 0x16d0, 0x1192, "Azeron Classic/Compact", 0, XTYPE_XBOX360 },
+ { 0x16d0, 0x1212, "Azeron Cyro Lefty", 0, XTYPE_XBOX360 },
+ { 0x16d0, 0x12f7, "Azeron Cyborg II", 0, XTYPE_XBOX360 },
+ { 0x16d0, 0x13ea, "Azeron Keyzen", 0, XTYPE_XBOX360 },
+ { 0x1689, 0xfd00, "Razer Onza Tournament Edition", 0, XTYPE_XBOX360 },
{ 0x17ef, 0x6182, "Lenovo Legion Controller for Windows", 0, XTYPE_XBOX360 },
{ 0x1949, 0x041a, "Amazon Game Controller", 0, XTYPE_XBOX360 },
{ 0x1a86, 0xe310, "Legion Go S", 0, XTYPE_XBOX360 },
@@ -533,6 +541,7 @@ static const struct usb_device_id xpad_table[] = {
XPAD_XBOX360_VENDOR(0x15e4), /* Numark Xbox 360 controllers */
XPAD_XBOX360_VENDOR(0x162e), /* Joytech Xbox 360 controllers */
XPAD_XBOX360_VENDOR(0x1689), /* Razer Onza */
+ XPAD_XBOX360_VENDOR(0x16d0), /* Azeron controllers */
XPAD_XBOX360_VENDOR(0x17ef), /* Lenovo */
XPAD_XBOX360_VENDOR(0x1949), /* Amazon controllers */
XPAD_XBOX360_VENDOR(0x1a86), /* Nanjing Qinheng Microelectronics (WCH) */
--
2.43.0
^ permalink raw reply related
* Re: [RFC PATCH] HID: BPF: add keyboard behavioral anomaly detection
From: sashiko-bot @ 2026-06-19 7:45 UTC (permalink / raw)
To: krishgulati7; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260619073607.393248-1-krishgulati7@gmail.com>
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Evasion of detector via large HID descriptors with padded payloads.
- [High] Missing Report ID validation causes false positives and state corruption on composite devices.
- [High] Mismatched report size rounding causes silent event dropping and logical out-of-bounds reads.
- [High] LRU map eviction permanently disables detection for a device instead of resetting its state.
- [High] Uninitialized padding bytes in the stack-allocated `struct dev_info` will cause the BPF verifier to reject the program.
- [High] Keystroke heuristic fails to detect keys when no empty report is sent between strokes (morphed keys).
- [Medium] Missing `bpf_spin_lock` protection allows data races between concurrent `kdb_hook` or `probe` invocations.
--
commit 103b2fc551543479bbac0ff3ea588c3508776a4c
Author: Krish Gulati <krishgulati7@gmail.com>
HID: BPF: add keyboard behavioral anomaly detection
This patch implements a HID-BPF struct_ops program that detects automated
HID injection attacks. It does this by measuring post-enumeration delay
and tracking inter-keystroke timing using Welford's online variance algorithm.
State is stored in an LRU hash map keyed by the HID ID.
Link: https://lore.kernel.org/linux-input/adSxXidgeWF0-Ewn@beelink/
> diff --git a/src/bpf/testing/0010-Generic__keyboard.bpf.c b/src/bpf/testing/0010-Generic__keyboard.bpf.c
> index 0000000000000..91145870768f6
> --- /dev/null
> +++ b/src/bpf/testing/0010-Generic__keyboard.bpf.c
[ ... ]
> +static __always_inline void welford(struct dev_info *dev_state,
> + __u64 interval_ms)
> +{
> + __s64 x = (__s64)interval_ms * HID_GUARD_WELFORD_SCALE;
> + __s64 delta, delta2, delta_abs, signed_div_res;
> + __u64 div_res;
> +
> + dev_state->count++;
[Severity: Medium]
Will this lockless compound update be corrupted by concurrent executions?
The dev_details LRU hash map stores per-device state which kdb_hook() reads
and modifies directly via a map value pointer locklessly. Updating Welford
variance (count, mean, M2) is a compound operation.
If kdb_hook() executes concurrently for the same device, such as across
multiple endpoints on a composite device, or if the userspace probe()
program is executed simultaneously, torn reads and writes will occur.
Does this structure require a bpf_spin_lock to prevent data races?
> + delta = x - dev_state->mean;
[ ... ]
> +SEC(HID_BPF_DEVICE_EVENT)
> +int BPF_PROG(kdb_hook, struct hid_bpf_ctx *hctx)
> +{
> + __u32 hid_id = hctx->hid->id;
> +
> + struct dev_info *info;
> +
> + info = bpf_map_lookup_elem(&dev_details, &hid_id);
> +
> + if (!info)
> + return 0;
[Severity: High]
Does an LRU map eviction permanently disable detection for a device instead
of resetting its state?
The dev_details map uses BPF_MAP_TYPE_LRU_HASH with 128 max entries.
When a device is evicted, bpf_map_lookup_elem() returns NULL, and
kdb_hook() immediately returns 0, completely ignoring the event.
Because elements are only inserted by the userspace probe() program, an
evicted device is never automatically re-inserted by the kernel hook.
Could an attacker deliberately present 128 dummy HID interfaces to flood
the map, permanently evict the payload device, and bypass the guard entirely?
> +
> + __u32 size = info->report_size;
> + __u32 fetch_size;
> + __u8 *data;
> +
> + if (size <= 8)
> + fetch_size = 8;
> + else if (size <= 16)
> + fetch_size = 16;
> + else
> + fetch_size = 32;
[Severity: Critical]
Does hardcapping the analysis size to 32 bytes allow attackers to evade
detection by padding their malicious HID descriptor?
An attacker could craft a custom USB HID descriptor that defines dummy
fields or padding for the first 32 bytes, placing the actual Keyboard Usage
page array at offset 32 or higher.
The loop will only iterate over the static padding and completely ignore
the keystrokes. Meanwhile, the Linux HID subsystem correctly parses the full
report and processes the hidden keycodes, allowing arbitrary keystroke
injection that bypasses this guard.
> +
> + data = hid_bpf_get_data(hctx, 0, fetch_size);
> +
> + if (!data)
> + return 0;
[Severity: High]
Will this size rounding cause silent event dropping and logical out-of-bounds
reads?
The BPF program rounds info->report_size up to a power-of-two fetch_size
(8, 16, or 32). If a keyboard has a 9-byte report, fetch_size becomes 16.
If the kernel's ctx->allocated_size for this device is exactly 9 bytes,
hid_bpf_get_data(hctx, 0, 16) will exceed the bounds and return NULL.
The program then returns 0, silently skipping analysis for the report
while the kernel continues processing the keystrokes.
If allocated_size is large enough, the loop uses fetch_size as the upper
bound, reading past valid report data into padding or uninitialized memory,
polluting the active key tracking.
> + int now_active_ks = 0, was_active_ks = 0;
> +#pragma unroll
> + for (int i = 0; i < 32; i++) {
> + if (i >= fetch_size)
> + break;
> + now_active_ks += ((__u8)data[i] + 255) >> 8;
[Severity: High]
Will this logic indiscriminately process all incoming raw reports, even
those not belonging to the keyboard?
Many HID keyboards are composite devices, such as wireless keyboard and
mouse receivers, that send multiple report types over the same interface.
Since the report ID is observed in probe() but not stored in dev_info,
this hook counts all non-zero bytes across all input reports as keystrokes.
Mouse movements, which send reports with changing non-zero bytes, will be
incorrectly interpreted as rapid keystrokes.
Will this corrupt the Welford variance tracking and cause false positive
anomaly detections?
> + was_active_ks += ((__u8)info->prev_report[i] + 255) >> 8;
> +
> + info->prev_report[i] = data[i];
> + }
> +
> + __u64 current_ms = bpf_ktime_get_ns() / HID_GUARD_NSEC_PER_MSEC;
> +
> + if (now_active_ks > was_active_ks) {
[Severity: High]
Does this heuristic fail to detect keys when no empty report is sent
between strokes?
The detection logic relies strictly on an increase in the total number of
non-zero bytes to identify a keypress.
Automated injection tools routinely optimize injections by morphing
keycodes directly without sending an intervening empty (all-zeros) report.
If an attacker's tool changes key 'A' to key 'B' in consecutive reports,
both reports have exactly one non-zero byte.
The condition 1 > 1 evaluates to false, causing the keystroke to be missed
and bypassing the variance check entirely.
[ ... ]
> +SEC("syscall")
> +int probe(struct hid_bpf_probe_args *ctx)
> +{
> + struct hid_rdesc_report *input;
> + struct hid_rdesc_field *field;
> + struct hid_rdesc_collection *col;
> +
> + hid_bpf_for_each_input_report(&HID_REPORT_DESCRIPTOR, input) {
> + __u32 size_in_bytes = (input->size_in_bits + 7) / 8;
> +
> + bpf_printk("Report size: %d\n", size_in_bytes);
> + if (input->report_id != 0)
> + size_in_bytes += 1;
[Severity: High]
Is the report ID needed here to validate incoming reports later?
The report ID is observed here but not stored in the map, making it
impossible for kdb_hook() to distinguish between keyboard reports and
other HID reports on a composite device.
> +
> + bpf_printk("Report size after report_id: %d\n", size_in_bytes);
> +
> + hid_bpf_for_each_field(input, field) {
> + hid_bpf_for_each_collection(field, col) {
> + if (col->usage_page ==
> + HidUsagePage_GenericDesktop &&
> + col->usage_id == HidUsage_GD_Keyboard) {
> + __u32 key = ctx->hid;
> + struct dev_info info = {
> + .connection_time =
> + bpf_ktime_get_ns(),
> + .count = 0,
> + .report_size = size_in_bytes
> + };
[Severity: High]
Will the uninitialized padding bytes in this struct cause the BPF verifier
to reject the program?
The dev_info object is initialized using a designated initializer list.
While this zero-initializes unspecified members, it does not guarantee that
the compiler will zero the 4 bytes of padding between report_size and
prev_report.
The BPF verifier strictly tracks initialization of stack memory at the byte
level. When bpf_map_update_elem() reads the struct from the stack, the
verifier will detect the uninitialized padding and reject the program with
an 'invalid indirect read from stack' error.
Should this structure be explicitly zeroed using __builtin_memset?
> + bpf_map_update_elem(&dev_details, &key,
> + &info, BPF_ANY);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260619073607.393248-1-krishgulati7@gmail.com?part=1
^ permalink raw reply
* [RFC PATCH] HID: BPF: add keyboard behavioral anomaly detection
From: krishgulati7 @ 2026-06-19 7:36 UTC (permalink / raw)
To: bentiss; +Cc: linux-input, linux-kernel, jikos, Krish Gulati
In-Reply-To: <adSxXidgeWF0-Ewn@beelink>
From: Krish Gulati <krishgulati7@gmail.com>
Implements a HID-BPF struct_ops program that detects behaviorally
anomalous keyboard input consistent with automated HID injection
attacks (USB Rubber Ducky and similar).
This is a direct response to Benjamin Tissoires's recommendation in
the hid-omg-detect review thread [1], where a kernel driver approach
was rejected on the grounds that a HID driver can only bind exclusively
to a device, racing with and displacing the legitimate driver. The
HID-BPF struct_ops approach resolves this: the program attaches
alongside the existing driver, receives the raw event stream before
kernel processing, and never takes exclusive ownership of the device.
Two independent detection signals are implemented:
Post-Enumeration Delay (PED):
Measures the delta between device connection (recorded at probe()
time via bpf_ktime_get_ns()) and the first input packet. Legitimate
users take hundreds of milliseconds to react after plugging in a
device; injection tools begin transmitting within 25-100ms. Events
are classified into three bands: SUSPICIOUS (<50ms), WARNING
(<300ms), and NORMAL (>=300ms). Thresholds are placeholder values
and MUST be empirically calibrated before production use.
Welford online variance (inter-keystroke timing):
Tracks keydown-to-keydown intervals per device using Welford's
single-pass online algorithm. BPF forbids floating-point, so the
running mean is maintained in fixed-point (scaled by 1024) to
prevent integer truncation at small counts. Intervals exceeding
10 seconds are treated as typing-session breaks and excluded from
the variance calculation. Detection fires after
HID_GUARD_MIN_SAMPLES (5) samples when variance falls below
HID_GUARD_VARIANCE_THRESH_MS2 (1600 ms^2), flagging suspiciously
metronomic timing inconsistent with human input.
Per-device state (Welford accumulators, PED timestamp, report size)
is maintained in a BPF_MAP_TYPE_LRU_HASH keyed by hid->id. The LRU
eviction policy prevents map exhaustion at the cost of two documented
trade-offs: eviction can silently reset Welford history (potential
map-flood attack) and may clear connection_time (PED blindspot on
device re-wake after eviction).
Detection results are currently surfaced via bpf_printk(). A
BPF_MAP_TYPE_RINGBUF interface with configurable userspace daemon
is planned; deferred pending validation of detection heuristics.
Signal design is grounded in: Neuner et al., "USBlock: Blocking
USB-based Keylogger Attacks", DBSec 2018.
[1] https://lore.kernel.org/linux-input/adSxXidgeWF0-Ewn@beelink/
Link: https://lore.kernel.org/linux-input/adSxXidgeWF0-Ewn@beelink/
Signed-off-by: Krish Gulati <krishgulati7@gmail.com>
---
src/bpf/testing/0010-Generic__keyboard.bpf.c | 285 +++++++++++++++++++
1 file changed, 285 insertions(+)
create mode 100644 src/bpf/testing/0010-Generic__keyboard.bpf.c
diff --git a/src/bpf/testing/0010-Generic__keyboard.bpf.c b/src/bpf/testing/0010-Generic__keyboard.bpf.c
new file mode 100644
index 0000000..9114587
--- /dev/null
+++ b/src/bpf/testing/0010-Generic__keyboard.bpf.c
@@ -0,0 +1,285 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include "hid_bpf.h"
+#include "hid_bpf_helpers.h"
+#include "hid_report_helpers.h"
+#include "hid_usages.h"
+#include "vmlinux.h"
+#include <bpf/bpf_tracing.h>
+
+#define HID_GUARD_NSEC_PER_MSEC 1000000ULL
+/*
+ * Post-enumeration delay thresholds (raw nanoseconds, matching
+ * connection_time and first_packet_time which are both __u64
+ * bpf_ktime_get_ns() values).
+ *
+ * LIMITATIONS: placeholder values based on rough figures from USB
+ * Rubber Ducky DETECT_READY research (modern hosts HID-ready in
+ * ~25-100ms) versus typical human reaction time after device
+ * connection (hundreds of ms to low seconds). NOT derived from a
+ * controlled study of this specific signal and MUST be empirically
+ * calibrated against real device/host combinations before any
+ * production use. Treat as a tunable, not a validated security
+ * boundary.
+ */
+#define HID_GUARD_PED_SUSPICIOUS_THRESH (50 * HID_GUARD_NSEC_PER_MSEC)
+#define HID_GUARD_PED_WARNING_THRESH (300 * HID_GUARD_NSEC_PER_MSEC)
+
+/*not derived from real typing data yet*/
+#define HID_GUARD_MIN_SAMPLES 5
+
+/*
+ * Variance is "too metronomic
+ * to be a human," expressed in ms^2 so we never need sqrt().
+ */
+#define HID_GUARD_VARIANCE_THRESH_MS2 (40ULL * 40ULL)
+
+/*
+ * Idle gaps this large are treated as a break in the typing session,
+ * not a sample — interval excluded from Welford, timer rebased.
+ * Borrowed threshold from hid-omg-detect (10s); not independently
+ * validated for this signal.
+ */
+#define HID_GUARD_IDLE_GAP_THRESH_MS (10ULL * 1000ULL)
+
+/*
+ * fixed-point factor: BPF has no floating values, so mean is stored as
+ * real_mean * HID_GUARD_WELFORD_SCALE.
+ * Without this, delta/count truncates to 0
+ * as soon as count exceeds the typical delta (in millisecond)
+ */
+#define HID_GUARD_WELFORD_SCALE 1024
+
+enum hid_guard_ped_flag {
+ HID_GUARD_PED_NO_ENTRY = 0,
+ HID_GUARD_PED_SUSPICIOUS = 1,
+ HID_GUARD_PED_WARNING = 2,
+ HID_GUARD_PED_NORMAL = 3,
+};
+
+struct dev_info {
+ __u64 connection_time;
+ __u32 report_size;
+ __u64 prev_report[32];
+ /*welford's variables*/
+ __u64 prev_keydown_ts;
+ __u64 count;
+ __s64 mean;
+ __u64 M2;
+ /*
+ * unsigned as delta * delta2 is always >= 0
+ * (as delta2 is delta scaled by the same sign)
+ */
+};
+
+/*
+ * BPF_MAP_TYPE_LRU_HASH:
+ * Using an LRU map automatically prevents exhaustion by silently evicting
+ * the oldest idle devices. It requires no syntax changes to the rest of the
+ * code (lookup/update helpers work identically), but introduces behavioral
+ * trade-offs:
+ *
+ * 1. Eviction wipes Welford variance history. An attacker
+ * could theoretically flood the map to flush their device and reset their
+ * score.
+ * 2. PED Blindspot: Eviction deletes the 'connection_time' set during probe().
+ * If an evicted device wakes up, it will bypass post-enumeration delay
+ * checks.
+ */
+struct {
+ __uint(type, BPF_MAP_TYPE_LRU_HASH);
+ __type(key, __u32);
+ __type(value, struct dev_info);
+ __uint(max_entries, 128);
+} dev_details SEC(".maps");
+
+/*
+ * POST ENUMERATION DELAY
+ *
+ * Computes the delta between the first input packet timestamp and
+ * connection_time (set at probe() time), both raw __u64 nanoseconds
+ * from bpf_ktime_get_ns(). Called once per device, the caller gates
+ * this on connection_time != 0, and this function clears it to 0
+ * afterward so it never runs twice for the same device.
+ *
+ * Returns a hid_guard_ped_flag classifying the delay.
+ */
+static __always_inline enum hid_guard_ped_flag
+post_enumeration_delay(struct dev_info *info, __u64 first_packet_time)
+{
+ __u64 delta;
+
+ if (first_packet_time < info->connection_time)
+ return HID_GUARD_PED_NO_ENTRY;
+
+ delta = first_packet_time - info->connection_time;
+
+ if (delta < HID_GUARD_PED_SUSPICIOUS_THRESH)
+ return HID_GUARD_PED_SUSPICIOUS;
+ if (delta < HID_GUARD_PED_WARNING_THRESH)
+ return HID_GUARD_PED_WARNING;
+
+ return HID_GUARD_PED_NORMAL;
+}
+
+static __always_inline void welford(struct dev_info *dev_state,
+ __u64 interval_ms)
+{
+ __s64 x = (__s64)interval_ms * HID_GUARD_WELFORD_SCALE;
+ __s64 delta, delta2, delta_abs, signed_div_res;
+ __u64 div_res;
+
+ dev_state->count++;
+ delta = x - dev_state->mean;
+ delta_abs = delta < 0 ? -delta : delta;
+ div_res = (__u64)delta_abs / (__u64)dev_state->count;
+ signed_div_res = delta < 0 ? -(__s64)div_res : (__s64)div_res;
+
+ dev_state->mean += signed_div_res;
+ delta2 = x - dev_state->mean;
+
+ dev_state->M2 += (__u64)(delta * delta2);
+
+ bpf_printk("W[Count:%llu] Int:%llu ms, scaled_x:%lld\n",
+ dev_state->count, interval_ms, x);
+ bpf_printk(" -> delta1:%lld, mean:%lld\n", delta, dev_state->mean);
+ bpf_printk(" -> delta2:%lld, M2:%llu\n", delta2, dev_state->M2);
+}
+
+HID_BPF_CONFIG(HID_DEVICE(BUS_USB, HID_GROUP_ANY, HID_VID_ANY, HID_PID_ANY),
+ HID_DEVICE(BUS_BLUETOOTH, HID_GROUP_ANY, HID_VID_ANY,
+ HID_PID_ANY));
+
+SEC(HID_BPF_DEVICE_EVENT)
+int BPF_PROG(kdb_hook, struct hid_bpf_ctx *hctx)
+{
+ __u32 hid_id = hctx->hid->id;
+
+ struct dev_info *info;
+
+ info = bpf_map_lookup_elem(&dev_details, &hid_id);
+
+ if (!info)
+ return 0;
+
+ __u32 size = info->report_size;
+ __u32 fetch_size;
+ __u8 *data;
+
+ if (size <= 8)
+ fetch_size = 8;
+ else if (size <= 16)
+ fetch_size = 16;
+ else
+ fetch_size = 32;
+
+ data = hid_bpf_get_data(hctx, 0, fetch_size);
+
+ if (!data)
+ return 0;
+ int now_active_ks = 0, was_active_ks = 0;
+#pragma unroll
+ for (int i = 0; i < 32; i++) {
+ if (i >= fetch_size)
+ break;
+ now_active_ks += ((__u8)data[i] + 255) >> 8;
+ was_active_ks += ((__u8)info->prev_report[i] + 255) >> 8;
+
+ info->prev_report[i] = data[i];
+ }
+
+ __u64 current_ms = bpf_ktime_get_ns() / HID_GUARD_NSEC_PER_MSEC;
+
+ if (now_active_ks > was_active_ks) {
+ if (info->prev_keydown_ts != 0) {
+ __u64 interval_ms = current_ms - info->prev_keydown_ts;
+
+ if (interval_ms < HID_GUARD_IDLE_GAP_THRESH_MS) {
+ welford(info, interval_ms);
+ } else {
+ bpf_printk(
+ "hid %d: idle gap %llu ms excluded from sample\n",
+ hid_id, interval_ms);
+ }
+ }
+
+ if (info->count >= HID_GUARD_MIN_SAMPLES) {
+ __u64 variance_m2 =
+ info->M2 /
+ ((__u64)HID_GUARD_WELFORD_SCALE *
+ HID_GUARD_WELFORD_SCALE * (info->count - 1));
+ /*
+ * the initial interval x was multiplied by HID_GUARD_WELFORD_SCALE, both
+ * delta and delta2 are also scaled by that factor,
+ * thus scale^2 in the denominator
+ */
+ if (variance_m2 < HID_GUARD_VARIANCE_THRESH_MS2) {
+ bpf_printk(
+ "hid %d: Suspeciously regular typing, variance=%llu ms^2\n",
+ hid_id, variance_m2);
+ }
+ }
+
+ info->prev_keydown_ts = current_ms;
+ }
+
+ if (info->connection_time != 0) {
+ enum hid_guard_ped_flag ped_flag =
+ post_enumeration_delay(info, bpf_ktime_get_ns());
+
+ bpf_printk("PED flag for hid %d: %d\n", hid_id, ped_flag);
+
+ /*
+ * Prevent re-evaluation on subsequent packets for this device
+ */
+ info->connection_time = 0;
+ }
+ return 0;
+}
+
+HID_BPF_OPS(hook_keyboard) = {
+ .hid_device_event = (void *)kdb_hook,
+};
+
+struct hid_rdesc_descriptor HID_REPORT_DESCRIPTOR;
+
+SEC("syscall")
+int probe(struct hid_bpf_probe_args *ctx)
+{
+ struct hid_rdesc_report *input;
+ struct hid_rdesc_field *field;
+ struct hid_rdesc_collection *col;
+
+ hid_bpf_for_each_input_report(&HID_REPORT_DESCRIPTOR, input) {
+ __u32 size_in_bytes = (input->size_in_bits + 7) / 8;
+
+ bpf_printk("Report size: %d\n", size_in_bytes);
+ if (input->report_id != 0)
+ size_in_bytes += 1;
+
+ bpf_printk("Report size after report_id: %d\n", size_in_bytes);
+
+ hid_bpf_for_each_field(input, field) {
+ hid_bpf_for_each_collection(field, col) {
+ if (col->usage_page ==
+ HidUsagePage_GenericDesktop &&
+ col->usage_id == HidUsage_GD_Keyboard) {
+ __u32 key = ctx->hid;
+ struct dev_info info = {
+ .connection_time =
+ bpf_ktime_get_ns(),
+ .count = 0,
+ .report_size = size_in_bytes
+ };
+ bpf_map_update_elem(&dev_details, &key,
+ &info, BPF_ANY);
+ ctx->retval = 0;
+ return 0;
+ }
+ }
+ }
+ }
+ ctx->retval = -EINVAL;
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
--
2.54.0
^ permalink raw reply related
* Re: [PATCH 1/4] dt-bindings: input: Add binding for Qualcomm SPMI PMIC haptics
From: Krzysztof Kozlowski @ 2026-06-19 4:18 UTC (permalink / raw)
To: Fenglin Wu
Cc: linux-arm-msm, Dmitry Torokhov, 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: <c7191868-d141-4095-b703-230237721f28@oss.qualcomm.com>
On 17/06/2026 13:02, Fenglin Wu wrote:
>
> On 6/17/2026 6:35 PM, Krzysztof Kozlowski wrote:
>> On Tue, Jun 16, 2026 at 03:08:24AM -0700, Fenglin Wu wrote:
>>> ....
>>> +
>>> + qcom,lra-period-us:
>>> + description:
>>> + LRA actuator initial resonance period in microseconds
>>> + (1,000,000 / resonant_freq_hz). Used to configure T_LRA-based play
>>> + rates and the auto-resonance zero-crossing window.
>> This does not feel like static characteristic. Isn't period depending on
>> intensity of vibration you want to have? Why would that be fixed per
>> board?
>
> This period is specifically used for playbacks that require
> auto-resonance to be enabled, which I referred to as "T_LRA-based" and
> "auto-resonance zero-crossing window." It plays a key role in the
> "DIRECT_PLAY" mode, which produces a constant vibration effect. To
> adjust the vibration intensity during this constant effect, the hardware
> does it by scaling the peak voltage of the driver signals, rather than
> changing the frequency.
But maybe changing frequency runtime still would be useful?
What does the frequency depend on? IOW, how do you choose the value here
- based on some motor-part characteristics?
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v5 2/3] HID: nintendo: Add rumble support for Switch 2 controllers
From: Vicki Pfau @ 2026-06-19 4:17 UTC (permalink / raw)
To: sashiko-reviews; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260514005353.EF8D8C19425@smtp.kernel.org>
Let's tackle some more hallucinations.
On 5/13/26 5:53 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] Mixing jiffies and milliseconds in delayed work scheduling logic causes an integer underflow, permanently stalling the haptic rumble.
> - [High] A guaranteed NULL pointer dereference occurs in the error logging path of switch2_rumble_work, as well as a TOCTOU race condition.
> - [Medium] `ns2->ctlr_type` is checked before it is dynamically initialized, causing union corruption for GameCube controllers.
> - [Low] Implicit type promotion in the max() macro causes a compiler warning.
> - [Low] Kconfig documentation claims individual motor control for the Pro Controller, but the implementation identicalizes the rumble data for both motors.
> --
>
> commit d72f0f8e0dadde2cacc27050b92088c2e5cbea0e
> Author: Vicki Pfau <vi@endrift.com>
> Subject: HID: nintendo: Add rumble support for Switch 2 controllers
>
> This commit introduces rumble support for Switch 2 controllers, handling both HD
> Rumble linear resonant actuators and eccentric rotating mass haptics. It adds
> state machines for rumble scheduling and output reporting via hdev.
>
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index 26ef589f652c6..a7fe74d982475 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -850,10 +850,10 @@ config NINTENDO_FF
>> depends on HID_NINTENDO
>> select INPUT_FF_MEMLESS
>> help
>> - Say Y here if you have a Nintendo Switch controller and want to enable
>> - force feedback support for it. This works for both joy-cons, the pro
>> - controller, and the NSO N64 controller. For the pro controller, both
>> - rumble motors can be controlled individually.
>> + Say Y here if you have a Nintendo Switch or Switch 2 controller and want
>> + to enable force feedback support for it. This works for Joy-Cons, the Pro
>> + Controllers, and the NSO N64 and GameCube controller. For the Pro
>> + Controller, both rumble motors can be controlled individually.
>
> This isn't a bug, but is the help text accurate regarding individual motor
> control? The implementation in switch2_rumble_work() appears to encode the
> exact same rumble configuration to both the left and right motor offsets,
> which would make both motors vibrate identically.
I didn't add this text, so I'm ingoring this.
>
>> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
>> index 2193afcdefabc..9801799a90758 100644
>> --- a/drivers/hid/hid-nintendo.c
>> +++ b/drivers/hid/hid-nintendo.c
>
> [ ... ]
>
>> @@ -3103,6 +3155,107 @@ static inline bool switch2_ctlr_is_joycon(enum switch2_ctlr_type type)
>> return type == NS2_CTLR_TYPE_JCL || type == NS2_CTLR_TYPE_JCR;
>> }
>>
>> +#if IS_ENABLED(CONFIG_NINTENDO_FF)
>> +static void switch2_encode_rumble(struct switch2_hd_rumble *rumble, uint8_t buffer[5])
>> +{
>> + buffer[0] = rumble->hi_freq;
>> + buffer[1] = (rumble->hi_freq >> 8) | (rumble->hi_amp << 2);
>> + buffer[2] = (rumble->hi_amp >> 6) | (rumble->lo_freq << 4);
>> + buffer[3] = (rumble->lo_freq >> 4) | (rumble->lo_amp << 6);
>> + buffer[4] = rumble->lo_amp >> 2;
>> +}
>> +
>> +static int switch2_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect)
>> +{
>> + struct switch2_controller *ns2 = input_get_drvdata(dev);
>> +
>> + if (effect->type != FF_RUMBLE)
>> + return 0;
>> +
>> + guard(spinlock_irqsave)(&ns2->rumble_lock);
>> + if (ns2->ctlr_type == NS2_CTLR_TYPE_GC) {
>> + ns2->rumble.sd.amplitude = max(effect->u.rumble.strong_magnitude,
>> + effect->u.rumble.weak_magnitude >> 1);
>
> Will this max calculation trigger a compiler warning? The strict type checking
> in the max macro might fail because effect->u.rumble.weak_magnitude >> 1
> promotes to an int, while strong_magnitude is a u16.
It doesn't cause a warning locally, but my warnings seem lax for some reason. I'm not sure how to fix that, but I guess adding a cast here is harmless.
>
>> + } else {
>> + ns2->rumble.hd.hi_amp = effect->u.rumble.weak_magnitude * RUMBLE_MAX >> 16;
>> + ns2->rumble.hd.lo_amp = effect->u.rumble.strong_magnitude * RUMBLE_MAX >> 16;
>> + }
>> +
>> + if (ns2->hdev)
>> + schedule_delayed_work(&ns2->rumble_work, 0);
>> +
>> + return 0;
>> +}
>> +
>> +static void switch2_rumble_work(struct work_struct *work)
>> +{
>> + struct switch2_controller *ns2 = container_of(to_delayed_work(work),
>> + struct switch2_controller, rumble_work);
>> + unsigned long current_ms = jiffies_to_msecs(get_jiffies_64());
>> + unsigned long flags;
>> + bool active;
>> + int ret;
>
> [ ... ]
>
>> + if (active) {
>> + unsigned long interval = msecs_to_jiffies(4);
>> +
>> + if (!ns2->last_rumble_work)
>> + ns2->last_rumble_work = current_ms;
>> + else
>> + ns2->last_rumble_work += interval;
>> + schedule_delayed_work(&ns2->rumble_work,
>> + ns2->last_rumble_work + interval - current_ms);
>
> Does this code underflow the scheduled delay?
>
> The current_ms variable is retrieved in milliseconds using jiffies_to_msecs(),
> but interval is calculated in jiffies using msecs_to_jiffies(). Since
> ns2->last_rumble_work is initialized to milliseconds but incremented by
> jiffies, subtracting current_ms could yield a negative value.
>
> On systems where HZ is not 1000, could this underflow the delay parameter and
> schedule the next work millions of jiffies in the future?
Fixing in v6.
>
>> + } else {
>> + ns2->last_rumble_work = 0;
>> + }
>> + spin_unlock_irqrestore(&ns2->rumble_lock, flags);
>> +
>> + if (!ns2->hdev) {
>> + cancel_delayed_work(&ns2->rumble_work);
>> + ret = -ENODEV;
>> + } else {
>> + ret = hid_hw_output_report(ns2->hdev, ns2->rumble_buffer, 64);
>> + }
>> +
>> + if (ret < 0)
>> + hid_dbg(ns2->hdev, "Failed to send output report ret=%d\n", ret);
>
> If ns2->hdev is NULL, we set ret = -ENODEV and then reach this hid_dbg call.
> Could this result in a NULL pointer dereference, as the hid_dbg macro passes
> ns2->hdev to dev_dbg()?
Fixing in v6
>
> Also, since ns2->hdev is accessed locklessly and without READ_ONCE(), is there
> a time-of-check to time-of-use race where the compiler might reload the pointer
> between the check and hid_hw_output_report()?
This can only get changed in switch2_remove, which is missing a cancel_delayed_work_sync. Fixing in v6.
>
> [ ... ]
>
>> @@ -3881,6 +4048,14 @@ static int switch2_probe(struct hid_device *hdev, const struct hid_device_id *id
>>
>> switch2_leds_create(ns2);
>>
>> +#if IS_ENABLED(CONFIG_NINTENDO_FF)
>> + if (ns2->ctlr_type != NS2_CTLR_TYPE_GC) {
>> + ns2->rumble.hd.hi_freq = RUMBLE_HI_FREQ;
>> + ns2->rumble.hd.lo_freq = RUMBLE_LO_FREQ;
>> + }
>
> Are we checking ns2->ctlr_type before it is dynamically initialized?
>
> During probe, ns2->ctlr_type appears to be 0 since the struct is freshly
> allocated and the type is discovered later during the init state machine.
> This would cause the high and low frequencies to always be written.
>
> Since hd and sd share a union, could this corrupt the sd.error and
> sd.amplitude fields for actual GameCube controllers?
Really, really unimportant and technically "works by accident" territory but I'll move the logic anyway.
>
>> + spin_lock_init(&ns2->rumble_lock);
>> + INIT_DELAYED_WORK(&ns2->rumble_work, switch2_rumble_work);
>> +#endif
>> hid_set_drvdata(hdev, ns2);
>
^ permalink raw reply
* Re: [PATCH v5 1/3] HID: nintendo: Add preliminary Switch 2 controller driver
From: Vicki Pfau @ 2026-06-19 3:56 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260514001510.964E7C19425@smtp.kernel.org>
Guess I'm doing LLM review replies now.
On 5/13/26 5:15 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
> - [High] Sleeping in atomic context and AB-BA deadlock in `switch2_usb_disconnect`
> - [High] Race condition in IN URB tracking causes permanent starvation
> - [High] Dangling pointers and use-after-free on probe failure
> - [High] Memory corruption and double free due to incorrect devm input lifecycle management
> - [High] NULL pointer dereference in USB probe error path
> - [High] Division by zero crash from unvalidated trigger calibration data
> - [High] BUG_ON kernel panic when `ida_free` is called with U32_MAX
> - [High] Out-of-bounds read when parsing NS2_CMD_FW_INFO response
> - [High] Race condition due to URB submission before initialization of synchronization primitives
> - [Medium] Infinite USB retry storm on protocol errors
> - [Medium] Unaligned memory access on byte array casts
> --
>
> commit 00cb7265ad5983144c6923702e2ee2c3c047d29e
> Author: Vicki Pfau <vi@endrift.com>
> Subject: HID: nintendo: Add preliminary Switch 2 controller driver
>
> This adds a new driver for the Switch 2 controllers. The Switch 2 uses an
> unusual split-interface design such that input and rumble occur on the main
> HID interface, but all other communication occurs over a "configuration"
> interface.
>
>> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
>> index 29008c2cc5304..2193afcdefabc 100644
>> --- a/drivers/hid/hid-nintendo.c
>> +++ b/drivers/hid/hid-nintendo.c
> [ ... ]
>> +static int switch2_init_input(struct switch2_controller *ns2)
>> +{
> [ ... ]
>> + input = devm_input_allocate_device(&hdev->dev);
>> + if (!input)
>> + return -ENOMEM;
> [ ... ]
>> + ret = input_register_device(input);
>> + if (ret < 0) {
>> + hid_err(ns2->hdev, "Failed to register input; ret=%d\n", ret);
>> + input_free_device(input);
>
> Calling input_free_device or input_unregister_device on a devm-managed
> input device can cause double frees when devres cleanup fires. Can this
> lead to memory corruption?
False positive. The documentation clearly states there is no devm equivalent and input_unregister_device works fine on devm-managed devices. The source of input_free_device also clearly shows logic for handling devm, and the source of input_device_unregister calls into a static devm_input_device_unregister if needed.
>
>> + return ret;
>> + }
> [ ... ]
>> +static void switch2_controller_put(struct switch2_controller *ns2)
>> +{
>> + struct input_dev *input;
>> + bool do_free;
> [ ... ]
>> + if (input)
>> + input_unregister_device(input);
>
> Calling input_unregister_device on a devm-managed input device can cause
> issues. If the input device remains alive (tied to the HID device devres)
> and its child LEDs are not released, will reconnecting the USB cable
> allocate a new input device and corrupt the LED lists by re-registering
> the exact same ns2->leds structures?
Same false positive
>
>> +
>> + if (do_free) {
> [ ... ]
>> +static void switch2_report_trigger(struct input_dev *input, uint8_t zero, int abs, uint8_t data)
>> +{
>> + int value = (NS2_TRIGGER_RANGE + 1) * (data - zero) / (232 - zero);
>
> The zero variable is loaded directly from the controller's factory calibration
> flash. If a device reports a zero point of exactly 232, will this trigger a
> division by zero exception and crash the kernel?
Ok, an actual finding. I'll have this fixed in v6.
>
>> +
>> + input_report_abs(input, abs, clamp(value, 0, NS2_TRIGGER_RANGE));
>> +}
> [ ... ]
>> +static int switch2_read_flash(struct switch2_controller *ns2, uint32_t address,
>> + uint8_t size)
>> +{
>> + uint8_t message[8] = { size, 0x7e };
>> +
>> + if (!ns2->cfg)
>> + return -ENOTCONN;
>> + *(__le32 *)&message[4] = __cpu_to_le32(address);
>
> The driver directly casts &message[4] to __le32. Since message is a uint8_t
> array, it is only guaranteed to be 1-byte aligned. On strict architectures,
> will this unaligned memory write trigger a hardware alignment fault?
Good catch. Fixed in v6.
>
>> + return ns2->cfg->send_command(NS2_CMD_FLASH, NS2_SUBCMD_FLASH_READ, message,
>> + sizeof(message), ns2->cfg);
>> +}
> [ ... ]
>> +int switch2_receive_command(struct switch2_controller *ns2,
>> + const uint8_t *message, size_t length)
>> +{
> [ ... ]
>> + header = (const struct switch2_cmd_header *)message;
>> + if (!(header->flags & NS2_FLAG_OK)) {
>> + ret = -EIO;
>> + goto exit;
>> + }
>> + message = &message[8];
>> + switch (header->command) {
> [ ... ]
>> + case NS2_CMD_FW_INFO:
>> + if (header->subcommand == NS2_SUBCMD_FW_INFO_GET) {
>> + if (length < sizeof(ns2->version)) {
>> + ret = -EINVAL;
>> + goto exit;
>> + }
>> + memcpy(&ns2->version, message, sizeof(ns2->version));
>
> The message pointer is advanced by 8 bytes to skip the header, but length
> is not decremented. The bounds check compares length against
> sizeof(ns2->version). If length is exactly sizeof(ns2->version), the check
> passes, but the advanced message pointer only has length - 8 valid bytes
> remaining. Will the memcpy over-read into uninitialized memory?
Fixed in v6.
>
>> + ns2->ctlr_type = ns2->version.ctlr_type;
>> + switch2_init_step_done(ns2, NS2_INIT_GET_FIRMWARE_INFO);
>> + }
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> +exit:
>> + if (ns2->init_step < NS2_INIT_DONE)
>> + switch2_init_controller(ns2);
>
> If a command response has the NACK flag set or encounters another error,
> the code jumps to exit and invokes switch2_init_controller. Since
> ns2->init_step has not advanced, the state machine will immediately re-send
> the exact same command. Can this cause an infinite tight feedback loop of
> failures and retries, flooding the USB bus?
This doesn't happen for problems already described in the thread previously.
>
>> +
>> + return ret;
>> +}
> [ ... ]
>> +int switch2_controller_attach_cfg(const char *phys, struct switch2_cfg_intf *cfg)
>> +{
> [ ... ]
>> + guard(mutex)(&ns2->lock);
>> + WARN_ON(ns2->cfg);
>> + ns2->cfg = cfg;
>> +
>> + if (ns2->hdev)
>> + return switch2_init_controller(ns2);
>
> If switch2_init_controller fails here or in switch2_probe, the error is
> returned immediately without unrolling the state. Will ns2->cfg or ns2->hdev
> remain set, potentially leading to a use-after-free when subsequent device
> events occur?
This is a good catch. I'm not sure what the right fix is though; obviously a half-configured state is bad, but I'm not sure a full teardown is the best approach; it can leave it in a weird state, even if it's not a UAF risk. I'll go with a full teardown for now at least.
>
>> + return 0;
>> +}
> [ ... ]
>> +static void switch2_remove(struct hid_device *hdev)
>> +{
>> + struct switch2_controller *ns2 = hid_get_drvdata(hdev);
>> +
>> + hid_hw_close(hdev);
>> + mutex_lock(&ns2->lock);
>> + WARN_ON(ns2->hdev != hdev);
>> + ns2->hdev = NULL;
>> + mutex_unlock(&ns2->lock);
>> + ida_free(&nintendo_player_id_allocator, ns2->player_id);
>
> If ida_alloc fails during probe, ns2->player_id remains at its default
> value of U32_MAX. When ida_free is called, the IDA framework enforces a
> check on the ID. Casting U32_MAX to a signed integer yields -1. Will this
> trigger the BUG_ON and cause a kernel panic?
I can't find any evidence of such a BUG_ON. A negative number will just early return. This seems like a hallucination.
>
>> + switch2_controller_put(ns2);
>> + hid_hw_stop(hdev);
>> +}
> [ ... ]
>> diff --git a/drivers/input/joystick/nintendo-switch2-usb.c b/drivers/input/joystick/nintendo-switch2-usb.c
>> new file mode 100644
>> index 0000000000000..ebd89d852e21a
>> --- /dev/null
>> +++ b/drivers/input/joystick/nintendo-switch2-usb.c
> [ ... ]
>> +static void switch2_bulk_in(struct urb *urb)
>> +{
> [ ... ]
>> + spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
>> + for (i = 0; i < NS2_IN_URBS; i++) {
>> + int err;
>> + struct switch2_urb *ns2_urb;
>> +
>> + if (ns2_usb->bulk_in[i].urb == urb) {
>> + ns2_usb->message_in = i;
>
> If multiple URBs complete in quick succession before the work item executes,
> the message_in index is overwritten. Will this cause the work item to only
> process the last completed URB, leaving earlier URBs indefinitely active
> and leading to starvation of the URB pool?
>
In practice this probably won't happen but I guess it's worth fixing.
>> + continue;
>> + }
> [ ... ]
>> +static int switch2_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
>> +{
> [ ... ]
>> + ns2_usb->bulk_in[0].active = true;
>> + ret = usb_submit_urb(ns2_usb->bulk_in[0].urb, GFP_ATOMIC);
>> + if (ret < 0)
>> + goto err_free_out;
>> +
>> + init_usb_anchor(&ns2_usb->bulk_out_anchor);
>> + spin_lock_init(&ns2_usb->bulk_out_lock);
>> + init_usb_anchor(&ns2_usb->bulk_in_anchor);
>> + spin_lock_init(&ns2_usb->bulk_in_lock);
>> + INIT_WORK(&ns2_usb->message_in_work, switch2_usb_message_in_work);
>
> usb_submit_urb is invoked for bulk_in[0] before the initialization of the
> spinlocks, anchors, and work struct. If the URB completes immediately, the
> completion handler will execute and attempt to use uninitialized
> synchronization primitives. Can this cause a crash?
I don't know if this is possible but I guess I'll fix it.
>
>> +
>> + usb_set_intfdata(intf, ns2_usb);
> [ ... ]
>> +err_kill_urb:
>> + usb_kill_urb(ns2_usb->bulk_in[0].urb);
>> +err_free_out:
>> + for (i = 0; i < NS2_OUT_URBS; i++) {
>> + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_out[i].data,
>> + ns2_usb->bulk_out[i].urb->transfer_dma);
>> + usb_free_urb(ns2_usb->bulk_out[i].urb);
>> + }
>> +err_free_in:
>> + for (i = 0; i < NS2_IN_URBS; i++) {
>> + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_in[i].data,
>> + ns2_usb->bulk_in[i].urb->transfer_dma);
>
> If usb_alloc_urb fails during the loop, it jumps to err_free_in. During
> cleanup, it accesses ns2_usb->bulk_in[i].urb->transfer_dma. Since urb is
> NULL for the failed index, will this trigger a NULL pointer dereference?
I removed the NULL check due to checkpatch telling me I didn't need it but I guess I'll put it back.
>
>> + usb_free_urb(ns2_usb->bulk_in[i].urb);
>> + }
>> + devm_kfree(&intf->dev, ns2_usb);
>> +
>> + return ret;
>> +}
>> +
>> +static void switch2_usb_disconnect(struct usb_interface *intf)
>> +{
>> + struct switch2_usb *ns2_usb = usb_get_intfdata(intf);
>> + unsigned long flags;
>> + int i;
>> +
>> + spin_lock_irqsave(&ns2_usb->bulk_out_lock, flags);
>> + usb_kill_anchored_urbs(&ns2_usb->bulk_out_anchor);
>
> Both usb_kill_anchored_urbs and cancel_work_sync can sleep. Is it safe to
> call them while holding a spinlock?
Fixing in v6.
>
>> + for (i = 0; i < NS2_OUT_URBS; i++) {
>> + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_out[i].data,
>> + ns2_usb->bulk_out[i].urb->transfer_dma);
>> + usb_free_urb(ns2_usb->bulk_out[i].urb);
>> + }
>> + spin_unlock_irqrestore(&ns2_usb->bulk_out_lock, flags);
>> +
>> + spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
>> + usb_kill_anchored_urbs(&ns2_usb->bulk_in_anchor);
>> + cancel_work_sync(&ns2_usb->message_in_work);
>
> Furthermore, cancel_work_sync waits for message_in_work which attempts to
> acquire bulk_in_lock. Does this cause a deadlock since the lock is already
> held by the disconnecting thread?
Likewise.
>
>> + for (i = 0; i < NS2_IN_URBS; i++) {
>
^ permalink raw reply
* Re: [PATCH v5 1/3] HID: nintendo: Add preliminary Switch 2 controller driver
From: Vicki Pfau @ 2026-06-19 4:02 UTC (permalink / raw)
To: Silvan Jegen, sashiko-reviews; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <25DJXAMCCCNLA.234CWZVQ8IOM4@homearch.localdomain>
On 5/23/26 1:57 AM, Silvan Jegen wrote:
> Heyhey!
>
> Below some comments regarding the findings (note that I am not an expert
> on neither Linux kernel drivers in general nor USB HID specifically).
>
> sashiko-bot@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
>> - [High] Sleeping in atomic context and AB-BA deadlock in `switch2_usb_disconnect`
>> - [High] Race condition in IN URB tracking causes permanent starvation
>> - [High] Dangling pointers and use-after-free on probe failure
>> - [High] Memory corruption and double free due to incorrect devm input lifecycle management
>> - [High] NULL pointer dereference in USB probe error path
>> - [High] Division by zero crash from unvalidated trigger calibration data
>> - [High] BUG_ON kernel panic when `ida_free` is called with U32_MAX
>> - [High] Out-of-bounds read when parsing NS2_CMD_FW_INFO response
>> - [High] Race condition due to URB submission before initialization of synchronization primitives
>> - [Medium] Infinite USB retry storm on protocol errors
>> - [Medium] Unaligned memory access on byte array casts
>> --
>>
>> commit 00cb7265ad5983144c6923702e2ee2c3c047d29e
>> Author: Vicki Pfau <vi@endrift.com>
>> Subject: HID: nintendo: Add preliminary Switch 2 controller driver
>>
>> This adds a new driver for the Switch 2 controllers. The Switch 2 uses an
>> unusual split-interface design such that input and rumble occur on the main
>> HID interface, but all other communication occurs over a "configuration"
>> interface.
>>
>>> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
>>> index 29008c2cc5304..2193afcdefabc 100644
>>> --- a/drivers/hid/hid-nintendo.c
>>> +++ b/drivers/hid/hid-nintendo.c
>> [ ... ]
>>> +static int switch2_init_input(struct switch2_controller *ns2)
>>> +{
>> [ ... ]
>>> + input = devm_input_allocate_device(&hdev->dev);
>>> + if (!input)
>>> + return -ENOMEM;
>> [ ... ]
>>> + ret = input_register_device(input);
>>> + if (ret < 0) {
>>> + hid_err(ns2->hdev, "Failed to register input; ret=%d\n", ret);
>>> + input_free_device(input);
>>
>> Calling input_free_device or input_unregister_device on a devm-managed
>> input device can cause double frees when devres cleanup fires. Can this
>> lead to memory corruption?
>
> I think sashiko is actually correct here, unfortunately. The documentation
> for devm_input_allocate_device states that
>
> Managed input devices do not need to be explicitly unregistered or
> freed as it will be done automatically when owner device unbinds from
> its driver (or binding fails). ...
>
> Looks like we can remove all the input_unregister_device and
> input_free_device calls here.
sashiko is not correct. See my previous reply.
>
>>
>>> + return ret;
>>> + }
>> [ ... ]
>>> +static void switch2_controller_put(struct switch2_controller *ns2)
>>> +{
>>> + struct input_dev *input;
>>> + bool do_free;
>> [ ... ]
>>> + if (input)
>>> + input_unregister_device(input);
>>
>> Calling input_unregister_device on a devm-managed input device can cause
>> issues. If the input device remains alive (tied to the HID device devres)
>> and its child LEDs are not released, will reconnecting the USB cable
>> allocate a new input device and corrupt the LED lists by re-registering
>> the exact same ns2->leds structures?
>
> This input_unregister_device can also be removed.
>
>>
>>> +
>>> + if (do_free) {
>> [ ... ]
>>> +static void switch2_report_trigger(struct input_dev *input, uint8_t zero, int abs, uint8_t data)
>>> +{
>>> + int value = (NS2_TRIGGER_RANGE + 1) * (data - zero) / (232 - zero);
>>
>> The zero variable is loaded directly from the controller's factory calibration
>> flash. If a device reports a zero point of exactly 232, will this trigger a
>> division by zero exception and crash the kernel?
>
> Not sure this is an issue in practice, but we might want to protect the
> code in any case by doing something like the below perhaps (untested).
>
> int value = (NS2_TRIGGER_RANGE + 1) * (data - zero) / max_t(int, (232 - zero), 1);
>
>
It won't be an issue in practice but it's harmless to fix.
>>
>>> +
>>> + input_report_abs(input, abs, clamp(value, 0, NS2_TRIGGER_RANGE));
>>> +}
>> [ ... ]
>>> +static int switch2_read_flash(struct switch2_controller *ns2, uint32_t address,
>>> + uint8_t size)
>>> +{
>>> + uint8_t message[8] = { size, 0x7e };
>>> +
>>> + if (!ns2->cfg)
>>> + return -ENOTCONN;
>>> + *(__le32 *)&message[4] = __cpu_to_le32(address);
>>
>> The driver directly casts &message[4] to __le32. Since message is a uint8_t
>> array, it is only guaranteed to be 1-byte aligned. On strict architectures,
>> will this unaligned memory write trigger a hardware alignment fault?
>
> I assume this could be an issue and we should just use something like
> the below (only compile-tested).
>
> put_unaligned_le32(__cpu_to_le32(address), &message[4]);
>
Correct.
>
>>
>>> + return ns2->cfg->send_command(NS2_CMD_FLASH, NS2_SUBCMD_FLASH_READ, message,
>>> + sizeof(message), ns2->cfg);
>>> +}
>> [ ... ]
>>> +int switch2_receive_command(struct switch2_controller *ns2,
>>> + const uint8_t *message, size_t length)
>>> +{
>> [ ... ]
>>> + header = (const struct switch2_cmd_header *)message;
>>> + if (!(header->flags & NS2_FLAG_OK)) {
>>> + ret = -EIO;
>>> + goto exit;
>>> + }
>>> + message = &message[8];
>>> + switch (header->command) {
>> [ ... ]
>>> + case NS2_CMD_FW_INFO:
>>> + if (header->subcommand == NS2_SUBCMD_FW_INFO_GET) {
>>> + if (length < sizeof(ns2->version)) {
>>> + ret = -EINVAL;
>>> + goto exit;
>>> + }
>>> + memcpy(&ns2->version, message, sizeof(ns2->version));
>>
>> The message pointer is advanced by 8 bytes to skip the header, but length
>> is not decremented. The bounds check compares length against
>> sizeof(ns2->version). If length is exactly sizeof(ns2->version), the check
>> passes, but the advanced message pointer only has length - 8 valid bytes
>> remaining. Will the memcpy over-read into uninitialized memory?
>
> Sounds like something that we should fix by either decreasing length or
> increasing the size in the check.
>
>>
>>> + ns2->ctlr_type = ns2->version.ctlr_type;
>>> + switch2_init_step_done(ns2, NS2_INIT_GET_FIRMWARE_INFO);
>>> + }
>>> + break;
>>> + default:
>>> + break;
>>> + }
>>> +
>>> +exit:
>>> + if (ns2->init_step < NS2_INIT_DONE)
>>> + switch2_init_controller(ns2);
>>
>> If a command response has the NACK flag set or encounters another error,
>> the code jumps to exit and invokes switch2_init_controller. Since
>> ns2->init_step has not advanced, the state machine will immediately re-send
>> the exact same command. Can this cause an infinite tight feedback loop of
>> failures and retries, flooding the USB bus?
>
> We have discussed this before and it doesn't seem to be an issue in
> practice.
>
> If we want to be on the safe side, we could pass a retry count and abort
> on too many error retries, perhaps?
I had considered a retry count but getting it to work properly was fiddly. This should only go wrong if there's a malicious device attached, as in practice the Switch 2 controllers won't do this, and bus abuse is trivial with a malicious device regardless.
>
>
>>> +
>>> + return ret;
>>> +}
>> [ ... ]
>>> +int switch2_controller_attach_cfg(const char *phys, struct switch2_cfg_intf *cfg)
>>> +{
>> [ ... ]
>>> + guard(mutex)(&ns2->lock);
>>> + WARN_ON(ns2->cfg);
>>> + ns2->cfg = cfg;
>>> +
>>> + if (ns2->hdev)
>>> + return switch2_init_controller(ns2);
>>
>> If switch2_init_controller fails here or in switch2_probe, the error is
>> returned immediately without unrolling the state. Will ns2->cfg or ns2->hdev
>> remain set, potentially leading to a use-after-free when subsequent device
>> events occur?
>
> AFAICT, since we are always checking for both ns2->cfg and ns2->hdev
> before doing any processing in switch2_init_controller, there should
> never be a use-after-free
>
> It *is* true that we don't call hid_hw_{close,stop} in switch2_probe,
> if the switch2_init_controller call fails though. It might make sense
> to return a specific error and use it to check whether we want to give
> up and call these?
The problem was that it will return an error after setting ns2->cfg, which causes the caller to free it without removing it. That's a real bug that can cause UAF.
>
>>
>>> + return 0;
>>> +}
>> [ ... ]
>>> +static void switch2_remove(struct hid_device *hdev)
>>> +{
>>> + struct switch2_controller *ns2 = hid_get_drvdata(hdev);
>>> +
>>> + hid_hw_close(hdev);
>>> + mutex_lock(&ns2->lock);
>>> + WARN_ON(ns2->hdev != hdev);
>>> + ns2->hdev = NULL;
>>> + mutex_unlock(&ns2->lock);
>>> + ida_free(&nintendo_player_id_allocator, ns2->player_id);
>>
>> If ida_alloc fails during probe, ns2->player_id remains at its default
>> value of U32_MAX. When ida_free is called, the IDA framework enforces a
>> check on the ID. Casting U32_MAX to a signed integer yields -1. Will this
>> trigger the BUG_ON and cause a kernel panic?
>
> ida_free seems to return early on
>
> if ((int)id < 0)
> return;
>
> so that should be fine as is.
>
>>
>>> + switch2_controller_put(ns2);
>>> + hid_hw_stop(hdev);
>>> +}
>> [ ... ]
>>> diff --git a/drivers/input/joystick/nintendo-switch2-usb.c b/drivers/input/joystick/nintendo-switch2-usb.c
>>> new file mode 100644
>>> index 0000000000000..ebd89d852e21a
>>> --- /dev/null
>>> +++ b/drivers/input/joystick/nintendo-switch2-usb.c
>> [ ... ]
>>> +static void switch2_bulk_in(struct urb *urb)
>>> +{
>> [ ... ]
>>> + spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
>>> + for (i = 0; i < NS2_IN_URBS; i++) {
>>> + int err;
>>> + struct switch2_urb *ns2_urb;
>>> +
>>> + if (ns2_usb->bulk_in[i].urb == urb) {
>>> + ns2_usb->message_in = i;
>>
>> If multiple URBs complete in quick succession before the work item executes,
>> the message_in index is overwritten. Will this cause the work item to only
>> process the last completed URB, leaving earlier URBs indefinitely active
>> and leading to starvation of the URB pool?
>>
>>> + continue;
>>> + }
>> [ ... ]
>>> +static int switch2_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
>>> +{
>> [ ... ]
>>> + ns2_usb->bulk_in[0].active = true;
>>> + ret = usb_submit_urb(ns2_usb->bulk_in[0].urb, GFP_ATOMIC);
>>> + if (ret < 0)
>>> + goto err_free_out;
>>> +
>>> + init_usb_anchor(&ns2_usb->bulk_out_anchor);
>>> + spin_lock_init(&ns2_usb->bulk_out_lock);
>>> + init_usb_anchor(&ns2_usb->bulk_in_anchor);
>>> + spin_lock_init(&ns2_usb->bulk_in_lock);
>>> + INIT_WORK(&ns2_usb->message_in_work, switch2_usb_message_in_work);
>>
>> usb_submit_urb is invoked for bulk_in[0] before the initialization of the
>> spinlocks, anchors, and work struct. If the URB completes immediately, the
>> completion handler will execute and attempt to use uninitialized
>> synchronization primitives. Can this cause a crash?
>
> As the documentation for usb_submit_urb mentions that the call is async,
> I assume that this is indeed a theoretical possibility.
>
>>
>>> +
>>> + usb_set_intfdata(intf, ns2_usb);
>> [ ... ]
>>> +err_kill_urb:
>>> + usb_kill_urb(ns2_usb->bulk_in[0].urb);
>>> +err_free_out:
>>> + for (i = 0; i < NS2_OUT_URBS; i++) {
>>> + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_out[i].data,
>>> + ns2_usb->bulk_out[i].urb->transfer_dma);
>>> + usb_free_urb(ns2_usb->bulk_out[i].urb);
>>> + }
>>> +err_free_in:
>>> + for (i = 0; i < NS2_IN_URBS; i++) {
>>> + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_in[i].data,
>>> + ns2_usb->bulk_in[i].urb->transfer_dma);
>>
>> If usb_alloc_urb fails during the loop, it jumps to err_free_in. During
>> cleanup, it accesses ns2_usb->bulk_in[i].urb->transfer_dma. Since urb is
>> NULL for the failed index, will this trigger a NULL pointer dereference?
>
> Yes, most likely we would need a new label for this case.
>
>>
>>> + usb_free_urb(ns2_usb->bulk_in[i].urb);
>>> + }
>>> + devm_kfree(&intf->dev, ns2_usb);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static void switch2_usb_disconnect(struct usb_interface *intf)
>>> +{
>>> + struct switch2_usb *ns2_usb = usb_get_intfdata(intf);
>>> + unsigned long flags;
>>> + int i;
>>> +
>>> + spin_lock_irqsave(&ns2_usb->bulk_out_lock, flags);
>>> + usb_kill_anchored_urbs(&ns2_usb->bulk_out_anchor);
>>
>> Both usb_kill_anchored_urbs and cancel_work_sync can sleep. Is it safe to
>> call them while holding a spinlock?
>
> According to some googling, this indeed seems to be an isuse. The
> non-sleeping way to cancel transfer requests would be to use
> https://www.kernel.org/doc/html/v6.1/driver-api/usb/usb.html#c.usb_unlink_anchored_urbs
This was removed in 6.17 and has been unused since 2009.
>
>
>>
>>> + for (i = 0; i < NS2_OUT_URBS; i++) {
>>> + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_out[i].data,
>>> + ns2_usb->bulk_out[i].urb->transfer_dma);
>>> + usb_free_urb(ns2_usb->bulk_out[i].urb);
>>> + }
>>> + spin_unlock_irqrestore(&ns2_usb->bulk_out_lock, flags);
>>> +
>>> + spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
>>> + usb_kill_anchored_urbs(&ns2_usb->bulk_in_anchor);
>>> + cancel_work_sync(&ns2_usb->message_in_work);
>>
>> Furthermore, cancel_work_sync waits for message_in_work which attempts to
>> acquire bulk_in_lock. Does this cause a deadlock since the lock is already
>> held by the disconnecting thread?
>
> Yes, this looks like a real concern as well.
>
> Cheers,
> Silvan
>
>>
>>> + for (i = 0; i < NS2_IN_URBS; i++) {
^ permalink raw reply
* Re: [PATCH v5 4/5] HID: asus: add support for xgm led
From: sashiko-bot @ 2026-06-19 0:23 UTC (permalink / raw)
To: Denis Benato; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260619001103.1189200-5-denis.benato@linux.dev>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] A failure to initialize the cosmetic XG Mobile LED aborts the entire HID device probe.
- [High] Failing to restore the XGM LED brightness causes the entire HID device resume process to fail.
Pre-existing issues:
- [High] The devm-managed battery is not explicitly unregistered before stopping the hardware, leading to use-after-stop during teardown.
--
commit 7aa41eaa0bd3e715bef4c850703c7d47445cad14
Author: Denis Benato <denis.benato@linux.dev>
HID: asus: add support for xgm led
XG mobile stations have very bright leds behind the fan that can be
turned either ON or OFF: add a cled interface to allow controlling the
brightness of those red leds.
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 0fb8cd6437b77..06e4888f090ca 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -1361,7 +1432,17 @@ static int __maybe_unused asus_resume(struct hid_device *hdev)
> if (drvdata->listener.brightness_set)
> asus_kbd_backlight_set(&drvdata->listener, drvdata->kbd_backlight_brightness);
>
> + if (drvdata->xgm_led) {
> + ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
> + if (ret) {
> + hid_err(hdev, "Asus failed to restore xgm brightness: %d\n", ret);
> + goto asus_resume_err;
> + }
> + }
> +
> return 0;
> +asus_resume_err:
> + return ret;
[Severity: High]
In asus_resume(), if asus_xgm_led_set() fails to restore the LED state,
returning an error causes the entire HID device resume process to fail.
Will this leave the device in an inconsistent state and break input
functionality?
As documented in the comments for the backlight, non-fatal LED state
restorations shouldn't fail the system resume. Could this just ignore the error
here as well?
[ ... ]
> @@ -1491,6 +1572,16 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
> }
> }
>
> + if (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID) &&
> + ((hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2022) ||
> + (hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2023))) {
> + ret = asus_xgm_init(hdev, drvdata);
> + if (ret) {
> + hid_err(hdev, "Failed to initialize xg mobile: %d\n", ret);
> + goto err_stop_hw;
> + }
> + }
> +
[Severity: High]
If asus_xgm_init() fails due to a transient USB error during the initial
asus_xgm_led_set() call, will this completely abort the HID device probe and
disable the keyboard?
Since similar features like the keyboard backlight only emit a warning on
failure, should a non-fatal cosmetic LED initialization failure also just warn
and allow the driver to continue normal operation?
[ ... ]
> @@ -1540,6 +1631,9 @@ static void asus_remove(struct hid_device *hdev)
> if (drvdata->listener.brightness_set)
> asus_hid_unregister_listener(&drvdata->listener);
>
> + if (drvdata->xgm_led)
> + devm_led_classdev_unregister(&hdev->dev, &drvdata->xgm_led->cdev);
> +
> asus_worker_stop(drvdata->worker);
> hid_hw_stop(hdev);
> }
[Severity: High]
This is a pre-existing issue, but the devm-managed battery is not explicitly
unregistered before stopping the hardware.
Because the battery is registered via devm_power_supply_register() before
hid_hw_start(), and hid_hw_stop() is called here before the devres cleanup,
could this lead to a use-after-stop?
If a concurrent read of the battery sysfs properties happens during teardown, it
triggers asus_battery_get_property() which calls hid_hw_raw_request() on a
stopped HID device. Can this cause bus hangs or undefined behavior?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260619001103.1189200-1-denis.benato@linux.dev?part=4
^ permalink raw reply
* Re: [PATCH v5 1/5] HID: asus: refactor the two workqueues and init sequence
From: sashiko-bot @ 2026-06-19 0:23 UTC (permalink / raw)
To: Denis Benato; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260619001103.1189200-2-denis.benato@linux.dev>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ABBA deadlock between driver removal/probe failure and the deferred WMI_FAN action in the workqueue due to inverted lock acquisition order of `hdev->driver_input_lock`.
- [High] Unbounded dynamic allocation of workqueue actions allows an untrusted USB device or unprivileged user to exhaust kernel atomic memory pools.
- [Medium] Passing the raw packet size as `bufsize` to `hid_report_raw_event()` bypasses the HID core's automatic padding, leading to valid short packets being incorrectly rejected.
--
commit 656f5a6cc923dccccd418cd0aefa2239f221bf47
Author: Denis Benato <denis.benato@linux.dev>
HID: asus: refactor the two workqueues and init sequence
Multiple issues have been found within the hid-asus driver. This patch
joins the two workqueues into one and reuses the stopping mechanism
of the brightness workqueue. It also moves the asus_wmi_send_event()
sleeping call away from atomic context.
Link: https://lore.kernel.org/all/20260613154732.60A4B1F000E9@smtp.kernel.org/
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index d34d74df3dc0b..40191eb9e2e86 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -331,25 +380,71 @@ static int asus_e1239t_event(struct asus_drvdata *drvdat, u8 *data, int size)
> }
[ ... ]
> +static int asus_kbd_wmi_fan_send(struct asus_drvdata *drvdata, u8 *report_data,
> + size_t report_size)
> +{
> + struct asus_work_action *action;
> + unsigned long flags;
> +
> + if (report_size > FEATURE_KBD_REPORT_SIZE) {
> + hid_err(drvdata->hdev, "Invalid report size for fan event: %zu\n", report_size);
> + return -EINVAL;
> }
>
> + action = kzalloc(sizeof(struct asus_work_action), GFP_NOWAIT);
[Severity: High]
Does this code allow an unbounded number of allocations?
If a device sends a flood of fan events, or if a local user rapidly changes
the brightness, it seems this could queue an unlimited number of actions
using GFP_NOWAIT or GFP_ATOMIC.
Could this exhaust kernel atomic memory pools and lead to a local denial of
service?
> + if (!action)
> + return -ENOMEM;
> +
> + action->type = WMI_FAN;
> + action->data.fan_hid_data.report_size = report_size;
> + memcpy(action->data.fan_hid_data.report_data, report_data, report_size);
> + INIT_LIST_HEAD(&action->node);
> +
> + spin_lock_irqsave(&drvdata->worker->lock, flags);
> + asus_worker_schedule(drvdata->worker, action);
> + spin_unlock_irqrestore(&drvdata->worker->lock, flags);
>
> return 0;
> }
[ ... ]
> @@ -569,59 +668,157 @@ static int asus_kbd_disable_oobe(struct hid_device *hdev)
[ ... ]
> +static void asus_kbd_wmi_fan(struct hid_device *hdev, struct hid_raw_event_data *data)
> {
[ ... ]
> + /*
> + * Fallback: pass the raw event to the HID core; to avoid
> + * racing against the hid_report_raw_event() that generated
> + * this event use the same locking mechanism and wait for
> + * that function to terminate and signal the deferred execution
> + * before raising the stored event.
> + */
> + down(&hdev->driver_input_lock);
[Severity: High]
Can this code cause an ABBA deadlock during device probe failure or removal?
The HID core holds hdev->driver_input_lock during hid_device_probe() and
hid_device_remove().
If asus_remove() or the error path in asus_probe() calls asus_worker_stop()
and blocks on cancel_work_sync(&worker->work) while driver_input_lock is
held:
HID core thread:
hid_device_remove() (acquires driver_input_lock)
-> asus_remove()
-> asus_worker_stop()
-> cancel_work_sync() (waits for worker)
Worker thread:
asus_work()
-> asus_kbd_wmi_fan()
-> down(&hdev->driver_input_lock) (waits for HID core thread)
Would this lead to an unrecoverable deadlock?
> + hid_report_raw_event(hdev, HID_INPUT_REPORT,
> + data->report_data, data->report_size,
> + data->report_size, 1);
[Severity: Medium]
Does passing data->report_size as the bufsize argument drop valid short
packets?
The HID core natively handles short packets by padding the buffer up to the
expected report descriptor size (rsize), provided the allocated buffer size
(bufsize) is large enough.
If bufsize is artificially restricted to data->report_size, the safety check
"if (bsize < rsize)" inside hid_report_raw_event() might falsely trigger and
reject the valid short event with -EINVAL.
Should FEATURE_KBD_REPORT_SIZE be used as the bufsize parameter instead?
> + up(&hdev->driver_input_lock);
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260619001103.1189200-1-denis.benato@linux.dev?part=1
^ permalink raw reply
* [PATCH v5 0/5] HID: asus: security fixes and more hardware support
From: Denis Benato @ 2026-06-19 0:10 UTC (permalink / raw)
To: linux-kernel
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Luke D . Jones,
Mateusz Schyboll, Denis Benato, Denis Benato, Antheas Kapenekakis,
Connor Belli
Hi all,
I have added support for controlling the (way too bright) XG mobile
LEDs in hid-asus and added the i2c version of already supported
hardware that was probed only when it's a USB: these are two separate
features changes and are the only two that are not fixes for
pre-existing issue (see below).
Auto-review bot has spotted a bunch of pre-existing problems alongside
problems in my own code therefore at this point I am going to fix the
more problems I can and including those fixes and improvements in this
patchset.
For v4 I decided to follow Antheas' suggestion of reusing the existing
workqueue and by making it more generic I solved a good bunch of issues.
The v5 iteration is simply me fixing a bunch of bugs in new code spotted
by the bot. Thanks for providing such a useful tool!
On a side node this patchset has a few more warnings: specifically
"WARNING: Prefer kzalloc_obj over kzalloc with sizeof" but it's a false
positive as that would introduce sleeping calls in atomic contexts.
Link: https://lore.kernel.org/all/20260612124830.984592-1-denis.benato@linux.dev/
Link: https://lore.kernel.org/all/30258f28-4036-4e99-9494-031212544dc2@linux.dev/
Best regards,
Denis Benato
Link v2: https://lore.kernel.org/all/20260612142326.1704858-1-denis.benato@linux.dev/
Link v3: https://lore.kernel.org/all/20260613153029.2559774-1-denis.benato@linux.dev/
Link v4: https://lore.kernel.org/all/20260615165058.3845-1-denis.benato@linux.dev/
Changelog:
- v1
- Initial submission (as separate patches)
- v2
- HID: asus: add i2c entry for FA808UM and other TUFs
- Joined to the patchset
- HID: asus: add support for xgm led
- Use the correct callback set_brightness_blocking
- Stop hardcoding the cled name
- HID: asus: prevent wrong pointer cast
- Added
- HID: asus: mitigate possible use-after-free
- Added
- v3
- HID: asus: add support for xgm led
- check devm_kasprintf result
- HID: asus: prevent a late KEY_FN_ESC to trigger a use-after-free
- Added
- HID: asus: avoid sleeping calls in atomic context
- Added
- HID: asus: cleanup keyboard listener on failure: avoid use-after-free
- Added
- v4
- HID: asus: refactor the two workqueues and init sequence
- Added to replace previous patches in a clearer way
- HID: asus: add support for xgm led
- Simplify cled interface allocation and registration
- v5
- HID: asus: refactor the two workqueues and init sequence
- replace kzalloc_obj in atomix context with kzalloc(..., GFP_ATOMIC/NOWAIT)
- better destroy the queue on _probe in case of errors
- Fix a data race for deferred asus_kbd_wmi_fan()
- HID: asus: add support for xgm led
- improve teardown of the cled interface
Denis Benato (5):
HID: asus: refactor the two workqueues and init sequence
HID: asus: remove extraneous OOM error
HID: asus: fix a off-by-one in mcu_parse_version_string() validation
HID: asus: add support for xgm led
HID: asus: add i2c entry for FA808UM and other TUFs
drivers/hid/hid-asus.c | 491 ++++++++++++++++++++++++++++++++---------
1 file changed, 382 insertions(+), 109 deletions(-)
--
2.47.3
^ permalink raw reply
* [PATCH v5 5/5] HID: asus: add i2c entry for FA808UM and other TUFs
From: Denis Benato @ 2026-06-19 0:11 UTC (permalink / raw)
To: linux-kernel
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Luke D . Jones,
Mateusz Schyboll, Denis Benato, Denis Benato, Antheas Kapenekakis,
Connor Belli
In-Reply-To: <20260619001103.1189200-1-denis.benato@linux.dev>
On newer TUF laptops the keyboard HID device uses the same PID/VID of a
USB device that was found in ROG laptops: add it to hid-asus as i2c too.
Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
drivers/hid/hid-asus.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 06e4888f090c..b155eaf106a5 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -1770,6 +1770,9 @@ static const struct hid_device_id asus_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_HID_FN_LOCK },
+ { HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK,
+ USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
+ QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_HID_FN_LOCK },
{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
--
2.47.3
^ permalink raw reply related
* [PATCH v5 4/5] HID: asus: add support for xgm led
From: Denis Benato @ 2026-06-19 0:11 UTC (permalink / raw)
To: linux-kernel
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Luke D . Jones,
Mateusz Schyboll, Denis Benato, Denis Benato, Antheas Kapenekakis,
Connor Belli
In-Reply-To: <20260619001103.1189200-1-denis.benato@linux.dev>
XG mobile stations have very bright leds behind the fan that can be
turned either ON or OFF: add a cled interface to allow controlling the
brightness of those red leds.
Reviewed-by: Antheas Kapenekakis <lkml@antheas.dev>
Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
drivers/hid/hid-asus.c | 94 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 94 insertions(+)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 0fb8cd6437b7..06e4888f090c 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -51,6 +51,8 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define FEATURE_KBD_LED_REPORT_ID1 0x5d
#define FEATURE_KBD_LED_REPORT_ID2 0x5e
+#define ROG_XGM_REPORT_SIZE 300
+
#define ROG_ALLY_REPORT_SIZE 64
#define ROG_ALLY_X_MIN_MCU 313
#define ROG_ALLY_MIN_MCU 319
@@ -143,6 +145,11 @@ struct asus_worker {
bool removed;
};
+struct asus_xgm_led {
+ struct led_classdev cdev;
+ struct hid_device *hdev;
+};
+
struct asus_touchpad_info {
int max_x;
int max_y;
@@ -169,6 +176,7 @@ struct asus_drvdata {
unsigned long battery_next_query;
struct asus_hid_listener listener;
bool fn_lock;
+ struct asus_xgm_led *xgm_led;
};
static int asus_report_battery(struct asus_drvdata *, u8 *, int);
@@ -1125,6 +1133,26 @@ static int asus_battery_probe(struct hid_device *hdev)
return ret;
}
+static int asus_xgm_led_set(struct led_classdev *led_cdev, enum led_brightness value)
+{
+ const u8 buf[ROG_XGM_REPORT_SIZE] = {
+ FEATURE_KBD_LED_REPORT_ID2, 0xC5, (value) ? 0x50 : 0x00
+ };
+ struct asus_xgm_led *xgm = container_of(led_cdev, struct asus_xgm_led, cdev);
+ int ret;
+
+ ret = asus_kbd_set_report(xgm->hdev, buf, ROG_XGM_REPORT_SIZE);
+ if (ret < 0) {
+ hid_err(xgm->hdev, "Unable to set XG mobile led state: %d\n", ret);
+ return ret;
+ } else if (ret != ROG_XGM_REPORT_SIZE) {
+ hid_err(xgm->hdev, "Unexpected partial transfer to XG mobile: %d\n", ret);
+ return -EIO;
+ }
+
+ return 0;
+}
+
static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
{
struct input_dev *input = hi->input;
@@ -1349,9 +1377,52 @@ static int asus_start_multitouch(struct hid_device *hdev)
return 0;
}
+static int asus_xgm_init(struct hid_device *hdev, struct asus_drvdata *drvdata)
+{
+ const char *name;
+ int ret;
+
+ drvdata->xgm_led = devm_kzalloc(&hdev->dev, sizeof(*drvdata->xgm_led), GFP_KERNEL);
+ if (drvdata->xgm_led == NULL)
+ return -ENOMEM;
+
+ name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "asus:xgm-%s:led",
+ strlen(hdev->uniq) ? hdev->uniq : dev_name(&hdev->dev));
+
+ if (name == NULL) {
+ ret = -ENOMEM;
+ goto asus_xgm_init_err;
+ }
+
+ drvdata->xgm_led->hdev = hdev;
+ drvdata->xgm_led->cdev.name = name;
+ drvdata->xgm_led->cdev.brightness = 1;
+ drvdata->xgm_led->cdev.max_brightness = 1;
+ drvdata->xgm_led->cdev.brightness_set_blocking = asus_xgm_led_set;
+
+ /* LED state is arbitrary on boot, set a default */
+ ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
+ if (ret) {
+ hid_err(hdev, "Asus failed to set xgm led: %d\n", ret);
+ goto asus_xgm_init_err;
+ }
+
+ ret = devm_led_classdev_register(&hdev->dev, &drvdata->xgm_led->cdev);
+ if (ret) {
+ hid_err(hdev, "Asus failed to register xgm led: %d\n", ret);
+ goto asus_xgm_init_err;
+ }
+
+ return 0;
+asus_xgm_init_err:
+ drvdata->xgm_led = NULL;
+ return ret;
+}
+
static int __maybe_unused asus_resume(struct hid_device *hdev)
{
struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+ int ret;
/*
* If we have a backlight listener registered, restore the previous state,
@@ -1361,7 +1432,17 @@ static int __maybe_unused asus_resume(struct hid_device *hdev)
if (drvdata->listener.brightness_set)
asus_kbd_backlight_set(&drvdata->listener, drvdata->kbd_backlight_brightness);
+ if (drvdata->xgm_led) {
+ ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
+ if (ret) {
+ hid_err(hdev, "Asus failed to restore xgm brightness: %d\n", ret);
+ goto asus_resume_err;
+ }
+ }
+
return 0;
+asus_resume_err:
+ return ret;
}
static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
@@ -1491,6 +1572,16 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
}
}
+ if (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID) &&
+ ((hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2022) ||
+ (hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2023))) {
+ ret = asus_xgm_init(hdev, drvdata);
+ if (ret) {
+ hid_err(hdev, "Failed to initialize xg mobile: %d\n", ret);
+ goto err_stop_hw;
+ }
+ }
+
/* Laptops keyboard backlight is always at 0x5a */
if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
(asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID)) &&
@@ -1540,6 +1631,9 @@ static void asus_remove(struct hid_device *hdev)
if (drvdata->listener.brightness_set)
asus_hid_unregister_listener(&drvdata->listener);
+ if (drvdata->xgm_led)
+ devm_led_classdev_unregister(&hdev->dev, &drvdata->xgm_led->cdev);
+
asus_worker_stop(drvdata->worker);
hid_hw_stop(hdev);
}
--
2.47.3
^ permalink raw reply related
* [PATCH v5 3/5] HID: asus: fix a off-by-one in mcu_parse_version_string() validation
From: Denis Benato @ 2026-06-19 0:11 UTC (permalink / raw)
To: linux-kernel
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Luke D . Jones,
Mateusz Schyboll, Denis Benato, Denis Benato, Antheas Kapenekakis,
Connor Belli
In-Reply-To: <20260619001103.1189200-1-denis.benato@linux.dev>
In mcu_parse_version_string() a size validation for response is stricter
that it needs to be: relax the check by one byte.
The device always answer with a greater byte count so this does
not introduce visible changes.
Fixes: ("hid-asus: check ROG Ally MCU version and warn")
Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
drivers/hid/hid-asus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index d31e71ce3d19..0fb8cd6437b7 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -841,7 +841,7 @@ static int mcu_parse_version_string(const u8 *response, size_t response_size)
dots++;
}
- if (dots != 2 || p >= end || (p + 3) >= end)
+ if (dots != 2 || end - p < 3)
return -EINVAL;
memcpy(buf, p, 3);
--
2.47.3
^ permalink raw reply related
* [PATCH v5 2/5] HID: asus: remove extraneous OOM error
From: Denis Benato @ 2026-06-19 0:11 UTC (permalink / raw)
To: linux-kernel
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Luke D . Jones,
Mateusz Schyboll, Denis Benato, Denis Benato, Antheas Kapenekakis,
Connor Belli
In-Reply-To: <20260619001103.1189200-1-denis.benato@linux.dev>
If devm_kzalloc fails an allocation error is already being reported:
no need to repeat it. For new code this behavior is disincentivized
and checkpatch.pl reports a warning.
Reviewed-by: Antheas Kapenekakis <lkml@antheas.dev>
Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
drivers/hid/hid-asus.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 40191eb9e2e8..d31e71ce3d19 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -1383,10 +1383,8 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
int ret;
drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
- if (drvdata == NULL) {
- hid_err(hdev, "Can't alloc Asus descriptor\n");
+ if (drvdata == NULL)
return -ENOMEM;
- }
hid_set_drvdata(hdev, drvdata);
--
2.47.3
^ permalink raw reply related
* [PATCH v5 1/5] HID: asus: refactor the two workqueues and init sequence
From: Denis Benato @ 2026-06-19 0:10 UTC (permalink / raw)
To: linux-kernel
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Luke D . Jones,
Mateusz Schyboll, Denis Benato, Denis Benato, Antheas Kapenekakis,
Connor Belli, sahiko-bot
In-Reply-To: <20260619001103.1189200-1-denis.benato@linux.dev>
Multiple issues have been found within the hid-asus driver:
- unchecked size in asus_raw_event()
- unclean teardown of asus_probe on failure
- possible use-after-free in asus_probe
- multiple workqueue used for jobs where one was enough
- sleeping calls in atomic context
- packets of incorrect size being sent to the keyboard controller
Join the two workqueues into one reusing the stopping mechanism
of the brightness workqueue, use the joined workqueue to also
move the asus_wmi_send_event() sleeping call away from atomic
context and add a size check in asus_raw_event().
Fixes: f631011e36b8 ("HID: hid-asus: Implement fn lock for Asus ProArt P16")
Fixes: 1489a34e97ef ("HID: asus: Implement Fn+F5 fan control key handler")
Fixes: b34b5945a769 ("HID: asus: listen to the asus-wmi brightness device instead of creating one")
Reported-by: sahiko-bot@kernel.org
Closes: https://lore.kernel.org/all/20260613154732.60A4B1F000E9@smtp.kernel.org/
Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
drivers/hid/hid-asus.c | 400 +++++++++++++++++++++++++++++------------
1 file changed, 289 insertions(+), 111 deletions(-)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index d34d74df3dc0..40191eb9e2e8 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -109,11 +109,36 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define TRKID_SGN ((TRKID_MAX + 1) >> 1)
-struct asus_kbd_leds {
- struct asus_hid_listener listener;
+enum asus_work_action_type {
+ FN_LOCK_SYNC,
+ BRIGHTNESS_SET,
+ WMI_FAN,
+};
+
+struct hid_raw_event_data {
+ u8 report_data[FEATURE_KBD_REPORT_SIZE];
+ size_t report_size;
+};
+
+struct asus_work_action {
+ struct list_head node;
+ enum asus_work_action_type type;
+ union {
+ /* Data for BRIGHTNESS_SET */
+ unsigned int brightness;
+
+ /* Data for FN_LOCK_SYNC */
+ bool fn_lock;
+
+ /* Data for WMI_FAN */
+ struct hid_raw_event_data fan_hid_data;
+ } data;
+};
+
+struct asus_worker {
struct hid_device *hdev;
struct work_struct work;
- unsigned int brightness;
+ struct list_head actions;
spinlock_t lock;
bool removed;
};
@@ -133,7 +158,8 @@ struct asus_drvdata {
struct hid_device *hdev;
struct input_dev *input;
struct input_dev *tp_kbd_input;
- struct asus_kbd_leds *kbd_backlight;
+ struct asus_worker *worker;
+ unsigned int kbd_backlight_brightness;
const struct asus_touchpad_info *tp;
struct power_supply *battery;
struct power_supply_desc battery_desc;
@@ -141,7 +167,7 @@ struct asus_drvdata {
int battery_stat;
bool battery_in_query;
unsigned long battery_next_query;
- struct work_struct fn_lock_sync_work;
+ struct asus_hid_listener listener;
bool fn_lock;
};
@@ -211,6 +237,29 @@ static const u8 asus_report_id_init[] = {
FEATURE_KBD_LED_REPORT_ID2
};
+/*
+ * Send events to asus-wmi driver for handling special keys
+ */
+static int asus_wmi_send_event(struct asus_drvdata *drvdata, u8 code)
+{
+ int err;
+ u32 retval;
+
+ err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
+ ASUS_WMI_METHODID_NOTIF, code, &retval);
+ if (err) {
+ pr_warn("Failed to notify asus-wmi: %d\n", err);
+ return err;
+ }
+
+ if (retval != 0) {
+ pr_warn("Failed to notify asus-wmi (retval): 0x%x\n", retval);
+ return -EIO;
+ }
+
+ return 0;
+}
+
static void asus_report_contact_down(struct asus_drvdata *drvdat,
int toolType, u8 *data)
{
@@ -331,25 +380,71 @@ static int asus_e1239t_event(struct asus_drvdata *drvdat, u8 *data, int size)
}
/*
- * Send events to asus-wmi driver for handling special keys
+ * Used in atomic contexts to schedule work involving sleeps operations or
+ * asus-wmi interactions.
+ *
+ * Caller is responsible to store relevant data in the structure to carry out
+ * the required action.
+ *
+ * This function must be called while the spin lock protecting the workqueue
+ * is already being held.
*/
-static int asus_wmi_send_event(struct asus_drvdata *drvdata, u8 code)
+static void asus_worker_schedule(struct asus_worker *worker, struct asus_work_action *action)
{
- int err;
- u32 retval;
-
- err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
- ASUS_WMI_METHODID_NOTIF, code, &retval);
- if (err) {
- pr_warn("Failed to notify asus-wmi: %d\n", err);
- return err;
+ if (worker->removed) {
+ kfree(action);
+ return;
}
- if (retval != 0) {
- pr_warn("Failed to notify asus-wmi (retval): 0x%x\n", retval);
- return -EIO;
+ list_add_tail(&action->node, &worker->actions);
+ schedule_work(&worker->work);
+}
+
+static int asus_kbd_fn_lock_set(struct asus_drvdata *drvdata, bool enabled)
+{
+ struct asus_work_action *action;
+ unsigned long flags;
+
+ action = kzalloc(sizeof(struct asus_work_action), GFP_ATOMIC);
+ if (!action)
+ return -ENOMEM;
+
+ drvdata->fn_lock = enabled;
+ action->type = FN_LOCK_SYNC;
+ action->data.fn_lock = drvdata->fn_lock;
+ INIT_LIST_HEAD(&action->node);
+
+ spin_lock_irqsave(&drvdata->worker->lock, flags);
+ asus_worker_schedule(drvdata->worker, action);
+ spin_unlock_irqrestore(&drvdata->worker->lock, flags);
+
+ return 0;
+}
+
+static int asus_kbd_wmi_fan_send(struct asus_drvdata *drvdata, u8 *report_data,
+ size_t report_size)
+{
+ struct asus_work_action *action;
+ unsigned long flags;
+
+ if (report_size > FEATURE_KBD_REPORT_SIZE) {
+ hid_err(drvdata->hdev, "Invalid report size for fan event: %zu\n", report_size);
+ return -EINVAL;
}
+ action = kzalloc(sizeof(struct asus_work_action), GFP_NOWAIT);
+ if (!action)
+ return -ENOMEM;
+
+ action->type = WMI_FAN;
+ action->data.fan_hid_data.report_size = report_size;
+ memcpy(action->data.fan_hid_data.report_data, report_data, report_size);
+ INIT_LIST_HEAD(&action->node);
+
+ spin_lock_irqsave(&drvdata->worker->lock, flags);
+ asus_worker_schedule(drvdata->worker, action);
+ spin_unlock_irqrestore(&drvdata->worker->lock, flags);
+
return 0;
}
@@ -357,6 +452,7 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
struct hid_usage *usage, __s32 value)
{
struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+ int ret;
if ((usage->hid & HID_USAGE_PAGE) == HID_UP_ASUSVENDOR &&
(usage->hid & HID_USAGE) != 0x00 &&
@@ -375,8 +471,11 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
return !asus_hid_event(ASUS_EV_BRTTOGGLE);
case KEY_FN_ESC:
if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
- drvdata->fn_lock = !drvdata->fn_lock;
- schedule_work(&drvdata->fn_lock_sync_work);
+ ret = asus_kbd_fn_lock_set(drvdata, !drvdata->fn_lock);
+ if (ret) {
+ hid_err(hdev, "Error while toggling FN lock: %d\n", ret);
+ return ret;
+ }
}
break;
}
@@ -389,6 +488,12 @@ static int asus_raw_event(struct hid_device *hdev,
struct hid_report *report, u8 *data, int size)
{
struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+ int ret;
+
+ if (size < 2) {
+ hid_dbg(hdev, "Unexpected keyboard report size %d\n", size);
+ return 0;
+ }
if (drvdata->battery && data[0] == BATTERY_REPORT_ID)
return asus_report_battery(drvdata, data, size);
@@ -414,19 +519,13 @@ static int asus_raw_event(struct hid_device *hdev,
* pass to userspace so it can implement its own fan control.
*/
if (data[1] == ASUS_FAN_CTRL_KEY_CODE) {
- int ret = asus_wmi_send_event(drvdata, ASUS_FAN_CTRL_KEY_CODE);
+ ret = asus_kbd_wmi_fan_send(drvdata, data, size);
- if (ret == 0) {
- /* Successfully handled by asus-wmi, block event */
+ /* if execution deferred successfully block event */
+ if (ret == 0)
return -1;
- }
- /*
- * Warn if asus-wmi failed (but not if it's unavailable).
- * Let the event reach userspace in all failure cases.
- */
- if (ret != -ENODEV)
- hid_warn(hdev, "Failed to notify asus-wmi: %d\n", ret);
+ return ret;
}
/*
@@ -569,59 +668,157 @@ static int asus_kbd_disable_oobe(struct hid_device *hdev)
return 0;
}
-static int asus_kbd_set_fn_lock(struct hid_device *hdev, bool enabled)
+static void asus_kbd_set_fn_lock(struct hid_device *hdev, bool enabled)
{
- u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xd0, 0x4e, !!enabled };
+ const u8 buf[FEATURE_KBD_REPORT_SIZE] = { FEATURE_KBD_REPORT_ID, 0xd0, 0x4e, !!enabled };
+ int ret;
- return asus_kbd_set_report(hdev, buf, sizeof(buf));
+ ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
+ if (ret < 0)
+ hid_err(hdev, "Asus failed to set fn lock: %d\n", ret);
}
-static void asus_sync_fn_lock(struct work_struct *work)
+static void asus_kbd_set_brightness(struct hid_device *hdev, u8 brightness)
{
- struct asus_drvdata *drvdata =
- container_of(work, struct asus_drvdata, fn_lock_sync_work);
+ const u8 buf[FEATURE_KBD_REPORT_SIZE] = {
+ FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, brightness
+ };
+ int ret;
- asus_kbd_set_fn_lock(drvdata->hdev, drvdata->fn_lock);
+ ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
+ if (ret < 0)
+ hid_err(hdev, "Asus failed to set keyboard backlight: %d\n", ret);
}
-static void asus_schedule_work(struct asus_kbd_leds *led)
+static void asus_kbd_wmi_fan(struct hid_device *hdev, struct hid_raw_event_data *data)
{
+ struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+ int ret;
+
+ ret = asus_wmi_send_event(drvdata, ASUS_FAN_CTRL_KEY_CODE);
+
+ /*
+ * Warn if asus-wmi failed (but not if it's unavailable).
+ * Let the event reach userspace in all failure cases.
+ */
+ switch (ret) {
+ case -ENODEV:
+ break;
+ case 0:
+ return;
+ default:
+ hid_warn(hdev, "Failed to notify asus-wmi: %d\n", ret);
+ break;
+ }
+
+ /*
+ * Fallback: pass the raw event to the HID core; to avoid
+ * racing against the hid_report_raw_event() that generated
+ * this event use the same locking mechanism and wait for
+ * that function to terminate and signal the deferred execution
+ * before raising the stored event.
+ */
+ down(&hdev->driver_input_lock);
+ hid_report_raw_event(hdev, HID_INPUT_REPORT,
+ data->report_data, data->report_size,
+ data->report_size, 1);
+ up(&hdev->driver_input_lock);
+}
+
+static void asus_kbd_backlight_set(struct asus_hid_listener *listener, int brightness)
+{
+ struct asus_drvdata *drvdata = container_of(listener, struct asus_drvdata, listener);
+ struct asus_worker *worker = drvdata->worker;
+ struct asus_work_action *action;
unsigned long flags;
- spin_lock_irqsave(&led->lock, flags);
- if (!led->removed)
- schedule_work(&led->work);
- spin_unlock_irqrestore(&led->lock, flags);
+ drvdata->kbd_backlight_brightness = brightness;
+
+ action = kzalloc(sizeof(struct asus_work_action), GFP_NOWAIT);
+ if (!action)
+ return;
+
+ action->type = BRIGHTNESS_SET;
+ action->data.brightness = brightness;
+ INIT_LIST_HEAD(&action->node);
+
+ spin_lock_irqsave(&worker->lock, flags);
+ asus_worker_schedule(worker, action);
+ spin_unlock_irqrestore(&worker->lock, flags);
}
-static void asus_kbd_backlight_set(struct asus_hid_listener *listener,
- int brightness)
+static void asus_work(struct work_struct *work)
{
- struct asus_kbd_leds *led = container_of(listener, struct asus_kbd_leds,
- listener);
+ struct asus_worker *worker = container_of(work, struct asus_worker, work);
+ struct asus_work_action *action = NULL;
unsigned long flags;
- spin_lock_irqsave(&led->lock, flags);
- led->brightness = brightness;
- spin_unlock_irqrestore(&led->lock, flags);
+ /* Save the action to be performed and clear the flag */
+ spin_lock_irqsave(&worker->lock, flags);
+ if (!list_empty(&worker->actions)) {
+ action = list_first_entry(&worker->actions,
+ struct asus_work_action, node);
+ list_del(&action->node);
+ }
+ spin_unlock_irqrestore(&worker->lock, flags);
+
+ if (!action)
+ return;
+
+ switch (action->type) {
+ case BRIGHTNESS_SET:
+ asus_kbd_set_brightness(worker->hdev, action->data.brightness);
+ break;
+ case FN_LOCK_SYNC:
+ asus_kbd_set_fn_lock(worker->hdev, action->data.fn_lock);
+ break;
+ case WMI_FAN:
+ asus_kbd_wmi_fan(worker->hdev, &action->data.fan_hid_data);
+ break;
+ default:
+ hid_err(worker->hdev, "Invalid action type: %d\n", action->type);
+ break;
+ }
+
+ kfree(action);
- asus_schedule_work(led);
+ /* Re-schedule if there are more pending actions */
+ spin_lock_irqsave(&worker->lock, flags);
+ if (!list_empty(&worker->actions))
+ schedule_work(&worker->work);
+ spin_unlock_irqrestore(&worker->lock, flags);
}
-static void asus_kbd_backlight_work(struct work_struct *work)
+static int asus_worker_create(struct hid_device *hdev, struct asus_drvdata *drvdata)
{
- struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds, work);
- u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, 0x00 };
- int ret;
+ drvdata->worker = devm_kzalloc(&hdev->dev, sizeof(struct asus_worker), GFP_KERNEL);
+ if (!drvdata->worker)
+ return -ENOMEM;
+
+ drvdata->worker->removed = false;
+ drvdata->worker->hdev = hdev;
+ INIT_LIST_HEAD(&drvdata->worker->actions);
+
+ INIT_WORK(&drvdata->worker->work, asus_work);
+ spin_lock_init(&drvdata->worker->lock);
+
+ return 0;
+}
+
+static void asus_worker_stop(struct asus_worker *worker)
+{
+ struct asus_work_action *action, *tmp;
unsigned long flags;
- spin_lock_irqsave(&led->lock, flags);
- buf[4] = led->brightness;
- spin_unlock_irqrestore(&led->lock, flags);
+ spin_lock_irqsave(&worker->lock, flags);
+ worker->removed = true;
+ list_for_each_entry_safe(action, tmp, &worker->actions, node) {
+ list_del(&action->node);
+ kfree(action);
+ }
+ spin_unlock_irqrestore(&worker->lock, flags);
- ret = asus_kbd_set_report(led->hdev, buf, sizeof(buf));
- if (ret < 0)
- hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret);
+ cancel_work_sync(&worker->work);
}
/*
@@ -760,23 +957,11 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
le16_to_cpu(udev->descriptor.idProduct));
}
- drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
- sizeof(struct asus_kbd_leds),
- GFP_KERNEL);
- if (!drvdata->kbd_backlight)
- return -ENOMEM;
-
- drvdata->kbd_backlight->removed = false;
- drvdata->kbd_backlight->brightness = 0;
- drvdata->kbd_backlight->hdev = hdev;
- drvdata->kbd_backlight->listener.brightness_set = asus_kbd_backlight_set;
- INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_backlight_work);
- spin_lock_init(&drvdata->kbd_backlight->lock);
-
- ret = asus_hid_register_listener(&drvdata->kbd_backlight->listener);
+ drvdata->listener.brightness_set = asus_kbd_backlight_set;
+ ret = asus_hid_register_listener(&drvdata->listener);
if (ret < 0) {
- /* No need to have this still around */
- devm_kfree(&hdev->dev, drvdata->kbd_backlight);
+ hid_err(hdev, "Unable to register kbd brightness listener: %d\n", ret);
+ drvdata->listener.brightness_set = NULL;
}
return ret;
@@ -965,11 +1150,9 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
}
}
- if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
- drvdata->fn_lock = true;
- INIT_WORK(&drvdata->fn_lock_sync_work, asus_sync_fn_lock);
- asus_kbd_set_fn_lock(hdev, true);
- }
+ if ((drvdata->quirks & QUIRK_HID_FN_LOCK) &&
+ (asus_kbd_fn_lock_set(drvdata, true)))
+ hid_warn(hdev, "Error while setting FN lock to ON\n");
if (drvdata->tp) {
int ret;
@@ -1004,11 +1187,9 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
drvdata->input = input;
- if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
- drvdata->fn_lock = true;
- INIT_WORK(&drvdata->fn_lock_sync_work, asus_sync_fn_lock);
- asus_kbd_set_fn_lock(hdev, true);
- }
+ if ((drvdata->quirks & QUIRK_HID_FN_LOCK) &&
+ (asus_kbd_fn_lock_set(drvdata, true)))
+ hid_warn(hdev, "Error while setting FN lock to ON\n");
return 0;
}
@@ -1171,20 +1352,16 @@ static int asus_start_multitouch(struct hid_device *hdev)
static int __maybe_unused asus_resume(struct hid_device *hdev)
{
struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
- int ret = 0;
- if (drvdata->kbd_backlight) {
- const u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4,
- drvdata->kbd_backlight->brightness };
- ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
- if (ret < 0) {
- hid_err(hdev, "Asus failed to set keyboard backlight: %d\n", ret);
- goto asus_resume_err;
- }
- }
+ /*
+ * If we have a backlight listener registered, restore the previous state,
+ * in case of error do not fail: most models restore the backlight
+ * automatically, and the error is non-fatal.
+ */
+ if (drvdata->listener.brightness_set)
+ asus_kbd_backlight_set(&drvdata->listener, drvdata->kbd_backlight_brightness);
-asus_resume_err:
- return ret;
+ return 0;
}
static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
@@ -1294,8 +1471,15 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
is_vendor = true;
}
+ ret = asus_worker_create(hdev, drvdata);
+ if (ret) {
+ hid_warn(hdev, "Failed to initialize worker: %d\n", ret);
+ return ret;
+ }
+
ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
if (ret) {
+ asus_worker_stop(drvdata->worker);
hid_err(hdev, "Asus hw start failed: %d\n", ret);
return ret;
}
@@ -1343,6 +1527,10 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
return 0;
err_stop_hw:
+ if (drvdata->listener.brightness_set)
+ asus_hid_unregister_listener(&drvdata->listener);
+
+ asus_worker_stop(drvdata->worker);
hid_hw_stop(hdev);
return ret;
}
@@ -1350,21 +1538,11 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
static void asus_remove(struct hid_device *hdev)
{
struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
- unsigned long flags;
-
- if (drvdata->kbd_backlight) {
- asus_hid_unregister_listener(&drvdata->kbd_backlight->listener);
-
- spin_lock_irqsave(&drvdata->kbd_backlight->lock, flags);
- drvdata->kbd_backlight->removed = true;
- spin_unlock_irqrestore(&drvdata->kbd_backlight->lock, flags);
-
- cancel_work_sync(&drvdata->kbd_backlight->work);
- }
- if (drvdata->quirks & QUIRK_HID_FN_LOCK)
- cancel_work_sync(&drvdata->fn_lock_sync_work);
+ if (drvdata->listener.brightness_set)
+ asus_hid_unregister_listener(&drvdata->listener);
+ asus_worker_stop(drvdata->worker);
hid_hw_stop(hdev);
}
--
2.47.3
^ permalink raw reply related
* Re: [PATCH v2 0/2] firmware: arm_scmi: Ensure automatic module loading
From: Hans de Goede @ 2026-06-18 20:31 UTC (permalink / raw)
To: Bjorn Andersson, Sudeep Holla, Cristian Marussi,
Nathan Chancellor, Nicolas Schier, Michael Turquette
Cc: arm-scmi, linux-arm-kernel, linux-kernel, linux-kbuild,
Stephen Boyd, Brian Masney, Rafael J. Wysocki, Viresh Kumar,
Frank Li, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Guenter Roeck, Jyoti Bhayana, Jonathan Cameron, David Lechner,
Nuno Sá, Andy Shevchenko, Dmitry Torokhov, Ulf Hansson,
Liam Girdwood, Mark Brown, Philipp Zabel, Alexandre Belloni,
linux-clk, linux-pm, imx, linux-hwmon, linux-iio, linux-input,
linux-rtc
In-Reply-To: <20260618-scmi-modalias-v2-0-8c7547c1be21@oss.qualcomm.com>
Hi,
On 18-Jun-26 17:56, Bjorn Andersson wrote:
> SCMI drivers such as the Arm SCMI CPUfreq driver are allowed to built as
> modules, but they are then not automatically loaded. Rework the SCMI
> device table alias support to make modpost consume the information from
> MODULE_DEVICE_TABLE(scmi, ...) and allow drivers to be loaded based on
> this information, if known. Also add a protocol-based alias to also
> trigger driver loading when only the SCMI protocol id is known.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
So I just gave this a test spin and unfortunately it does not work.
The problem with Fedora's kernel-config / setup is that the
request_module() from patch 2/2 runs from the initramfs, but
the scmi_cpufreq module is only available in the rootfs.
It does work if I explictly add the scmi_cpufreq module to
the initramfs, then it does get autoloaded.
We really need some place to put a uevent sysfs attr which then
gets replayed when udev is restarted from the rootfs and then
re-reads all the uevent files as part of its coldplug
enumeration.
I wonder if it is ok for a single uevent file to have
multiple MODALIAS= lines in there.
Regards,
Hans
> ---
> Changes in v2:
> - Use request_module_nowait()
> - Drop #include <linux/mod_devicetable.h> from scmi_protocol.h
> - Link to v1: https://patch.msgid.link/20260616-scmi-modalias-v1-0-662b8dd52ab2@oss.qualcomm.com
>
> To: Sudeep Holla <sudeep.holla@kernel.org>
> To: Cristian Marussi <cristian.marussi@arm.com>
> To: Michael Turquette <mturquette@baylibre.com>
> To: Nicolas Schier <nsc@kernel.org>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: Brian Masney <bmasney@redhat.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Frank Li <Frank.Li@nxp.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Jyoti Bhayana <jbhayana@google.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: David Lechner <dlechner@baylibre.com>
> Cc: Nuno Sá <nuno.sa@analog.com>
> Cc: Andy Shevchenko <andy@kernel.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Ulf Hansson <ulfh@kernel.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: arm-scmi@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-clk@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-pm@vger.kernel.org
> Cc: imx@lists.linux.dev
> Cc: linux-hwmon@vger.kernel.org
> Cc: linux-iio@vger.kernel.org
> Cc: linux-input@vger.kernel.org
> Cc: linux-rtc@vger.kernel.org
> Cc: linux-kbuild@vger.kernel.org
>
> ---
> Bjorn Andersson (2):
> module: add SCMI device table alias support
> firmware: arm_scmi: request modules for discovered protocols
>
> drivers/clk/clk-scmi.c | 1 +
> drivers/cpufreq/scmi-cpufreq.c | 1 +
> drivers/firmware/arm_scmi/bus.c | 20 ++++++++++----------
> drivers/firmware/arm_scmi/driver.c | 3 +++
> drivers/firmware/arm_scmi/scmi_power_control.c | 1 +
> drivers/firmware/imx/sm-cpu.c | 1 +
> drivers/firmware/imx/sm-lmm.c | 1 +
> drivers/firmware/imx/sm-misc.c | 1 +
> drivers/hwmon/scmi-hwmon.c | 1 +
> drivers/iio/common/scmi_sensors/scmi_iio.c | 1 +
> drivers/input/keyboard/imx-sm-bbm-key.c | 1 +
> drivers/pmdomain/arm/scmi_perf_domain.c | 1 +
> drivers/pmdomain/arm/scmi_pm_domain.c | 1 +
> drivers/powercap/arm_scmi_powercap.c | 1 +
> drivers/regulator/scmi-regulator.c | 1 +
> drivers/reset/reset-scmi.c | 1 +
> drivers/rtc/rtc-imx-sm-bbm.c | 1 +
> include/linux/mod_devicetable.h | 12 ++++++++++++
> include/linux/scmi_protocol.h | 5 +----
> scripts/mod/devicetable-offsets.c | 4 ++++
> scripts/mod/file2alias.c | 13 +++++++++++++
> 21 files changed, 58 insertions(+), 14 deletions(-)
> ---
> base-commit: 8d6dbbbe3ba62de0a63e962ee004afb848c8e3ac
> change-id: 20260616-scmi-modalias-0f32421bd452
>
> Best regards,
> --
> Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
>
>
^ permalink raw reply
* Re: [PATCH v2 1/2] module: add SCMI device table alias support
From: Frank Li @ 2026-06-18 18:16 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Sudeep Holla, Cristian Marussi, Nathan Chancellor, Nicolas Schier,
Michael Turquette, arm-scmi, linux-arm-kernel, linux-kernel,
linux-kbuild, Hans de Goede, Stephen Boyd, Brian Masney,
Rafael J. Wysocki, Viresh Kumar, Frank Li, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Guenter Roeck,
Jyoti Bhayana, Jonathan Cameron, David Lechner, Nuno Sá,
Andy Shevchenko, Dmitry Torokhov, Ulf Hansson, Liam Girdwood,
Mark Brown, Philipp Zabel, Alexandre Belloni, linux-clk, linux-pm,
imx, linux-hwmon, linux-iio, linux-input, linux-rtc
In-Reply-To: <20260618-scmi-modalias-v2-1-8c7547c1be21@oss.qualcomm.com>
On Thu, Jun 18, 2026 at 03:56:34PM +0000, Bjorn Andersson wrote:
>
> SCMI client drivers already describe their bus match data with
> MODULE_DEVICE_TABLE(scmi, ...), but modpost does not know how to consume
> SCMI device tables. As a result, SCMI modules do not get generated module
> aliases from their id tables.
>
> Move struct scmi_device_id to mod_devicetable.h so it has a fixed layout
> visible to modpost, add the corresponding generated offsets and teach
> file2alias to emit scmi:<protocol>:<name> aliases.
>
> Use the same stable alias format for SCMI device uevents and sysfs
> modaliases. The previous string included the instance-specific device
> name, which is not useful for matching modules.
>
> Assisted-by: Codex:GPT-5.5
> Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> ---
Reviewed-by: Frank Li <Frank.Li@nxp.com>
^ permalink raw reply
* Re: [PATCH v2 1/2] module: add SCMI device table alias support
From: sashiko-bot @ 2026-06-18 16:16 UTC (permalink / raw)
To: Bjorn Andersson; +Cc: dmitry.torokhov, linux-input, linux-hwmon, Frank.Li, imx
In-Reply-To: <20260618-scmi-modalias-v2-1-8c7547c1be21@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Dropping `scmi_requested_devices_mtx` during list iteration in `scmi_protocol_device_unrequest` leads to Use-After-Free and double-free.
- [Critical] `scmi_child_dev_find` returning an unreferenced pointer causes a Use-After-Free if the device is concurrently unregistered.
- [High] If `device_register` fails in `__scmi_device_create` for the `SCMI_PROTOCOL_SYSTEM` protocol, the atomic flag `scmi_syspower_registered` is not reset, permanently preventing future registrations.
--
commit 90162c8443181b14c6b5151402b556abe1a600f4
Author: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
module: add SCMI device table alias support
SCMI client drivers already describe their bus match data with
MODULE_DEVICE_TABLE(scmi, ...), but modpost does not know how to consume
SCMI device tables. As a result, SCMI modules do not get generated module
aliases from their id tables.
> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> index 793be9eabaedd..70781146fa613 100644
> --- a/drivers/firmware/arm_scmi/bus.c
> +++ b/drivers/firmware/arm_scmi/bus.c
[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
__scmi_device_create() leak the atomic state for scmi_syspower_registered?
When device_register() fails for the SCMI_PROTOCOL_SYSTEM protocol, the code
jumps to the put_dev error path:
put_dev:
put_device(&scmi_dev->dev);
ida_free(&scmi_bus_id, id);
return NULL;
This path cleans up memory and the IDA, but it doesn't seem to reset the
global scmi_syspower_registered atomic flag back to 0. Does this permanently
block subsequent attempts to register the system power protocol until reboot?
[ ... ]
> @@ -197,18 +199,18 @@ scmi_protocol_table_unregister(const struct scmi_device_id *id_table)
> {
> const struct scmi_device_id *entry;
>
> - for (entry = id_table; entry->name; entry++)
> + for (entry = id_table; entry->name[0]; entry++)
> scmi_protocol_device_unrequest(entry);
[Severity: Critical]
This is a pre-existing issue, but can the lock dropping in
scmi_protocol_device_unrequest() lead to a use-after-free or double-free?
scmi_protocol_device_unrequest() drops the scmi_requested_devices_mtx lock
during list iteration:
mutex_lock(&scmi_requested_devices_mtx);
phead = idr_find(&scmi_requested_devices, id_table->protocol_id);
if (phead) {
...
if (!strcmp(victim->id_table->name, id_table->name)) {
list_del(&victim->node);
mutex_unlock(&scmi_requested_devices_mtx);
...
kfree(victim);
mutex_lock(&scmi_requested_devices_mtx);
break;
}
}
if (list_empty(phead)) {
idr_remove(&scmi_requested_devices,
id_table->protocol_id);
kfree(phead);
}
}
If another thread concurrently removes an entry from the same phead list,
sees the list is empty, and frees phead, wouldn't the original thread
evaluate if (list_empty(phead)) with a freed pointer when it reacquires the
lock and breaks the loop?
[ ... ]
> @@ -245,7 +247,7 @@ static struct scmi_device *scmi_child_dev_find(struct device *parent,
> struct device *dev;
>
> id_table[0].protocol_id = prot_id;
> - id_table[0].name = name;
> + strscpy(id_table[0].name, name, sizeof(id_table[0].name));
>
> dev = device_find_child(parent, &id_table, scmi_match_by_id_table);
[Severity: Critical]
This is a pre-existing issue, but does returning an unreferenced pointer here
cause a use-after-free if the device is concurrently unregistered?
Right after the device_find_child() call above, the reference is dropped:
if (!dev)
return NULL;
/* Drop the refcnt bumped implicitly by device_find_child */
put_device(dev);
return to_scmi_dev(dev);
device_find_child() increments the reference count of the device, but
put_device() immediately drops it. The caller will receive a raw pointer
without holding any locks or active references. Could the memory be freed
out from under the caller before they use it?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-scmi-modalias-v2-0-8c7547c1be21@oss.qualcomm.com?part=1
^ permalink raw reply
* Re: [PATCH v2 2/2] firmware: arm_scmi: request modules for discovered protocols
From: sashiko-bot @ 2026-06-18 16:15 UTC (permalink / raw)
To: Bjorn Andersson; +Cc: linux-hwmon, Frank.Li, linux-input, imx, dmitry.torokhov
In-Reply-To: <20260618-scmi-modalias-v2-2-8c7547c1be21@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Use-After-Free in `scmi_protocol_device_unrequest` during concurrent driver unregistration.
- [High] IDA and SystemPower protocol state leak when the SCMI platform device is unbound.
--
commit 55b65f66dfc5c3002d324389dda6dc66790201cc
Author: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
firmware: arm_scmi: request modules for discovered protocols
This commit enables autoloading for scmi driver modules by emitting a
protocol-level alias for scmi device ID table entries. It requests the
alias when discovering implemented protocols so that driver modules can
be loaded before the concrete client name is available.
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 0fd6a947499e7..7d33fab94e281 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
[ ... ]
[Severity: High]
This is a pre-existing issue, but can concurrent driver unregistration
lead to a use-after-free in drivers/firmware/arm_scmi/bus.c?
Looking at scmi_protocol_device_unrequest(), it drops the
scmi_requested_devices_mtx lock to call blocking_notifier_call_chain().
If two threads execute concurrent module unloads for different scmi
drivers on the same protocol:
Thread 1 completes the notifier, re-acquires the lock, and frees phead:
if (list_empty(phead)) {
idr_remove(&scmi_requested_devices, id_table->protocol_id);
kfree(phead);
}
Would Thread 2 then evaluate list_empty(phead) using the just-freed
phead pointer when it finishes its own notifier and re-acquires the lock?
[Severity: High]
This isn't a bug introduced by this patch, but does the scmi platform
device unbinding leak the scmi_bus_id ida and system power protocol state?
When scmi_remove() calls scmi_destroy_protocol_devices() with a NULL
name, it eventually reaches scmi_child_dev_find() in
drivers/firmware/arm_scmi/bus.c:
strscpy(id_table[0].name, name, sizeof(id_table[0].name));
Since strscpy() does not accept a NULL source pointer, doesn't this
trigger a warning and leave the target name empty?
If this empty name causes scmi_dev_match_by_id_table() to fail, the
device won't be found and the manual cleanup in __scmi_device_destroy()
is bypassed.
The device core will then automatically unregister the child devices
using:
static void scmi_device_release(struct device *dev)
{
struct scmi_device *scmi_dev = to_scmi_dev(dev);
kfree_const(scmi_dev->name);
kfree(scmi_dev);
}
Since this release callback skips explicit ida and state cleanup, does
this permanently prevent the system power protocol device from being
recreated if the driver is rebound?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-scmi-modalias-v2-0-8c7547c1be21@oss.qualcomm.com?part=2
^ permalink raw reply
* [PATCH v2 2/2] firmware: arm_scmi: request modules for discovered protocols
From: Bjorn Andersson @ 2026-06-18 15:56 UTC (permalink / raw)
To: Sudeep Holla, Cristian Marussi, Nathan Chancellor, Nicolas Schier,
Michael Turquette
Cc: arm-scmi, linux-arm-kernel, linux-kernel, linux-kbuild,
Hans de Goede, Bjorn Andersson, Stephen Boyd, Brian Masney,
Rafael J. Wysocki, Viresh Kumar, Frank Li, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Guenter Roeck,
Jyoti Bhayana, Jonathan Cameron, David Lechner, Nuno Sá,
Andy Shevchenko, Dmitry Torokhov, Ulf Hansson, Liam Girdwood,
Mark Brown, Philipp Zabel, Alexandre Belloni, linux-clk, linux-pm,
imx, linux-hwmon, linux-iio, linux-input, linux-rtc
In-Reply-To: <20260618-scmi-modalias-v2-0-8c7547c1be21@oss.qualcomm.com>
SCMI client devices are created from SCMI driver id tables. If such a
driver is modular, the core does not know the driver's client name until
the module has already loaded, so normal device uevent based autoloading
cannot break the dependency cycle.
Emit a protocol-level alias for each SCMI device id table entry and
request that alias when the SCMI core discovers an implemented protocol.
This loads modules that have registered interest in the protocol; their
normal SCMI driver registration then requests the concrete client device
and the SCMI bus matches it by protocol and name.
This allows e.g. ARM_SCMI_CPUFREQ=m to autoload on systems that expose
only the SCMI Performance protocol node, where the cpufreq client name
is Linux-internal and not available from firmware before loading the
module.
Assisted-by: Codex:GPT-5.5
Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
drivers/firmware/arm_scmi/driver.c | 2 ++
include/linux/mod_devicetable.h | 1 +
scripts/mod/file2alias.c | 4 +++-
3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 0fd6a947499e..7d33fab94e28 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -48,6 +48,7 @@
#include <trace/events/scmi.h>
#define SCMI_VENDOR_MODULE_ALIAS_FMT "scmi-protocol-0x%02x-%s"
+#define SCMI_MODULE_ALIAS_FMT SCMI_PROTOCOL_MODULE_PREFIX "0x%02x"
static DEFINE_IDA(scmi_id);
@@ -3363,6 +3364,7 @@ static int scmi_probe(struct platform_device *pdev)
}
of_node_get(child);
+ request_module_nowait(SCMI_MODULE_ALIAS_FMT, prot_id);
scmi_create_protocol_devices(child, info, prot_id, NULL);
}
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 769382f2eadd..2cc7e78e35a3 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -477,6 +477,7 @@ struct rpmsg_device_id {
#define SCMI_NAME_SIZE 32
#define SCMI_MODULE_PREFIX "scmi:"
+#define SCMI_PROTOCOL_MODULE_PREFIX "scmi-protocol-"
struct scmi_device_id {
__u8 protocol_id;
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index a5283f4c8e6f..40a37b6bf1ad 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -852,7 +852,7 @@ static void do_rpmsg_entry(struct module *mod, void *symval)
module_alias_printf(mod, false, RPMSG_DEVICE_MODALIAS_FMT, *name);
}
-/* Looks like: scmi:NN:S */
+/* Looks like: scmi:NN:S and scmi-protocol-0xNN */
static void do_scmi_entry(struct module *mod, void *symval)
{
DEF_FIELD(symval, scmi_device_id, protocol_id);
@@ -860,6 +860,8 @@ static void do_scmi_entry(struct module *mod, void *symval)
module_alias_printf(mod, false, SCMI_MODULE_PREFIX "%02x:%s",
protocol_id, *name);
+ module_alias_printf(mod, false, SCMI_PROTOCOL_MODULE_PREFIX "0x%02x",
+ protocol_id);
}
/* Looks like: i2c:S */
--
2.53.0
^ permalink raw reply related
* [PATCH v2 1/2] module: add SCMI device table alias support
From: Bjorn Andersson @ 2026-06-18 15:56 UTC (permalink / raw)
To: Sudeep Holla, Cristian Marussi, Nathan Chancellor, Nicolas Schier,
Michael Turquette
Cc: arm-scmi, linux-arm-kernel, linux-kernel, linux-kbuild,
Hans de Goede, Bjorn Andersson, Stephen Boyd, Brian Masney,
Rafael J. Wysocki, Viresh Kumar, Frank Li, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Guenter Roeck,
Jyoti Bhayana, Jonathan Cameron, David Lechner, Nuno Sá,
Andy Shevchenko, Dmitry Torokhov, Ulf Hansson, Liam Girdwood,
Mark Brown, Philipp Zabel, Alexandre Belloni, linux-clk, linux-pm,
imx, linux-hwmon, linux-iio, linux-input, linux-rtc
In-Reply-To: <20260618-scmi-modalias-v2-0-8c7547c1be21@oss.qualcomm.com>
SCMI client drivers already describe their bus match data with
MODULE_DEVICE_TABLE(scmi, ...), but modpost does not know how to consume
SCMI device tables. As a result, SCMI modules do not get generated module
aliases from their id tables.
Move struct scmi_device_id to mod_devicetable.h so it has a fixed layout
visible to modpost, add the corresponding generated offsets and teach
file2alias to emit scmi:<protocol>:<name> aliases.
Use the same stable alias format for SCMI device uevents and sysfs
modaliases. The previous string included the instance-specific device
name, which is not useful for matching modules.
Assisted-by: Codex:GPT-5.5
Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
drivers/clk/clk-scmi.c | 1 +
drivers/cpufreq/scmi-cpufreq.c | 1 +
drivers/firmware/arm_scmi/bus.c | 20 ++++++++++----------
drivers/firmware/arm_scmi/driver.c | 1 +
drivers/firmware/arm_scmi/scmi_power_control.c | 1 +
drivers/firmware/imx/sm-cpu.c | 1 +
drivers/firmware/imx/sm-lmm.c | 1 +
drivers/firmware/imx/sm-misc.c | 1 +
drivers/hwmon/scmi-hwmon.c | 1 +
drivers/iio/common/scmi_sensors/scmi_iio.c | 1 +
drivers/input/keyboard/imx-sm-bbm-key.c | 1 +
drivers/pmdomain/arm/scmi_perf_domain.c | 1 +
drivers/pmdomain/arm/scmi_pm_domain.c | 1 +
drivers/powercap/arm_scmi_powercap.c | 1 +
drivers/regulator/scmi-regulator.c | 1 +
drivers/reset/reset-scmi.c | 1 +
drivers/rtc/rtc-imx-sm-bbm.c | 1 +
include/linux/mod_devicetable.h | 11 +++++++++++
include/linux/scmi_protocol.h | 5 +----
scripts/mod/devicetable-offsets.c | 4 ++++
scripts/mod/file2alias.c | 11 +++++++++++
21 files changed, 53 insertions(+), 14 deletions(-)
diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index 7c562559ad8b..b9e29e124302 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -11,6 +11,7 @@
#include <linux/err.h>
#include <linux/of.h>
#include <linux/module.h>
+#include <linux/mod_devicetable.h>
#include <linux/scmi_protocol.h>
#define NOT_ATOMIC false
diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
index 4edb4f7a8aa9..affa005bf8b1 100644
--- a/drivers/cpufreq/scmi-cpufreq.c
+++ b/drivers/cpufreq/scmi-cpufreq.c
@@ -15,6 +15,7 @@
#include <linux/energy_model.h>
#include <linux/export.h>
#include <linux/module.h>
+#include <linux/mod_devicetable.h>
#include <linux/of.h>
#include <linux/pm_opp.h>
#include <linux/pm_qos.h>
diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index 793be9eabaed..70781146fa61 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -10,14 +10,16 @@
#include <linux/atomic.h>
#include <linux/types.h>
#include <linux/module.h>
+#include <linux/mod_devicetable.h>
#include <linux/of.h>
#include <linux/kernel.h>
#include <linux/slab.h>
+#include <linux/string.h>
#include <linux/device.h>
#include "common.h"
-#define SCMI_UEVENT_MODALIAS_FMT "%s:%02x:%s"
+#define SCMI_UEVENT_MODALIAS_FMT SCMI_MODULE_PREFIX "%02x:%s"
BLOCKING_NOTIFIER_HEAD(scmi_requested_devices_nh);
EXPORT_SYMBOL_GPL(scmi_requested_devices_nh);
@@ -141,7 +143,7 @@ static int scmi_protocol_table_register(const struct scmi_device_id *id_table)
int ret = 0;
const struct scmi_device_id *entry;
- for (entry = id_table; entry->name && ret == 0; entry++)
+ for (entry = id_table; entry->name[0] && ret == 0; entry++)
ret = scmi_protocol_device_request(entry);
return ret;
@@ -197,18 +199,18 @@ scmi_protocol_table_unregister(const struct scmi_device_id *id_table)
{
const struct scmi_device_id *entry;
- for (entry = id_table; entry->name; entry++)
+ for (entry = id_table; entry->name[0]; entry++)
scmi_protocol_device_unrequest(entry);
}
static int scmi_dev_match_by_id_table(struct scmi_device *scmi_dev,
const struct scmi_device_id *id_table)
{
- if (!id_table || !id_table->name)
+ if (!id_table || !id_table->name[0])
return 0;
/* Always skip transport devices from matching */
- for (; id_table->protocol_id && id_table->name; id_table++)
+ for (; id_table->protocol_id && id_table->name[0]; id_table++)
if (id_table->protocol_id == scmi_dev->protocol_id &&
strncmp(scmi_dev->name, "__scmi_transport_device", 23) &&
!strcmp(id_table->name, scmi_dev->name))
@@ -245,7 +247,7 @@ static struct scmi_device *scmi_child_dev_find(struct device *parent,
struct device *dev;
id_table[0].protocol_id = prot_id;
- id_table[0].name = name;
+ strscpy(id_table[0].name, name, sizeof(id_table[0].name));
dev = device_find_child(parent, &id_table, scmi_match_by_id_table);
if (!dev)
@@ -282,8 +284,7 @@ static int scmi_device_uevent(const struct device *dev, struct kobj_uevent_env *
const struct scmi_device *scmi_dev = to_scmi_dev(dev);
return add_uevent_var(env, "MODALIAS=" SCMI_UEVENT_MODALIAS_FMT,
- dev_name(&scmi_dev->dev), scmi_dev->protocol_id,
- scmi_dev->name);
+ scmi_dev->protocol_id, scmi_dev->name);
}
static ssize_t modalias_show(struct device *dev,
@@ -292,8 +293,7 @@ static ssize_t modalias_show(struct device *dev,
struct scmi_device *scmi_dev = to_scmi_dev(dev);
return sysfs_emit(buf, SCMI_UEVENT_MODALIAS_FMT,
- dev_name(&scmi_dev->dev), scmi_dev->protocol_id,
- scmi_dev->name);
+ scmi_dev->protocol_id, scmi_dev->name);
}
static DEVICE_ATTR_RO(modalias);
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 3e0d975ec94c..0fd6a947499e 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -30,6 +30,7 @@
#include <linux/hashtable.h>
#include <linux/list.h>
#include <linux/module.h>
+#include <linux/mod_devicetable.h>
#include <linux/of.h>
#include <linux/platform_device.h>
#include <linux/processor.h>
diff --git a/drivers/firmware/arm_scmi/scmi_power_control.c b/drivers/firmware/arm_scmi/scmi_power_control.c
index 955736336061..1900bb77383e 100644
--- a/drivers/firmware/arm_scmi/scmi_power_control.c
+++ b/drivers/firmware/arm_scmi/scmi_power_control.c
@@ -45,6 +45,7 @@
#include <linux/math.h>
#include <linux/module.h>
+#include <linux/mod_devicetable.h>
#include <linux/mutex.h>
#include <linux/pm.h>
#include <linux/printk.h>
diff --git a/drivers/firmware/imx/sm-cpu.c b/drivers/firmware/imx/sm-cpu.c
index 091b014f739f..53a8d1cee5ea 100644
--- a/drivers/firmware/imx/sm-cpu.c
+++ b/drivers/firmware/imx/sm-cpu.c
@@ -5,6 +5,7 @@
#include <linux/firmware/imx/sm.h>
#include <linux/module.h>
+#include <linux/mod_devicetable.h>
#include <linux/of.h>
#include <linux/platform_device.h>
#include <linux/scmi_protocol.h>
diff --git a/drivers/firmware/imx/sm-lmm.c b/drivers/firmware/imx/sm-lmm.c
index 6807bf563c03..f4dc198187a8 100644
--- a/drivers/firmware/imx/sm-lmm.c
+++ b/drivers/firmware/imx/sm-lmm.c
@@ -5,6 +5,7 @@
#include <linux/firmware/imx/sm.h>
#include <linux/module.h>
+#include <linux/mod_devicetable.h>
#include <linux/of.h>
#include <linux/platform_device.h>
#include <linux/scmi_protocol.h>
diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
index ac9af824c2d4..5e39e79a9d8a 100644
--- a/drivers/firmware/imx/sm-misc.c
+++ b/drivers/firmware/imx/sm-misc.c
@@ -7,6 +7,7 @@
#include <linux/device/devres.h>
#include <linux/firmware/imx/sm.h>
#include <linux/module.h>
+#include <linux/mod_devicetable.h>
#include <linux/of.h>
#include <linux/platform_device.h>
#include <linux/scmi_protocol.h>
diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
index eec223d174c0..57b91e931c5d 100644
--- a/drivers/hwmon/scmi-hwmon.c
+++ b/drivers/hwmon/scmi-hwmon.c
@@ -8,6 +8,7 @@
#include <linux/hwmon.h>
#include <linux/module.h>
+#include <linux/mod_devicetable.h>
#include <linux/scmi_protocol.h>
#include <linux/slab.h>
#include <linux/sysfs.h>
diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
index 442b40ef27cf..3babc4261965 100644
--- a/drivers/iio/common/scmi_sensors/scmi_iio.c
+++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
@@ -15,6 +15,7 @@
#include <linux/kernel.h>
#include <linux/kthread.h>
#include <linux/module.h>
+#include <linux/mod_devicetable.h>
#include <linux/mutex.h>
#include <linux/scmi_protocol.h>
#include <linux/time.h>
diff --git a/drivers/input/keyboard/imx-sm-bbm-key.c b/drivers/input/keyboard/imx-sm-bbm-key.c
index 96486bd23d60..36e349136ee7 100644
--- a/drivers/input/keyboard/imx-sm-bbm-key.c
+++ b/drivers/input/keyboard/imx-sm-bbm-key.c
@@ -6,6 +6,7 @@
#include <linux/input.h>
#include <linux/jiffies.h>
#include <linux/module.h>
+#include <linux/mod_devicetable.h>
#include <linux/of.h>
#include <linux/platform_device.h>
#include <linux/rtc.h>
diff --git a/drivers/pmdomain/arm/scmi_perf_domain.c b/drivers/pmdomain/arm/scmi_perf_domain.c
index 3693423459c9..741375ad325b 100644
--- a/drivers/pmdomain/arm/scmi_perf_domain.c
+++ b/drivers/pmdomain/arm/scmi_perf_domain.c
@@ -8,6 +8,7 @@
#include <linux/err.h>
#include <linux/device.h>
#include <linux/module.h>
+#include <linux/mod_devicetable.h>
#include <linux/pm_domain.h>
#include <linux/pm_opp.h>
#include <linux/scmi_protocol.h>
diff --git a/drivers/pmdomain/arm/scmi_pm_domain.c b/drivers/pmdomain/arm/scmi_pm_domain.c
index 3d73aef21d2f..0948d05c9e3c 100644
--- a/drivers/pmdomain/arm/scmi_pm_domain.c
+++ b/drivers/pmdomain/arm/scmi_pm_domain.c
@@ -8,6 +8,7 @@
#include <linux/err.h>
#include <linux/io.h>
#include <linux/module.h>
+#include <linux/mod_devicetable.h>
#include <linux/pm_domain.h>
#include <linux/scmi_protocol.h>
diff --git a/drivers/powercap/arm_scmi_powercap.c b/drivers/powercap/arm_scmi_powercap.c
index ab66e9a3b1e2..332e4e26f1e5 100644
--- a/drivers/powercap/arm_scmi_powercap.c
+++ b/drivers/powercap/arm_scmi_powercap.c
@@ -10,6 +10,7 @@
#include <linux/limits.h>
#include <linux/list.h>
#include <linux/module.h>
+#include <linux/mod_devicetable.h>
#include <linux/powercap.h>
#include <linux/scmi_protocol.h>
#include <linux/slab.h>
diff --git a/drivers/regulator/scmi-regulator.c b/drivers/regulator/scmi-regulator.c
index c005e65ba0ec..f55f228cb133 100644
--- a/drivers/regulator/scmi-regulator.c
+++ b/drivers/regulator/scmi-regulator.c
@@ -25,6 +25,7 @@
#include <linux/linear_range.h>
#include <linux/module.h>
+#include <linux/mod_devicetable.h>
#include <linux/of.h>
#include <linux/regulator/driver.h>
#include <linux/regulator/machine.h>
diff --git a/drivers/reset/reset-scmi.c b/drivers/reset/reset-scmi.c
index 4335811e0cfa..a6739df1d3c2 100644
--- a/drivers/reset/reset-scmi.c
+++ b/drivers/reset/reset-scmi.c
@@ -6,6 +6,7 @@
*/
#include <linux/module.h>
+#include <linux/mod_devicetable.h>
#include <linux/of.h>
#include <linux/device.h>
#include <linux/reset-controller.h>
diff --git a/drivers/rtc/rtc-imx-sm-bbm.c b/drivers/rtc/rtc-imx-sm-bbm.c
index daa472be7c80..c8643718cef1 100644
--- a/drivers/rtc/rtc-imx-sm-bbm.c
+++ b/drivers/rtc/rtc-imx-sm-bbm.c
@@ -5,6 +5,7 @@
#include <linux/jiffies.h>
#include <linux/module.h>
+#include <linux/mod_devicetable.h>
#include <linux/platform_device.h>
#include <linux/rtc.h>
#include <linux/scmi_protocol.h>
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 3b0c9a251a2e..769382f2eadd 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -473,6 +473,17 @@ struct rpmsg_device_id {
kernel_ulong_t driver_data;
};
+/* scmi */
+
+#define SCMI_NAME_SIZE 32
+#define SCMI_MODULE_PREFIX "scmi:"
+
+struct scmi_device_id {
+ __u8 protocol_id;
+ char name[SCMI_NAME_SIZE];
+ kernel_ulong_t driver_data;
+};
+
/* i2c */
#define I2C_NAME_SIZE 20
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 5ab73b1ab9aa..61f5ecfe0133 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -951,10 +951,7 @@ struct scmi_device {
#define to_scmi_dev(d) container_of_const(d, struct scmi_device, dev)
-struct scmi_device_id {
- u8 protocol_id;
- const char *name;
-};
+struct scmi_device_id;
struct scmi_driver {
const char *name;
diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
index b4178c42d08f..da5bd712c8da 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -144,6 +144,10 @@ int main(void)
DEVID(rpmsg_device_id);
DEVID_FIELD(rpmsg_device_id, name);
+ DEVID(scmi_device_id);
+ DEVID_FIELD(scmi_device_id, protocol_id);
+ DEVID_FIELD(scmi_device_id, name);
+
DEVID(i2c_device_id);
DEVID_FIELD(i2c_device_id, name);
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 8d36c74dec2d..a5283f4c8e6f 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -852,6 +852,16 @@ static void do_rpmsg_entry(struct module *mod, void *symval)
module_alias_printf(mod, false, RPMSG_DEVICE_MODALIAS_FMT, *name);
}
+/* Looks like: scmi:NN:S */
+static void do_scmi_entry(struct module *mod, void *symval)
+{
+ DEF_FIELD(symval, scmi_device_id, protocol_id);
+ DEF_FIELD_ADDR(symval, scmi_device_id, name);
+
+ module_alias_printf(mod, false, SCMI_MODULE_PREFIX "%02x:%s",
+ protocol_id, *name);
+}
+
/* Looks like: i2c:S */
static void do_i2c_entry(struct module *mod, void *symval)
{
@@ -1491,6 +1501,7 @@ static const struct devtable devtable[] = {
{"virtio", SIZE_virtio_device_id, do_virtio_entry},
{"vmbus", SIZE_hv_vmbus_device_id, do_vmbus_entry},
{"rpmsg", SIZE_rpmsg_device_id, do_rpmsg_entry},
+ {"scmi", SIZE_scmi_device_id, do_scmi_entry},
{"i2c", SIZE_i2c_device_id, do_i2c_entry},
{"i3c", SIZE_i3c_device_id, do_i3c_entry},
{"slim", SIZE_slim_device_id, do_slim_entry},
--
2.53.0
^ permalink raw reply related
* [PATCH v2 0/2] firmware: arm_scmi: Ensure automatic module loading
From: Bjorn Andersson @ 2026-06-18 15:56 UTC (permalink / raw)
To: Sudeep Holla, Cristian Marussi, Nathan Chancellor, Nicolas Schier,
Michael Turquette
Cc: arm-scmi, linux-arm-kernel, linux-kernel, linux-kbuild,
Hans de Goede, Bjorn Andersson, Stephen Boyd, Brian Masney,
Rafael J. Wysocki, Viresh Kumar, Frank Li, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Guenter Roeck,
Jyoti Bhayana, Jonathan Cameron, David Lechner, Nuno Sá,
Andy Shevchenko, Dmitry Torokhov, Ulf Hansson, Liam Girdwood,
Mark Brown, Philipp Zabel, Alexandre Belloni, linux-clk, linux-pm,
imx, linux-hwmon, linux-iio, linux-input, linux-rtc
SCMI drivers such as the Arm SCMI CPUfreq driver are allowed to built as
modules, but they are then not automatically loaded. Rework the SCMI
device table alias support to make modpost consume the information from
MODULE_DEVICE_TABLE(scmi, ...) and allow drivers to be loaded based on
this information, if known. Also add a protocol-based alias to also
trigger driver loading when only the SCMI protocol id is known.
Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
Changes in v2:
- Use request_module_nowait()
- Drop #include <linux/mod_devicetable.h> from scmi_protocol.h
- Link to v1: https://patch.msgid.link/20260616-scmi-modalias-v1-0-662b8dd52ab2@oss.qualcomm.com
To: Sudeep Holla <sudeep.holla@kernel.org>
To: Cristian Marussi <cristian.marussi@arm.com>
To: Michael Turquette <mturquette@baylibre.com>
To: Nicolas Schier <nsc@kernel.org>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Brian Masney <bmasney@redhat.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Frank Li <Frank.Li@nxp.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Jyoti Bhayana <jbhayana@google.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: David Lechner <dlechner@baylibre.com>
Cc: Nuno Sá <nuno.sa@analog.com>
Cc: Andy Shevchenko <andy@kernel.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Ulf Hansson <ulfh@kernel.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: arm-scmi@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-clk@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-pm@vger.kernel.org
Cc: imx@lists.linux.dev
Cc: linux-hwmon@vger.kernel.org
Cc: linux-iio@vger.kernel.org
Cc: linux-input@vger.kernel.org
Cc: linux-rtc@vger.kernel.org
Cc: linux-kbuild@vger.kernel.org
---
Bjorn Andersson (2):
module: add SCMI device table alias support
firmware: arm_scmi: request modules for discovered protocols
drivers/clk/clk-scmi.c | 1 +
drivers/cpufreq/scmi-cpufreq.c | 1 +
drivers/firmware/arm_scmi/bus.c | 20 ++++++++++----------
drivers/firmware/arm_scmi/driver.c | 3 +++
drivers/firmware/arm_scmi/scmi_power_control.c | 1 +
drivers/firmware/imx/sm-cpu.c | 1 +
drivers/firmware/imx/sm-lmm.c | 1 +
drivers/firmware/imx/sm-misc.c | 1 +
drivers/hwmon/scmi-hwmon.c | 1 +
drivers/iio/common/scmi_sensors/scmi_iio.c | 1 +
drivers/input/keyboard/imx-sm-bbm-key.c | 1 +
drivers/pmdomain/arm/scmi_perf_domain.c | 1 +
drivers/pmdomain/arm/scmi_pm_domain.c | 1 +
drivers/powercap/arm_scmi_powercap.c | 1 +
drivers/regulator/scmi-regulator.c | 1 +
drivers/reset/reset-scmi.c | 1 +
drivers/rtc/rtc-imx-sm-bbm.c | 1 +
include/linux/mod_devicetable.h | 12 ++++++++++++
include/linux/scmi_protocol.h | 5 +----
scripts/mod/devicetable-offsets.c | 4 ++++
scripts/mod/file2alias.c | 13 +++++++++++++
21 files changed, 58 insertions(+), 14 deletions(-)
---
base-commit: 8d6dbbbe3ba62de0a63e962ee004afb848c8e3ac
change-id: 20260616-scmi-modalias-0f32421bd452
Best regards,
--
Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
^ permalink raw reply
* Re: [PATCH v8 2/7] mfd: Add driver for ASUS Transformer embedded controller
From: Svyatoslav Ryhel @ 2026-06-18 12:54 UTC (permalink / raw)
To: Lee Jones
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <20260618122605.GH1672911@google.com>
чт, 18 черв. 2026 р. о 15:26 Lee Jones <lee@kernel.org> пише:
>
> On Thu, 11 Jun 2026, Svyatoslav Ryhel wrote:
>
> > чт, 11 черв. 2026 р. о 14:17 Lee Jones <lee@kernel.org> пише:
> > >
> > > On Thu, 28 May 2026, Svyatoslav Ryhel wrote:
> > > > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > >
> > > > Support Nuvoton NPCE795-based ECs as used in Asus Transformer TF201,
> > > > TF300T, TF300TG, TF300TL and TF700T pad and dock, as well as TF101 dock
> > > > and TF600T, P1801-T and TF701T pad. This is a glue driver handling
> > > > detection and common operations for EC's functions.
> > > >
> > > > Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > > ---
> > > > drivers/mfd/Kconfig | 16 +
> > > > drivers/mfd/Makefile | 1 +
> > > > drivers/mfd/asus-transformer-ec.c | 542 ++++++++++++++++++++++++
> > > > include/linux/mfd/asus-transformer-ec.h | 92 ++++
> > > > 4 files changed, 651 insertions(+)
> > > > create mode 100644 drivers/mfd/asus-transformer-ec.c
> > > > create mode 100644 include/linux/mfd/asus-transformer-ec.h
>
> [...]
>
> > > > +static void asus_ec_clear_buffer(struct asus_ec_data *ddata)
> > > > +{
> > > > + int ret, retry = ASUSEC_RSP_BUFFER_SIZE;
> > > > +
> > > > + /*
> > > > + * Read the buffer till we get valid data by checking ASUSEC_OBF_MASK
> > > > + * of the status byte or till we reach end of the 256 byte buffer.
> > > > + */
> > > > + while (retry--) {
> > > > + ret = i2c_smbus_read_i2c_block_data(ddata->client, ASUSEC_READ_BUF,
> > > > + ASUSEC_ENTRY_SIZE,
> > > > + ddata->ec_buf);
> > > > + if (ret < ASUSEC_ENTRY_SIZE)
> > > > + continue;
> > > > +
> > > > + if (ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK)
> > > > + continue;
> > > > +
> > > > + break;
> > > > + }
> > > > +}
> > > > +
> > > > +static int asus_ec_log_info(struct asus_ec_data *ddata, unsigned int reg,
> > > > + const char *name, const char **out)
>
> If we can avoid points to pointers, then please do.
>
> We already have ddata, so we can just set the name?
>
> It will remove a lot of the following complexity / ugliness.
>
> > > > +{
> > > > + struct device *dev = &ddata->client->dev;
> > > > + u8 buf[ASUSEC_ENTRY_BUFSIZE];
> > > > + int ret;
> > > > +
> > > > + memset(buf, 0, ASUSEC_ENTRY_BUFSIZE);
> > > > + ret = i2c_smbus_read_i2c_block_data(ddata->ec.dockram, reg,
> > > > + ASUSEC_ENTRY_SIZE, buf);
> > > > + if (ret < ASUSEC_ENTRY_SIZE)
> > > > + return ret;
> > >
> > > Same here. These should be negative.
> > >
> >
> > return ret < 0 ? ret : -EIO same as above
> >
> > > > +
> > > > + if (buf[0] > ASUSEC_ENTRY_SIZE) {
> > > > + dev_err(dev, "bad data len; buffer: %*ph; ret: %d\n",
> > > > + ASUSEC_ENTRY_BUFSIZE, buf, ret);
> > > > + return -EPROTO;
> > > > + }
> > > > +
> > > > + if (!ddata->logging_disabled) {
> > > > + dev_info(dev, "%-14s: %.*s\n", name, buf[0], buf + 1);
> > > > +
> > > > + if (out) {
> > > > + *out = devm_kasprintf(dev, GFP_KERNEL, "%.*s",
> > > > + buf[0], buf + 1);
> > > > + if (!*out)
> > > > + return -ENOMEM;
> > > > + }
> > > > + }
> > >
> > > FWIW, I hate this! What does it give you now that development is done?
> > >
> >
> > We have already discussed this, and you agreed that EC and firmware
> > prints may stay! This prints EC model and firmware info as well as EC
> > firmware behavior. It allows identify possible new revisions of EC -
> > Firmware combo and address possible regressions (check if it is chip
> > malfunction or firmware needs a new programming model) without
> > rebuilding kernel and digging downstream kernel for needed bits of
> > code.
>
> Right, so just print it out and remove all of the 'logging_disabled' and
> 'out' nonsense.
>
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int asus_ec_reset(struct asus_ec_data *ddata)
> > > > +{
> > > > + int retry, ret;
> > > > +
> > > > + guard(mutex)(&ddata->ecreq_lock);
> > > > +
> > > > + for (retry = 0; retry < ASUSEC_RETRY_MAX; retry++) {
> > >
> > > for (int retry = ... is generally preferred for throwaway variables.
> > >
> >
> > Not that I care too much, but I am defining ret anyway, why not add
> > retry too there?
>
> This is the new and preferred way to use throw-away variables.
>
> ret is not a throw-away variable.
>
> [...]
>
> > > > +static int asus_ec_set_factory_mode(struct asus_ec_data *ddata,
> > > > + enum asusec_mode fmode)
> > > > +{
> > > > + dev_info(&ddata->client->dev, "Entering %s mode.\n",
> > > > + fmode == ASUSEC_MODE_FACTORY ? "factory" : "normal");
> > > > +
> > > > + return asus_dockram_access_ctl(ddata->ec.dockram, NULL,
> > > > + ASUSEC_CTL_FACTORY_MODE,
> > > > + fmode == ASUSEC_MODE_FACTORY ?
> > > > + ASUSEC_CTL_FACTORY_MODE : 0);
> > >
> > > Why not create make:
> > >
> > > ASUSEC_MODE_FACTORY == ASUSEC_CTL_FACTORY_MODE
> > >
> > > What happens to NORMAL?
> > >
> >
> > ASUSEC_CTL_FACTORY_MODE is a bit in the ctl register. For NORMAL mode
>
> I get that, but if the values can be shared, it make the code simpler.
>
> > bit is cleared,
> > for FACTORY bit it set, for NONE bit is ignored.
> >
> > > > +}
> > > > +
> > > > +static int asus_ec_detect(struct asus_ec_data *ddata)
> > > > +{
> > > > + int ret;
> > > > +
> > > > + ret = asus_ec_reset(ddata);
> > > > + if (ret)
> > > > + goto err_exit;
> > > > +
> > > > + asus_ec_clear_buffer(ddata);
> > > > +
> > > > + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_MODEL, "Model",
> > > > + &ddata->ec.model);
>
> Where is this model used?
>
Model is passed to cells to form names, particularly input device names.
> > > You can use 100-chars and make the code look beautiful! :)
> > >
> >
> > Not every subsystem permits 100 chars, some stick to 80 as a strict
> > rule, so it is better be safe.
>
> Right, but we are forward thinking here.
>
> You can and should use 100-chars in this subsystem.
>
> > > > + if (ret)
> > > > + goto err_exit;
> > > > +
> > > > + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_FW, "FW version",
> > > > + NULL);
> > > > + if (ret)
> > > > + goto err_exit;
> > > > +
> > > > + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_CFGFMT, "Config format",
> > > > + NULL);
> > > > + if (ret)
> > > > + goto err_exit;
> > > > +
> > > > + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_HW, "HW version",
> > > > + NULL);
> > > > + if (ret)
> > > > + goto err_exit;
> > > > +
> > > > + /* Disable logging on next EC request */
> > >
> > > Why, but why?
> > >
> >
> > Cause EC requests are frequent (handshake/reset) and constant logging
> > same data is not acceptable.
>
> Then rid the prints entirely or do them at a more appropriate point like
> during probe?
>
> Or maybe consider dev_info_once() and friends.
>
I totally forgot about dev_info_once(), thank you.
> > > > + ddata->logging_disabled = true;
> > > > +
> > > > + /* Check and inform about EC firmware behavior */
> > > > + ret = asus_ec_susb_on_status(ddata);
> > > > + if (ret)
> > > > + goto err_exit;
> > > > +
> > > > + ddata->ec.name = ddata->info->name;
> > > > +
> > > > + /* Some EC require factory mode to be set normal on each request */
> > > > + if (ddata->info->fmode)
> > > > + ret = asus_ec_set_factory_mode(ddata, ddata->info->fmode);
> > > > +
> > > > +err_exit:
> > > > + if (ret)
> > > > + dev_err(&ddata->client->dev, "failed to access EC: %d\n", ret);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static void asus_ec_handle_smi(struct asus_ec_data *ddata, unsigned int code)
> > > > +{
> > > > + switch (code) {
> > > > + case ASUSEC_SMI_HANDSHAKE:
> > > > + case ASUSEC_SMI_RESET:
> > > > + asus_ec_detect(ddata);
> > > > + break;
> > > > + }
> > > > +}
> > > > +
> > > > +static irqreturn_t asus_ec_interrupt(int irq, void *dev_id)
> > > > +{
> > > > + struct asus_ec_data *ddata = dev_id;
> > > > + unsigned long notify_action;
> > > > + int ret;
> > > > +
> > > > + ret = i2c_smbus_read_i2c_block_data(ddata->client, ASUSEC_READ_BUF,
> > > > + ASUSEC_ENTRY_SIZE, ddata->ec_buf);
> > > > + if (ret < ASUSEC_ENTRY_SIZE ||
> > > > + !(ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK))
> > >
> > > Unwrap for readability.
> > >
> > > Also, I think a comment would be helpful.
> > >
> >
> > if (ret < ASUSEC_ENTRY_SIZE)
> > return IRQ_NONE;
> >
> > ret = ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK;
> > if (!ret)
> > return IRQ_NONE;
> >
> > This would be acceptable? (I will add comments later on)
>
> Yes, better.
>
> If you're not using ret again, you could just put 'ddata.." inside the if().
>
I thought about this, but that would require combining it with (!)
which will not help with readabilty.
> > > > + return IRQ_NONE;
> > > > +
> > > > + notify_action = ddata->ec_buf[ASUSEC_IRQ_STATUS];
> > > > + if (notify_action & ASUSEC_SMI_MASK) {
> > > > + unsigned int code = ddata->ec_buf[ASUSEC_SMI_CODE];
> > > > +
> > > > + asus_ec_handle_smi(ddata, code);
> > > > +
> > > > + notify_action |= code << 8;
> > > > + }
> > > > +
> > > > + blocking_notifier_call_chain(&ddata->ec.notify_list,
> > > > + notify_action, ddata->ec_buf);
> > > > +
> > > > + return IRQ_HANDLED;
> > > > +}
> > > > +
> > > > +static void asus_ec_release_dockram_dev(void *client)
> > > > +{
> > > > + i2c_unregister_device(client);
> > > > +}
> > > > +
> > > > +static struct i2c_client *devm_asus_dockram_get(struct device *dev)
> > > > +{
> > > > + struct i2c_client *parent = to_i2c_client(dev);
> > > > + struct i2c_client *dockram;
> > > > + struct dockram_ec_data *ddata;
> > > > + int ret;
> > > > +
> > > > + dockram = i2c_new_ancillary_device(parent, "dockram",
> > > > + parent->addr + 2);
> > >
> > > Could we define a macro for the address offset '2' here to avoid using a magic
> > > number?
> > >
> >
> > It seems that you are excessively concerned with "magic numbers".
>
> Bingo! I HATE magic numbers.
>
> https://lore.kernel.org/all/?q=%22Lee+Jones%22+magic
>
> ~900 messages! =:-D
>
AHAHAH, ok, this makes things clearer. Lemmy have a note about this quirk.
> [...]
>
> > > > +static const struct asus_ec_chip_info asus_ec_tf600t_pad_data = {
> > > > + .name = "pad",
> > > > + .variant = ASUSEC_TF600T_PAD,
> > > > + .fmode = ASUSEC_MODE_NORMAL,
> > > > +};
> > >
> > > Any reason not to just pass the identifier (variant) and add the name
> > > and fmode attribues to the switch() above?
> >
> > Why not set it here, I am not passing any mfd or any other API via of data.
>
> I get that, and you're not breaking any of my golden rules.
>
> However, I just think doing everything in one place, usually a which
> based off of the 'variant' which you pass as a single value, is a nicer,
> more consolidated way of doing things.
>
Well, I would need to pass OF data regardless, so why not bundle it
with all required info and leave probes switch for strictly mfd cell
assignment. I suppose this is more a personal preference issue. I hope
you will not mind if I leave this as it is?
All other comments you have left and I did not answer directly were
read and acknowledged. Thank you!
> --
> Lee Jones
^ permalink raw reply
* Re: [PATCH v8 2/7] mfd: Add driver for ASUS Transformer embedded controller
From: Lee Jones @ 2026-06-18 12:26 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <CAPVz0n0caBBt6A+AFeUpGdxvb3Qhoui7khLCt3747bPUKmMXhQ@mail.gmail.com>
On Thu, 11 Jun 2026, Svyatoslav Ryhel wrote:
> чт, 11 черв. 2026 р. о 14:17 Lee Jones <lee@kernel.org> пише:
> >
> > On Thu, 28 May 2026, Svyatoslav Ryhel wrote:
> > > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > >
> > > Support Nuvoton NPCE795-based ECs as used in Asus Transformer TF201,
> > > TF300T, TF300TG, TF300TL and TF700T pad and dock, as well as TF101 dock
> > > and TF600T, P1801-T and TF701T pad. This is a glue driver handling
> > > detection and common operations for EC's functions.
> > >
> > > Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > ---
> > > drivers/mfd/Kconfig | 16 +
> > > drivers/mfd/Makefile | 1 +
> > > drivers/mfd/asus-transformer-ec.c | 542 ++++++++++++++++++++++++
> > > include/linux/mfd/asus-transformer-ec.h | 92 ++++
> > > 4 files changed, 651 insertions(+)
> > > create mode 100644 drivers/mfd/asus-transformer-ec.c
> > > create mode 100644 include/linux/mfd/asus-transformer-ec.h
[...]
> > > +static void asus_ec_clear_buffer(struct asus_ec_data *ddata)
> > > +{
> > > + int ret, retry = ASUSEC_RSP_BUFFER_SIZE;
> > > +
> > > + /*
> > > + * Read the buffer till we get valid data by checking ASUSEC_OBF_MASK
> > > + * of the status byte or till we reach end of the 256 byte buffer.
> > > + */
> > > + while (retry--) {
> > > + ret = i2c_smbus_read_i2c_block_data(ddata->client, ASUSEC_READ_BUF,
> > > + ASUSEC_ENTRY_SIZE,
> > > + ddata->ec_buf);
> > > + if (ret < ASUSEC_ENTRY_SIZE)
> > > + continue;
> > > +
> > > + if (ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK)
> > > + continue;
> > > +
> > > + break;
> > > + }
> > > +}
> > > +
> > > +static int asus_ec_log_info(struct asus_ec_data *ddata, unsigned int reg,
> > > + const char *name, const char **out)
If we can avoid points to pointers, then please do.
We already have ddata, so we can just set the name?
It will remove a lot of the following complexity / ugliness.
> > > +{
> > > + struct device *dev = &ddata->client->dev;
> > > + u8 buf[ASUSEC_ENTRY_BUFSIZE];
> > > + int ret;
> > > +
> > > + memset(buf, 0, ASUSEC_ENTRY_BUFSIZE);
> > > + ret = i2c_smbus_read_i2c_block_data(ddata->ec.dockram, reg,
> > > + ASUSEC_ENTRY_SIZE, buf);
> > > + if (ret < ASUSEC_ENTRY_SIZE)
> > > + return ret;
> >
> > Same here. These should be negative.
> >
>
> return ret < 0 ? ret : -EIO same as above
>
> > > +
> > > + if (buf[0] > ASUSEC_ENTRY_SIZE) {
> > > + dev_err(dev, "bad data len; buffer: %*ph; ret: %d\n",
> > > + ASUSEC_ENTRY_BUFSIZE, buf, ret);
> > > + return -EPROTO;
> > > + }
> > > +
> > > + if (!ddata->logging_disabled) {
> > > + dev_info(dev, "%-14s: %.*s\n", name, buf[0], buf + 1);
> > > +
> > > + if (out) {
> > > + *out = devm_kasprintf(dev, GFP_KERNEL, "%.*s",
> > > + buf[0], buf + 1);
> > > + if (!*out)
> > > + return -ENOMEM;
> > > + }
> > > + }
> >
> > FWIW, I hate this! What does it give you now that development is done?
> >
>
> We have already discussed this, and you agreed that EC and firmware
> prints may stay! This prints EC model and firmware info as well as EC
> firmware behavior. It allows identify possible new revisions of EC -
> Firmware combo and address possible regressions (check if it is chip
> malfunction or firmware needs a new programming model) without
> rebuilding kernel and digging downstream kernel for needed bits of
> code.
Right, so just print it out and remove all of the 'logging_disabled' and
'out' nonsense.
> > > + return 0;
> > > +}
> > > +
> > > +static int asus_ec_reset(struct asus_ec_data *ddata)
> > > +{
> > > + int retry, ret;
> > > +
> > > + guard(mutex)(&ddata->ecreq_lock);
> > > +
> > > + for (retry = 0; retry < ASUSEC_RETRY_MAX; retry++) {
> >
> > for (int retry = ... is generally preferred for throwaway variables.
> >
>
> Not that I care too much, but I am defining ret anyway, why not add
> retry too there?
This is the new and preferred way to use throw-away variables.
ret is not a throw-away variable.
[...]
> > > +static int asus_ec_set_factory_mode(struct asus_ec_data *ddata,
> > > + enum asusec_mode fmode)
> > > +{
> > > + dev_info(&ddata->client->dev, "Entering %s mode.\n",
> > > + fmode == ASUSEC_MODE_FACTORY ? "factory" : "normal");
> > > +
> > > + return asus_dockram_access_ctl(ddata->ec.dockram, NULL,
> > > + ASUSEC_CTL_FACTORY_MODE,
> > > + fmode == ASUSEC_MODE_FACTORY ?
> > > + ASUSEC_CTL_FACTORY_MODE : 0);
> >
> > Why not create make:
> >
> > ASUSEC_MODE_FACTORY == ASUSEC_CTL_FACTORY_MODE
> >
> > What happens to NORMAL?
> >
>
> ASUSEC_CTL_FACTORY_MODE is a bit in the ctl register. For NORMAL mode
I get that, but if the values can be shared, it make the code simpler.
> bit is cleared,
> for FACTORY bit it set, for NONE bit is ignored.
>
> > > +}
> > > +
> > > +static int asus_ec_detect(struct asus_ec_data *ddata)
> > > +{
> > > + int ret;
> > > +
> > > + ret = asus_ec_reset(ddata);
> > > + if (ret)
> > > + goto err_exit;
> > > +
> > > + asus_ec_clear_buffer(ddata);
> > > +
> > > + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_MODEL, "Model",
> > > + &ddata->ec.model);
Where is this model used?
> > You can use 100-chars and make the code look beautiful! :)
> >
>
> Not every subsystem permits 100 chars, some stick to 80 as a strict
> rule, so it is better be safe.
Right, but we are forward thinking here.
You can and should use 100-chars in this subsystem.
> > > + if (ret)
> > > + goto err_exit;
> > > +
> > > + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_FW, "FW version",
> > > + NULL);
> > > + if (ret)
> > > + goto err_exit;
> > > +
> > > + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_CFGFMT, "Config format",
> > > + NULL);
> > > + if (ret)
> > > + goto err_exit;
> > > +
> > > + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_HW, "HW version",
> > > + NULL);
> > > + if (ret)
> > > + goto err_exit;
> > > +
> > > + /* Disable logging on next EC request */
> >
> > Why, but why?
> >
>
> Cause EC requests are frequent (handshake/reset) and constant logging
> same data is not acceptable.
Then rid the prints entirely or do them at a more appropriate point like
during probe?
Or maybe consider dev_info_once() and friends.
> > > + ddata->logging_disabled = true;
> > > +
> > > + /* Check and inform about EC firmware behavior */
> > > + ret = asus_ec_susb_on_status(ddata);
> > > + if (ret)
> > > + goto err_exit;
> > > +
> > > + ddata->ec.name = ddata->info->name;
> > > +
> > > + /* Some EC require factory mode to be set normal on each request */
> > > + if (ddata->info->fmode)
> > > + ret = asus_ec_set_factory_mode(ddata, ddata->info->fmode);
> > > +
> > > +err_exit:
> > > + if (ret)
> > > + dev_err(&ddata->client->dev, "failed to access EC: %d\n", ret);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static void asus_ec_handle_smi(struct asus_ec_data *ddata, unsigned int code)
> > > +{
> > > + switch (code) {
> > > + case ASUSEC_SMI_HANDSHAKE:
> > > + case ASUSEC_SMI_RESET:
> > > + asus_ec_detect(ddata);
> > > + break;
> > > + }
> > > +}
> > > +
> > > +static irqreturn_t asus_ec_interrupt(int irq, void *dev_id)
> > > +{
> > > + struct asus_ec_data *ddata = dev_id;
> > > + unsigned long notify_action;
> > > + int ret;
> > > +
> > > + ret = i2c_smbus_read_i2c_block_data(ddata->client, ASUSEC_READ_BUF,
> > > + ASUSEC_ENTRY_SIZE, ddata->ec_buf);
> > > + if (ret < ASUSEC_ENTRY_SIZE ||
> > > + !(ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK))
> >
> > Unwrap for readability.
> >
> > Also, I think a comment would be helpful.
> >
>
> if (ret < ASUSEC_ENTRY_SIZE)
> return IRQ_NONE;
>
> ret = ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK;
> if (!ret)
> return IRQ_NONE;
>
> This would be acceptable? (I will add comments later on)
Yes, better.
If you're not using ret again, you could just put 'ddata.." inside the if().
> > > + return IRQ_NONE;
> > > +
> > > + notify_action = ddata->ec_buf[ASUSEC_IRQ_STATUS];
> > > + if (notify_action & ASUSEC_SMI_MASK) {
> > > + unsigned int code = ddata->ec_buf[ASUSEC_SMI_CODE];
> > > +
> > > + asus_ec_handle_smi(ddata, code);
> > > +
> > > + notify_action |= code << 8;
> > > + }
> > > +
> > > + blocking_notifier_call_chain(&ddata->ec.notify_list,
> > > + notify_action, ddata->ec_buf);
> > > +
> > > + return IRQ_HANDLED;
> > > +}
> > > +
> > > +static void asus_ec_release_dockram_dev(void *client)
> > > +{
> > > + i2c_unregister_device(client);
> > > +}
> > > +
> > > +static struct i2c_client *devm_asus_dockram_get(struct device *dev)
> > > +{
> > > + struct i2c_client *parent = to_i2c_client(dev);
> > > + struct i2c_client *dockram;
> > > + struct dockram_ec_data *ddata;
> > > + int ret;
> > > +
> > > + dockram = i2c_new_ancillary_device(parent, "dockram",
> > > + parent->addr + 2);
> >
> > Could we define a macro for the address offset '2' here to avoid using a magic
> > number?
> >
>
> It seems that you are excessively concerned with "magic numbers".
Bingo! I HATE magic numbers.
https://lore.kernel.org/all/?q=%22Lee+Jones%22+magic
~900 messages! =:-D
[...]
> > > +static const struct asus_ec_chip_info asus_ec_tf600t_pad_data = {
> > > + .name = "pad",
> > > + .variant = ASUSEC_TF600T_PAD,
> > > + .fmode = ASUSEC_MODE_NORMAL,
> > > +};
> >
> > Any reason not to just pass the identifier (variant) and add the name
> > and fmode attribues to the switch() above?
>
> Why not set it here, I am not passing any mfd or any other API via of data.
I get that, and you're not breaking any of my golden rules.
However, I just think doing everything in one place, usually a which
based off of the 'variant' which you pass as a single value, is a nicer,
more consolidated way of doing things.
--
Lee Jones
^ permalink raw reply
* Re: [PATCH] HID: lg-g15: cancel pending work on remove to fix a use-after-free
From: Hans de Goede @ 2026-06-18 10:28 UTC (permalink / raw)
To: Maoyi Xie, Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel
In-Reply-To: <178176639579.3377656.12792307991044339915@maoyixie.com>
Hi,
On 18-Jun-26 09:06, Maoyi Xie wrote:
> lg_g15_data is allocated with devm and holds a work item. The report
> handlers schedule that work straight from device input.
> lg_g15_event() and lg_g15_v2_event() do it on the backlight cycle key,
> and lg_g510_leds_event() does it too. The worker dereferences the
> lg_g15_data back through container_of.
>
> The driver had no remove callback and never cancelled the work. So if a
> report scheduled the work and the keyboard was then unplugged, devres
> freed lg_g15_data while the work was still pending or running, and the
> worker touched freed memory. This is a use-after-free. It is reachable
> as a race on device unplug.
>
> Add a remove callback that cancels the work before devres frees the
> state. g15->work is only initialized for the models that schedule it
> (G15, G15 v2, G510). The G13 and Z-10 leave it zeroed, so guard the
> cancel on g15->work.func to avoid cancelling a work that was never set
> up. The g15 NULL test mirrors the one already in lg_g15_raw_event().
>
> Fixes: 97b741aba918 ("HID: lg-g15: Add keyboard and LCD backlight control")
> Cc: stable@vger.kernel.org
> Suggested-by: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Maoyi Xie <maoyixie.tju@gmail.com>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Regards,
Hans
> ---
> drivers/hid/hid-lg-g15.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/hid/hid-lg-g15.c b/drivers/hid/hid-lg-g15.c
> index 1a88bc44ada4..02ef3e2094b4 100644
> --- a/drivers/hid/hid-lg-g15.c
> +++ b/drivers/hid/hid-lg-g15.c
> @@ -1374,11 +1374,27 @@ static const struct hid_device_id lg_g15_devices[] = {
> };
> MODULE_DEVICE_TABLE(hid, lg_g15_devices);
>
> +static void lg_g15_remove(struct hid_device *hdev)
> +{
> + struct lg_g15_data *g15 = hid_get_drvdata(hdev);
> +
> + /*
> + * g15->work is only initialized for the models that schedule it
> + * (G15, G15 v2, G510). The G13 and Z-10 leave it zeroed, so only
> + * cancel it when it was set up.
> + */
> + if (g15 && g15->work.func)
> + cancel_work_sync(&g15->work);
> +
> + hid_hw_stop(hdev);
> +}
> +
> static struct hid_driver lg_g15_driver = {
> .name = "lg-g15",
> .id_table = lg_g15_devices,
> .raw_event = lg_g15_raw_event,
> .probe = lg_g15_probe,
> + .remove = lg_g15_remove,
> };
> module_hid_driver(lg_g15_driver);
>
^ 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