[PATCH 1/2] leds: simply LED trigger list management

[PATCH 1/2] leds: simply LED trigger list management

[Kim, Milo]

 There are two list_heads for handling LED trigger function.
 'trig_list' of led_classdev and 'led_cdevs' of led_trigger.
 Those are added/removed with led_trigger_set().
 To find exact LED device, those are scanned in led_trigger_event() and
 led_trigger_blink_setup().

 But without additional lists, we can get LED device information.
 Here is a simple solution.
  "Scan LED list and find same trigger pointer as requested. Then we can
   get LED device from the LED device list."

 Changed list iteration: refer LED list and find the trigger.
   Just find same registered trigger pointer in each LED device.
   We can get LED device from registered LED device list.

 Removed list heads: 'trig_list' and 'led_cdevs'.
   Not used any more

Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
---
 drivers/leds/led-triggers.c |   27 ++++++++-------------------
 include/linux/leds.h        |    2 --
 2 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 3c972b2..5aa0252 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -101,7 +101,6 @@ EXPORT_SYMBOL_GPL(led_trigger_show);
 /* Caller must ensure led_cdev->trigger_lock held */
 void led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
 {
-	unsigned long flags;
 	char *event = NULL;
 	char *envp[2];
 	const char *name;
@@ -111,10 +110,6 @@ void led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
 
 	/* Remove any existing trigger */
 	if (led_cdev->trigger) {
-		write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
-		list_del(&led_cdev->trig_list);
-		write_unlock_irqrestore(&led_cdev->trigger->leddev_list_lock,
-			flags);
 		cancel_work_sync(&led_cdev->set_brightness_work);
 		led_stop_software_blink(led_cdev);
 		if (led_cdev->trigger->deactivate)
@@ -123,9 +118,6 @@ void led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
 		led_set_brightness(led_cdev, LED_OFF);
 	}
 	if (trig) {
-		write_lock_irqsave(&trig->leddev_list_lock, flags);
-		list_add_tail(&led_cdev->trig_list, &trig->led_cdevs);
-		write_unlock_irqrestore(&trig->leddev_list_lock, flags);
 		led_cdev->trigger = trig;
 		if (trig->activate)
 			trig->activate(led_cdev);
@@ -187,7 +179,6 @@ int led_trigger_register(struct led_trigger *trig)
 	struct led_trigger *_trig;
 
 	rwlock_init(&trig->leddev_list_lock);
-	INIT_LIST_HEAD(&trig->led_cdevs);
 
 	down_write(&triggers_list_lock);
 	/* Make sure the trigger's name isn't already in use */
@@ -242,17 +233,15 @@ EXPORT_SYMBOL_GPL(led_trigger_unregister);
 void led_trigger_event(struct led_trigger *trig,
 			enum led_brightness brightness)
 {
-	struct list_head *entry;
+	struct led_classdev *led_cdev;
 
 	if (!trig)
 		return;
 
 	read_lock(&trig->leddev_list_lock);
-	list_for_each(entry, &trig->led_cdevs) {
-		struct led_classdev *led_cdev;
-
-		led_cdev = list_entry(entry, struct led_classdev, trig_list);
-		led_set_brightness(led_cdev, brightness);
+	list_for_each_entry(led_cdev, &leds_list, node) {
+		if (led_cdev->trigger == trig)
+			led_set_brightness(led_cdev, brightness);
 	}
 	read_unlock(&trig->leddev_list_lock);
 }
@@ -264,16 +253,16 @@ static void led_trigger_blink_setup(struct led_trigger *trig,
 			     int oneshot,
 			     int invert)
 {
-	struct list_head *entry;
+	struct led_classdev *led_cdev;
 
 	if (!trig)
 		return;
 
 	read_lock(&trig->leddev_list_lock);
-	list_for_each(entry, &trig->led_cdevs) {
-		struct led_classdev *led_cdev;
+	list_for_each_entry(led_cdev, &leds_list, node) {
+		if (led_cdev->trigger != trig)
+			continue;
 
-		led_cdev = list_entry(entry, struct led_classdev, trig_list);
 		if (oneshot)
 			led_blink_set_oneshot(led_cdev, delay_on, delay_off,
 					      invert);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 0d9b5ee..4c593c6 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -78,7 +78,6 @@ struct led_classdev {
 	struct rw_semaphore	 trigger_lock;
 
 	struct led_trigger	*trigger;
-	struct list_head	 trig_list;
 	void			*trigger_data;
 	/* true if activated - deactivate routine uses it to do cleanup */
 	bool			activated;
@@ -154,7 +153,6 @@ struct led_trigger {
 
 	/* LEDs under control by this trigger (for simple triggers) */
 	rwlock_t	  leddev_list_lock;
-	struct list_head  led_cdevs;
 
 	/* Link to next registered trigger */
 	struct list_head  next_trig;
-- 
1.7.9.5


Best Regards,
Milo

Re: [PATCH 1/2] leds: simply LED trigger list management

[Nathan Lynch]

On Thu, 2013-01-17 at 01:06 +0000, Kim, Milo wrote:
> @@ -242,17 +233,15 @@ EXPORT_SYMBOL_GPL(led_trigger_unregister);
>  void led_trigger_event(struct led_trigger *trig,
>  			enum led_brightness brightness)
>  {
> -	struct list_head *entry;
> +	struct led_classdev *led_cdev;
>  
>  	if (!trig)
>  		return;
>  
>  	read_lock(&trig->leddev_list_lock);
> -	list_for_each(entry, &trig->led_cdevs) {
> -		struct led_classdev *led_cdev;
> -
> -		led_cdev = list_entry(entry, struct led_classdev, trig_list);
> -		led_set_brightness(led_cdev, brightness);
> +	list_for_each_entry(led_cdev, &leds_list, node) {
> +		if (led_cdev->trigger == trig)
> +			led_set_brightness(led_cdev, brightness);
>  	}
>  	read_unlock(&trig->leddev_list_lock);

Continuing to use trig->leddev_list_lock doesn't seem right.  Shouldn't
traversal of leds_list be guarded by the leds_list_lock rwsem?  And if
so, is it safe to use a potentially-blocking lock in this context?

RE: [PATCH 1/2] leds: simply LED trigger list management

[Kim, Milo]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2079 bytes --]

> -----Original Message-----
> From: Nathan Lynch [mailto:ntl@pobox.com]
> 
> On Thu, 2013-01-17 at 01:06 +0000, Kim, Milo wrote:
> > @@ -242,17 +233,15 @@ EXPORT_SYMBOL_GPL(led_trigger_unregister);
> >  void led_trigger_event(struct led_trigger *trig,
> >  			enum led_brightness brightness)
> >  {
> > -	struct list_head *entry;
> > +	struct led_classdev *led_cdev;
> >
> >  	if (!trig)
> >  		return;
> >
> >  	read_lock(&trig->leddev_list_lock);
> > -	list_for_each(entry, &trig->led_cdevs) {
> > -		struct led_classdev *led_cdev;
> > -
> > -		led_cdev = list_entry(entry, struct led_classdev,
> trig_list);
> > -		led_set_brightness(led_cdev, brightness);
> > +	list_for_each_entry(led_cdev, &leds_list, node) {
> > +		if (led_cdev->trigger == trig)
> > +			led_set_brightness(led_cdev, brightness);
> >  	}
> >  	read_unlock(&trig->leddev_list_lock);
> 
> Continuing to use trig->leddev_list_lock doesn't seem right.  Shouldn't
> traversal of leds_list be guarded by the leds_list_lock rwsem?  And if
> so, is it safe to use a potentially-blocking lock in this context?
> 

(Sorry for the typo in title: 'simply' -> 'simplify')

Thanks for your opinion. I agree with you.
The read_lock()/unlock() of 'leddev_list_lock' should be replaced with
down_read()/up_read() of 'leds_list_lock'.
Then, RW lock of led_trigger can be removed also.

BTW, I need more education about the concurrency.
We can see complex RW down/up safe code with list management in LED class driver.
RW semaphores are 'leds_list_lock', 'triggers_list_lock' and 'trigger_lock'.
Are those are safe access in case a user-space via sysfs and driver API calls by
 LED device(s) can happen at the same time?
If so, can we make them more simple?

I think *entry point* of access can be wrapped with semaphores or MUTEX
rather than guard code for accessing LED lists one by one.

I would like to have your opinion and others' too.

Best Regards,
Milo

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/2] leds: simply LED trigger list management

[Nathan Lynch]

On Fri, 2013-01-18 at 00:00 +0000, Kim, Milo wrote:
> > -----Original Message-----
> > From: Nathan Lynch [mailto:ntl@pobox.com]
> > 
> > On Thu, 2013-01-17 at 01:06 +0000, Kim, Milo wrote:
> > > @@ -242,17 +233,15 @@ EXPORT_SYMBOL_GPL(led_trigger_unregister);
> > >  void led_trigger_event(struct led_trigger *trig,
> > >  			enum led_brightness brightness)
> > >  {
> > > -	struct list_head *entry;
> > > +	struct led_classdev *led_cdev;
> > >
> > >  	if (!trig)
> > >  		return;
> > >
> > >  	read_lock(&trig->leddev_list_lock);
> > > -	list_for_each(entry, &trig->led_cdevs) {
> > > -		struct led_classdev *led_cdev;
> > > -
> > > -		led_cdev = list_entry(entry, struct led_classdev,
> > trig_list);
> > > -		led_set_brightness(led_cdev, brightness);
> > > +	list_for_each_entry(led_cdev, &leds_list, node) {
> > > +		if (led_cdev->trigger == trig)
> > > +			led_set_brightness(led_cdev, brightness);
> > >  	}
> > >  	read_unlock(&trig->leddev_list_lock);
> > 
> > Continuing to use trig->leddev_list_lock doesn't seem right.  Shouldn't
> > traversal of leds_list be guarded by the leds_list_lock rwsem?  And if
> > so, is it safe to use a potentially-blocking lock in this context?
> > 
> 
> (Sorry for the typo in title: 'simply' -> 'simplify')
> 
> Thanks for your opinion. I agree with you.
> The read_lock()/unlock() of 'leddev_list_lock' should be replaced with
> down_read()/up_read() of 'leds_list_lock'.
> Then, RW lock of led_trigger can be removed also.

But led_trigger_event() can be called from atomic/interrupt context, no?
We can't use a rwsem in this path.

And I meant to mention earlier -- this change would cause this function
to scan all led devices in the system, whereas right now it consults
only the leds that are associated with the trigger.  That seems like a
step backwards for a potentially performance-sensitive path.


> BTW, I need more education about the concurrency.
> We can see complex RW down/up safe code with list management in LED class driver.
> RW semaphores are 'leds_list_lock', 'triggers_list_lock' and 'trigger_lock'.
> Are those are safe access in case a user-space via sysfs and driver API calls by
>  LED device(s) can happen at the same time?
> If so, can we make them more simple?
>
> I think *entry point* of access can be wrapped with semaphores or MUTEX
> rather than guard code for accessing LED lists one by one.

It seems to me that the relatively fine-grained lists and locks that
exist right now provide a certain level of flexibility and performance
that would be hard to achieve with a coarser model.