Re: [PATCH] leds: Add mutex protection in brightness_show()

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

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