[PATCH v4 0/3] leds: Introduce userspace leds driver

[PATCH v4 0/3] leds: Introduce userspace leds driver

[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

[PATCH v4 1/3] 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. Closing the file handle to /dev/uleds will remove
the leds class device.

Signed-off-by: David Lechner <david@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 <david@lechnology.com>
+ *
+ * Based on uinput.c: Aristeu Sergio Rozanski Filho <aris@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 <david@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

[PATCH v4 2/3] leds: Use macro for max device node name size

[David Lechner]

Use a macro instead of hard-coding the max device node name size. The
uleds driver introduced a macro for this value, so using it.

Signed-off-by: David Lechner <david@lechnology.com>
---
 drivers/leds/led-class.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index aa84e5b..731e4eb 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -20,6 +20,7 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/timer.h>
+#include <uapi/linux/uleds.h>
 #include "leds.h"
 
 static struct class *leds_class;
@@ -187,7 +188,7 @@ static int led_classdev_next_name(const char *init_name, char *name,
  */
 int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
 {
-	char name[64];
+	char name[LEDS_MAX_NAME_SIZE];
 	int ret;
 
 	ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
-- 
2.7.4

[PATCH v4 3/3] tools/leds: Add uledmon program for monitoring userspace LEDs

[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 <david@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

Re: [PATCH v4 0/3] leds: Introduce userspace leds driver

[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

On 09/16/2016 09:16 PM, David Lechner wrote:
> 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
>

Re: [PATCH v4 1/3] leds: Introduce userspace leds driver

[Linus Walleij]

On Fri, Sep 16, 2016 at 9:16 PM, David Lechner <david@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

Re: [PATCH v4 1/3] leds: Introduce userspace leds driver

[Linus Walleij]

On Thu, Sep 22, 2016 at 3:43 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Sep 16, 2016 at 9:16 PM, David Lechner <david@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

Re: [PATCH v4 1/3] leds: Introduce userspace leds driver

[David Lechner]

On 09/22/2016 08:43 AM, Linus Walleij wrote:
> On Fri, Sep 16, 2016 at 9:16 PM, David Lechner <david@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


>> +See tools/leds/uledmon.c for an example userspace program.
>
> Where is this program? Not in this patch for sure.
>
> Yours,
> Linus Walleij
>

Re: [PATCH v4 1/3] leds: Introduce userspace leds driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 646 bytes --]

On Thu 2016-09-22 15:43:35, Linus Walleij wrote:
> On Fri, Sep 16, 2016 at 9:16 PM, David Lechner <david@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.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v4 1/3] leds: Introduce userspace leds driver

[Jacek Anaszewski]

Hi David,

On 09/16/2016 09:16 PM, David Lechner wrote:
> 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 <david@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 <david@lechnology.com>
> + *
> + * Based on uinput.c: Aristeu Sergio Rozanski Filho <aris@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;

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


> +	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 <david@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_ */
>

Re: [PATCH v4 1/3] leds: Introduce userspace leds driver

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

Re: [PATCH v4 1/3] leds: Introduce userspace leds driver

[Jacek Anaszewski]

On 11/08/2016 08:08 PM, David Lechner wrote:
>
>
> 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?

Exactly.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v4 1/3] leds: Introduce userspace leds driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1015 bytes --]

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,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v4 1/3] leds: Introduce userspace leds driver

[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.

-- 
Best regards,
Jacek Anaszewski