| 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
| ||
|
Date: Sat, 25 Apr 2020 22:23:07 +0200
From: Pavel Machek <pavel@....cz>
To: Dan Murphy <dmurphy@...com>
Cc: jacek.anaszewski@...il.com, linux-leds@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v20 03/17] leds: multicolor: Introduce a multicolor class
definition
Hi!
ting/sysfs-class-led-multicolor
> new file mode 100644
> index 000000000000..ada0dbecfeab
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor
> @@ -0,0 +1,42 @@
> +What: /sys/class/leds/<led>/multi_led_index
> +Date: March 2020
> +KernelVersion: 5.8
> +Contact: Dan Murphy <dmurphy@...com>
> +Description: read
> + The multi_led_index array, when read, will output the LED colors
> + by name as they are indexed in the multi_led_intensity file.
Can we make it multi_index? We are already in leds directory, and it
is a bit shorter.
> +What: /sys/class/leds/<led>/num_multi_leds
> +Date: March 2020
> +KernelVersion: 5.8
> +Contact: Dan Murphy <dmurphy@...com>
> +Description: read
> + The num_multi_leds indicates the number of LEDs defined in the
> + multi_led_intensity and multi_led_index files.
Please drop this one.
> +What: /sys/class/leds/<led>/multi_led_intensity
> +Date: March 2020
> +KernelVersion: 5.8
> +Contact: Dan Murphy <dmurphy@...com>
> +Description: read/write
> + Intensity level for the LED color within the array.
> + The intensities for each color must be entered based on the
> + multi_led_index array.
And let this one be multi_intensity.
> +For more details on hue and lightness notions please refer to
> +https://en.wikipedia.org/wiki/CIECAM02.
I'd drop this reference. multi_intensity file controls both hue and
saturation AFAICT.
> +Example:
> +A user first writes the multi_led_intensity file with the brightness levels
> +for each LED that are necessary to achieve a blueish violet output from a
> +multicolor LED group.
I don't believe we can guarantee that. 255/255/255 will produce
different colors on different hardware (not white), and 43/226/138
will also produce different colors....
> +cat /sys/class/leds/multicolor:status/multi_led_index
> +green blue red
Hmm. We should really make sure LEDs are ordered as "red green
blue". Yes, userspace should support any order, but...
> +The user can control the brightness of that multicolor LED group by writing the
> +parent 'brightness' control. Assuming a parent max_brightness of 255 the user
delete "parent", twice?
> + for (i = 0; i < mcled_cdev->num_colors; i++)
> + mcled_cdev->multicolor_info[i].color_brightness = brightness *
> + mcled_cdev->multicolor_info[i].color_led_intensity /
> + led_cdev->max_brightness;
It would be good to get this under ~80 characters. Perhaps shorter
identifiers would help... shortening multicolor_ to mc_?
> +static ssize_t multi_led_intensity_store(struct device *dev,
> + struct device_attribute *intensity_attr,
> + const char *buf, size_t size)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
> + int nrchars, offset = 0;
> + int intensity_value[LED_COLOR_ID_MAX];
> + int i;
> + ssize_t ret;
> +
> + mutex_lock(&led_cdev->led_access);
> +
> + for (i = 0; i < mcled_cdev->num_colors; i++) {
> + ret = sscanf(buf + offset, "%i%n",
> + &intensity_value[i], &nrchars);
> + if (ret != 1) {
> + dev_err(led_cdev->dev,
dev_dbg, at most. It is user-triggerable.
> + "Incorrect number of LEDs expected %i values intensity was not applied\n",
> + mcled_cdev->num_colors);
> + goto err_out;
Should not we return -ERRNO to userspace on error?
> + }
> + offset += nrchars;
> + }
This checks for "not enough" intensities. Do we need check for "too
many" intensities?
> +static ssize_t multi_led_intensity_show(struct device *dev,
> + struct device_attribute *intensity_attr,
> + char *buf)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
> + int len = 0;
> + int i;
> +
> + for (i = 0; i < mcled_cdev->num_colors; i++)
> + len += sprintf(buf + len, "%d ",
> + mcled_cdev->multicolor_info[i].color_led_intensity);
> +
> + len += sprintf(buf + len, "%s", "\n");
This will result in extra " " before end of line.
Please don't use "%s", "\n" to add single character. "\n" would be enough.
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
> + int len = 0;
> + int index;
> + int i;
> +
> + for (i = 0; i < mcled_cdev->num_colors; i++) {
> + index = mcled_cdev->multicolor_info[i].color_index;
> + len += sprintf(buf + len, "%s ", led_colors[index]);
> + }
> +
> + len += sprintf(buf + len, "%s", "\n");
Same here.
> +int led_classdev_multicolor_register_ext(struct device *parent,
> + struct led_classdev_mc *mcled_cdev,
> + struct led_init_data *init_data)
> +{
> + struct led_classdev *led_cdev;
> +
> + if (!mcled_cdev)
> + return -EINVAL;
> +
> + if (!mcled_cdev->num_colors)
> + return -EINVAL;
if (num_colors > max)... ?
> +#ifndef __LINUX_MULTICOLOR_LEDS_H_INCLUDED
> +#define __LINUX_MULTICOLOR_LEDS_H_INCLUDED
Usual style is "_LINUX_MULTICOLOR_LEDS_H".
> +#else
> +
> +static inline int led_classdev_multicolor_register_ext(struct device *parent,
double space after "inline".
> + struct led_classdev_mc *mcled_cdev,
> + struct led_init_data *init_data)
> +{
> + return -EINVAL;
> +}
Do we need to include these stubs? I guess it is okay to have them,
OTOH I'd expect drivers to depend on MULTICOLOR being available...
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Download attachment "signature.asc" of type "application/pgp-signature" (182 bytes)
Powered by blists - more mailing lists