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

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?

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]>