Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1273787 > unrolled thread

[PATCH] leds: turn off the LED and wait for completion on unregistering LED class device

Back to article view | Back to linux.kernel

Contents

  [PATCH] leds: turn off the LED and wait for completion on unregistering LED class device Milo Kim <milo.kim@ti.com> - 2015-11-20 09:10 +0100
    Re: [PATCH] leds: turn off the LED and wait for completion on  unregistering LED class device Jacek Anaszewski <j.anaszewski@samsung.com> - 2015-11-20 10:00 +0100

#1273787 — [PATCH] leds: turn off the LED and wait for completion on unregistering LED class device

Workqueue, 'set_brightness_work' is used for scheduling brightness control.
This workqueue is canceled when the LED class device is unregistered.
Currently, LED subsystem handles like below.

  cancel_work_sync(&led_cdev->set_brightness_work)
  led_set_brightness(led_cdev, LED_OFF)

However, this could be a problem.
Workqueue is going to be canceled but LED device needs to be off.
The worst case is null pointer access due to scheduling a workqueue.

LED module is loaded.
  LED driver private data is allocated by using devm_zalloc().

LED module is unloaded.
  led_classdev_unregister() is called.
    cancel_work_sync()
      led_set_brightness(led_cdev, LED_OFF)
        schedule_work() if LED driver uses brightness_set_blocking()
        In the meantime, driver private data will be freed.

        ..scheduling..

        brightness_set_blocking() callback is invoked.
          For the brightness control, LED driver tries to access private
          data but resource is removed!

To avoid this problem, LED subsystem should turn off the brightness first
and wait for completion.

  led_set_brightness(led_cdev, LED_OFF)
  flush_work(&led_cdev->set_brightness_work)

It guarantees that LED driver turns off the brightness prior to
resource management.

Cc: linux-leds@vger.kernel.org
Cc: linux-kernel@vger.kernel.org 
Signed-off-by: Milo Kim <milo.kim@ti.com>
---
 drivers/leds/led-class.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index d946991..14139c3 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -245,12 +245,13 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
 	up_write(&led_cdev->trigger_lock);
 #endif
 
-	cancel_work_sync(&led_cdev->set_brightness_work);
-
 	/* Stop blinking */
 	led_stop_software_blink(led_cdev);
+
 	led_set_brightness(led_cdev, LED_OFF);
 
+	flush_work(&led_cdev->set_brightness_work);
+
 	device_unregister(led_cdev->dev);
 
 	down_write(&leds_list_lock);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1273868 — Re: [PATCH] leds: turn off the LED and wait for completion on unregistering LED class device

Hi Milo,

Thanks for catching this, applied.

Best Regards,
Jacek Anaszewski

On 11/20/2015 09:03 AM, Milo Kim wrote:
> Workqueue, 'set_brightness_work' is used for scheduling brightness control.
> This workqueue is canceled when the LED class device is unregistered.
> Currently, LED subsystem handles like below.
>
>    cancel_work_sync(&led_cdev->set_brightness_work)
>    led_set_brightness(led_cdev, LED_OFF)
>
> However, this could be a problem.
> Workqueue is going to be canceled but LED device needs to be off.
> The worst case is null pointer access due to scheduling a workqueue.
>
> LED module is loaded.
>    LED driver private data is allocated by using devm_zalloc().
>
> LED module is unloaded.
>    led_classdev_unregister() is called.
>      cancel_work_sync()
>        led_set_brightness(led_cdev, LED_OFF)
>          schedule_work() if LED driver uses brightness_set_blocking()
>          In the meantime, driver private data will be freed.
>
>          ..scheduling..
>
>          brightness_set_blocking() callback is invoked.
>            For the brightness control, LED driver tries to access private
>            data but resource is removed!
>
> To avoid this problem, LED subsystem should turn off the brightness first
> and wait for completion.
>
>    led_set_brightness(led_cdev, LED_OFF)
>    flush_work(&led_cdev->set_brightness_work)
>
> It guarantees that LED driver turns off the brightness prior to
> resource management.
>
> Cc: linux-leds@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Milo Kim <milo.kim@ti.com>
> ---
>   drivers/leds/led-class.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index d946991..14139c3 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -245,12 +245,13 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
>   	up_write(&led_cdev->trigger_lock);
>   #endif
>
> -	cancel_work_sync(&led_cdev->set_brightness_work);
> -
>   	/* Stop blinking */
>   	led_stop_software_blink(led_cdev);
> +
>   	led_set_brightness(led_cdev, LED_OFF);
>
> +	flush_work(&led_cdev->set_brightness_work);
> +
>   	device_unregister(led_cdev->dev);
>
>   	down_write(&leds_list_lock);
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web