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

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

On 11/28/2017 10:35 PM, Jacek Anaszewski wrote:
> On 11/28/2017 05:32 AM, Craig McQueen wrote:
>> 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);
> 
> It will be also prone to races. Every solution not employing
> mutual exclusive section will be. I'm starting to think if the
> best we can do isn't just preventing brightness setting when
> blink disable is pending.
> 
> 
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index fd83c7f..9c775a2 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -228,6 +228,11 @@ EXPORT_SYMBOL_GPL(led_stop_software_blink);
>  void led_set_brightness(struct led_classdev *led_cdev,
>                         enum led_brightness brightness)
>  {
> +       if (test_bit(LED_BLINK_DISABLE, &led_cdev->work_flags)) {
> +               dev_err(led_cdev->dev,
> +                       "Setting an LED's brightness failed - blink
> disable pending\n");
> +               return;
> +       }

Of course it will not work too since we can be preempted here
by the other process that will set LED_BLINK_DISABLE.
After waking up this one will not be aware of the flag state
change.

>         /*
>          * If software blink is active, delay brightness setting
>          * until the next timer tick.
> 
> 
>