Re: [RFC PATCH 1/9] leds: Load trigger modules on-demand if used as hw control trigger

[Rong Zhang]

Hi Lee,

On Tue, 2026-03-10 at 12:01 +0000, Lee Jones wrote:
> On Sat, 28 Feb 2026, Rong Zhang wrote:
> 
> > In the following patches, we are about to support hardware initiated
> 
> Let's not talk about other commits.  Only tell us what's happening here.
> 
> > trigger transitions to/from the device's hw control trigger. In case
> > the LED hardware switches itself to hw control mode, hw control trigger
> > must be loaded before so that the transition can be processed.
> > 
> > Load the trigger module specified by hw_control_trigger, so that
> > hardware initiated trigger transitions can be processed when the hw
> 
> "hardware"
> 
> > control trigger is compiled as a module.
> > 
> > Signed-off-by: Rong Zhang <i@xxxxxxxx>
> > ---
> >  drivers/leds/led-class.c    |  1 +
> >  drivers/leds/led-triggers.c | 33 +++++++++++++++++++++++++++++++++
> >  drivers/leds/leds.h         |  1 +
> >  3 files changed, 35 insertions(+)
> > 
> > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> > index d34a194535604..0fa45f22246e3 100644
> > --- a/drivers/leds/led-class.c
> > +++ b/drivers/leds/led-class.c
> > @@ -576,6 +576,7 @@ int led_classdev_register_ext(struct device *parent,
> >  
> >  #ifdef CONFIG_LEDS_TRIGGERS
> >  	led_trigger_set_default(led_cdev);
> > +	led_load_hw_control_trigger(led_cdev);
> 
> led_trigger_load_hw_control
> 
> >  #endif
> >  
> >  	mutex_unlock(&led_cdev->led_access);
> > diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> > index b1223218bda11..3066bc91a5f94 100644
> > --- a/drivers/leds/led-triggers.c
> > +++ b/drivers/leds/led-triggers.c
> > @@ -313,6 +313,39 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
> >  }
> >  EXPORT_SYMBOL_GPL(led_trigger_set_default);
> >  
> > +static inline bool led_match_hw_control_trigger(struct led_classdev *led_cdev,
> > +						struct led_trigger *trig)
> > +{
> > +	return (!strcmp(led_cdev->hw_control_trigger, trig->name) &&
> > +		trigger_relevant(led_cdev, trig));
> 
> 
> This is ugly.  Break it out and provide some commentary.
> 
> > +}
> > +
> > +void led_load_hw_control_trigger(struct led_classdev *led_cdev)
> > +{
> > +	struct led_trigger *trig;
> > +	bool found = false;
> > +
> > +	if (!led_cdev->hw_control_trigger)
> > +		return;
> > +
> > +	/* default_trigger is handled by led_trigger_set_default(). */
> 
> Sentences start with uppercase chars.
> 
> > +	if (led_cdev->default_trigger &&
> > +	    !strcmp(led_cdev->default_trigger, led_cdev->hw_control_trigger))
> > +		return;
> 
> Do you need to check default_trigger?
> 
> strcmp() should be able to handle empty strings.

It checks the NULLity of led_cdev->default_trigger instead of its
emptiness. While passing an empty string to strcmp is fine, passing a
NULL pointer to strcmp() leads to a NULL dereference.

Thanks for your review. ACK to other comments :)

Thanks,
Rong

> 
> > +
> > +	down_read(&triggers_list_lock);
> > +	list_for_each_entry(trig, &trigger_list, next_trig) {
> > +		found = led_match_hw_control_trigger(led_cdev, trig);
> > +		if (found)
> > +			break;
> > +	}
> > +	up_read(&triggers_list_lock);
> > +
> > +	if (!found)
> > +		request_module_nowait("ledtrig:%s", led_cdev->hw_control_trigger);
> > +}
> > +EXPORT_SYMBOL_GPL(led_load_hw_control_trigger);
> > +
> >  /* LED Trigger Interface */
> >  
> >  int led_trigger_register(struct led_trigger *trig)
> > diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> > index bee46651e068f..e85afd4d04fd0 100644
> > --- a/drivers/leds/leds.h
> > +++ b/drivers/leds/leds.h
> > @@ -21,6 +21,7 @@ void led_init_core(struct led_classdev *led_cdev);
> >  void led_stop_software_blink(struct led_classdev *led_cdev);
> >  void led_set_brightness_nopm(struct led_classdev *led_cdev, unsigned int value);
> >  void led_set_brightness_nosleep(struct led_classdev *led_cdev, unsigned int value);
> > +void led_load_hw_control_trigger(struct led_classdev *led_cdev);
> >  ssize_t led_trigger_read(struct file *filp, struct kobject *kobj,
> >  			const struct bin_attribute *attr, char *buf,
> >  			loff_t pos, size_t count);
> > -- 
> > 2.51.0
> >