Re: [PATCH v2 2/2] leds: trigger: pattern: notify usespace if pattern finished

[Martin Kurbanov]

On 21.11.2022 15:38, Martin Kurbanov wrote:
> In the current moment, userspace caller can schedule LED pattern with
> appropriate parameters, but it doesn't have ability to listen to any
> events indicated pattern finished. This patch implements such an event
> using sysfs node and sysfs_notify_dirent() call.
> 
> Signed-off-by: Martin Kurbanov <mmkurbanov@xxxxxxxxxxxxxx>
> ---
>  .../testing/sysfs-class-led-trigger-pattern   | 11 ++++
>  drivers/leds/trigger/ledtrig-pattern.c        | 63 ++++++++++++++++++-
>  2 files changed, 73 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> index 8c57d2780554..b2564123b54b 100644
> --- a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> @@ -38,3 +38,14 @@ Description:
>  
>  		It should be noticed that some leds, like EL15203000 may
>  		only support indefinitely patterns, so they always store -1.
> +
> +What:		/sys/class/leds/<led>/is_running
> +Date:		October 2022
> +KernelVersion:	6.1
> +Description:
> +		Indicates running of a software pattern for the LED.
> +
> +		This file is read only and supports poll() to detect when the
> +		software pattern ended.
> +
> +		1 means pattern is running and number 0 are finish or not run.
> diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c
> index 354304b404aa..19a6b5dcd7af 100644
> --- a/drivers/leds/trigger/ledtrig-pattern.c
> +++ b/drivers/leds/trigger/ledtrig-pattern.c
> @@ -33,7 +33,9 @@ struct pattern_trig_data {
>  	int delta_t;
>  	bool is_indefinite;
>  	bool is_hw_pattern;
> +	bool is_running;
>  	struct timer_list timer;
> +	struct kernfs_node *is_running_kn;
>  };
>  
>  static void pattern_trig_update_patterns(struct pattern_trig_data *data)
> @@ -76,8 +78,14 @@ static void pattern_trig_timer_function(struct timer_list *t)
>  	struct pattern_trig_data *data = from_timer(data, t, timer);
>  
>  	for (;;) {
> -		if (!data->is_indefinite && !data->repeat)
> +		if (!data->is_indefinite && !data->repeat) {
> +			data->is_running = false;
> +
> +			if (data->is_running_kn)
> +				sysfs_notify_dirent(data->is_running_kn);
> +
>  			break;
> +		}
>  
>  		if (data->curr->brightness == data->next->brightness) {
>  			/* Step change of brightness */
> @@ -137,6 +145,7 @@ static int pattern_trig_start_pattern(struct led_classdev *led_cdev)
>  	data->curr = data->patterns;
>  	data->next = data->patterns + 1;
>  	data->timer.expires = jiffies;
> +	data->is_running = true;
>  	add_timer(&data->timer);
>  
>  	return 0;
> @@ -176,6 +185,7 @@ static ssize_t repeat_store(struct device *dev, struct device_attribute *attr,
>  	mutex_lock(&data->lock);
>  
>  	del_timer_sync(&data->timer);
> +	data->is_running = false;
>  
>  	if (data->is_hw_pattern)
>  		led_cdev->pattern_clear(led_cdev);
> @@ -267,6 +277,7 @@ static ssize_t pattern_trig_store_patterns(struct led_classdev *led_cdev,
>  	mutex_lock(&data->lock);
>  
>  	del_timer_sync(&data->timer);
> +	data->is_running = false;
>  
>  	if (data->is_hw_pattern)
>  		led_cdev->pattern_clear(led_cdev);
> @@ -327,6 +338,16 @@ static ssize_t hw_pattern_store(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(hw_pattern);
>  
> +static ssize_t is_running_show(struct device *dev,
> +			       struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct pattern_trig_data *data = led_get_trigger_data(led_cdev);
> +
> +	return sysfs_emit(buf, "%d\n", data->is_running);
> +}
> +static DEVICE_ATTR_RO(is_running);
> +
>  static umode_t pattern_trig_attrs_mode(struct kobject *kobj,
>  				       struct attribute *attr, int index)
>  {
> @@ -382,9 +403,41 @@ static void pattern_init(struct led_classdev *led_cdev)
>  	kfree(pattern);
>  }
>  
> +static int pattern_trig_add_is_running(struct led_classdev *led_cdev)
> +{
> +	struct pattern_trig_data *data = led_get_trigger_data(led_cdev);
> +	struct device *dev = led_cdev->dev;
> +	int ret;
> +
> +	ret = device_create_file(dev, &dev_attr_is_running);
> +	if (ret) {
> +		dev_err(dev,
> +			"Error creating is_running (%pe)\n", ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	data->is_running_kn = sysfs_get_dirent(dev->kobj.sd, "is_running");
> +	if (!data->is_running_kn) {
> +		dev_err(dev, "Error getting is_running kernelfs\n");
> +		device_remove_file(dev, &dev_attr_is_running);
> +		return -ENXIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static void pattern_trig_remove_is_running(struct led_classdev *led_cdev)
> +{
> +	struct pattern_trig_data *data = led_get_trigger_data(led_cdev);
> +
> +	sysfs_put(data->is_running_kn);
> +	device_remove_file(led_cdev->dev, &dev_attr_is_running);
> +}
> +
>  static int pattern_trig_activate(struct led_classdev *led_cdev)
>  {
>  	struct pattern_trig_data *data;
> +	int err;
>  
>  	data = kzalloc(sizeof(*data), GFP_KERNEL);
>  	if (!data)
> @@ -403,6 +456,13 @@ static int pattern_trig_activate(struct led_classdev *led_cdev)
>  	data->led_cdev = led_cdev;
>  	led_set_trigger_data(led_cdev, data);
>  	timer_setup(&data->timer, pattern_trig_timer_function, 0);
> +
> +	err = pattern_trig_add_is_running(led_cdev);
> +	if (err)
> +		dev_warn(led_cdev->dev,
> +			 "pattern ended notifications disabled (%pe)\n",
> +			 ERR_PTR(err));
> +
>  	led_cdev->activated = true;
>  
>  	if (led_cdev->flags & LED_INIT_DEFAULT_TRIGGER) {
> @@ -428,6 +488,7 @@ static void pattern_trig_deactivate(struct led_classdev *led_cdev)
>  		led_cdev->pattern_clear(led_cdev);
>  
>  	del_timer_sync(&data->timer);
> +	pattern_trig_remove_is_running(led_cdev);
>  
>  	led_set_brightness(led_cdev, LED_OFF);
>  	kfree(data);

Hello Andy,

In the previous patch series feedback you mentioned two main problems:
sysfs node creation time and life cycle, and sysfs node creation method.
Let me explain why I didn't fix the above items.

1) About sysfs node creation time and its life cycle. In my opinion,
sysfs node should exist only when user has activated pattern explicitly;
otherwise, it will mislead potential user in the case when pattern is
not activated, but sysfs node existed.
2) About pattern_trig_attrs. We need to use sysfs_notify_dirent()
instead of sysfs_notify(), cause sysfs_notify() can sleep on the lock,
but it's not acceptable for the pattern code in the timer context.
Considering this, we have to create sysfs node in the
pattern_trig_activate() directly and retrieve kernfs_node for required
sysfs_notify_dirent() routine.