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  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:   Sat, 11 Aug 2018 10:17:11 +0800
From:   Baolin Wang <>
To:     Jacek Anaszewski <>
Cc:     Pavel Machek <>,,
        Mark Brown <>,
        Linux LED Subsystem <>,
        LKML <>,
        Bjorn Andersson <>
Subject: Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger

Hi Jacek,

On 11 August 2018 at 02:10, Jacek Anaszewski <> wrote:
> Hi Baolin,
> On 08/10/2018 05:26 PM, Baolin Wang wrote:
>> Hi Jacek,
>> On 9 August 2018 at 21:21, Jacek Anaszewski <> wrote:
>>> Hi Baolin,
>>> On 08/09/2018 07:48 AM, Baolin Wang wrote:
>>> [...]
>>>>>>>> +static int pattern_trig_start_pattern(struct pattern_trig_data *data,
>>>>>>>> +                                   struct led_classdev *led_cdev)
>>>>>>>> +{
>>>>>>>> +     if (!data->npatterns)
>>>>>>>> +             return 0;
>>>>>>>> +
>>>>>>>> +     if (led_cdev->pattern_set) {
>>>>>>>> +             return led_cdev->pattern_set(led_cdev, data->patterns,
>>>>>>>> +                                          data->npatterns, data->repeat);
>>>>>>> I think that it would be more flexible if software pattern fallback
>>>>>>> was applied in case of pattern_set failure. Otherwise, it would
>>>>>>> lead to the situation where LED class devices that support hardware
>>>>>>> blinking couldn't be applied the same set of patterns as LED class
>>>>>>> devices that don't implement pattern_set. The latter will always have to
>>>>>>> resort to using software pattern engine which will accept far greater
>>>>>>> amount of pattern combinations.
>>>>>> Hmmm, I am not sure this is useful for hardware pattern, since the LED
>>>>>> hardware can be diverse which is hard to be compatible with software
>>>>>> pattern.
>>>>>> For example, for our SC27XX LED, it only supports 4 hardware patterns
>>>>>> setting (low time, rising time, high time and falling time) to make it
>>>>>> work at breathing mode. If user provides 3 or 5 patterns values, it
>>>>>> will failed to issue pattern_set(). But at this time, we don't want to
>>>>>> use software pattern instead since it will be not the breathing mode
>>>>>> expected by user. I don't know if there are other special LED
>>>>>> hardware.
>>>>> Good point. So this is the issue we should dwell on, since the
>>>>> requested pattern effect should look similar on all devices.
>>>>> Of course in case of software pattern it will be often impossible
>>>>> to achieve the same fluency. Similarly as in case of rendering graphics
>>>>> with and without acceleration.
>>>>> In case of your device, I'd say that we'd need more complex description
>>>>> of breathing mode pattern. More complex than just four intervals.
>>>>> It should reflect all the intervals the hardware engine needs to perform
>>>>> to achieve the breathing effect. Can this information be inferred from
>>>>> the docs?
>>>> >From our docs, it only provides registers to set the low time, rising
>>>> time, high time and falling time (value unit is 0.125s and max value
>>>> is 63 = 7.875s) to enable breathing mode. So each interval value can
>>>> be 1 ~ 63. But that is still hard for software pattern to simulate the
>>>> breathing mode performed by hardware pattern.
>>> Software fallback is not expected to show similar performance to the
>>> hardware engine on the whole span of the supported time range.
>>> Having min rise time value at 125ms, and given that max_brightness
>>> is 255, then we'd have 255 / 125 = 2.04 of brightness level rise per
>>> 1ms. So, the pattern for rising edge could look like (assuming we
>>> stop at 254):
>>> 0 1 2 1 4 1 6 1 8 1 10 1 ... 254 1
>> Right, maybe this can work, maybe not. Since we can met different
>> cases when failed to issue pattern_set(). Maybe the LED hardware
>> occurs some errors, in this case we should shutdown the LED, not
>> enable the software pattern instead, which still can not work.
> This is arguable. Timer trigger always resorts to using software
> fallback if blink_set() fails.

Timer trigger is simple which only need delay_on and delay_off, that
means software can be easy to show the same performance. But hardware
pattern can be diverse, we need to convert hardware patterns to
software patterns.

>> Maybe
>> driver can set NULL for pattern_set/clear interfaces to disable
>> hardware pattern, then next time user will perform the patterns with
>> software pattern mode.
> Resetting any ops after LED class driver is probed should be
> deemed forbidden, since LED core can make some decisions basing on
> whether given ops are initialized or not.


>> For our SC27XX LED, like I said if user provides only 3 pattern values
>> which will failed to issue pattern_set(). But I still do not need use
>> software patterns to show similar performance, instead it will still
>> keep the last hardware pattern performance ( It did not overlap the
>> previous hardware pattern setting). Maybe different drivers have
>> different error handling, that's why I think we can leave driver to
>> choose the proper way to handle.
> ABI semantics is generic, i.e. common for all drivers. Driver can
> always log any problems occurred while setting pattern, but it shouldn't
> necessarily need to prevent pattern trigger from using software
> fallback.


>> Honestly, can we keep this pattern trigger simple and easy at first?
>> If some drivers want to use software patterns to perform again once
>> their hardware patterns performed failed (IMHO our SC27XX LED do not
>> need), then we can add this feature, at that time we will have users
>> to test and optimize the feature. Maybe I am wrong:)
> In other words you want to prevent users from exploiting the flexibility
> of pattern trigger, and limit them to using always only breathing
> scheme for SC27XX LED. What if someone would like to employ pattern
> trigger for encoding morse message to report system errors?

I think we can change to use timer trigger or other triggers.

> And secondly, we cannot keep the interface simple at first and then
> change it, because this is against Linux rule, which says that
> interface cannot break existing users.

Make sense.

>>> Now, I'm starting to wonder if we shouldn't have specialized trigger
>>> for breathing patterns, that would accept brightness level change per
>>> time period. Pattern trigger needs more flexibility and inferring if the
>>> hardware can handle given series of pattern intervals would entail
>>> unnecessary code complexity.
>>> Such breathing trigger would require triplets comprised of
>>> start brightness, end brightness and a duration of the brightness
>>> transition.
>> But this is the only place we can set the hardware patterns according
>> to our previous discussion. Thanks.
> Thinking more about it, introducing another trigger for something being
> also a pattern is a bad idea.


> Perhaps, as an alternative, we could allow for providing mathematical
> formula to define the edge of brightness change.
> I wonder what Pavel thinks.

Baolin Wang
Best Regards

Powered by blists - more mailing lists