Re: [PATCH RFC 3/4] leds: add delay_on, delay_off and invert attributes to disk trigger

[Markus Probst <markus.probst@posteo.de>]

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

On Fri, 2026-01-23 at 19:05 +0000, Markus Probst wrote:
> Add delay_on, delay_off and invert device attributes to leds using the
> disk trigger.
> 
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> ---
>  drivers/leds/trigger/ledtrig-disk.c | 194 +++++++++++++++++++++++++++++++++---
>  1 file changed, 182 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-disk.c b/drivers/leds/trigger/ledtrig-disk.c
> index e9b87ee944f2..ed5ef83a5b35 100644
> --- a/drivers/leds/trigger/ledtrig-disk.c
> +++ b/drivers/leds/trigger/ledtrig-disk.c
> @@ -9,31 +9,201 @@
>  
>  #include <linux/kernel.h>
>  #include <linux/init.h>
> +#include <linux/list.h>
>  #include <linux/leds.h>
> +#include "../leds.h"
>  
> -#define BLINK_DELAY 30
> +#define DEFAULT_BLINK_DELAY 30
>  
> -DEFINE_LED_TRIGGER(ledtrig_disk);
> -DEFINE_LED_TRIGGER(ledtrig_disk_read);
> -DEFINE_LED_TRIGGER(ledtrig_disk_write);
> +struct ledtrig_disk_data {
> +	unsigned long delay_on;
> +	unsigned long delay_off;
> +	unsigned int invert;
> +};
> +
> +static ssize_t led_delay_on_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *led_cdev = led_trigger_get_led(dev);
> +	struct ledtrig_disk_data *disk_data = led_get_trigger_data(led_cdev);
> +
> +	return sprintf(buf, "%lu\n", disk_data->delay_on);
> +}
> +
> +static ssize_t led_delay_on_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	struct led_classdev *led_cdev = led_trigger_get_led(dev);
> +	struct ledtrig_disk_data *disk_data = led_get_trigger_data(led_cdev);
> +	unsigned long state;
> +	ssize_t ret;
> +
> +	ret = kstrtoul(buf, 10, &state);
> +	if (ret)
> +		return ret;
> +
> +	disk_data->delay_on = state;
> +
> +	return size;
> +}
> +
> +static ssize_t led_delay_off_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *led_cdev = led_trigger_get_led(dev);
> +	struct ledtrig_disk_data *disk_data = led_get_trigger_data(led_cdev);
> +
> +	return sprintf(buf, "%lu\n", disk_data->delay_off);
> +}
> +
> +static ssize_t led_delay_off_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	struct led_classdev *led_cdev = led_trigger_get_led(dev);
> +	struct ledtrig_disk_data *disk_data = led_get_trigger_data(led_cdev);
> +	unsigned long state;
> +	ssize_t ret;
> +
> +	ret = kstrtoul(buf, 10, &state);
> +	if (ret)
> +		return ret;
> +
> +	disk_data->delay_off = state;
> +
> +	return size;
> +}
> +
> +static ssize_t led_invert_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct ledtrig_disk_data *disk_data =
> +		led_trigger_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", disk_data->invert);
> +}
> +
> +static ssize_t led_invert_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	struct led_classdev *led_cdev = led_trigger_get_led(dev);
> +	struct ledtrig_disk_data *disk_data = led_get_trigger_data(led_cdev);
> +	unsigned long state;
> +	int ret;
> +
> +	ret = kstrtoul(buf, 0, &state);
> +	if (ret)
> +		return ret;
> +
> +	led_set_brightness_nosleep(led_cdev, state ? LED_FULL : LED_OFF);
> +	disk_data->invert = !!state;
> +
> +	return size;
> +}
> +
> +static DEVICE_ATTR(delay_on, 0644, led_delay_on_show, led_delay_on_store);
> +static DEVICE_ATTR(delay_off, 0644, led_delay_off_show, led_delay_off_store);
> +static DEVICE_ATTR(invert, 0644, led_invert_show, led_invert_store);
> +
> +static struct attribute *ledtrig_disk_attrs[] = {
> +	&dev_attr_delay_on.attr,
> +	&dev_attr_delay_off.attr,
> +	&dev_attr_invert.attr,
> +	NULL
> +};
> +ATTRIBUTE_GROUPS(ledtrig_disk);
> +
> +static void pattern_init(struct led_classdev *led_cdev, struct ledtrig_disk_data *disk_data)
> +{
> +	unsigned int size = 0;
> +
> +	u32 *pattern __free(kfree) = led_get_default_pattern(led_cdev, &size);
> +	if (!pattern)
> +		return;
> +
> +	if (size != 3) {
> +		dev_warn(led_cdev->dev,
> +			 "Expected 3 but got %u values for delays + invert pattern\n",
> +			 size);
> +		return;
> +	}
> +
> +	disk_data->delay_on = pattern[0];
> +	disk_data->delay_off = pattern[1];
> +	disk_data->invert = !!pattern[2];
> +}
> +
> +static int ledtrig_disk_activate(struct led_classdev *led_cdev)
> +{
> +	struct ledtrig_disk_data *disk_data;
> +
> +	disk_data = kzalloc(sizeof(*disk_data), GFP_KERNEL);
> +	if (!disk_data)
> +		return -ENOMEM;
> +
> +	disk_data->delay_on = DEFAULT_BLINK_DELAY;
> +	disk_data->delay_off = DEFAULT_BLINK_DELAY;
> +
> +	led_set_trigger_data(led_cdev, disk_data);
> +
> +	if (led_cdev->flags & LED_INIT_DEFAULT_TRIGGER) {
> +		pattern_init(led_cdev, disk_data);
> +		/*
> +		 * Mark as initialized even on pattern_init() error because
> +		 * any consecutive call to it would produce the same error.
> +		 */
> +		led_cdev->flags &= ~LED_INIT_DEFAULT_TRIGGER;
> +	}
> +
> +	led_set_brightness_nosleep(led_cdev, disk_data->invert ? LED_FULL : LED_OFF);
> +
> +	return 0;
> +}
> +
> +static struct led_trigger ledtrig_disk = {
> +	.name = "disk-activity",
> +	.activate = ledtrig_disk_activate,
> +	.groups = ledtrig_disk_groups,
> +};
> +static struct led_trigger ledtrig_disk_read = {
> +	.name = "disk-read",
> +	.activate = ledtrig_disk_activate,
> +	.groups = ledtrig_disk_groups,
> +};
> +static struct led_trigger ledtrig_disk_write = {
> +	.name = "disk-write",
> +	.activate = ledtrig_disk_activate,
> +	.groups = ledtrig_disk_groups,
> +};
> +
> +static void ledtrig_disk_blink_oneshot(struct led_trigger *trig)
> +{
> +	struct led_classdev *led_cdev;
> +	struct ledtrig_disk_data *disk_data;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(led_cdev, &trig->led_cdevs, trig_list) {
> +		disk_data = led_get_trigger_data(led_cdev);
Also there is likely a race condition here, as there is a time gap
between the led being added to trig->led_cdevs and
ledtrig_disk_activate being run.

Is there a better way to avoid the race condition, than defining an own
list for each trigger?

Thanks
- Markus Probst

> +		led_blink_set_oneshot(led_cdev, &disk_data->delay_on, &disk_data->delay_off,
> +				      disk_data->invert);
> +	}
> +	rcu_read_unlock();
> +}
>  
>  void ledtrig_disk_activity(bool write)
>  {
> -	led_trigger_blink_oneshot(ledtrig_disk, BLINK_DELAY, BLINK_DELAY, 0);
> +	ledtrig_disk_blink_oneshot(&ledtrig_disk);
>  	if (write)
> -		led_trigger_blink_oneshot(ledtrig_disk_write,
> -					  BLINK_DELAY, BLINK_DELAY, 0);
> +		ledtrig_disk_blink_oneshot(&ledtrig_disk_write);
>  	else
> -		led_trigger_blink_oneshot(ledtrig_disk_read,
> -					  BLINK_DELAY, BLINK_DELAY, 0);
> +		ledtrig_disk_blink_oneshot(&ledtrig_disk_read);
>  }
>  EXPORT_SYMBOL(ledtrig_disk_activity);
>  
>  static int __init ledtrig_disk_init(void)
>  {
> -	led_trigger_register_simple("disk-activity", &ledtrig_disk);
> -	led_trigger_register_simple("disk-read", &ledtrig_disk_read);
> -	led_trigger_register_simple("disk-write", &ledtrig_disk_write);
> +	led_trigger_register(&ledtrig_disk);
> +	led_trigger_register(&ledtrig_disk_read);
> +	led_trigger_register(&ledtrig_disk_write);
>  
>  	return 0;
>  }

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]