| 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
| ||
|
Message-ID: <89bdb562-9301-509e-ffe3-6b4dc20610df@gmail.com>
Date: Thu, 12 Jul 2018 23:41:02 +0200
From: Jacek Anaszewski <jacek.anaszewski@...il.com>
To: Baolin Wang <baolin.wang@...aro.org>
Cc: Pavel Machek <pavel@....cz>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Mark Brown <broonie@...nel.org>,
Linux LED Subsystem <linux-leds@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
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@...il.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@...aro.org> wrote:
>>>>
>>>> From: Bjorn Andersson <bjorn.andersson@...aro.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@...aro.org>
>>>> Signed-off-by: Baolin Wang <baolin.wang@...aro.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.
>> 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
Powered by blists - more mailing lists