From: Fabio Baltieri <fabio.baltieri@gmail.com>
To: Bryan Wu <bryan.wu@canonical.com>
Cc: rpurdie@rpsys.net, linux-kernel@vger.kernel.org,
akpm@linux-foundation.org, linux-leds@vger.kernel.org,
broonie@opensource.wolfsonmicro.com, shuahkhan@gmail.com,
raph@8d.com, tpiepho@freescale.com
Subject: Re: [PATCH 1/2] leds: use workqueue in led_set_brightness() API internally
Date: Sat, 15 Sep 2012 17:23:03 +0200 [thread overview]
Message-ID: <20120915152303.GA1434@gmail.com> (raw)
In-Reply-To: <1347609183-26730-2-git-send-email-bryan.wu@canonical.com>
Hello Bryan,
On Fri, Sep 14, 2012 at 03:53:02PM +0800, Bryan Wu wrote:
> The API function led_set_brightness() and __led_set_brightness will
> call .brightness_set() function provided by led class drivers. So
> .brightness_set() function will run in atomic context, which requires
> led class drivers use workqueue in .brightness_set().
>
> Finally, all the led class driver implemented their own workqueue in
> .brightness_set(). For those missing this, we will face runtime warning
> or error when running it in atomic context.
>
> This patch adds a workqueue in led_set_brightness() internally. LED
> class driver doesn't need to care about duplicated workqueue stuff in
> their own driver.
>
> Also this patch unified the led_set_brightness() and __led_set_brightness()
I like the idea of a semplification of led driver's code and the removal
of the confusion between the two variants of led_set_brightness.
Despite that, due to the lockless nature of the soft-blink code in the
led subsystem I expect it to break quite easility with this kind of
modification.
> Signed-off-by: Bryan Wu <bryan.wu@canonical.com>
> ---
> drivers/leds/led-class.c | 23 ++++++++++++-----------
> drivers/leds/led-core.c | 15 +++++++--------
> drivers/leds/leds.h | 11 ++---------
> drivers/leds/ledtrig-backlight.c | 8 ++++----
> drivers/leds/ledtrig-default-on.c | 2 +-
> drivers/leds/ledtrig-gpio.c | 6 +++---
> drivers/leds/ledtrig-heartbeat.c | 2 +-
> drivers/leds/ledtrig-oneshot.c | 4 ++--
> drivers/leds/ledtrig-transient.c | 8 ++++----
> include/linux/leds.h | 12 +++++++-----
> 10 files changed, 43 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 48cce18..7a3e886 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -53,7 +53,7 @@ static ssize_t led_brightness_store(struct device *dev,
>
> if (state == LED_OFF)
> led_trigger_remove(led_cdev);
> - __led_set_brightness(led_cdev, state);
> + led_set_brightness(led_cdev, state);
>
> return size;
> }
> @@ -82,7 +82,7 @@ static void led_timer_function(unsigned long data)
> unsigned long delay;
>
> if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) {
> - __led_set_brightness(led_cdev, LED_OFF);
> + led_set_brightness(led_cdev, LED_OFF);
> return;
> }
>
> @@ -105,7 +105,7 @@ static void led_timer_function(unsigned long data)
> delay = led_cdev->blink_delay_off;
> }
>
> - __led_set_brightness(led_cdev, brightness);
> + led_set_brightness(led_cdev, brightness);
>
> /* Return in next iteration if led is in one-shot mode and we are in
> * the final blink state so that the led is toggled each delay_on +
> @@ -124,14 +124,15 @@ static void led_timer_function(unsigned long data)
> mod_timer(&led_cdev->blink_timer, jiffies + msecs_to_jiffies(delay));
> }
>
> -static void set_brightness_delayed(struct work_struct *ws)
> +static void led_set_brightness_work(struct work_struct *ws)
> {
> struct led_classdev *led_cdev =
> container_of(ws, struct led_classdev, set_brightness_work);
>
> - led_stop_software_blink(led_cdev);
> + if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
> + led_stop_software_blink(led_cdev);
This actually breaks soft-blink, as led_set_brightness_work is called by
the soft-blink code itself when soft blink is active. You may probabily
want replicate the actual set code in led_timer_function() or to keep it
in an internal function.
One of the effects is that blink_delay_{on,off} are erroneously set to 0
when setting 'timer' or 'oneshot' as current trigger.
> - __led_set_brightness(led_cdev, led_cdev->delayed_set_value);
> + led_cdev->brightness_set(led_cdev, led_cdev->brightness);
Is it ok to access led_cdev->brightness without locks? I'm afraid that
this may race in SMP if another CPU is running led_set_brightness.
The lockless solution would be to make sure the work is canceled before
setting the new value, through that may not be kind to hardirq context.
> }
>
> /**
> @@ -141,7 +142,7 @@ static void set_brightness_delayed(struct work_struct *ws)
> void led_classdev_suspend(struct led_classdev *led_cdev)
> {
> led_cdev->flags |= LED_SUSPENDED;
> - led_cdev->brightness_set(led_cdev, 0);
> + led_set_brightness(led_cdev, 0);
> }
> EXPORT_SYMBOL_GPL(led_classdev_suspend);
>
> @@ -151,7 +152,7 @@ EXPORT_SYMBOL_GPL(led_classdev_suspend);
> */
> void led_classdev_resume(struct led_classdev *led_cdev)
> {
> - led_cdev->brightness_set(led_cdev, led_cdev->brightness);
> + led_set_brightness(led_cdev, led_cdev->brightness);
> led_cdev->flags &= ~LED_SUSPENDED;
> }
> EXPORT_SYMBOL_GPL(led_classdev_resume);
> @@ -201,7 +202,7 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>
> led_update_brightness(led_cdev);
>
> - INIT_WORK(&led_cdev->set_brightness_work, set_brightness_delayed);
> + INIT_WORK(&led_cdev->set_brightness_work, led_set_brightness_work);
>
> init_timer(&led_cdev->blink_timer);
> led_cdev->blink_timer.function = led_timer_function;
> @@ -233,12 +234,12 @@ 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);
> +
Is it possible that this code runs with led_set_brightness_work()
already scheduled on another CPU? In that case flush_work would not
wait for it.
Fabio
> device_unregister(led_cdev->dev);
>
> down_write(&leds_list_lock);
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index ce8921a..c212262 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -45,7 +45,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
>
> /* never off - just set to brightness */
> if (!delay_off) {
> - __led_set_brightness(led_cdev, led_cdev->blink_brightness);
> + led_set_brightness(led_cdev, led_cdev->blink_brightness);
> return;
> }
>
> @@ -114,13 +114,12 @@ EXPORT_SYMBOL_GPL(led_stop_software_blink);
> void led_set_brightness(struct led_classdev *led_cdev,
> enum led_brightness brightness)
> {
> - /* delay brightness setting if need to stop soft-blink timer */
> - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
> - led_cdev->delayed_set_value = brightness;
> - schedule_work(&led_cdev->set_brightness_work);
> - return;
> - }
> + if (brightness > led_cdev->max_brightness)
> + brightness = led_cdev->max_brightness;
>
> - __led_set_brightness(led_cdev, brightness);
> + led_cdev->brightness = brightness;
> +
> + if (!(led_cdev->flags & LED_SUSPENDED))
> + schedule_work(&led_cdev->set_brightness_work);
> }
> EXPORT_SYMBOL(led_set_brightness);
> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> index 4c50365..745bf06 100644
> --- a/drivers/leds/leds.h
> +++ b/drivers/leds/leds.h
> @@ -17,15 +17,8 @@
> #include <linux/rwsem.h>
> #include <linux/leds.h>
>
> -static inline void __led_set_brightness(struct led_classdev *led_cdev,
> - enum led_brightness value)
> -{
> - if (value > led_cdev->max_brightness)
> - value = led_cdev->max_brightness;
> - led_cdev->brightness = value;
> - if (!(led_cdev->flags & LED_SUSPENDED))
> - led_cdev->brightness_set(led_cdev, value);
> -}
> +void led_set_brightness(struct led_classdev *led_cdev,
> + enum led_brightness value);
>
> static inline int led_get_brightness(struct led_classdev *led_cdev)
> {
> diff --git a/drivers/leds/ledtrig-backlight.c b/drivers/leds/ledtrig-backlight.c
> index b941685..e272686 100644
> --- a/drivers/leds/ledtrig-backlight.c
> +++ b/drivers/leds/ledtrig-backlight.c
> @@ -46,9 +46,9 @@ static int fb_notifier_callback(struct notifier_block *p,
>
> if ((n->old_status == UNBLANK) ^ n->invert) {
> n->brightness = led->brightness;
> - __led_set_brightness(led, LED_OFF);
> + led_set_brightness(led, LED_OFF);
> } else {
> - __led_set_brightness(led, n->brightness);
> + led_set_brightness(led, n->brightness);
> }
>
> n->old_status = new_status;
> @@ -87,9 +87,9 @@ static ssize_t bl_trig_invert_store(struct device *dev,
>
> /* After inverting, we need to update the LED. */
> if ((n->old_status == BLANK) ^ n->invert)
> - __led_set_brightness(led, LED_OFF);
> + led_set_brightness(led, LED_OFF);
> else
> - __led_set_brightness(led, n->brightness);
> + led_set_brightness(led, n->brightness);
>
> return num;
> }
> diff --git a/drivers/leds/ledtrig-default-on.c b/drivers/leds/ledtrig-default-on.c
> index eac1f1b..a4ef54b 100644
> --- a/drivers/leds/ledtrig-default-on.c
> +++ b/drivers/leds/ledtrig-default-on.c
> @@ -19,7 +19,7 @@
>
> static void defon_trig_activate(struct led_classdev *led_cdev)
> {
> - __led_set_brightness(led_cdev, led_cdev->max_brightness);
> + led_set_brightness(led_cdev, led_cdev->max_brightness);
> }
>
> static struct led_trigger defon_led_trigger = {
> diff --git a/drivers/leds/ledtrig-gpio.c b/drivers/leds/ledtrig-gpio.c
> index ba215dc..f057c10 100644
> --- a/drivers/leds/ledtrig-gpio.c
> +++ b/drivers/leds/ledtrig-gpio.c
> @@ -54,12 +54,12 @@ static void gpio_trig_work(struct work_struct *work)
>
> if (tmp) {
> if (gpio_data->desired_brightness)
> - __led_set_brightness(gpio_data->led,
> + led_set_brightness(gpio_data->led,
> gpio_data->desired_brightness);
> else
> - __led_set_brightness(gpio_data->led, LED_FULL);
> + led_set_brightness(gpio_data->led, LED_FULL);
> } else {
> - __led_set_brightness(gpio_data->led, LED_OFF);
> + led_set_brightness(gpio_data->led, LED_OFF);
> }
> }
>
> diff --git a/drivers/leds/ledtrig-heartbeat.c b/drivers/leds/ledtrig-heartbeat.c
> index 1edc746..a019fbb 100644
> --- a/drivers/leds/ledtrig-heartbeat.c
> +++ b/drivers/leds/ledtrig-heartbeat.c
> @@ -74,7 +74,7 @@ static void led_heartbeat_function(unsigned long data)
> break;
> }
>
> - __led_set_brightness(led_cdev, brightness);
> + led_set_brightness(led_cdev, brightness);
> mod_timer(&heartbeat_data->timer, jiffies + delay);
> }
>
> diff --git a/drivers/leds/ledtrig-oneshot.c b/drivers/leds/ledtrig-oneshot.c
> index 2c029aa..2b10b28 100644
> --- a/drivers/leds/ledtrig-oneshot.c
> +++ b/drivers/leds/ledtrig-oneshot.c
> @@ -63,9 +63,9 @@ static ssize_t led_invert_store(struct device *dev,
> oneshot_data->invert = !!state;
>
> if (oneshot_data->invert)
> - __led_set_brightness(led_cdev, LED_FULL);
> + led_set_brightness(led_cdev, LED_FULL);
> else
> - __led_set_brightness(led_cdev, LED_OFF);
> + led_set_brightness(led_cdev, LED_OFF);
>
> return size;
> }
> diff --git a/drivers/leds/ledtrig-transient.c b/drivers/leds/ledtrig-transient.c
> index 398f104..83179f4 100644
> --- a/drivers/leds/ledtrig-transient.c
> +++ b/drivers/leds/ledtrig-transient.c
> @@ -41,7 +41,7 @@ static void transient_timer_function(unsigned long data)
> struct transient_trig_data *transient_data = led_cdev->trigger_data;
>
> transient_data->activate = 0;
> - __led_set_brightness(led_cdev, transient_data->restore_state);
> + led_set_brightness(led_cdev, transient_data->restore_state);
> }
>
> static ssize_t transient_activate_show(struct device *dev,
> @@ -72,7 +72,7 @@ static ssize_t transient_activate_store(struct device *dev,
> if (state == 0 && transient_data->activate == 1) {
> del_timer(&transient_data->timer);
> transient_data->activate = state;
> - __led_set_brightness(led_cdev, transient_data->restore_state);
> + led_set_brightness(led_cdev, transient_data->restore_state);
> return size;
> }
>
> @@ -80,7 +80,7 @@ static ssize_t transient_activate_store(struct device *dev,
> if (state == 1 && transient_data->activate == 0 &&
> transient_data->duration != 0) {
> transient_data->activate = state;
> - __led_set_brightness(led_cdev, transient_data->state);
> + led_set_brightness(led_cdev, transient_data->state);
> transient_data->restore_state =
> (transient_data->state == LED_FULL) ? LED_OFF : LED_FULL;
> mod_timer(&transient_data->timer,
> @@ -203,7 +203,7 @@ static void transient_trig_deactivate(struct led_classdev *led_cdev)
>
> if (led_cdev->activated) {
> del_timer_sync(&transient_data->timer);
> - __led_set_brightness(led_cdev, transient_data->restore_state);
> + led_set_brightness(led_cdev, transient_data->restore_state);
> device_remove_file(led_cdev->dev, &dev_attr_activate);
> device_remove_file(led_cdev->dev, &dev_attr_duration);
> device_remove_file(led_cdev->dev, &dev_attr_state);
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 5676197..d7f5697 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -43,10 +43,15 @@ struct led_classdev {
> #define LED_BLINK_ONESHOT_STOP (1 << 18)
> #define LED_BLINK_INVERT (1 << 19)
>
> - /* Set LED brightness level */
> - /* Must not sleep, use a workqueue if needed */
> + /*
> + * Set LED brightness level
> + * use a workqueue internally, so driver writer doesn't need to
> + * care about using duplicated workqueue in brightness_set()
> + */
> void (*brightness_set)(struct led_classdev *led_cdev,
> enum led_brightness brightness);
> + struct work_struct set_brightness_work;
> +
> /* Get LED brightness level */
> enum led_brightness (*brightness_get)(struct led_classdev *led_cdev);
>
> @@ -70,9 +75,6 @@ struct led_classdev {
> struct timer_list blink_timer;
> int blink_brightness;
>
> - struct work_struct set_brightness_work;
> - int delayed_set_value;
> -
> #ifdef CONFIG_LEDS_TRIGGERS
> /* Protects the trigger data below */
> struct rw_semaphore trigger_lock;
> --
> 1.7.11.4
>
--
Fabio Baltieri
next prev parent reply other threads:[~2012-09-15 15:23 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-09-14 7:53 [RFC] [PATCH 0/2] leds: use workqueue in led_set_brightness() API internally Bryan Wu
2012-09-14 7:53 ` [PATCH 1/2] " Bryan Wu
2012-09-14 22:25 ` Shuah Khan
2012-09-15 15:23 ` Fabio Baltieri [this message]
2012-09-14 7:53 ` [PATCH 2/2] leds-gpio: remove workqueue in .brightness_set() Bryan Wu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120915152303.GA1434@gmail.com \
--to=fabio.baltieri@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=broonie@opensource.wolfsonmicro.com \
--cc=bryan.wu@canonical.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=raph@8d.com \
--cc=rpurdie@rpsys.net \
--cc=shuahkhan@gmail.com \
--cc=tpiepho@freescale.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.