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@xxxxxxxxx>
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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
