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 ?
Regards,
--
Remi
