> -----Original Message----- > From: Nathan Lynch [mailto:n...@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
