From 811b5440c6e4998755990fd2c1455f42f3aae3b0 Mon Sep 17 00:00:00 2001 From: Arnd Bergmann Date: Mon, 27 Sep 2021 15:15:47 +0200 Subject: led-class-flash: fix -Wrestrict warning gcc-11 warns when building with W=1: drivers/leds/led-class-flash.c: In function 'flash_fault_show': drivers/leds/led-class-flash.c:210:16: error: 'sprintf' argument 3 overlaps destination object 'buf' [-Werror=restrict] 210 | return sprintf(buf, "%s\n", buf); | ^~~~~~~~~~~~~~~~~~~~~~~~~ drivers/leds/led-class-flash.c:187:54: note: destination object referenced by 'restrict'-qualified argument 1 was declared here 187 | struct device_attribute *attr, char *buf) | ~~~~~~^~~ There is no need for the sprintf() here when a strcat() does the same thing without invoking undefined behavior. Signed-off-by: Arnd Bergmann Signed-off-by: Pavel Machek --- drivers/leds/led-class-flash.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers') diff --git a/drivers/leds/led-class-flash.c b/drivers/leds/led-class-flash.c index 185e17055317..6fe9d700dfef 100644 --- a/drivers/leds/led-class-flash.c +++ b/drivers/leds/led-class-flash.c @@ -207,7 +207,7 @@ static ssize_t flash_fault_show(struct device *dev, mask <<= 1; } - return sprintf(buf, "%s\n", buf); + return strlen(strcat(buf, "\n")); } static DEVICE_ATTR_RO(flash_fault); -- cgit v1.2.3-70-g09d2 From 2a5a8fa8b23144d14567d6f8293dd6fbeecee393 Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Wed, 15 Sep 2021 18:16:01 +0200 Subject: leds: trigger: use RCU to protect the led_cdevs list Even with the previous commit 27af8e2c90fb ("leds: trigger: fix potential deadlock with libata") to this file, we still get lockdep unhappy, and Boqun explained the report here: https://lore.kernel.org/r/YNA+d1X4UkoQ7g8a@boqun-archlinux Effectively, this means that the read_lock_irqsave() isn't enough here because another CPU might be trying to do a write lock, and thus block the readers. This is all pretty messy, but it doesn't seem right that the LEDs framework imposes some locking requirements on users, in particular we'd have to make the spinlock in the iwlwifi driver always disable IRQs, even if we don't need that for any other reason, just to avoid this deadlock. Since writes to the led_cdevs list are rare (and are done by userspace), just switch the list to RCU. This costs a synchronize_rcu() at removal time so we can ensure things are correct, but that seems like a small price to pay for getting lock-free iterations and no deadlocks (nor any locking requirements imposed on users.) Signed-off-by: Johannes Berg Signed-off-by: Pavel Machek --- drivers/leds/led-triggers.c | 41 +++++++++++++++++++++-------------------- include/linux/leds.h | 2 +- 2 files changed, 22 insertions(+), 21 deletions(-) (limited to 'drivers') diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c index 4e7b78a84149..072491d3e17b 100644 --- a/drivers/leds/led-triggers.c +++ b/drivers/leds/led-triggers.c @@ -157,7 +157,6 @@ EXPORT_SYMBOL_GPL(led_trigger_read); /* Caller must ensure led_cdev->trigger_lock held */ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig) { - unsigned long flags; char *event = NULL; char *envp[2]; const char *name; @@ -171,10 +170,13 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig) /* Remove any existing trigger */ if (led_cdev->trigger) { - write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags); - list_del(&led_cdev->trig_list); - write_unlock_irqrestore(&led_cdev->trigger->leddev_list_lock, - flags); + spin_lock(&led_cdev->trigger->leddev_list_lock); + list_del_rcu(&led_cdev->trig_list); + spin_unlock(&led_cdev->trigger->leddev_list_lock); + + /* ensure it's no longer visible on the led_cdevs list */ + synchronize_rcu(); + cancel_work_sync(&led_cdev->set_brightness_work); led_stop_software_blink(led_cdev); if (led_cdev->trigger->deactivate) @@ -186,9 +188,9 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig) led_set_brightness(led_cdev, LED_OFF); } if (trig) { - write_lock_irqsave(&trig->leddev_list_lock, flags); - list_add_tail(&led_cdev->trig_list, &trig->led_cdevs); - write_unlock_irqrestore(&trig->leddev_list_lock, flags); + spin_lock(&trig->leddev_list_lock); + list_add_tail_rcu(&led_cdev->trig_list, &trig->led_cdevs); + spin_unlock(&trig->leddev_list_lock); led_cdev->trigger = trig; if (trig->activate) @@ -223,9 +225,10 @@ err_add_groups: trig->deactivate(led_cdev); err_activate: - write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags); - list_del(&led_cdev->trig_list); - write_unlock_irqrestore(&led_cdev->trigger->leddev_list_lock, flags); + spin_lock(&led_cdev->trigger->leddev_list_lock); + list_del_rcu(&led_cdev->trig_list); + spin_unlock(&led_cdev->trigger->leddev_list_lock); + synchronize_rcu(); led_cdev->trigger = NULL; led_cdev->trigger_data = NULL; led_set_brightness(led_cdev, LED_OFF); @@ -285,7 +288,7 @@ int led_trigger_register(struct led_trigger *trig) struct led_classdev *led_cdev; struct led_trigger *_trig; - rwlock_init(&trig->leddev_list_lock); + spin_lock_init(&trig->leddev_list_lock); INIT_LIST_HEAD(&trig->led_cdevs); down_write(&triggers_list_lock); @@ -378,15 +381,14 @@ void led_trigger_event(struct led_trigger *trig, enum led_brightness brightness) { struct led_classdev *led_cdev; - unsigned long flags; if (!trig) return; - read_lock_irqsave(&trig->leddev_list_lock, flags); - list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list) + rcu_read_lock(); + list_for_each_entry_rcu(led_cdev, &trig->led_cdevs, trig_list) led_set_brightness(led_cdev, brightness); - read_unlock_irqrestore(&trig->leddev_list_lock, flags); + rcu_read_unlock(); } EXPORT_SYMBOL_GPL(led_trigger_event); @@ -397,20 +399,19 @@ static void led_trigger_blink_setup(struct led_trigger *trig, int invert) { struct led_classdev *led_cdev; - unsigned long flags; if (!trig) return; - read_lock_irqsave(&trig->leddev_list_lock, flags); - list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list) { + rcu_read_lock(); + list_for_each_entry_rcu(led_cdev, &trig->led_cdevs, trig_list) { if (oneshot) led_blink_set_oneshot(led_cdev, delay_on, delay_off, invert); else led_blink_set(led_cdev, delay_on, delay_off); } - read_unlock_irqrestore(&trig->leddev_list_lock, flags); + rcu_read_unlock(); } void led_trigger_blink(struct led_trigger *trig, diff --git a/include/linux/leds.h b/include/linux/leds.h index a0b730be40ad..ba4861ec73d3 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -360,7 +360,7 @@ struct led_trigger { struct led_hw_trigger_type *trigger_type; /* LEDs under control by this trigger (for simple triggers) */ - rwlock_t leddev_list_lock; + spinlock_t leddev_list_lock; struct list_head led_cdevs; /* Link to next registered trigger */ -- cgit v1.2.3-70-g09d2 From 97b31c1f8eb865bc3aa5f4a08286a6406d782ea8 Mon Sep 17 00:00:00 2001 From: Sebastian Andrzej Siewior Date: Wed, 13 Oct 2021 11:37:19 +0200 Subject: leds: trigger: Disable CPU trigger on PREEMPT_RT The CPU trigger is invoked on ARM from CPU-idle. That trigger later invokes led_trigger_event() which may invoke the callback of the actual driver. That driver can acquire a spinlock_t which is okay on kernel without PREEMPT_RT. On a PREEMPT_RT enabled kernel this lock is turned into a sleeping lock and must not be acquired with disabled interrupts. Disable the CPU trigger on PREEMPT_RT. Signed-off-by: Sebastian Andrzej Siewior Link: https://lkml.kernel.org/r/20210924111501.m57cwwn7ahiyxxdd@linutronix.de Signed-off-by: Pavel Machek --- drivers/leds/trigger/Kconfig | 1 + 1 file changed, 1 insertion(+) (limited to 'drivers') diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig index 1f1d57288085..dc6816d36d06 100644 --- a/drivers/leds/trigger/Kconfig +++ b/drivers/leds/trigger/Kconfig @@ -64,6 +64,7 @@ config LEDS_TRIGGER_BACKLIGHT config LEDS_TRIGGER_CPU bool "LED CPU Trigger" + depends on !PREEMPT_RT help This allows LEDs to be controlled by active CPUs. This shows the active CPUs across an array of LEDs so you can see which -- cgit v1.2.3-70-g09d2