linux-kernel - Re: [PATCH v3 2/3] leds: Add control of the voltage/current regulator to the LED core

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:   Fri, 20 Sep 2019 14:29:58 +0200
From:   Jean-Jacques Hiblot <jjhiblot@...com>
To:     Jacek Anaszewski <jacek.anaszewski@...il.com>, <pavel@....cz>,
        <robh+dt@...nel.org>, <mark.rutland@....com>,
        <daniel.thompson@...aro.org>
CC:     <dmurphy@...com>, <linux-leds@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>
Subject: Re: [PATCH v3 2/3] leds: Add control of the voltage/current regulator
 to the LED core

Hi Jacek,

On 18/07/2019 19:49, Jacek Anaszewski wrote:
> On 7/18/19 3:31 PM, Jean-Jacques Hiblot wrote:
>> On 18/07/2019 14:24, Jacek Anaszewski wrote:
>>> Hi Jean,
>>>
>>> Thank you for the updated patch set.
>>>
>>> I have some more comments below.
>>>
>>> On 7/17/19 3:59 PM, Jean-Jacques Hiblot wrote:
>>>>    +static bool __led_need_regulator_update(struct led_classdev
>>>> *led_cdev,
>>>> +                    int brightness)
>>>> +{
>>>> +    bool new_state = (brightness != LED_OFF);
>>> How about:
>>>
>>> bool new_state = !!brightness;
>> Throughout the code LED_OFF is used when the LED is turned off. I think
>> it would be more consistent to use it there too.
> Basically brightness is a scalar and 0 always means off.
> We treat enum led_brightness as a legacy type - it is no
> longer valid on the whole its span since LED_FULL = 255
> was depreciated with addition of max_brightness property.
>
> IMHO use of reverse logic here only hinders code analysis.
>
>>>> +
>>>> +    return led_cdev->regulator && led_cdev->regulator_state !=
>>>> new_state;
>>>> +}
>>>> +static int __led_handle_regulator(struct led_classdev *led_cdev,
>>>> +                int brightness)
>>>> +{
>>>> +    int rc;
>>>> +
>>>> +    if (__led_need_regulator_update(led_cdev, brightness)) {
>>>> +
>>>> +        if (brightness != LED_OFF)
>>>> +            rc = regulator_enable(led_cdev->regulator);
>>>> +        else
>>>> +            rc = regulator_disable(led_cdev->regulator);
>>>> +        if (rc)
>>>> +            return rc;
>>>> +
>>>> +        led_cdev->regulator_state = (brightness != LED_OFF);
>>>> +    }
>>>> +    return 0;
>>>> +}
>>> Let's have these function names without leading underscores.
>> OK.
>>>>    static int __led_set_brightness(struct led_classdev *led_cdev,
>>>>                    enum led_brightness value)
>>>>    {
>>>> @@ -115,6 +142,8 @@ static void set_brightness_delayed(struct
>>>> work_struct *ws)
>>>>        if (ret == -ENOTSUPP)
>>>>            ret = __led_set_brightness_blocking(led_cdev,
>>>>                        led_cdev->delayed_set_value);
>>>> +    __led_handle_regulator(led_cdev, led_cdev->delayed_set_value)
>>> If you called it from __led_set_brightness() and
>> We cannot call it from __led_set_brightness() because it is supposed not
>> to block.
> You're right. The problematic part is that with regulator handling
> we cannot treat the whole brightness setting operation uniformly
> for brightness_set op case, i.e. without mediation of a workqueue.
>
> Now you have to fire workqueue in led_set_brightness_nopm()
> even for brightness_set() op path, if regulator state needs update.
> This is ugly and can be misleading. Can be also error prone and
> have non-obvious implications for software blink state transitions.

Taking your queue I reworked the series to take better care of the 
concurrency issues.

I believe it's in better shape right now.

>
> I think we would first need to improve locking between the workqueue
> and led_timer_function(). I proposed a patch [0] over a year
> ago.

I tried the patch and get a lot of warning because of triggers on 
storage devices.

Making led_set_brightness() not callable from a IRQ context, is probably 
not the right approach anymore.


JJ

>
> Only then we could think of adding another asynchronous dependency
> to the brightness setting chain.
>
> [0] https://lkml.org/lkml/2018/1/17/1144
>