Re: [PATCH 1/3] leds: trigger: Introduce a kernel panic LED trigger

Re: [PATCH 1/3] leds: trigger: Introduce a kernel panic LED trigger

[Jacek Anaszewski]

Hi Ezequiel,

On 03/30/2016 09:11 PM, Ezequiel Garcia wrote:
> +lkml
>
> On 30 Mar 11:29 AM, Jacek Anaszewski wrote:
>> Hi Ezequiel,
>>
>> Thanks for the patch. I've tested it on exynos4412-trats2 board
>> with leds-aat1290 driver, by executing:
>>
>> echo "c" > /proc/sysrq-trigger
>>
>> I was able to notice the blinking then.
>>
>> Applied to the for-next branch of linux-leds.git.
>>
>
> Notice that we currently need LEDs to be dedicated
> to the panic trigger, which is pretty lame as LEDs
> are scarce and are most likely assigned to something else.
>
> So, here's a toy patch to switch all the installed LEDs
> to the panic trigger on kernel panic. Patch is half-baked,
> but it shows the idea.
>
> (Interestingly, it also blinks the LEDs on a USB keyboard!)
>
> Is there any value in polishing this and find a way upstream?

I like the idea, but I'd rather explicitly mark LEDs as panic
indicators.

How about adding a "kernel-panic-indicator" Device Tree property to
common LED bindings? LED class device could be registered for the
panic trigger on kernel panic notification only when the property is
present.

Adding Rob, Mark and devicetree list.

> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index aa84e5b37593..caaf6161a7ae 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -321,6 +321,20 @@ void devm_led_classdev_unregister(struct device *dev,
>   }
>   EXPORT_SYMBOL_GPL(devm_led_classdev_unregister);
>
> +static int led_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)
> +		led_trigger_set_at_panic(led_cdev, "panic");
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block led_panic_nb = {
> +	.notifier_call = led_panic_notifier,
> +};
> +
>   static int __init leds_init(void)
>   {
>   	leds_class = class_create(THIS_MODULE, "leds");
> @@ -328,6 +342,8 @@ static int __init leds_init(void)
>   		return PTR_ERR(leds_class);
>   	leds_class->pm = &leds_class_dev_pm_ops;
>   	leds_class->dev_groups = led_groups;
> +	atomic_notifier_chain_register(&panic_notifier_list,
> +				       &led_panic_nb);
>   	return 0;
>   }
>
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 2181581795d3..8c1d33acdfa8 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -148,6 +148,21 @@ void led_trigger_remove(struct led_classdev *led_cdev)
>   }
>   EXPORT_SYMBOL_GPL(led_trigger_remove);
>
> +void led_trigger_set_at_panic(struct led_classdev *led_cdev, const char *name)
> +{

"name" parameter is not required, since setting other trigger than panic
on kernel panic doesn't make sense.

> +	struct led_trigger *trig;
> +
> +	list_for_each_entry(trig, &trigger_list, next_trig) {
> +		if (strcmp(name, trig->name))
> +			continue;
> +		list_add_tail(&led_cdev->trig_list, &trig->led_cdevs);
> +		led_cdev->trigger = trig;
> +		if (trig->activate)
> +			trig->activate(led_cdev);
> +		break;
> +	}
> +}
> +
>   void led_trigger_set_default(struct led_classdev *led_cdev)
>   {
>   	struct led_trigger *trig;
> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> index db3f20da7221..8cfa10f626a6 100644
> --- a/drivers/leds/leds.h
> +++ b/drivers/leds/leds.h
> @@ -21,6 +21,7 @@ static inline int led_get_brightness(struct led_classdev *led_cdev)
>   	return led_cdev->brightness;
>   }
>
> +void led_trigger_set_at_panic(struct led_classdev *led_cdev, const char *name);
>   void led_init_core(struct led_classdev *led_cdev);
>   void led_stop_software_blink(struct led_classdev *led_cdev);
>   void led_set_brightness_nopm(struct led_classdev *led_cdev,
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 1/3] leds: trigger: Introduce a kernel panic LED trigger

[Jacek Anaszewski]

On 03/31/2016 09:04 AM, Jacek Anaszewski wrote:
> Hi Ezequiel,
>
> On 03/30/2016 09:11 PM, Ezequiel Garcia wrote:
>> +lkml
>>
>> On 30 Mar 11:29 AM, Jacek Anaszewski wrote:
>>> Hi Ezequiel,
>>>
>>> Thanks for the patch. I've tested it on exynos4412-trats2 board
>>> with leds-aat1290 driver, by executing:
>>>
>>> echo "c" > /proc/sysrq-trigger
>>>
>>> I was able to notice the blinking then.
>>>
>>> Applied to the for-next branch of linux-leds.git.
>>>
>>
>> Notice that we currently need LEDs to be dedicated
>> to the panic trigger, which is pretty lame as LEDs
>> are scarce and are most likely assigned to something else.
>>
>> So, here's a toy patch to switch all the installed LEDs
>> to the panic trigger on kernel panic. Patch is half-baked,
>> but it shows the idea.
>>
>> (Interestingly, it also blinks the LEDs on a USB keyboard!)
>>
>> Is there any value in polishing this and find a way upstream?
>
> I like the idea, but I'd rather explicitly mark LEDs as panic
> indicators.
>
> How about adding a "kernel-panic-indicator" Device Tree property to
> common LED bindings? LED class device could be registered for the
> panic trigger on kernel panic notification only when the property is
> present.

Of course we would need similar solution for ACPI based platforms,
however I am not sure if this approach is feasible in that case, since
device configuration data is enclosed in firmware then.
Adding linux-acpi list.

> Adding Rob, Mark and devicetree list.
>
>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>> index aa84e5b37593..caaf6161a7ae 100644
>> --- a/drivers/leds/led-class.c
>> +++ b/drivers/leds/led-class.c
>> @@ -321,6 +321,20 @@ void devm_led_classdev_unregister(struct device
>> *dev,
>>   }
>>   EXPORT_SYMBOL_GPL(devm_led_classdev_unregister);
>>
>> +static int led_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)
>> +        led_trigger_set_at_panic(led_cdev, "panic");
>> +    return NOTIFY_DONE;
>> +}
>> +
>> +static struct notifier_block led_panic_nb = {
>> +    .notifier_call = led_panic_notifier,
>> +};
>> +
>>   static int __init leds_init(void)
>>   {
>>       leds_class = class_create(THIS_MODULE, "leds");
>> @@ -328,6 +342,8 @@ static int __init leds_init(void)
>>           return PTR_ERR(leds_class);
>>       leds_class->pm = &leds_class_dev_pm_ops;
>>       leds_class->dev_groups = led_groups;
>> +    atomic_notifier_chain_register(&panic_notifier_list,
>> +                       &led_panic_nb);
>>       return 0;
>>   }
>>
>> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
>> index 2181581795d3..8c1d33acdfa8 100644
>> --- a/drivers/leds/led-triggers.c
>> +++ b/drivers/leds/led-triggers.c
>> @@ -148,6 +148,21 @@ void led_trigger_remove(struct led_classdev
>> *led_cdev)
>>   }
>>   EXPORT_SYMBOL_GPL(led_trigger_remove);
>>
>> +void led_trigger_set_at_panic(struct led_classdev *led_cdev, const
>> char *name)
>> +{
>
> "name" parameter is not required, since setting other trigger than panic
> on kernel panic doesn't make sense.
>
>> +    struct led_trigger *trig;
>> +
>> +    list_for_each_entry(trig, &trigger_list, next_trig) {
>> +        if (strcmp(name, trig->name))
>> +            continue;
>> +        list_add_tail(&led_cdev->trig_list, &trig->led_cdevs);
>> +        led_cdev->trigger = trig;
>> +        if (trig->activate)
>> +            trig->activate(led_cdev);
>> +        break;
>> +    }
>> +}
>> +
>>   void led_trigger_set_default(struct led_classdev *led_cdev)
>>   {
>>       struct led_trigger *trig;
>> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
>> index db3f20da7221..8cfa10f626a6 100644
>> --- a/drivers/leds/leds.h
>> +++ b/drivers/leds/leds.h
>> @@ -21,6 +21,7 @@ static inline int led_get_brightness(struct
>> led_classdev *led_cdev)
>>       return led_cdev->brightness;
>>   }
>>
>> +void led_trigger_set_at_panic(struct led_classdev *led_cdev, const
>> char *name);
>>   void led_init_core(struct led_classdev *led_cdev);
>>   void led_stop_software_blink(struct led_classdev *led_cdev);
>>   void led_set_brightness_nopm(struct led_classdev *led_cdev,
>>
>
>


-- 
Best regards,
Jacek Anaszewski