[PATCH v3 1/2] leds: uleds documentation fixes

[PATCH v3 1/2] leds: uleds documentation fixes

[Craig McQueen]

* Add max_brightness field to struct for setup.
* Reads are a signed int value, not a single unsigned byte value.

Signed-off-by: Craig McQueen <craig@mcqueen.au>
---
 Documentation/leds/uleds.rst | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/leds/uleds.rst b/Documentation/leds/uleds.rst
index 83221098009c..4077745dae00 100644
--- a/Documentation/leds/uleds.rst
+++ b/Documentation/leds/uleds.rst
@@ -17,16 +17,16 @@ structure to it (found in kernel public header file linux/uleds.h)::
 
     struct uleds_user_dev {
 	char name[LED_MAX_NAME_SIZE];
+	int max_brightness;
     };
 
 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 current brightness is found by reading an int value from the character
+device. 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.
-- 
2.48.1

[PATCH v3 2/2] leds: Add some setup ioctls to uleds driver

[Craig McQueen]

* Add an ioctl for setup as an alternative to doing a write.
  This is similar to the ioctl that was added to uinput for setup.
* Add an ioctl to set a default trigger for the LED.

Signed-off-by: Craig McQueen <craig@mcqueen.au>
---
 Documentation/leds/uleds.rst |   6 ++
 drivers/leds/uleds.c         | 125 ++++++++++++++++++++++++++++-------
 include/uapi/linux/uleds.h   |  30 ++++++++-
 3 files changed, 137 insertions(+), 24 deletions(-)

diff --git a/Documentation/leds/uleds.rst b/Documentation/leds/uleds.rst
index 4077745dae00..0512e577d63a 100644
--- a/Documentation/leds/uleds.rst
+++ b/Documentation/leds/uleds.rst
@@ -24,6 +24,12 @@ 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".
 
+Alternatively, setup can be done with an ioctl call, passing a pointer to
+the structure.
+
+There is also an ioctl call to configure a default trigger for the LED, by
+passing a pointer to a structure containing the name of a trigger.
+
 The current brightness is found by reading an int value from the character
 device. Reading will block until the brightness changes. The device node can
 also be polled to notify when the brightness value changes.
diff --git a/drivers/leds/uleds.c b/drivers/leds/uleds.c
index 374a841f18c3..598e376a4b1b 100644
--- a/drivers/leds/uleds.c
+++ b/drivers/leds/uleds.c
@@ -6,6 +6,7 @@
  *
  * Based on uinput.c: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
  */
+#include <linux/ctype.h>
 #include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/leds.h>
@@ -25,13 +26,14 @@ enum uleds_state {
 };
 
 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;
-	int			brightness;
-	bool			new_data;
+	struct uleds_user_dev		user_dev;
+	struct uleds_user_trigger	default_trigger;
+	struct led_classdev		led_cdev;
+	struct mutex			mutex;
+	enum uleds_state		state;
+	wait_queue_head_t		waitq;
+	int				brightness;
+	bool				new_data;
 };
 
 static struct miscdevice uleds_misc;
@@ -70,15 +72,17 @@ static int uleds_open(struct inode *inode, struct file *file)
 	return 0;
 }
 
-static ssize_t uleds_write(struct file *file, const char __user *buffer,
-			   size_t count, loff_t *ppos)
+static bool is_led_name_valid(const char *name)
 {
-	struct uleds_device *udev = file->private_data;
-	const char *name;
-	int ret;
+	return ((name[0] != '\0') &&
+		(strcmp(name, ".") != 0) &&
+		(strcmp(name, "..") != 0) &&
+		(strchr(name, '/') == NULL));
+}
 
-	if (count == 0)
-		return 0;
+static int dev_setup(struct uleds_device *udev, const char __user *buffer)
+{
+	int ret;
 
 	ret = mutex_lock_interruptible(&udev->mutex);
 	if (ret)
@@ -89,20 +93,13 @@ static ssize_t uleds_write(struct file *file, const char __user *buffer,
 		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, '/')) {
+	if (!is_led_name_valid(udev->user_dev.name)) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -120,7 +117,6 @@ static ssize_t uleds_write(struct file *file, const char __user *buffer,
 
 	udev->new_data = true;
 	udev->state = ULEDS_STATE_REGISTERED;
-	ret = count;
 
 out:
 	mutex_unlock(&udev->mutex);
@@ -128,6 +124,23 @@ static ssize_t uleds_write(struct file *file, const char __user *buffer,
 	return ret;
 }
 
+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;
+	if (count != sizeof(struct uleds_user_dev))
+		return -EINVAL;
+
+	ret = dev_setup(udev, buffer);
+	if (ret < 0)
+		return ret;
+	return count;
+}
+
 static ssize_t uleds_read(struct file *file, char __user *buffer, size_t count,
 			  loff_t *ppos)
 {
@@ -179,6 +192,71 @@ static __poll_t uleds_poll(struct file *file, poll_table *wait)
 	return 0;
 }
 
+/*
+ * Trigger name validation: Allow only alphanumeric, hyphen or underscore.
+ */
+static bool is_trigger_name_valid(const char *name)
+{
+	size_t i;
+
+	if (name[0] == '\0')
+		return false;
+
+	for (i = 0; i < TRIG_NAME_MAX; i++) {
+		if (name[i] == '\0')
+			break;
+		if (!isalnum(name[i]) && name[i] != '-' && name[i] != '_')
+			return false;
+	}
+	/* Length check and avoid any special names. */
+	return ((i < TRIG_NAME_MAX) &&
+		(strcmp(name, "default") != 0));
+}
+
+static long uleds_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct uleds_device *udev = file->private_data;
+	struct uleds_user_trigger default_trigger;
+	int retval = 0;
+
+	switch (cmd) {
+	case ULEDS_IOC_DEV_SETUP:
+		retval = dev_setup(udev, (const char __user *)arg);
+		break;
+
+	case ULEDS_IOC_SET_DEFAULT_TRIGGER:
+		retval = copy_from_user(&default_trigger,
+			(struct uleds_user_trigger __user *)arg,
+			sizeof(default_trigger));
+		if (retval)
+			return retval;
+		retval = mutex_lock_interruptible(&udev->mutex);
+		if (retval)
+			return retval;
+		if (default_trigger.name[0] == '\0') {
+			udev->led_cdev.default_trigger = NULL;
+		} else {
+			if (!is_trigger_name_valid(default_trigger.name)) {
+				mutex_unlock(&udev->mutex);
+				return -EINVAL;
+			}
+			memcpy(&udev->default_trigger, &default_trigger,
+				sizeof(udev->default_trigger));
+			udev->led_cdev.default_trigger = udev->default_trigger.name;
+		}
+		if (udev->state == ULEDS_STATE_REGISTERED)
+			led_trigger_set_default(&udev->led_cdev);
+		mutex_unlock(&udev->mutex);
+		break;
+
+	default:
+		retval = -ENOIOCTLCMD;
+		break;
+	}
+
+	return retval;
+}
+
 static int uleds_release(struct inode *inode, struct file *file)
 {
 	struct uleds_device *udev = file->private_data;
@@ -200,6 +278,7 @@ static const struct file_operations uleds_fops = {
 	.read		= uleds_read,
 	.write		= uleds_write,
 	.poll		= uleds_poll,
+	.unlocked_ioctl	= uleds_ioctl,
 };
 
 static struct miscdevice uleds_misc = {
diff --git a/include/uapi/linux/uleds.h b/include/uapi/linux/uleds.h
index 4d32a39965f8..0e9861a8c31f 100644
--- a/include/uapi/linux/uleds.h
+++ b/include/uapi/linux/uleds.h
@@ -15,11 +15,39 @@
 #ifndef _UAPI__ULEDS_H_
 #define _UAPI__ULEDS_H_
 
-#define LED_MAX_NAME_SIZE	64
+#define LED_MAX_NAME_SIZE		64
+#define ULEDS_TRIGGER_MAX_NAME_SIZE	64
 
 struct uleds_user_dev {
 	char name[LED_MAX_NAME_SIZE];
 	int max_brightness;
 };
 
+struct uleds_user_trigger {
+	char name[ULEDS_TRIGGER_MAX_NAME_SIZE];
+};
+
+
+/* ioctl commands */
+
+#define ULEDS_IOC_MAGIC		'l'
+
+/*
+ * Initial setup.
+ * E.g.:
+ *	int retval;
+ *	struct uleds_user_dev dev_setup = { "mainboard:green:battery", 255 };
+ *	retval = ioctl(fd, ULEDS_IOC_DEV_SETUP, &dev_setup);
+ */
+#define ULEDS_IOC_DEV_SETUP		_IOW(ULEDS_IOC_MAGIC, 0x01, struct uleds_user_dev)
+
+/*
+ * Set the default trigger.
+ * E.g.:
+ *	int retval;
+ *	struct uleds_user_trigger default_trigger = { "battery" };
+ *	retval = ioctl(fd, ULEDS_IOC_SET_DEFAULT_TRIGGER, &default_trigger);
+ */
+#define ULEDS_IOC_SET_DEFAULT_TRIGGER	_IOW(ULEDS_IOC_MAGIC, 0x02, struct uleds_user_trigger)
+
 #endif /* _UAPI__ULEDS_H_ */
-- 
2.48.1

Re: [PATCH v3 2/2] leds: Add some setup ioctls to uleds driver

[Lee Jones]

On Mon, 17 Mar 2025, Craig McQueen wrote:

> * Add an ioctl for setup as an alternative to doing a write.
>   This is similar to the ioctl that was added to uinput for setup.
> * Add an ioctl to set a default trigger for the LED.
> 
> Signed-off-by: Craig McQueen <craig@mcqueen.au>
> ---
>  Documentation/leds/uleds.rst |   6 ++
>  drivers/leds/uleds.c         | 125 ++++++++++++++++++++++++++++-------
>  include/uapi/linux/uleds.h   |  30 ++++++++-
>  3 files changed, 137 insertions(+), 24 deletions(-)
> 
> diff --git a/Documentation/leds/uleds.rst b/Documentation/leds/uleds.rst
> index 4077745dae00..0512e577d63a 100644
> --- a/Documentation/leds/uleds.rst
> +++ b/Documentation/leds/uleds.rst
> @@ -24,6 +24,12 @@ 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".
>  
> +Alternatively, setup can be done with an ioctl call, passing a pointer to
> +the structure.
> +
> +There is also an ioctl call to configure a default trigger for the LED, by
> +passing a pointer to a structure containing the name of a trigger.
> +
>  The current brightness is found by reading an int value from the character
>  device. Reading will block until the brightness changes. The device node can
>  also be polled to notify when the brightness value changes.
> diff --git a/drivers/leds/uleds.c b/drivers/leds/uleds.c
> index 374a841f18c3..598e376a4b1b 100644
> --- a/drivers/leds/uleds.c
> +++ b/drivers/leds/uleds.c
> @@ -6,6 +6,7 @@
>   *
>   * Based on uinput.c: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
>   */
> +#include <linux/ctype.h>
>  #include <linux/fs.h>
>  #include <linux/init.h>
>  #include <linux/leds.h>
> @@ -25,13 +26,14 @@ enum uleds_state {
>  };
>  
>  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;
> -	int			brightness;
> -	bool			new_data;
> +	struct uleds_user_dev		user_dev;
> +	struct uleds_user_trigger	default_trigger;
> +	struct led_classdev		led_cdev;
> +	struct mutex			mutex;
> +	enum uleds_state		state;
> +	wait_queue_head_t		waitq;
> +	int				brightness;
> +	bool				new_data;
>  };
>  
>  static struct miscdevice uleds_misc;
> @@ -70,15 +72,17 @@ static int uleds_open(struct inode *inode, struct file *file)
>  	return 0;
>  }
>  
> -static ssize_t uleds_write(struct file *file, const char __user *buffer,
> -			   size_t count, loff_t *ppos)
> +static bool is_led_name_valid(const char *name)
>  {
> -	struct uleds_device *udev = file->private_data;
> -	const char *name;
> -	int ret;
> +	return ((name[0] != '\0') &&
> +		(strcmp(name, ".") != 0) &&
> +		(strcmp(name, "..") != 0) &&
> +		(strchr(name, '/') == NULL));
> +}
>  
> -	if (count == 0)
> -		return 0;
> +static int dev_setup(struct uleds_device *udev, const char __user *buffer)

uleds_dev_setup()

> +{
> +	int ret;
>  
>  	ret = mutex_lock_interruptible(&udev->mutex);
>  	if (ret)
> @@ -89,20 +93,13 @@ static ssize_t uleds_write(struct file *file, const char __user *buffer,
>  		goto out;
>  	}
>  
> -	if (count != sizeof(struct uleds_user_dev)) {
> -		ret = -EINVAL;
> -		goto out;
> -	}

Why is this no longer required?

> -
>  	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, '/')) {
> +	if (!is_led_name_valid(udev->user_dev.name)) {

Maybe we should make this official and put it in a header somewhere?

>  		ret = -EINVAL;
>  		goto out;
>  	}
> @@ -120,7 +117,6 @@ static ssize_t uleds_write(struct file *file, const char __user *buffer,
>  
>  	udev->new_data = true;
>  	udev->state = ULEDS_STATE_REGISTERED;
> -	ret = count;
>  
>  out:
>  	mutex_unlock(&udev->mutex);
> @@ -128,6 +124,23 @@ static ssize_t uleds_write(struct file *file, const char __user *buffer,
>  	return ret;
>  }
>  
> +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;
> +	if (count != sizeof(struct uleds_user_dev))
> +		return -EINVAL;
> +
> +	ret = dev_setup(udev, buffer);
> +	if (ret < 0)
> +		return ret;

Nit: '\n'

> +	return count;
> +}
> +
>  static ssize_t uleds_read(struct file *file, char __user *buffer, size_t count,
>  			  loff_t *ppos)
>  {
> @@ -179,6 +192,71 @@ static __poll_t uleds_poll(struct file *file, poll_table *wait)
>  	return 0;
>  }
>  
> +/*
> + * Trigger name validation: Allow only alphanumeric, hyphen or underscore.
> + */

This is clearly a single line comment.

The first part is also invalidated by the function's nomenclature.

> +static bool is_trigger_name_valid(const char *name)
> +{
> +	size_t i;
> +
> +	if (name[0] == '\0')
> +		return false;
> +
> +	for (i = 0; i < TRIG_NAME_MAX; i++) {

Shouldn't this be <=?

> +		if (name[i] == '\0')
> +			break;
> +		if (!isalnum(name[i]) && name[i] != '-' && name[i] != '_')
> +			return false;
> +	}

Nit: '\n'

> +	/* Length check and avoid any special names. */
> +	return ((i < TRIG_NAME_MAX) &&

What if the name is exactly the max?

> +		(strcmp(name, "default") != 0));

This can go on the line above.

How about?

   return !((i < TRIG_NAME_MAX) && (strcmp(name, "default"))

> +}
> +
> +static long uleds_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	struct uleds_device *udev = file->private_data;
> +	struct uleds_user_trigger default_trigger;
> +	int retval = 0;
> +
> +	switch (cmd) {
> +	case ULEDS_IOC_DEV_SETUP:
> +		retval = dev_setup(udev, (const char __user *)arg);
> +		break;
> +
> +	case ULEDS_IOC_SET_DEFAULT_TRIGGER:
> +		retval = copy_from_user(&default_trigger,
> +			(struct uleds_user_trigger __user *)arg,
> +			sizeof(default_trigger));

Subsequent lines need tabbing out to the open parenthesis.

> +		if (retval)
> +			return retval;

Please open things out.

Squished code is hard to read code.

> +		retval = mutex_lock_interruptible(&udev->mutex);
> +		if (retval)
> +			return retval;
> +		if (default_trigger.name[0] == '\0') {
> +			udev->led_cdev.default_trigger = NULL;
> +		} else {
> +			if (!is_trigger_name_valid(default_trigger.name)) {
> +				mutex_unlock(&udev->mutex);
> +				return -EINVAL;
> +			}
> +			memcpy(&udev->default_trigger, &default_trigger,
> +				sizeof(udev->default_trigger));
> +			udev->led_cdev.default_trigger = udev->default_trigger.name;
> +		}
> +		if (udev->state == ULEDS_STATE_REGISTERED)
> +			led_trigger_set_default(&udev->led_cdev);
> +		mutex_unlock(&udev->mutex);
> +		break;
> +
> +	default:
> +		retval = -ENOIOCTLCMD;
> +		break;
> +	}
> +
> +	return retval;
> +}
> +
>  static int uleds_release(struct inode *inode, struct file *file)
>  {
>  	struct uleds_device *udev = file->private_data;
> @@ -200,6 +278,7 @@ static const struct file_operations uleds_fops = {
>  	.read		= uleds_read,
>  	.write		= uleds_write,
>  	.poll		= uleds_poll,
> +	.unlocked_ioctl	= uleds_ioctl,
>  };
>  
>  static struct miscdevice uleds_misc = {
> diff --git a/include/uapi/linux/uleds.h b/include/uapi/linux/uleds.h
> index 4d32a39965f8..0e9861a8c31f 100644
> --- a/include/uapi/linux/uleds.h
> +++ b/include/uapi/linux/uleds.h
> @@ -15,11 +15,39 @@
>  #ifndef _UAPI__ULEDS_H_
>  #define _UAPI__ULEDS_H_
>  
> -#define LED_MAX_NAME_SIZE	64
> +#define LED_MAX_NAME_SIZE		64
> +#define ULEDS_TRIGGER_MAX_NAME_SIZE	64
>  
>  struct uleds_user_dev {
>  	char name[LED_MAX_NAME_SIZE];
>  	int max_brightness;
>  };
>  
> +struct uleds_user_trigger {
> +	char name[ULEDS_TRIGGER_MAX_NAME_SIZE];
> +};
> +
> +

Remove this double line spacing.

> +/* ioctl commands */
> +
> +#define ULEDS_IOC_MAGIC		'l'
> +
> +/*
> + * Initial setup.
> + * E.g.:
> + *	int retval;
> + *	struct uleds_user_dev dev_setup = { "mainboard:green:battery", 255 };
> + *	retval = ioctl(fd, ULEDS_IOC_DEV_SETUP, &dev_setup);
> + */
> +#define ULEDS_IOC_DEV_SETUP		_IOW(ULEDS_IOC_MAGIC, 0x01, struct uleds_user_dev)
> +
> +/*
> + * Set the default trigger.
> + * E.g.:
> + *	int retval;
> + *	struct uleds_user_trigger default_trigger = { "battery" };
> + *	retval = ioctl(fd, ULEDS_IOC_SET_DEFAULT_TRIGGER, &default_trigger);
> + */
> +#define ULEDS_IOC_SET_DEFAULT_TRIGGER	_IOW(ULEDS_IOC_MAGIC, 0x02, struct uleds_user_trigger)
> +
>  #endif /* _UAPI__ULEDS_H_ */
> -- 
> 2.48.1
> 
> 

-- 
Lee Jones [李琼斯]