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;
+ }
/*
* If software blink is active, delay brightness setting
* until the next timer tick.
--
Best regards,
Jacek Anaszewski
