* [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
* Re: [PATCH v8 4/7] input: keyboard: Add driver for ASUS Transformer dock multimedia keys
From: Svyatoslav Ryhel @ 2026-06-18 9:45 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Lee Jones,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <CAPVz0n0r-1SXH_dfS9HkQJrF7e-6+O5Me2bPjcscnizmfTfjZg@mail.gmail.com>
чт, 18 черв. 2026 р. о 12:18 Svyatoslav Ryhel <clamor95@gmail.com> пише:
>
> ср, 17 черв. 2026 р. о 00:23 Dmitry Torokhov <dmitry.torokhov@gmail.com> пише:
> >
> > On Tue, Jun 16, 2026 at 09:25:25AM +0300, Svyatoslav Ryhel wrote:
> > > вт, 16 черв. 2026 р. о 07:26 Dmitry Torokhov <dmitry.torokhov@gmail.com> пише:
> > > >
> > > > Hi Svyatoslav,
> > > >
> > > > On Thu, May 28, 2026 at 08:32:00AM +0300, Svyatoslav Ryhel wrote:
> > > > > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > > >
> > > > > Add support for multimedia top button row of ASUS Transformer's Mobile
> > > > > Dock keyboard. Driver is made that function keys (F1-F12) are used by
> > > > > default which suits average Linux use better and with pressing
> > > > > ScreenLock + AltGr function keys layout is switched to multimedia keys.
> > > > > Since this only modifies codes sent by asus-ec-keys it doesn't affect
> > > > > normal keyboards at all.
> > > >
> > > > I think using input handler to intercept ScreenLock + AltGr is quite
> > > > awkward. I think this also passes the original key events (unless you
> > > > make it a filter not a regular handler).
> > > >
> > > > I do not see benefit for reacting to AltGr+ScreenLock on other keyboards
> > > > to activate the special mode on this one. So given the fact that you
> > > > already mange the data stream when you split it into "serio" ports,
> > > > maybe just intercept this key combo right there and create the input
> > > > device and signal input events right there?
> > > >
> > >
> > > Though it seems awkward at a first glance, media keys are integrated
> > > with a standard keyboard in a detachable dock. It is highly unlikely
> > > that media keys will be used with a different keyboard then the one
> > > that is integrated with dock. Additionally, the ScreenLock key has a
> > > code specific to this driver and is not in general use, so even if any
> > > standard keyboard has AltGr but none has ScreenLock specific to this
> > > driver except the dock itself. Handler is also set as observer so it
> > > should not interfere with work of other input devices.
> >
> > I am not concerned about it interfering with other drivers, I am
> > concerned about it unnecessarily connecting to unrelated devices
> > (anything that declares EV_KEY).
> >
I can add check in asus_ec_input_connect() to strictly connect only to
dock keyboard, will this be sufficient?
> > Again, I think having input handler is not appropriate here. I would
> > fold this patch into the patch that introduces the 2 serio ports,
> > enhanced the data stream analysis to detect your key combo, and then
> > report through this new input device. You do not need to have the round
> > trip through atkbd and the new input handler for this.
This sounds like a load of additional work, I would rather remove
layout swapping entirely (even though it is a useful feature) then
rewrite everything from scratch.
> >
>
> I will try with filtering first if you don't mind. If that will not
> work I'll consider folding. Do you have any good examples of handler
> with filter to use as an inspiration? Thank you.
>
> > Thanks.
> >
> > --
> > Dmitry
^ permalink raw reply
* Re: [PATCH v8 4/7] input: keyboard: Add driver for ASUS Transformer dock multimedia keys
From: Svyatoslav Ryhel @ 2026-06-18 9:18 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Lee Jones,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <ajGyejCSRMhY4G2R@google.com>
ср, 17 черв. 2026 р. о 00:23 Dmitry Torokhov <dmitry.torokhov@gmail.com> пише:
>
> On Tue, Jun 16, 2026 at 09:25:25AM +0300, Svyatoslav Ryhel wrote:
> > вт, 16 черв. 2026 р. о 07:26 Dmitry Torokhov <dmitry.torokhov@gmail.com> пише:
> > >
> > > Hi Svyatoslav,
> > >
> > > On Thu, May 28, 2026 at 08:32:00AM +0300, Svyatoslav Ryhel wrote:
> > > > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > >
> > > > Add support for multimedia top button row of ASUS Transformer's Mobile
> > > > Dock keyboard. Driver is made that function keys (F1-F12) are used by
> > > > default which suits average Linux use better and with pressing
> > > > ScreenLock + AltGr function keys layout is switched to multimedia keys.
> > > > Since this only modifies codes sent by asus-ec-keys it doesn't affect
> > > > normal keyboards at all.
> > >
> > > I think using input handler to intercept ScreenLock + AltGr is quite
> > > awkward. I think this also passes the original key events (unless you
> > > make it a filter not a regular handler).
> > >
> > > I do not see benefit for reacting to AltGr+ScreenLock on other keyboards
> > > to activate the special mode on this one. So given the fact that you
> > > already mange the data stream when you split it into "serio" ports,
> > > maybe just intercept this key combo right there and create the input
> > > device and signal input events right there?
> > >
> >
> > Though it seems awkward at a first glance, media keys are integrated
> > with a standard keyboard in a detachable dock. It is highly unlikely
> > that media keys will be used with a different keyboard then the one
> > that is integrated with dock. Additionally, the ScreenLock key has a
> > code specific to this driver and is not in general use, so even if any
> > standard keyboard has AltGr but none has ScreenLock specific to this
> > driver except the dock itself. Handler is also set as observer so it
> > should not interfere with work of other input devices.
>
> I am not concerned about it interfering with other drivers, I am
> concerned about it unnecessarily connecting to unrelated devices
> (anything that declares EV_KEY).
>
> Again, I think having input handler is not appropriate here. I would
> fold this patch into the patch that introduces the 2 serio ports,
> enhanced the data stream analysis to detect your key combo, and then
> report through this new input device. You do not need to have the round
> trip through atkbd and the new input handler for this.
>
I will try with filtering first if you don't mind. If that will not
work I'll consider folding. Do you have any good examples of handler
with filter to use as an inspiration? Thank you.
> Thanks.
>
> --
> Dmitry
^ permalink raw reply
* [BUG] Bug report in HID: input: Add HID_BATTERY_QUIRK_DYNAMIC for Elan touchscreens
From: Eric Lin @ 2026-06-18 9:06 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input
Hi, I found a bug booting Linux 7.1 on my MSI Prestiage 16 Flip AI+,
when I unplugged the embedded stylus (MSI Nano Pen) from this laptop,
I got kernel OOPS.
Here are the relevant logs I got from `dmesg`
```
[ 44.523130] BUG: unable to handle page fault for address: ffffffffffffffe4
[ 44.523146] #PF: supervisor read access in kernel mode
[ 44.523151] #PF: error_code(0x0000) - not-present page
[ 44.523156] PGD 767025067 P4D 767025067 PUD 767027067 PMD 0
[ 44.523164] Oops: Oops: 0000 [#1] SMP NOPTI
[ 44.523172] CPU: 3 UID: 0 PID: 567 Comm: irq/108-CUST000 Not
tainted 7.1.0-1-mainline #1 PREEMPT(full)
4b7fa715112ff45ec03c02ca0cc4f3e6646a47e2
[ 44.523180] Hardware name: Micro-Star International Co., Ltd.
Prestige 16 Flip AI+ C3MTG/MS-2622, BIOS E2622IMS.117 04/27/2026
[ 44.523184] RIP: 0010:hidinput_hid_event+0x3b1/0xad0
[ 44.523197] Code: 1f 40 00 66 66 2e 0f 1f 84 00 00 00 00 00 66 66
2e 0f 1f 84 00 00 00 00 00 48 8b 46 38 48 8d 70 c8 48 39 c2 0f 84 ef
fd ff ff <3b> 4e 1c 75 ea 48 85 f6 0f 84 e1 fd ff ff 41 8b 00 3d 44 00
85 00
[ 44.523202] RSP: 0018:ffffcb8ac13f7cc8 EFLAGS: 00010282
[ 44.523208] RAX: 0000000000000000 RBX: ffff8b7b862d6000 RCX: 0000000000000007
[ 44.523212] RDX: ffff8b7b862d7c20 RSI: ffffffffffffffc8 RDI: ffff8b7b862d6000
[ 44.523215] RBP: 0000000000000001 R08: ffff8b7b88267288 R09: 0000000000000000
[ 44.523217] R10: ffff8b7baa294000 R11: ffff8b7b88267200 R12: ffff8b7b88267288
[ 44.523219] R13: ffff8b7b862d6000 R14: ffffffffc0954100 R15: ffff8b7b88267200
[ 44.523223] FS: 0000000000000000(0000) GS:ffff8b8348813000(0000)
knlGS:0000000000000000
[ 44.523227] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 44.523231] CR2: ffffffffffffffe4 CR3: 0000000767022005 CR4: 0000000108f72ef0
[ 44.523235] PKRU: 55555554
[ 44.523237] Call Trace:
[ 44.523243] <TASK>
[ 44.523256] hid_process_event+0x118/0x130
[ 44.523267] hid_report_raw_event+0x367/0x530
[ 44.523276] __hid_input_report+0x178/0x240
[ 44.523284] ? pm_wakeup_dev_event+0x30/0x70
[ 44.523294] ? __pfx_irq_thread_fn+0x10/0x10
[ 44.523305] hid_safe_input_report+0x14/0x20
[ 44.523316] i2c_hid_irq+0xcf/0x170 [i2c_hid
bde5e468c566fa9b3a9febe814b6fe7c454afeff]
[ 44.523327] irq_thread_fn+0x25/0x60
[ 44.523336] irq_thread+0x1cb/0x330
[ 44.523343] ? __pfx_irq_thread_dtor+0x10/0x10
[ 44.523351] ? __pfx_irq_thread+0x10/0x10
[ 44.523358] kthread+0xe1/0x120
[ 44.523367] ? __pfx_kthread+0x10/0x10
[ 44.523373] ret_from_fork+0x2a7/0x330
[ 44.523381] ? __pfx_kthread+0x10/0x10
[ 44.523386] ret_from_fork_asm+0x1a/0x30
[ 44.523398] </TASK>
[ 44.523400] Modules linked in: snd_seq_dummy rfcomm snd_hrtimer
snd_seq snd_seq_device ccm algif_aead des_generic libdes md4
nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_limit
nft_ct nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 uhid algif_hash
algif_skcipher af_alg nf_tables bnep nfnetlink f2fs vfat
lz4hc_compress fat lz4_compress snd_ctl_led snd_soc_sof_sdw
snd_sof_probes snd_soc_intel_hda_dsp_common snd_soc_rt712_sdca
snd_soc_rt1320_sdw regmap_sdw_mbq snd_hda_codec_intelhdmi regmap_sdw
snd_hda_codec_hdmi snd_soc_dmic snd_hda_intel snd_sof_pci_intel_ptl
snd_sof_pci_intel_lnl snd_sof_pci_intel_mtl snd_sof_intel_hda_generic
soundwire_intel snd_sof_intel_hda_sdw_bpt snd_sof_intel_hda_common
snd_soc_hdac_hda snd_sof_intel_hda_mlink snd_sof_intel_hda
soundwire_cadence snd_sof_pci snd_sof_xtensa_dsp snd_sof snd_sof_utils
snd_hda_ext_core snd_hda_codec snd_hda_core snd_intel_dspcfg
intel_rapl_msr snd_intel_sdw_acpi intel_uncore_frequency
intel_uncore_frequency_common snd_soc_acpi_intel_match
intel_tcc_cooling
[ 44.523493] snd_soc_acpi_intel_sdca_quirks x86_pkg_temp_thermal
soundwire_generic_allocation intel_powerclamp snd_soc_sdw_utils
coretemp snd_soc_acpi snd_hwdep soundwire_bus uvcvideo iwlmld
kvm_intel spi_nor snd_soc_sdca iTCO_wdt videobuf2_vmalloc
intel_pmc_bxt mac80211 mtd snd_soc_core uvc
processor_thermal_device_pci mei_gsc_proxy snd_compress kvm ptp
videobuf2_memops videobuf2_v4l2 processor_thermal_device pps_core
ac97_bus processor_thermal_wt_hint videobuf2_common libarc4 irqbypass
snd_pcm_dmaengine intel_cstate platform_temperature_control snd_pcm
hid_sensor_gyro_3d hid_sensor_custom_intel_hinge hid_sensor_rotation
hid_sensor_accel_3d hid_sensor_prox hid_sensor_incl_3d hid_sensor_als
hid_sensor_magn_3d processor_thermal_soc_slider snd_timer i2c_i801
hid_sensor_trigger dptf_power intel_uncore int3403_thermal
intel_pmc_core btintel_pcie processor_thermal_rfim videodev iwlwifi
kfifo_buf snd i2c_smbus processor_thermal_rapl hid_sensor_iio_common
btintel soundcore spi_intel_pci pcspkr pmt_telemetry wmi_bmof mc
[ 44.523586] msi_wmi_platform msi_wmi i2c_mux crc8 industrialio
int3400_thermal intel_rapl_common spi_intel intel_hid pmt_discovery
soc_button_array bluetooth acpi_thermal_rel acpi_pad acpi_tad
sparse_keymap cfg80211 processor_thermal_wt_req pmt_class mei_me
processor_thermal_power_floor processor_thermal_mbox mei intel_vpu
intel_pmc_ssram_telemetry rfkill int340x_thermal_zone igen6_edac
joydev mousedev mac_hid dm_crypt encrypted_keys trusted asn1_encoder
tee hid_sensor_custom intel_ishtp_hid xe drm_gpuvm drm_gpusvm_helper
drm_buddy ucsi_acpi gpu_sched typec_ucsi drm_exec roles hid_multitouch
drm_suballoc_helper aesni_intel video typec drm_ttm_helper gf128mul
hid_sensor_hub aead ttm nvme i2c_algo_bit i2c_hid_acpi wmi i2c_hid
nvme_core drm_display_helper intel_lpss_pci intel_ish_ipc nvme_keyring
intel_lpss pinctrl_intel_platform nvme_auth thunderbolt intel_ishtp
cec intel_vsec idma64 serio_raw dm_mirror dm_region_hash dm_log dm_mod
i2c_dev crypto_user ec_sys pkcs8_key_parser
[ 44.523684] CR2: ffffffffffffffe4
[ 44.523689] ---[ end trace 0000000000000000 ]---
[ 44.523694] RIP: 0010:hidinput_hid_event+0x3b1/0xad0
[ 44.523699] Code: 1f 40 00 66 66 2e 0f 1f 84 00 00 00 00 00 66 66
2e 0f 1f 84 00 00 00 00 00 48 8b 46 38 48 8d 70 c8 48 39 c2 0f 84 ef
fd ff ff <3b> 4e 1c 75 ea 48 85 f6 0f 84 e1 fd ff ff 41 8b 00 3d 44 00
85 00
[ 44.523702] RSP: 0018:ffffcb8ac13f7cc8 EFLAGS: 00010282
[ 44.523705] RAX: 0000000000000000 RBX: ffff8b7b862d6000 RCX: 0000000000000007
[ 44.523708] RDX: ffff8b7b862d7c20 RSI: ffffffffffffffc8 RDI: ffff8b7b862d6000
[ 44.523710] RBP: 0000000000000001 R08: ffff8b7b88267288 R09: 0000000000000000
[ 44.523711] R10: ffff8b7baa294000 R11: ffff8b7b88267200 R12: ffff8b7b88267288
[ 44.523713] R13: ffff8b7b862d6000 R14: ffffffffc0954100 R15: ffff8b7b88267200
[ 44.523715] FS: 0000000000000000(0000) GS:ffff8b8348813000(0000)
knlGS:0000000000000000
[ 44.523718] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 44.523720] CR2: ffffffffffffffe4 CR3: 0000000767022005 CR4: 0000000108f72ef0
[ 44.523723] PKRU: 55555554
[ 44.523726] note: irq/108-CUST000[567] exited with irqs disabled
[ 44.523748] kernel tried to execute NX-protected page - exploit
attempt? (uid: 0)
[ 44.523751] BUG: unable to handle page fault for address: ffff8b7ba8969bc0
[ 44.523753] #PF: supervisor instruction fetch in kernel mode
[ 44.523756] #PF: error_code(0x0011) - permissions violation
[ 44.523758] PGD 768201067 P4D 768201067 PUD 87f7ff067 PMD 10a5a6063
PTE 8000000128969163
[ 44.523764] Oops: Oops: 0011 [#2] SMP NOPTI
[ 44.523769] CPU: 3 UID: 0 PID: 567 Comm: irq/108-CUST000 Tainted: G
D 7.1.0-1-mainline #1 PREEMPT(full)
4b7fa715112ff45ec03c02ca0cc4f3e6646a47e2
[ 44.523774] Tainted: [D]=DIE
[ 44.523776] Hardware name: Micro-Star International Co., Ltd.
Prestige 16 Flip AI+ C3MTG/MS-2622, BIOS E2622IMS.117 04/27/2026
[ 44.523778] RIP: 0010:0xffff8b7ba8969bc0
[ 44.523873] Code: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 <00> 00 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
03 00
[ 44.523876] RSP: 0018:ffffcb8ac13f7eb8 EFLAGS: 00010286
[ 44.523880] RAX: ffff8b7ba8969bc0 RBX: 96f2eb95fa502b00 RCX: 0000000000000000
[ 44.523883] RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffffcb8ac13f7ea0
[ 44.523885] RBP: ffff8b7ba8969bc0 R08: 0000000000000000 R09: 0000000000000000
[ 44.523888] R10: ffffcb8ac13f7de8 R11: ffffcb8ac13f7de0 R12: 0000000000000009
[ 44.523891] R13: ffff8b7ba8970000 R14: ffff8b7ba8969b01 R15: 0000000000000000
[ 44.523893] FS: 0000000000000000(0000) GS:ffff8b8348813000(0000)
knlGS:0000000000000000
[ 44.523896] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 44.523899] CR2: ffff8b7ba8969bc0 CR3: 0000000767022005 CR4: 0000000108f72ef0
[ 44.523902] PKRU: 55555554
[ 44.523905] Call Trace:
[ 44.523909] <TASK>
[ 44.523911] ? task_work_run+0x66/0xa0
[ 44.523920] ? do_exit+0x300/0xba0
[ 44.523930] ? make_task_dead+0x8d/0x90
[ 44.523936] ? rewind_stack_and_make_dead+0x16/0x20
[ 44.523946] </TASK>
[ 44.523948] Modules linked in: snd_seq_dummy rfcomm snd_hrtimer
snd_seq snd_seq_device ccm algif_aead des_generic libdes md4
nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_limit
nft_ct nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 uhid algif_hash
algif_skcipher af_alg nf_tables bnep nfnetlink f2fs vfat
lz4hc_compress fat lz4_compress snd_ctl_led snd_soc_sof_sdw
snd_sof_probes snd_soc_intel_hda_dsp_common snd_soc_rt712_sdca
snd_soc_rt1320_sdw regmap_sdw_mbq snd_hda_codec_intelhdmi regmap_sdw
snd_hda_codec_hdmi snd_soc_dmic snd_hda_intel snd_sof_pci_intel_ptl
snd_sof_pci_intel_lnl snd_sof_pci_intel_mtl snd_sof_intel_hda_generic
soundwire_intel snd_sof_intel_hda_sdw_bpt snd_sof_intel_hda_common
snd_soc_hdac_hda snd_sof_intel_hda_mlink snd_sof_intel_hda
soundwire_cadence snd_sof_pci snd_sof_xtensa_dsp snd_sof snd_sof_utils
snd_hda_ext_core snd_hda_codec snd_hda_core snd_intel_dspcfg
intel_rapl_msr snd_intel_sdw_acpi intel_uncore_frequency
intel_uncore_frequency_common snd_soc_acpi_intel_match
intel_tcc_cooling
[ 44.524031] snd_soc_acpi_intel_sdca_quirks x86_pkg_temp_thermal
soundwire_generic_allocation intel_powerclamp snd_soc_sdw_utils
coretemp snd_soc_acpi snd_hwdep soundwire_bus uvcvideo iwlmld
kvm_intel spi_nor snd_soc_sdca iTCO_wdt videobuf2_vmalloc
intel_pmc_bxt mac80211 mtd snd_soc_core uvc
processor_thermal_device_pci mei_gsc_proxy snd_compress kvm ptp
videobuf2_memops videobuf2_v4l2 processor_thermal_device pps_core
ac97_bus processor_thermal_wt_hint videobuf2_common libarc4 irqbypass
snd_pcm_dmaengine intel_cstate platform_temperature_control snd_pcm
hid_sensor_gyro_3d hid_sensor_custom_intel_hinge hid_sensor_rotation
hid_sensor_accel_3d hid_sensor_prox hid_sensor_incl_3d hid_sensor_als
hid_sensor_magn_3d processor_thermal_soc_slider snd_timer i2c_i801
hid_sensor_trigger dptf_power intel_uncore int3403_thermal
intel_pmc_core btintel_pcie processor_thermal_rfim videodev iwlwifi
kfifo_buf snd i2c_smbus processor_thermal_rapl hid_sensor_iio_common
btintel soundcore spi_intel_pci pcspkr pmt_telemetry wmi_bmof mc
[ 44.524118] msi_wmi_platform msi_wmi i2c_mux crc8 industrialio
int3400_thermal intel_rapl_common spi_intel intel_hid pmt_discovery
soc_button_array bluetooth acpi_thermal_rel acpi_pad acpi_tad
sparse_keymap cfg80211 processor_thermal_wt_req pmt_class mei_me
processor_thermal_power_floor processor_thermal_mbox mei intel_vpu
intel_pmc_ssram_telemetry rfkill int340x_thermal_zone igen6_edac
joydev mousedev mac_hid dm_crypt encrypted_keys trusted asn1_encoder
tee hid_sensor_custom intel_ishtp_hid xe drm_gpuvm drm_gpusvm_helper
drm_buddy ucsi_acpi gpu_sched typec_ucsi drm_exec roles hid_multitouch
drm_suballoc_helper aesni_intel video typec drm_ttm_helper gf128mul
hid_sensor_hub aead ttm nvme i2c_algo_bit i2c_hid_acpi wmi i2c_hid
nvme_core drm_display_helper intel_lpss_pci intel_ish_ipc nvme_keyring
intel_lpss pinctrl_intel_platform nvme_auth thunderbolt intel_ishtp
cec intel_vsec idma64 serio_raw dm_mirror dm_region_hash dm_log dm_mod
i2c_dev crypto_user ec_sys pkcs8_key_parser
[ 44.524222] CR2: ffff8b7ba8969bc0
[ 44.524225] ---[ end trace 0000000000000000 ]---
[ 44.524228] RIP: 0010:hidinput_hid_event+0x3b1/0xad0
[ 44.524233] Code: 1f 40 00 66 66 2e 0f 1f 84 00 00 00 00 00 66 66
2e 0f 1f 84 00 00 00 00 00 48 8b 46 38 48 8d 70 c8 48 39 c2 0f 84 ef
fd ff ff <3b> 4e 1c 75 ea 48 85 f6 0f 84 e1 fd ff ff 41 8b 00 3d 44 00
85 00
[ 44.524236] RSP: 0018:ffffcb8ac13f7cc8 EFLAGS: 00010282
[ 44.524240] RAX: 0000000000000000 RBX: ffff8b7b862d6000 RCX: 0000000000000007
[ 44.524243] RDX: ffff8b7b862d7c20 RSI: ffffffffffffffc8 RDI: ffff8b7b862d6000
[ 44.524246] RBP: 0000000000000001 R08: ffff8b7b88267288 R09: 0000000000000000
[ 44.524249] R10: ffff8b7baa294000 R11: ffff8b7b88267200 R12: ffff8b7b88267288
[ 44.524252] R13: ffff8b7b862d6000 R14: ffffffffc0954100 R15: ffff8b7b88267200
[ 44.524255] FS: 0000000000000000(0000) GS:ffff8b8348813000(0000)
knlGS:0000000000000000
[ 44.524258] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 44.524261] CR2: ffff8b7ba8969bc0 CR3: 0000000767022005 CR4: 0000000108f72ef0
[ 44.524264] PKRU: 55555554
[ 44.524267] note: irq/108-CUST000[567] exited with irqs disabled
[ 44.524294] Fixing recursive fault but reboot is needed!
```
^ permalink raw reply
* Re: [PATCH v4 4/4] Input: snvs_pwrkey - report press event in interrupt handler
From: sashiko-bot @ 2026-06-18 7:41 UTC (permalink / raw)
To: joy.zou; +Cc: Frank.Li, imx, dmitry.torokhov, linux-input
In-Reply-To: <20260618-b4-pwrkey-v4-4-4bfda105bdf3@oss.nxp.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The spinlock scope is too narrow, allowing the timer softirq and interrupt handler to interleave their event reporting, which can leave the key permanently stuck in the pressed state.
--
commit 115416424bcbaae3f95f1195f01f6e828f579e10
Author: Joy Zou <joy.zou@nxp.com>
Input: snvs_pwrkey - report press event in interrupt handler
This patch modifies the driver to report key press events directly in the
interrupt handler during system suspend to prevent event loss. It also adds a
spinlock to protect the keystate and suspended tracking flag.
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
[ ... ]
> @@ -49,14 +51,21 @@ static void imx_imx_snvs_check_for_events(struct timer_list *t)
> struct pwrkey_drv_data *pdata = timer_container_of(pdata, t,
> check_timer);
> struct input_dev *input = pdata->input;
> + bool state_changed = false;
> u32 state;
>
> regmap_read(pdata->snvs, SNVS_HPSR_REG, &state);
> state = state & SNVS_HPSR_BTN ? 1 : 0;
>
> - /* only report new event if status changed */
> - if (state ^ pdata->keystate) {
> - pdata->keystate = state;
> + scoped_guard(spinlock_irqsave, &pdata->lock) {
> + /* only report new event if status changed */
> + if (state ^ pdata->keystate) {
> + pdata->keystate = state;
> + state_changed = true;
> + }
> + }
> +
> + if (state_changed) {
> input_event(input, EV_KEY, pdata->keycode, state);
[Severity: High]
Is the spinlock scope here too narrow?
Because the lock is dropped before calling input_event(), it seems the
interrupt handler could preempt this thread and reorder the event delivery
to the input core.
> input_sync(input);
> pm_relax(pdata->input->dev.parent);
[ ... ]
> @@ -92,6 +103,21 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
> input_sync(input);
> pm_relax(input->dev.parent);
> } else {
> + /*
> + * Report key press events directly in interrupt handler to prevent event
> + * loss during system suspend.
> + */
> + scoped_guard(spinlock_irqsave, &pdata->lock) {
> + suspended = pdata->suspended;
> + if (suspended) {
> + pdata->keystate = 1;
> + keystate = pdata->keystate;
> + }
> + }
> + if (suspended) {
> + input_report_key(input, pdata->keycode, keystate);
[Severity: High]
Could dropping the lock before calling input_report_key() lead to a race
with the timer softirq emitting its own input events?
For example, if the timer softirq reads state as 0 in
imx_imx_snvs_check_for_events(), updates pdata->keystate to 0, and drops
the lock, an interrupt could then preempt it:
CPU0 (timer softirq)
scoped_guard(...) {
pdata->keystate = 0;
}
CPU0 (hardirq preempts before input_event is called)
scoped_guard(...) {
pdata->keystate = 1;
}
input_report_key(..., 1); // PRESS emitted
CPU0 (timer softirq resumes)
input_event(..., 0); // RELEASE emitted
In this scenario, the input core would receive PRESS followed by RELEASE,
leaving the final input state as RELEASE (0). However, pdata->keystate
remains PRESS (1).
Would this cause the next physical key press to be ignored since the timer
will see state=1, but the XOR against keystate (already 1) evaluates to 0,
effectively losing the event permanently?
> + input_sync(input);
> + }
> mod_timer(&pdata->check_timer,
> jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-b4-pwrkey-v4-0-4bfda105bdf3@oss.nxp.com?part=4
^ permalink raw reply
* Re: [PATCH v4 3/4] Input: snvs_pwrkey - use local device pointer to simple code
From: sashiko-bot @ 2026-06-18 7:39 UTC (permalink / raw)
To: joy.zou; +Cc: linux-input, imx, dmitry.torokhov, Frank.Li
In-Reply-To: <20260618-b4-pwrkey-v4-3-4bfda105bdf3@oss.nxp.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Missing cleanup for device wakeup and wake IRQ on driver unbind causes memory leaks and re-bind warnings.
--
commit aed6a36738ed05979b6e5650a6efcf08cfb05a1d
Author: Joy Zou <joy.zou@nxp.com>
Input: snvs_pwrkey - use local device pointer to simple code
This patch introduces a local device pointer in imx_snvs_pwrkey_probe()
to simplify the code. It replaces repeated references to the platform device
pointer with the new local variable.
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
[ ... ]
> @@ -191,27 +192,27 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
[ ... ]
> - device_init_wakeup(&pdev->dev, pdata->wakeup);
> - error = dev_pm_set_wake_irq(&pdev->dev, pdata->irq);
> + device_init_wakeup(dev, pdata->wakeup);
> + error = dev_pm_set_wake_irq(dev, pdata->irq);
[Severity: High]
This is a pre-existing issue, but since there is no remove callback in this
driver, does this leak the wake IRQ allocation when the driver is unbound?
When the driver is unbound via sysfs, the wake_irq structure allocated by
dev_pm_set_wake_irq() seems to remain attached to dev->power.wakeirq.
If the driver is subsequently rebound, will dev_pm_set_wake_irq() fail with
-EEXIST and trigger a kernel warning about the wake irq already being
initialized?
Furthermore, because device_del() does not automatically free
dev->power.wakeirq, could this memory permanently leak when the platform
device is destroyed?
Would it be better to use devm_pm_set_wake_irq() here to ensure proper
cleanup?
> if (error)
> - dev_err(&pdev->dev, "irq wake enable failed.\n");
> + dev_err(dev, "irq wake enable failed.\n");
>
> return 0;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-b4-pwrkey-v4-0-4bfda105bdf3@oss.nxp.com?part=3
^ permalink raw reply
* Re: [PATCH v4 1/4] Input: snvs_pwrkey - make use of dev_err_probe()
From: sashiko-bot @ 2026-06-18 7:38 UTC (permalink / raw)
To: joy.zou; +Cc: linux-input, Frank.Li, imx, dmitry.torokhov
In-Reply-To: <20260618-b4-pwrkey-v4-1-4bfda105bdf3@oss.nxp.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [Medium] Memory leak of struct wake_irq on device unbind
--
commit 469e290fcf154d6638767e1e60cb2427638bea98
Author: Joy Zou <joy.zou@nxp.com>
Input: snvs_pwrkey - make use of dev_err_probe()
This commit updates the probe function of the snvs_pwrkey driver to
use dev_err_probe() for error handling. This simplifies the code and
improves logging for deferred probes.
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index 954055aaf6e29..8cc6863d26ed8 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
[ ... ]
> @@ -209,16 +201,12 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> error = devm_request_irq(&pdev->dev, pdata->irq,
> imx_snvs_pwrkey_interrupt,
> 0, pdev->name, pdev);
> - if (error) {
> - dev_err(&pdev->dev, "interrupt not available.\n");
> - return error;
> - }
> + if (error)
> + return dev_err_probe(&pdev->dev, error, "interrupt not available.\n");
>
> error = input_register_device(input);
> - if (error < 0) {
> - dev_err(&pdev->dev, "failed to register input device\n");
> - return error;
> - }
> + if (error < 0)
> + return dev_err_probe(&pdev->dev, error, "failed to register input device\n");
>
> device_init_wakeup(&pdev->dev, pdata->wakeup);
> error = dev_pm_set_wake_irq(&pdev->dev, pdata->irq);
[Severity: Medium]
This is a pre-existing issue, but does this dev_pm_set_wake_irq() allocation
leak when the driver is unbound?
Because this driver relies entirely on devres for resource management and
does not implement a .remove callback, there is no explicit call to
dev_pm_clear_wake_irq() upon driver unbind.
The device core does not automatically free dev->power.wakeirq during
device_pm_remove(), so this allocated wake_irq structure appears to leak
every time the driver is unbound.
Could this be fixed by switching to the managed variant,
devm_pm_set_wake_irq()?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-b4-pwrkey-v4-0-4bfda105bdf3@oss.nxp.com?part=1
^ permalink raw reply
* [PATCH v4 4/4] Input: snvs_pwrkey - report press event in interrupt handler
From: joy.zou @ 2026-06-18 7:31 UTC (permalink / raw)
To: Dmitry Torokhov, Frank Li, Bough Chen, Peng Fan, Jacky Bai, Ye Li
Cc: Joy Zou, imx, linux-input, linux-kernel, Joy Zou
In-Reply-To: <20260618-b4-pwrkey-v4-0-4bfda105bdf3@oss.nxp.com>
From: Joy Zou <joy.zou@nxp.com>
The driver implements debounce protection using a timer-based mechanism:
when a key interrupt occurs, a timer is scheduled to verify the key state
after DEBOUNCE_TIME before reporting the event. This works well during
normal operation.
However, key press events can be lost during system resume on platforms
like i.MX8MQ-EVK because:
1. During the no_irq resume phase, PCIe driver restoration can take up to
200ms with IRQs disabled.
2. The power key interrupt remains pending during the no_irq phase.
3. If the key is released before IRQs are re-enabled, the timer eventually
runs but sees the key as released and skips reporting the event.
Report key press events directly in interrupt handler to prevent event
loss during system suspend. This is safe because:
1. Only one event is reported per suspend cycle.
2. Normal operation retains the existing timer-based debounce mechanism.
Signed-off-by: Joy Zou <joy.zou@nxp.com>
---
Changes for v3:
1. Add spinlock for pdata->keystate and pdata->suspended per AI review
comments.
2. Replace hardcode value 1 with local variable keystate in input_report_key()
under suspended.
Changes for v2:
1. Add a boolean variable suspended and PM callback functions to replace
the use of the is_suspended field per AI review comments.
2. Move event report handle to else branch in suspended state, since the
pdata->minor_rev == 0 branch has no debounce detection per AI review
comments.
3. Modify the commit message.
---
drivers/input/keyboard/snvs_pwrkey.c | 60 ++++++++++++++++++++++++++++++++++--
1 file changed, 57 insertions(+), 3 deletions(-)
diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index cbe44a38d2b3..98c33d0bb1ec 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -39,6 +39,8 @@ struct pwrkey_drv_data {
int keycode;
int keystate; /* 1:pressed */
int wakeup;
+ bool suspended; /* Track suspend state */
+ spinlock_t lock; /* Protects keystate and suspended */
struct timer_list check_timer;
struct input_dev *input;
u8 minor_rev;
@@ -49,14 +51,21 @@ static void imx_imx_snvs_check_for_events(struct timer_list *t)
struct pwrkey_drv_data *pdata = timer_container_of(pdata, t,
check_timer);
struct input_dev *input = pdata->input;
+ bool state_changed = false;
u32 state;
regmap_read(pdata->snvs, SNVS_HPSR_REG, &state);
state = state & SNVS_HPSR_BTN ? 1 : 0;
- /* only report new event if status changed */
- if (state ^ pdata->keystate) {
- pdata->keystate = state;
+ scoped_guard(spinlock_irqsave, &pdata->lock) {
+ /* only report new event if status changed */
+ if (state ^ pdata->keystate) {
+ pdata->keystate = state;
+ state_changed = true;
+ }
+ }
+
+ if (state_changed) {
input_event(input, EV_KEY, pdata->keycode, state);
input_sync(input);
pm_relax(pdata->input->dev.parent);
@@ -74,6 +83,8 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
struct platform_device *pdev = dev_id;
struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
struct input_dev *input = pdata->input;
+ bool suspended;
+ int keystate;
u32 lp_status;
pm_wakeup_event(input->dev.parent, 0);
@@ -92,6 +103,21 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
input_sync(input);
pm_relax(input->dev.parent);
} else {
+ /*
+ * Report key press events directly in interrupt handler to prevent event
+ * loss during system suspend.
+ */
+ scoped_guard(spinlock_irqsave, &pdata->lock) {
+ suspended = pdata->suspended;
+ if (suspended) {
+ pdata->keystate = 1;
+ keystate = pdata->keystate;
+ }
+ }
+ if (suspended) {
+ input_report_key(input, pdata->keycode, keystate);
+ input_sync(input);
+ }
mod_timer(&pdata->check_timer,
jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
}
@@ -151,6 +177,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
if (pdata->irq < 0)
return pdata->irq;
+ spin_lock_init(&pdata->lock);
error = of_property_read_u32(np, "power-off-time-sec", &val);
if (!error) {
switch (val) {
@@ -217,6 +244,32 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
return 0;
}
+static int __maybe_unused imx_snvs_pwrkey_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
+
+ scoped_guard(spinlock_irqsave, &pdata->lock)
+ pdata->suspended = true;
+
+ return 0;
+}
+
+static int __maybe_unused imx_snvs_pwrkey_resume(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
+
+ scoped_guard(spinlock_irqsave, &pdata->lock)
+ pdata->suspended = false;
+
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(imx_snvs_pwrkey_pm_ops,
+ imx_snvs_pwrkey_suspend,
+ imx_snvs_pwrkey_resume);
+
static const struct of_device_id imx_snvs_pwrkey_ids[] = {
{ .compatible = "fsl,sec-v4.0-pwrkey" },
{ /* sentinel */ }
@@ -227,6 +280,7 @@ static struct platform_driver imx_snvs_pwrkey_driver = {
.driver = {
.name = "snvs_pwrkey",
.of_match_table = imx_snvs_pwrkey_ids,
+ .pm = &imx_snvs_pwrkey_pm_ops,
},
.probe = imx_snvs_pwrkey_probe,
};
--
2.34.1
^ permalink raw reply related
* [PATCH v4 3/4] Input: snvs_pwrkey - use local device pointer to simple code
From: joy.zou @ 2026-06-18 7:31 UTC (permalink / raw)
To: Dmitry Torokhov, Frank Li, Bough Chen, Peng Fan, Jacky Bai, Ye Li
Cc: Joy Zou, imx, linux-input, linux-kernel, Joy Zou
In-Reply-To: <20260618-b4-pwrkey-v4-0-4bfda105bdf3@oss.nxp.com>
From: Joy Zou <joy.zou@nxp.com>
Use local struct device pointer to avoid reference the platform_device
pointer every time.
No functional change.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Joy Zou <joy.zou@nxp.com>
---
Changes for v4:
1. Add Reviewed-by tag.
Changes for v2:
1. Use dev instead of &pdev->dev for devm_input_allocate_device(),
which was missed in patch v1 per AI review comments.
2. Modify commit message.
---
drivers/input/keyboard/snvs_pwrkey.c | 41 ++++++++++++++++++------------------
1 file changed, 21 insertions(+), 20 deletions(-)
diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index d58bbbe9fd58..cbe44a38d2b3 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -112,6 +112,7 @@ static void imx_snvs_pwrkey_act(void *pdata)
static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
struct pwrkey_drv_data *pdata;
struct input_dev *input;
struct device_node *np;
@@ -122,26 +123,26 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
u32 vid;
/* Get SNVS register Page */
- np = pdev->dev.of_node;
+ np = dev->of_node;
if (!np)
- return dev_err_probe(&pdev->dev, -ENODEV, "Device tree node not found\n");
+ return dev_err_probe(dev, -ENODEV, "Device tree node not found\n");
- pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+ pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata)
return -ENOMEM;
pdata->snvs = syscon_regmap_lookup_by_phandle(np, "regmap");
if (IS_ERR(pdata->snvs))
- return dev_err_probe(&pdev->dev, PTR_ERR(pdata->snvs), "Can't get snvs syscon\n");
+ return dev_err_probe(dev, PTR_ERR(pdata->snvs), "Can't get snvs syscon\n");
if (of_property_read_u32(np, "linux,keycode", &pdata->keycode)) {
pdata->keycode = KEY_POWER;
- dev_warn(&pdev->dev, "KEY_POWER without setting in dts\n");
+ dev_warn(dev, "KEY_POWER without setting in dts\n");
}
- clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
+ clk = devm_clk_get_optional_enabled(dev, NULL);
if (IS_ERR(clk))
- return dev_err_probe(&pdev->dev, PTR_ERR(clk),
+ return dev_err_probe(dev, PTR_ERR(clk),
"Failed to get snvs clock (%pe)\n", clk);
pdata->wakeup = of_property_read_bool(np, "wakeup-source");
@@ -162,7 +163,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
bpt = (val / 5) - 1;
break;
default:
- return dev_err_probe(&pdev->dev, -EINVAL,
+ return dev_err_probe(dev, -EINVAL,
"power-off-time-sec %d out of range\n", val);
}
@@ -180,9 +181,9 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
timer_setup(&pdata->check_timer, imx_imx_snvs_check_for_events, 0);
- input = devm_input_allocate_device(&pdev->dev);
+ input = devm_input_allocate_device(dev);
if (!input)
- return dev_err_probe(&pdev->dev, -ENOMEM, "failed to allocate the input device\n");
+ return dev_err_probe(dev, -ENOMEM, "failed to allocate the input device\n");
input->name = pdev->name;
input->phys = "snvs-pwrkey/input0";
@@ -191,27 +192,27 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
input_set_capability(input, EV_KEY, pdata->keycode);
/* input customer action to cancel release timer */
- error = devm_add_action(&pdev->dev, imx_snvs_pwrkey_act, pdata);
+ error = devm_add_action(dev, imx_snvs_pwrkey_act, pdata);
if (error)
- return dev_err_probe(&pdev->dev, error, "failed to register remove action\n");
+ return dev_err_probe(dev, error, "failed to register remove action\n");
pdata->input = input;
platform_set_drvdata(pdev, pdata);
- error = devm_request_irq(&pdev->dev, pdata->irq,
- imx_snvs_pwrkey_interrupt,
- 0, pdev->name, pdev);
+ error = devm_request_irq(dev, pdata->irq,
+ imx_snvs_pwrkey_interrupt,
+ 0, pdev->name, pdev);
if (error)
- return dev_err_probe(&pdev->dev, error, "interrupt not available.\n");
+ return dev_err_probe(dev, error, "interrupt not available.\n");
error = input_register_device(input);
if (error < 0)
- return dev_err_probe(&pdev->dev, error, "failed to register input device\n");
+ return dev_err_probe(dev, error, "failed to register input device\n");
- device_init_wakeup(&pdev->dev, pdata->wakeup);
- error = dev_pm_set_wake_irq(&pdev->dev, pdata->irq);
+ device_init_wakeup(dev, pdata->wakeup);
+ error = dev_pm_set_wake_irq(dev, pdata->irq);
if (error)
- dev_err(&pdev->dev, "irq wake enable failed.\n");
+ dev_err(dev, "irq wake enable failed.\n");
return 0;
}
--
2.34.1
^ permalink raw reply related
* [PATCH v4 2/4] Input: snvs_pwrkey - propagate error code of platform_get_irq()
From: joy.zou @ 2026-06-18 7:31 UTC (permalink / raw)
To: Dmitry Torokhov, Frank Li, Bough Chen, Peng Fan, Jacky Bai, Ye Li
Cc: Joy Zou, imx, linux-input, linux-kernel, Joy Zou
In-Reply-To: <20260618-b4-pwrkey-v4-0-4bfda105bdf3@oss.nxp.com>
From: Joy Zou <joy.zou@nxp.com>
Hardcoding -EINVAL discards the actual error code, which breaks probe
deferral (-EPROBE_DEFER) and loses critical diagnostic information
needed for proper kernel error handling.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Joy Zou <joy.zou@nxp.com>
---
Changes in v4:
1. modify the subject description.
2. add Reviewed-by tag.
---
drivers/input/keyboard/snvs_pwrkey.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index 8cc6863d26ed..d58bbbe9fd58 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -148,7 +148,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
pdata->irq = platform_get_irq(pdev, 0);
if (pdata->irq < 0)
- return -EINVAL;
+ return pdata->irq;
error = of_property_read_u32(np, "power-off-time-sec", &val);
if (!error) {
--
2.34.1
^ permalink raw reply related
* [PATCH v4 1/4] Input: snvs_pwrkey - make use of dev_err_probe()
From: joy.zou @ 2026-06-18 7:31 UTC (permalink / raw)
To: Dmitry Torokhov, Frank Li, Bough Chen, Peng Fan, Jacky Bai, Ye Li
Cc: Joy Zou, imx, linux-input, linux-kernel, Joy Zou
In-Reply-To: <20260618-b4-pwrkey-v4-0-4bfda105bdf3@oss.nxp.com>
From: Joy Zou <joy.zou@nxp.com>
Add dev_err_probe() at return path of probe() to support users to
identify issues easier.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Joy Zou <joy.zou@nxp.com>
---
Changes in v4:
1. Use dev_err_probe() for devm_input_allocate_device() to simplify
error handling.
Changes in v2:
1. Drop dev_err_probe() change for platform_get_irq() per AI review comments.
2. Add Reviewed-by tag.
---
drivers/input/keyboard/snvs_pwrkey.c | 44 +++++++++++++-----------------------
1 file changed, 16 insertions(+), 28 deletions(-)
diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index 954055aaf6e2..8cc6863d26ed 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -124,17 +124,15 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
/* Get SNVS register Page */
np = pdev->dev.of_node;
if (!np)
- return -ENODEV;
+ return dev_err_probe(&pdev->dev, -ENODEV, "Device tree node not found\n");
pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata)
return -ENOMEM;
pdata->snvs = syscon_regmap_lookup_by_phandle(np, "regmap");
- if (IS_ERR(pdata->snvs)) {
- dev_err(&pdev->dev, "Can't get snvs syscon\n");
- return PTR_ERR(pdata->snvs);
- }
+ if (IS_ERR(pdata->snvs))
+ return dev_err_probe(&pdev->dev, PTR_ERR(pdata->snvs), "Can't get snvs syscon\n");
if (of_property_read_u32(np, "linux,keycode", &pdata->keycode)) {
pdata->keycode = KEY_POWER;
@@ -142,10 +140,9 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
}
clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
- if (IS_ERR(clk)) {
- dev_err(&pdev->dev, "Failed to get snvs clock (%pe)\n", clk);
- return PTR_ERR(clk);
- }
+ if (IS_ERR(clk))
+ return dev_err_probe(&pdev->dev, PTR_ERR(clk),
+ "Failed to get snvs clock (%pe)\n", clk);
pdata->wakeup = of_property_read_bool(np, "wakeup-source");
@@ -165,9 +162,8 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
bpt = (val / 5) - 1;
break;
default:
- dev_err(&pdev->dev,
- "power-off-time-sec %d out of range\n", val);
- return -EINVAL;
+ return dev_err_probe(&pdev->dev, -EINVAL,
+ "power-off-time-sec %d out of range\n", val);
}
regmap_update_bits(pdata->snvs, SNVS_LPCR_REG, SNVS_LPCR_BPT_MASK,
@@ -185,10 +181,8 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
timer_setup(&pdata->check_timer, imx_imx_snvs_check_for_events, 0);
input = devm_input_allocate_device(&pdev->dev);
- if (!input) {
- dev_err(&pdev->dev, "failed to allocate the input device\n");
- return -ENOMEM;
- }
+ if (!input)
+ return dev_err_probe(&pdev->dev, -ENOMEM, "failed to allocate the input device\n");
input->name = pdev->name;
input->phys = "snvs-pwrkey/input0";
@@ -198,10 +192,8 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
/* input customer action to cancel release timer */
error = devm_add_action(&pdev->dev, imx_snvs_pwrkey_act, pdata);
- if (error) {
- dev_err(&pdev->dev, "failed to register remove action\n");
- return error;
- }
+ if (error)
+ return dev_err_probe(&pdev->dev, error, "failed to register remove action\n");
pdata->input = input;
platform_set_drvdata(pdev, pdata);
@@ -209,16 +201,12 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
error = devm_request_irq(&pdev->dev, pdata->irq,
imx_snvs_pwrkey_interrupt,
0, pdev->name, pdev);
- if (error) {
- dev_err(&pdev->dev, "interrupt not available.\n");
- return error;
- }
+ if (error)
+ return dev_err_probe(&pdev->dev, error, "interrupt not available.\n");
error = input_register_device(input);
- if (error < 0) {
- dev_err(&pdev->dev, "failed to register input device\n");
- return error;
- }
+ if (error < 0)
+ return dev_err_probe(&pdev->dev, error, "failed to register input device\n");
device_init_wakeup(&pdev->dev, pdata->wakeup);
error = dev_pm_set_wake_irq(&pdev->dev, pdata->irq);
--
2.34.1
^ permalink raw reply related
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