[PATCH] leds: Introduce userspace leds driver

[David Lechner]

This driver creates a userspace leds driver similar to uinput.

New leds are created by opening /dev/uleds and writing a uleds_user_dev
struct. A new leds class device is registered with the name given in the
struct. Reading will return a single byte that is the current brightness.
The poll() syscall is also supported. It will be triggered whenever the
brightness changes.

Signed-off-by: David Lechner <da...@lechnology.com>
---

Here is a sample python program that I used to test that it works as expected.

#!/usr/bin/env python3

import select
from threading import Timer

ULEDS_MAX_NAME_SIZE = 80

name = 'test'


def change_brightness(b):
with open('/sys/class/leds/{}/brightness'.format(name) , 'w') as f:
f.write(str(b))


with open('/dev/uleds', 'rb+', 0) as uleds:
bname = name.encode("ascii")
# create the leds class device
uleds.write(bname + b'\0' * (ULEDS_MAX_NAME_SIZE - len(bname)))

# test that it works
print(uleds.read(1))

# schedule some brighness changes at 1 second intervals
Timer(1, change_brightness, (1,)).start()
# this won't trigger poll since brighness does not change
Timer(2, change_brightness, (1,)).start()
Timer(3, change_brightness, (2,)).start()

# wait for the scheduled changes
p = select.poll()
p.register(uleds.fileno(), select.POLLIN)
p.poll()
print(uleds.read(1))
p.poll()
print(uleds.read(1))



drivers/leds/Kconfig | 8 ++
drivers/leds/Makefile | 3 +
drivers/leds/uleds.c | 224 +++++++++++++++++++++++++++++++++++++++++++++
include/uapi/linux/Kbuild | 1 +
include/uapi/linux/uleds.h | 23 +++++
5 files changed, 259 insertions(+)
create mode 100644 drivers/leds/uleds.c
create mode 100644 include/uapi/linux/uleds.h

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 9dcc9b1..25c6168 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -631,6 +631,14 @@ config LEDS_VERSATILE
This option enabled support for the LEDs on the ARM Versatile
and RealView boards. Say Y to enabled these.

+config LEDS_USER
+ tristate "Userspace LED support"
+ depends on LEDS_CLASS
+ help
+ This option enables support for userspace LEDs. Say 'y' to enable this
+ support in kernel. To compile this driver as a module, choose 'm' here:
+ the module will be called leds-user.
+
comment "LED Triggers"
source "drivers/leds/trigger/Kconfig"

diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 0684c86..d723eeb 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -72,5 +72,8 @@ obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o
# LED SPI Drivers
obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o

+# LED Userspace Drivers
+obj-$(CONFIG_LEDS_USER) += uleds.o
+
# LED Triggers
obj-$(CONFIG_LEDS_TRIGGERS) += trigger/
diff --git a/drivers/leds/uleds.c b/drivers/leds/uleds.c
new file mode 100644
index 0000000..992bca4
--- /dev/null
+++ b/drivers/leds/uleds.c
@@ -0,0 +1,224 @@
+/*
+ * Userspace driver for leds subsystem
+ *
+ * Copyright (C) 2016 David Lechner <da...@lechnology.com>
+ *
+ * Based on uinput.c: Aristeu Sergio Rozanski Filho <ar...@cathedrallabs.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#include <linux/poll.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/fs.h>
+#include <linux/leds.h>
+#include <linux/miscdevice.h>
+
+#include <uapi/linux/uleds.h>
+
+#define ULEDS_NAME "uleds"
+
+enum uleds_state {
+ ULEDS_STATE_UNKNOWN,
+ ULEDS_STATE_REGISTERED,
+};
+
+struct uleds_device {
+ struct uleds_user_dev user_dev;
+ struct led_classdev led_cdev;
+ struct mutex mutex;
+ enum uleds_state state;
+ wait_queue_head_t waitq;
+ unsigned char brightness;
+ unsigned char new_data;
+};
+
+static void uleds_brightness_set(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ struct uleds_device *udev = container_of(led_cdev, struct uleds_device,
+ led_cdev);
+
+ if (udev->brightness != brightness) {
+ udev->brightness = brightness;
+ udev->new_data = 1;
+ wake_up_interruptible(&udev->waitq);
+ }
+}
+
+static int uleds_open(struct inode *inode, struct file *file)
+{
+ struct uleds_device *udev;
+
+ udev = kzalloc(sizeof(*udev), GFP_KERNEL);
+ if (!udev)
+ return -ENOMEM;
+
+ udev->led_cdev.name = udev->user_dev.name;
+ udev->led_cdev.max_brightness = LED_FULL;
+ udev->led_cdev.brightness_set = uleds_brightness_set;
+
+ mutex_init(&udev->mutex);
+ init_waitqueue_head(&udev->waitq);
+ udev->state = ULEDS_STATE_UNKNOWN;
+
+ file->private_data = udev;
+ nonseekable_open(inode, file);
+
+ return 0;
+}
+
+static ssize_t uleds_write(struct file *file, const char __user *buffer,
+ size_t count, loff_t *ppos)
+{
+ struct uleds_device *udev = file->private_data;
+ int ret;
+
+ if (count == 0)
+ return 0;
+
+ ret = mutex_lock_interruptible(&udev->mutex);
+ if (ret)
+ return ret;
+
+ if (udev->state == ULEDS_STATE_REGISTERED) {
+ ret = -EBUSY;
+ goto out;
+ }
+
+ if (count != sizeof(struct uleds_user_dev)) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if (copy_from_user(&udev->user_dev, buffer,
+ sizeof(struct uleds_user_dev))) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ if (!udev->user_dev.name[0]) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ ret = led_classdev_register(NULL, &udev->led_cdev);
+ if (ret < 0)
+ goto out;
+
+ udev->state = ULEDS_STATE_REGISTERED;
+ ret = count;
+
+out:
+ mutex_unlock(&udev->mutex);
+
+ return ret;
+}
+
+static ssize_t uleds_read(struct file *file, char __user *buffer, size_t count,
+ loff_t *ppos)
+{
+ struct uleds_device *udev = file->private_data;
+ ssize_t retval;
+
+ if (count == 0)
+ return 0;
+
+ if (count != 1)
+ return -EINVAL;
+
+ do {
+ retval = mutex_lock_interruptible(&udev->mutex);
+ if (retval)
+ return retval;
+
+ if (udev->state != ULEDS_STATE_REGISTERED) {
+ retval = -ENODEV;
+ } else if (!udev->new_data && (file->f_flags & O_NONBLOCK)) {
+ retval = -EAGAIN;
+ } else {
+ retval = copy_to_user(buffer, &udev->brightness, 1);
+ udev->new_data = 0;
+ retval = 1;
+ }
+
+ mutex_unlock(&udev->mutex);
+
+ if (retval || count == 0)
+ break;
+
+ if (!(file->f_flags & O_NONBLOCK))
+ retval = wait_event_interruptible(udev->waitq,
+ udev->new_data ||
+ udev->state != ULEDS_STATE_REGISTERED);
+ } while (retval == 0);
+
+ return retval;
+}
+
+static unsigned int uleds_poll(struct file *file, poll_table *wait)
+{
+ struct uleds_device *udev = file->private_data;
+
+ poll_wait(file, &udev->waitq, wait);
+
+ if (udev->new_data)
+ return POLLIN | POLLRDNORM;
+
+ return 0;
+}
+
+static int uleds_release(struct inode *inode, struct file *file)
+{
+ struct uleds_device *udev = file->private_data;
+
+ if (udev->state == ULEDS_STATE_REGISTERED) {
+ udev->state = ULEDS_STATE_UNKNOWN;
+ led_classdev_unregister(&udev->led_cdev);
+ }
+ kfree(udev);
+
+ return 0;
+}
+
+static const struct file_operations uleds_fops = {
+ .owner = THIS_MODULE,
+ .open = uleds_open,
+ .release = uleds_release,
+ .read = uleds_read,
+ .write = uleds_write,
+ .poll = uleds_poll,
+ .llseek = no_llseek,
+};
+
+static struct miscdevice uleds_misc = {
+ .fops = &uleds_fops,
+ .minor = MISC_DYNAMIC_MINOR,
+ .name = ULEDS_NAME,
+};
+
+static int __init uleds_init(void)
+{
+ return misc_register(&uleds_misc);
+}
+module_init(uleds_init);
+
+static void __exit uleds_exit(void)
+{
+ misc_deregister(&uleds_misc);
+}
+module_exit(uleds_exit);
+
+MODULE_AUTHOR("David Lechner <da...@lechnology.com>");
+MODULE_DESCRIPTION("Userspace driver for leds subsystem");
+MODULE_LICENSE("GPL");
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
index 185f8ea..416f5e6 100644
--- a/include/uapi/linux/Kbuild
+++ b/include/uapi/linux/Kbuild
@@ -421,6 +421,7 @@ header-y += udp.h
header-y += uhid.h
header-y += uinput.h
header-y += uio.h
+header-y += uleds.h
header-y += ultrasound.h
header-y += un.h
header-y += unistd.h
diff --git a/include/uapi/linux/uleds.h b/include/uapi/linux/uleds.h
new file mode 100644
index 0000000..e78ed46
--- /dev/null
+++ b/include/uapi/linux/uleds.h
@@ -0,0 +1,23 @@
+/*
+ * Userspace driver support for leds subsystem
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#ifndef _UAPI__ULEDS_H_
+#define _UAPI__ULEDS_H_
+
+#define ULEDS_MAX_NAME_SIZE 80
+
+struct uleds_user_dev {
+ char name[ULEDS_MAX_NAME_SIZE];
+};
+
+#endif /* _UAPI__ULEDS_H_ */
--
2.7.4

[Peter Meerwald-Stadler]

Hi,

> This driver creates a userspace leds driver similar to uinput.

is the module really called leds-user as claimed by Kconfig?
or rather uleds

p.

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

[David Lechner]

On 09/06/2016 12:09 AM, Peter Meerwald-Stadler wrote:
> Hi,
>
>> This driver creates a userspace leds driver similar to uinput.
>
> is the module really called leds-user as claimed by Kconfig?
> or rather uleds
>

It is supposed to be "uleds". Thanks for catching that.

[Jacek Anaszewski]

Hi David,

Thanks for the patch. It is very nice. I have only one minor remark
in the code.

I think that it would be good to add a documentation for this
driver to Documentation/leds, with exemplary C program instead
of python one. The program could poll the dev node in a loop,
which allows to conveniently check the impact of setting particular
triggers and all other brightness change events.

Please arrange include directives in alphabetical order.

Best regards,
Jacek Anaszewski

[David Lechner]

This driver creates a userspace leds driver similar to uinput.

New leds are created by opening /dev/uleds and writing a uleds_user_dev
struct. A new leds class device is registered with the name given in the
struct. Reading will return a single byte that is the current brightness.
The poll() syscall is also supported. It will be triggered whenever the

brightness changes. Closing the file handle to /dev/uleds will remove
the leds class device.

Signed-off-by: David Lechner <da...@lechnology.com>
---

v2 changes:

* sort #includes
* fix typo in Kconfig
* Add Documentation text file

Documentation/leds/uleds.txt | 105 ++++++++++++++++++++

drivers/leds/Kconfig | 8 ++
drivers/leds/Makefile | 3 +
drivers/leds/uleds.c | 224 +++++++++++++++++++++++++++++++++++++++++++

include/uapi/linux/Kbuild | 1 +
include/uapi/linux/uleds.h | 23 +++++

6 files changed, 364 insertions(+)
create mode 100644 Documentation/leds/uleds.txt

create mode 100644 drivers/leds/uleds.c
create mode 100644 include/uapi/linux/uleds.h

diff --git a/Documentation/leds/uleds.txt b/Documentation/leds/uleds.txt
new file mode 100644
index 0000000..1c033d8
--- /dev/null
+++ b/Documentation/leds/uleds.txt
@@ -0,0 +1,105 @@
+Userspace LEDs
+==============
+
+The uleds driver supports userspace LEDs. This can be useful for testing
+triggers and can also be used to implement virtual LEDs.
+
+
+Usage
+=====
+
+When the driver is loaded, a character devices is created at /dev/uleds. To
+create a new leds class device, open /dev/uleds and write a uleds_user_dev
+structure to it.
+
+ #define ULEDS_MAX_NAME_SIZE 80
+
+ struct uleds_user_dev {

+ char name[ULEDS_MAX_NAME_SIZE];
+ };
+

+A new leds class device will be created with the name given. The name can be
+any valid file name, but consider using the leds class convention of
+"devicename:color:function".
+
+The current brightness is found by reading a single byte from the character
+device. Values are unsigned: 0 to 255. Reading does not block and always returns
+the most recent brightness value. The device node can also be polled to notify
+when the brightness value changes.
+
+The leds class device will be removed when the open file handle to /dev/uleds
+is closed.
+
+
+Example
+=======
+
+/*
+ * uledmon.c
+ *
+ * This program creates a new userspace leds class device and monitor it. A
+ * timestamp and brightness value is printed each time the brightness changes.
+ *
+ * Usage: uledmon <device-name>
+ *
+ * <device-name> is the name of the leds class device to be created. Pressing
+ * CTRL+C will exit.
+ */
+
+#include <fcntl.h>
+#include <poll.h>
+#include <stdio.h>
+#include <string.h>
+#include <time.h>
+#include <unistd.h>
+
+#include <linux/uleds.h>
+
+int main(int argc, char const *argv[])
+{
+ struct uleds_user_dev uleds_dev;
+ int fd, ret;
+ struct pollfd pfd;
+ unsigned char brightness;
+ struct timespec ts;
+
+ if (argc != 2) {
+ fprintf(stderr, "Requires <device-name> argument\n");
+ return 1;
+ }
+
+ strncpy(uleds_dev.name, argv[1], ULEDS_MAX_NAME_SIZE);
+
+ fd = open("/dev/uleds", O_RDWR);
+ if (fd == -1) {
+ perror("Failed to open /dev/uleds");
+ return 1;
+ }
+
+ ret = write(fd, &uleds_dev, sizeof(uleds_dev));
+ if (ret == -1) {
+ perror("Failed to write to /dev/uleds");
+ close(fd);
+ return 1;
+ }
+
+ pfd.fd = fd;
+ pfd.events = POLLIN;
+ pfd.revents = 0;
+
+ while (!(pfd.revents & (POLLERR | POLLHUP | POLLNVAL))) {
+ ret = read(fd, &brightness, 1);
+ if (ret == -1) {
+ perror("Failed to read from /dev/uleds");
+ close(fd);
+ return 1;
+ }
+ clock_gettime(CLOCK_MONOTONIC, &ts);
+ printf("[%ld.%09ld] %u\n", ts.tv_sec, ts.tv_nsec, brightness);
+ poll(&pfd, 1, -1);
+ }
+
+ close(fd);

+
+ return 0;
+}

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 9dcc9b1..94550f6 100644

--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -631,6 +631,14 @@ config LEDS_VERSATILE
This option enabled support for the LEDs on the ARM Versatile
and RealView boards. Say Y to enabled these.

+config LEDS_USER
+ tristate "Userspace LED support"
+ depends on LEDS_CLASS
+ help
+ This option enables support for userspace LEDs. Say 'y' to enable this
+ support in kernel. To compile this driver as a module, choose 'm' here:

+ the module will be called uleds.

+
comment "LED Triggers"
source "drivers/leds/trigger/Kconfig"

diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 0684c86..d723eeb 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -72,5 +72,8 @@ obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o
# LED SPI Drivers
obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o

+# LED Userspace Drivers
+obj-$(CONFIG_LEDS_USER) += uleds.o
+
# LED Triggers
obj-$(CONFIG_LEDS_TRIGGERS) += trigger/
diff --git a/drivers/leds/uleds.c b/drivers/leds/uleds.c
new file mode 100644

index 0000000..77a0bee

--- /dev/null
+++ b/drivers/leds/uleds.c
@@ -0,0 +1,224 @@
+/*
+ * Userspace driver for leds subsystem
+ *
+ * Copyright (C) 2016 David Lechner <da...@lechnology.com>
+ *
+ * Based on uinput.c: Aristeu Sergio Rozanski Filho <ar...@cathedrallabs.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */

+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/leds.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>

+#include <linux/poll.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+

2.7.4

[Peter Meerwald-Stadler]

> This driver creates a userspace leds driver similar to uinput.

nitpicking below

device is

monitors

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

[Jacek Anaszewski]

Hi David,

Thanks for the update. Even more nitpicking below.

Please also rebase the patch on top of current linux-leds.git, for-next
branch.

Maybe it would be worth to mention that these definitions are
available in the public kernel API header.

> +
> +A new leds class device will be created with the name given. The name can be

s/leds/LED/

> +any valid file name, but consider using the leds class convention of

s/leds class/LED class device naming convention/

> +"devicename:color:function".
> +
> +The current brightness is found by reading a single byte from the character
> +device. Values are unsigned: 0 to 255. Reading does not block and always returns
> +the most recent brightness value. The device node can also be polled to notify
> +when the brightness value changes.
> +
> +The leds class device will be removed when the open file handle to /dev/uleds

s/leds/LED/

> +is closed.
> +
> +
> +Example
> +=======
> +
> +/*
> + * uledmon.c
> + *
> + * This program creates a new userspace leds class device and monitor it. A

s/leds/LED/

> + * timestamp and brightness value is printed each time the brightness changes.
> + *
> + * Usage: uledmon <device-name>
> + *
> + * <device-name> is the name of the leds class device to be created. Pressing

s/leds/LED/

Best regards,
Jacek Anaszewski

[David Lechner]

This driver creates a userspace leds driver similar to uinput.

New leds are created by opening /dev/uleds and writing a uleds_user_dev
struct. A new leds class device is registered with the name given in the
struct. Reading will return a single byte that is the current brightness.
The poll() syscall is also supported. It will be triggered whenever the
brightness changes. Closing the file handle to /dev/uleds will remove
the leds class device.

Signed-off-by: David Lechner <da...@lechnology.com>
---

v2 changes:

* sort #includes
* fix typo in Kconfig
* Add Documentation text file

v3 changes:

* fix typos in docs
* rename "leds class" to "LED class" in docs
* rebase on linux-leds/for-next

Documentation/leds/uleds.txt | 105 ++++++++++++++++++++
drivers/leds/Kconfig | 8 ++
drivers/leds/Makefile | 3 +
drivers/leds/uleds.c | 224 +++++++++++++++++++++++++++++++++++++++++++
include/uapi/linux/Kbuild | 1 +
include/uapi/linux/uleds.h | 23 +++++
6 files changed, 364 insertions(+)
create mode 100644 Documentation/leds/uleds.txt
create mode 100644 drivers/leds/uleds.c
create mode 100644 include/uapi/linux/uleds.h

diff --git a/Documentation/leds/uleds.txt b/Documentation/leds/uleds.txt
new file mode 100644

index 0000000..de11dbb

--- /dev/null
+++ b/Documentation/leds/uleds.txt
@@ -0,0 +1,105 @@
+Userspace LEDs
+==============
+
+The uleds driver supports userspace LEDs. This can be useful for testing
+triggers and can also be used to implement virtual LEDs.
+
+
+Usage
+=====
+

+When the driver is loaded, a character device is created at /dev/uleds. To
+create a new LED class device, open /dev/uleds and write a uleds_user_dev
+structure to it (found in kernel public header file linux/uleds.h).

+
+ #define ULEDS_MAX_NAME_SIZE 80
+
+ struct uleds_user_dev {
+ char name[ULEDS_MAX_NAME_SIZE];
+ };

+
+A new LED class device will be created with the name given. The name can be
+any valid file name, but consider using the LED class naming convention of

+"devicename:color:function".
+
+The current brightness is found by reading a single byte from the character
+device. Values are unsigned: 0 to 255. Reading does not block and always returns
+the most recent brightness value. The device node can also be polled to notify
+when the brightness value changes.
+

+The LED class device will be removed when the open file handle to /dev/uleds

+is closed.
+
+
+Example
+=======
+
+/*
+ * uledmon.c
+ *

+ * This program creates a new userspace LED class device and monitors it. A

+ * timestamp and brightness value is printed each time the brightness changes.
+ *
+ * Usage: uledmon <device-name>
+ *

+ * <device-name> is the name of the LED class device to be created. Pressing

index 7a628c6..5fd3f4c 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -659,6 +659,14 @@ config LEDS_MLXCPLD
This option enabled support for the LEDs on the Mellanox

boards. Say Y to enabled these.

+config LEDS_USER
+ tristate "Userspace LED support"
+ depends on LEDS_CLASS
+ help
+ This option enables support for userspace LEDs. Say 'y' to enable this
+ support in kernel. To compile this driver as a module, choose 'm' here:
+ the module will be called uleds.
+
comment "LED Triggers"
source "drivers/leds/trigger/Kconfig"

diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile

index 3965070..d5331ff 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -75,5 +75,8 @@ obj-$(CONFIG_LEDS_MLXCPLD) += leds-mlxcpld.o

2.7.4

[David Lechner]

On 09/09/2016 02:30 PM, Kelly French wrote:
>
> Replying privately, as I'm not involved in the kernel community.
>

By offering your opinion, you are now involved in the community. No need
to be shy. :-)

> I'm looking forward to this functionality. I'm a little worried about
> the interface. Is it possible to create the new uled devices in some
> other way?
>
> Maybe one insmod per uled device? Maybe a new /proc/ entry (yuck) where
> you can echo the name into it, which will trigger udev to create the
> appropraite /dev/uled-name device?
>
> I'm hoping that the final interface is something I can just "cat
> /dev/uled-name" instead of having to write a binary struct into it at
> the beginning. I'm even ok with just echoing text into the dev entry
> before reading. It's the binary struct that's bugging me.
>


I originally considered using configfs to setup new LEDs[1], which would
be very much like what you are suggesting, but I find the way my current
patch works to be much simpler.

With this patch, you can create multiple LEDs. You just have to open a
file handle to /dev/uleds for each LED that you want to create. Also,
the LED device is automatically destroyed when the file handle is
closed. I think this is nice because if the program that created it
crashes, then it is automatically cleaned up.

I agree it would be much more convenient to not use a binary struct, but
I think the complexity of implementing it outweighs any benefit to be
gained. And it is easy enough to use an interactive python shell to
handle the things that bash can't if the interactive part is what you
really want.



[1]: https://lkml.org/lkml/2016/7/25/505

[Jacek Anaszewski]

Hi David,

Thanks for addressing the issues. I've applied the patch to the
for-next branch of linux-leds.git, after making few more
modifications indicated below.

Thanks,
Jacek Anaszewski

On 09/09/2016 06:49 PM, David Lechner wrote:
> This driver creates a userspace leds driver similar to uinput.

s/leds/LED class/

Also in the commit title:

s/leds/LED class/

>
> New leds are created by opening /dev/uleds and writing a uleds_user_dev

s/leds/LED class devices/

> struct. A new leds class device is registered with the name given in the

s/leds/LED/

> struct. Reading will return a single byte that is the current brightness.
> The poll() syscall is also supported. It will be triggered whenever the
> brightness changes. Closing the file handle to /dev/uleds will remove
> the leds class device.

s/leds/LED/

s/leds subsystem/the LED subsystem/

s/leds subsystem/the LED subsystem/

> +MODULE_LICENSE("GPL");
> diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
> index 185f8ea..416f5e6 100644
> --- a/include/uapi/linux/Kbuild
> +++ b/include/uapi/linux/Kbuild
> @@ -421,6 +421,7 @@ header-y += udp.h
> header-y += uhid.h
> header-y += uinput.h
> header-y += uio.h
> +header-y += uleds.h
> header-y += ultrasound.h
> header-y += un.h
> header-y += unistd.h
> diff --git a/include/uapi/linux/uleds.h b/include/uapi/linux/uleds.h
> new file mode 100644
> index 0000000..e78ed46
> --- /dev/null
> +++ b/include/uapi/linux/uleds.h
> @@ -0,0 +1,23 @@
> +/*
> + * Userspace driver support for leds subsystem

s/leds subsystem/the LED subsystem/

[David Lechner]

On 09/12/2016 03:18 AM, Jacek Anaszewski wrote:
> Hi David,
>
> Thanks for addressing the issues. I've applied the patch to the
> for-next branch of linux-leds.git, after making few more
> modifications indicated below.

Great!

[Pavel Machek]

Hi!

> Thanks for the patch. It is very nice. I have only one minor remark
> in the code.
>
> I think that it would be good to add a documentation for this
> driver to Documentation/leds, with exemplary C program instead
> of python one. The program could poll the dev node in a loop,
> which allows to conveniently check the impact of setting particular
> triggers and all other brightness change events.

Actually, I'd say that python is fine -- it is used heavily in tools/.

But perhaps the example program should go to tools/?

> >def change_brightness(b):
> > with open('/sys/class/leds/{}/brightness'.format(name) , 'w') as f:
> > f.write(str(b))
> >
> >
> >with open('/dev/uleds', 'rb+', 0) as uleds:
> > bname = name.encode("ascii")
> > # create the leds class device
> > uleds.write(bname + b'\0' * (ULEDS_MAX_NAME_SIZE - len(bname)))

Umm. I don't see it in the kernel code. You let userspace provide a
LED name?

Where is the LED name tested for sanity? I guess there could be a lot
of fun naming a led ".." for example...

Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[Pavel Machek]

Hi!

> >@@ -0,0 +1,23 @@
> >+/*
> >+ * Userspace driver support for leds subsystem
> >+ *
> >+ * This program is free software; you can redistribute it and/or modify
> >+ * it under the terms of the GNU General Public License as published by
> >+ * the Free Software Foundation; either version 2 of the License, or
> >+ * (at your option) any later version.
> >+ *
> >+ * This program is distributed in the hope that it will be useful,
> >+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >+ * GNU General Public License for more details.
> >+ */
> >+#ifndef _UAPI__ULEDS_H_
> >+#define _UAPI__ULEDS_H_
> >+
> >+#define ULEDS_MAX_NAME_SIZE 80
> >+
> >+struct uleds_user_dev {
> >+ char name[ULEDS_MAX_NAME_SIZE];
> >+};

We already have path component length limit somewhere, right? Just use
it?

(And is struct with char array good idea at all? Perphaps it can just
use write() length up to something reasonable, and not bother with new
header file for userspace?)

[Pavel Machek]

Hi!

> >+ if (copy_from_user(&udev->user_dev, buffer,
> >+ sizeof(struct uleds_user_dev))) {
> >+ ret = -EFAULT;
> >+ goto out;
> >+ }
> >+
> >+ if (!udev->user_dev.name[0]) {
> >+ ret = -EINVAL;
> >+ goto out;
> >+ }
> >+
> >+ ret = led_classdev_register(NULL, &udev->led_cdev);
> >+ if (ret < 0)
> >+ goto out;

No sanity checking on the name -> probably a security hole. Do not
push this upstream before this is fixed.

Thanks,

[Pavel Machek]

Hi!

> > >+ if (copy_from_user(&udev->user_dev, buffer,
> > >+ sizeof(struct uleds_user_dev))) {
> > >+ ret = -EFAULT;
> > >+ goto out;
> > >+ }
> > >+
> > >+ if (!udev->user_dev.name[0]) {
> > >+ ret = -EINVAL;
> > >+ goto out;
> > >+ }
> > >+
> > >+ ret = led_classdev_register(NULL, &udev->led_cdev);
> > >+ if (ret < 0)
> > >+ goto out;
>
> No sanity checking on the name -> probably a security hole. Do not
> push this upstream before this is fixed.

And actually... is it possible to have more then one userspace LED
with this interface? I do have RGB LED connect on /dev/ttyUSB0, and I
guess userspace driver would be appropriate, but it needs 3
channels...

Best regards,

[Jacek Anaszewski]

Hi Pavel,

On 09/15/2016 03:35 PM, Pavel Machek wrote:
> Hi!
>
>>>> + if (copy_from_user(&udev->user_dev, buffer,
>>>> + sizeof(struct uleds_user_dev))) {
>>>> + ret = -EFAULT;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + if (!udev->user_dev.name[0]) {
>>>> + ret = -EINVAL;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + ret = led_classdev_register(NULL, &udev->led_cdev);
>>>> + if (ret < 0)
>>>> + goto out;
>>
>> No sanity checking on the name -> probably a security hole. Do not
>> push this upstream before this is fixed.
>
> And actually... is it possible to have more then one userspace LED
> with this interface? I do have RGB LED connect on /dev/ttyUSB0, and I
> guess userspace driver would be appropriate, but it needs 3
> channels...

Each new successful write to /dev/uleds creates a new LED class device.

See Documentation/leds/uleds.txt.

[Jacek Anaszewski]

Hi Pavel,

In fact in this case the addition of another public header can be
avoided.

[Jacek Anaszewski]

Hi Pavel,

On 09/15/2016 02:41 PM, Pavel Machek wrote:
> Hi!
>
>> Thanks for the patch. It is very nice. I have only one minor remark
>> in the code.
>>
>> I think that it would be good to add a documentation for this
>> driver to Documentation/leds, with exemplary C program instead
>> of python one. The program could poll the dev node in a loop,
>> which allows to conveniently check the impact of setting particular
>> triggers and all other brightness change events.
>
> Actually, I'd say that python is fine -- it is used heavily in tools/.
>
> But perhaps the example program should go to tools/?

I see python scripts only in perf tools. Let's stay by C application.

David, could you please prepare another version of the patch, that
splits test app code to the separate file in tools/leds (the directory
doesn't exist so far), also providing suitable Makefile?

>>> def change_brightness(b):
>>> with open('/sys/class/leds/{}/brightness'.format(name) , 'w') as f:
>>> f.write(str(b))
>>>
>>>
>>> with open('/dev/uleds', 'rb+', 0) as uleds:
>>> bname = name.encode("ascii")
>>> # create the leds class device
>>> uleds.write(bname + b'\0' * (ULEDS_MAX_NAME_SIZE - len(bname)))
>
> Umm. I don't see it in the kernel code. You let userspace provide a
> LED name?
>
> Where is the LED name tested for sanity? I guess there could be a lot
> of fun naming a led ".." for example...

Good point.

[Jacek Anaszewski]

Hi Pavel,

On 09/15/2016 03:08 PM, Pavel Machek wrote:
> Hi!
>
>>> + if (copy_from_user(&udev->user_dev, buffer,
>>> + sizeof(struct uleds_user_dev))) {
>>> + ret = -EFAULT;
>>> + goto out;
>>> + }
>>> +
>>> + if (!udev->user_dev.name[0]) {
>>> + ret = -EINVAL;
>>> + goto out;
>>> + }
>>> +
>>> + ret = led_classdev_register(NULL, &udev->led_cdev);
>>> + if (ret < 0)
>>> + goto out;
>
> No sanity checking on the name -> probably a security hole. Do not
> push this upstream before this is fixed.

Thanks for catching this.

David, please check if the LED name sticks to the LED class
device naming convention.

And one thing that caught my eye only now - please use
devm_led_classdev_register().

For now I'm dropping the patch.

[David Lechner]

The main reason I did it this way is in case someone wants to extend
this to also, for example, set the max_brightness value. If we use an
arbitrary size string, we could never add max_brightness without
breaking userspace.

If we are sure we will never want to pass any other parameters other
than name, then we can do away with the struct.

[David Lechner]

On 09/15/2016 09:54 AM, Jacek Anaszewski wrote:

> Hi Pavel,
>
> On 09/15/2016 03:08 PM, Pavel Machek wrote:
>> Hi!
>>
>>>> + if (copy_from_user(&udev->user_dev, buffer,
>>>> + sizeof(struct uleds_user_dev))) {
>>>> + ret = -EFAULT;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + if (!udev->user_dev.name[0]) {
>>>> + ret = -EINVAL;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + ret = led_classdev_register(NULL, &udev->led_cdev);
>>>> + if (ret < 0)
>>>> + goto out;
>>
>> No sanity checking on the name -> probably a security hole. Do not
>> push this upstream before this is fixed.
>
> Thanks for catching this.
>
> David, please check if the LED name sticks to the LED class
> device naming convention.

I don't think it is a good idea to enforce the LED class naming
convention. Someone may have a userspace application they want to test
that has a hard-coded name that does not follow the convention.

>
> And one thing that caught my eye only now - please use
> devm_led_classdev_register().

How do I use devm_* when there is no parent device?

[David Lechner]

On 09/15/2016 10:31 AM, David Lechner wrote:

>
> How do I use devm_* when there is no parent device?
>
>

Answering my own question. I seen now that struct miscdevice has struct
device that should be used as the parent.

[David Lechner]

On 09/15/2016 09:54 AM, Jacek Anaszewski wrote:

As a matter of fact, in led_classdev_register(), the name is limited to
64. This is hard-coded (no #define).

char name[64];

The given name is truncated to this length in a call to
led_classdev_next_name().

[David Lechner]

On 09/15/2016 09:54 AM, Jacek Anaszewski wrote:

> Hi Pavel,
>
> On 09/15/2016 03:08 PM, Pavel Machek wrote:
>> Hi!
>>
>>>> + if (copy_from_user(&udev->user_dev, buffer,
>>>> + sizeof(struct uleds_user_dev))) {
>>>> + ret = -EFAULT;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + if (!udev->user_dev.name[0]) {
>>>> + ret = -EINVAL;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + ret = led_classdev_register(NULL, &udev->led_cdev);
>>>> + if (ret < 0)
>>>> + goto out;
>>
>> No sanity checking on the name -> probably a security hole. Do not
>> push this upstream before this is fixed.
>

If this is a serious security issue, then you should also raise an issue
with input maintainers because this is the extent of sanity checking for
uinput device names as well.

I must confess that I am no security expert, so unless you can give
specific examples of what potential threats are, I will not be able to
guess what I need to do to fix it.

After some digging around the kernel, I don't see many instances of
validating device node names. The best I have found so far comes from
create_entry() in binfmt_misc.c

if (!e->name[0] ||
!strcmp(e->name, ".") ||
!strcmp(e->name, "..") ||
strchr(e->name, '/'))
goto einval;

Would something like this be a sufficient sanity check? I suppose we
could also check for non-printing characters, but I don't think ignoring
them would be a security issue.

[Pavel Machek]

Hi!

+The current brightness is found by reading a single byte from the
character
+device. Values are unsigned: 0 to 255. Reading does not block and
always returns
+the most recent brightness value. The device node can also be polled
to notify
+when the brightness value changes.

What is going on there? We have O_NONBLOCK, user should be able to
select if he wants blocking behaviour or not.

And yes, there's interface for creating a LED, but not one for
deleting it?

And... how is it going to work with multiple LEDs? Userspace gets
single bytes with brightness. How does it know to which LED the
brightness belongs?

[Pavel Machek]

Hi!

> >>>>+ if (copy_from_user(&udev->user_dev, buffer,
> >>>>+ sizeof(struct uleds_user_dev))) {
> >>>>+ ret = -EFAULT;
> >>>>+ goto out;
> >>>>+ }
> >>>>+
> >>>>+ if (!udev->user_dev.name[0]) {
> >>>>+ ret = -EINVAL;
> >>>>+ goto out;
> >>>>+ }
> >>>>+
> >>>>+ ret = led_classdev_register(NULL, &udev->led_cdev);
> >>>>+ if (ret < 0)
> >>>>+ goto out;
> >>
> >>No sanity checking on the name -> probably a security hole. Do not
> >>push this upstream before this is fixed.
> >
>
> If this is a serious security issue, then you should also raise an issue
> with input maintainers because this is the extent of sanity checking for
> uinput device names as well.

I guess that should be fixed. But lets not add new ones.

> I must confess that I am no security expert, so unless you can give specific
> examples of what potential threats are, I will not be able to guess what I
> need to do to fix it.
>
> After some digging around the kernel, I don't see many instances of
> validating device node names. The best I have found so far comes from
> create_entry() in binfmt_misc.c
>
> if (!e->name[0] ||
> !strcmp(e->name, ".") ||
> !strcmp(e->name, "..") ||
> strchr(e->name, '/'))
> goto einval;
>
> Would something like this be a sufficient sanity check? I suppose we could
> also check for non-printing characters, but I don't think ignoring them
> would be a security issue.

That would be minimum, yes. I guess it would be better/easier to just
limit the names to [a-zA-Z:-_0-9]*?

[Pavel Machek]

Umm.

Noone has applications with hardcoded names that are not possible
today, right?

And better not encourage crazy names.

Best regards,

[Pavel Machek]

Hi!

+static ssize_t uleds_read(struct file *file, char __user *buffer,
size_t count,
+ loff_t *ppos)
+{
+ struct uleds_device *udev = file->private_data;
+ ssize_t retval;
+
+ if (count == 0)
+ return 0;
+
+ if (count != 1)
+ return -EINVAL;

This is quite anti-social. You are free to return 1 byte on any read
(short read), but please allow reads with bigger buffers.

[Pavel Machek]

Umm. No, only one write is permitted to /dev/uleds according to the
code. In the email thread, it says:

# With this patch, you can create multiple LEDs. You just have to open a
# file handle to /dev/uleds for each LED that you want to create. Also,
# the LED device is automatically destroyed when the file handle is
# closed. I think this is nice because if the program that created it
# crashes, then it is automatically cleaned up.

...which should be actually ok. Sorry for the noise.

[Jacek Anaszewski]

Hi David,

This is sound argument. Let's limit the name size to 64, as in case
of name variable in led_classdev_register(). This patch could also
add include directive "#include <uapi/leds/uleds.h>" to
drivers/leds/led-class.c and replace 64 with a new LED_MAX_NAME_LEN
macro defined in the uleds.h header.

[Jacek Anaszewski]

Right, and we also could check if there are no more then two ":"
characters in the name.

[David Lechner]

Again, I am going to disagree here. docs/sysfs-rules.txt says nothing
about restricting characters for device names, so I don't think we
should do so here. In fact, the only thing it says about names is
"applications need to handle spaces and characters like '!' in the
name". My opinion is that if people want to give devices dumb names with
special characters and spaces, we should let them.

If someone can point out a real security issue here, then I will gladly
fix it, otherwise I am inclined to leave it as it is (with the checks
for '.', '..' and '/').

If this was a regular userspace library, I would feel differently, but
since the kernel has limited means to pass errors to userspace, all of
these checks will pass the same -EINVAL to userspace if they fail. We
could print error messages to the kernel log, but it is really annoying
to have to check the kernel log to find out why your userspace
application is not working. Any if you are not a kernel hacker, you
would probably not even know to check the kernel logs.

[David Lechner]

Here is the actual `ls /sys/class/leds` from my Raspberry Pi:

led0 pistorms:BA:red:ev3dev pistorms:BB:red:ev3dev
pistorms:BA:blue:ev3dev pistorms:BB:blue:ev3dev
pistorms:BA:green:ev3dev pistorms:BB:green:ev3dev



Suppose I want to use uleds on my desktop to simulate my Raspberry Pi.
If we restrict the name to the LEDs class convention of
device:color:function, then I can't do this. led0 does not follow the
convention at all. The other do follow the convention, but only if we
allow that the device portion of the name can also include ':'.

It's too late, the crazy names already exist.

[David Lechner]

On 09/16/2016 12:59 AM, Pavel Machek wrote:
> Hi!
>
> +The current brightness is found by reading a single byte from the
> character
> +device. Values are unsigned: 0 to 255. Reading does not block and
> always returns
> +the most recent brightness value. The device node can also be polled
> to notify
> +when the brightness value changes.
>
> What is going on there? We have O_NONBLOCK, user should be able to
> select if he wants blocking behaviour or not.

I will look into this.

>
> And yes, there's interface for creating a LED, but not one for
> deleting it?

uleds.txt says "The LED class device will be removed when the open file
handle to /dev/uleds is closed."

Is this not clear?

>
> And... how is it going to work with multiple LEDs? Userspace gets
> single bytes with brightness. How does it know to which LED the
> brightness belongs?

It looks like this is missing from uleds.txt. By opening multiple file
handles to /dev/uleds, you can create multiple devices. Each file handle
will return a different value when read that corresponds to the LEDs
class device that belongs to it.

[David Lechner]

On 09/16/2016 01:07 AM, Pavel Machek wrote:
> Hi!
>
> +static ssize_t uleds_read(struct file *file, char __user *buffer,
> size_t count,
> + loff_t *ppos)
> +{
> + struct uleds_device *udev = file->private_data;
> + ssize_t retval;
> +
> + if (count == 0)
> + return 0;
> +
> + if (count != 1)
> + return -EINVAL;
>
> This is quite anti-social. You are free to return 1 byte on any read
> (short read), but please allow reads with bigger buffers.
>
> Thanks,
> Pavel
>

Sure.

[David Lechner]

v2 changes:

* sort #includes
* fix typo in Kconfig
* Add Documentation text file

v3 changes:

* fix typos in docs
* rename "leds class" to "LED class" in docs
* rebase on linux-leds/for-next

v4 changes:

* Use devm_led_classdev_register() instead of led_classdev_register()
* Clarified how to create multiple devices in documentation
* uledmon.c now uses blocking reads instead of poll()
* Add some sanity checking on user-provided device name to make sure it is not
a directory name
* Reading from /dev/uleds allows any size read buffer - still only returns one
byte regardless of size
* Reading from /dev/uleds blocks until the brightness is changed
* LEDS_MAX_NAME_SIZE is reduced to 64 to match existing name size limit
* New patch to use LEDS_MAX_NAME_SIZE in drivers/leds/led-class.c
* Moved example code to tool/leds/uledmon.c (new patch with Makefile)

David Lechner (3):
leds: Introduce userspace leds driver
leds: Use macro for max device node name size
tools/leds: Add uledmon program for monitoring userspace LEDs

Documentation/leds/uleds.txt | 36 +++++++
drivers/leds/Kconfig | 8 ++
drivers/leds/Makefile | 3 +
drivers/leds/led-class.c | 3 +-
drivers/leds/uleds.c | 230 +++++++++++++++++++++++++++++++++++++++++++
include/uapi/linux/Kbuild | 1 +
include/uapi/linux/uleds.h | 23 +++++
tools/Makefile | 7 +-
tools/leds/.gitignore | 1 +
tools/leds/Makefile | 13 +++
tools/leds/uledmon.c | 62 ++++++++++++
11 files changed, 383 insertions(+), 4 deletions(-)
create mode 100644 Documentation/leds/uleds.txt
create mode 100644 drivers/leds/uleds.c
create mode 100644 include/uapi/linux/uleds.h
create mode 100644 tools/leds/.gitignore
create mode 100644 tools/leds/Makefile
create mode 100644 tools/leds/uledmon.c

--
2.7.4

[David Lechner]

This driver creates a userspace leds driver similar to uinput.

New leds are created by opening /dev/uleds and writing a uleds_user_dev
struct. A new leds class device is registered with the name given in the
struct. Reading will return a single byte that is the current brightness.
The poll() syscall is also supported. It will be triggered whenever the
brightness changes. Closing the file handle to /dev/uleds will remove
the leds class device.

Signed-off-by: David Lechner <da...@lechnology.com>
---

Documentation/leds/uleds.txt | 36 +++++++
drivers/leds/Kconfig | 8 ++
drivers/leds/Makefile | 3 +

drivers/leds/uleds.c | 230 +++++++++++++++++++++++++++++++++++++++++++
include/uapi/linux/Kbuild | 1 +
include/uapi/linux/uleds.h | 23 +++++

6 files changed, 301 insertions(+)

create mode 100644 Documentation/leds/uleds.txt
create mode 100644 drivers/leds/uleds.c
create mode 100644 include/uapi/linux/uleds.h

diff --git a/Documentation/leds/uleds.txt b/Documentation/leds/uleds.txt
new file mode 100644
index 0000000..486d473
--- /dev/null
+++ b/Documentation/leds/uleds.txt
@@ -0,0 +1,36 @@
+Userspace LEDs
+==============
+
+The uleds driver supports userspace LEDs. This can be useful for testing
+triggers and can also be used to implement virtual LEDs.
+
+
+Usage
+=====
+
+When the driver is loaded, a character device is created at /dev/uleds. To
+create a new LED class device, open /dev/uleds and write a uleds_user_dev
+structure to it (found in kernel public header file linux/uleds.h).
+
+ #define LEDS_MAX_NAME_SIZE 64
+
+ struct uleds_user_dev {
+ char name[LEDS_MAX_NAME_SIZE];
+ };
+
+A new LED class device will be created with the name given. The name can be
+any valid sysfs device node name, but consider using the LED class naming
+convention of "devicename:color:function".
+

+The current brightness is found by reading a single byte from the character

+device. Values are unsigned: 0 to 255. Reading will block until the brightness
+changes. The device node can also be polled to notify when the brightness value
+changes.
+
+The LED class device will be removed when the open file handle to /dev/uleds
+is closed.
+
+Multiple LED class devices are created by opening additional file handles to
+/dev/uleds.
+
+See tools/leds/uledmon.c for an example userspace program.
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 7a628c6..5fd3f4c 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -659,6 +659,14 @@ config LEDS_MLXCPLD
This option enabled support for the LEDs on the Mellanox
boards. Say Y to enabled these.

+config LEDS_USER
+ tristate "Userspace LED support"
+ depends on LEDS_CLASS
+ help
+ This option enables support for userspace LEDs. Say 'y' to enable this
+ support in kernel. To compile this driver as a module, choose 'm' here:
+ the module will be called uleds.
+
comment "LED Triggers"
source "drivers/leds/trigger/Kconfig"

diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 3965070..d5331ff 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -75,5 +75,8 @@ obj-$(CONFIG_LEDS_MLXCPLD) += leds-mlxcpld.o
# LED SPI Drivers
obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o

+# LED Userspace Drivers
+obj-$(CONFIG_LEDS_USER) += uleds.o
+
# LED Triggers
obj-$(CONFIG_LEDS_TRIGGERS) += trigger/
diff --git a/drivers/leds/uleds.c b/drivers/leds/uleds.c
new file mode 100644
index 0000000..59ea23e
--- /dev/null
+++ b/drivers/leds/uleds.c
@@ -0,0 +1,229 @@
+/*
+ * Userspace driver for leds subsystem
+ *
+ * Copyright (C) 2016 David Lechner <da...@lechnology.com>
+ *
+ * Based on uinput.c: Aristeu Sergio Rozanski Filho <ar...@cathedrallabs.org>

+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */

+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/leds.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/poll.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+
+#include <uapi/linux/uleds.h>
+
+#define ULEDS_NAME "uleds"
+
+enum uleds_state {
+ ULEDS_STATE_UNKNOWN,
+ ULEDS_STATE_REGISTERED,
+};
+
+struct uleds_device {
+ struct uleds_user_dev user_dev;
+ struct led_classdev led_cdev;
+ struct mutex mutex;
+ enum uleds_state state;
+ wait_queue_head_t waitq;
+ unsigned char brightness;
+ bool new_data;
+};
+
+static struct miscdevice uleds_misc;
+
+static void uleds_brightness_set(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ struct uleds_device *udev = container_of(led_cdev, struct uleds_device,
+ led_cdev);
+
+ if (udev->brightness != brightness) {
+ udev->brightness = brightness;
+ udev->new_data = true;
+ wake_up_interruptible(&udev->waitq);
+ }
+}
+
+static int uleds_open(struct inode *inode, struct file *file)
+{
+ struct uleds_device *udev;
+
+ udev = kzalloc(sizeof(*udev), GFP_KERNEL);
+ if (!udev)
+ return -ENOMEM;
+
+ udev->led_cdev.name = udev->user_dev.name;
+ udev->led_cdev.max_brightness = LED_FULL;
+ udev->led_cdev.brightness_set = uleds_brightness_set;
+
+ mutex_init(&udev->mutex);
+ init_waitqueue_head(&udev->waitq);
+ udev->state = ULEDS_STATE_UNKNOWN;
+
+ file->private_data = udev;
+ nonseekable_open(inode, file);
+

+ return 0;
+}
+

+static ssize_t uleds_write(struct file *file, const char __user *buffer,
+ size_t count, loff_t *ppos)

+{
+ struct uleds_device *udev = file->private_data;

+ const char *name;
+ int ret;

+
+ if (count == 0)
+ return 0;
+

+ ret = mutex_lock_interruptible(&udev->mutex);
+ if (ret)
+ return ret;
+
+ if (udev->state == ULEDS_STATE_REGISTERED) {
+ ret = -EBUSY;
+ goto out;
+ }
+
+ if (count != sizeof(struct uleds_user_dev)) {

+ ret = -EINVAL;
+ goto out;
+ }
+

+ if (copy_from_user(&udev->user_dev, buffer,
+ sizeof(struct uleds_user_dev))) {
+ ret = -EFAULT;
+ goto out;
+ }
+

+ name = udev->user_dev.name;
+ if (!name[0] || !strcmp(name, ".") || !strcmp(name, "..") ||
+ strchr(name, '/')) {

+ ret = -EINVAL;
+ goto out;
+ }
+

+ ret = devm_led_classdev_register(uleds_misc.this_device,
+ &udev->led_cdev);

+ if (ret < 0)
+ goto out;

+
+ udev->new_data = true;
+ udev->state = ULEDS_STATE_REGISTERED;
+ ret = count;
+
+out:
+ mutex_unlock(&udev->mutex);
+
+ return ret;
+}
+

+static ssize_t uleds_read(struct file *file, char __user *buffer, size_t count,
+ loff_t *ppos)
+{
+ struct uleds_device *udev = file->private_data;
+ ssize_t retval;
+
+ if (count == 0)
+ return 0;
+

+ do {
+ retval = mutex_lock_interruptible(&udev->mutex);
+ if (retval)
+ return retval;
+
+ if (udev->state != ULEDS_STATE_REGISTERED) {
+ retval = -ENODEV;
+ } else if (!udev->new_data && (file->f_flags & O_NONBLOCK)) {
+ retval = -EAGAIN;
+ } else if (udev->new_data) {
+ retval = copy_to_user(buffer, &udev->brightness, 1);
+ udev->new_data = false;
+ retval = 1;
+ }
+
+ mutex_unlock(&udev->mutex);
+
+ if (retval)
+ break;
+
+ if (!(file->f_flags & O_NONBLOCK))
+ retval = wait_event_interruptible(udev->waitq,
+ udev->new_data ||
+ udev->state != ULEDS_STATE_REGISTERED);
+ } while (retval == 0);
+
+ return retval;
+}
+
+static unsigned int uleds_poll(struct file *file, poll_table *wait)

+{
+ struct uleds_device *udev = file->private_data;
+

+ poll_wait(file, &udev->waitq, wait);
+
+ if (udev->new_data)
+ return POLLIN | POLLRDNORM;
+

+ return 0;
+}
+

+static int uleds_release(struct inode *inode, struct file *file)

+{
+ struct uleds_device *udev = file->private_data;
+

+ if (udev->state == ULEDS_STATE_REGISTERED) {
+ udev->state = ULEDS_STATE_UNKNOWN;
+ devm_led_classdev_unregister(uleds_misc.this_device,
+ &udev->led_cdev);
+ }
+ kfree(udev);
+

+ return 0;
+}
+

+static const struct file_operations uleds_fops = {
+ .owner = THIS_MODULE,
+ .open = uleds_open,
+ .release = uleds_release,
+ .read = uleds_read,
+ .write = uleds_write,
+ .poll = uleds_poll,
+ .llseek = no_llseek,
+};
+
+static struct miscdevice uleds_misc = {
+ .fops = &uleds_fops,
+ .minor = MISC_DYNAMIC_MINOR,
+ .name = ULEDS_NAME,
+};
+
+static int __init uleds_init(void)
+{
+ return misc_register(&uleds_misc);
+}
+module_init(uleds_init);
+
+static void __exit uleds_exit(void)
+{
+ misc_deregister(&uleds_misc);
+}
+module_exit(uleds_exit);
+
+MODULE_AUTHOR("David Lechner <da...@lechnology.com>");
+MODULE_DESCRIPTION("Userspace driver for leds subsystem");
+MODULE_LICENSE("GPL");
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
index 185f8ea..416f5e6 100644
--- a/include/uapi/linux/Kbuild
+++ b/include/uapi/linux/Kbuild
@@ -421,6 +421,7 @@ header-y += udp.h
header-y += uhid.h
header-y += uinput.h
header-y += uio.h
+header-y += uleds.h
header-y += ultrasound.h
header-y += un.h
header-y += unistd.h
diff --git a/include/uapi/linux/uleds.h b/include/uapi/linux/uleds.h
new file mode 100644
index 0000000..6f277f3
--- /dev/null
+++ b/include/uapi/linux/uleds.h

@@ -0,0 +1,23 @@
+/*
+ * Userspace driver support for leds subsystem
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#ifndef _UAPI__ULEDS_H_
+#define _UAPI__ULEDS_H_
+

+#define LEDS_MAX_NAME_SIZE 64
+
+struct uleds_user_dev {
+ char name[LEDS_MAX_NAME_SIZE];
+};
+
+#endif /* _UAPI__ULEDS_H_ */
--
2.7.4

[David Lechner]

The uleds driver provides userspace LED devices. This tool is used to
create one of these devices and monitor the changes in brighness for
testing purposes.

Signed-off-by: David Lechner <da...@lechnology.com>
---

tools/Makefile | 7 +++---
tools/leds/.gitignore | 1 +
tools/leds/Makefile | 13 +++++++++++
tools/leds/uledmon.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 80 insertions(+), 3 deletions(-)

create mode 100644 tools/leds/.gitignore
create mode 100644 tools/leds/Makefile
create mode 100644 tools/leds/uledmon.c

diff --git a/tools/Makefile b/tools/Makefile
index daa8fb3..00caacd 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -17,6 +17,7 @@ help:
@echo ' hv - tools used when in Hyper-V clients'
@echo ' iio - IIO tools'
@echo ' kvm_stat - top-like utility for displaying kvm statistics'
+ @echo ' leds - LEDs tools'
@echo ' lguest - a minimal 32-bit x86 hypervisor'
@echo ' net - misc networking tools'
@echo ' perf - Linux performance measurement and analysis tool'
@@ -56,7 +57,7 @@ acpi: FORCE
cpupower: FORCE
$(call descend,power/$@)

-cgroup firewire hv guest spi usb virtio vm net iio gpio objtool: FORCE
+cgroup firewire hv guest spi usb virtio vm net iio gpio objtool leds: FORCE
$(call descend,$@)

liblockdep: FORCE
@@ -126,7 +127,7 @@ acpi_clean:
cpupower_clean:
$(call descend,power/cpupower,clean)

-cgroup_clean hv_clean firewire_clean lguest_clean spi_clean usb_clean virtio_clean vm_clean net_clean iio_clean gpio_clean objtool_clean:
+cgroup_clean hv_clean firewire_clean lguest_clean spi_clean usb_clean virtio_clean vm_clean net_clean iio_clean gpio_clean objtool_clean leds_clean:
$(call descend,$(@:_clean=),clean)

liblockdep_clean:
@@ -164,6 +165,6 @@ clean: acpi_clean cgroup_clean cpupower_clean hv_clean firewire_clean lguest_cle
perf_clean selftests_clean turbostat_clean spi_clean usb_clean virtio_clean \
vm_clean net_clean iio_clean x86_energy_perf_policy_clean tmon_clean \
freefall_clean build_clean libbpf_clean libsubcmd_clean liblockdep_clean \
- gpio_clean objtool_clean
+ gpio_clean objtool_clean leds_clean

.PHONY: FORCE
diff --git a/tools/leds/.gitignore b/tools/leds/.gitignore
new file mode 100644
index 0000000..ac96d9f
--- /dev/null
+++ b/tools/leds/.gitignore
@@ -0,0 +1 @@
+uledmon
diff --git a/tools/leds/Makefile b/tools/leds/Makefile
new file mode 100644
index 0000000..c03a79e
--- /dev/null
+++ b/tools/leds/Makefile
@@ -0,0 +1,13 @@
+# Makefile for LEDs tools
+
+CC = $(CROSS_COMPILE)gcc
+CFLAGS = -Wall -Wextra -g -I../../include/uapi
+
+all: uledmon
+%: %.c
+ $(CC) $(CFLAGS) -o $@ $^
+
+clean:
+ $(RM) uledmon
+
+.PHONY: all clean
diff --git a/tools/leds/uledmon.c b/tools/leds/uledmon.c
new file mode 100644
index 0000000..44d8634
--- /dev/null
+++ b/tools/leds/uledmon.c
@@ -0,0 +1,62 @@
+/*
+ * uledmon.c
+ *
+ * This program creates a new userspace LED class device and monitors it. A
+ * timestamp and brightness value is printed each time the brightness changes.
+ *
+ * Usage: uledmon <device-name>
+ *
+ * <device-name> is the name of the LED class device to be created. Pressing
+ * CTRL+C will exit.
+ */
+
+#include <fcntl.h>
+#include <stdio.h>
+#include <string.h>
+#include <time.h>
+#include <unistd.h>
+
+#include <linux/uleds.h>
+
+int main(int argc, char const *argv[])
+{
+ struct uleds_user_dev uleds_dev;
+ int fd, ret;
+ unsigned char brightness;
+ struct timespec ts;
+
+ if (argc != 2) {
+ fprintf(stderr, "Requires <device-name> argument\n");
+ return 1;
+ }
+
+ strncpy(uleds_dev.name, argv[1], LEDS_MAX_NAME_SIZE);
+
+ fd = open("/dev/uleds", O_RDWR);
+ if (fd == -1) {
+ perror("Failed to open /dev/uleds");
+ return 1;
+ }
+
+ ret = write(fd, &uleds_dev, sizeof(uleds_dev));
+ if (ret == -1) {
+ perror("Failed to write to /dev/uleds");
+ close(fd);
+ return 1;
+ }
+
+ while (1) {
+ ret = read(fd, &brightness, 1);
+ if (ret == -1) {
+ perror("Failed to read from /dev/uleds");
+ close(fd);
+ return 1;
+ }
+ clock_gettime(CLOCK_MONOTONIC, &ts);
+ printf("[%ld.%09ld] %u\n", ts.tv_sec, ts.tv_nsec, brightness);
+ }
+
+ close(fd);

+
+ return 0;
+}

--
2.7.4

[Pavel Machek]

Hi!

> >>>After some digging around the kernel, I don't see many instances of
> >>>validating device node names. The best I have found so far comes from
> >>>create_entry() in binfmt_misc.c
> >>>
> >>> if (!e->name[0] ||
> >>> !strcmp(e->name, ".") ||
> >>> !strcmp(e->name, "..") ||
> >>> strchr(e->name, '/'))
> >>> goto einval;
> >>>
> >>>Would something like this be a sufficient sanity check? I suppose we
> >>>could
> >>>also check for non-printing characters, but I don't think ignoring them
> >>>would be a security issue.
> >>
> >>That would be minimum, yes. I guess it would be better/easier to just
> >>limit the names to [a-zA-Z:-_0-9]*?
> >
> >Right, and we also could check if there are no more then two ":"
> >characters in the name.
> >
>
> Again, I am going to disagree here. docs/sysfs-rules.txt says nothing about
> restricting characters for device names, so I don't think we should do so
> here. In fact, the only thing it says about names is "applications need to
> handle spaces and characters like '!' in the name". My opinion is that if
> people want to give devices dumb names with special characters and spaces,
> we should let them.

You should be able to emulate your leds on the rapsperry pi. So
checking number of :'s does not make sense.

OTOH having a LED called ^[c that clears your screen, or having
invalid utf-8 in name .. is just going to cause problems for someone,
somewhere. Perhaps you can even use mouse reporting escape sequences
to prepare some nice surprise for admin doing "dmesg". Don't go
there.. please.

> If someone can point out a real security issue here, then I will gladly fix
> it, otherwise I am inclined to leave it as it is (with the checks for '.',
> '..' and '/').

Thanks for those checks.

But I'd really disallow control characters (<0x20), space and
non-ascii stuff (>0x7f). Yes, userspace _should_ handle that ok, but
device names usually don't contain crazy characters, I'm pretty sure
there is printk() with something like that (which would have sideeffects)..

> If this was a regular userspace library, I would feel differently, but since
> the kernel has limited means to pass errors to userspace, all of these
> checks will pass the same -EINVAL to userspace if they fail. We could print
> error messages to the kernel log, but it is really annoying to have to check
> the kernel log to find out why your userspace application is not working.
> Any if you are not a kernel hacker, you would probably not even know to
> check the kernel logs.

People doing device drivers normally know about printk()... (and I
don't expect people to hit the name limits too often.)

But people are normally careless, and do dangerous stuff such as
"dmesg" and "ls /sys/class/leds". If those can contain crazy
characters, bad things can happen.

Best regards,

[Jacek Anaszewski]

Hi David,

Thanks for applying the remarks. Since it is close to the merge
window now, I'm applying the patch set to the for-4.10 branch of
linux-leds.git.

Thanks,
Jacek Anaszewski

[Linus Walleij]

On Thu, Sep 22, 2016 at 3:43 PM, Linus Walleij <linus....@linaro.org> wrote:

> On Fri, Sep 16, 2016 at 9:16 PM, David Lechner <da...@lechnology.com> wrote:

>> +See tools/leds/uledmon.c for an example userspace program.
>

> Where is this program? Not in this patch for sure.

Ah I found it by looking a bit closer at my mail archive, sorry
about the fuzz.

Yours,
Linus Walleij

[Linus Walleij]

On Fri, Sep 16, 2016 at 9:16 PM, David Lechner <da...@lechnology.com> wrote:

> This driver creates a userspace leds driver similar to uinput.

Just out of curiosity: what is this used for?
The typical usecase evades me, so I'd really want to know.

> +See tools/leds/uledmon.c for an example userspace program.

Where is this program? Not in this patch for sure.

Yours,
Linus Walleij

[David Lechner]

On 09/22/2016 08:43 AM, Linus Walleij wrote:
> On Fri, Sep 16, 2016 at 9:16 PM, David Lechner <da...@lechnology.com> wrote:
>
>> This driver creates a userspace leds driver similar to uinput.
>
> Just out of curiosity: what is this used for?
> The typical usecase evades me, so I'd really want to know.
>

Several use cases are explained in this thread:
https://lkml.org/lkml/2016/7/25/505

[Pavel Machek]

On Thu 2016-09-22 15:43:35, Linus Walleij wrote:
> On Fri, Sep 16, 2016 at 9:16 PM, David Lechner <da...@lechnology.com> wrote:
>
> > This driver creates a userspace leds driver similar to uinput.
>
> Just out of curiosity: what is this used for?
> The typical usecase evades me, so I'd really want to know.

I have RGB led connected using ESP board over ttyUSB0. I guess I'll
want to keep the driver in userspace, but I still may want "normal"
/sys/class/leds interface for it.

[Jacek Anaszewski]

Hi David,

On 09/16/2016 09:16 PM, David Lechner wrote:

I've just noticed that this is wrong, since LED subsystem
brightness type is enum led_brightness, i.e. int.
LED_FULL (255) value is a legacy enum value that can be overridden
by max_brightness property.

Please submit a fix so that I could merge it with the original
patch before sending it upstream.

Thanks,
Jacek Anaszewski

[David Lechner]

On 11/8/16 5:26 AM, Jacek Anaszewski wrote:
> Hi David,

>

>> +struct uleds_device {
>> + struct uleds_user_dev user_dev;
>> + struct led_classdev led_cdev;
>> + struct mutex mutex;
>> + enum uleds_state state;
>> + wait_queue_head_t waitq;
>> + unsigned char brightness;
>
> I've just noticed that this is wrong, since LED subsystem
> brightness type is enum led_brightness, i.e. int.
> LED_FULL (255) value is a legacy enum value that can be overridden
> by max_brightness property.
>
> Please submit a fix so that I could merge it with the original
> patch before sending it upstream.
>
> Thanks,
> Jacek Anaszewski
>

The brightness should be a 32-bit integer then?

[Jacek Anaszewski]

Exactly.

[Pavel Machek]

Hi!

> >+struct uleds_device {
> >+ struct uleds_user_dev user_dev;
> >+ struct led_classdev led_cdev;
> >+ struct mutex mutex;
> >+ enum uleds_state state;
> >+ wait_queue_head_t waitq;
> >+ unsigned char brightness;
>
> I've just noticed that this is wrong, since LED subsystem
> brightness type is enum led_brightness, i.e. int.
> LED_FULL (255) value is a legacy enum value that can be overridden
> by max_brightness property.
>
> Please submit a fix so that I could merge it with the original
> patch before sending it upstream.

Actually... perhaps you want to wait with merging the userspace driver
till the locking is solved in the LED subsystem? Maybe I'm wrong, but
I have feeling that userspace driver will have unusual requirements
w.r.t. locking, and that it would be good to have that solved,
first...

Best regards,

[Jacek Anaszewski]

Hi,

On 11/09/2016 08:05 AM, Pavel Machek wrote:
> Hi!
>
>>> +struct uleds_device {
>>> + struct uleds_user_dev user_dev;
>>> + struct led_classdev led_cdev;
>>> + struct mutex mutex;
>>> + enum uleds_state state;
>>> + wait_queue_head_t waitq;
>>> + unsigned char brightness;
>>
>> I've just noticed that this is wrong, since LED subsystem
>> brightness type is enum led_brightness, i.e. int.
>> LED_FULL (255) value is a legacy enum value that can be overridden
>> by max_brightness property.
>>
>> Please submit a fix so that I could merge it with the original
>> patch before sending it upstream.
>
> Actually... perhaps you want to wait with merging the userspace driver
> till the locking is solved in the LED subsystem? Maybe I'm wrong, but
> I have feeling that userspace driver will have unusual requirements
> w.r.t. locking, and that it would be good to have that solved,
> first...

If you think about locking between led_set_brightness() and
led_update_brightness() then we have no ready solution for that.
Do you have any?

If not then we have to live with that until one is devised.
After adding the locking in brightness_show the risk of races will be
only in case of concurrent calls to led_set_brightness() and
led_update_brightness() from kernel, whereas currently there are no
such use cases.