linux-kernel - Re: [PATCH v4 1/3] led: make led_set_brightness_sync() use led_set_brightness

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] [day] [month] [year] [list]

Date:   Tue, 24 Sep 2019 15:43:21 +0200
From:   Jean-Jacques Hiblot <jjhiblot@...com>
To:     Jacek Anaszewski <jacek.anaszewski@...il.com>, <pavel@....cz>,
        <daniel.thompson@...aro.org>
CC:     <linux-leds@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <dmurphy@...com>, <tomi.valkeinen@...com>
Subject: Re: [PATCH v4 1/3] led: make led_set_brightness_sync() use
 led_set_brightness_nosleep()


On 23/09/2019 23:03, Jacek Anaszewski wrote:
> Hi Jean,
>
> On 9/23/19 11:14 AM, Jean-Jacques Hiblot wrote:
>> Hi Jacek,
>>
>> On 20/09/2019 23:10, Jacek Anaszewski wrote:
>>> Hi Jean,
>>>
>>> On 9/20/19 2:25 PM, Jean-Jacques Hiblot wrote:
>>>> Making led_set_brightness_sync() use led_set_brightness_nosleep() has 2
>>>> advantages:
>>>> - works for LED controllers that do not provide
>>>> brightness_set_blocking()
>>>> - When the blocking callback is used, it uses the workqueue to update
>>>> the
>>>>     LED state, removing the need for mutual exclusion between
>>>>     led_set_brightness_sync() and set_brightness_delayed().
>>> And third:
>>>
>>> - it compromises the "sync" part of the function name :-)
>> Making it sync is the role of the flush_work() function. It waits until
>> the deferred work has been done.
> The thing is not in the blocking character of the function, but rather
> in the fastest possible way of setting torch brightness.
> led_set_brightness_nosleep() will defer brightness_set_blocking op
> to the workqueue so this condition will not be met then.

OK. I see the point there.

>
> This function was added specifically for LED class flash v4l2 wrapper:
> drivers/media/v4l2-core/v4l2-flash-led-class.c.
>
> It may need an addition of support for brightness_set only drivers,
> but we haven't had a use case so far, since all client flash LED
> controllers are driven via blocking buses (there are not many of them).
>
> Also, when LED flash class (and thus LED class also as a parent)
> is hijacked by v4l2-flash-led wrapper, its sysfs is disabled,
> so it is not possible to set e.g. timer trigger which could
> interfere with the led_set_brightness_sync() (and it also returns
> -EBUSY when blinking is enabled).

Then this is a really special use case and we don't really have to  
worry about synchronization with the other ways to set the LED 
brightness. I'll drop any change to this function then.

Thanks for the detailed explanation.

JJ


>
>>> This function has been introduced specifically to be blocking
>>> and have the immediate effect. Its sole client is
>>> drivers/media/v4l2-core/v4l2-flash-led-class.c.
>>>
>>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@...com>
>>>> ---
>>>>    drivers/leds/led-core.c | 12 +++++++-----
>>>>    1 file changed, 7 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>>>> index f1f718dbe0f8..50e28a8f9357 100644
>>>> --- a/drivers/leds/led-core.c
>>>> +++ b/drivers/leds/led-core.c
>>>> @@ -294,15 +294,17 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep);
>>>>    int led_set_brightness_sync(struct led_classdev *led_cdev,
>>>>                    enum led_brightness value)
>>>>    {
>>>> +    int ret;
>>>> +
>>>>        if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
>>>>            return -EBUSY;
>>>>    -    led_cdev->brightness = min(value, led_cdev->max_brightness);
>>>> -
>>>> -    if (led_cdev->flags & LED_SUSPENDED)
>>>> -        return 0;
>>>> +    ret = led_set_brightness_nosleep(led_cdev, value);
>>>> +    if (!ret)
>>>> +        return ret;
>>>>    -    return __led_set_brightness_blocking(led_cdev,
>>>> led_cdev->brightness);
>>>> +    flush_work(&led_cdev->set_brightness_work);
>>>> +    return 0;
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(led_set_brightness_sync);
>>>>