* [PATCH 0/1] leds: trigger: input-events: Rewrite to fix a serious locking issue
@ 2024-06-02 16:02 Hans de Goede
2024-06-02 16:02 ` [PATCH 1/1] " Hans de Goede
0 siblings, 1 reply; 5+ messages in thread
From: Hans de Goede @ 2024-06-02 16:02 UTC (permalink / raw)
To: Pavel Machek, Lee Jones; +Cc: Hans de Goede, linux-leds
Hi All,
As first discussed here:
https://lore.kernel.org/linux-leds/20240601195528.48308-1-hdegoede@redhat.com/
Shortly after ledtrig-input-events being merged into leds-for-next I found
a serious locking issue with it :|
The single patch in this series fixes this. As indicated in the subject
this is close to a full rewrite of ledtrig-input-events. The core
functionality is preserved and since ledtrig-input-events ATM only exists
on leds-for-next the other behavioral changes involved are not a problem.
Lee, I personally think it makes sense to keep the history and apply this
on top (1). But if you prefer you could also drop the current version and
then I'll send out a new patch directly adding the new version of
the input-events trigger.
Regards,
Hans
1) Especially in the light of the similar issues in the netdev trigger
Hans de Goede (1):
leds: trigger: input-events: Rewrite to fix a serious locking issue
drivers/leds/trigger/ledtrig-input-events.c | 136 +++++---------------
1 file changed, 34 insertions(+), 102 deletions(-)
--
2.45.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/1] leds: trigger: input-events: Rewrite to fix a serious locking issue
2024-06-02 16:02 [PATCH 0/1] leds: trigger: input-events: Rewrite to fix a serious locking issue Hans de Goede
@ 2024-06-02 16:02 ` Hans de Goede
2024-06-13 14:31 ` Lee Jones
2024-06-20 9:40 ` (subset) " Lee Jones
0 siblings, 2 replies; 5+ messages in thread
From: Hans de Goede @ 2024-06-02 16:02 UTC (permalink / raw)
To: Pavel Machek, Lee Jones; +Cc: Hans de Goede, linux-leds
The input subsystem registers LEDs with default triggers while holding
the input_lock and input_register_handler() takes the input_lock this
means that a triggers activate method cannot directly call
input_register_handler() as the old ledtrig-input-events code is doing.
The initial implementation of the input-events trigger mainly did not use
the simple LED trigger mechanism because that mechanism had an issue with
the initial state of a newly activated LED not matching the last
led_trigger_event() call for the trigger. This issue has been fixed in
commit 822c91e72eac ("leds: trigger: Store brightness set by
led_trigger_event()").
Rewrite the "input-events" trigger to use the simple LED trigger mechanism,
registering a single input_handler at module_init() time and using
led_trigger_event() to set the brightness for all LEDs controlled by this
trigger.
Compared to the old code this looses the ability for the user to configure
a different brightness for the on state then LED_FULL, this is standard for
simple LED triggers and since this trigger is only in for-leds-next ATM
losing that functionality is not a regression.
This also changes the configurability of the LED off timeout from a per
LED setting to a global setting (runtime modifiable module-parameter).
Switching to registering a single input_handler at module_init() time fixes
the following locking issue reported by lockdep:
[ 2840.220145] usb 1-1.3: new low-speed USB device number 3 using xhci_hcd
[ 2840.307172] usb 1-1.3: New USB device found, idVendor=0603, idProduct=0002, bcdDevice= 2.21
[ 2840.307375] usb 1-1.3: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[ 2840.307423] usb 1-1.3: Product: USB Composite Device
[ 2840.307456] usb 1-1.3: Manufacturer: SINO WEALTH
[ 2840.333985] input: SINO WEALTH USB Composite Device as /devices/pci0000:00/0000:00:14.0/usb1/1-1/1-1.3/1-1.3:1.0/0003:0603:0002.0007/input/input19
[ 2840.386545] ======================================================
[ 2840.386549] WARNING: possible circular locking dependency detected
[ 2840.386554] 6.10.0-rc1+ #97 Tainted: G C E
[ 2840.386558] ------------------------------------------------------
[ 2840.386562] kworker/1:1/52 is trying to acquire lock:
[ 2840.386566] ffff98fcf1629300 (&led_cdev->led_access){+.+.}-{3:3}, at: led_classdev_register_ext+0x1c6/0x380
[ 2840.386590]
but task is already holding lock:
[ 2840.386593] ffffffff88130cc8 (input_mutex){+.+.}-{3:3}, at: input_register_device.cold+0x47/0x150
[ 2840.386608]
which lock already depends on the new lock.
[ 2840.386611]
the existing dependency chain (in reverse order) is:
[ 2840.386615]
-> #3 (input_mutex){+.+.}-{3:3}:
[ 2840.386624] __mutex_lock+0x8c/0xc10
[ 2840.386634] input_register_handler+0x1c/0xf0
[ 2840.386641] 0xffffffffc142c437
[ 2840.386655] led_trigger_set+0x1e1/0x2e0
[ 2840.386661] led_trigger_register+0x170/0x1b0
[ 2840.386666] do_one_initcall+0x5e/0x3a0
[ 2840.386675] do_init_module+0x60/0x220
[ 2840.386683] __do_sys_init_module+0x15f/0x190
[ 2840.386689] do_syscall_64+0x93/0x180
[ 2840.386696] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ 2840.386705]
-> #2 (&led_cdev->trigger_lock){+.+.}-{3:3}:
[ 2840.386714] down_write+0x3b/0xd0
[ 2840.386720] led_trigger_register+0x12c/0x1b0
[ 2840.386725] rfkill_register+0xec/0x340 [rfkill]
[ 2840.386739] wiphy_register+0x82a/0x930 [cfg80211]
[ 2840.386907] brcmf_cfg80211_attach+0xcbd/0x1430 [brcmfmac]
[ 2840.386952] brcmf_attach+0x1ba/0x4c0 [brcmfmac]
[ 2840.386991] brcmf_pcie_setup+0x899/0xc70 [brcmfmac]
[ 2840.387030] brcmf_fw_request_done+0x13b/0x180 [brcmfmac]
[ 2840.387070] request_firmware_work_func+0x3b/0x70
[ 2840.387078] process_one_work+0x21a/0x590
[ 2840.387085] worker_thread+0x1d1/0x3e0
[ 2840.387090] kthread+0xee/0x120
[ 2840.387096] ret_from_fork+0x30/0x50
[ 2840.387105] ret_from_fork_asm+0x1a/0x30
[ 2840.387112]
-> #1 (leds_list_lock){++++}-{3:3}:
[ 2840.387123] down_write+0x3b/0xd0
[ 2840.387129] led_classdev_register_ext+0x29e/0x380
[ 2840.387134] 0xffffffffc0e6b74c
[ 2840.387143] platform_probe+0x40/0xa0
[ 2840.387151] really_probe+0xde/0x340
[ 2840.387157] __driver_probe_device+0x78/0x110
[ 2840.387162] driver_probe_device+0x1f/0xa0
[ 2840.387168] __driver_attach+0xba/0x1c0
[ 2840.387173] bus_for_each_dev+0x6b/0xb0
[ 2840.387180] bus_add_driver+0x111/0x1f0
[ 2840.387185] driver_register+0x6e/0xc0
[ 2840.387191] do_one_initcall+0x5e/0x3a0
[ 2840.387197] do_init_module+0x60/0x220
[ 2840.387204] __do_sys_init_module+0x15f/0x190
[ 2840.387210] do_syscall_64+0x93/0x180
[ 2840.387217] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ 2840.387224]
-> #0 (&led_cdev->led_access){+.+.}-{3:3}:
[ 2840.387233] __lock_acquire+0x11c6/0x1f20
[ 2840.387239] lock_acquire+0xc8/0x2b0
[ 2840.387244] __mutex_lock+0x8c/0xc10
[ 2840.387251] led_classdev_register_ext+0x1c6/0x380
[ 2840.387256] input_leds_connect+0x139/0x260
[ 2840.387262] input_attach_handler.isra.0+0x75/0x90
[ 2840.387268] input_register_device.cold+0xa1/0x150
[ 2840.387274] hidinput_connect+0x848/0xb00
[ 2840.387280] hid_connect+0x567/0x5a0
[ 2840.387288] hid_hw_start+0x3f/0x60
[ 2840.387294] hid_device_probe+0x10d/0x190
[ 2840.387298] really_probe+0xde/0x340
[ 2840.387304] __driver_probe_device+0x78/0x110
[ 2840.387309] driver_probe_device+0x1f/0xa0
[ 2840.387314] __device_attach_driver+0x85/0x110
[ 2840.387320] bus_for_each_drv+0x78/0xc0
[ 2840.387326] __device_attach+0xb0/0x1b0
[ 2840.387332] bus_probe_device+0x94/0xb0
[ 2840.387337] device_add+0x64a/0x860
[ 2840.387343] hid_add_device+0xe5/0x240
[ 2840.387349] usbhid_probe+0x4bb/0x600
[ 2840.387356] usb_probe_interface+0xea/0x2b0
[ 2840.387363] really_probe+0xde/0x340
[ 2840.387368] __driver_probe_device+0x78/0x110
[ 2840.387373] driver_probe_device+0x1f/0xa0
[ 2840.387378] __device_attach_driver+0x85/0x110
[ 2840.387383] bus_for_each_drv+0x78/0xc0
[ 2840.387390] __device_attach+0xb0/0x1b0
[ 2840.387395] bus_probe_device+0x94/0xb0
[ 2840.387400] device_add+0x64a/0x860
[ 2840.387405] usb_set_configuration+0x5e8/0x880
[ 2840.387411] usb_generic_driver_probe+0x3e/0x60
[ 2840.387418] usb_probe_device+0x3d/0x120
[ 2840.387423] really_probe+0xde/0x340
[ 2840.387428] __driver_probe_device+0x78/0x110
[ 2840.387434] driver_probe_device+0x1f/0xa0
[ 2840.387439] __device_attach_driver+0x85/0x110
[ 2840.387444] bus_for_each_drv+0x78/0xc0
[ 2840.387451] __device_attach+0xb0/0x1b0
[ 2840.387456] bus_probe_device+0x94/0xb0
[ 2840.387461] device_add+0x64a/0x860
[ 2840.387466] usb_new_device.cold+0x141/0x38f
[ 2840.387473] hub_event+0x1166/0x1980
[ 2840.387479] process_one_work+0x21a/0x590
[ 2840.387484] worker_thread+0x1d1/0x3e0
[ 2840.387488] kthread+0xee/0x120
[ 2840.387493] ret_from_fork+0x30/0x50
[ 2840.387500] ret_from_fork_asm+0x1a/0x30
[ 2840.387506]
other info that might help us debug this:
[ 2840.387509] Chain exists of:
&led_cdev->led_access --> &led_cdev->trigger_lock --> input_mutex
[ 2840.387520] Possible unsafe locking scenario:
[ 2840.387523] CPU0 CPU1
[ 2840.387526] ---- ----
[ 2840.387529] lock(input_mutex);
[ 2840.387534] lock(&led_cdev->trigger_lock);
[ 2840.387540] lock(input_mutex);
[ 2840.387545] lock(&led_cdev->led_access);
[ 2840.387550]
*** DEADLOCK ***
[ 2840.387552] 7 locks held by kworker/1:1/52:
[ 2840.387557] #0: ffff98fcc1d07148 ((wq_completion)usb_hub_wq){+.+.}-{0:0}, at: process_one_work+0x4af/0x590
[ 2840.387570] #1: ffffb67e00213e60 ((work_completion)(&hub->events)){+.+.}-{0:0}, at: process_one_work+0x1d5/0x590
[ 2840.387583] #2: ffff98fcc6582190 (&dev->mutex){....}-{3:3}, at: hub_event+0x57/0x1980
[ 2840.387596] #3: ffff98fccb3c6990 (&dev->mutex){....}-{3:3}, at: __device_attach+0x26/0x1b0
[ 2840.387610] #4: ffff98fcc5260960 (&dev->mutex){....}-{3:3}, at: __device_attach+0x26/0x1b0
[ 2840.387622] #5: ffff98fce3999a20 (&dev->mutex){....}-{3:3}, at: __device_attach+0x26/0x1b0
[ 2840.387635] #6: ffffffff88130cc8 (input_mutex){+.+.}-{3:3}, at: input_register_device.cold+0x47/0x150
[ 2840.387649]
stack backtrace:
[ 2840.387653] CPU: 1 PID: 52 Comm: kworker/1:1 Tainted: G C E 6.10.0-rc1+ #97
[ 2840.387659] Hardware name: Xiaomi Inc Mipad2/Mipad, BIOS MIPad-P4.X64.0043.R03.1603071414 03/07/2016
[ 2840.387665] Workqueue: usb_hub_wq hub_event
[ 2840.387674] Call Trace:
[ 2840.387681] <TASK>
[ 2840.387689] dump_stack_lvl+0x68/0x90
[ 2840.387700] check_noncircular+0x10d/0x120
[ 2840.387710] ? register_lock_class+0x38/0x480
[ 2840.387717] ? check_noncircular+0x74/0x120
[ 2840.387727] __lock_acquire+0x11c6/0x1f20
[ 2840.387736] lock_acquire+0xc8/0x2b0
[ 2840.387743] ? led_classdev_register_ext+0x1c6/0x380
[ 2840.387753] __mutex_lock+0x8c/0xc10
[ 2840.387760] ? led_classdev_register_ext+0x1c6/0x380
[ 2840.387766] ? _raw_spin_unlock_irqrestore+0x35/0x60
[ 2840.387773] ? klist_next+0x158/0x160
[ 2840.387781] ? led_classdev_register_ext+0x1c6/0x380
[ 2840.387787] ? lockdep_init_map_type+0x58/0x250
[ 2840.387796] ? led_classdev_register_ext+0x1c6/0x380
[ 2840.387802] led_classdev_register_ext+0x1c6/0x380
[ 2840.387810] ? kvasprintf+0x70/0xb0
[ 2840.387820] ? kasprintf+0x3e/0x50
[ 2840.387829] input_leds_connect+0x139/0x260
[ 2840.387838] input_attach_handler.isra.0+0x75/0x90
[ 2840.387846] input_register_device.cold+0xa1/0x150
[ 2840.387854] hidinput_connect+0x848/0xb00
[ 2840.387862] ? usbhid_start+0x45b/0x7b0
[ 2840.387870] hid_connect+0x567/0x5a0
[ 2840.387878] ? __mutex_unlock_slowpath+0x2d/0x260
[ 2840.387891] hid_hw_start+0x3f/0x60
[ 2840.387899] hid_device_probe+0x10d/0x190
[ 2840.387906] ? __pfx___device_attach_driver+0x10/0x10
[ 2840.387913] really_probe+0xde/0x340
[ 2840.387919] ? pm_runtime_barrier+0x50/0x90
[ 2840.387927] __driver_probe_device+0x78/0x110
[ 2840.387934] driver_probe_device+0x1f/0xa0
[ 2840.387941] __device_attach_driver+0x85/0x110
[ 2840.387949] bus_for_each_drv+0x78/0xc0
[ 2840.387959] __device_attach+0xb0/0x1b0
[ 2840.387967] bus_probe_device+0x94/0xb0
[ 2840.387974] device_add+0x64a/0x860
[ 2840.387982] ? __debugfs_create_file+0x14a/0x1c0
[ 2840.387993] hid_add_device+0xe5/0x240
[ 2840.388002] usbhid_probe+0x4bb/0x600
[ 2840.388013] usb_probe_interface+0xea/0x2b0
[ 2840.388021] ? __pfx___device_attach_driver+0x10/0x10
[ 2840.388028] really_probe+0xde/0x340
[ 2840.388034] ? pm_runtime_barrier+0x50/0x90
[ 2840.388040] __driver_probe_device+0x78/0x110
[ 2840.388048] driver_probe_device+0x1f/0xa0
[ 2840.388055] __device_attach_driver+0x85/0x110
[ 2840.388062] bus_for_each_drv+0x78/0xc0
[ 2840.388071] __device_attach+0xb0/0x1b0
[ 2840.388079] bus_probe_device+0x94/0xb0
[ 2840.388086] device_add+0x64a/0x860
[ 2840.388094] ? __mutex_unlock_slowpath+0x2d/0x260
[ 2840.388103] usb_set_configuration+0x5e8/0x880
[ 2840.388114] ? __pfx___device_attach_driver+0x10/0x10
[ 2840.388121] usb_generic_driver_probe+0x3e/0x60
[ 2840.388129] usb_probe_device+0x3d/0x120
[ 2840.388137] really_probe+0xde/0x340
[ 2840.388142] ? pm_runtime_barrier+0x50/0x90
[ 2840.388149] __driver_probe_device+0x78/0x110
[ 2840.388156] driver_probe_device+0x1f/0xa0
[ 2840.388163] __device_attach_driver+0x85/0x110
[ 2840.388171] bus_for_each_drv+0x78/0xc0
[ 2840.388180] __device_attach+0xb0/0x1b0
[ 2840.388188] bus_probe_device+0x94/0xb0
[ 2840.388195] device_add+0x64a/0x860
[ 2840.388202] ? lockdep_hardirqs_on+0x78/0x100
[ 2840.388210] ? _raw_spin_unlock_irqrestore+0x35/0x60
[ 2840.388219] usb_new_device.cold+0x141/0x38f
[ 2840.388227] hub_event+0x1166/0x1980
[ 2840.388242] process_one_work+0x21a/0x590
[ 2840.388249] ? move_linked_works+0x70/0xa0
[ 2840.388260] worker_thread+0x1d1/0x3e0
[ 2840.388268] ? __pfx_worker_thread+0x10/0x10
[ 2840.388273] kthread+0xee/0x120
[ 2840.388279] ? __pfx_kthread+0x10/0x10
[ 2840.388287] ret_from_fork+0x30/0x50
[ 2840.388294] ? __pfx_kthread+0x10/0x10
[ 2840.388301] ret_from_fork_asm+0x1a/0x30
[ 2840.388315] </TASK>
[ 2840.415630] hid-generic 0003:0603:0002.0007: input,hidraw6: USB HID v1.10 Keyboard [SINO WEALTH USB Composite Device] on usb-0000:00:14.0-1.3/input0
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/leds/trigger/ledtrig-input-events.c | 136 +++++---------------
1 file changed, 34 insertions(+), 102 deletions(-)
diff --git a/drivers/leds/trigger/ledtrig-input-events.c b/drivers/leds/trigger/ledtrig-input-events.c
index 1de0176799f9..1c79731562c2 100644
--- a/drivers/leds/trigger/ledtrig-input-events.c
+++ b/drivers/leds/trigger/ledtrig-input-events.c
@@ -3,31 +3,32 @@
* Input Events LED trigger
*
* Copyright (C) 2024 Hans de Goede <hansg@kernel.org>
- * Partially based on Atsushi Nemoto's ledtrig-heartbeat.c.
*/
#include <linux/input.h>
#include <linux/jiffies.h>
#include <linux/leds.h>
#include <linux/module.h>
+#include <linux/moduleparam.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
#include <linux/workqueue.h>
#include "../leds.h"
-#define DEFAULT_LED_OFF_DELAY_MS 5000
+static unsigned long led_off_delay_ms = 5000;
+module_param(led_off_delay_ms, ulong, 0644);
+MODULE_PARM_DESC(led_off_delay_ms,
+ "Specify delay in ms for turning LEDs off after last input event");
-struct input_events_data {
- struct input_handler handler;
+static struct input_events_data {
struct delayed_work work;
spinlock_t lock;
- struct led_classdev *led_cdev;
- int led_cdev_saved_flags;
/* To avoid repeatedly setting the brightness while there are events */
bool led_on;
unsigned long led_off_time;
- unsigned long led_off_delay;
-};
+} input_events_data;
+
+static struct led_trigger *input_events_led_trigger;
static void led_input_events_work(struct work_struct *work)
{
@@ -41,62 +42,24 @@ static void led_input_events_work(struct work_struct *work)
* running before a new event pushed led_off_time back.
*/
if (time_after_eq(jiffies, data->led_off_time)) {
- led_set_brightness_nosleep(data->led_cdev, LED_OFF);
+ led_trigger_event(input_events_led_trigger, LED_OFF);
data->led_on = false;
}
spin_unlock_irq(&data->lock);
}
-static ssize_t delay_show(struct device *dev, struct device_attribute *attr, char *buf)
-{
- struct input_events_data *input_events_data = led_trigger_get_drvdata(dev);
-
- return sysfs_emit(buf, "%lu\n", input_events_data->led_off_delay);
-}
-
-static ssize_t delay_store(struct device *dev, struct device_attribute *attr,
- const char *buf, size_t size)
-{
- struct input_events_data *input_events_data = led_trigger_get_drvdata(dev);
- unsigned long delay;
- int ret;
-
- ret = kstrtoul(buf, 0, &delay);
- if (ret)
- return ret;
-
- /* Clamp between 0.5 and 1000 seconds */
- delay = clamp_val(delay, 500UL, 1000000UL);
- input_events_data->led_off_delay = msecs_to_jiffies(delay);
-
- return size;
-}
-
-static DEVICE_ATTR_RW(delay);
-
-static struct attribute *input_events_led_attrs[] = {
- &dev_attr_delay.attr,
- NULL
-};
-ATTRIBUTE_GROUPS(input_events_led);
-
static void input_events_event(struct input_handle *handle, unsigned int type,
unsigned int code, int val)
{
- struct input_events_data *data =
- container_of(handle->handler, struct input_events_data, handler);
- unsigned long led_off_delay = READ_ONCE(data->led_off_delay);
- struct led_classdev *led_cdev = data->led_cdev;
+ struct input_events_data *data = &input_events_data;
+ unsigned long led_off_delay = msecs_to_jiffies(led_off_delay_ms);
unsigned long flags;
- if (test_and_clear_bit(LED_BLINK_BRIGHTNESS_CHANGE, &led_cdev->work_flags))
- led_cdev->blink_brightness = led_cdev->new_blink_brightness;
-
spin_lock_irqsave(&data->lock, flags);
if (!data->led_on) {
- led_set_brightness_nosleep(led_cdev, led_cdev->blink_brightness);
+ led_trigger_event(input_events_led_trigger, LED_FULL);
data->led_on = true;
}
data->led_off_time = jiffies + led_off_delay;
@@ -118,7 +81,7 @@ static int input_events_connect(struct input_handler *handler, struct input_dev
handle->dev = dev;
handle->handler = handler;
- handle->name = "input-events";
+ handle->name = KBUILD_MODNAME;
ret = input_register_handle(handle);
if (ret)
@@ -160,72 +123,41 @@ static const struct input_device_id input_events_ids[] = {
{ }
};
-static int input_events_activate(struct led_classdev *led_cdev)
+static struct input_handler input_events_handler = {
+ .name = KBUILD_MODNAME,
+ .event = input_events_event,
+ .connect = input_events_connect,
+ .disconnect = input_events_disconnect,
+ .id_table = input_events_ids,
+};
+
+static int __init input_events_init(void)
{
- struct input_events_data *data;
int ret;
- data = kzalloc(sizeof(*data), GFP_KERNEL);
- if (!data)
- return -ENOMEM;
+ INIT_DELAYED_WORK(&input_events_data.work, led_input_events_work);
+ spin_lock_init(&input_events_data.lock);
- data->handler.name = "input-events";
- data->handler.event = input_events_event;
- data->handler.connect = input_events_connect;
- data->handler.disconnect = input_events_disconnect;
- data->handler.id_table = input_events_ids;
+ led_trigger_register_simple("input-events", &input_events_led_trigger);
- INIT_DELAYED_WORK(&data->work, led_input_events_work);
- spin_lock_init(&data->lock);
-
- data->led_cdev = led_cdev;
- data->led_cdev_saved_flags = led_cdev->flags;
- data->led_off_delay = msecs_to_jiffies(DEFAULT_LED_OFF_DELAY_MS);
-
- /*
- * Use led_cdev->blink_brightness + LED_BLINK_SW flag so that sysfs
- * brightness writes will change led_cdev->new_blink_brightness for
- * configuring the on state brightness (like ledtrig-heartbeat).
- */
- if (!led_cdev->blink_brightness)
- led_cdev->blink_brightness = led_cdev->max_brightness;
-
- /* Start with LED off */
- led_set_brightness_nosleep(data->led_cdev, LED_OFF);
-
- ret = input_register_handler(&data->handler);
+ ret = input_register_handler(&input_events_handler);
if (ret) {
- kfree(data);
+ led_trigger_unregister_simple(input_events_led_trigger);
return ret;
}
- set_bit(LED_BLINK_SW, &led_cdev->work_flags);
-
- /* Turn LED off during suspend, original flags are restored on deactivate() */
- led_cdev->flags |= LED_CORE_SUSPENDRESUME;
-
- led_set_trigger_data(led_cdev, data);
return 0;
}
-static void input_events_deactivate(struct led_classdev *led_cdev)
+static void __exit input_events_exit(void)
{
- struct input_events_data *data = led_get_trigger_data(led_cdev);
-
- led_cdev->flags = data->led_cdev_saved_flags;
- clear_bit(LED_BLINK_SW, &led_cdev->work_flags);
- input_unregister_handler(&data->handler);
- cancel_delayed_work_sync(&data->work);
- kfree(data);
+ input_unregister_handler(&input_events_handler);
+ cancel_delayed_work_sync(&input_events_data.work);
+ led_trigger_unregister_simple(input_events_led_trigger);
}
-static struct led_trigger input_events_led_trigger = {
- .name = "input-events",
- .activate = input_events_activate,
- .deactivate = input_events_deactivate,
- .groups = input_events_led_groups,
-};
-module_led_trigger(input_events_led_trigger);
+module_init(input_events_init);
+module_exit(input_events_exit);
MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
MODULE_DESCRIPTION("Input Events LED trigger");
--
2.45.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] leds: trigger: input-events: Rewrite to fix a serious locking issue
2024-06-02 16:02 ` [PATCH 1/1] " Hans de Goede
@ 2024-06-13 14:31 ` Lee Jones
2024-06-14 16:04 ` Hans de Goede
2024-06-20 9:40 ` (subset) " Lee Jones
1 sibling, 1 reply; 5+ messages in thread
From: Lee Jones @ 2024-06-13 14:31 UTC (permalink / raw)
To: Hans de Goede; +Cc: Pavel Machek, linux-leds
On Sun, 02 Jun 2024, Hans de Goede wrote:
> The input subsystem registers LEDs with default triggers while holding
> the input_lock and input_register_handler() takes the input_lock this
> means that a triggers activate method cannot directly call
> input_register_handler() as the old ledtrig-input-events code is doing.
>
> The initial implementation of the input-events trigger mainly did not use
> the simple LED trigger mechanism because that mechanism had an issue with
> the initial state of a newly activated LED not matching the last
> led_trigger_event() call for the trigger. This issue has been fixed in
> commit 822c91e72eac ("leds: trigger: Store brightness set by
> led_trigger_event()").
>
> Rewrite the "input-events" trigger to use the simple LED trigger mechanism,
> registering a single input_handler at module_init() time and using
> led_trigger_event() to set the brightness for all LEDs controlled by this
> trigger.
>
> Compared to the old code this looses the ability for the user to configure
> a different brightness for the on state then LED_FULL, this is standard for
> simple LED triggers and since this trigger is only in for-leds-next ATM
> losing that functionality is not a regression.
>
> This also changes the configurability of the LED off timeout from a per
> LED setting to a global setting (runtime modifiable module-parameter).
>
> Switching to registering a single input_handler at module_init() time fixes
> the following locking issue reported by lockdep:
>
> [ 2840.220145] usb 1-1.3: new low-speed USB device number 3 using xhci_hcd
> [ 2840.307172] usb 1-1.3: New USB device found, idVendor=0603, idProduct=0002, bcdDevice= 2.21
> [ 2840.307375] usb 1-1.3: New USB device strings: Mfr=1, Product=2, SerialNumber=0
> [ 2840.307423] usb 1-1.3: Product: USB Composite Device
> [ 2840.307456] usb 1-1.3: Manufacturer: SINO WEALTH
> [ 2840.333985] input: SINO WEALTH USB Composite Device as /devices/pci0000:00/0000:00:14.0/usb1/1-1/1-1.3/1-1.3:1.0/0003:0603:0002.0007/input/input19
>
> [ 2840.386545] ======================================================
> [ 2840.386549] WARNING: possible circular locking dependency detected
> [ 2840.386554] 6.10.0-rc1+ #97 Tainted: G C E
> [ 2840.386558] ------------------------------------------------------
> [ 2840.386562] kworker/1:1/52 is trying to acquire lock:
> [ 2840.386566] ffff98fcf1629300 (&led_cdev->led_access){+.+.}-{3:3}, at: led_classdev_register_ext+0x1c6/0x380
> [ 2840.386590]
> but task is already holding lock:
> [ 2840.386593] ffffffff88130cc8 (input_mutex){+.+.}-{3:3}, at: input_register_device.cold+0x47/0x150
> [ 2840.386608]
> which lock already depends on the new lock.
>
> [ 2840.386611]
> the existing dependency chain (in reverse order) is:
> [ 2840.386615]
> -> #3 (input_mutex){+.+.}-{3:3}:
> [ 2840.386624] __mutex_lock+0x8c/0xc10
> [ 2840.386634] input_register_handler+0x1c/0xf0
> [ 2840.386641] 0xffffffffc142c437
> [ 2840.386655] led_trigger_set+0x1e1/0x2e0
> [ 2840.386661] led_trigger_register+0x170/0x1b0
> [ 2840.386666] do_one_initcall+0x5e/0x3a0
> [ 2840.386675] do_init_module+0x60/0x220
> [ 2840.386683] __do_sys_init_module+0x15f/0x190
> [ 2840.386689] do_syscall_64+0x93/0x180
> [ 2840.386696] entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [ 2840.386705]
> -> #2 (&led_cdev->trigger_lock){+.+.}-{3:3}:
> [ 2840.386714] down_write+0x3b/0xd0
> [ 2840.386720] led_trigger_register+0x12c/0x1b0
> [ 2840.386725] rfkill_register+0xec/0x340 [rfkill]
> [ 2840.386739] wiphy_register+0x82a/0x930 [cfg80211]
> [ 2840.386907] brcmf_cfg80211_attach+0xcbd/0x1430 [brcmfmac]
> [ 2840.386952] brcmf_attach+0x1ba/0x4c0 [brcmfmac]
> [ 2840.386991] brcmf_pcie_setup+0x899/0xc70 [brcmfmac]
> [ 2840.387030] brcmf_fw_request_done+0x13b/0x180 [brcmfmac]
> [ 2840.387070] request_firmware_work_func+0x3b/0x70
> [ 2840.387078] process_one_work+0x21a/0x590
> [ 2840.387085] worker_thread+0x1d1/0x3e0
> [ 2840.387090] kthread+0xee/0x120
> [ 2840.387096] ret_from_fork+0x30/0x50
> [ 2840.387105] ret_from_fork_asm+0x1a/0x30
> [ 2840.387112]
> -> #1 (leds_list_lock){++++}-{3:3}:
> [ 2840.387123] down_write+0x3b/0xd0
> [ 2840.387129] led_classdev_register_ext+0x29e/0x380
> [ 2840.387134] 0xffffffffc0e6b74c
> [ 2840.387143] platform_probe+0x40/0xa0
> [ 2840.387151] really_probe+0xde/0x340
> [ 2840.387157] __driver_probe_device+0x78/0x110
> [ 2840.387162] driver_probe_device+0x1f/0xa0
> [ 2840.387168] __driver_attach+0xba/0x1c0
> [ 2840.387173] bus_for_each_dev+0x6b/0xb0
> [ 2840.387180] bus_add_driver+0x111/0x1f0
> [ 2840.387185] driver_register+0x6e/0xc0
> [ 2840.387191] do_one_initcall+0x5e/0x3a0
> [ 2840.387197] do_init_module+0x60/0x220
> [ 2840.387204] __do_sys_init_module+0x15f/0x190
> [ 2840.387210] do_syscall_64+0x93/0x180
> [ 2840.387217] entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [ 2840.387224]
> -> #0 (&led_cdev->led_access){+.+.}-{3:3}:
> [ 2840.387233] __lock_acquire+0x11c6/0x1f20
> [ 2840.387239] lock_acquire+0xc8/0x2b0
> [ 2840.387244] __mutex_lock+0x8c/0xc10
> [ 2840.387251] led_classdev_register_ext+0x1c6/0x380
> [ 2840.387256] input_leds_connect+0x139/0x260
> [ 2840.387262] input_attach_handler.isra.0+0x75/0x90
> [ 2840.387268] input_register_device.cold+0xa1/0x150
> [ 2840.387274] hidinput_connect+0x848/0xb00
> [ 2840.387280] hid_connect+0x567/0x5a0
> [ 2840.387288] hid_hw_start+0x3f/0x60
> [ 2840.387294] hid_device_probe+0x10d/0x190
> [ 2840.387298] really_probe+0xde/0x340
> [ 2840.387304] __driver_probe_device+0x78/0x110
> [ 2840.387309] driver_probe_device+0x1f/0xa0
> [ 2840.387314] __device_attach_driver+0x85/0x110
> [ 2840.387320] bus_for_each_drv+0x78/0xc0
> [ 2840.387326] __device_attach+0xb0/0x1b0
> [ 2840.387332] bus_probe_device+0x94/0xb0
> [ 2840.387337] device_add+0x64a/0x860
> [ 2840.387343] hid_add_device+0xe5/0x240
> [ 2840.387349] usbhid_probe+0x4bb/0x600
> [ 2840.387356] usb_probe_interface+0xea/0x2b0
> [ 2840.387363] really_probe+0xde/0x340
> [ 2840.387368] __driver_probe_device+0x78/0x110
> [ 2840.387373] driver_probe_device+0x1f/0xa0
> [ 2840.387378] __device_attach_driver+0x85/0x110
> [ 2840.387383] bus_for_each_drv+0x78/0xc0
> [ 2840.387390] __device_attach+0xb0/0x1b0
> [ 2840.387395] bus_probe_device+0x94/0xb0
> [ 2840.387400] device_add+0x64a/0x860
> [ 2840.387405] usb_set_configuration+0x5e8/0x880
> [ 2840.387411] usb_generic_driver_probe+0x3e/0x60
> [ 2840.387418] usb_probe_device+0x3d/0x120
> [ 2840.387423] really_probe+0xde/0x340
> [ 2840.387428] __driver_probe_device+0x78/0x110
> [ 2840.387434] driver_probe_device+0x1f/0xa0
> [ 2840.387439] __device_attach_driver+0x85/0x110
> [ 2840.387444] bus_for_each_drv+0x78/0xc0
> [ 2840.387451] __device_attach+0xb0/0x1b0
> [ 2840.387456] bus_probe_device+0x94/0xb0
> [ 2840.387461] device_add+0x64a/0x860
> [ 2840.387466] usb_new_device.cold+0x141/0x38f
> [ 2840.387473] hub_event+0x1166/0x1980
> [ 2840.387479] process_one_work+0x21a/0x590
> [ 2840.387484] worker_thread+0x1d1/0x3e0
> [ 2840.387488] kthread+0xee/0x120
> [ 2840.387493] ret_from_fork+0x30/0x50
> [ 2840.387500] ret_from_fork_asm+0x1a/0x30
> [ 2840.387506]
> other info that might help us debug this:
>
> [ 2840.387509] Chain exists of:
> &led_cdev->led_access --> &led_cdev->trigger_lock --> input_mutex
>
> [ 2840.387520] Possible unsafe locking scenario:
>
> [ 2840.387523] CPU0 CPU1
> [ 2840.387526] ---- ----
> [ 2840.387529] lock(input_mutex);
> [ 2840.387534] lock(&led_cdev->trigger_lock);
> [ 2840.387540] lock(input_mutex);
> [ 2840.387545] lock(&led_cdev->led_access);
> [ 2840.387550]
> *** DEADLOCK ***
>
> [ 2840.387552] 7 locks held by kworker/1:1/52:
> [ 2840.387557] #0: ffff98fcc1d07148 ((wq_completion)usb_hub_wq){+.+.}-{0:0}, at: process_one_work+0x4af/0x590
> [ 2840.387570] #1: ffffb67e00213e60 ((work_completion)(&hub->events)){+.+.}-{0:0}, at: process_one_work+0x1d5/0x590
> [ 2840.387583] #2: ffff98fcc6582190 (&dev->mutex){....}-{3:3}, at: hub_event+0x57/0x1980
> [ 2840.387596] #3: ffff98fccb3c6990 (&dev->mutex){....}-{3:3}, at: __device_attach+0x26/0x1b0
> [ 2840.387610] #4: ffff98fcc5260960 (&dev->mutex){....}-{3:3}, at: __device_attach+0x26/0x1b0
> [ 2840.387622] #5: ffff98fce3999a20 (&dev->mutex){....}-{3:3}, at: __device_attach+0x26/0x1b0
> [ 2840.387635] #6: ffffffff88130cc8 (input_mutex){+.+.}-{3:3}, at: input_register_device.cold+0x47/0x150
> [ 2840.387649]
> stack backtrace:
> [ 2840.387653] CPU: 1 PID: 52 Comm: kworker/1:1 Tainted: G C E 6.10.0-rc1+ #97
> [ 2840.387659] Hardware name: Xiaomi Inc Mipad2/Mipad, BIOS MIPad-P4.X64.0043.R03.1603071414 03/07/2016
> [ 2840.387665] Workqueue: usb_hub_wq hub_event
> [ 2840.387674] Call Trace:
> [ 2840.387681] <TASK>
> [ 2840.387689] dump_stack_lvl+0x68/0x90
> [ 2840.387700] check_noncircular+0x10d/0x120
> [ 2840.387710] ? register_lock_class+0x38/0x480
> [ 2840.387717] ? check_noncircular+0x74/0x120
> [ 2840.387727] __lock_acquire+0x11c6/0x1f20
> [ 2840.387736] lock_acquire+0xc8/0x2b0
> [ 2840.387743] ? led_classdev_register_ext+0x1c6/0x380
> [ 2840.387753] __mutex_lock+0x8c/0xc10
> [ 2840.387760] ? led_classdev_register_ext+0x1c6/0x380
> [ 2840.387766] ? _raw_spin_unlock_irqrestore+0x35/0x60
> [ 2840.387773] ? klist_next+0x158/0x160
> [ 2840.387781] ? led_classdev_register_ext+0x1c6/0x380
> [ 2840.387787] ? lockdep_init_map_type+0x58/0x250
> [ 2840.387796] ? led_classdev_register_ext+0x1c6/0x380
> [ 2840.387802] led_classdev_register_ext+0x1c6/0x380
> [ 2840.387810] ? kvasprintf+0x70/0xb0
> [ 2840.387820] ? kasprintf+0x3e/0x50
> [ 2840.387829] input_leds_connect+0x139/0x260
> [ 2840.387838] input_attach_handler.isra.0+0x75/0x90
> [ 2840.387846] input_register_device.cold+0xa1/0x150
> [ 2840.387854] hidinput_connect+0x848/0xb00
> [ 2840.387862] ? usbhid_start+0x45b/0x7b0
> [ 2840.387870] hid_connect+0x567/0x5a0
> [ 2840.387878] ? __mutex_unlock_slowpath+0x2d/0x260
> [ 2840.387891] hid_hw_start+0x3f/0x60
> [ 2840.387899] hid_device_probe+0x10d/0x190
> [ 2840.387906] ? __pfx___device_attach_driver+0x10/0x10
> [ 2840.387913] really_probe+0xde/0x340
> [ 2840.387919] ? pm_runtime_barrier+0x50/0x90
> [ 2840.387927] __driver_probe_device+0x78/0x110
> [ 2840.387934] driver_probe_device+0x1f/0xa0
> [ 2840.387941] __device_attach_driver+0x85/0x110
> [ 2840.387949] bus_for_each_drv+0x78/0xc0
> [ 2840.387959] __device_attach+0xb0/0x1b0
> [ 2840.387967] bus_probe_device+0x94/0xb0
> [ 2840.387974] device_add+0x64a/0x860
> [ 2840.387982] ? __debugfs_create_file+0x14a/0x1c0
> [ 2840.387993] hid_add_device+0xe5/0x240
> [ 2840.388002] usbhid_probe+0x4bb/0x600
> [ 2840.388013] usb_probe_interface+0xea/0x2b0
> [ 2840.388021] ? __pfx___device_attach_driver+0x10/0x10
> [ 2840.388028] really_probe+0xde/0x340
> [ 2840.388034] ? pm_runtime_barrier+0x50/0x90
> [ 2840.388040] __driver_probe_device+0x78/0x110
> [ 2840.388048] driver_probe_device+0x1f/0xa0
> [ 2840.388055] __device_attach_driver+0x85/0x110
> [ 2840.388062] bus_for_each_drv+0x78/0xc0
> [ 2840.388071] __device_attach+0xb0/0x1b0
> [ 2840.388079] bus_probe_device+0x94/0xb0
> [ 2840.388086] device_add+0x64a/0x860
> [ 2840.388094] ? __mutex_unlock_slowpath+0x2d/0x260
> [ 2840.388103] usb_set_configuration+0x5e8/0x880
> [ 2840.388114] ? __pfx___device_attach_driver+0x10/0x10
> [ 2840.388121] usb_generic_driver_probe+0x3e/0x60
> [ 2840.388129] usb_probe_device+0x3d/0x120
> [ 2840.388137] really_probe+0xde/0x340
> [ 2840.388142] ? pm_runtime_barrier+0x50/0x90
> [ 2840.388149] __driver_probe_device+0x78/0x110
> [ 2840.388156] driver_probe_device+0x1f/0xa0
> [ 2840.388163] __device_attach_driver+0x85/0x110
> [ 2840.388171] bus_for_each_drv+0x78/0xc0
> [ 2840.388180] __device_attach+0xb0/0x1b0
> [ 2840.388188] bus_probe_device+0x94/0xb0
> [ 2840.388195] device_add+0x64a/0x860
> [ 2840.388202] ? lockdep_hardirqs_on+0x78/0x100
> [ 2840.388210] ? _raw_spin_unlock_irqrestore+0x35/0x60
> [ 2840.388219] usb_new_device.cold+0x141/0x38f
> [ 2840.388227] hub_event+0x1166/0x1980
> [ 2840.388242] process_one_work+0x21a/0x590
> [ 2840.388249] ? move_linked_works+0x70/0xa0
> [ 2840.388260] worker_thread+0x1d1/0x3e0
> [ 2840.388268] ? __pfx_worker_thread+0x10/0x10
> [ 2840.388273] kthread+0xee/0x120
> [ 2840.388279] ? __pfx_kthread+0x10/0x10
> [ 2840.388287] ret_from_fork+0x30/0x50
> [ 2840.388294] ? __pfx_kthread+0x10/0x10
> [ 2840.388301] ret_from_fork_asm+0x1a/0x30
> [ 2840.388315] </TASK>
> [ 2840.415630] hid-generic 0003:0603:0002.0007: input,hidraw6: USB HID v1.10 Keyboard [SINO WEALTH USB Composite Device] on usb-0000:00:14.0-1.3/input0
Do we really have to add the _whole_ trace?
Is there no way to snip it a little?
Or perhaps Link: to the whole trace?
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/leds/trigger/ledtrig-input-events.c | 136 +++++---------------
> 1 file changed, 34 insertions(+), 102 deletions(-)
This is nice!
> diff --git a/drivers/leds/trigger/ledtrig-input-events.c b/drivers/leds/trigger/ledtrig-input-events.c
> index 1de0176799f9..1c79731562c2 100644
> --- a/drivers/leds/trigger/ledtrig-input-events.c
> +++ b/drivers/leds/trigger/ledtrig-input-events.c
> @@ -3,31 +3,32 @@
> * Input Events LED trigger
> *
> * Copyright (C) 2024 Hans de Goede <hansg@kernel.org>
> - * Partially based on Atsushi Nemoto's ledtrig-heartbeat.c.
> */
>
> #include <linux/input.h>
> #include <linux/jiffies.h>
> #include <linux/leds.h>
> #include <linux/module.h>
> +#include <linux/moduleparam.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> #include <linux/workqueue.h>
> #include "../leds.h"
>
> -#define DEFAULT_LED_OFF_DELAY_MS 5000
> +static unsigned long led_off_delay_ms = 5000;
> +module_param(led_off_delay_ms, ulong, 0644);
> +MODULE_PARM_DESC(led_off_delay_ms,
> + "Specify delay in ms for turning LEDs off after last input event");
>
> -struct input_events_data {
> - struct input_handler handler;
> +static struct input_events_data {
> struct delayed_work work;
> spinlock_t lock;
> - struct led_classdev *led_cdev;
> - int led_cdev_saved_flags;
> /* To avoid repeatedly setting the brightness while there are events */
> bool led_on;
> unsigned long led_off_time;
> - unsigned long led_off_delay;
> -};
> +} input_events_data;
Does this fix really require statically declared structs?
> +static struct led_trigger *input_events_led_trigger;
>
> static void led_input_events_work(struct work_struct *work)
> {
> @@ -41,62 +42,24 @@ static void led_input_events_work(struct work_struct *work)
> * running before a new event pushed led_off_time back.
> */
> if (time_after_eq(jiffies, data->led_off_time)) {
> - led_set_brightness_nosleep(data->led_cdev, LED_OFF);
> + led_trigger_event(input_events_led_trigger, LED_OFF);
> data->led_on = false;
> }
>
> spin_unlock_irq(&data->lock);
> }
>
> -static ssize_t delay_show(struct device *dev, struct device_attribute *attr, char *buf)
> -{
> - struct input_events_data *input_events_data = led_trigger_get_drvdata(dev);
> -
> - return sysfs_emit(buf, "%lu\n", input_events_data->led_off_delay);
> -}
> -
> -static ssize_t delay_store(struct device *dev, struct device_attribute *attr,
> - const char *buf, size_t size)
> -{
> - struct input_events_data *input_events_data = led_trigger_get_drvdata(dev);
> - unsigned long delay;
> - int ret;
> -
> - ret = kstrtoul(buf, 0, &delay);
> - if (ret)
> - return ret;
> -
> - /* Clamp between 0.5 and 1000 seconds */
> - delay = clamp_val(delay, 500UL, 1000000UL);
> - input_events_data->led_off_delay = msecs_to_jiffies(delay);
> -
> - return size;
> -}
> -
> -static DEVICE_ATTR_RW(delay);
> -
> -static struct attribute *input_events_led_attrs[] = {
> - &dev_attr_delay.attr,
> - NULL
> -};
> -ATTRIBUTE_GROUPS(input_events_led);
> -
> static void input_events_event(struct input_handle *handle, unsigned int type,
> unsigned int code, int val)
> {
> - struct input_events_data *data =
> - container_of(handle->handler, struct input_events_data, handler);
> - unsigned long led_off_delay = READ_ONCE(data->led_off_delay);
> - struct led_classdev *led_cdev = data->led_cdev;
> + struct input_events_data *data = &input_events_data;
IFF we have to use statically declared structs ...
Why not just use input_events_data.foo?
> + unsigned long led_off_delay = msecs_to_jiffies(led_off_delay_ms);
> unsigned long flags;
>
> - if (test_and_clear_bit(LED_BLINK_BRIGHTNESS_CHANGE, &led_cdev->work_flags))
> - led_cdev->blink_brightness = led_cdev->new_blink_brightness;
> -
> spin_lock_irqsave(&data->lock, flags);
>
> if (!data->led_on) {
> - led_set_brightness_nosleep(led_cdev, led_cdev->blink_brightness);
> + led_trigger_event(input_events_led_trigger, LED_FULL);
> data->led_on = true;
> }
> data->led_off_time = jiffies + led_off_delay;
> @@ -118,7 +81,7 @@ static int input_events_connect(struct input_handler *handler, struct input_dev
>
> handle->dev = dev;
> handle->handler = handler;
> - handle->name = "input-events";
> + handle->name = KBUILD_MODNAME;
>
> ret = input_register_handle(handle);
> if (ret)
> @@ -160,72 +123,41 @@ static const struct input_device_id input_events_ids[] = {
> { }
> };
>
> -static int input_events_activate(struct led_classdev *led_cdev)
> +static struct input_handler input_events_handler = {
> + .name = KBUILD_MODNAME,
> + .event = input_events_event,
> + .connect = input_events_connect,
> + .disconnect = input_events_disconnect,
> + .id_table = input_events_ids,
> +};
> +
> +static int __init input_events_init(void)
> {
> - struct input_events_data *data;
> int ret;
>
> - data = kzalloc(sizeof(*data), GFP_KERNEL);
> - if (!data)
> - return -ENOMEM;
> + INIT_DELAYED_WORK(&input_events_data.work, led_input_events_work);
> + spin_lock_init(&input_events_data.lock);
>
> - data->handler.name = "input-events";
> - data->handler.event = input_events_event;
> - data->handler.connect = input_events_connect;
> - data->handler.disconnect = input_events_disconnect;
> - data->handler.id_table = input_events_ids;
> + led_trigger_register_simple("input-events", &input_events_led_trigger);
>
> - INIT_DELAYED_WORK(&data->work, led_input_events_work);
> - spin_lock_init(&data->lock);
> -
> - data->led_cdev = led_cdev;
> - data->led_cdev_saved_flags = led_cdev->flags;
> - data->led_off_delay = msecs_to_jiffies(DEFAULT_LED_OFF_DELAY_MS);
> -
> - /*
> - * Use led_cdev->blink_brightness + LED_BLINK_SW flag so that sysfs
> - * brightness writes will change led_cdev->new_blink_brightness for
> - * configuring the on state brightness (like ledtrig-heartbeat).
> - */
> - if (!led_cdev->blink_brightness)
> - led_cdev->blink_brightness = led_cdev->max_brightness;
> -
> - /* Start with LED off */
> - led_set_brightness_nosleep(data->led_cdev, LED_OFF);
> -
> - ret = input_register_handler(&data->handler);
> + ret = input_register_handler(&input_events_handler);
> if (ret) {
> - kfree(data);
> + led_trigger_unregister_simple(input_events_led_trigger);
> return ret;
> }
>
> - set_bit(LED_BLINK_SW, &led_cdev->work_flags);
> -
> - /* Turn LED off during suspend, original flags are restored on deactivate() */
> - led_cdev->flags |= LED_CORE_SUSPENDRESUME;
> -
> - led_set_trigger_data(led_cdev, data);
> return 0;
> }
>
> -static void input_events_deactivate(struct led_classdev *led_cdev)
> +static void __exit input_events_exit(void)
> {
> - struct input_events_data *data = led_get_trigger_data(led_cdev);
> -
> - led_cdev->flags = data->led_cdev_saved_flags;
> - clear_bit(LED_BLINK_SW, &led_cdev->work_flags);
> - input_unregister_handler(&data->handler);
> - cancel_delayed_work_sync(&data->work);
> - kfree(data);
> + input_unregister_handler(&input_events_handler);
> + cancel_delayed_work_sync(&input_events_data.work);
> + led_trigger_unregister_simple(input_events_led_trigger);
> }
>
> -static struct led_trigger input_events_led_trigger = {
> - .name = "input-events",
> - .activate = input_events_activate,
> - .deactivate = input_events_deactivate,
> - .groups = input_events_led_groups,
> -};
> -module_led_trigger(input_events_led_trigger);
> +module_init(input_events_init);
> +module_exit(input_events_exit);
>
> MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
> MODULE_DESCRIPTION("Input Events LED trigger");
> --
> 2.45.1
>
>
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] leds: trigger: input-events: Rewrite to fix a serious locking issue
2024-06-13 14:31 ` Lee Jones
@ 2024-06-14 16:04 ` Hans de Goede
0 siblings, 0 replies; 5+ messages in thread
From: Hans de Goede @ 2024-06-14 16:04 UTC (permalink / raw)
To: Lee Jones; +Cc: Pavel Machek, linux-leds
Hi Lee,
On 6/13/24 4:31 PM, Lee Jones wrote:
> On Sun, 02 Jun 2024, Hans de Goede wrote:
>
>> The input subsystem registers LEDs with default triggers while holding
>> the input_lock and input_register_handler() takes the input_lock this
>> means that a triggers activate method cannot directly call
>> input_register_handler() as the old ledtrig-input-events code is doing.
>>
>> The initial implementation of the input-events trigger mainly did not use
>> the simple LED trigger mechanism because that mechanism had an issue with
>> the initial state of a newly activated LED not matching the last
>> led_trigger_event() call for the trigger. This issue has been fixed in
>> commit 822c91e72eac ("leds: trigger: Store brightness set by
>> led_trigger_event()").
>>
>> Rewrite the "input-events" trigger to use the simple LED trigger mechanism,
>> registering a single input_handler at module_init() time and using
>> led_trigger_event() to set the brightness for all LEDs controlled by this
>> trigger.
>>
>> Compared to the old code this looses the ability for the user to configure
>> a different brightness for the on state then LED_FULL, this is standard for
>> simple LED triggers and since this trigger is only in for-leds-next ATM
>> losing that functionality is not a regression.
>>
>> This also changes the configurability of the LED off timeout from a per
>> LED setting to a global setting (runtime modifiable module-parameter).
>>
>> Switching to registering a single input_handler at module_init() time fixes
>> the following locking issue reported by lockdep:
>>
>> [ 2840.220145] usb 1-1.3: new low-speed USB device number 3 using xhci_hcd
>> [ 2840.307172] usb 1-1.3: New USB device found, idVendor=0603, idProduct=0002, bcdDevice= 2.21
>> [ 2840.307375] usb 1-1.3: New USB device strings: Mfr=1, Product=2, SerialNumber=0
>> [ 2840.307423] usb 1-1.3: Product: USB Composite Device
>> [ 2840.307456] usb 1-1.3: Manufacturer: SINO WEALTH
>> [ 2840.333985] input: SINO WEALTH USB Composite Device as /devices/pci0000:00/0000:00:14.0/usb1/1-1/1-1.3/1-1.3:1.0/0003:0603:0002.0007/input/input19
>>
>> [ 2840.386545] ======================================================
>> [ 2840.386549] WARNING: possible circular locking dependency detected
>> [ 2840.386554] 6.10.0-rc1+ #97 Tainted: G C E
>> [ 2840.386558] ------------------------------------------------------
>> [ 2840.386562] kworker/1:1/52 is trying to acquire lock:
>> [ 2840.386566] ffff98fcf1629300 (&led_cdev->led_access){+.+.}-{3:3}, at: led_classdev_register_ext+0x1c6/0x380
>> [ 2840.386590]
>> but task is already holding lock:
>> [ 2840.386593] ffffffff88130cc8 (input_mutex){+.+.}-{3:3}, at: input_register_device.cold+0x47/0x150
>> [ 2840.386608]
>> which lock already depends on the new lock.
>>
>> [ 2840.386611]
>> the existing dependency chain (in reverse order) is:
>> [ 2840.386615]
>> -> #3 (input_mutex){+.+.}-{3:3}:
>> [ 2840.386624] __mutex_lock+0x8c/0xc10
>> [ 2840.386634] input_register_handler+0x1c/0xf0
>> [ 2840.386641] 0xffffffffc142c437
>> [ 2840.386655] led_trigger_set+0x1e1/0x2e0
>> [ 2840.386661] led_trigger_register+0x170/0x1b0
>> [ 2840.386666] do_one_initcall+0x5e/0x3a0
>> [ 2840.386675] do_init_module+0x60/0x220
>> [ 2840.386683] __do_sys_init_module+0x15f/0x190
>> [ 2840.386689] do_syscall_64+0x93/0x180
>> [ 2840.386696] entry_SYSCALL_64_after_hwframe+0x76/0x7e
>> [ 2840.386705]
>> -> #2 (&led_cdev->trigger_lock){+.+.}-{3:3}:
>> [ 2840.386714] down_write+0x3b/0xd0
>> [ 2840.386720] led_trigger_register+0x12c/0x1b0
>> [ 2840.386725] rfkill_register+0xec/0x340 [rfkill]
>> [ 2840.386739] wiphy_register+0x82a/0x930 [cfg80211]
>> [ 2840.386907] brcmf_cfg80211_attach+0xcbd/0x1430 [brcmfmac]
>> [ 2840.386952] brcmf_attach+0x1ba/0x4c0 [brcmfmac]
>> [ 2840.386991] brcmf_pcie_setup+0x899/0xc70 [brcmfmac]
>> [ 2840.387030] brcmf_fw_request_done+0x13b/0x180 [brcmfmac]
>> [ 2840.387070] request_firmware_work_func+0x3b/0x70
>> [ 2840.387078] process_one_work+0x21a/0x590
>> [ 2840.387085] worker_thread+0x1d1/0x3e0
>> [ 2840.387090] kthread+0xee/0x120
>> [ 2840.387096] ret_from_fork+0x30/0x50
>> [ 2840.387105] ret_from_fork_asm+0x1a/0x30
>> [ 2840.387112]
>> -> #1 (leds_list_lock){++++}-{3:3}:
>> [ 2840.387123] down_write+0x3b/0xd0
>> [ 2840.387129] led_classdev_register_ext+0x29e/0x380
>> [ 2840.387134] 0xffffffffc0e6b74c
>> [ 2840.387143] platform_probe+0x40/0xa0
>> [ 2840.387151] really_probe+0xde/0x340
>> [ 2840.387157] __driver_probe_device+0x78/0x110
>> [ 2840.387162] driver_probe_device+0x1f/0xa0
>> [ 2840.387168] __driver_attach+0xba/0x1c0
>> [ 2840.387173] bus_for_each_dev+0x6b/0xb0
>> [ 2840.387180] bus_add_driver+0x111/0x1f0
>> [ 2840.387185] driver_register+0x6e/0xc0
>> [ 2840.387191] do_one_initcall+0x5e/0x3a0
>> [ 2840.387197] do_init_module+0x60/0x220
>> [ 2840.387204] __do_sys_init_module+0x15f/0x190
>> [ 2840.387210] do_syscall_64+0x93/0x180
>> [ 2840.387217] entry_SYSCALL_64_after_hwframe+0x76/0x7e
>> [ 2840.387224]
>> -> #0 (&led_cdev->led_access){+.+.}-{3:3}:
>> [ 2840.387233] __lock_acquire+0x11c6/0x1f20
>> [ 2840.387239] lock_acquire+0xc8/0x2b0
>> [ 2840.387244] __mutex_lock+0x8c/0xc10
>> [ 2840.387251] led_classdev_register_ext+0x1c6/0x380
>> [ 2840.387256] input_leds_connect+0x139/0x260
>> [ 2840.387262] input_attach_handler.isra.0+0x75/0x90
>> [ 2840.387268] input_register_device.cold+0xa1/0x150
>> [ 2840.387274] hidinput_connect+0x848/0xb00
>> [ 2840.387280] hid_connect+0x567/0x5a0
>> [ 2840.387288] hid_hw_start+0x3f/0x60
>> [ 2840.387294] hid_device_probe+0x10d/0x190
>> [ 2840.387298] really_probe+0xde/0x340
>> [ 2840.387304] __driver_probe_device+0x78/0x110
>> [ 2840.387309] driver_probe_device+0x1f/0xa0
>> [ 2840.387314] __device_attach_driver+0x85/0x110
>> [ 2840.387320] bus_for_each_drv+0x78/0xc0
>> [ 2840.387326] __device_attach+0xb0/0x1b0
>> [ 2840.387332] bus_probe_device+0x94/0xb0
>> [ 2840.387337] device_add+0x64a/0x860
>> [ 2840.387343] hid_add_device+0xe5/0x240
>> [ 2840.387349] usbhid_probe+0x4bb/0x600
>> [ 2840.387356] usb_probe_interface+0xea/0x2b0
>> [ 2840.387363] really_probe+0xde/0x340
>> [ 2840.387368] __driver_probe_device+0x78/0x110
>> [ 2840.387373] driver_probe_device+0x1f/0xa0
>> [ 2840.387378] __device_attach_driver+0x85/0x110
>> [ 2840.387383] bus_for_each_drv+0x78/0xc0
>> [ 2840.387390] __device_attach+0xb0/0x1b0
>> [ 2840.387395] bus_probe_device+0x94/0xb0
>> [ 2840.387400] device_add+0x64a/0x860
>> [ 2840.387405] usb_set_configuration+0x5e8/0x880
>> [ 2840.387411] usb_generic_driver_probe+0x3e/0x60
>> [ 2840.387418] usb_probe_device+0x3d/0x120
>> [ 2840.387423] really_probe+0xde/0x340
>> [ 2840.387428] __driver_probe_device+0x78/0x110
>> [ 2840.387434] driver_probe_device+0x1f/0xa0
>> [ 2840.387439] __device_attach_driver+0x85/0x110
>> [ 2840.387444] bus_for_each_drv+0x78/0xc0
>> [ 2840.387451] __device_attach+0xb0/0x1b0
>> [ 2840.387456] bus_probe_device+0x94/0xb0
>> [ 2840.387461] device_add+0x64a/0x860
>> [ 2840.387466] usb_new_device.cold+0x141/0x38f
>> [ 2840.387473] hub_event+0x1166/0x1980
>> [ 2840.387479] process_one_work+0x21a/0x590
>> [ 2840.387484] worker_thread+0x1d1/0x3e0
>> [ 2840.387488] kthread+0xee/0x120
>> [ 2840.387493] ret_from_fork+0x30/0x50
>> [ 2840.387500] ret_from_fork_asm+0x1a/0x30
>> [ 2840.387506]
>> other info that might help us debug this:
>>
>> [ 2840.387509] Chain exists of:
>> &led_cdev->led_access --> &led_cdev->trigger_lock --> input_mutex
>>
>> [ 2840.387520] Possible unsafe locking scenario:
>>
>> [ 2840.387523] CPU0 CPU1
>> [ 2840.387526] ---- ----
>> [ 2840.387529] lock(input_mutex);
>> [ 2840.387534] lock(&led_cdev->trigger_lock);
>> [ 2840.387540] lock(input_mutex);
>> [ 2840.387545] lock(&led_cdev->led_access);
>> [ 2840.387550]
>> *** DEADLOCK ***
>>
>> [ 2840.387552] 7 locks held by kworker/1:1/52:
>> [ 2840.387557] #0: ffff98fcc1d07148 ((wq_completion)usb_hub_wq){+.+.}-{0:0}, at: process_one_work+0x4af/0x590
>> [ 2840.387570] #1: ffffb67e00213e60 ((work_completion)(&hub->events)){+.+.}-{0:0}, at: process_one_work+0x1d5/0x590
>> [ 2840.387583] #2: ffff98fcc6582190 (&dev->mutex){....}-{3:3}, at: hub_event+0x57/0x1980
>> [ 2840.387596] #3: ffff98fccb3c6990 (&dev->mutex){....}-{3:3}, at: __device_attach+0x26/0x1b0
>> [ 2840.387610] #4: ffff98fcc5260960 (&dev->mutex){....}-{3:3}, at: __device_attach+0x26/0x1b0
>> [ 2840.387622] #5: ffff98fce3999a20 (&dev->mutex){....}-{3:3}, at: __device_attach+0x26/0x1b0
>> [ 2840.387635] #6: ffffffff88130cc8 (input_mutex){+.+.}-{3:3}, at: input_register_device.cold+0x47/0x150
>> [ 2840.387649]
>> stack backtrace:
>> [ 2840.387653] CPU: 1 PID: 52 Comm: kworker/1:1 Tainted: G C E 6.10.0-rc1+ #97
>> [ 2840.387659] Hardware name: Xiaomi Inc Mipad2/Mipad, BIOS MIPad-P4.X64.0043.R03.1603071414 03/07/2016
>> [ 2840.387665] Workqueue: usb_hub_wq hub_event
>> [ 2840.387674] Call Trace:
>> [ 2840.387681] <TASK>
>> [ 2840.387689] dump_stack_lvl+0x68/0x90
>> [ 2840.387700] check_noncircular+0x10d/0x120
>> [ 2840.387710] ? register_lock_class+0x38/0x480
>> [ 2840.387717] ? check_noncircular+0x74/0x120
>> [ 2840.387727] __lock_acquire+0x11c6/0x1f20
>> [ 2840.387736] lock_acquire+0xc8/0x2b0
>> [ 2840.387743] ? led_classdev_register_ext+0x1c6/0x380
>> [ 2840.387753] __mutex_lock+0x8c/0xc10
>> [ 2840.387760] ? led_classdev_register_ext+0x1c6/0x380
>> [ 2840.387766] ? _raw_spin_unlock_irqrestore+0x35/0x60
>> [ 2840.387773] ? klist_next+0x158/0x160
>> [ 2840.387781] ? led_classdev_register_ext+0x1c6/0x380
>> [ 2840.387787] ? lockdep_init_map_type+0x58/0x250
>> [ 2840.387796] ? led_classdev_register_ext+0x1c6/0x380
>> [ 2840.387802] led_classdev_register_ext+0x1c6/0x380
>> [ 2840.387810] ? kvasprintf+0x70/0xb0
>> [ 2840.387820] ? kasprintf+0x3e/0x50
>> [ 2840.387829] input_leds_connect+0x139/0x260
>> [ 2840.387838] input_attach_handler.isra.0+0x75/0x90
>> [ 2840.387846] input_register_device.cold+0xa1/0x150
>> [ 2840.387854] hidinput_connect+0x848/0xb00
>> [ 2840.387862] ? usbhid_start+0x45b/0x7b0
>> [ 2840.387870] hid_connect+0x567/0x5a0
>> [ 2840.387878] ? __mutex_unlock_slowpath+0x2d/0x260
>> [ 2840.387891] hid_hw_start+0x3f/0x60
>> [ 2840.387899] hid_device_probe+0x10d/0x190
>> [ 2840.387906] ? __pfx___device_attach_driver+0x10/0x10
>> [ 2840.387913] really_probe+0xde/0x340
>> [ 2840.387919] ? pm_runtime_barrier+0x50/0x90
>> [ 2840.387927] __driver_probe_device+0x78/0x110
>> [ 2840.387934] driver_probe_device+0x1f/0xa0
>> [ 2840.387941] __device_attach_driver+0x85/0x110
>> [ 2840.387949] bus_for_each_drv+0x78/0xc0
>> [ 2840.387959] __device_attach+0xb0/0x1b0
>> [ 2840.387967] bus_probe_device+0x94/0xb0
>> [ 2840.387974] device_add+0x64a/0x860
>> [ 2840.387982] ? __debugfs_create_file+0x14a/0x1c0
>> [ 2840.387993] hid_add_device+0xe5/0x240
>> [ 2840.388002] usbhid_probe+0x4bb/0x600
>> [ 2840.388013] usb_probe_interface+0xea/0x2b0
>> [ 2840.388021] ? __pfx___device_attach_driver+0x10/0x10
>> [ 2840.388028] really_probe+0xde/0x340
>> [ 2840.388034] ? pm_runtime_barrier+0x50/0x90
>> [ 2840.388040] __driver_probe_device+0x78/0x110
>> [ 2840.388048] driver_probe_device+0x1f/0xa0
>> [ 2840.388055] __device_attach_driver+0x85/0x110
>> [ 2840.388062] bus_for_each_drv+0x78/0xc0
>> [ 2840.388071] __device_attach+0xb0/0x1b0
>> [ 2840.388079] bus_probe_device+0x94/0xb0
>> [ 2840.388086] device_add+0x64a/0x860
>> [ 2840.388094] ? __mutex_unlock_slowpath+0x2d/0x260
>> [ 2840.388103] usb_set_configuration+0x5e8/0x880
>> [ 2840.388114] ? __pfx___device_attach_driver+0x10/0x10
>> [ 2840.388121] usb_generic_driver_probe+0x3e/0x60
>> [ 2840.388129] usb_probe_device+0x3d/0x120
>> [ 2840.388137] really_probe+0xde/0x340
>> [ 2840.388142] ? pm_runtime_barrier+0x50/0x90
>> [ 2840.388149] __driver_probe_device+0x78/0x110
>> [ 2840.388156] driver_probe_device+0x1f/0xa0
>> [ 2840.388163] __device_attach_driver+0x85/0x110
>> [ 2840.388171] bus_for_each_drv+0x78/0xc0
>> [ 2840.388180] __device_attach+0xb0/0x1b0
>> [ 2840.388188] bus_probe_device+0x94/0xb0
>> [ 2840.388195] device_add+0x64a/0x860
>> [ 2840.388202] ? lockdep_hardirqs_on+0x78/0x100
>> [ 2840.388210] ? _raw_spin_unlock_irqrestore+0x35/0x60
>> [ 2840.388219] usb_new_device.cold+0x141/0x38f
>> [ 2840.388227] hub_event+0x1166/0x1980
>> [ 2840.388242] process_one_work+0x21a/0x590
>> [ 2840.388249] ? move_linked_works+0x70/0xa0
>> [ 2840.388260] worker_thread+0x1d1/0x3e0
>> [ 2840.388268] ? __pfx_worker_thread+0x10/0x10
>> [ 2840.388273] kthread+0xee/0x120
>> [ 2840.388279] ? __pfx_kthread+0x10/0x10
>> [ 2840.388287] ret_from_fork+0x30/0x50
>> [ 2840.388294] ? __pfx_kthread+0x10/0x10
>> [ 2840.388301] ret_from_fork_asm+0x1a/0x30
>> [ 2840.388315] </TASK>
>> [ 2840.415630] hid-generic 0003:0603:0002.0007: input,hidraw6: USB HID v1.10 Keyboard [SINO WEALTH USB Composite Device] on usb-0000:00:14.0-1.3/input0
>
> Do we really have to add the _whole_ trace?
>
> Is there no way to snip it a little?
It should be possible to shorten it somewhat for v2.
> Or perhaps Link: to the whole trace?
>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> drivers/leds/trigger/ledtrig-input-events.c | 136 +++++---------------
>> 1 file changed, 34 insertions(+), 102 deletions(-)
>
> This is nice!
>
>> diff --git a/drivers/leds/trigger/ledtrig-input-events.c b/drivers/leds/trigger/ledtrig-input-events.c
>> index 1de0176799f9..1c79731562c2 100644
>> --- a/drivers/leds/trigger/ledtrig-input-events.c
>> +++ b/drivers/leds/trigger/ledtrig-input-events.c
>> @@ -3,31 +3,32 @@
>> * Input Events LED trigger
>> *
>> * Copyright (C) 2024 Hans de Goede <hansg@kernel.org>
>> - * Partially based on Atsushi Nemoto's ledtrig-heartbeat.c.
>> */
>>
>> #include <linux/input.h>
>> #include <linux/jiffies.h>
>> #include <linux/leds.h>
>> #include <linux/module.h>
>> +#include <linux/moduleparam.h>
>> #include <linux/slab.h>
>> #include <linux/spinlock.h>
>> #include <linux/workqueue.h>
>> #include "../leds.h"
>>
>> -#define DEFAULT_LED_OFF_DELAY_MS 5000
>> +static unsigned long led_off_delay_ms = 5000;
>> +module_param(led_off_delay_ms, ulong, 0644);
>> +MODULE_PARM_DESC(led_off_delay_ms,
>> + "Specify delay in ms for turning LEDs off after last input event");
>>
>> -struct input_events_data {
>> - struct input_handler handler;
>> +static struct input_events_data {
>> struct delayed_work work;
>> spinlock_t lock;
>> - struct led_classdev *led_cdev;
>> - int led_cdev_saved_flags;
>> /* To avoid repeatedly setting the brightness while there are events */
>> bool led_on;
>> unsigned long led_off_time;
>> - unsigned long led_off_delay;
>> -};
>> +} input_events_data;
>
> Does this fix really require statically declared structs?
With the move the the simple LED trigger model there is a single
global state for all LEDs controlled by this trigger rather then
per LED state.
So we need to have global static data for this. To keep the diff
compared to the previous version small; and because I personally
find doing so somewhat cleaner; I have chosen to keep this global
state in a struct rather then using multiple global variables
for this.
If you strongly prefer using multiple global variables for this
then that can be changed for v2.
>
>> +static struct led_trigger *input_events_led_trigger;
>>
>> static void led_input_events_work(struct work_struct *work)
>> {
>> @@ -41,62 +42,24 @@ static void led_input_events_work(struct work_struct *work)
>> * running before a new event pushed led_off_time back.
>> */
>> if (time_after_eq(jiffies, data->led_off_time)) {
>> - led_set_brightness_nosleep(data->led_cdev, LED_OFF);
>> + led_trigger_event(input_events_led_trigger, LED_OFF);
>> data->led_on = false;
>> }
>>
>> spin_unlock_irq(&data->lock);
>> }
>>
>> -static ssize_t delay_show(struct device *dev, struct device_attribute *attr, char *buf)
>> -{
>> - struct input_events_data *input_events_data = led_trigger_get_drvdata(dev);
>> -
>> - return sysfs_emit(buf, "%lu\n", input_events_data->led_off_delay);
>> -}
>> -
>> -static ssize_t delay_store(struct device *dev, struct device_attribute *attr,
>> - const char *buf, size_t size)
>> -{
>> - struct input_events_data *input_events_data = led_trigger_get_drvdata(dev);
>> - unsigned long delay;
>> - int ret;
>> -
>> - ret = kstrtoul(buf, 0, &delay);
>> - if (ret)
>> - return ret;
>> -
>> - /* Clamp between 0.5 and 1000 seconds */
>> - delay = clamp_val(delay, 500UL, 1000000UL);
>> - input_events_data->led_off_delay = msecs_to_jiffies(delay);
>> -
>> - return size;
>> -}
>> -
>> -static DEVICE_ATTR_RW(delay);
>> -
>> -static struct attribute *input_events_led_attrs[] = {
>> - &dev_attr_delay.attr,
>> - NULL
>> -};
>> -ATTRIBUTE_GROUPS(input_events_led);
>> -
>> static void input_events_event(struct input_handle *handle, unsigned int type,
>> unsigned int code, int val)
>> {
>> - struct input_events_data *data =
>> - container_of(handle->handler, struct input_events_data, handler);
>> - unsigned long led_off_delay = READ_ONCE(data->led_off_delay);
>> - struct led_classdev *led_cdev = data->led_cdev;
>> + struct input_events_data *data = &input_events_data;
>
> IFF we have to use statically declared structs ...
>
> Why not just use input_events_data.foo?
Yes that will work too. I went this route both for readability
(data->foo is easier to read then input_events_data.foo) as
well as to keep the diff smaller since some of the old code
which this patch keeps around has data->foo.
Note both arguments clearly are invalid if the decision is made
to switch to using multiple global variables instead of using
a static global struct.
>
>> + unsigned long led_off_delay = msecs_to_jiffies(led_off_delay_ms);
>> unsigned long flags;
>>
>> - if (test_and_clear_bit(LED_BLINK_BRIGHTNESS_CHANGE, &led_cdev->work_flags))
>> - led_cdev->blink_brightness = led_cdev->new_blink_brightness;
>> -
>> spin_lock_irqsave(&data->lock, flags);
>>
>> if (!data->led_on) {
>> - led_set_brightness_nosleep(led_cdev, led_cdev->blink_brightness);
>> + led_trigger_event(input_events_led_trigger, LED_FULL);
>> data->led_on = true;
>> }
>> data->led_off_time = jiffies + led_off_delay;
>> @@ -118,7 +81,7 @@ static int input_events_connect(struct input_handler *handler, struct input_dev
>>
>> handle->dev = dev;
>> handle->handler = handler;
>> - handle->name = "input-events";
>> + handle->name = KBUILD_MODNAME;
>>
>> ret = input_register_handle(handle);
>> if (ret)
>> @@ -160,72 +123,41 @@ static const struct input_device_id input_events_ids[] = {
>> { }
>> };
>>
>> -static int input_events_activate(struct led_classdev *led_cdev)
>> +static struct input_handler input_events_handler = {
>> + .name = KBUILD_MODNAME,
>> + .event = input_events_event,
>> + .connect = input_events_connect,
>> + .disconnect = input_events_disconnect,
>> + .id_table = input_events_ids,
>> +};
>> +
>> +static int __init input_events_init(void)
>> {
>> - struct input_events_data *data;
>> int ret;
>>
>> - data = kzalloc(sizeof(*data), GFP_KERNEL);
>> - if (!data)
>> - return -ENOMEM;
>> + INIT_DELAYED_WORK(&input_events_data.work, led_input_events_work);
>> + spin_lock_init(&input_events_data.lock);
>>
>> - data->handler.name = "input-events";
>> - data->handler.event = input_events_event;
>> - data->handler.connect = input_events_connect;
>> - data->handler.disconnect = input_events_disconnect;
>> - data->handler.id_table = input_events_ids;
>> + led_trigger_register_simple("input-events", &input_events_led_trigger);
>>
>> - INIT_DELAYED_WORK(&data->work, led_input_events_work);
>> - spin_lock_init(&data->lock);
>> -
>> - data->led_cdev = led_cdev;
>> - data->led_cdev_saved_flags = led_cdev->flags;
>> - data->led_off_delay = msecs_to_jiffies(DEFAULT_LED_OFF_DELAY_MS);
>> -
>> - /*
>> - * Use led_cdev->blink_brightness + LED_BLINK_SW flag so that sysfs
>> - * brightness writes will change led_cdev->new_blink_brightness for
>> - * configuring the on state brightness (like ledtrig-heartbeat).
>> - */
>> - if (!led_cdev->blink_brightness)
>> - led_cdev->blink_brightness = led_cdev->max_brightness;
>> -
>> - /* Start with LED off */
>> - led_set_brightness_nosleep(data->led_cdev, LED_OFF);
>> -
>> - ret = input_register_handler(&data->handler);
>> + ret = input_register_handler(&input_events_handler);
>> if (ret) {
>> - kfree(data);
>> + led_trigger_unregister_simple(input_events_led_trigger);
>> return ret;
>> }
>>
>> - set_bit(LED_BLINK_SW, &led_cdev->work_flags);
>> -
>> - /* Turn LED off during suspend, original flags are restored on deactivate() */
>> - led_cdev->flags |= LED_CORE_SUSPENDRESUME;
>> -
>> - led_set_trigger_data(led_cdev, data);
>> return 0;
>> }
>>
>> -static void input_events_deactivate(struct led_classdev *led_cdev)
>> +static void __exit input_events_exit(void)
>> {
>> - struct input_events_data *data = led_get_trigger_data(led_cdev);
>> -
>> - led_cdev->flags = data->led_cdev_saved_flags;
>> - clear_bit(LED_BLINK_SW, &led_cdev->work_flags);
>> - input_unregister_handler(&data->handler);
>> - cancel_delayed_work_sync(&data->work);
>> - kfree(data);
>> + input_unregister_handler(&input_events_handler);
>> + cancel_delayed_work_sync(&input_events_data.work);
>> + led_trigger_unregister_simple(input_events_led_trigger);
>> }
>>
>> -static struct led_trigger input_events_led_trigger = {
>> - .name = "input-events",
>> - .activate = input_events_activate,
>> - .deactivate = input_events_deactivate,
>> - .groups = input_events_led_groups,
>> -};
>> -module_led_trigger(input_events_led_trigger);
>> +module_init(input_events_init);
>> +module_exit(input_events_exit);
>>
>> MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
>> MODULE_DESCRIPTION("Input Events LED trigger");
>> --
>> 2.45.1
>>
>>
>
Regards,
Hans
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: (subset) [PATCH 1/1] leds: trigger: input-events: Rewrite to fix a serious locking issue
2024-06-02 16:02 ` [PATCH 1/1] " Hans de Goede
2024-06-13 14:31 ` Lee Jones
@ 2024-06-20 9:40 ` Lee Jones
1 sibling, 0 replies; 5+ messages in thread
From: Lee Jones @ 2024-06-20 9:40 UTC (permalink / raw)
To: Pavel Machek, Hans de Goede; +Cc: linux-leds
On Sun, 02 Jun 2024 18:02:03 +0200, Hans de Goede wrote:
> The input subsystem registers LEDs with default triggers while holding
> the input_lock and input_register_handler() takes the input_lock this
> means that a triggers activate method cannot directly call
> input_register_handler() as the old ledtrig-input-events code is doing.
>
> The initial implementation of the input-events trigger mainly did not use
> the simple LED trigger mechanism because that mechanism had an issue with
> the initial state of a newly activated LED not matching the last
> led_trigger_event() call for the trigger. This issue has been fixed in
> commit 822c91e72eac ("leds: trigger: Store brightness set by
> led_trigger_event()").
>
> [...]
Applied, thanks!
[1/1] leds: trigger: input-events: Rewrite to fix a serious locking issue
commit: 284ad8e7d5eeb1659b63f44fd094373900322de6
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-06-20 9:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-02 16:02 [PATCH 0/1] leds: trigger: input-events: Rewrite to fix a serious locking issue Hans de Goede
2024-06-02 16:02 ` [PATCH 1/1] " Hans de Goede
2024-06-13 14:31 ` Lee Jones
2024-06-14 16:04 ` Hans de Goede
2024-06-20 9:40 ` (subset) " Lee Jones
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).