RE: Bug when using both "set" and "blink" functions

[Craig McQueen <craig.mcqueen@innerrange.com.au>]

Jacek Anaszewski wrote:
> On 11/27/2017 07:51 AM, Craig McQueen wrote:
> > Jacek Anaszewski wrote:
> >> I checked earlier discussed case i.e. disabling trigger and
> >> immediately setting brightness to non-zero value and it seems to work
> >> fine. Checked with uleds
> >> driver:
> >>
> >> # echo "timer" > trigger
> >> # echo 0 > brightness; echo 100 > brightness
> >>
> >> Brightness always is 100 after that, even when setting delay_on/off to 1.
> >
> > I still think there's a race condition, but you don't discover it when using
> userspace, because there's sufficient time between setting the brightness to
> 0 and later 100, for LED_BLINK_DISABLE to be processed in-between.
> >
> > But I'm using a custom kernel driver that tries to go from blinking to steady-
> on using these function calls:
> >
> > 		led_trigger_event(trigger, LED_OFF);   /* which calls
> led_set_brightness(led_cdev, LED_OFF) */
> > 		led_trigger_event(trigger, LED_FULL);  /* which calls
> > led_set_brightness(led_cdev, LED_FULL) */
> >
> > In this case, LED_BLINK_DISABLE is not yet processed before the second
> function call, so the second function call becomes ineffective, because the
> pending LED_BLINK_DISABLE is later processed, turning off the LED and
> leaving it off.
> 
> Have you verified it by inserting printks in the led_set_brightness() and
> brightness_set op of your driver?

No, I didn't verify it. I deduced it by code review of led-core.c, and its use of LED_BLINK_DISABLE in led_set_brightness() and set_brightness_delayed().

Then, I made a patch (in an earlier email) which modified led_set_brightness(), and that fixed the issue. However, I was concerned that there still could be a race condition if set_brightness_delayed() happened to run in the middle of a call to led_set_brightness(led_cdev, LED_FULL). In that case, the call led_set_brightness(led_cdev, LED_FULL) could still fail.

> Regardless of that, I strongly advise backporting the mainline patches.
> If that doesn't help then make sure that brightness_set op of your driver
> implements proper locking scheme.

The problem seems to be nothing to do with the brightness_set op of the driver.

> If that doesn't help too, then you could
> try to come up with your patch for the LED core, that fixes the issue for you.
> It can be hard to address your particular case otherwise.

This code in led-core.c seems quite tricky to avoid any race conditions, without using a spinlock or semaphore. Here is another proposal which I am considering:

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index ef1360445413..5fe7826deab2 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -109,8 +109,8 @@ static void set_brightness_delayed(struct work_struct *ws)
        int ret = 0;
 
        if (test_and_clear_bit(LED_BLINK_DISABLE, &led_cdev->work_flags)) {
-               led_cdev->delayed_set_value = LED_OFF;
                led_stop_software_blink(led_cdev);
+               led_cdev->delayed_set_value = led_cdev->new_blink_brightness;
        }
 
        ret = __led_set_brightness(led_cdev, led_cdev->delayed_set_value);
@@ -239,15 +239,24 @@ void led_set_brightness(struct led_classdev *led_cdev,
                 * work queue task to avoid problems in case we are called
                 * from hard irq context.
                 */
+               led_cdev->new_blink_brightness = brightness;
                if (brightness == LED_OFF) {
                        set_bit(LED_BLINK_DISABLE, &led_cdev->work_flags);
                        schedule_work(&led_cdev->set_brightness_work);
                } else {
                        set_bit(LED_BLINK_BRIGHTNESS_CHANGE,
                                &led_cdev->work_flags);
-                       led_cdev->new_blink_brightness = brightness;
                }
-               return;
+
+               if (test_bit(LED_BLINK_SW, &led_cdev->work_flags)) {
+                       /*
+                        * Test LED_BLINK_SW again, to handle race condition
+                        * with set_brightness_delayed(). If it's no longer
+                        * set, then blink has just been stopped, so continue
+                        * with led_set_brightness_nosleep() below.
+                        */
+                       return;
+               }
        }
 
        led_set_brightness_nosleep(led_cdev, brightness);


-- 
Craig McQueen