The following sequence:
echo timer >/sys/class/leds/<name>/trigger
echo 1 >/sys/class/leds/<name>/brightness
should change the ON brightness for blinking.
The function led_set_brightness() was mistakenly initiating the
delayed blink stop procedure, which resulted in no blinking with
the timer trigger still active.
This patch fixes the problem by changing led_set_brightness()
to not initiate the delayed blink stop. The callers that need
a blink stop, were updated to explicitly call led_stop_software_blink().
CC: Bryan Wu <[email protected]>
CC: Richard Purdie <[email protected]>
CC: Jacek Anaszewski <[email protected]>
CC: Kyungmin Park <[email protected]>
CC: [email protected]
CC: [email protected]
Signed-off-by: Stas Sergeev <[email protected]>
---
drivers/leds/led-class.c | 5 +++++
drivers/leds/led-core.c | 3 +--
drivers/leds/trigger/ledtrig-oneshot.c | 1 +
drivers/leds/trigger/ledtrig-timer.c | 2 ++
4 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 795ec99..65c2c80 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -121,6 +121,11 @@ static void led_timer_function(unsigned long data)
brightness = led_get_brightness(led_cdev);
if (!brightness) {
/* Time to switch the LED on. */
+ if (led_cdev->delayed_set_value) {
+ led_cdev->blink_brightness =
+ led_cdev->delayed_set_value;
+ led_cdev->delayed_set_value = 0;
+ }
brightness = led_cdev->blink_brightness;
delay = led_cdev->blink_delay_on;
} else {
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 9886dac..3210020 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -119,10 +119,9 @@ void led_set_brightness(struct led_classdev *led_cdev,
{
int ret = 0;
- /* delay brightness setting if need to stop soft-blink timer */
+ /* delay brightness setting to next soft-blink */
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;
}
diff --git a/drivers/leds/trigger/ledtrig-oneshot.c b/drivers/leds/trigger/ledtrig-oneshot.c
index fbd02cd..c1bff10 100644
--- a/drivers/leds/trigger/ledtrig-oneshot.c
+++ b/drivers/leds/trigger/ledtrig-oneshot.c
@@ -177,6 +177,7 @@ static void oneshot_trig_deactivate(struct led_classdev *led_cdev)
}
/* Stop blinking */
+ led_stop_software_blink(led_cdev);
led_set_brightness(led_cdev, LED_OFF);
}
diff --git a/drivers/leds/trigger/ledtrig-timer.c b/drivers/leds/trigger/ledtrig-timer.c
index 8d09327..3577c23 100644
--- a/drivers/leds/trigger/ledtrig-timer.c
+++ b/drivers/leds/trigger/ledtrig-timer.c
@@ -17,6 +17,7 @@
#include <linux/device.h>
#include <linux/ctype.h>
#include <linux/leds.h>
+#include "../leds.h"
static ssize_t led_delay_on_show(struct device *dev,
struct device_attribute *attr, char *buf)
@@ -103,6 +104,7 @@ static void timer_trig_deactivate(struct led_classdev *led_cdev)
}
/* Stop blinking */
+ led_stop_software_blink(led_cdev);
led_set_brightness(led_cdev, LED_OFF);
}
--
1.7.9.5
On 05/13/2015 04:20 PM, Stas Sergeev wrote:
>
> The following sequence:
> echo timer >/sys/class/leds/<name>/trigger
> echo 1 >/sys/class/leds/<name>/brightness
> should change the ON brightness for blinking.
> The function led_set_brightness() was mistakenly initiating the
> delayed blink stop procedure, which resulted in no blinking with
> the timer trigger still active.
>
> This patch fixes the problem by changing led_set_brightness()
> to not initiate the delayed blink stop. The callers that need
> a blink stop, were updated to explicitly call led_stop_software_blink().
>
> CC: Bryan Wu <[email protected]>
> CC: Richard Purdie <[email protected]>
> CC: Jacek Anaszewski <[email protected]>
> CC: Kyungmin Park <[email protected]>
> CC: [email protected]
> CC: [email protected]
>
> Signed-off-by: Stas Sergeev <[email protected]>
> ---
> drivers/leds/led-class.c | 5 +++++
> drivers/leds/led-core.c | 3 +--
> drivers/leds/trigger/ledtrig-oneshot.c | 1 +
> drivers/leds/trigger/ledtrig-timer.c | 2 ++
> 4 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 795ec99..65c2c80 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -121,6 +121,11 @@ static void led_timer_function(unsigned long data)
> brightness = led_get_brightness(led_cdev);
> if (!brightness) {
> /* Time to switch the LED on. */
> + if (led_cdev->delayed_set_value) {
> + led_cdev->blink_brightness =
> + led_cdev->delayed_set_value;
> + led_cdev->delayed_set_value = 0;
> + }
> brightness = led_cdev->blink_brightness;
> delay = led_cdev->blink_delay_on;
> } else {
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 9886dac..3210020 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -119,10 +119,9 @@ void led_set_brightness(struct led_classdev *led_cdev,
> {
> int ret = 0;
>
> - /* delay brightness setting if need to stop soft-blink timer */
> + /* delay brightness setting to next soft-blink */
How about:
/* delay brightness setting if soft-blink is activated */
> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
> led_cdev->delayed_set_value = brightness;
> - schedule_work(&led_cdev->set_brightness_work);
This line is still required. Please refer to the patch d23a22a74.
The author however mistakenly assumes that setting brightness while
soft blink is activated should result in disabling the trigger. This is
contrary to the documentation, which says:
"You can change the brightness value of a LED independently of the timer
trigger. However, if you set the brightness value to LED_OFF it will
also disable the timer trigger."
What needs tweaking here is set_brightness_delayed function, which
should call led_stop_software_blink only if delayed_set_value is 0.
Please fix the patch accordingly.
> return;
> }
>
> diff --git a/drivers/leds/trigger/ledtrig-oneshot.c b/drivers/leds/trigger/ledtrig-oneshot.c
> index fbd02cd..c1bff10 100644
> --- a/drivers/leds/trigger/ledtrig-oneshot.c
> +++ b/drivers/leds/trigger/ledtrig-oneshot.c
> @@ -177,6 +177,7 @@ static void oneshot_trig_deactivate(struct led_classdev *led_cdev)
> }
>
> /* Stop blinking */
> + led_stop_software_blink(led_cdev);
This won't be needed.
> led_set_brightness(led_cdev, LED_OFF);
> }
>
> diff --git a/drivers/leds/trigger/ledtrig-timer.c b/drivers/leds/trigger/ledtrig-timer.c
> index 8d09327..3577c23 100644
> --- a/drivers/leds/trigger/ledtrig-timer.c
> +++ b/drivers/leds/trigger/ledtrig-timer.c
> @@ -17,6 +17,7 @@
> #include <linux/device.h>
> #include <linux/ctype.h>
> #include <linux/leds.h>
> +#include "../leds.h"
>
> static ssize_t led_delay_on_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> @@ -103,6 +104,7 @@ static void timer_trig_deactivate(struct led_classdev *led_cdev)
> }
>
> /* Stop blinking */
> + led_stop_software_blink(led_cdev);
This won't be needed.
> led_set_brightness(led_cdev, LED_OFF);
> }
>
--
Best Regards,
Jacek Anaszewski
14.05.2015 11:44, Jacek Anaszewski пишет:
>
>> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
>> led_cdev->delayed_set_value = brightness;
>> - schedule_work(&led_cdev->set_brightness_work);
>
> This line is still required. Please refer to the patch d23a22a74.
But I really want to get rid of this line and
vacate the work-queue, so that in the future the
per-driver work-queues can be replaced with this.
Why do you think it is still needed?
Are there any valid cases where you need to disable the
soft-blink from hard-irq context? IMHO the soft-blink is
only disabled as a result of user's actions, which is not
in a hard-irq context.
Could you please explain the use-case?
>> /* Stop blinking */
>> + led_stop_software_blink(led_cdev);
> This won't be needed.
Indeed, but with these changes there should be no requirement
for disabling a soft-blink from hard-irq context, which is what
I really wanted to have. What am I missing?
On 05/14/2015 11:45 AM, Stas Sergeev wrote:
> 14.05.2015 11:44, Jacek Anaszewski пишет:
>>
>>> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
>>> led_cdev->delayed_set_value = brightness;
>>> - schedule_work(&led_cdev->set_brightness_work);
>>
>> This line is still required. Please refer to the patch d23a22a74.
> But I really want to get rid of this line and
> vacate the work-queue, so that in the future the
> per-driver work-queues can be replaced with this.
set_brightness_delayed functions can be used for this as well.
It would only need to be extended.
> Why do you think it is still needed?
> Are there any valid cases where you need to disable the
> soft-blink from hard-irq context? IMHO the soft-blink is
> only disabled as a result of user's actions, which is not
> in a hard-irq context.
> Could you please explain the use-case?
>
>>> /* Stop blinking */
>>> + led_stop_software_blink(led_cdev);
>> This won't be needed.
> Indeed, but with these changes there should be no requirement
> for disabling a soft-blink from hard-irq context, which is what
> I really wanted to have. What am I missing?
>
Please look at this [1]. Author mentions setting brightness
from sound-card irq handler.
[1] http://www.spinics.net/lists/linux-leds/msg00006.html
--
Best Regards,
Jacek Anaszewski
14.05.2015 13:33, Jacek Anaszewski пишет:
>> Indeed, but with these changes there should be no requirement
>> for disabling a soft-blink from hard-irq context, which is what
>> I really wanted to have. What am I missing?
> Please look at this [1]. Author mentions setting brightness
> from sound-card irq handler.
>
> [1] http://www.spinics.net/lists/linux-leds/msg00006.html
He points to the following (out-of-tree??) code:
---
if ((jiffies / HZ / 2) & 1)
led_trigger_blink_oneshot(ledtrig_ide,
&ide_blink_delay, &ide_blink_delay, 0);
if ((jiffies / HZ / 4) & 1)
led_trigger_event(ledtrig_ide, 100);
if ((jiffies / HZ / 8) & 1)
led_trigger_event(ledtrig_ide, 0);
---
I think the problem was that oneshot_trig_deactivate() was
not doing led_stop_software_blink(led_cdev), and so he needed
a work-queue for switching out from oneshot trigger.
My patch fixes exactly that: now oneshot trigger does the
proper cleanup itself.
Do you think my patch is not enough to handle this case?
14.05.2015 14:03, Stas Sergeev пишет:
> 14.05.2015 13:33, Jacek Anaszewski пишет:
>>> Indeed, but with these changes there should be no requirement
>>> for disabling a soft-blink from hard-irq context, which is what
>>> I really wanted to have. What am I missing?
>> Please look at this [1]. Author mentions setting brightness
>> from sound-card irq handler.
>>
>> [1] http://www.spinics.net/lists/linux-leds/msg00006.html
> He points to the following (out-of-tree??) code:
> ---
> if ((jiffies / HZ / 2) & 1)
> led_trigger_blink_oneshot(ledtrig_ide,
> &ide_blink_delay, &ide_blink_delay, 0);
> if ((jiffies / HZ / 4) & 1)
> led_trigger_event(ledtrig_ide, 100);
> if ((jiffies / HZ / 8) & 1)
> led_trigger_event(ledtrig_ide, 0);
> ---
> I think the problem was that oneshot_trig_deactivate() was
> not doing led_stop_software_blink(led_cdev), and so he needed
> a work-queue for switching out from oneshot trigger.
> My patch fixes exactly that: now oneshot trigger does the
> proper cleanup itself.
> Do you think my patch is not enough to handle this case?
Likely not, if he switches triggers in hard-irq context.
Though I wonder if it is important to care about the out-of-tree
code that could use the work-queue itself?
Anyway, I think I should split the patch to the fix and
the cleanup, to make the life easier.
On 05/14/2015 01:03 PM, Stas Sergeev wrote:
> 14.05.2015 13:33, Jacek Anaszewski пишет:
>>> Indeed, but with these changes there should be no requirement
>>> for disabling a soft-blink from hard-irq context, which is what
>>> I really wanted to have. What am I missing?
>> Please look at this [1]. Author mentions setting brightness
>> from sound-card irq handler.
>>
>> [1] http://www.spinics.net/lists/linux-leds/msg00006.html
> He points to the following (out-of-tree??) code:
> ---
> if ((jiffies / HZ / 2) & 1)
> led_trigger_blink_oneshot(ledtrig_ide,
> &ide_blink_delay, &ide_blink_delay, 0);
> if ((jiffies / HZ / 4) & 1)
> led_trigger_event(ledtrig_ide, 100);
> if ((jiffies / HZ / 8) & 1)
> led_trigger_event(ledtrig_ide, 0);
> ---
> I think the problem was that oneshot_trig_deactivate() was
> not doing led_stop_software_blink(led_cdev), and so he needed
> a work-queue for switching out from oneshot trigger.
> My patch fixes exactly that: now oneshot trigger does the
> proper cleanup itself.
> Do you think my patch is not enough to handle this case?
>
This problem is related to the issue of locking between hard IRQ
and Softirqs [1]. I've just discovered also that there had been already
an attempt [2] of using work queue internally in led_set_brightness,
but it wouldn't have found its way to the mainline. Probably for a
reason.
[1]
https://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/c188.html
[2] http://www.serverphorums.com/read.php?12,563432
--
Best Regards,
Jacek Anaszewski