Re: [PATCH] led: core: Fix race on software blink cancellation

[Jacek Anaszewski]

On 02/08/2018 10:50 PM, Pavel Machek wrote:
> Hi!
> 
>> Any comments? I'd like to have some acks before applying
>> this patch.
> 
> I can't say I like it.

Please point out the bits you don't like and spot any risks.

>>> Commit d23a22a74fde ("leds: delay led_set_brightness if stopping soft-blink")
>>> made a modifications to the LED core allowing for led_set_brightness() to be
>>> called from hard-irq context when soft blink is being handled in soft-irq.
>>>
>>> Since that time LED core has undergone modifications related to addition of
>>> generic support for delegating brightness setting to a workqueue as well as
>>> subsequent fixes for blink setting use cases.
>>>
>>> After that the LED core code became hard to maintain and analyze, especially
>>> due to the imposed hard-irq context compatibility. It also turned out that
>>> in some cases a LED remained off after executing following sequence of commands:
>>>
>>> 1. echo timer > trigger
>>> 2. echo 0 > brightness
>>> 3. echo 100 > brightness
>>>
>>> The reason was LED_BLINK_DISABLE operation delegated to a set_brightness_work,
>>> triggered in 2., which was handled after 3. took effect.
> 
> Could we wait for delayed work to be done before setting the
> brightness to fix this one?

Without mutually exclusive section you can always be preempted
after wait test succeeds and before requesting new brightness.
Then the other process can jump in, request other brightness
(and change it in struct led_classdev via led_set_brightness_nosleep()),
which causes we end up with non deterministic LED class device
state after that.

Every solution without mutually exclusive section is prone to races
here.

-- 
Best regards,
Jacek Anaszewski