Re: [PATCH 15/18] ledtrig-blkdev: Add sysfs attributes to [un]link LEDs & devices

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Sep 03, 2021 at 03:45:45PM -0500, Ian Pilcher wrote:
> /sys/class/leds/<led>/add_blkdev - to create device/LED associations
> 
> /sys/class/leds/<led>/delete_blkdev to remove device/LED associations
> 
> For both attributes, accept multiple device names separated by whitespace
> 
> Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
> ---
>  drivers/leds/trigger/ledtrig-blkdev.c | 48 +++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
> index b2ec85b805d0..db82d37fc721 100644
> --- a/drivers/leds/trigger/ledtrig-blkdev.c
> +++ b/drivers/leds/trigger/ledtrig-blkdev.c
> @@ -509,3 +509,51 @@ static void blkdev_deactivate(struct led_classdev *const led_dev)
>  
>  	module_put(THIS_MODULE);
>  }
> +
> +
> +/*
> + *
> + *	sysfs attributes to add & delete devices from LEDs
> + *
> + */
> +
> +static ssize_t blkdev_add_or_del(struct device *const dev,
> +				 struct device_attribute *const attr,
> +				 const char *const buf, const size_t count);
> +
> +static struct device_attribute ledtrig_blkdev_attr_add =
> +	__ATTR(add_blkdev, 0200, NULL, blkdev_add_or_del);
> +
> +static struct device_attribute ledtrig_blkdev_attr_del =
> +	__ATTR(delete_blkdev, 0200, NULL, blkdev_add_or_del);

DEVICE_ATTR_RO()?  Or something like that?  Do not use __ATTR() for
device attributes if at all possible, worst case, use DEVICE_ATTR()
here.

And the mode settings are odd, are you sure you want that?

> +static ssize_t blkdev_add_or_del(struct device *const dev,
> +				 struct device_attribute *const attr,
> +				 const char *const buf, const size_t count)
> +{
> +	struct ledtrig_blkdev_led *const led = led_trigger_get_drvdata(dev);
> +	const char *const disk_name = blkdev_skip_space(buf);
> +	const char *const endp = blkdev_find_space(disk_name);
> +	const ptrdiff_t name_len = endp - disk_name;	/* always >= 0 */
> +	int ret;
> +
> +	if (name_len == 0) {
> +		pr_info("blkdev LED: empty block device name\n");

Looks like debugging code, please remove.

And how can this ever happen?

> +		return -EINVAL;
> +	}
> +
> +	if (attr == &ledtrig_blkdev_attr_del) {
> +		blkdev_disk_delete(led, disk_name, name_len);
> +	} else {	/* attr == &ledtrig_blkdev_attr_add */
> +		ret = blkdev_disk_add(led, disk_name, name_len);

Why do you have a single attribute callback that does two totally
different things?  Just have 2 different callback functions please, it
makes things much easier to review and maintain over time.

thanks,

greg k-h