Re: [PATCH 04/10] leds: class: Create "brightness_hw_changed" attribute atomically

[Rong Zhang]

Hi Armin,

Nice refactor!

On Sun, 2026-10-18 at 15:46 +0200, Armin Wolf wrote:
> Using device_create_file() might cause userspace applications
> listening for uevents to inspect a new LED device before the kernel
> has registered all sysfs attributes for it. This might cause those
> applications to incorrectly assume that the new LED device does not
> support the "brightness_hw_changed" attribute.
> 
> Fix this by letting the device core create the sysfs attribute before
> the uevent is being send to userspace.
> 
> Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
> ---
>  drivers/leds/led-class.c | 85 +++++++++++++++-------------------------
>  include/linux/leds.h     |  2 -
>  2 files changed, 31 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 3775ad13762f..d460f041ed86 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -17,6 +17,7 @@
>  #include <linux/property.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
> +#include <linux/sysfs.h>
>  #include <linux/timer.h>
>  #include <uapi/linux/uleds.h>
>  #include <linux/of.h>
> @@ -101,14 +102,6 @@ static const struct attribute_group led_group = {
>  	.attrs_const = led_class_attrs,
>  };
>  
> -static const struct attribute_group *const led_groups[] = {

This is a back-and-forth change. How about moving led_groups to the right
place in PATCH 1 beforehand?

Other than that, FWIW:

   Reviewed-by: Rong Zhang <i@xxxxxxxx>

Thanks,
Rong

> -	&led_group,
> -#ifdef CONFIG_LEDS_TRIGGERS
> -	&led_trigger_group,
> -#endif
> -	NULL,
> -};
> -
>  #ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
>  static ssize_t brightness_hw_changed_show(struct device *dev, const struct device_attribute *attr,
>  					  char *buf)
> @@ -123,51 +116,48 @@ static ssize_t brightness_hw_changed_show(struct device *dev, const struct devic
>  
>  static const DEVICE_ATTR_RO(brightness_hw_changed);
>  
> -static int led_add_brightness_hw_changed(struct led_classdev *led_cdev)
> -{
> -	struct device *dev = led_cdev->dev;
> -	int ret;
> +static const struct attribute *const led_brightness_hw_changed_attrs[] = {
> +	&dev_attr_brightness_hw_changed.attr,
> +	NULL,
> +};
>  
> -	ret = device_create_file(dev, &dev_attr_brightness_hw_changed);
> -	if (ret) {
> -		dev_err(dev, "Error creating brightness_hw_changed\n");
> -		return ret;
> -	}
> +static umode_t led_brightness_hw_changed_is_visible(struct kobject *kobj,
> +						    const struct attribute *attr, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
>  
> -	led_cdev->brightness_hw_changed_kn =
> -		sysfs_get_dirent(dev->kobj.sd, "brightness_hw_changed");
> -	if (!led_cdev->brightness_hw_changed_kn) {
> -		dev_err(dev, "Error getting brightness_hw_changed kn\n");
> -		device_remove_file(dev, &dev_attr_brightness_hw_changed);
> -		return -ENXIO;
> -	}
> +	if (attr == &dev_attr_brightness_hw_changed.attr &&
> +	    led_cdev->flags & LED_BRIGHT_HW_CHANGED)
> +		return attr->mode;
>  
>  	return 0;
>  }
>  
> -static void led_remove_brightness_hw_changed(struct led_classdev *led_cdev)
> -{
> -	sysfs_put(led_cdev->brightness_hw_changed_kn);
> -	device_remove_file(led_cdev->dev, &dev_attr_brightness_hw_changed);
> -}
> +static const struct attribute_group led_brightness_hw_changed_group = {
> +	.is_visible_const = led_brightness_hw_changed_is_visible,
> +	.attrs_const = led_brightness_hw_changed_attrs,
> +};
> +#endif	/* CONFIG_LEDS_BRIGHTNESS_HW_CHANGED */
>  
> +static const struct attribute_group *const led_groups[] = {
> +	&led_group,
> +#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
> +	&led_brightness_hw_changed_group,
> +#endif
> +#ifdef CONFIG_LEDS_TRIGGERS
> +	&led_trigger_group,
> +#endif
> +	NULL,
> +};
> +
> +#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
>  void led_classdev_notify_brightness_hw_changed(struct led_classdev *led_cdev, unsigned int brightness)
>  {
> -	if (WARN_ON(!led_cdev->brightness_hw_changed_kn))
> -		return;
> -
>  	led_cdev->brightness_hw_changed = brightness;
> -	sysfs_notify_dirent(led_cdev->brightness_hw_changed_kn);
> +	sysfs_notify(&led_cdev->dev->kobj, NULL, "brightness_hw_changed");
>  }
>  EXPORT_SYMBOL_GPL(led_classdev_notify_brightness_hw_changed);
> -#else
> -static int led_add_brightness_hw_changed(struct led_classdev *led_cdev)
> -{
> -	return 0;
> -}
> -static void led_remove_brightness_hw_changed(struct led_classdev *led_cdev)
> -{
> -}
>  #endif
>  
>  /**
> @@ -544,16 +534,6 @@ int led_classdev_register_ext(struct device *parent,
>  	if (init_data && init_data->fwnode)
>  		device_set_node(led_cdev->dev, init_data->fwnode);
>  
> -	if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
> -		ret = led_add_brightness_hw_changed(led_cdev);
> -		if (ret) {
> -			device_unregister(led_cdev->dev);
> -			led_cdev->dev = NULL;
> -			mutex_unlock(&led_cdev->led_access);
> -			return ret;
> -		}
> -	}
> -
>  	led_cdev->work_flags = 0;
>  #ifdef CONFIG_LEDS_TRIGGERS
>  	init_rwsem(&led_cdev->trigger_lock);
> @@ -626,9 +606,6 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
>  
>  	flush_work(&led_cdev->set_brightness_work);
>  
> -	if (led_cdev->flags & LED_BRIGHT_HW_CHANGED)
> -		led_remove_brightness_hw_changed(led_cdev);
> -
>  	device_unregister(led_cdev->dev);
>  
>  	down_write(&leds_list_lock);
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index beaf23993063..6f975078cc14 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -21,7 +21,6 @@ struct attribute_group;
>  struct device_node;
>  struct fwnode_handle;
>  struct gpio_desc;
> -struct kernfs_node;
>  struct led_pattern;
>  struct platform_device;
>  
> @@ -237,7 +236,6 @@ struct led_classdev {
>  
>  #ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
>  	int			 brightness_hw_changed;
> -	struct kernfs_node	*brightness_hw_changed_kn;
>  #endif
>  
>  #ifdef CONFIG_LEDS_TRIGGERS_HW_CHANGED