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

[Nathan Lynch <ntl@pobox.com>]

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.