Hi,
On 11/09/2016 09:26 PM, Pavel Machek wrote:
Hi!
Thanks for the analysis. Either way, this patch, with the modification
I mentioned in my previous message is required to assure proper
LED sysfs locking.
Regarding the races between user and atomic context, I think that
it should be system root's responsibility to define LED access
policy. If a LED is registered for any trigger events then setting
brightness from user space should be made impossible. Such a hint
could be even added to the Documentation/leds/leds-class.txt.
No, kernel locking may not depend on "root did not do anything
stupid". Sorry.
Is there problem with led_access becoming a spinlock, and
brightness_set_blocking taking it internally while it reads the
brightness (but not while sleeping)?
led_timer_function() does not seem to have any locking. IMO it needs
some, as it does not use atomic accesses.
Locking in led_timer_function() wouldn't solve the problem as
there is led_set_brightness_nosleep() called in it which in turn
can schedule set_brightness_work. The problem is that we can't
hold a spinlock in set_brightness_delayed() as it can block
if LED class driver uses brightness_set_blocking op.
Do we need a spinlock protecting led_classdev.flags and
delayed_set_value?
Would it be good idea to create new "blink_cancel" so brightness_set
is used just for .. brightness and not for timer manipulations?
Should we do something like this for consistency?
We would have to change the ABI I'm afraid.
BTW how is locking expected to work with userland LED drivers? What if
userland LED driver accesses /sys files for its own LED?
What do you mean by userland LED driver accessing sysfs files?
I'd really
prefer that patch not to be merged until we get locking right.
diff --git a/include/linux/leds.h b/include/linux/leds.h
index ddfcb2d..60e436d 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -88,11 +88,11 @@ struct led_classdev {
unsigned long blink_delay_on, blink_delay_off;
struct timer_list blink_timer;
- int blink_brightness;
+ enum led_brightness blink_brightness;
void (*flash_resume)(struct led_classdev *led_cdev);
struct work_struct set_brightness_work;
- int delayed_set_value;
+ enum led_brightness delayed_set_value;
Good point. I'll keep it in mind.
#ifdef CONFIG_LEDS_TRIGGERS
/* Protects the trigger data below */
--
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
