2016-04-13 18:08:42

by Ezequiel Garcia

[permalink] [raw]
Subject: [PATCH v2 0/3] Extend the LED panic trigger

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


2016-04-13 18:08:46

by Ezequiel Garcia

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

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

2016-04-13 18:08:52

by Ezequiel Garcia

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

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 <[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..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

2016-04-13 18:09:08

by Ezequiel Garcia

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

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

2016-04-14 08:57:24

by Jacek Anaszewski

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

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 <[email protected]>
> ---
> 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;
>

2016-04-14 16:34:05

by Rob Herring (Arm)

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

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 <[email protected]>
> ---
> Documentation/devicetree/bindings/leds/common.txt | 3 +++
> 1 file changed, 3 insertions(+)

Acked-by: Rob Herring <[email protected]>

2016-04-14 16:46:55

by Robin Murphy

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

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 <[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..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
>

2016-04-19 16:18:40

by Ezequiel Garcia

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

Hi Jacek,

On 14 April 2016 at 05:57, Jacek Anaszewski <[email protected]> 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
http://www.vanguardiasur.com.ar