[PATCH v2 0/3] Extend the LED panic trigger

[PATCH v2 0/3] Extend the LED panic trigger

[Ezequiel Garcia]

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
to this function.

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.

Feedback and other ideas on how to implement this are most welcomed.

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 ++
 drivers/leds/led-triggers.c                       | 57 +++++++++++++++++++++++
 drivers/leds/leds-gpio.c                          |  4 ++
 include/linux/leds.h                              |  2 +
 4 files changed, 66 insertions(+)

-- 
2.7.0

[PATCH v2 1/3] leds: triggers: Allow to switch the trigger to "panic" on a kernel panic

[Ezequiel Garcia]

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 <ezequiel@vanguardiasur.com.ar>
---
 drivers/leds/led-triggers.c | 57 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/leds.h        |  1 +
 2 files changed, 58 insertions(+)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 2181581795d3..d27020daf711 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -148,6 +148,48 @@ void led_trigger_remove(struct led_classdev *led_cdev)
 }
 EXPORT_SYMBOL_GPL(led_trigger_remove);
 
+/*
+ * 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,
+};
+
 void led_trigger_set_default(struct led_classdev *led_cdev)
 {
 	struct led_trigger *trig;
@@ -356,6 +398,21 @@ void led_trigger_unregister_simple(struct led_trigger *trig)
 }
 EXPORT_SYMBOL_GPL(led_trigger_unregister_simple);
 
+static int __init leds_trigger_init(void)
+{
+	atomic_notifier_chain_register(&panic_notifier_list,
+				       &led_trigger_panic_nb);
+	return 0;
+}
+
+static void __exit leds_trigger_exit(void)
+{
+	atomic_notifier_chain_unregister(&panic_notifier_list,
+					 &led_trigger_panic_nb);
+}
+module_init(leds_trigger_init);
+module_exit(leds_trigger_exit);
+
 MODULE_AUTHOR("Richard Purdie");
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("LED Triggers Core");
diff --git a/include/linux/leds.h b/include/linux/leds.h
index f203a8f89d30..49adf9c6e326 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

[PATCH v2 2/3] devicetree: leds: Introduce "panic-indicator" optional property

[Ezequiel Garcia]

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.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 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..7b646a7808ce 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 properties 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

[PATCH v2 3/3] leds: gpio: Support the "panic-indicator" firmware property

[Ezequiel Garcia]

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 <ezequiel@vanguardiasur.com.ar>
---
 drivers/leds/leds-gpio.c | 4 ++++
 include/linux/leds.h     | 1 +
 2 files changed, 5 insertions(+)

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 49adf9c6e326..1067fb5f9296 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -359,6 +359,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

Re: [PATCH v2 3/3] leds: gpio: Support the "panic-indicator" firmware property

[Jacek Anaszewski]

Hi Ezequiel,

It would be good to update also leds-gpio bindings,
of course in a separate patch:

Documentation/devicetree/bindings/leds/leds-gpio.txt

Thanks,
Jacek Anaszewski

On 04/13/2016 08:08 PM, Ezequiel Garcia wrote:
> 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 <ezequiel@vanguardiasur.com.ar>
> ---
>   drivers/leds/leds-gpio.c | 4 ++++
>   include/linux/leds.h     | 1 +
>   2 files changed, 5 insertions(+)
>
> 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 49adf9c6e326..1067fb5f9296 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -359,6 +359,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;
>

Re: [PATCH v2 2/3] devicetree: leds: Introduce "panic-indicator" optional property

[Rob Herring]

On Wed, Apr 13, 2016 at 03:08:18PM -0300, Ezequiel Garcia wrote:
> 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.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
>  Documentation/devicetree/bindings/leds/common.txt | 3 +++
>  1 file changed, 3 insertions(+)

Acked-by: Rob Herring <rob@kernel.org>

Re: [PATCH v2 2/3] devicetree: leds: Introduce "panic-indicator" optional property

[Robin Murphy]

Hi Ezequiel,

On 13/04/16 19:08, Ezequiel Garcia wrote:
> 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.
>
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
>   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..7b646a7808ce 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 properties specifies that the LED should be used,

s/properties/property/

Maybe phrasing it as "should also be used" might make the intention a 
bit clearer, as well.

Robin.

> +		    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
>

Re: [PATCH v2 3/3] leds: gpio: Support the "panic-indicator" firmware property

[Ezequiel Garcia]

Hi Jacek,

On 14 April 2016 at 05:57, Jacek Anaszewski <j.anaszewski@samsung.com> wrote:
> Hi Ezequiel,
>
> It would be good to update also leds-gpio bindings,
> of course in a separate patch:
>
> Documentation/devicetree/bindings/leds/leds-gpio.txt
>

Yes, you are right. I will send a new series adding this.

Thanks,
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar