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

[Armin Wolf <W_Armin@gmx.de>]

Am 19.10.26 um 13:51 schrieb 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@gmx.de>
>> ---
>>   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@rong.moe>
>
> Thanks,
> Rong

I would prefer moving led_groups[] in patch 4, because patch 1 deals with an entirely
different topic.

Sashiko has also found a major issue in this patch, so i will have to rework it
quite a bit for the v2 revision. I will therefore not include your Reviewed-by tag
just yet, because the changes are quite substantial.

Thanks,
Armin Wolf

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