As per commit 916fe619951f ("leds: trigger: Introduce a kernel
panic LED trigger"), the kernel now supports a new LED trigger
to hook on the panic blink.
However, the only way of using this is to dedicate a LED device,
making it rather useless.
To overcome this limitation, the present series introduces the
capability to switch the LED trigger of certain LED devices upon
a kernel panic (using the panic notifier).
The decision of which LEDs should be switched to the panic trigger
is left to each LED device driver. As an example, a devicetree
boolean property is introduced and used in the leds-gpio driver.
The big change in this v3 is that I've moved the panic trigger
switching away from the core code and it's now part of
ledtrig-panic.c. Pavel, Jacek: How does it look?
Changes from v2:
* Added Rob's "panic-indicator" devicetree property Acked-by.
* Fix typo, as pointed out by Robin Murphy.
* Documented "panic-indicator" in bindings/leds/leds-gpio.txt.
* Moved the panic trigger switching from the trigger core code,
to ledtrig-panic.c.
Changes from v1:
* Dropped the led_trigger_event_nosleep API, and instead just
clear the blink_delay_{on, off} when the panic is notified.
This results in less changes.
* Changed the flag to LED_PANIC_INDICATOR, as requested by Jacek.
* Changed the firmware property name to "panic-indicator", as
requested by Jacek.
Ezequiel Garcia (3):
leds: triggers: Allow to switch the trigger to "panic" on a kernel
panic
devicetree: leds: Introduce "panic-indicator" optional property
leds: gpio: Support the "panic-indicator" firmware property
Documentation/devicetree/bindings/leds/common.txt | 3 ++
.../devicetree/bindings/leds/leds-gpio.txt | 2 +
drivers/leds/led-triggers.c | 2 +-
drivers/leds/leds-gpio.c | 4 ++
drivers/leds/leds.h | 1 +
drivers/leds/trigger/Kconfig | 3 ++
drivers/leds/trigger/ledtrig-panic.c | 47 ++++++++++++++++++++++
include/linux/leds.h | 2 +
8 files changed, 63 insertions(+), 1 deletion(-)
--
2.7.0
This commit adds a new led_cdev flag LED_PANIC_INDICATOR, which
allows to mark a specific LED to be switched to the "panic"
trigger, on a kernel panic.
This is useful to allow the user to assign a regular trigger
to a given LED, and still blink that LED on a kernel panic.
Signed-off-by: Ezequiel Garcia <[email protected]>
---
drivers/leds/led-triggers.c | 2 +-
drivers/leds/leds.h | 1 +
drivers/leds/trigger/Kconfig | 3 +++
drivers/leds/trigger/ledtrig-panic.c | 47 ++++++++++++++++++++++++++++++++++++
include/linux/leds.h | 1 +
5 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 2181581795d3..55fa65e1ae03 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -26,7 +26,7 @@
* Nests outside led_cdev->trigger_lock
*/
static DECLARE_RWSEM(triggers_list_lock);
-static LIST_HEAD(trigger_list);
+LIST_HEAD(trigger_list);
/* Used by LED Class */
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index db3f20da7221..7d38e6b9a740 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -30,5 +30,6 @@ void led_set_brightness_nosleep(struct led_classdev *led_cdev,
extern struct rw_semaphore leds_list_lock;
extern struct list_head leds_list;
+extern struct list_head trigger_list;
#endif /* __LEDS_H_INCLUDED */
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index beac8c31c51b..4e4521c9072a 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -121,6 +121,9 @@ config LEDS_TRIGGER_PANIC
depends on LEDS_TRIGGERS
help
This allows LEDs to be configured to blink on a kernel panic.
+ Enabling this option will allow to mark certain LEDs as 'panic-indicators',
+ allowing to blink them on a kernel panic, even if they are set to
+ a different trigger.
If unsure, say Y.
endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/ledtrig-panic.c b/drivers/leds/trigger/ledtrig-panic.c
index 627b350c5ec3..3e447bd2064a 100644
--- a/drivers/leds/trigger/ledtrig-panic.c
+++ b/drivers/leds/trigger/ledtrig-panic.c
@@ -11,10 +11,54 @@
#include <linux/kernel.h>
#include <linux/init.h>
+#include <linux/notifier.h>
#include <linux/leds.h>
+#include "../leds.h"
static struct led_trigger *trigger;
+/*
+ * This is a called in a special context by the atomic panic
+ * notifier. This means the trigger can be changed without
+ * worrying about locking.
+ */
+static void led_trigger_set_panic(struct led_classdev *led_cdev)
+{
+ struct led_trigger *trig;
+
+ list_for_each_entry(trig, &trigger_list, next_trig) {
+ if (strcmp("panic", trig->name))
+ continue;
+ if (led_cdev->trigger)
+ list_del(&led_cdev->trig_list);
+ list_add_tail(&led_cdev->trig_list, &trig->led_cdevs);
+
+ /* Avoid the delayed blink path */
+ led_cdev->blink_delay_on = 0;
+ led_cdev->blink_delay_off = 0;
+
+ led_cdev->trigger = trig;
+ if (trig->activate)
+ trig->activate(led_cdev);
+ break;
+ }
+}
+
+static int led_trigger_panic_notifier(struct notifier_block *nb,
+ unsigned long code, void *unused)
+{
+ struct led_classdev *led_cdev;
+
+ list_for_each_entry(led_cdev, &leds_list, node)
+ if (led_cdev->flags & LED_PANIC_INDICATOR)
+ led_trigger_set_panic(led_cdev);
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block led_trigger_panic_nb = {
+ .notifier_call = led_trigger_panic_notifier,
+};
+
static long led_panic_blink(int state)
{
led_trigger_event(trigger, state ? LED_FULL : LED_OFF);
@@ -23,6 +67,9 @@ static long led_panic_blink(int state)
static int __init ledtrig_panic_init(void)
{
+ atomic_notifier_chain_register(&panic_notifier_list,
+ &led_trigger_panic_nb);
+
led_trigger_register_simple("panic", &trigger);
panic_blink = led_panic_blink;
return 0;
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 19eb10278bea..7e9fb00e15e8 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -50,6 +50,7 @@ struct led_classdev {
#define LED_SYSFS_DISABLE (1 << 22)
#define LED_DEV_CAP_FLASH (1 << 23)
#define LED_HW_PLUGGABLE (1 << 24)
+#define LED_PANIC_INDICATOR (1 << 25)
/* Set LED brightness level
* Must not sleep. Use brightness_set_blocking for drivers
--
2.7.0
Calling a GPIO LEDs is quite likely to work even if the kernel
has paniced, so they are ideal to blink in this situation.
This commit adds support for the new "panic-indicator"
firmware property, allowing to mark a given LED to blink on
a kernel panic.
Signed-off-by: Ezequiel Garcia <[email protected]>
---
Documentation/devicetree/bindings/leds/leds-gpio.txt | 2 ++
drivers/leds/leds-gpio.c | 4 ++++
include/linux/leds.h | 1 +
3 files changed, 7 insertions(+)
diff --git a/Documentation/devicetree/bindings/leds/leds-gpio.txt b/Documentation/devicetree/bindings/leds/leds-gpio.txt
index fea1ebfe24a9..cbbeb1850910 100644
--- a/Documentation/devicetree/bindings/leds/leds-gpio.txt
+++ b/Documentation/devicetree/bindings/leds/leds-gpio.txt
@@ -23,6 +23,8 @@ LED sub-node properties:
property is not present.
- retain-state-suspended: (optional) The suspend state can be retained.Such
as charge-led gpio.
+- panic-indicator : (optional)
+ see Documentation/devicetree/bindings/leds/common.txt
Examples:
diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 61143f55597e..8229f063b483 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -127,6 +127,8 @@ static int create_gpio_led(const struct gpio_led *template,
led_dat->cdev.brightness = state ? LED_FULL : LED_OFF;
if (!template->retain_state_suspended)
led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
+ if (template->panic_indicator)
+ led_dat->cdev.flags |= LED_PANIC_INDICATOR;
ret = gpiod_direction_output(led_dat->gpiod, state);
if (ret < 0)
@@ -200,6 +202,8 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
if (fwnode_property_present(child, "retain-state-suspended"))
led.retain_state_suspended = 1;
+ if (fwnode_property_present(child, "panic-indicator"))
+ led.panic_indicator = 1;
ret = create_gpio_led(&led, &priv->leds[priv->num_leds],
dev, NULL);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 7e9fb00e15e8..d2b13066e781 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -365,6 +365,7 @@ struct gpio_led {
unsigned gpio;
unsigned active_low : 1;
unsigned retain_state_suspended : 1;
+ unsigned panic_indicator : 1;
unsigned default_state : 2;
/* default_state should be one of LEDS_GPIO_DEFSTATE_(ON|OFF|KEEP) */
struct gpio_desc *gpiod;
--
2.7.0
It's desirable to specify which LEDs are to be blinked on a kernel
panic. Therefore, introduce a devicetree boolean property to mark
which LEDs should be treated this way, if possible.
Acked-by: Rob Herring <[email protected]>
Signed-off-by: Ezequiel Garcia <[email protected]>
---
Documentation/devicetree/bindings/leds/common.txt | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
index 68419843e32f..af10678ea2f6 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -37,6 +37,9 @@ Optional properties for child nodes:
property is mandatory for the LEDs in the non-flash modes
(e.g. torch or indicator).
+- panic-indicator : This property specifies that the LED should be used,
+ if at all possible, as a panic indicator.
+
Required properties for flash LED child nodes:
- flash-max-microamp : Maximum flash LED supply current in microamperes.
- flash-max-timeout-us : Maximum timeout in microseconds after which the flash
--
2.7.0
On Thu 2016-04-28 19:03:37, Ezequiel Garcia wrote:
> As per commit 916fe619951f ("leds: trigger: Introduce a kernel
> panic LED trigger"), the kernel now supports a new LED trigger
> to hook on the panic blink.
>
> However, the only way of using this is to dedicate a LED device,
> making it rather useless.
>
> To overcome this limitation, the present series introduces the
> capability to switch the LED trigger of certain LED devices upon
> a kernel panic (using the panic notifier).
>
> The decision of which LEDs should be switched to the panic trigger
> is left to each LED device driver. As an example, a devicetree
> boolean property is introduced and used in the leds-gpio driver.
>
> The big change in this v3 is that I've moved the panic trigger
> switching away from the core code and it's now part of
> ledtrig-panic.c. Pavel, Jacek: How does it look?
Seems better now. Thanks for doing that.
For the series:
Acked-by: Pavel Machek <[email protected]>
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Ezequiel,
Thanks for the update. It's indeed reasonable to have all the
switching infrastructure in ledtrig-panic.c.
I've noticed two minor issues below.
On 04/29/2016 12:03 AM, Ezequiel Garcia wrote:
> This commit adds a new led_cdev flag LED_PANIC_INDICATOR, which
> allows to mark a specific LED to be switched to the "panic"
> trigger, on a kernel panic.
>
> This is useful to allow the user to assign a regular trigger
> to a given LED, and still blink that LED on a kernel panic.
>
> Signed-off-by: Ezequiel Garcia <[email protected]>
> ---
> drivers/leds/led-triggers.c | 2 +-
> drivers/leds/leds.h | 1 +
> drivers/leds/trigger/Kconfig | 3 +++
> drivers/leds/trigger/ledtrig-panic.c | 47 ++++++++++++++++++++++++++++++++++++
> include/linux/leds.h | 1 +
> 5 files changed, 53 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 2181581795d3..55fa65e1ae03 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -26,7 +26,7 @@
> * Nests outside led_cdev->trigger_lock
> */
> static DECLARE_RWSEM(triggers_list_lock);
> -static LIST_HEAD(trigger_list);
> +LIST_HEAD(trigger_list);
>
> /* Used by LED Class */
>
> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> index db3f20da7221..7d38e6b9a740 100644
> --- a/drivers/leds/leds.h
> +++ b/drivers/leds/leds.h
> @@ -30,5 +30,6 @@ void led_set_brightness_nosleep(struct led_classdev *led_cdev,
>
> extern struct rw_semaphore leds_list_lock;
> extern struct list_head leds_list;
> +extern struct list_head trigger_list;
>
> #endif /* __LEDS_H_INCLUDED */
> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> index beac8c31c51b..4e4521c9072a 100644
> --- a/drivers/leds/trigger/Kconfig
> +++ b/drivers/leds/trigger/Kconfig
> @@ -121,6 +121,9 @@ config LEDS_TRIGGER_PANIC
> depends on LEDS_TRIGGERS
> help
> This allows LEDs to be configured to blink on a kernel panic.
> + Enabling this option will allow to mark certain LEDs as 'panic-indicators',
s/"panic-indicators"/panic indicators/
I understand that you referred here to the DT property name, but this
is not obvious at first glance, and it is an implementation detail.
> + allowing to blink them on a kernel panic, even if they are set to
> + a different trigger.
> If unsure, say Y.
>
> endif # LEDS_TRIGGERS
> diff --git a/drivers/leds/trigger/ b/drivers/leds/trigger/ledtrig-panic.c
> index 627b350c5ec3..3e447bd2064a 100644
> --- a/drivers/leds/trigger/ledtrig-panic.c
> +++ b/drivers/leds/trigger/ledtrig-panic.c
> @@ -11,10 +11,54 @@
>
> #include <linux/kernel.h>
> #include <linux/init.h>
> +#include <linux/notifier.h>
> #include <linux/leds.h>
> +#include "../leds.h"
>
> static struct led_trigger *trigger;
>
> +/*
> + * This is a called in a special context by the atomic panic
s/is a/is/
> + * notifier. This means the trigger can be changed without
> + * worrying about locking.
> + */
> +static void led_trigger_set_panic(struct led_classdev *led_cdev)
> +{
> + struct led_trigger *trig;
> +
> + list_for_each_entry(trig, &trigger_list, next_trig) {
> + if (strcmp("panic", trig->name))
> + continue;
> + if (led_cdev->trigger)
> + list_del(&led_cdev->trig_list);
> + list_add_tail(&led_cdev->trig_list, &trig->led_cdevs);
> +
> + /* Avoid the delayed blink path */
> + led_cdev->blink_delay_on = 0;
> + led_cdev->blink_delay_off = 0;
> +
> + led_cdev->trigger = trig;
> + if (trig->activate)
> + trig->activate(led_cdev);
> + break;
> + }
> +}
> +
> +static int led_trigger_panic_notifier(struct notifier_block *nb,
> + unsigned long code, void *unused)
> +{
> + struct led_classdev *led_cdev;
> +
> + list_for_each_entry(led_cdev, &leds_list, node)
> + if (led_cdev->flags & LED_PANIC_INDICATOR)
> + led_trigger_set_panic(led_cdev);
> + return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block led_trigger_panic_nb = {
> + .notifier_call = led_trigger_panic_notifier,
> +};
> +
> static long led_panic_blink(int state)
> {
> led_trigger_event(trigger, state ? LED_FULL : LED_OFF);
> @@ -23,6 +67,9 @@ static long led_panic_blink(int state)
>
> static int __init ledtrig_panic_init(void)
> {
> + atomic_notifier_chain_register(&panic_notifier_list,
> + &led_trigger_panic_nb);
> +
> led_trigger_register_simple("panic", &trigger);
> panic_blink = led_panic_blink;
> return 0;
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 19eb10278bea..7e9fb00e15e8 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -50,6 +50,7 @@ struct led_classdev {
> #define LED_SYSFS_DISABLE (1 << 22)
> #define LED_DEV_CAP_FLASH (1 << 23)
> #define LED_HW_PLUGGABLE (1 << 24)
> +#define LED_PANIC_INDICATOR (1 << 25)
>
> /* Set LED brightness level
> * Must not sleep. Use brightness_set_blocking for drivers
>
--
Best regards,
Jacek Anaszewski
On 29/04/16 00:03, Ezequiel Garcia wrote:
> As per commit 916fe619951f ("leds: trigger: Introduce a kernel
> panic LED trigger"), the kernel now supports a new LED trigger
> to hook on the panic blink.
>
> However, the only way of using this is to dedicate a LED device,
> making it rather useless.
>
> To overcome this limitation, the present series introduces the
> capability to switch the LED trigger of certain LED devices upon
> a kernel panic (using the panic notifier).
>
> The decision of which LEDs should be switched to the panic trigger
> is left to each LED device driver. As an example, a devicetree
> boolean property is introduced and used in the leds-gpio driver.
>
> The big change in this v3 is that I've moved the panic trigger
> switching away from the core code and it's now part of
> ledtrig-panic.c. Pavel, Jacek: How does it look?
>
> Changes from v2:
>
> * Added Rob's "panic-indicator" devicetree property Acked-by.
>
> * Fix typo, as pointed out by Robin Murphy.
>
> * Documented "panic-indicator" in bindings/leds/leds-gpio.txt.
>
> * Moved the panic trigger switching from the trigger core code,
> to ledtrig-panic.c.
>
> Changes from v1:
>
> * Dropped the led_trigger_event_nosleep API, and instead just
> clear the blink_delay_{on, off} when the panic is notified.
> This results in less changes.
>
> * Changed the flag to LED_PANIC_INDICATOR, as requested by Jacek.
>
> * Changed the firmware property name to "panic-indicator", as
> requested by Jacek.
>
> Ezequiel Garcia (3):
> leds: triggers: Allow to switch the trigger to "panic" on a kernel
> panic
> devicetree: leds: Introduce "panic-indicator" optional property
> leds: gpio: Support the "panic-indicator" firmware property
>
> Documentation/devicetree/bindings/leds/common.txt | 3 ++
> .../devicetree/bindings/leds/leds-gpio.txt | 2 +
> drivers/leds/led-triggers.c | 2 +-
> drivers/leds/leds-gpio.c | 4 ++
> drivers/leds/leds.h | 1 +
> drivers/leds/trigger/Kconfig | 3 ++
> drivers/leds/trigger/ledtrig-panic.c | 47 ++++++++++++++++++++++
> include/linux/leds.h | 2 +
> 8 files changed, 63 insertions(+), 1 deletion(-)
>
Looks good to me.
Reviewed-by: Matthias Brugger <[email protected]>