Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface

[Baolin Wang]

Hi Jacek,

On 12 July 2018 at 05:10, Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx> 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@xxxxxxxxxx> wrote:
>>>
>>> From: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxx>
>>> Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxx>
>>> ---
>>> 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.

>
> 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?

>>
>>>   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. Or am I missed anything?

Thanks for your comments.

-- 
Baolin Wang
Best Regards