Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <89bdb562-9301-509e-ffe3-6b4dc20610df@gmail.com>
References: <1665b877dc2f886a90a00e3ca3b7425372d99b6e.1530248085.git.baolin.wang@linaro.org>
 <CAMz4kuJfPKykB6G-8C25mmMCfQgefv-Y=k2OQYZR_e0Tvg3guQ@mail.gmail.com>
 <8da1b769-8aa3-9698-467a-2e7b0707fecf@gmail.com> <CAMz4kuKZ6Z1h=yzFYJihEH1ARC5FKtDC6RLz0u8SmaekOZ=utw@mail.gmail.com>
 <89bdb562-9301-509e-ffe3-6b4dc20610df@gmail.com>
From:   Baolin Wang <baolin.wang@linaro.org>
Date:   Fri, 13 Jul 2018 09:58:03 +0800
Message-ID: <CAMz4ku+Chen0YhtBYdwg_JA1jmBc-XQM+fFhStZMEoDehmPYBQ@mail.gmail.com>
Subject: Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
To:     Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc:     Pavel Machek <pavel@ucw.cz>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Mark Brown <broonie@kernel.org>,
        Linux LED Subsystem <linux-leds@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Jacek,

On 13 July 2018 at 05:41, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> Hi Baolin,
>
>
> On 07/12/2018 02:24 PM, Baolin Wang wrote:
>>
>> Hi Jacek,
>>
>> On 12 July 2018 at 05:10, Jacek Anaszewski <jacek.anaszewski@gmail.com>
>> wrote:
>>>
>>> Hi Baolin.
>>>
>>>
>>> On 07/11/2018 01:02 PM, Baolin Wang wrote:
>>>>
>>>>
>>>> Hi Jacek and Pavel,
>>>>
>>>> On 29 June 2018 at 13:03, Baolin Wang <baolin.wang@linaro.org> wrote:
>>>>>
>>>>>
>>>>> From: Bjorn Andersson <bjorn.andersson@linaro.org>
>>>>>
>>>>> Some LED controllers have support for autonomously controlling
>>>>> brightness over time, according to some preprogrammed pattern or
>>>>> function.
>>>>>
>>>>> This adds a new optional operator that LED class drivers can implement
>>>>> if they support such functionality as well as a new device attribute to
>>>>> configure the pattern for a given LED.
>>>>>
>>>>> [Baolin Wang did some minor improvements.]
>>>>>
>>>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>> ---
>>>>> Changes from v2:
>>>>>    - Change kernel version to 4.19.
>>>>>    - Force user to return error pointer if failed to issue
>>>>> pattern_get().
>>>>>    - Use strstrip() to trim trailing newline.
>>>>>    - Other optimization.
>>>>>
>>>>> Changes from v1:
>>>>>    - Add some comments suggested by Pavel.
>>>>>    - Change 'delta_t' can be 0.
>>>>>
>>>>> Note: I removed the pattern repeat check and will get the repeat number
>>>>> by adding
>>>>> one extra file named 'pattern_repeat' according to previous discussion.
>>>>> ---
>>>>
>>>>
>>>>
>>>> Do you have any comments for this version patch set? Thanks.
>>>
>>>
>>>
>>> I tried modifying uleds.c driver to support pattern ops, but
>>> I'm getting segfault when doing "cat pattern". I didn't give
>>> it serious testing and analysis - will do it at weekend.
>>>
>>> It also drew my attention to the issue of desired pattern sysfs
>>> interface semantics on uninitialized pattern. In your implementation
>>> user seems to be unable to determine if the pattern is activated
>>> or not. We should define the semantics for this use case and
>>> describe it in the documentation. Possibly pattern could
>>> return alone new line character then.
>>
>>
>> I am not sure I get your points correctly. If user writes values to
>> pattern interface which means we activated the pattern.
>> If I am wrong, could you elaborate on the issue you concerned? Thanks.
>
>
> Now I see, that writing empty string disables the pattern, right?
> It should be explicitly stated in the pattern file documentation.

Yes, you are right. OK, I will add some documentation for this. Thanks.

>>> This is the code snippet I've used for testing pattern interface:
>>>
>>> static struct led_pattern ptrn[10];
>>> static int ptrn_len;
>>>
>>> static int uled_pattern_clear(struct led_classdev *ldev)
>>> {
>>>          return 0;
>>> }
>>>
>>> static int uled_pattern_set(struct led_classdev *ldev,
>>>                                    struct led_pattern *pattern,
>>>                                    int len)
>>> {
>>>          int i;
>>>
>>>          for (i = 0; i < len; i++) {
>>>                  ptrn[i].brightness = pattern[i].brightness;
>>>                  ptrn[i].delta_t = pattern[i].delta_t;
>>>          }
>>>
>>>          ptrn_len = len;
>>>
>>>          return 0;
>>> }
>>>
>>> static struct led_pattern *uled_pattern_get(struct led_classdev *ldev,
>>>                                                    int *len)
>>> {
>>>          int i;
>>>
>>>          for (i = 0; i < ptrn_len; i++) {
>>>                  ptrn[i].brightness = 3;
>>>                  ptrn[i].delta_t = 5;
>>>          }
>>>
>>>          *len = ptrn_len;
>>>
>>>          return ptrn;
>>>
>>> }
>>
>>
>> The reason you met segfault when doing "cat pattern" is you should not
>> return one static pattern array, since in pattern_show() it will help
>> to free the pattern memory, could you change to return one pattern
>> pointer with dynamic allocation like my patch 2?
>
>
> Thanks for pointing this out.
>
>
>>>>>    Documentation/ABI/testing/sysfs-class-led |   17 +++++
>>>>>    drivers/leds/led-class.c                  |  119
>>>>> +++++++++++++++++++++++++++++
>>>>>    include/linux/leds.h                      |   19 +++++
>>>>>    3 files changed, 155 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/ABI/testing/sysfs-class-led
>>>>> b/Documentation/ABI/testing/sysfs-class-led
>>>>> index 5f67f7a..e01ac55 100644
>>>>> --- a/Documentation/ABI/testing/sysfs-class-led
>>>>> +++ b/Documentation/ABI/testing/sysfs-class-led
>>>>> @@ -61,3 +61,20 @@ Description:
>>>>>                   gpio and backlight triggers. In case of the backlight
>>>>> trigger,
>>>>>                   it is useful when driving a LED which is intended to
>>>>> indicate
>>>>>                   a device in a standby like state.
>>>>> +
>>>>> +What: /sys/class/leds/<led>/pattern
>>>>> +Date: June 2018
>>>>> +KernelVersion: 4.19
>>>>> +Description:
>>>>> +       Specify a pattern for the LED, for LED hardware that support
>>>>> +       altering the brightness as a function of time.
>>>>> +
>>>>> +       The pattern is given by a series of tuples, of brightness and
>>>>> +       duration (ms). The LED is expected to traverse the series and
>>>>> +       each brightness value for the specified duration. Duration of
>>>>> +       0 means brightness should immediately change to new value.
>>>>> +
>>>>> +       As LED hardware might have different capabilities and precision
>>>>> +       the requested pattern might be slighly adjusted by the driver
>>>>> +       and the resulting pattern of such operation should be returned
>>>>> +       when this file is read.
>>>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>>>>> index 3c7e348..8a685a2 100644
>>>>> --- a/drivers/leds/led-class.c
>>>>> +++ b/drivers/leds/led-class.c
>>>>> @@ -74,6 +74,123 @@ static ssize_t max_brightness_show(struct device
>>>>> *dev,
>>>>>    }
>>>>>    static DEVICE_ATTR_RO(max_brightness);
>>>>>
>>>>> +static ssize_t pattern_show(struct device *dev,
>>>>> +                           struct device_attribute *attr, char *buf)
>>>>> +{
>>>>> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>>>> +       struct led_pattern *pattern;
>>>>> +       size_t offset = 0;
>>>>> +       int count, n, i;
>>>>> +
>>>>> +       if (!led_cdev->pattern_get)
>>>>> +               return -EOPNOTSUPP;
>>>
>>>
>>>
>>> Please check this in led_classdev_register() and fail
>>> if pattern_set is initialized, but pattern_get or pattern_clear not.
>>
>>
>> Sure.
>>
>>>>> +       pattern = led_cdev->pattern_get(led_cdev, &count);
>>>>> +       if (IS_ERR(pattern))
>>>>> +               return PTR_ERR(pattern);
>>>>> +
>>>>> +       for (i = 0; i < count; i++) {
>>>>> +               n = snprintf(buf + offset, PAGE_SIZE - offset, "%d %d
>>>>> ",
>>>>> +                            pattern[i].brightness,
>>>>> pattern[i].delta_t);
>>>>> +
>>>>> +               if (offset + n >= PAGE_SIZE)
>>>>> +                       goto err_nospc;
>>>>> +
>>>>> +               offset += n;
>>>>> +       }
>>>>> +
>>>>> +       buf[offset - 1] = '\n';
>>>>> +
>>>>> +       kfree(pattern);
>>>>> +       return offset;
>>>>> +
>>>>> +err_nospc:
>>>>> +       kfree(pattern);
>>>>> +       return -ENOSPC;
>>>>> +}
>>>>> +
>>>>> +static ssize_t pattern_store(struct device *dev,
>>>>> +                            struct device_attribute *attr,
>>>>> +                            const char *buf, size_t size)
>>>>> +{
>>>>> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>>>> +       struct led_pattern *pattern = NULL;
>>>>> +       unsigned long val;
>>>>> +       char *sbegin;
>>>>> +       char *elem;
>>>>> +       char *s;
>>>>> +       int ret, len = 0;
>>>>> +       bool odd = true;
>>>>> +
>>>>> +       sbegin = kstrndup(buf, size, GFP_KERNEL);
>>>>> +       if (!sbegin)
>>>>> +               return -ENOMEM;
>>>>> +
>>>>> +       /*
>>>>> +        * Trim trailing newline, if the remaining string is empty,
>>>>> +        * clear the pattern.
>>>>> +        */
>>>>> +       s = strstrip(sbegin);
>>>>> +       if (!*s) {
>>>>> +               ret = led_cdev->pattern_clear(led_cdev);
>>>>> +               goto out;
>>>>> +       }
>>>>> +
>>>>> +       pattern = kcalloc(size, sizeof(*pattern), GFP_KERNEL);
>>>>> +       if (!pattern) {
>>>>> +               ret = -ENOMEM;
>>>>> +               goto out;
>>>>> +       }
>>>>> +
>>>>> +       /* Parse out the brightness & delta_t touples */
>>>>> +       while ((elem = strsep(&s, " ")) != NULL) {
>>>>> +               ret = kstrtoul(elem, 10, &val);
>>>>> +               if (ret)
>>>>> +                       goto out;
>>>>> +
>>>>> +               if (odd) {
>>>>> +                       pattern[len].brightness = val;
>>>>> +               } else {
>>>>> +                       pattern[len].delta_t = val;
>>>>> +                       len++;
>>>>> +               }
>>>>> +
>>>>> +               odd = !odd;
>>>>> +       }
>>>>> +
>>>>> +       /*
>>>>> +        * Fail if we didn't find any data points or last data point
>>>>> was
>>>>> partial
>>>>> +        */
>>>>> +       if (!len || !odd) {
>>>>> +               ret = -EINVAL;
>>>>> +               goto out;
>>>>> +       }
>>>>> +
>>>>> +       ret = led_cdev->pattern_set(led_cdev, pattern, len);
>>>>> +
>>>>> +out:
>>>>> +       kfree(pattern);
>>>>> +       kfree(sbegin);
>>>>> +       return ret < 0 ? ret : size;
>>>>> +}
>>>>> +static DEVICE_ATTR_RW(pattern);
>>>>> +
>>>>> +static umode_t led_class_attrs_mode(struct kobject *kobj,
>>>>> +                                   struct attribute *attr, int index)
>>>>> +{
>>>>> +       struct device *dev = container_of(kobj, struct device, kobj);
>>>>> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>>>> +
>>>>> +       if (attr == &dev_attr_brightness.attr)
>>>>> +               return attr->mode;
>>>>> +       if (attr == &dev_attr_max_brightness.attr)
>>>>> +               return attr->mode;
>>>>> +       if (attr == &dev_attr_pattern.attr && led_cdev->pattern_set)
>>>>> +               return attr->mode;
>>>>> +
>>>>> +       return 0;
>>>>> +}
>>>
>>>
>>>
>>>>>    #ifdef CONFIG_LEDS_TRIGGERS
>>>>>    static DEVICE_ATTR(trigger, 0644, led_trigger_show,
>>>>> led_trigger_store);
>>>>>    static struct attribute *led_trigger_attrs[] = {
>>>>> @@ -88,11 +205,13 @@ static ssize_t max_brightness_show(struct device
>>>>> *dev,
>>>>>    static struct attribute *led_class_attrs[] = {
>>>>>           &dev_attr_brightness.attr,
>>>>>           &dev_attr_max_brightness.attr,
>>>>> +       &dev_attr_pattern.attr,
>>>>>           NULL,
>>>>>    };
>>>>>
>>>>>    static const struct attribute_group led_group = {
>>>>>           .attrs = led_class_attrs,
>>>>> +       .is_visible = led_class_attrs_mode,
>>>
>>>
>>>
>>> This should not be needed if we'll validate pattern ops
>>> in led_classdev_register().
>>
>>
>> I am afraid we can not remove it. If the driver did not supply
>> pattern_set/get/clear, we should not still show the pattern interfaces
>> for userspace.
>
>
> You're right.
>
>
> --
> Best regards,
> Jacek Anaszewski


-- 
Baolin Wang
Best Regards