Jacek Anaszewski wrote: > On 11/23/2017 01:55 AM, Craig McQueen wrote: > > Jacek Anaszewski wrote: > >> Hi Craig, > >> > >> On 11/22/2017 01:55 AM, Craig McQueen wrote: > >>> I'd like to control a LED to possibly be any of: > >>> > >>> * Off > >>> * On > >>> * Blinking > >>> > >>> I've had this working fine in 3.14.x kernel. > >>> > >>> But when upgrading to 4.4.x, I found that the transition from > >>> "blinking" to > >> "on" didn't work. Namely, if it's blinking and then I call > >> led_set_brightness(led_cdev, LED_FULL), then it wouldn't work (I > >> can't remember if it turned off, or remained blinking; it wasn't on > >> anyway). I worked around it by calling led_set_brightness(led_cdev, > >> LED_OFF) just before led_set_brightness(led_cdev, LED_FULL). > >> > >> This is by design. Setting brightness to LED_OFF deactivates any > >> active trigger. There were some inconsistencies in the API semantics > >> and it was fixed. > >> > >> See Documentation/ABI/testing/sysfs-class-led > >> > >> What: /sys/class/leds/<led>/brightness > >> Date: March 2006 > >> KernelVersion: 2.6.17 > >> Contact: Richard Purdie <rpurdie@xxxxxxxxx> > >> Description: > >> Set the brightness of the LED. Most LEDs don't > >> have hardware brightness support, so will just be turned on for > >> non-zero brightness settings. The value is between 0 and > >> /sys/class/leds/<led>/max_brightness. > >> > >> Writing 0 to this file clears active trigger. > >> > >> Writing non-zero to this file while trigger is active changes the > >> top brightness trigger is going to use. > > > > That makes sense. Which is why, when I changed to kernel 4.4.x, I made > the "on" case do led_set_brightness(led_cdev, LED_OFF); > led_set_brightness(led_cdev, LED_FULL);. As I described in another email, I > am actually creating LED triggers, so my code calls led_trigger_event(trigger, > LED_OFF); led_trigger_event(trigger, LED_FULL);. That seems to meet the > design intent of the API. > > > >>> Now I have upgraded to 4.9.x, and found that the transition from > "blinking" > >> to "on" again isn't working. The LED ends up being off instead of on. > >>> > >>> Examining the code of led_set_brightness(): > >>> > >>> * Behaviour is different depending on whether the brightness is > >>> LED_OFF > >> (it schedules the blink to be turned off "soon") or other (it alters > >> the brightness of subsequent blinks). > >>> * It looks as though there are race conditions in the transition > >>> from blinking > >> to steady off -- it schedules the blink to be turned off "soon", so > >> it's difficult to guarantee a reliable transition from blinking to on/off. > >>> > >>> The combination of the above two points makes it seem difficult to > >> robustly go from blinking to off/on. > >> > >> Please make sure you have the following patch: > >> > >> Author: Hans de Goede <hdegoede@xxxxxxxxxx> 2016-11-08 14:38:46 > >> Committer: Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx> 2016-11-22 > >> 12:07:05 > >> Parent: 8338eab50ffb3399a938d723f9605383ed9f8473 (leds: Add user LED > >> driver for NIC78bx device) > >> Follows: v4.9-rc1 > >> Precedes: leds_for_4.10 > >> > >> led: core: Use atomic bit-field for the blink-flags > >> > >> All the LED_BLINK* flags are accessed read-modify-write from e.g. > >> led_set_brightness and led_blink_set_oneshot while both > >> set_brightness_work and the blink_timer may be running. > >> > >> If these race then the modify step done by one of them may be lost, > >> switch the LED_BLINK* flags to a new atomic work_flags bit-field > >> to avoid this race. > > > > I don't have that patch, I don't think. I see it in commit > a9c6ce57ec2f136d08141e8220a0ffaca216f7b0 which goes into kernel 4.10. But > it looks as though that's not the essence of the issue. > > It's actually eb1610b4c273370f491c5e194e5a56e3470d81e8. > > > > >>> So, my questions are: > >>> > >>> * What is the correct way to use the API to reliably control an LED > >>> for a > >> combination of off/on/blinking? > > > > It sounds as though calling led_set_brightness(led_cdev, LED_OFF); > led_set_brightness(led_cdev, LED_FULL); in-principle should behave > according to API design intent. > > > >>> * Is my above analysis of the code correct, so that there are indeed > >>> race > >> conditions going from blinking to off, leading to undefined > >> behaviour? Can that be fixed? > > > > Looking at the code, it looks problematic that led_set_brightness(led_cdev, > LED_OFF) does not immediately disable soft blinking, but schedules it for > "soon in the future". > > Comments in led_set_brightness() explain the reason for which delaying > brightness setting is required, i.e. to avoid the problem when > led_set_brightness() is called from hard irq context with timer trigger > enabled in the same time. See the original patch introducing deferred > brightness setting: > > commit d23a22a74fded23a12434c9463fe66cec2b0afcd > Author: Fabio Baltieri <fabio.baltieri@xxxxxxxxx> > Date: Wed Aug 15 21:44:34 2012 +0800 > > leds: delay led_set_brightness if stopping soft-blink > > Delay execution of led_set_brightness() if need to stop soft-blink > timer. > > This allows led_set_brightness to be called in hard-irq context even if > soft-blink was activated on that LED. > > Signed-off-by: Fabio Baltieri <fabio.baltieri@xxxxxxxxx> > Cc: Pawel Moll <pawel.moll@xxxxxxx> > Signed-off-by: Bryan Wu <bryan.wu@xxxxxxxxxxxxx> > > > In general it allows to avoid conflict explained in [0]. > > [0] > https://www.kernel.org/pub/linux/kernel/people/rusty/kernel- > locking/c188.html > > > Then, when led_set_brightness(led_cdev, LED_FULL) is called, it is not > > effective because the previous call with LED_OFF has not actually done > > anything yet. > > You really should test your code with the patch > eb1610b4c27 ("led: core: Use atomic bit-field for the blink-flags'). > > Changing the brightness to any non-zero value while blinking is on shouldn't > cause races after introducing flag LED_BLINK_BRIGHTNESS_CHANGE, which is > set in led_brightness_set() when blinking is on, and new brightness is applied > upon next blink transition to on. This doesn't stop blinking though, which is in > line with doc. > > Now I see that there can be a problem if you set brightness to 0 while > blinking and immediately set it to any non-zero value, before > LED_BLINK_DISABLE is handled. The last operation can have no effect then. > I'll check it. I've considered the following fix: diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c index ef1360445413..5e60a6c3a52f 100644 --- a/drivers/leds/led-core.c +++ b/drivers/leds/led-core.c @@ -109,7 +109,6 @@ 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); } @@ -241,7 +240,10 @@ void led_set_brightness(struct led_classdev *led_cdev, */ if (brightness == LED_OFF) { set_bit(LED_BLINK_DISABLE, &led_cdev->work_flags); + led_cdev->delayed_set_value = brightness; schedule_work(&led_cdev->set_brightness_work); + } else if (test_bit(LED_BLINK_DISABLE, &led_cdev->work_flags)) { + led_cdev->delayed_set_value = brightness; } else { set_bit(LED_BLINK_BRIGHTNESS_CHANGE, &led_cdev->work_flags); However, I think it has problems with concurrency/race conditions. I'm having difficulty thinking of an elegant solution that is free of race conditions, without resorting to spinlocks etc. I'll be very interested to see what you propose.
