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 09:55:00 +0200
From:	Jacek Anaszewski <j.anaszewski@...sung.com>
To:	Stas Sergeev <stsp@...t.ru>
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

On 04/30/2015 07:11 PM, Stas Sergeev wrote:
> 28.04.2015 15:58, Jacek Anaszewski пишет:
>>>> I tried it with Samsung M0 board and
>>>> my leds-aat1290 driver. It didn't work well. And for small delay
>>>> intervals it will not have a chance to work reliably with all drivers,
>>>> especially the ones which use mutex in their brightness_set op,
>>>> since mutex can sleep.
>>> OK, I can remove the nsec resolution.
>>
>> usec also didn't work, please look at my use case and warning:
>>
>> echo "timer" > trigger
>> echo 1 > delay_on
>> echo 1 > delay_off
>> echo usec > delay_unit
>> [  178.584433] hrtimer: interrupt took 300747 ns
>>
>> Only some time later I realized that for AAT1290 brightness is set
>> through ASCwire pulse protocol, which takes few ms.
>>
>> Please note that with this approach users would have to wonder why
>> they are getting the warnings and why they can't get their LEDs to work
>> with given settings.
> I've now found that the drivers itself use a work queue
> when needed. And some drivers, like leds_gpio, even do this:
> ---
>          if (led_dat->can_sleep) {
>                  led_dat->new_level = level;
>                  schedule_work(&led_dat->work);
>          } else {
>                  set_brightness_now();
>          }
> ---

This is to handle GPIO expander chips, for which gpio set/get functions
must sleep while waiting for I2C or SPI transfer completion.

> 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.

> 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.

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.

I am becoming reluctant towards the whole idea, as we will be
unable to guarantee the stability of a delay interval.

-- 
Best Regards,
Jacek Anaszewski
--
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/