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@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxx>
>   */
> +#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 [李琼斯]