[PATCH v1] leds: trigger: pattern: notify userpace if pattern finished

[PATCH v1] leds: trigger: pattern: notify userpace if pattern finished

[Martin Kurbanov]

In the current moment, userspace caller can schedule led pattern with
appropriate parameters, but it doesn't have ability to listen 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@sberdevices.ru>
---
 drivers/leds/trigger/ledtrig-pattern.c | 64 +++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c
index 43a265dc4696..54c4b957052f 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 running;
 	struct timer_list timer;
+	struct kernfs_node *pattern_ended;
 };

 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->running = false;
+
+			if (data->pattern_ended)
+				sysfs_notify_dirent(data->pattern_ended);
+
 			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->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->running = false;

 	if (data->is_hw_pattern)
 		led_cdev->pattern_clear(led_cdev);
@@ -268,6 +278,7 @@ static ssize_t pattern_trig_store_patterns(struct led_classdev *led_cdev,
 	mutex_lock(&data->lock);

 	del_timer_sync(&data->timer);
+	data->running = false;

 	if (data->is_hw_pattern)
 		led_cdev->pattern_clear(led_cdev);
@@ -330,6 +341,17 @@ static ssize_t hw_pattern_store(struct device *dev,

 static DEVICE_ATTR_RW(hw_pattern);

+static ssize_t pattern_ended_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 scnprintf(buf, PAGE_SIZE, "%d\n", !data->running);
+}
+
+static DEVICE_ATTR_RO(pattern_ended);
+
 static umode_t pattern_trig_attrs_mode(struct kobject *kobj,
 				       struct attribute *attr, int index)
 {
@@ -385,9 +407,41 @@ static void pattern_init(struct led_classdev *led_cdev)
 	kfree(pattern);
 }

+static int pattern_trig_add_pattern_ended(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_pattern_ended);
+	if (ret) {
+		dev_err(dev,
+			"Error creating pattern_ended (%pe)\n", ERR_PTR(ret));
+		return ret;
+	}
+
+	data->pattern_ended = sysfs_get_dirent(dev->kobj.sd, "pattern_ended");
+	if (!data->pattern_ended) {
+		dev_err(dev, "Error getting pattern_ended kernelfs\n");
+		device_remove_file(dev, &dev_attr_pattern_ended);
+		return -ENXIO;
+	}
+
+	return 0;
+}
+
+static void pattern_trig_remove_pattern_ended(struct led_classdev *led_cdev)
+{
+	struct pattern_trig_data *data = led_get_trigger_data(led_cdev);
+
+	sysfs_put(data->pattern_ended);
+	device_remove_file(led_cdev->dev, &dev_attr_pattern_ended);
+}
+
 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)
@@ -406,6 +460,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_pattern_ended(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) {
@@ -431,6 +492,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_pattern_ended(led_cdev);

 	led_set_brightness(led_cdev, LED_OFF);
 	kfree(data);
--
2.37.2

Re: [PATCH v1] leds: trigger: pattern: notify userpace if pattern finished

[Andy Shevchenko]

On Wed, Aug 24, 2022 at 3:06 PM Martin Kurbanov
<mmkurbanov@sberdevices.ru> wrote:
>
> In the current moment, userspace caller can schedule led pattern with

LED

> appropriate parameters, but it doesn't have ability to listen any events

listen to

> indicated pattern finished. This patch implements such an event using
> sysfs node and sysfs_notify_dirent() call.

Where is the ABI documentation for that?

Also, any example of user space code (GitHub repository / gist) how to
use the feature?

...

>         bool is_indefinite;
>         bool is_hw_pattern;
> +       bool running;

is_running ?

...

> +static ssize_t pattern_ended_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 scnprintf(buf, PAGE_SIZE, "%d\n", !data->running);

sysfs_emit().

> +}

> +

No need for this blank line.

> +static DEVICE_ATTR_RO(pattern_ended);

WRT previous two comments, if the current code is stylic in the above
way, you can convert it first and then add this patch in a series.

...

> +static int pattern_trig_add_pattern_ended(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_pattern_ended);
> +       if (ret) {
> +               dev_err(dev,
> +                       "Error creating pattern_ended (%pe)\n", ERR_PTR(ret));
> +               return ret;
> +       }
> +
> +       data->pattern_ended = sysfs_get_dirent(dev->kobj.sd, "pattern_ended");
> +       if (!data->pattern_ended) {
> +               dev_err(dev, "Error getting pattern_ended kernelfs\n");
> +               device_remove_file(dev, &dev_attr_pattern_ended);
> +               return -ENXIO;
> +       }
> +
> +       return 0;
> +}
> +
> +static void pattern_trig_remove_pattern_ended(struct led_classdev *led_cdev)
> +{
> +       struct pattern_trig_data *data = led_get_trigger_data(led_cdev);
> +
> +       sysfs_put(data->pattern_ended);
> +       device_remove_file(led_cdev->dev, &dev_attr_pattern_ended);
> +}

I'm wondering if you can always have a file and instead provide a
value there, so user space may use epoll() mechanism on that. It will
simplify your code here.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] leds: trigger: pattern: notify userpace if pattern finished

[Martin Kurbanov]

Hi. Thank you for quick reply.

On 2022-08-24 18:51, Andy Shevchenko wrote:
...
>> +static void pattern_trig_remove_pattern_ended(struct led_classdev *led_cdev)
>> +{
>> +       struct pattern_trig_data *data = led_get_trigger_data(led_cdev);
>> +
>> +       sysfs_put(data->pattern_ended);
>> +       device_remove_file(led_cdev->dev, &dev_attr_pattern_ended);
>> +}
> 
> I'm wondering if you can always have a file and instead provide a
> value there, so user space may use epoll() mechanism on that. It will
> simplify your code here.
> 
Do you mean to add 'dev_attr_pattern_ended' to 'pattern_trig_attrs',
and use 'sysfs_notify' instead of 'sysfs_notify_dirent' call'?

-- 
Best Regards,
Kurbanov Martin

Re: [PATCH v1] leds: trigger: pattern: notify userpace if pattern finished

[Dmitry Rokosov]

[...]

> > +static int pattern_trig_add_pattern_ended(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_pattern_ended);
> > +       if (ret) {
> > +               dev_err(dev,
> > +                       "Error creating pattern_ended (%pe)\n", ERR_PTR(ret));
> > +               return ret;
> > +       }
> > +
> > +       data->pattern_ended = sysfs_get_dirent(dev->kobj.sd, "pattern_ended");
> > +       if (!data->pattern_ended) {
> > +               dev_err(dev, "Error getting pattern_ended kernelfs\n");
> > +               device_remove_file(dev, &dev_attr_pattern_ended);
> > +               return -ENXIO;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static void pattern_trig_remove_pattern_ended(struct led_classdev *led_cdev)
> > +{
> > +       struct pattern_trig_data *data = led_get_trigger_data(led_cdev);
> > +
> > +       sysfs_put(data->pattern_ended);
> > +       device_remove_file(led_cdev->dev, &dev_attr_pattern_ended);
> > +}
> 
> I'm wondering if you can always have a file and instead provide a
> value there, so user space may use epoll() mechanism on that. It will
> simplify your code here.

Could you please explain what you mean? In the current implementation
userspace can use epoll() already.

-- 
Thank you,
Dmitry

Re: [PATCH v1] leds: trigger: pattern: notify userpace if pattern finished

[Andy Shevchenko]

On Fri, Sep 2, 2022 at 1:43 AM Dmitry Rokosov <ddrokosov@sberdevices.ru> wrote:

...

> > > +static void pattern_trig_remove_pattern_ended(struct led_classdev *led_cdev)
> > > +{
> > > +       struct pattern_trig_data *data = led_get_trigger_data(led_cdev);
> > > +
> > > +       sysfs_put(data->pattern_ended);
> > > +       device_remove_file(led_cdev->dev, &dev_attr_pattern_ended);
> > > +}
> >
> > I'm wondering if you can always have a file and instead provide a
> > value there, so user space may use epoll() mechanism on that. It will
> > simplify your code here.
>
> Could you please explain what you mean? In the current implementation
> userspace can use epoll() already.

On the suddenly disappeared file?

-- 
With Best Regards,
Andy Shevchenko