Re: [PATCH v2] leds: led-triggers: Improvements for default trigger

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

On 3/13/25 00:56, Craig McQueen wrote:
> On Thu, 13 Mar 2025 06:40:35 +1100 Jacek Anaszewski  wrote:
>   > Hi Craig,
>   >
>   > Thanks for the update.
>   >
>   > On 3/12/25 02:11, Craig McQueen wrote:
>   > > Accept "default" written to sysfs trigger attr.
>   > > If the text "default" is written to the LED's sysfs 'trigger' attr, then
>   > > call led_trigger_set_default() to set the LED to its default trigger.
>   > >
>   > > If the default trigger is set to "none", then led_trigger_set_default()
>   > > will remove a trigger. This is in contrast to the default trigger being
>   > > unset, in which case led_trigger_set_default() does nothing.
>   > > ---
>   > >   Documentation/ABI/testing/sysfs-class-led |  6 ++++++
>   > >   drivers/leds/led-triggers.c               | 26 ++++++++++++++++++++++-
>   > >   2 files changed, 31 insertions(+), 1 deletion(-)
>   > >
>   > > diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
>   > > index 2e24ac3bd7ef..0313b82644f2 100644
>   > > --- a/Documentation/ABI/testing/sysfs-class-led
>   > > +++ b/Documentation/ABI/testing/sysfs-class-led
>   > > @@ -72,6 +72,12 @@ Description:
>   > >           /sys/class/leds/ once a given trigger is selected. For
>   > >           their documentation see `sysfs-class-led-trigger-*`.
>   > >
>   > > +        Writing "none" removes the trigger for this LED.
>   > > +
>   > > +        Writing "default" sets the trigger to the LED's default trigger
>   > > +        (which would often be configured in the device tree for the
>   > > +        hardware).
>   > > +
>   > >   What:        /sys/class/leds//inverted
>   > >   Date:        January 2011
>   > >   KernelVersion:    2.6.38
>   > > diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
>   > > index b2d40f87a5ff..00c898af9969 100644
>   > > --- a/drivers/leds/led-triggers.c
>   > > +++ b/drivers/leds/led-triggers.c
>   > > @@ -49,11 +49,21 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
>   > >           goto unlock;
>   > >       }
>   > >
>   > > +    /* Empty string. Do nothing. */
>   > > +    if (sysfs_streq(buf, "")) {
>   > > +        goto unlock;
>   > > +    }
>   > > +
>   >
>   > Do we need this? It seems to be the same case as any other arbitrary
>   > string, for which we obviously don't have special handling.
> 
> I'll defer to you on this. I can remove it.

I don't see much gain in it, let's skip it.

>   > >       if (sysfs_streq(buf, "none")) {
>   > >           led_trigger_remove(led_cdev);
>   > >           goto unlock;
>   > >       }
>   > >
>   > > +    if (sysfs_streq(buf, "default")) {
>   > > +        led_trigger_set_default(led_cdev);
>   > > +        goto unlock;
>   > > +    }
>   > > +
>   > >       down_read(&triggers_list_lock);
>   > >       list_for_each_entry(trig, &trigger_list, next_trig) {
>   > >           if (sysfs_streq(buf, trig->name) && trigger_relevant(led_cdev, trig)) {
>   > > @@ -98,6 +108,9 @@ static int led_trigger_format(char *buf, size_t size,
>   > >       int len = led_trigger_snprintf(buf, size, "%s",
>   > >                          led_cdev->trigger ? "none" : "[none]");
>   > >
>   > > +    if (led_cdev->default_trigger && led_cdev->default_trigger[0])
>   > > +        len += led_trigger_snprintf(buf + len, size - len, " default");
>   > > +
>   > >       list_for_each_entry(trig, &trigger_list, next_trig) {
>   > >           bool hit;
>   > >
>   > > @@ -278,9 +291,20 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
>   > >       struct led_trigger *trig;
>   > >       bool found = false;
>   > >
>   > > -    if (!led_cdev->default_trigger)
>   > > +    /* NULL pointer or empty string. Do nothing. */
>   > > +    if (!led_cdev->default_trigger || !led_cdev->default_trigger[0])
>   > >           return;
>   > >
>   > > +    /* This case isn't sensible. Do nothing. */
>   > > +    if (!strcmp(led_cdev->default_trigger, "default"))
>   > > +        return;
>   >
>   > This is rather validation of the default trigger name obtained from DT,
>   > which, if at all, should be done after parsing DT property in
>   > led_classdev_register_ext(). I'd skip it anyway.
> 
> In a separate patch I've submitted for the uleds driver, I want to add the
> ability for users to set a default trigger for a userspace LED. Maybe I should
> add extra validation of the trigger name in that uleds driver patch.

I'd do that even in a separate patch.

>   > > +    /* Remove trigger. */
>   > > +    if (!strcmp(led_cdev->default_trigger, "none")) {
>   > > +        led_trigger_remove(led_cdev);
>   >
>   > This will be handled be led_trigger_set() called from
>   > led_match_default_trigger() below.
> 
> I added this because users should have a way to get led_trigger_set_default()
> to reset a LED's trigger to none, i.e. to have a default trigger of "none".
> 
> If led_cdev->default_trigger is NULL then the function exits early, so that
> doesn't achieve it.
> 
> Without the proposed change, if led_cdev->default_trigger is "none", then
> it will look for a trigger literally named "none", and won't find it, so it will
> follow the !found code path, which isn't helpful.
> 
> So, it seems beneficial to add explicit handling of "none" to remove any
> existing trigger.

Makes sense.

>   > > +        return;
>   > > +    }
>   > > +
>   > >       down_read(&triggers_list_lock);
>   > >       down_write(&led_cdev->trigger_lock);
>   > >       list_for_each_entry(trig, &trigger_list, next_trig) {
> 

-- 
Best regards,
Jacek Anaszewski