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

Jacek Anaszewski
2016-03-31T07:28:57Z

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