Re: [PATCH] leds: make led_blink_set IRQ safe

[Hugh Dickins <hughd@google.com>]

On Tue, 19 Aug 2014, Vincent Donnefort wrote:

> This patch introduces a work which take care of reseting the blink workqueue and
> avoid calling the cancel_delayed_work_sync function which may sleep, from an IRQ
> context.
> 
> Signed-off-by: Vincent Donnefort <vdonnefort@gmail.com>

Thanks.  It does work for me.  Though the problem was more general than
stated above: not just a problem in IRQ context, but in any atomic context.

I don't suppose it has any effect on Valdis's lockdep issue, which I
didn't get to see myself; but we should let Valdis report back on that.

May I (most ungratefully!) say that your patch doesn't fill me with
confidence that it's the right solution: adding yet another work_struct
to get around the issue seemed dubious to me, I wonder if it might expose
new races.

But rest assured that I know nothing about this, and I'm not at all
qualified to review your patch: I hope Bryan and others will do so.

Thanks,
Hugh

> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 129729d..0971554 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -148,6 +148,24 @@ static void led_work_function(struct work_struct *ws)
>  			   msecs_to_jiffies(delay));
>  }
>  
> +static void reset_blink_work_delayed(struct work_struct *ws)
> +{
> +	struct led_classdev *led_cdev =
> +		container_of(ws, struct led_classdev, reset_blink_work);
> +
> +	cancel_delayed_work_sync(&led_cdev->blink_work);
> +
> +	if (!led_cdev->blink_delay_on) {
> +		__led_set_brightness(led_cdev, LED_OFF);
> +		return;
> +	} else if (!led_cdev->blink_delay_off) {
> +		__led_set_brightness(led_cdev, led_cdev->blink_brightness);
> +		return;
> +	}
> +
> +	queue_delayed_work(system_wq, &led_cdev->blink_work, 1);
> +}
> +
>  static void set_brightness_delayed(struct work_struct *ws)
>  {
>  	struct led_classdev *led_cdev =
> @@ -234,6 +252,7 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>  	INIT_WORK(&led_cdev->set_brightness_work, set_brightness_delayed);
>  
>  	INIT_DELAYED_WORK(&led_cdev->blink_work, led_work_function);
> +	INIT_WORK(&led_cdev->reset_blink_work, reset_blink_work_delayed);
>  
>  #ifdef CONFIG_LEDS_TRIGGERS
>  	led_trigger_set_default(led_cdev);
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 4bb1168..959510a 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -40,19 +40,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
>  	led_cdev->blink_delay_on = delay_on;
>  	led_cdev->blink_delay_off = delay_off;
>  
> -	/* never on - just set to off */
> -	if (!delay_on) {
> -		__led_set_brightness(led_cdev, LED_OFF);
> -		return;
> -	}
> -
> -	/* never off - just set to brightness */
> -	if (!delay_off) {
> -		__led_set_brightness(led_cdev, led_cdev->blink_brightness);
> -		return;
> -	}
> -
> -	queue_delayed_work(system_wq, &led_cdev->blink_work, 1);
> +	schedule_work(&led_cdev->reset_blink_work);
>  }
>  
>  
> @@ -76,8 +64,6 @@ void led_blink_set(struct led_classdev *led_cdev,
>  		   unsigned long *delay_on,
>  		   unsigned long *delay_off)
>  {
> -	cancel_delayed_work_sync(&led_cdev->blink_work);
> -
>  	led_cdev->flags &= ~LED_BLINK_ONESHOT;
>  	led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
>  
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 6a599dc..6e5523d 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -69,6 +69,7 @@ struct led_classdev {
>  
>  	unsigned long		 blink_delay_on, blink_delay_off;
>  	struct delayed_work	 blink_work;
> +	struct work_struct	 reset_blink_work;
>  	int			 blink_brightness;
>  
>  	struct work_struct	set_brightness_work;
> -- 
> 1.9.1
> 
>