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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 04 May 2015 15:12:55 +0300
From:	Stas Sergeev <stsp@...t.ru>
To:	Jacek Anaszewski <j.anaszewski@...sung.com>
CC:	linux-leds@...r.kernel.org,
	Linux kernel <linux-kernel@...r.kernel.org>,
	Stas Sergeev <stsp@...rs.sourceforge.net>
Subject: Re: [PATCH v2 0/3] leds: blink resolution improvements

04.05.2015 10:55, Jacek Anaszewski пишет:
>> So it seems the problem is already solved on the per-driver
>> basis. I don't have leds-aat1290 driver, it is probably not
>> in the kernel.
> It is currently on linux-next/master branch.
So that driver is not in line with others.

>> It is likely forgetting to use the work-queue
>> the way all other drivers do. So I think my patch is good for
>> the in-kernel drivers.
>>
>> There is also a led_cdev->set_brightness_work, and it looks
>> unused. I could use it for my patch, but for what, if the
>> drivers already use the work queue when needed?
> It is used in led_set_brightness function.
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?

> I think that using work queues would compromise the whole idea of
> introducing intervals less than 1ms. After the task is delegated to
> work queue we are losing the control over the moment when it will get
> executed.
No one is going to allow sub-ms intervals when work-queue
is used. The proper solution would be to use work-queue for
drivers that can sleep, and allow sub-ms resolution for others.
Fortunately the drivers seem to already have that information
internally, and use work-queue on their own when needed.
leds-aat1290 may be an exception from that.

> I am becoming reluctant towards the whole idea, as we will be
> unable to guarantee the stability of a delay interval.
So why are you against the idea of improving the precision,
rather than against the code that prevents us from doing so?
The per-driver work queue use can be moved to led-core,
and the precise intervals can be guaranteed for the drivers
that do not need work queue.
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.
If I do such a massive change, I will certainly not be able
to properly test it, while you have a good test-case and
even a driver that needs such a change anyway. So I don't
see the point of being against that.

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