As per commit 916fe61995 ("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.
Ezequiel Garcia (5):
leds: triggers: Allow to switch the trigger to "panic" on a kernel
panic
leds: triggers: Add a led_trigger_event_nosleep API
leds: trigger: panic: Use led_trigger_event_nosleep
devicetree: leds: Introduce "panic-blink" optional property
leds: gpio: Support the panic-blink firmware property
Documentation/devicetree/bindings/leds/common.txt | 2 +
drivers/leds/led-triggers.c | 78 +++++++++++++++++++++--
drivers/leds/leds-gpio.c | 4 ++
drivers/leds/trigger/ledtrig-panic.c | 2 +-
include/linux/leds.h | 6 ++
5 files changed, 87 insertions(+), 5 deletions(-)
--
2.7.0
Now that we can mark any LED (even those in use by delayed blink
triggers) to trigger on a kernel panic, let's introduce a nosleep
led_trigger_event API.
This API is needed to skip the delayed blink path on
led_trigger_event. LEDs that are switched on a kernel panic,
might be in use by a delayed blink trigger.
This will be used by the panic LED trigger.
Signed-off-by: Ezequiel Garcia <[email protected]>
---
drivers/leds/led-triggers.c | 26 ++++++++++++++++++++++----
include/linux/leds.h | 4 ++++
2 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index f5c9d7c4d181..00b9d8497777 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -307,8 +307,9 @@ EXPORT_SYMBOL_GPL(devm_led_trigger_register);
/* Simple LED Tigger Interface */
-void led_trigger_event(struct led_trigger *trig,
- enum led_brightness brightness)
+static void do_led_trigger_event(struct led_trigger *trig,
+ enum led_brightness brightness,
+ bool nosleep)
{
struct led_classdev *led_cdev;
@@ -316,12 +317,29 @@ void led_trigger_event(struct led_trigger *trig,
return;
read_lock(&trig->leddev_list_lock);
- list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
- led_set_brightness(led_cdev, brightness);
+ list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list) {
+ if (nosleep)
+ led_set_brightness_nosleep(led_cdev, brightness);
+ else
+ led_set_brightness(led_cdev, brightness);
+ }
read_unlock(&trig->leddev_list_lock);
}
+
+void led_trigger_event(struct led_trigger *trig,
+ enum led_brightness brightness)
+{
+ do_led_trigger_event(trig, brightness, false);
+}
EXPORT_SYMBOL_GPL(led_trigger_event);
+void led_trigger_event_nosleep(struct led_trigger *trig,
+ enum led_brightness brightness)
+{
+ do_led_trigger_event(trig, brightness, true);
+}
+EXPORT_SYMBOL_GPL(led_trigger_event_nosleep);
+
static void led_trigger_blink_setup(struct led_trigger *trig,
unsigned long *delay_on,
unsigned long *delay_off,
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 7f1428bb1e69..d33b230ce66d 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -259,6 +259,8 @@ extern void led_trigger_register_simple(const char *name,
extern void led_trigger_unregister_simple(struct led_trigger *trigger);
extern void led_trigger_event(struct led_trigger *trigger,
enum led_brightness event);
+extern void led_trigger_event_nosleep(struct led_trigger *trigger,
+ enum led_brightness event);
extern void led_trigger_blink(struct led_trigger *trigger,
unsigned long *delay_on,
unsigned long *delay_off);
@@ -305,6 +307,8 @@ static inline void led_trigger_register_simple(const char *name,
static inline void led_trigger_unregister_simple(struct led_trigger *trigger) {}
static inline void led_trigger_event(struct led_trigger *trigger,
enum led_brightness event) {}
+static inline void led_trigger_event_nosleep(struct led_trigger *trigger,
+ enum led_brightness event) {}
static inline void led_trigger_blink(struct led_trigger *trigger,
unsigned long *delay_on,
unsigned long *delay_off) {}
--
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.
Signed-off-by: Ezequiel Garcia <[email protected]>
---
Documentation/devicetree/bindings/leds/common.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
index 68419843e32f..dd409df9203a 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -37,6 +37,8 @@ Optional properties for child nodes:
property is mandatory for the LEDs in the non-flash modes
(e.g. torch or indicator).
+- panic-blink : Mark this LED to be used by the panic LED trigger.
+
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
Calling led_trigger_event may schedule a delayed LED set,
if the LED was being used by a delayed blink trigger when
the kernel paniced.
Therefore, we must use led_trigger_event_nosleep to override
this situation, and set the LED unconditionally.
Signed-off-by: Ezequiel Garcia <[email protected]>
---
drivers/leds/trigger/ledtrig-panic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/trigger/ledtrig-panic.c b/drivers/leds/trigger/ledtrig-panic.c
index 627b350c5ec3..145c238d55f2 100644
--- a/drivers/leds/trigger/ledtrig-panic.c
+++ b/drivers/leds/trigger/ledtrig-panic.c
@@ -17,7 +17,7 @@ static struct led_trigger *trigger;
static long led_panic_blink(int state)
{
- led_trigger_event(trigger, state ? LED_FULL : LED_OFF);
+ led_trigger_event_nosleep(trigger, state ? LED_FULL : LED_OFF);
return 0;
}
--
2.7.0
GPIO LEDs are relatively cheap, and so are ideal to blink on
a kernel panic. This commit adds support for the new "panic-blink"
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..74e35dcaf874 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_blink)
+ led_dat->cdev.flags |= LED_BLINK_AT_PANIC;
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-blink"))
+ led.panic_blink = 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 d33b230ce66d..e8e99cc5f99e 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -363,6 +363,7 @@ struct gpio_led {
unsigned gpio;
unsigned active_low : 1;
unsigned retain_state_suspended : 1;
+ unsigned panic_blink : 1;
unsigned default_state : 2;
/* default_state should be one of LEDS_GPIO_DEFSTATE_(ON|OFF|KEEP) */
struct gpio_desc *gpiod;
--
2.7.0
This commit adds a new led_cdev flag LED_BLINK_AT_PANIC, 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 | 52 +++++++++++++++++++++++++++++++++++++++++++++
include/linux/leds.h | 1 +
2 files changed, 53 insertions(+)
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 2181581795d3..f5c9d7c4d181 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -148,6 +148,43 @@ 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);
+ 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_BLINK_AT_PANIC)
+ 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 +393,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..7f1428bb1e69 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_BLINK_AT_PANIC (1 << 25)
/* Set LED brightness level
* Must not sleep. Use brightness_set_blocking for drivers
--
2.7.0
Hi Ezequiel,
Thanks for the patch. I have one comment below.
On 04/04/2016 10:22 PM, Ezequiel Garcia wrote:
> This commit adds a new led_cdev flag LED_BLINK_AT_PANIC, 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 | 52 +++++++++++++++++++++++++++++++++++++++++++++
> include/linux/leds.h | 1 +
> 2 files changed, 53 insertions(+)
>
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 2181581795d3..f5c9d7c4d181 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -148,6 +148,43 @@ 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);
> + 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_BLINK_AT_PANIC)
> + 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 +393,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..7f1428bb1e69 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_BLINK_AT_PANIC (1 << 25)
LED_BLINK prefixed macros control LED subsystem inherent
blinking feature.
Would you mind renaming it to LED_PANIC_INDICATOR?
>
> /* Set LED brightness level
> * Must not sleep. Use brightness_set_blocking for drivers
>
--
Best regards,
Jacek Anaszewski
Hi Ezequiel,
On 04/04/2016 10:22 PM, Ezequiel Garcia wrote:
> Now that we can mark any LED (even those in use by delayed blink
> triggers) to trigger on a kernel panic, let's introduce a nosleep
> led_trigger_event API.
>
> This API is needed to skip the delayed blink path on
> led_trigger_event. LEDs that are switched on a kernel panic,
> might be in use by a delayed blink trigger.
>
> This will be used by the panic LED trigger.
>
> Signed-off-by: Ezequiel Garcia <[email protected]>
> ---
> drivers/leds/led-triggers.c | 26 ++++++++++++++++++++++----
> include/linux/leds.h | 4 ++++
> 2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index f5c9d7c4d181..00b9d8497777 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -307,8 +307,9 @@ EXPORT_SYMBOL_GPL(devm_led_trigger_register);
>
> /* Simple LED Tigger Interface */
>
> -void led_trigger_event(struct led_trigger *trig,
> - enum led_brightness brightness)
> +static void do_led_trigger_event(struct led_trigger *trig,
> + enum led_brightness brightness,
> + bool nosleep)
> {
> struct led_classdev *led_cdev;
>
> @@ -316,12 +317,29 @@ void led_trigger_event(struct led_trigger *trig,
> return;
>
> read_lock(&trig->leddev_list_lock);
> - list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
> - led_set_brightness(led_cdev, brightness);
led_set_brightness() can gently disable blinking if passed 0 in the
brightness argument. IMHO this patch is not needed then.
> + list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list) {
> + if (nosleep)
> + led_set_brightness_nosleep(led_cdev, brightness);
> + else
> + led_set_brightness(led_cdev, brightness);
> + }
> read_unlock(&trig->leddev_list_lock);
> }
> +
> +void led_trigger_event(struct led_trigger *trig,
> + enum led_brightness brightness)
> +{
> + do_led_trigger_event(trig, brightness, false);
> +}
> EXPORT_SYMBOL_GPL(led_trigger_event);
>
> +void led_trigger_event_nosleep(struct led_trigger *trig,
> + enum led_brightness brightness)
> +{
> + do_led_trigger_event(trig, brightness, true);
> +}
> +EXPORT_SYMBOL_GPL(led_trigger_event_nosleep);
> +
> static void led_trigger_blink_setup(struct led_trigger *trig,
> unsigned long *delay_on,
> unsigned long *delay_off,
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 7f1428bb1e69..d33b230ce66d 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -259,6 +259,8 @@ extern void led_trigger_register_simple(const char *name,
> extern void led_trigger_unregister_simple(struct led_trigger *trigger);
> extern void led_trigger_event(struct led_trigger *trigger,
> enum led_brightness event);
> +extern void led_trigger_event_nosleep(struct led_trigger *trigger,
> + enum led_brightness event);
> extern void led_trigger_blink(struct led_trigger *trigger,
> unsigned long *delay_on,
> unsigned long *delay_off);
> @@ -305,6 +307,8 @@ static inline void led_trigger_register_simple(const char *name,
> static inline void led_trigger_unregister_simple(struct led_trigger *trigger) {}
> static inline void led_trigger_event(struct led_trigger *trigger,
> enum led_brightness event) {}
> +static inline void led_trigger_event_nosleep(struct led_trigger *trigger,
> + enum led_brightness event) {}
> static inline void led_trigger_blink(struct led_trigger *trigger,
> unsigned long *delay_on,
> unsigned long *delay_off) {}
>
--
Best regards,
Jacek Anaszewski
Hi Ezequiel,
On 04/04/2016 10:22 PM, 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.
>
> Signed-off-by: Ezequiel Garcia <[email protected]>
> ---
> Documentation/devicetree/bindings/leds/common.txt | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> index 68419843e32f..dd409df9203a 100644
> --- a/Documentation/devicetree/bindings/leds/common.txt
> +++ b/Documentation/devicetree/bindings/leds/common.txt
> @@ -37,6 +37,8 @@ Optional properties for child nodes:
> property is mandatory for the LEDs in the non-flash modes
> (e.g. torch or indicator).
>
> +- panic-blink : Mark this LED to be used by the panic LED trigger.
I'd say that 'panic-indicator' would be more informative.
Please next time cc also devicetree list and maintainers.
> +
> 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
>
--
Best regards,
Jacek Anaszewski
On 5 April 2016 at 18:36, Jacek Anaszewski <[email protected]> wrote:
> Hi Ezequiel,
>
>
> On 04/04/2016 10:22 PM, Ezequiel Garcia wrote:
>>
>> Now that we can mark any LED (even those in use by delayed blink
>> triggers) to trigger on a kernel panic, let's introduce a nosleep
>> led_trigger_event API.
>>
>> This API is needed to skip the delayed blink path on
>> led_trigger_event. LEDs that are switched on a kernel panic,
>> might be in use by a delayed blink trigger.
>>
>> This will be used by the panic LED trigger.
>>
>> Signed-off-by: Ezequiel Garcia <[email protected]>
>> ---
>> drivers/leds/led-triggers.c | 26 ++++++++++++++++++++++----
>> include/linux/leds.h | 4 ++++
>> 2 files changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
>> index f5c9d7c4d181..00b9d8497777 100644
>> --- a/drivers/leds/led-triggers.c
>> +++ b/drivers/leds/led-triggers.c
>> @@ -307,8 +307,9 @@ EXPORT_SYMBOL_GPL(devm_led_trigger_register);
>>
>> /* Simple LED Tigger Interface */
>>
>> -void led_trigger_event(struct led_trigger *trig,
>> - enum led_brightness brightness)
>> +static void do_led_trigger_event(struct led_trigger *trig,
>> + enum led_brightness brightness,
>> + bool nosleep)
>> {
>> struct led_classdev *led_cdev;
>>
>> @@ -316,12 +317,29 @@ void led_trigger_event(struct led_trigger *trig,
>> return;
>>
>> read_lock(&trig->leddev_list_lock);
>> - list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
>> - led_set_brightness(led_cdev, brightness);
>
>
> led_set_brightness() can gently disable blinking if passed 0 in the
> brightness argument. IMHO this patch is not needed then.
>
>
Yes, but the blinking disable is deferred, and so might never
run (e.g. I'd say it won't run on a non-preemptible kernel).
I think we need this API, or otherwise some way of circumventing
the deferred path on led_set_brightness. For instance, we
could turn off delay_on and delay_off in the panic atomic notifier.
I'm not strongly convinced by any approach, but this API seemed
slightly cleaner.
--
Ezequiel García, VanguardiaSur
http://www.vanguardiasur.com.ar
On 5 April 2016 at 18:37, Jacek Anaszewski <[email protected]> wrote:
> Hi Ezequiel,
>
> On 04/04/2016 10:22 PM, 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.
>>
>> Signed-off-by: Ezequiel Garcia <[email protected]>
>> ---
>> Documentation/devicetree/bindings/leds/common.txt | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/common.txt
>> b/Documentation/devicetree/bindings/leds/common.txt
>> index 68419843e32f..dd409df9203a 100644
>> --- a/Documentation/devicetree/bindings/leds/common.txt
>> +++ b/Documentation/devicetree/bindings/leds/common.txt
>> @@ -37,6 +37,8 @@ Optional properties for child nodes:
>> property is mandatory for the LEDs in the non-flash
>> modes
>> (e.g. torch or indicator).
>>
>> +- panic-blink : Mark this LED to be used by the panic LED trigger.
>
>
> I'd say that 'panic-indicator' would be more informative.
>
Sure, that looks much better.
> Please next time cc also devicetree list and maintainers.
>
Will do.
Thanks for the reviews!
--
Ezequiel García, VanguardiaSur
http://www.vanguardiasur.com.ar
On 04/06/2016 06:38 AM, Ezequiel Garcia wrote:
> On 5 April 2016 at 18:36, Jacek Anaszewski <[email protected]> wrote:
>> Hi Ezequiel,
>>
>>
>> On 04/04/2016 10:22 PM, Ezequiel Garcia wrote:
>>>
>>> Now that we can mark any LED (even those in use by delayed blink
>>> triggers) to trigger on a kernel panic, let's introduce a nosleep
>>> led_trigger_event API.
>>>
>>> This API is needed to skip the delayed blink path on
>>> led_trigger_event. LEDs that are switched on a kernel panic,
>>> might be in use by a delayed blink trigger.
>>>
>>> This will be used by the panic LED trigger.
>>>
>>> Signed-off-by: Ezequiel Garcia <[email protected]>
>>> ---
>>> drivers/leds/led-triggers.c | 26 ++++++++++++++++++++++----
>>> include/linux/leds.h | 4 ++++
>>> 2 files changed, 26 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
>>> index f5c9d7c4d181..00b9d8497777 100644
>>> --- a/drivers/leds/led-triggers.c
>>> +++ b/drivers/leds/led-triggers.c
>>> @@ -307,8 +307,9 @@ EXPORT_SYMBOL_GPL(devm_led_trigger_register);
>>>
>>> /* Simple LED Tigger Interface */
>>>
>>> -void led_trigger_event(struct led_trigger *trig,
>>> - enum led_brightness brightness)
>>> +static void do_led_trigger_event(struct led_trigger *trig,
>>> + enum led_brightness brightness,
>>> + bool nosleep)
>>> {
>>> struct led_classdev *led_cdev;
>>>
>>> @@ -316,12 +317,29 @@ void led_trigger_event(struct led_trigger *trig,
>>> return;
>>>
>>> read_lock(&trig->leddev_list_lock);
>>> - list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
>>> - led_set_brightness(led_cdev, brightness);
>>
>>
>> led_set_brightness() can gently disable blinking if passed 0 in the
>> brightness argument. IMHO this patch is not needed then.
>>
>>
>
> Yes, but the blinking disable is deferred, and so might never
> run (e.g. I'd say it won't run on a non-preemptible kernel).
>
> I think we need this API, or otherwise some way of circumventing
> the deferred path on led_set_brightness. For instance, we
> could turn off delay_on and delay_off in the panic atomic notifier.
Yes, I prefer the latter, as it requires less changes.
> I'm not strongly convinced by any approach, but this API seemed
> slightly cleaner.
>
--
Best regards,
Jacek Anaszewski
On Mon, Apr 4, 2016 at 3:22 PM, Ezequiel Garcia
<[email protected]> 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.
>
> Signed-off-by: Ezequiel Garcia <[email protected]>
> ---
> Documentation/devicetree/bindings/leds/common.txt | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> index 68419843e32f..dd409df9203a 100644
> --- a/Documentation/devicetree/bindings/leds/common.txt
> +++ b/Documentation/devicetree/bindings/leds/common.txt
> @@ -37,6 +37,8 @@ Optional properties for child nodes:
> property is mandatory for the LEDs in the non-flash modes
> (e.g. torch or indicator).
>
> +- panic-blink : Mark this LED to be used by the panic LED trigger.
> +
We already have a way to specify LED triggers. Why can't that be used?
Rob
On 6 April 2016 at 12:12, Rob Herring <[email protected]> wrote:
> On Mon, Apr 4, 2016 at 3:22 PM, Ezequiel Garcia
> <[email protected]> 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.
>>
>> Signed-off-by: Ezequiel Garcia <[email protected]>
>> ---
>> Documentation/devicetree/bindings/leds/common.txt | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
>> index 68419843e32f..dd409df9203a 100644
>> --- a/Documentation/devicetree/bindings/leds/common.txt
>> +++ b/Documentation/devicetree/bindings/leds/common.txt
>> @@ -37,6 +37,8 @@ Optional properties for child nodes:
>> property is mandatory for the LEDs in the non-flash modes
>> (e.g. torch or indicator).
>>
>> +- panic-blink : Mark this LED to be used by the panic LED trigger.
>> +
>
> We already have a way to specify LED triggers. Why can't that be used?
>
Because this is not about specifying a LED trigger.
The use case is the following: a LED is assigned to some LED trigger.
When the kernel panics, we want to switch that LED to the panic trigger,
so it blinks signalling the panic.
IOW, this allows a LED to be used for "normal operation stuff trigger",
but blink if the kernel panics.
--
Ezequiel García, VanguardiaSur
http://www.vanguardiasur.com.ar
On Mon 2016-04-04 17:22:02, Ezequiel Garcia wrote:
> This commit adds a new led_cdev flag LED_BLINK_AT_PANIC, 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 | 52 +++++++++++++++++++++++++++++++++++++++++++++
> include/linux/leds.h | 1 +
Could we get this out of the core? I'm pretty sure most users are not
interested...
Pavel
> +/*
> + * 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);
> + 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_BLINK_AT_PANIC)
> + 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 +393,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..7f1428bb1e69 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_BLINK_AT_PANIC (1 << 25)
>
> /* Set LED brightness level
> * Must not sleep. Use brightness_set_blocking for drivers
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sun 2016-04-24 11:25:51, Pavel Machek wrote:
> On Mon 2016-04-04 17:22:02, Ezequiel Garcia wrote:
> > This commit adds a new led_cdev flag LED_BLINK_AT_PANIC, 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 | 52 +++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/leds.h | 1 +
>
> Could we get this out of the core? I'm pretty sure most users are not
> interested...
Thinking about it some more....
This is not really a trigger. This is very special. Maybe hard-coded
handling, like we do for keyboard leds on x86 would be suitable?
>
> Pavel
>
> > +/*
> > + * 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);
> > + 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_BLINK_AT_PANIC)
> > + 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 +393,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..7f1428bb1e69 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_BLINK_AT_PANIC (1 << 25)
> >
> > /* Set LED brightness level
> > * Must not sleep. Use brightness_set_blocking for drivers
>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 04/24/2016 11:29 AM, Pavel Machek wrote:
> On Sun 2016-04-24 11:25:51, Pavel Machek wrote:
>> On Mon 2016-04-04 17:22:02, Ezequiel Garcia wrote:
>>> This commit adds a new led_cdev flag LED_BLINK_AT_PANIC, 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 | 52 +++++++++++++++++++++++++++++++++++++++++++++
>>> include/linux/leds.h | 1 +
>>
>> Could we get this out of the core? I'm pretty sure most users are not
>> interested...
Good point.
> Thinking about it some more....
>
> This is not really a trigger. This is very special. Maybe hard-coded
> handling, like we do for keyboard leds on x86 would be suitable?
Could you please spot the piece of code you have on mind?
>
>
>>
>> Pavel
>>
>>> +/*
>>> + * 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);
>>> + 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_BLINK_AT_PANIC)
>>> + 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 +393,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..7f1428bb1e69 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_BLINK_AT_PANIC (1 << 25)
>>>
>>> /* Set LED brightness level
>>> * Must not sleep. Use brightness_set_blocking for drivers
>>
>
--
Best regards,
Jacek Anaszewski
On 25 April 2016 at 03:56, Jacek Anaszewski <[email protected]> wrote:
> On 04/24/2016 11:29 AM, Pavel Machek wrote:
>>
>> On Sun 2016-04-24 11:25:51, Pavel Machek wrote:
>>>
>>> On Mon 2016-04-04 17:22:02, Ezequiel Garcia wrote:
>>>>
>>>> This commit adds a new led_cdev flag LED_BLINK_AT_PANIC, 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 | 52
>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>> include/linux/leds.h | 1 +
>>>
>>>
>>> Could we get this out of the core? I'm pretty sure most users are not
>>> interested...
>
>
> Good point.
>
Not sure how we can get it out of the core, and still implement it.
The goal is to run-time switch user-specified LEDs and blink them when
the kernel panics, and so it needs to mess up with some core private
structures.
Unless we don't want this feature at all.
--
Ezequiel García, VanguardiaSur
http://www.vanguardiasur.com.ar
Hi Ezequiel,
On 04/25/2016 06:27 PM, Ezequiel Garcia wrote:
> On 25 April 2016 at 03:56, Jacek Anaszewski <[email protected]> wrote:
>> On 04/24/2016 11:29 AM, Pavel Machek wrote:
>>>
>>> On Sun 2016-04-24 11:25:51, Pavel Machek wrote:
>>>>
>>>> On Mon 2016-04-04 17:22:02, Ezequiel Garcia wrote:
>>>>>
>>>>> This commit adds a new led_cdev flag LED_BLINK_AT_PANIC, 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 | 52
>>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>>> include/linux/leds.h | 1 +
>>>>
>>>>
>>>> Could we get this out of the core? I'm pretty sure most users are not
>>>> interested...
>>
>>
>> Good point.
>>
>
> Not sure how we can get it out of the core, and still implement it.
>
> The goal is to run-time switch user-specified LEDs and blink them when
> the kernel panics, and so it needs to mess up with some core private
> structures.
>
> Unless we don't want this feature at all.
>
It seems that the entire added code can be easily moved to a separate
module, let's say led-panic-notifier.c ? It should select
LEDS_TRIGGER_PANIC, as it would make no sense to have the former enabled
without the latter.
--
Best regards,
Jacek Anaszewski
On 26 April 2016 at 04:15, Jacek Anaszewski <[email protected]> wrote:
> Hi Ezequiel,
>
> On 04/25/2016 06:27 PM, Ezequiel Garcia wrote:
>>
>> On 25 April 2016 at 03:56, Jacek Anaszewski <[email protected]>
>> wrote:
>>>
>>> On 04/24/2016 11:29 AM, Pavel Machek wrote:
>>>>
>>>>
>>>> On Sun 2016-04-24 11:25:51, Pavel Machek wrote:
>>>>>
>>>>>
>>>>> On Mon 2016-04-04 17:22:02, Ezequiel Garcia wrote:
>>>>>>
>>>>>>
>>>>>> This commit adds a new led_cdev flag LED_BLINK_AT_PANIC, 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 | 52
>>>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>>>> include/linux/leds.h | 1 +
>>>>>
>>>>>
>>>>>
>>>>> Could we get this out of the core? I'm pretty sure most users are not
>>>>> interested...
>>>
>>>
>>>
>>> Good point.
>>>
>>
>> Not sure how we can get it out of the core, and still implement it.
>>
>> The goal is to run-time switch user-specified LEDs and blink them when
>> the kernel panics, and so it needs to mess up with some core private
>> structures.
>>
>> Unless we don't want this feature at all.
>>
>
> It seems that the entire added code can be easily moved to a separate
> module, let's say led-panic-notifier.c ? It should select
> LEDS_TRIGGER_PANIC, as it would make no sense to have the former enabled
> without the latter.
>
OK, let me try that.
--
Ezequiel García, VanguardiaSur
http://www.vanguardiasur.com.ar