Re: [PATCH RFC] leds: core: Report ENODATA for brightness of hardware controlled LED

On Sun, 2026-07-12 at 01:14 +0800, Rong Zhang wrote:
> Hi Thomas, Lee,
> 
> Hopefully I am not too late to appear in the thread. I only noticed it
> after it reached v7.2-rc1.
> 
> > While the LED is controlled fully by the hardware, the value cached by
> > the LED driver core is incorrect. Return ENODATA to userspace in this
> > case.
> > 
> > Signed-off-by: Thomas Weißschuh <[email protected]>
> > ---
> >  drivers/leds/led-class.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> > index 9e14ae588f78..a88e274228e5 100644
> > --- a/drivers/leds/led-class.c
> > +++ b/drivers/leds/led-class.c
> > @@ -27,12 +27,25 @@ static LIST_HEAD(leds_lookup_list);
> >  
> >  static struct workqueue_struct *leds_wq;
> >  
> > +static bool led_trigger_is_hw_controlled(struct led_classdev *led_cdev)
> > +{
> > +#ifdef CONFIG_LEDS_TRIGGERS
> > +	guard(rwsem_read)(&led_cdev->trigger_lock);
> > +	return led_cdev->trigger && led_cdev->trigger->trigger_type;
> 
> This is a sufficient but not necessary condition. A generic trigger
> coordinating with the driver using hw_control_* interfaces returns false
> here.
> 
> That's also one of the reasons why I would like to introduce a new
> interface for the LED core to query the hardware control (offloaded)
> state, see
> https://lore.kernel.org/all/[email protected]/
> 
> Thomas, could you kindly review the series and see if the series makes
> sense to cros_ec?
> 
> > +#else
> > +	return false;
> > +#endif
> > +}
> > +
> >  static ssize_t brightness_show(struct device *dev,
> >  		struct device_attribute *attr, char *buf)
> >  {
> >  	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> >  	unsigned int brightness;
> >  
> > +	if (led_trigger_is_hw_controlled(led_cdev))
> > +		return -ENODATA;
> > +
> 
> How about brightness_store()?
> 
> It's a no-op to set a brightness when the LED is controlled fully by the
> hardware, and returning -ENODATA on read effectively makes the attribute
> write-only. A write-only no-op attribute is very confusing.
> 
> We already call led_trigger_remove() when 0 (LED_OFF) is written to the
> brightness attribute. Should we also break the write-only no-op dilemma
> by doing the same in brightness_store() when
> led_trigger_is_hw_controlled() is true?

Sorry, I should have dropped the two paragraphs above from my draft
before sending the reply, but they were accidentally left behind. Please
ignore them.

Writing brightness is not a no-op as the following paragraphs will say.

Thanks,
Rong

> 
> While some software triggers use the brightness attribute to tune
> itself, hardware control triggers don't seem to do so.
> 
> I also glanced some net or phy drivers that provides hardware control.
> IIUC, calling brightness_set[_blocking]() while hardware control is
> active causes the drivers to force software control and set the desired
> brightness, leaving the hardware control trigger ("netdev") still active
> (!)
> 
> So we are already living in a world that writing to the brightness
> attribute effectively disables hardware control and makes the trigger
> out-of-sync.
> 
> Also note that Documentation/leds/leds-class.rst says:
> 
>    When the LED is in hw control, no software blink is possible and doing
>    so will effectively disable hw control.
> 
> IMO disabling hardware control in brightness_store() also matches such
> semantics, though I guess these words originally referred to software
> triggers.
> 
> I will submit a patch to deactivate private trigger upon
> brightness_store() soon. For public hardware control trigger (i.e.,
> "netdev"), it still needs my series to make it mutually exclusive with
> the brightness attribute while in hardware control mode.
> 
> Thanks,
> Rong
> 
> >  	mutex_lock(&led_cdev->led_access);
> >  	led_update_brightness(led_cdev);
> >  	brightness = led_cdev->brightness;
> > 
> > ---
> > base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
> > change-id: 20260404-cros_ec-leds-hw-trigger-brightness-124ff8ce5dd6
> > 
> > Best regards,
> > --  
> > Thomas Weißschuh <[email protected]>