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

Message-ID: <928fd71b-d1d3-cbf3-1aed-ae7fa97f6cf0@gmail.com>
Date:   Thu, 18 Jul 2019 19:49:14 +0200
From:   Jacek Anaszewski <jacek.anaszewski@...il.com>
To:     Jean-Jacques Hiblot <jjhiblot@...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

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.

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.

Only then we could think of adding another asynchronous dependency
to the brightness setting chain.

[0] https://lkml.org/lkml/2018/1/17/1144

-- 
Best regards,
Jacek Anaszewski