Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1385787 > unrolled thread

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

Started byPavel Machek <pavel@ucw.cz>
First post2016-04-24 11:30 +0200
Last post2016-04-27 22:20 +0200
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/5] leds: triggers: Allow to switch the trigger to  "panic" on a kernel panic Pavel Machek <pavel@ucw.cz> - 2016-04-24 11:30 +0200
    Re: [PATCH 1/5] leds: triggers: Allow to switch the trigger to "panic"  on a kernel panic Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-04-25 09:00 +0200
      Re: [PATCH 1/5] leds: triggers: Allow to switch the trigger to  "panic" on a kernel panic Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> - 2016-04-25 18:30 +0200
        Re: [PATCH 1/5] leds: triggers: Allow to switch the trigger to "panic"  on a kernel panic Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-04-26 09:20 +0200
          Re: [PATCH 1/5] leds: triggers: Allow to switch the trigger to  "panic" on a kernel panic Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> - 2016-04-27 22:20 +0200

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

FromPavel Machek <pavel@ucw.cz>
Date2016-04-24 11:30 +0200
SubjectRe: [PATCH 1/5] leds: triggers: Allow to switch the trigger to "panic" on a kernel panic
Message-ID<rrowx-26r-5@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

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

FromJacek Anaszewski <j.anaszewski@samsung.com>
Date2016-04-25 09:00 +0200
SubjectRe: [PATCH 1/5] leds: triggers: Allow to switch the trigger to "panic" on a kernel panic
Message-ID<rrIEW-1iI-7@gated-at.bofh.it>
In reply to#1385787
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

[toc] | [prev] | [next] | [standalone]

#1386648

FromEzequiel Garcia <ezequiel@vanguardiasur.com.ar>
Date2016-04-25 18:30 +0200
Message-ID<rrRyA-t4-59@gated-at.bofh.it>
In reply to#1386047
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

[toc] | [prev] | [next] | [standalone]

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

FromJacek Anaszewski <j.anaszewski@samsung.com>
Date2016-04-26 09:20 +0200
SubjectRe: [PATCH 1/5] leds: triggers: Allow to switch the trigger to "panic" on a kernel panic
Message-ID<rs5rQ-3pb-9@gated-at.bofh.it>
In reply to#1386648
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

[toc] | [prev] | [next] | [standalone]

#1389566

FromEzequiel Garcia <ezequiel@vanguardiasur.com.ar>
Date2016-04-27 22:20 +0200
Message-ID<rsE6f-7r0-13@gated-at.bofh.it>
In reply to#1387182
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web