| 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: <8da1b769-8aa3-9698-467a-2e7b0707fecf@gmail.com>
Date: Wed, 11 Jul 2018 23:10:27 +0200
From: Jacek Anaszewski <jacek.anaszewski@...il.com>
To: Baolin Wang <baolin.wang@...aro.org>, Pavel Machek <pavel@....cz>
Cc: 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/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.
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;
}
>
>> 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.
>> + 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().
>> };
>>
>> static const struct attribute_group *led_groups[] = {
>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>> index b7e8255..acdbb2f 100644
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -22,6 +22,7 @@
>> #include <linux/workqueue.h>
>>
>> struct device;
>> +struct led_pattern;
>> /*
>> * LED Core
>> */
>> @@ -88,6 +89,14 @@ struct led_classdev {
>> unsigned long *delay_on,
>> unsigned long *delay_off);
>>
>> + int (*pattern_set)(struct led_classdev *led_cdev,
>> + struct led_pattern *pattern, int len);
>> +
>> + int (*pattern_clear)(struct led_classdev *led_cdev);
>> +
>> + struct led_pattern *(*pattern_get)(struct led_classdev *led_cdev,
>> + int *len);
>> +
>> struct device *dev;
>> const struct attribute_group **groups;
>>
>> @@ -446,4 +455,14 @@ static inline void led_classdev_notify_brightness_hw_changed(
>> struct led_classdev *led_cdev, enum led_brightness brightness) { }
>> #endif
>>
>> +/**
>> + * struct led_pattern - brigheness value in a pattern
>> + * @delta_t: delay until next entry, in milliseconds
>> + * @brightness: brightness at time = 0
>> + */
>> +struct led_pattern {
>> + int delta_t;
>> + int brightness;
>> +};
>> +
>> #endif /* __LINUX_LEDS_H_INCLUDED */
>> --
>> 1.7.9.5
>>
>
>
>
--
Best regards,
Jacek Anaszewski
Powered by blists - more mailing lists