[PATCH 0/5] Extend the LED panic trigger

[PATCH 0/5] Extend the LED panic trigger

[Ezequiel Garcia]

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

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

[Ezequiel Garcia]

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 <ezequiel@vanguardiasur.com.ar>
---
 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

[PATCH 2/5] leds: triggers: Add a led_trigger_event_nosleep API

[Ezequiel Garcia]

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 <ezequiel@vanguardiasur.com.ar>
---
 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

[PATCH 3/5] leds: trigger: panic: Use led_trigger_event_nosleep

[Ezequiel Garcia]

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 <ezequiel@vanguardiasur.com.ar>
---
 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

[PATCH 4/5] devicetree: leds: Introduce "panic-blink" 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.

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

[PATCH 5/5] leds: gpio: Support the panic-blink firmware property

[Ezequiel Garcia]

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

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

[Jacek Anaszewski]

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 <ezequiel@vanguardiasur.com.ar>
> ---
>   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

Re: [PATCH 2/5] leds: triggers: Add a led_trigger_event_nosleep API

[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 <ezequiel@vanguardiasur.com.ar>
> ---
>   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

Re: [PATCH 4/5] devicetree: leds: Introduce "panic-blink" optional property

[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 <ezequiel@vanguardiasur.com.ar>
> ---
>   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

Re: [PATCH 2/5] leds: triggers: Add a led_trigger_event_nosleep API

[Ezequiel Garcia]

On 5 April 2016 at 18:36, Jacek Anaszewski <jacek.anaszewski@gmail.com> 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 <ezequiel@vanguardiasur.com.ar>
>> ---
>>   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
www.vanguardiasur.com.ar

Re: [PATCH 4/5] devicetree: leds: Introduce "panic-blink" optional property

[Ezequiel Garcia]

On 5 April 2016 at 18:37, Jacek Anaszewski <jacek.anaszewski@gmail.com> 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 <ezequiel@vanguardiasur.com.ar>
>> ---
>>   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
www.vanguardiasur.com.ar

Re: [PATCH 2/5] leds: triggers: Add a led_trigger_event_nosleep API

[Jacek Anaszewski]

On 04/06/2016 06:38 AM, Ezequiel Garcia wrote:
> On 5 April 2016 at 18:36, Jacek Anaszewski <jacek.anaszewski@gmail.com> 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 <ezequiel@vanguardiasur.com.ar>
>>> ---
>>>    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

Re: [PATCH 4/5] devicetree: leds: Introduce "panic-blink" optional property

[Rob Herring]

On Mon, Apr 4, 2016 at 3:22 PM, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> 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 <ezequiel@vanguardiasur.com.ar>
> ---
>  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

Re: [PATCH 4/5] devicetree: leds: Introduce "panic-blink" optional property

[Ezequiel Garcia]

On 6 April 2016 at 12:12, Rob Herring <robh@kernel.org> wrote:
> On Mon, Apr 4, 2016 at 3:22 PM, Ezequiel Garcia
> <ezequiel@vanguardiasur.com.ar> 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 <ezequiel@vanguardiasur.com.ar>
>> ---
>>  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
www.vanguardiasur.com.ar

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

[Pavel Machek]

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 <ezequiel@vanguardiasur.com.ar>


>  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

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

[Pavel Machek]

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 <ezequiel@vanguardiasur.com.ar>
> 
> 
> >  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

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

[Jacek Anaszewski]

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 <ezequiel@vanguardiasur.com.ar>
>>
>>
>>>   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

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

[Ezequiel Garcia]

On 25 April 2016 at 03:56, Jacek Anaszewski <j.anaszewski@samsung.com> 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 <ezequiel@vanguardiasur.com.ar>
>>>
>>>
>>>
>>>>   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
www.vanguardiasur.com.ar

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

[Jacek Anaszewski]

Hi Ezequiel,

On 04/25/2016 06:27 PM, Ezequiel Garcia wrote:
> On 25 April 2016 at 03:56, Jacek Anaszewski <j.anaszewski@samsung.com> 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 <ezequiel@vanguardiasur.com.ar>
>>>>
>>>>
>>>>
>>>>>    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

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

[Ezequiel Garcia]

On 26 April 2016 at 04:15, Jacek Anaszewski <j.anaszewski@samsung.com> wrote:
> Hi Ezequiel,
>
> On 04/25/2016 06:27 PM, Ezequiel Garcia wrote:
>>
>> On 25 April 2016 at 03:56, Jacek Anaszewski <j.anaszewski@samsung.com>
>> 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 <ezequiel@vanguardiasur.com.ar>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>    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
www.vanguardiasur.com.ar

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

[Pavel Machek]

On Mon 2016-04-25 08:56:46, Jacek Anaszewski 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 <ezequiel@vanguardiasur.com.ar>
> >>
> >>
> >>>  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.

Dunno. Perhaps we can have something like struct led *panic_led, which
architecture can set, and then some helper that would blink the panic
led...? Should be smaller than this and will only be compiled on
platforms where it makes sense. On x86 we already have such support...

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

drivers/input/serio/i8042.c: * i8042_panic_blink() will turn the keyboard LEDs on or off and is called

			       			   	     	 	  Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html