Hi,
On 20-Feb-25 9:46 AM, Remi Pommarel wrote:
> On Wed, Feb 19, 2025 at 12:52:36PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 19-Feb-25 11:41 AM, Remi Pommarel wrote:
>>> While commit fa15d8c69238 ("leds: Fix set_brightness_delayed() race")
>>> successfully forces led_set_brightness() to be called with LED_OFF at
>>> least once when switching from blinking to LED on state so that
>>> hw-blinking can be disabled, another race remains. Indeed in
>>> led_set_brightness(LED_OFF) followed by led_set_brightness(any)
>>> scenario the following CPU scheduling can happen:
>>>
>>> CPU0 CPU1
>>> ---- ----
>>> set_brightness_delayed() {
>>> test_and_clear_bit(BRIGHTNESS_OFF)
>>> led_set_brightness(LED_OFF) {
>>> set_bit(BRIGHTNESS_OFF)
>>> queue_work()
>>> }
>>> led_set_brightness(any) {
>>> set_bit(BRIGHTNESS)
>>> queue_work() //already queued
>>> }
>>> test_and_clear_bit(BRIGHTNESS)
>>> /* LED set with brightness any */
>>> }
>>>
>>> /* From previous CPU1 queue_work() */
>>> set_brightness_delayed() {
>>> test_and_clear_bit(BRIGHTNESS_OFF)
>>> /* LED turned off */
>>> test_and_clear_bit(BRIGHTNESS)
>>> /* Clear from previous run, LED remains off */
>>>
>>> In that case the led_set_brightness(LED_OFF)/led_set_brightness(any)
>>> sequence will be effectively executed in reverse order and LED will
>>> remain off.
>>>
>>> With the introduction of commit 32360bf6a5d4 ("leds: Introduce ordered
>>> workqueue for LEDs events instead of system_wq") the race is easier to
>>> trigger as sysfs brightness configuration does not wait for
>>> set_brightness_delayed() work to finish (flush_work() removal).
>>>
>>> Use delayed_set_value to optionnally re-configure brightness after a
>>> LED_OFF. That way a LED state could be configured more that once but
>>> final state will always be as expected.
>>>
>>> Fixes: fa15d8c69238 ("leds: Fix set_brightness_delayed() race")
>>> Signed-off-by: Remi Pommarel <repk@xxxxxxxxxxxx>
>>
>> Thanks, patch looks good to me:
>>
>
> Actually two additionnal remarks here. The first one is that now more
> than before, delayed_set_value store should be seen before work_flags
> modification on other CPUs. That means that a smp_mb__before_atomic()
> is needed before the two set_bit().
>
> The second one is that delayed_set_value can be bigger than a single
> byte, so theoretically store tearing can happen and
> set_brightness_delayed_set_brightness() could be called with an invalid
> value. WRITE_ONCE/READ_ONCE could prevent that but because the
> smp_mb__before_atomic() ensures that the "last" delayed_set_value is
> valid I don't mind having very seldom intermediate invalid values.
>
> So I think a v2 with smp_mb__before_atomic() is needed here, what do you
> think ?
Doing a v2 with smp_mb__before_atomic() sounds good to me.
Regards,
Hans
