Re: [PATCH] led: core: Fix race on software blink cancellation

[Jacek Anaszewski]

Any comments? I'd like to have some acks before applying
this patch.

Adding more people showing recently interest in the LED subsystem
related development.

Thanks,
Jacek Anaszewski

On 01/17/2018 10:12 PM, Jacek Anaszewski wrote:
> Commit d23a22a74fde ("leds: delay led_set_brightness if stopping soft-blink")
> made a modifications to the LED core allowing for led_set_brightness() to be
> called from hard-irq context when soft blink is being handled in soft-irq.
> 
> Since that time LED core has undergone modifications related to addition of
> generic support for delegating brightness setting to a workqueue as well as
> subsequent fixes for blink setting use cases.
> 
> After that the LED core code became hard to maintain and analyze, especially
> due to the imposed hard-irq context compatibility. It also turned out that
> in some cases a LED remained off after executing following sequence of commands:
> 
> 1. echo timer > trigger
> 2. echo 0 > brightness
> 3. echo 100 > brightness
> 
> The reason was LED_BLINK_DISABLE operation delegated to a set_brightness_work,
> triggered in 2., which was handled after 3. took effect.
> 
> In order to serialize above operations and in the same time avoid
> code overcomplication the hard-irq context compatibility is being removed,
> which allows to use spin_lock_bh() for LED blink setting serialization.
> 
>>From now, if in hard-irq context, users need to delegate led_set_brightness()
> to a workqueue in the place of use.
> 
> Reported-by: Craig McQueen <craig.mcqueen@xxxxxxxxxxxxxxxxx>
> Signed-off-by: Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx>
> ---
> Craig,
> 
> It would be great if you could confirm if this fixes your use case.
> 
>  drivers/leds/led-core.c                  | 79 +++++++++++++++++++-------------
>  drivers/leds/trigger/ledtrig-activity.c  |  3 --
>  drivers/leds/trigger/ledtrig-heartbeat.c |  3 --
>  include/linux/leds.h                     |  5 +-
>  4 files changed, 49 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index ede4fa0..25d711b 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -22,6 +22,9 @@
>  DECLARE_RWSEM(leds_list_lock);
>  EXPORT_SYMBOL_GPL(leds_list_lock);
>  
> +DEFINE_SPINLOCK(leds_soft_blink_lock);
> +EXPORT_SYMBOL_GPL(leds_soft_blink_lock);
> +
>  LIST_HEAD(leds_list);
>  EXPORT_SYMBOL_GPL(leds_list);
>  
> @@ -51,26 +54,31 @@ static void led_timer_function(struct timer_list *t)
>  	unsigned long brightness;
>  	unsigned long delay;
>  
> +	spin_lock_bh(&leds_soft_blink_lock);
> +
> +	/*
> +	 * Check if soft blinking wasn't disabled via led_set_brightness()
> +	 * in the meantime.
> +	 */
> +	if (!test_bit(LED_BLINK_SW, &led_cdev->work_flags))
> +		goto unlock;
> +
>  	if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) {
>  		led_set_brightness_nosleep(led_cdev, LED_OFF);
>  		clear_bit(LED_BLINK_SW, &led_cdev->work_flags);
> -		return;
> +		goto unlock;
>  	}
>  
>  	if (test_and_clear_bit(LED_BLINK_ONESHOT_STOP,
>  			       &led_cdev->work_flags)) {
>  		clear_bit(LED_BLINK_SW, &led_cdev->work_flags);
> -		return;
> +		goto unlock;
>  	}
>  
>  	brightness = led_get_brightness(led_cdev);
>  	if (!brightness) {
>  		/* Time to switch the LED on. */
> -		if (test_and_clear_bit(LED_BLINK_BRIGHTNESS_CHANGE,
> -					&led_cdev->work_flags))
> -			brightness = led_cdev->new_blink_brightness;
> -		else
> -			brightness = led_cdev->blink_brightness;
> +		brightness = led_cdev->blink_brightness;
>  		delay = led_cdev->blink_delay_on;
>  	} else {
>  		/* Store the current brightness value to be able
> @@ -100,6 +108,9 @@ static void led_timer_function(struct timer_list *t)
>  	}
>  
>  	mod_timer(&led_cdev->blink_timer, jiffies + msecs_to_jiffies(delay));
> +
> +unlock:
> +	spin_unlock_bh(&leds_soft_blink_lock);
>  }
>  
>  static void set_brightness_delayed(struct work_struct *ws)
> @@ -108,11 +119,6 @@ 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_DISABLE, &led_cdev->work_flags)) {
> -		led_cdev->delayed_set_value = LED_OFF;
> -		led_stop_software_blink(led_cdev);
> -	}
> -
>  	ret = __led_set_brightness(led_cdev, led_cdev->delayed_set_value);
>  	if (ret == -ENOTSUPP)
>  		ret = __led_set_brightness_blocking(led_cdev,
> @@ -131,6 +137,8 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
>  {
>  	int current_brightness;
>  
> +	spin_lock_bh(&leds_soft_blink_lock);
> +
>  	current_brightness = led_get_brightness(led_cdev);
>  	if (current_brightness)
>  		led_cdev->blink_brightness = current_brightness;
> @@ -143,18 +151,21 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
>  	/* never on - just set to off */
>  	if (!delay_on) {
>  		led_set_brightness_nosleep(led_cdev, LED_OFF);
> -		return;
> +		goto unlock;
>  	}
>  
>  	/* never off - just set to brightness */
>  	if (!delay_off) {
>  		led_set_brightness_nosleep(led_cdev,
>  					   led_cdev->blink_brightness);
> -		return;
> +		goto unlock;
>  	}
>  
>  	set_bit(LED_BLINK_SW, &led_cdev->work_flags);
>  	mod_timer(&led_cdev->blink_timer, jiffies + 1);
> +
> +unlock:
> +	spin_unlock_bh(&leds_soft_blink_lock);
>  }
>  
>  
> @@ -217,40 +228,46 @@ void led_blink_set_oneshot(struct led_classdev *led_cdev,
>  }
>  EXPORT_SYMBOL_GPL(led_blink_set_oneshot);
>  
> -void led_stop_software_blink(struct led_classdev *led_cdev)
> +void led_stop_software_blink_nolock(struct led_classdev *led_cdev)
>  {
> -	del_timer_sync(&led_cdev->blink_timer);
>  	led_cdev->blink_delay_on = 0;
>  	led_cdev->blink_delay_off = 0;
>  	clear_bit(LED_BLINK_SW, &led_cdev->work_flags);
>  }
> +
> +void led_stop_software_blink(struct led_classdev *led_cdev)
> +{
> +	spin_lock_bh(&leds_soft_blink_lock);
> +	led_stop_software_blink_nolock(led_cdev);
> +	spin_unlock_bh(&leds_soft_blink_lock);
> +}
>  EXPORT_SYMBOL_GPL(led_stop_software_blink);
>  
>  void led_set_brightness(struct led_classdev *led_cdev,
>  			enum led_brightness brightness)
>  {
> -	/*
> -	 * If software blink is active, delay brightness setting
> -	 * until the next timer tick.
> -	 */
> +	if (in_irq()) {
> +		dev_err(led_cdev->dev,
> +			"Cannot set LED brightness from hard irq context!");
> +		WARN_ON(1);
> +		return;
> +	}
> +
> +	spin_lock_bh(&leds_soft_blink_lock);
> +
>  	if (test_bit(LED_BLINK_SW, &led_cdev->work_flags)) {
> -		/*
> -		 * If we need to disable soft blinking delegate this to the
> -		 * work queue task to avoid problems in case we are called
> -		 * from hard irq context.
> -		 */
>  		if (brightness == LED_OFF) {
> -			set_bit(LED_BLINK_DISABLE, &led_cdev->work_flags);
> -			schedule_work(&led_cdev->set_brightness_work);
> +			led_stop_software_blink_nolock(led_cdev);
>  		} else {
> -			set_bit(LED_BLINK_BRIGHTNESS_CHANGE,
> -				&led_cdev->work_flags);
> -			led_cdev->new_blink_brightness = brightness;
> +			led_cdev->blink_brightness = brightness;
> +			goto unlock;
>  		}
> -		return;
>  	}
>  
>  	led_set_brightness_nosleep(led_cdev, brightness);
> +
> +unlock:
> +	spin_unlock_bh(&leds_soft_blink_lock);
>  }
>  EXPORT_SYMBOL_GPL(led_set_brightness);
>  
> diff --git a/drivers/leds/trigger/ledtrig-activity.c b/drivers/leds/trigger/ledtrig-activity.c
> index 5081894..6f62da7 100644
> --- a/drivers/leds/trigger/ledtrig-activity.c
> +++ b/drivers/leds/trigger/ledtrig-activity.c
> @@ -48,9 +48,6 @@ static void led_activity_function(struct timer_list *t)
>  	int cpus;
>  	int i;
>  
> -	if (test_and_clear_bit(LED_BLINK_BRIGHTNESS_CHANGE, &led_cdev->work_flags))
> -		led_cdev->blink_brightness = led_cdev->new_blink_brightness;
> -
>  	if (unlikely(panic_detected)) {
>  		/* full brightness in case of panic */
>  		led_set_brightness_nosleep(led_cdev, led_cdev->blink_brightness);
> diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c b/drivers/leds/trigger/ledtrig-heartbeat.c
> index f0896de..1453352 100644
> --- a/drivers/leds/trigger/ledtrig-heartbeat.c
> +++ b/drivers/leds/trigger/ledtrig-heartbeat.c
> @@ -47,9 +47,6 @@ static void led_heartbeat_function(struct timer_list *t)
>  		return;
>  	}
>  
> -	if (test_and_clear_bit(LED_BLINK_BRIGHTNESS_CHANGE, &led_cdev->work_flags))
> -		led_cdev->blink_brightness = led_cdev->new_blink_brightness;
> -
>  	/* acts like an actual heart beat -- ie thump-thump-pause... */
>  	switch (heartbeat_data->phase) {
>  	case 0:
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 5579c64..e520503 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -51,15 +51,13 @@ struct led_classdev {
>  #define LED_BRIGHT_HW_CHANGED	BIT(21)
>  #define LED_RETAIN_AT_SHUTDOWN	BIT(22)
>  
> -	/* set_brightness_work / blink_timer flags, atomic, private. */
> +	/* blink_timer flags, atomic, private. */
>  	unsigned long		work_flags;
>  
>  #define LED_BLINK_SW			0
>  #define LED_BLINK_ONESHOT		1
>  #define LED_BLINK_ONESHOT_STOP		2
>  #define LED_BLINK_INVERT		3
> -#define LED_BLINK_BRIGHTNESS_CHANGE 	4
> -#define LED_BLINK_DISABLE		5
>  
>  	/* Set LED brightness level
>  	 * Must not sleep. Use brightness_set_blocking for drivers
> @@ -97,7 +95,6 @@ struct led_classdev {
>  	unsigned long		 blink_delay_on, blink_delay_off;
>  	struct timer_list	 blink_timer;
>  	int			 blink_brightness;
> -	int			 new_blink_brightness;
>  	void			(*flash_resume)(struct led_classdev *led_cdev);
>  
>  	struct work_struct	set_brightness_work;
>