Re: [PATCH v2 0/3] leds: blink resolution improvements

From: Stas Sergeev
Date: Mon May 04 2015 - 13:20:52 EST


04.05.2015 18:22, Jacek Anaszewski ÐÐÑÐÑ:
On 05/04/2015 02:12 PM, Stas Sergeev wrote:
Only under that condition:
---
if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
led_cdev->delayed_set_value = brightness;
schedule_work(&led_cdev->set_brightness_work);
---

But the main condition is:
---
if (led_cdev->flags & SET_BRIGHTNESS_ASYNC) {
led_set_brightness_async(led_cdev, brightness);
---

So I think it is actually unused.
I don't see why schedule_work() above can't be just replaced
with led_set_brightness_async(). Is there the reason not to do so?
set_brightness_work not only sets the brightness but also
stops software blinking, which was the primary reason
for adding this work queue I think. Here is the commit message:
But led_trigger_set() does led_stop_software_blink(), which
IMHO means led_set_brightness() will in most cases be called
when sw blocking is already stopped. There seem to be just a
few cases where this is not true: oneshot_trig_deactivate() and
timer_trig_deactivate(), and I think I'll just change these two to
led_stop_software_blink(). I am pretty sure the work-queue is
not needed, but I'll have to test that with the patch it seems.

------------------------

leds: delay led_set_brightness if stopping soft-blink

Delay execution of led_set_brightness() if need to stop soft-blink
timer.

This allows led_set_brightness to be called in hard-irq context even if soft-blink was activated on that LED.
Instead of disabling the soft-blink beforehand, which is what led_trigger_set()
already does? I am probably missing something.

> Now your leds-aat1290 already asks for such a change,
because it can sleep but does not use a work-queue the
way other drivers do.
It doesn't need this change - it defines two ops: brightness_set
(the async one) and brightness_set_sync (the sync one). The
former is called from led_set_brightness_async and the latter
form led_set_brightness_sync.
led_set_brightness_async is called from led_set_brightness
for drivers that define SET_BRIGHTNESS_ASYNC flag and
led_set_brightness_sync for the drivers that define
SET_BRIGHTNESS_SYNC flags.

led_timer_function calls always led_set_brightness_async.
OK, I googled the patch:
https://lkml.org/lkml/2015/3/4/960
So the async one uses the work-queue, and the sync one
does not. Since led_timer_function calls always led_set_brightness_async,
it should always be using a work-queue.
But then I fail to explain your diagnostic that with my patch and
your driver, the hrtimer gives warning about a high interrupt
latency. I thought this is because your driver does sleeps and
does not use a work queue. Its not the case. Could you please
clarify, what then caused the high interrupt latency warning in
your testing?

So what should we do?
I can try the aforementioned massive clean-up with removing
the work-queue from every driver and using the one in
led-core, but my attempts have few chances to succeed
because of no test-cases. Or can you do this instead, so
that leds-aat1290 driver is in line with the others? Or any
other options I can try?

It would have to be done for the LED core and all drivers
in one patch set. We would have to get acks from all LED drivers'
authors (or at least from majority of them).

Once this is done we could think about adding optional hr timers
based triggers and invite people for testing.
As long as all drivers use the work-queue when needed and
there is no warning about high interrupt latency, I wonder if
there are some short-cuts to that route. :)
But I first need to understand where this latency came from.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/