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.
