| Message ID | 20181127150106.20213-1-linus.walleij@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leds: core: Support blocking HW blink operations | expand |
Nits: > ALl of this will be handled transparently from the > led_blink_set() call so all current users can keep > using that. "All". Plus, your lines are rather short. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/leds/led-core.c | 49 ++++++++++++++++++++++++++++++++++++++--- > include/linux/leds.h | 9 ++++++++ > 2 files changed, 55 insertions(+), 3 deletions(-) > > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c > index ede4fa0ac2cc..94870a3d59af 100644 > --- a/drivers/leds/led-core.c > +++ b/drivers/leds/led-core.c > @@ -108,6 +108,20 @@ static void set_brightness_delayed(struct work_struct *ws) > container_of(ws, struct led_classdev, set_brightness_work); > int ret = 0; > > + if (test_and_clear_bit(LED_BLINK_HW_SET, &led_cdev->work_flags)) { > + if (led_cdev->blink_set) > + ret = led_cdev->blink_set(led_cdev, > + &led_cdev->blink_delay_on, > + &led_cdev->blink_delay_off); How can we get here? > + else if (led_cdev->blink_set_blocking) > + ret = led_cdev->blink_set_blocking(led_cdev, > + &led_cdev->blink_delay_on, > + &led_cdev->blink_delay_off); > + if (ret) > + dev_err(led_cdev->dev, > + "setting hardware blink failed (%d)\n", ret); > + } This is a bit sad, as it disallows returning errors back to userspace. And I'm pretty sure little hardware can support all the possible delays.... > mod_timer(&led_cdev->blink_timer, jiffies + 1); > } > > +static int led_set_hardware_blink_nosleep(struct led_classdev *led_cdev, > + unsigned long *delay_on, > + unsigned long *delay_off) > +{ > + /* We have a non-blocking call, use it */ > + if (led_cdev->blink_set) > + return led_cdev->blink_set(led_cdev, delay_on, delay_off); > + > + /* Tell the worker to set up the blinking hardware */ > + if (delay_on) > + led_cdev->blink_delay_on = *delay_on; > + else > + led_cdev->blink_delay_on = 0; > + if (delay_off) > + led_cdev->blink_delay_off = *delay_off; > + else > + led_cdev->blink_delay_off = 0; > + set_bit(LED_BLINK_HW_SET, &led_cdev->work_flags); > + schedule_work(&led_cdev->set_brightness_work); > + > + return 0; > +} > > static void led_blink_setup(struct led_classdev *led_cdev, > unsigned long *delay_on, > unsigned long *delay_off) > { > + /* > + * If not oneshot, and we have hardware support for blinking, > + * relay the request to the driver. > + */ > if (!test_bit(LED_BLINK_ONESHOT, &led_cdev->work_flags) && > - led_cdev->blink_set && > - !led_cdev->blink_set(led_cdev, delay_on, delay_off)) > - return; > + (led_cdev->blink_set || led_cdev->blink_set_blocking)) { > + /* If this fails we fall back to software blink */ > + if (!led_set_hardware_blink_nosleep(led_cdev, > + delay_on, delay_off)) > + return; > + } > So, when can this actually be called from atomic context? (And deferring to work queue to set hardware blinking for one shot ... does not make sense.) led_trigger_blink_setup uses read_lock() -- unsafe from atomic AFAICT. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Tue 2018-11-27 16:01:06, Linus Walleij wrote: > I ran into this when working on a keyboard driver for > the Razer family: the .blink_set() callback for setting > hardware blinking on a LED only exist in a non-blocking > (fastpath) variant, such as when blinking can be enabled > by simply writing a memory-mapped register and protected > by spinlocks. > > On USB keyboards with blinkable LEDs controlled with USB > out-of-band commands this will of course not work: these > calls need to come from process context. > > To support this: add a new .blink_set_blocking() callback > in the same vein as .brightness_set_blocking() and add > a flag and some code to the delayed work so that this > will be able to fire the .blink_set_blocking() call. > > ALl of this will be handled transparently from the > led_blink_set() call so all current users can keep > using that. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> Fun. I just realized thinkpad x60 needs something similar... -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sat, Jun 1, 2019 at 10:37 PM Pavel Machek <pavel@ucw.cz> wrote: > On Tue 2018-11-27 16:01:06, Linus Walleij wrote: > > I ran into this when working on a keyboard driver for > > the Razer family: the .blink_set() callback for setting > > hardware blinking on a LED only exist in a non-blocking > > (fastpath) variant, such as when blinking can be enabled > > by simply writing a memory-mapped register and protected > > by spinlocks. > > > > On USB keyboards with blinkable LEDs controlled with USB > > out-of-band commands this will of course not work: these > > calls need to come from process context. > > > > To support this: add a new .blink_set_blocking() callback > > in the same vein as .brightness_set_blocking() and add > > a flag and some code to the delayed work so that this > > will be able to fire the .blink_set_blocking() call. > > > > ALl of this will be handled transparently from the > > led_blink_set() call so all current users can keep > > using that. > > > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > > Fun. I just realized thinkpad x60 needs something similar... Hm yeah. The discussion with Jacek came to the conclusion that he thinks (if I understand correctly!) that the LED core is too helpful and client drivers should create process contexts instead (like workers I suppose) and use the opaque interfaces from there, whether they are blocking or not, and that it was a mistake from the beginning to create a helper thread inside the LED core. I like APIs that are narrow and deep so I would prefer to do it my way (i.e. this patch) but arguably it is a matter of taste. I hope to get back to this patch set at some point. Yours, Linus Walleij
On 6/1/19 11:47 PM, Linus Walleij wrote: > On Sat, Jun 1, 2019 at 10:37 PM Pavel Machek <pavel@ucw.cz> wrote: >> On Tue 2018-11-27 16:01:06, Linus Walleij wrote: > >>> I ran into this when working on a keyboard driver for >>> the Razer family: the .blink_set() callback for setting >>> hardware blinking on a LED only exist in a non-blocking >>> (fastpath) variant, such as when blinking can be enabled >>> by simply writing a memory-mapped register and protected >>> by spinlocks. >>> >>> On USB keyboards with blinkable LEDs controlled with USB >>> out-of-band commands this will of course not work: these >>> calls need to come from process context. >>> >>> To support this: add a new .blink_set_blocking() callback >>> in the same vein as .brightness_set_blocking() and add >>> a flag and some code to the delayed work so that this >>> will be able to fire the .blink_set_blocking() call. >>> >>> ALl of this will be handled transparently from the >>> led_blink_set() call so all current users can keep >>> using that. >>> >>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> >> >> Fun. I just realized thinkpad x60 needs something similar... > > Hm yeah. The discussion with Jacek came to the conclusion that he > thinks (if I understand correctly!) that the LED core is too helpful and > client drivers should create process contexts instead (like workers > I suppose) and use the opaque interfaces from there, whether they > are blocking or not, and that it was a mistake from the beginning > to create a helper thread inside the LED core. > > I like APIs that are narrow and deep so I would prefer to do it my > way (i.e. this patch) but arguably it is a matter of taste. > > I hope to get back to this patch set at some point. Well, yes, I missed the fact that we already had the use case in mainline where blink_set is called from atomic context, which is led_trigger_blink{_oneshot}(). So, Pavel, you seem to have good setup for testing this Linus' patch. -- Best regards, Jacek Anaszewski
On Sun 2019-06-02 13:20:20, Jacek Anaszewski wrote: > On 6/1/19 11:47 PM, Linus Walleij wrote: > >On Sat, Jun 1, 2019 at 10:37 PM Pavel Machek <pavel@ucw.cz> wrote: > >>On Tue 2018-11-27 16:01:06, Linus Walleij wrote: > > > >>>I ran into this when working on a keyboard driver for > >>>the Razer family: the .blink_set() callback for setting > >>>hardware blinking on a LED only exist in a non-blocking > >>>(fastpath) variant, such as when blinking can be enabled > >>>by simply writing a memory-mapped register and protected > >>>by spinlocks. > >>> > >>>On USB keyboards with blinkable LEDs controlled with USB > >>>out-of-band commands this will of course not work: these > >>>calls need to come from process context. > >>> > >>>To support this: add a new .blink_set_blocking() callback > >>>in the same vein as .brightness_set_blocking() and add > >>>a flag and some code to the delayed work so that this > >>>will be able to fire the .blink_set_blocking() call. > >>> > >>>ALl of this will be handled transparently from the > >>>led_blink_set() call so all current users can keep > >>>using that. > >>> > >>>Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > >> > >>Fun. I just realized thinkpad x60 needs something similar... > > > >Hm yeah. The discussion with Jacek came to the conclusion that he > >thinks (if I understand correctly!) that the LED core is too helpful and > >client drivers should create process contexts instead (like workers > >I suppose) and use the opaque interfaces from there, whether they > >are blocking or not, and that it was a mistake from the beginning > >to create a helper thread inside the LED core. > > > >I like APIs that are narrow and deep so I would prefer to do it my > >way (i.e. this patch) but arguably it is a matter of taste. > > > >I hope to get back to this patch set at some point. > > Well, yes, I missed the fact that we already had the use case > in mainline where blink_set is called from atomic context, > which is led_trigger_blink{_oneshot}(). Yep. > So, Pavel, you seem to have good setup for testing this Linus' > patch. OTOH that is _not_ passed down to drivers, as they can blink, but not do oneshot, and blinking on X60 is limited to single frequency so likely not useful for triggers. But we probably want to annotate can/can not sleep and use lockdep to catch the bugs. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c index ede4fa0ac2cc..94870a3d59af 100644 --- a/drivers/leds/led-core.c +++ b/drivers/leds/led-core.c @@ -108,6 +108,20 @@ static void set_brightness_delayed(struct work_struct *ws) container_of(ws, struct led_classdev, set_brightness_work); int ret = 0; + if (test_and_clear_bit(LED_BLINK_HW_SET, &led_cdev->work_flags)) { + if (led_cdev->blink_set) + ret = led_cdev->blink_set(led_cdev, + &led_cdev->blink_delay_on, + &led_cdev->blink_delay_off); + else if (led_cdev->blink_set_blocking) + ret = led_cdev->blink_set_blocking(led_cdev, + &led_cdev->blink_delay_on, + &led_cdev->blink_delay_off); + if (ret) + dev_err(led_cdev->dev, + "setting hardware blink failed (%d)\n", ret); + } + 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); @@ -157,15 +171,44 @@ static void led_set_software_blink(struct led_classdev *led_cdev, mod_timer(&led_cdev->blink_timer, jiffies + 1); } +static int led_set_hardware_blink_nosleep(struct led_classdev *led_cdev, + unsigned long *delay_on, + unsigned long *delay_off) +{ + /* We have a non-blocking call, use it */ + if (led_cdev->blink_set) + return led_cdev->blink_set(led_cdev, delay_on, delay_off); + + /* Tell the worker to set up the blinking hardware */ + if (delay_on) + led_cdev->blink_delay_on = *delay_on; + else + led_cdev->blink_delay_on = 0; + if (delay_off) + led_cdev->blink_delay_off = *delay_off; + else + led_cdev->blink_delay_off = 0; + set_bit(LED_BLINK_HW_SET, &led_cdev->work_flags); + schedule_work(&led_cdev->set_brightness_work); + + return 0; +} static void led_blink_setup(struct led_classdev *led_cdev, unsigned long *delay_on, unsigned long *delay_off) { + /* + * If not oneshot, and we have hardware support for blinking, + * relay the request to the driver. + */ if (!test_bit(LED_BLINK_ONESHOT, &led_cdev->work_flags) && - led_cdev->blink_set && - !led_cdev->blink_set(led_cdev, delay_on, delay_off)) - return; + (led_cdev->blink_set || led_cdev->blink_set_blocking)) { + /* If this fails we fall back to software blink */ + if (!led_set_hardware_blink_nosleep(led_cdev, + delay_on, delay_off)) + return; + } /* blink with 1 Hz as default if nothing specified */ if (!*delay_on && !*delay_off) diff --git a/include/linux/leds.h b/include/linux/leds.h index 7393a316d9fa..57f3df0774e7 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -61,6 +61,7 @@ struct led_classdev { #define LED_BLINK_INVERT 3 #define LED_BLINK_BRIGHTNESS_CHANGE 4 #define LED_BLINK_DISABLE 5 +#define LED_BLINK_HW_SET 6 /* Set LED brightness level * Must not sleep. Use brightness_set_blocking for drivers @@ -88,6 +89,14 @@ struct led_classdev { int (*blink_set)(struct led_classdev *led_cdev, unsigned long *delay_on, unsigned long *delay_off); + /* + * Set LED blinking immediately but may sleep (block) the caller + * to access the LED device register. + */ + int (*blink_set_blocking)(struct led_classdev *led_cdev, + unsigned long *delay_on, + unsigned long *delay_off); + int (*pattern_set)(struct led_classdev *led_cdev, struct led_pattern *pattern, u32 len, int repeat);
I ran into this when working on a keyboard driver for the Razer family: the .blink_set() callback for setting hardware blinking on a LED only exist in a non-blocking (fastpath) variant, such as when blinking can be enabled by simply writing a memory-mapped register and protected by spinlocks. On USB keyboards with blinkable LEDs controlled with USB out-of-band commands this will of course not work: these calls need to come from process context. To support this: add a new .blink_set_blocking() callback in the same vein as .brightness_set_blocking() and add a flag and some code to the delayed work so that this will be able to fire the .blink_set_blocking() call. ALl of this will be handled transparently from the led_blink_set() call so all current users can keep using that. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/leds/led-core.c | 49 ++++++++++++++++++++++++++++++++++++++--- include/linux/leds.h | 9 ++++++++ 2 files changed, 55 insertions(+), 3 deletions(-) -- 2.19.1