[RESEND] leds: core: Support blocking HW blink operations

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.20.1

Hi Linus,

Could you share why can't it be fixed by moving blink_set()
to a workqueue in the driver?

Best regards,
Jacek Anaszewski


On 1/17/19 11:10 AM, 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>

> ---

>   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);

> +		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 5263f87e1d2c..7050383d72a7 100644

> --- a/include/linux/leds.h

> +++ b/include/linux/leds.h

> @@ -62,6 +62,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

> @@ -89,6 +90,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);

>

On Thu, Jan 17, 2019 at 10:21 PM Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:

> Could you share why can't it be fixed by moving blink_set()

> to a workqueue in the driver?


It can be done that way, but that will lead to that:

- The existing work struct in struct led_classdev
  provided by the LED core is not used, albeit being used
  by led_set_brightness()

- led_blink_set() will have different semantic requirements
  than led_set_brightness() which can be called from
  any context as it will defer to led_set_brightness_nosleep()
  -> led_set_brightness_nopm() -> kicks workqueue.

To me that appears arbitrarily inconsistent which is IMO not
a good thing in generic APIs, but maybe there are other
ways to see it?

Yours,
Linus Walleij

On 1/18/19 2:51 PM, Linus Walleij wrote:
> On Thu, Jan 17, 2019 at 10:21 PM Jacek Anaszewski

> <jacek.anaszewski@gmail.com> wrote:

> 

>> Could you share why can't it be fixed by moving blink_set()

>> to a workqueue in the driver?

> 

> It can be done that way, but that will lead to that:

> 

> - The existing work struct in struct led_classdev

>    provided by the LED core is not used, albeit being used

>    by led_set_brightness()


Generic support for deferring brightness setting is justified because
it is called from led_timer_function() in the LED core (software
blinking fallback) and from timers in triggers.

> - led_blink_set() will have different semantic requirements

>    than led_set_brightness() which can be called from

>    any context as it will defer to led_set_brightness_nosleep()

>    -> led_set_brightness_nopm() -> kicks workqueue.

> 

> To me that appears arbitrarily inconsistent which is IMO not

> a good thing in generic APIs, but maybe there are other

> ways to see it?


As stated above software blinking implementation in the core
enforces non-blocking semantics of brightness setting operations.
Currently we don't have anything similar required for blink
setting operations.

LED core maintainability is now hindered sufficiently with that
and also with bad design decision that allowed led_set_brightness()
to be called from hard irq context
(d23a22a74fde ("leds: delay led_set_brightness if stopping soft-blink"),
for which set_brightness_work was introduced initially, and its scope of
responsibilities was only extended when adding generic support for non
blocking brightness setting).

Please use in-driver workqueue for calling blink_set().

-- 
Best regards,
Jacek Anaszewski

On Sun, Jan 20, 2019 at 3:12 PM Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:

> As stated above software blinking implementation in the core

> enforces non-blocking semantics of brightness setting operations.

> Currently we don't have anything similar required for blink

> setting operations.

>

> LED core maintainability is now hindered sufficiently with that

> and also with bad design decision that allowed led_set_brightness()

> to be called from hard irq context

> (d23a22a74fde ("leds: delay led_set_brightness if stopping soft-blink"),

> for which set_brightness_work was introduced initially, and its scope of

> responsibilities was only extended when adding generic support for non

> blocking brightness setting).

>

> Please use in-driver workqueue for calling blink_set().


Aha I see your point clearly now.

I'll create a local delayed work!

Yours,
Linus Walleij