[PATCH 00/10] leds: class: Support custom sysfs groups on multicolor and flash LEDs

[PATCH 00/10] leds: class: Support custom sysfs groups on multicolor and flash LEDs

[Armin Wolf]

Until now drivers are unable to atomically register custom sysfs
groups below multicolor and flash LED class devices, because the
led_cdev->sysfs_groups field is being overwritten inside the
associated initialization functions. This patch series tries to
fix this.

The first three patches constify the sysfs attributes of all three
LED flavors, while the next four patches fix various unrelated issues
affecting LED device initialization. The next two patches migrate
the multicolor and flash LEDs to use struct device_type for
registering their sysfs attributes, so that led_cdev->sysfs_groups is
not touched anymore. The last patch contains a minor optimization
for multicolor LEDs.

All patches have been tested on real hardware, with the flash LED
interface being tested using a custom kernel module.

Armin Wolf (10):
  leds: class: Constify sysfs attributes
  leds: multicolor: Constify sysfs attributes
  leds: flash: Constify sysfs attributes
  leds: class: Create "brightness_hw_changed" attribute atomically
  leds: class: Fix race condition when creating the device
  leds: class: Take the LED access lock when reading
    brightness_hw_changed
  leds: class: Use correct type for brightness_hw_changed field
  leds: class: Split led_classdev_register_ext()
  leds: Support custom sysfs groups on multicolor and flash LEDs
  leds: multicolor: Adjust maximum subled intensity during
    initialization

 drivers/leds/led-class-flash.c      | 154 +++++++++--------
 drivers/leds/led-class-multicolor.c |  79 ++++-----
 drivers/leds/led-class.c            | 245 +++++++++++++++++-----------
 drivers/leds/leds.h                 |   3 +
 include/linux/led-class-flash.h     |   5 -
 include/linux/leds.h                |   4 +-
 6 files changed, 268 insertions(+), 222 deletions(-)

-- 
2.56.0

[PATCH 01/10] leds: class: Constify sysfs attributes

[Armin Wolf]

Mark all sysfs attributes as const to ensure that they are placed
in readonly memory to prevent accidental/malicious modification.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/leds/led-class.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 00be3cb96c00..3775ad13762f 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -27,8 +27,7 @@ static LIST_HEAD(leds_lookup_list);
 
 static struct workqueue_struct *leds_wq;
 
-static ssize_t brightness_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
+static ssize_t brightness_show(struct device *dev, const struct device_attribute *attr, char *buf)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	unsigned int brightness;
@@ -44,8 +43,8 @@ static ssize_t brightness_show(struct device *dev,
 	return sysfs_emit(buf, "%u\n", brightness);
 }
 
-static ssize_t brightness_store(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t size)
+static ssize_t brightness_store(struct device *dev, const struct device_attribute *attr,
+				const char *buf, size_t size)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	unsigned long state;
@@ -74,10 +73,11 @@ static ssize_t brightness_store(struct device *dev,
 	mutex_unlock(&led_cdev->led_access);
 	return ret;
 }
-static DEVICE_ATTR_RW(brightness);
 
-static ssize_t max_brightness_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
+static const DEVICE_ATTR_RW(brightness);
+
+static ssize_t max_brightness_show(struct device *dev, const struct device_attribute *attr,
+				   char *buf)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	unsigned int max_brightness;
@@ -88,19 +88,20 @@ static ssize_t max_brightness_show(struct device *dev,
 
 	return sysfs_emit(buf, "%u\n", max_brightness);
 }
-static DEVICE_ATTR_RO(max_brightness);
 
-static struct attribute *led_class_attrs[] = {
+static const DEVICE_ATTR_RO(max_brightness);
+
+static const struct attribute *const led_class_attrs[] = {
 	&dev_attr_brightness.attr,
 	&dev_attr_max_brightness.attr,
 	NULL,
 };
 
 static const struct attribute_group led_group = {
-	.attrs = led_class_attrs,
+	.attrs_const = led_class_attrs,
 };
 
-static const struct attribute_group *led_groups[] = {
+static const struct attribute_group *const led_groups[] = {
 	&led_group,
 #ifdef CONFIG_LEDS_TRIGGERS
 	&led_trigger_group,
@@ -109,8 +110,8 @@ static const struct attribute_group *led_groups[] = {
 };
 
 #ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
-static ssize_t brightness_hw_changed_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
+static ssize_t brightness_hw_changed_show(struct device *dev, const struct device_attribute *attr,
+					  char *buf)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 
@@ -120,7 +121,7 @@ static ssize_t brightness_hw_changed_show(struct device *dev,
 	return sysfs_emit(buf, "%u\n", led_cdev->brightness_hw_changed);
 }
 
-static DEVICE_ATTR_RO(brightness_hw_changed);
+static const DEVICE_ATTR_RO(brightness_hw_changed);
 
 static int led_add_brightness_hw_changed(struct led_classdev *led_cdev)
 {
-- 
2.56.0

Re: [PATCH 01/10] leds: class: Constify sysfs attributes

[sashiko-bot]

> Mark all sysfs attributes as const to ensure that they are placed
> in readonly memory to prevent accidental/malicious modification.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018135339.1109271-1-W_Armin@gmx.de?part=1

[PATCH 02/10] leds: multicolor: Constify sysfs attributes

[Armin Wolf]

Mark all sysfs attributes as const to ensure that they are placed
in readonly memory to prevent accidental/malicious modification.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/leds/led-class-multicolor.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
index 59d46f8a90d5..57df0b2e785b 100644
--- a/drivers/leds/led-class-multicolor.c
+++ b/drivers/leds/led-class-multicolor.c
@@ -10,6 +10,7 @@
 #include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/slab.h>
+#include <linux/sysfs.h>
 #include <linux/uaccess.h>
 
 static unsigned int led_mc_get_max_intensity(struct led_classdev_mc *mcled_cdev, size_t index)
@@ -47,7 +48,7 @@ int led_mc_calc_color_components(struct led_classdev_mc *mcled_cdev,
 EXPORT_SYMBOL_GPL(led_mc_calc_color_components);
 
 static ssize_t multi_max_intensity_show(struct device *dev,
-					struct device_attribute *intensity_attr, char *buf)
+					const struct device_attribute *intensity_attr, char *buf)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
@@ -64,11 +65,11 @@ static ssize_t multi_max_intensity_show(struct device *dev,
 	buf[len++] = '\n';
 	return len;
 }
-static DEVICE_ATTR_RO(multi_max_intensity);
+static const DEVICE_ATTR_RO(multi_max_intensity);
 
 static ssize_t multi_intensity_store(struct device *dev,
-				struct device_attribute *intensity_attr,
-				const char *buf, size_t size)
+				     const struct device_attribute *intensity_attr, const char *buf,
+				     size_t size)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
@@ -110,8 +111,7 @@ static ssize_t multi_intensity_store(struct device *dev,
 }
 
 static ssize_t multi_intensity_show(struct device *dev,
-			      struct device_attribute *intensity_attr,
-			      char *buf)
+				    const struct device_attribute *intensity_attr, char *buf)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
@@ -128,11 +128,10 @@ static ssize_t multi_intensity_show(struct device *dev,
 	buf[len++] = '\n';
 	return len;
 }
-static DEVICE_ATTR_RW(multi_intensity);
+static const DEVICE_ATTR_RW(multi_intensity);
 
 static ssize_t multi_index_show(struct device *dev,
-			      struct device_attribute *multi_index_attr,
-			      char *buf)
+				const struct device_attribute *multi_index_attr, char *buf)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
@@ -150,9 +149,9 @@ static ssize_t multi_index_show(struct device *dev,
 	buf[len++] = '\n';
 	return len;
 }
-static DEVICE_ATTR_RO(multi_index);
+static const DEVICE_ATTR_RO(multi_index);
 
-static struct attribute *led_multicolor_attrs[] = {
+static const struct attribute *const led_multicolor_attrs[] = {
 	&dev_attr_multi_max_intensity.attr,
 	&dev_attr_multi_intensity.attr,
 	&dev_attr_multi_index.attr,
-- 
2.56.0

Re: [PATCH 02/10] leds: multicolor: Constify sysfs attributes

[sashiko-bot]

> Mark all sysfs attributes as const to ensure that they are placed
> in readonly memory to prevent accidental/malicious modification.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018135339.1109271-1-W_Armin@gmx.de?part=2

[PATCH 03/10] leds: flash: Constify sysfs attributes

[Armin Wolf]

Mark all sysfs attributes as const to ensure that they are placed
in readonly memory to prevent accidental/malicious modification.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/leds/led-class-flash.c | 62 +++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/drivers/leds/led-class-flash.c b/drivers/leds/led-class-flash.c
index 181dfd49c6ad..953265fddb7d 100644
--- a/drivers/leds/led-class-flash.c
+++ b/drivers/leds/led-class-flash.c
@@ -12,6 +12,7 @@
 #include <linux/leds.h>
 #include <linux/module.h>
 #include <linux/slab.h>
+#include <linux/sysfs.h>
 
 #define has_flash_op(fled_cdev, op)				\
 	(fled_cdev && fled_cdev->ops->op)
@@ -34,7 +35,8 @@ static const char * const led_flash_fault_names[] = {
 };
 
 static ssize_t flash_brightness_store(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t size)
+				      const struct device_attribute *attr, const char *buf,
+				      size_t size)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
@@ -62,8 +64,8 @@ static ssize_t flash_brightness_store(struct device *dev,
 	return ret;
 }
 
-static ssize_t flash_brightness_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
+static ssize_t flash_brightness_show(struct device *dev, const struct device_attribute *attr,
+				     char *buf)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
@@ -73,20 +75,20 @@ static ssize_t flash_brightness_show(struct device *dev,
 
 	return sprintf(buf, "%u\n", fled_cdev->brightness.val);
 }
-static DEVICE_ATTR_RW(flash_brightness);
+static const DEVICE_ATTR_RW(flash_brightness);
 
-static ssize_t max_flash_brightness_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
+static ssize_t max_flash_brightness_show(struct device *dev, const struct device_attribute *attr,
+					 char *buf)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
 
 	return sprintf(buf, "%u\n", fled_cdev->brightness.max);
 }
-static DEVICE_ATTR_RO(max_flash_brightness);
+static const DEVICE_ATTR_RO(max_flash_brightness);
 
-static ssize_t flash_strobe_store(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t size)
+static ssize_t flash_strobe_store(struct device *dev, const struct device_attribute *attr,
+				  const char *buf, size_t size)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
@@ -116,8 +118,7 @@ static ssize_t flash_strobe_store(struct device *dev,
 	return ret;
 }
 
-static ssize_t flash_strobe_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
+static ssize_t flash_strobe_show(struct device *dev, const struct device_attribute *attr, char *buf)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
@@ -131,10 +132,10 @@ static ssize_t flash_strobe_show(struct device *dev,
 
 	return sprintf(buf, "%u\n", state);
 }
-static DEVICE_ATTR_RW(flash_strobe);
+static const DEVICE_ATTR_RW(flash_strobe);
 
-static ssize_t flash_timeout_store(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t size)
+static ssize_t flash_timeout_store(struct device *dev, const struct device_attribute *attr,
+				   const char *buf, size_t size)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
@@ -162,28 +163,27 @@ static ssize_t flash_timeout_store(struct device *dev,
 	return ret;
 }
 
-static ssize_t flash_timeout_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
+static ssize_t flash_timeout_show(struct device *dev, const struct device_attribute *attr,
+				  char *buf)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
 
 	return sprintf(buf, "%u\n", fled_cdev->timeout.val);
 }
-static DEVICE_ATTR_RW(flash_timeout);
+static const DEVICE_ATTR_RW(flash_timeout);
 
-static ssize_t max_flash_timeout_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
+static ssize_t max_flash_timeout_show(struct device *dev, const struct device_attribute *attr,
+				      char *buf)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
 
 	return sprintf(buf, "%u\n", fled_cdev->timeout.max);
 }
-static DEVICE_ATTR_RO(max_flash_timeout);
+static const DEVICE_ATTR_RO(max_flash_timeout);
 
-static ssize_t flash_fault_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
+static ssize_t flash_fault_show(struct device *dev, const struct device_attribute *attr, char *buf)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
@@ -205,44 +205,44 @@ static ssize_t flash_fault_show(struct device *dev,
 	ret += sysfs_emit_at(buf, ret, "\n");
 	return ret;
 }
-static DEVICE_ATTR_RO(flash_fault);
+static const DEVICE_ATTR_RO(flash_fault);
 
-static struct attribute *led_flash_strobe_attrs[] = {
+static const struct attribute *const led_flash_strobe_attrs[] = {
 	&dev_attr_flash_strobe.attr,
 	NULL,
 };
 
-static struct attribute *led_flash_timeout_attrs[] = {
+static const struct attribute *const led_flash_timeout_attrs[] = {
 	&dev_attr_flash_timeout.attr,
 	&dev_attr_max_flash_timeout.attr,
 	NULL,
 };
 
-static struct attribute *led_flash_brightness_attrs[] = {
+static const struct attribute *const led_flash_brightness_attrs[] = {
 	&dev_attr_flash_brightness.attr,
 	&dev_attr_max_flash_brightness.attr,
 	NULL,
 };
 
-static struct attribute *led_flash_fault_attrs[] = {
+static const struct attribute *const led_flash_fault_attrs[] = {
 	&dev_attr_flash_fault.attr,
 	NULL,
 };
 
 static const struct attribute_group led_flash_strobe_group = {
-	.attrs = led_flash_strobe_attrs,
+	.attrs_const = led_flash_strobe_attrs,
 };
 
 static const struct attribute_group led_flash_timeout_group = {
-	.attrs = led_flash_timeout_attrs,
+	.attrs_const = led_flash_timeout_attrs,
 };
 
 static const struct attribute_group led_flash_brightness_group = {
-	.attrs = led_flash_brightness_attrs,
+	.attrs_const = led_flash_brightness_attrs,
 };
 
 static const struct attribute_group led_flash_fault_group = {
-	.attrs = led_flash_fault_attrs,
+	.attrs_const = led_flash_fault_attrs,
 };
 
 static void led_flash_resume(struct led_classdev *led_cdev)
-- 
2.56.0

Re: [PATCH 03/10] leds: flash: Constify sysfs attributes

[sashiko-bot]

> Mark all sysfs attributes as const to ensure that they are placed
> in readonly memory to prevent accidental/malicious modification.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018135339.1109271-1-W_Armin@gmx.de?part=3

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

[Armin Wolf]

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[] = {
-	&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
-- 
2.56.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] leds: class: sleeping in atomic context in led_classdev_notify_brightness_hw_changed()
- [High] leds: NULL or ERR_PTR dereference in led_classdev_notify_brightness_hw_changed()
--

commit a70a68ffb21d71853f220e24fa090cea4db91df2
Author: Armin Wolf <W_Armin@gmx.de>

leds: class: Create "brightness_hw_changed" attribute atomically

This patch converts the brightness_hw_changed sysfs attribute to a static
attribute group with an is_visible callback so that the device core creates
it before sending the uevent to userspace. It also removes the cached
kernfs_node pointer from struct led_classdev and switches
led_classdev_notify_brightness_hw_changed() to use sysfs_notify().

> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 3775ad13762f5..d460f041ed86e 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c

[ ... ]

> @@ -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,
> +};

[ ... ]

> +#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);

[Severity: High]
Can replacing sysfs_notify_dirent() with sysfs_notify() here cause sleeping
in atomic context?

Previously, sysfs_notify_dirent() called kernfs_notify() on the cached
kernfs_node, which may be called from any context since it only uses
rcu_read_lock(), spin_lock_irqsave(&kernfs_notify_lock, flags), and
schedule_work().

With sysfs_notify(&led_cdev->dev->kobj, NULL, "brightness_hw_changed"), a
non-NULL attribute name causes sysfs_notify() to look up the node via
kernfs_find_and_get() -> kernfs_find_and_get_ns(), which acquires the
sleeping rw_semaphore root->kernfs_rwsem:

fs/sysfs/file.c:sysfs_notify() {
	...
	if (kn && attr) {
		tmp = kernfs_find_and_get(kn, attr);
		kernfs_put(kn);
		kn = tmp;
	}
	...
}

fs/kernfs/dir.c:kernfs_find_and_get_ns() {
	...
	down_read(&root->kernfs_rwsem);
	kn = kernfs_find_ns(parent, name, ns);
	kernfs_get(kn);
	up_read(&root->kernfs_rwsem);
	...
}

In drivers/hid/hid-lg-g15.c, lg_g13_event() and lg_g510_leds_event() call
led_classdev_notify_brightness_hw_changed() when a hardware backlight
brightness change report arrives:

drivers/hid/hid-lg-g15.c:lg_g13_event() {
	...
	if (backlight_disabled ^ g15->backlight_disabled) {
		led_classdev_notify_brightness_hw_changed(
			&g15->leds[LG_G15_KBD_BRIGHTNESS].mcdev.led_cdev,
			backlight_disabled
			? 0 : g15->leds[LG_G15_KBD_BRIGHTNESS].brightness);
		g15->backlight_disabled = backlight_disabled;
	}
	...
}

Those handlers are invoked from lg_g15_raw_event() via the USB interrupt URB
completion path hid_irq_in() -> hid_safe_input_report() ->
__hid_input_report(), which runs in hardirq/atomic context.

[Severity: High]
Also in led_classdev_notify_brightness_hw_changed(), could removing the
WARN_ON(!led_cdev->brightness_hw_changed_kn) guard lead to a NULL or ERR_PTR
dereference on led_cdev->dev (or a use-after-free after commit "leds: class:
Split led_classdev_register_ext()" later in the series)?

Previously, if a classdev was not registered or did not set
LED_BRIGHT_HW_CHANGED, led_cdev->brightness_hw_changed_kn was NULL and the
function warned and returned before touching led_cdev->dev. That behavior is
also documented in Documentation/leds/leds-class.rst:

  Calling led_classdev_notify_brightness_hw_changed on a classdev not
  registered with the LED_BRIGHT_HW_CHANGED flag is a bug and will
  trigger a WARN_ON.

Without a guard, sysfs_notify(&led_cdev->dev->kobj, ...) immediately
dereferences kobj->sd in fs/sysfs/file.c:sysfs_notify().

For example, in drivers/platform/x86/system76_acpi.c, system76_probe() skips
registering data->kb_led when data->kbled_type == KBLED_NONE, leaving
data->kb_led.dev as NULL:

drivers/platform/x86/system76_acpi.c:system76_probe() {
	...
	if (data->kbled_type != KBLED_NONE) {
		err = devm_led_classdev_register(&pdev->dev, &data->kb_led);
		if (err)
			return err;
	}
	...
}

However, system76_notify() is still installed and calls kb_led_notify() ->
led_classdev_notify_brightness_hw_changed(&data->kb_led, ...) through
kb_led_hotkey_hardware(), kb_led_hotkey_toggle(), kb_led_hotkey_down(), and
kb_led_hotkey_up() on ACPI events 0x80-0x83.

Similarly, in drivers/platform/x86/toshiba_acpi.c, toshiba_acpi_probe()
ignores the return value of led_classdev_register(&pdev->dev, &dev->kbd_led):

drivers/platform/x86/toshiba_acpi.c:toshiba_acpi_probe() {
	...
	if (dev->kbd_illum_supported &&
	    (dev->kbd_mode == SCI_KBD_MODE_FNZ || dev->kbd_type == 2)) {
		dev->kbd_led.name = "toshiba::kbd_backlight";
		dev->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
		dev->kbd_led.max_brightness = 1;
		dev->kbd_led.brightness_set = toshiba_kbd_backlight_set;
		dev->kbd_led.brightness_get = toshiba_kbd_backlight_get;
		led_classdev_register(&pdev->dev, &dev->kbd_led);
	}
	...
}

If registration fails, dev->kbd_led.dev is left as NULL or ERR_PTR (or a
freed pointer after "leds: class: Split led_classdev_register_ext()"), while
toshiba_acpi_notify() and toshiba_acpi_kbd_bl_work() still call
led_classdev_notify_brightness_hw_changed(&dev->kbd_led, ...).

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

[ ... ]

>
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018135339.1109271-1-W_Armin@gmx.de?part=4

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

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

[PATCH 05/10] leds: class: Fix race condition when creating the device

[Armin Wolf]

Userspace applications can access the sysfs attributes as soon as
device_create_with_groups() has returned. Two of those sysfs
attributes ("brightness_hw_changed" and "max_brightness") access
fields that are not properly initialized at this point, with only
"max_brightness" guarding against this by taking the LED access lock.

Fix the race conditon affecting "brightness_hw_changed" by creating
the device after the associated fields have been properly initialized.
This also allows us to drop the LED access lock inside
max_brightness_show(), as the maximum brightness does not change after
initialization.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/leds/led-class.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index d460f041ed86..e1f1b710536b 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -81,13 +81,8 @@ static ssize_t max_brightness_show(struct device *dev, const struct device_attri
 				   char *buf)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
-	unsigned int max_brightness;
 
-	mutex_lock(&led_cdev->led_access);
-	max_brightness = led_cdev->max_brightness;
-	mutex_unlock(&led_cdev->led_access);
-
-	return sysfs_emit(buf, "%u\n", max_brightness);
+	return sysfs_emit(buf, "%u\n", led_cdev->max_brightness);
 }
 
 static const DEVICE_ATTR_RO(max_brightness);
@@ -524,16 +519,6 @@ int led_classdev_register_ext(struct device *parent,
 		dev_warn(parent, "LED %s color identifier out of range\n", final_name);
 
 	mutex_init(&led_cdev->led_access);
-	mutex_lock(&led_cdev->led_access);
-	led_cdev->dev = device_create_with_groups(&leds_class, parent, 0,
-						  led_cdev, led_cdev->groups, "%s", final_name);
-	if (IS_ERR(led_cdev->dev)) {
-		mutex_unlock(&led_cdev->led_access);
-		return PTR_ERR(led_cdev->dev);
-	}
-	if (init_data && init_data->fwnode)
-		device_set_node(led_cdev->dev, init_data->fwnode);
-
 	led_cdev->work_flags = 0;
 #ifdef CONFIG_LEDS_TRIGGERS
 	init_rwsem(&led_cdev->trigger_lock);
@@ -549,6 +534,17 @@ int led_classdev_register_ext(struct device *parent,
 	if (!led_cdev->max_brightness)
 		led_cdev->max_brightness = LED_FULL;
 
+	mutex_lock(&led_cdev->led_access);
+	led_cdev->dev = device_create_with_groups(&leds_class, parent, 0, led_cdev,
+						  led_cdev->groups, "%s", final_name);
+	if (IS_ERR(led_cdev->dev)) {
+		mutex_unlock(&led_cdev->led_access);
+		return PTR_ERR(led_cdev->dev);
+	}
+
+	if (init_data && init_data->fwnode)
+		device_set_node(led_cdev->dev, init_data->fwnode);
+
 	led_update_brightness(led_cdev);
 
 	led_cdev->wq = leds_wq;
-- 
2.56.0

Re: [PATCH 05/10] leds: class: Fix race condition when creating the device

[sashiko-bot]

> Userspace applications can access the sysfs attributes as soon as
> device_create_with_groups() has returned. Two of those sysfs
> attributes ("brightness_hw_changed" and "max_brightness") access
> fields that are not properly initialized at this point, with only
> "max_brightness" guarding against this by taking the LED access lock.
> 
> Fix the race conditon affecting "brightness_hw_changed" by creating
> the device after the associated fields have been properly initialized.
> This also allows us to drop the LED access lock inside
> max_brightness_show(), as the maximum brightness does not change after
> initialization.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018135339.1109271-1-W_Armin@gmx.de?part=5

[PATCH 06/10] leds: class: Take the LED access lock when reading brightness_hw_changed

[Armin Wolf]

Reading the brightness_hw_changed field needs to be guarded against
concurrent writes by led_classdev_notify_brightness_hw_changed().

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/leds/led-class.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index e1f1b710536b..77a0fce03952 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -6,6 +6,7 @@
  * Copyright (C) 2005-2007 Richard Purdie <rpurdie@openedhand.com>
  */
 
+#include <linux/cleanup.h>
 #include <linux/ctype.h>
 #include <linux/device.h>
 #include <linux/err.h>
@@ -102,11 +103,15 @@ static ssize_t brightness_hw_changed_show(struct device *dev, const struct devic
 					  char *buf)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	unsigned int brightness_hw_changed;
 
-	if (led_cdev->brightness_hw_changed == -1)
+	scoped_guard(mutex, &led_cdev->led_access)
+		brightness_hw_changed = led_cdev->brightness_hw_changed;
+
+	if (brightness_hw_changed == -1)
 		return -ENODATA;
 
-	return sysfs_emit(buf, "%u\n", led_cdev->brightness_hw_changed);
+	return sysfs_emit(buf, "%u\n", brightness_hw_changed);
 }
 
 static const DEVICE_ATTR_RO(brightness_hw_changed);
-- 
2.56.0

Re: [PATCH 06/10] leds: class: Take the LED access lock when reading brightness_hw_changed

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] leds: one-sided locking in brightness_hw_changed_show()
--

commit ef875f7f46dcbf0a55900c763a37bd3cfcb02775
Author: Armin Wolf <W_Armin@gmx.de>

leds: class: Take the LED access lock when reading brightness_hw_changed

This patch acquires the led_cdev->led_access mutex in
brightness_hw_changed_show() to guard reading brightness_hw_changed
against concurrent updates by led_classdev_notify_brightness_hw_changed().

> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index e1f1b710536b0..77a0fce039528 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
[ ... ]
> @@ -102,11 +103,15 @@ static ssize_t brightness_hw_changed_show(struct device *dev, const struct devic
>  					  char *buf)
>  {
>  	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	unsigned int brightness_hw_changed;
>  
> -	if (led_cdev->brightness_hw_changed == -1)
> +	scoped_guard(mutex, &led_cdev->led_access)
> +		brightness_hw_changed = led_cdev->brightness_hw_changed;
> +
> +	if (brightness_hw_changed == -1)
>  		return -ENODATA;
>  
> -	return sysfs_emit(buf, "%u\n", led_cdev->brightness_hw_changed);
> +	return sysfs_emit(buf, "%u\n", brightness_hw_changed);
>  }
>  
>  static const DEVICE_ATTR_RO(brightness_hw_changed);

[Severity: Medium]
Does taking led_cdev->led_access here actually synchronize against
led_classdev_notify_brightness_hw_changed()?

Looking at drivers/leds/led-class.c, the writer updates
led_cdev->brightness_hw_changed without holding led_cdev->led_access:

void led_classdev_notify_brightness_hw_changed(struct led_classdev *led_cdev, unsigned int brightness)
{
	led_cdev->brightness_hw_changed = brightness;
	sysfs_notify(&led_cdev->dev->kobj, NULL, "brightness_hw_changed");
}

In addition, most callers invoke led_classdev_notify_brightness_hw_changed()
without holding led_cdev->led_access, such as t14s_kbd_bl_update() in
drivers/platform/arm64/lenovo-thinkpad-t14s.c:

static void t14s_kbd_bl_update(struct t14s_ec *ec)
{
	enum led_brightness brightness = t14s_kbd_bl_get(&ec->kbd_backlight);

	led_classdev_notify_brightness_hw_changed(&ec->kbd_backlight, brightness);
}

Some callers also run in atomic context where a sleeping mutex cannot be
taken, such as lg_g15_raw_event() -> lg_g13_event() in
drivers/hid/hid-lg-g15.c:

	if (backlight_disabled ^ g15->backlight_disabled) {
		led_classdev_notify_brightness_hw_changed(
			&g15->leds[LG_G15_KBD_BRIGHTNESS].mcdev.led_cdev,
			backlight_disabled
			? 0 : g15->leds[LG_G15_KBD_BRIGHTNESS].brightness);
		g15->backlight_disabled = backlight_disabled;
	}

Meanwhile, uniwill_ec_notify() in drivers/platform/x86/uniwill/uniwill-acpi.c
already holds led_cdev->led_access across the call:

	guard(mutex)(&led_cdev->led_access);

	/* Sync the LED brightness with the actual hardware state */
	ret = led_update_brightness(led_cdev);
	if (ret < 0)
		return ret;

	led_classdev_notify_brightness_hw_changed(led_cdev, brightness);

Since the writer side does not consistently hold led_cdev->led_access (and
cannot take it inside led_classdev_notify_brightness_hw_changed() without
sleeping in atomic context or deadlocking uniwill_ec_notify()), should
brightness_hw_changed_show() and led_classdev_notify_brightness_hw_changed()
use READ_ONCE() and WRITE_ONCE() instead, similar to how trigger_hw_changed
is handled in drivers/leds/led-triggers.c?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018135339.1109271-1-W_Armin@gmx.de?part=6

Re: [PATCH 06/10] leds: class: Take the LED access lock when reading brightness_hw_changed

[Armin Wolf]

Am 18.10.26 um 16:03 schrieb sashiko-bot@kernel.org:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] leds: one-sided locking in brightness_hw_changed_show()
> --
>
> commit ef875f7f46dcbf0a55900c763a37bd3cfcb02775
> Author: Armin Wolf <W_Armin@gmx.de>
>
> leds: class: Take the LED access lock when reading brightness_hw_changed
>
> This patch acquires the led_cdev->led_access mutex in
> brightness_hw_changed_show() to guard reading brightness_hw_changed
> against concurrent updates by led_classdev_notify_brightness_hw_changed().
>
>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>> index e1f1b710536b0..77a0fce039528 100644
>> --- a/drivers/leds/led-class.c
>> +++ b/drivers/leds/led-class.c
> [ ... ]
>> @@ -102,11 +103,15 @@ static ssize_t brightness_hw_changed_show(struct device *dev, const struct devic
>>   					  char *buf)
>>   {
>>   	struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> +	unsigned int brightness_hw_changed;
>>   
>> -	if (led_cdev->brightness_hw_changed == -1)
>> +	scoped_guard(mutex, &led_cdev->led_access)
>> +		brightness_hw_changed = led_cdev->brightness_hw_changed;
>> +
>> +	if (brightness_hw_changed == -1)
>>   		return -ENODATA;
>>   
>> -	return sysfs_emit(buf, "%u\n", led_cdev->brightness_hw_changed);
>> +	return sysfs_emit(buf, "%u\n", brightness_hw_changed);
>>   }
>>   
>>   static const DEVICE_ATTR_RO(brightness_hw_changed);
> [Severity: Medium]
> Does taking led_cdev->led_access here actually synchronize against
> led_classdev_notify_brightness_hw_changed()?
>
> Looking at drivers/leds/led-class.c, the writer updates
> led_cdev->brightness_hw_changed without holding led_cdev->led_access:
>
> void led_classdev_notify_brightness_hw_changed(struct led_classdev *led_cdev, unsigned int brightness)
> {
> 	led_cdev->brightness_hw_changed = brightness;
> 	sysfs_notify(&led_cdev->dev->kobj, NULL, "brightness_hw_changed");
> }
>
> In addition, most callers invoke led_classdev_notify_brightness_hw_changed()
> without holding led_cdev->led_access, such as t14s_kbd_bl_update() in
> drivers/platform/arm64/lenovo-thinkpad-t14s.c:
>
> static void t14s_kbd_bl_update(struct t14s_ec *ec)
> {
> 	enum led_brightness brightness = t14s_kbd_bl_get(&ec->kbd_backlight);
>
> 	led_classdev_notify_brightness_hw_changed(&ec->kbd_backlight, brightness);
> }
>
> Some callers also run in atomic context where a sleeping mutex cannot be
> taken, such as lg_g15_raw_event() -> lg_g13_event() in
> drivers/hid/hid-lg-g15.c:
>
> 	if (backlight_disabled ^ g15->backlight_disabled) {
> 		led_classdev_notify_brightness_hw_changed(
> 			&g15->leds[LG_G15_KBD_BRIGHTNESS].mcdev.led_cdev,
> 			backlight_disabled
> 			? 0 : g15->leds[LG_G15_KBD_BRIGHTNESS].brightness);
> 		g15->backlight_disabled = backlight_disabled;
> 	}
>
> Meanwhile, uniwill_ec_notify() in drivers/platform/x86/uniwill/uniwill-acpi.c
> already holds led_cdev->led_access across the call:
>
> 	guard(mutex)(&led_cdev->led_access);
>
> 	/* Sync the LED brightness with the actual hardware state */
> 	ret = led_update_brightness(led_cdev);
> 	if (ret < 0)
> 		return ret;
>
> 	led_classdev_notify_brightness_hw_changed(led_cdev, brightness);
>
> Since the writer side does not consistently hold led_cdev->led_access (and
> cannot take it inside led_classdev_notify_brightness_hw_changed() without
> sleeping in atomic context or deadlocking uniwill_ec_notify()), should
> brightness_hw_changed_show() and led_classdev_notify_brightness_hw_changed()
> use READ_ONCE() and WRITE_ONCE() instead, similar to how trigger_hw_changed
> is handled in drivers/leds/led-triggers.c?

I am not sure if READ_ONCE() guarantees atomic behavior here. What do the LED
subsystem maintainers say?

Thanks,
Armin Wolf

Re: [PATCH 06/10] leds: class: Take the LED access lock when reading brightness_hw_changed

[Rong Zhang]

Hi Armin,

On Sun, 2026-10-18 at 19:08 +0200, Armin Wolf wrote:
> Am 18.10.26 um 16:03 schrieb sashiko-bot@kernel.org:
> 
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [Medium] leds: one-sided locking in brightness_hw_changed_show()
> > --
> > 
> > commit ef875f7f46dcbf0a55900c763a37bd3cfcb02775
> > Author: Armin Wolf <W_Armin@gmx.de>
> > 
> > leds: class: Take the LED access lock when reading brightness_hw_changed
> > 
> > This patch acquires the led_cdev->led_access mutex in
> > brightness_hw_changed_show() to guard reading brightness_hw_changed
> > against concurrent updates by led_classdev_notify_brightness_hw_changed().
> > 
> > > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> > > index e1f1b710536b0..77a0fce039528 100644
> > > --- a/drivers/leds/led-class.c
> > > +++ b/drivers/leds/led-class.c
> > [ ... ]
> > > @@ -102,11 +103,15 @@ static ssize_t brightness_hw_changed_show(struct device *dev, const struct devic
> > >   					  char *buf)
> > >   {
> > >   	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> > > +	unsigned int brightness_hw_changed;
> > >   
> > > -	if (led_cdev->brightness_hw_changed == -1)
> > > +	scoped_guard(mutex, &led_cdev->led_access)
> > > +		brightness_hw_changed = led_cdev->brightness_hw_changed;
> > > +
> > > +	if (brightness_hw_changed == -1)
> > >   		return -ENODATA;
> > >   
> > > -	return sysfs_emit(buf, "%u\n", led_cdev->brightness_hw_changed);
> > > +	return sysfs_emit(buf, "%u\n", brightness_hw_changed);
> > >   }
> > >   
> > >   static const DEVICE_ATTR_RO(brightness_hw_changed);
> > [Severity: Medium]
> > Does taking led_cdev->led_access here actually synchronize against
> > led_classdev_notify_brightness_hw_changed()?
> > 
> > Looking at drivers/leds/led-class.c, the writer updates
> > led_cdev->brightness_hw_changed without holding led_cdev->led_access:
> > 
> > void led_classdev_notify_brightness_hw_changed(struct led_classdev *led_cdev, unsigned int brightness)
> > {
> > 	led_cdev->brightness_hw_changed = brightness;
> > 	sysfs_notify(&led_cdev->dev->kobj, NULL, "brightness_hw_changed");
> > }
> > 
> > In addition, most callers invoke led_classdev_notify_brightness_hw_changed()
> > without holding led_cdev->led_access, such as t14s_kbd_bl_update() in
> > drivers/platform/arm64/lenovo-thinkpad-t14s.c:
> > 
> > static void t14s_kbd_bl_update(struct t14s_ec *ec)
> > {
> > 	enum led_brightness brightness = t14s_kbd_bl_get(&ec->kbd_backlight);
> > 
> > 	led_classdev_notify_brightness_hw_changed(&ec->kbd_backlight, brightness);
> > }
> > 
> > Some callers also run in atomic context where a sleeping mutex cannot be
> > taken, such as lg_g15_raw_event() -> lg_g13_event() in
> > drivers/hid/hid-lg-g15.c:
> > 
> > 	if (backlight_disabled ^ g15->backlight_disabled) {
> > 		led_classdev_notify_brightness_hw_changed(
> > 			&g15->leds[LG_G15_KBD_BRIGHTNESS].mcdev.led_cdev,
> > 			backlight_disabled
> > 			? 0 : g15->leds[LG_G15_KBD_BRIGHTNESS].brightness);
> > 		g15->backlight_disabled = backlight_disabled;
> > 	}
> > 
> > Meanwhile, uniwill_ec_notify() in drivers/platform/x86/uniwill/uniwill-acpi.c
> > already holds led_cdev->led_access across the call:
> > 
> > 	guard(mutex)(&led_cdev->led_access);
> > 
> > 	/* Sync the LED brightness with the actual hardware state */
> > 	ret = led_update_brightness(led_cdev);
> > 	if (ret < 0)
> > 		return ret;
> > 
> > 	led_classdev_notify_brightness_hw_changed(led_cdev, brightness);
> > 
> > Since the writer side does not consistently hold led_cdev->led_access (and
> > cannot take it inside led_classdev_notify_brightness_hw_changed() without
> > sleeping in atomic context or deadlocking uniwill_ec_notify()), should
> > brightness_hw_changed_show() and led_classdev_notify_brightness_hw_changed()
> > use READ_ONCE() and WRITE_ONCE() instead, similar to how trigger_hw_changed
> > is handled in drivers/leds/led-triggers.c?
> 
> I am not sure if READ_ONCE() guarantees atomic behavior here. What do the LED
> subsystem maintainers say?

While I am not a maintainer, paired READ_ONCE() and WRITE_ONCE() is
usually sufficient, as the attribute is only a hint for userspace OSD.

The memory access itself is inherently atomic, i.e., neither a RMW
operation nor a split R/W. Since the led_cdev->brightness_hw_changed is
never used to synchronize other things, no memory barrier is needed here.
Using rwonce prevents the compiler from reordering memory access, which
fulfills our requirements.

Thanks,
Rong

> 
> Thanks,
> Armin Wolf

[PATCH 07/10] leds: class: Use correct type for brightness_hw_changed field

[Armin Wolf]

Device drivers can supply a unsigned integer using
led_classdev_notify_brightness_hw_changed(), but the associated
field can only store signed integers, leading to issues for very
large brightness values.

Fix this by using the correct type for the brightness_hw_changed
field.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 include/linux/leds.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/leds.h b/include/linux/leds.h
index 6f975078cc14..a057ae623d5a 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -235,7 +235,7 @@ struct led_classdev {
 #endif
 
 #ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
-	int			 brightness_hw_changed;
+	unsigned int		brightness_hw_changed;
 #endif
 
 #ifdef CONFIG_LEDS_TRIGGERS_HW_CHANGED
-- 
2.56.0

Re: [PATCH 07/10] leds: class: Use correct type for brightness_hw_changed field

[sashiko-bot]

> Device drivers can supply a unsigned integer using
> led_classdev_notify_brightness_hw_changed(), but the associated
> field can only store signed integers, leading to issues for very
> large brightness values.
> 
> Fix this by using the correct type for the brightness_hw_changed
> field.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018135339.1109271-1-W_Armin@gmx.de?part=7

[PATCH 08/10] leds: class: Split led_classdev_register_ext()

[Armin Wolf]

Split led_classdev_register_ext() into two seperate internal functions
to prepare for future changes around LED class device initialization.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/leds/led-class.c | 105 +++++++++++++++++++++++++++++++--------
 drivers/leds/leds.h      |   3 ++
 2 files changed, 88 insertions(+), 20 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 77a0fce03952..ace417a2388e 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -228,9 +228,15 @@ static struct led_classdev *led_module_get(struct device *led_dev)
 	return led_cdev;
 }
 
+static void led_classdev_release(struct device *dev)
+{
+	kfree(dev);
+}
+
 static const struct class leds_class = {
 	.name = "leds",
 	.dev_groups = led_groups,
+	.dev_release = led_classdev_release,
 	.pm = &leds_class_dev_pm_ops,
 };
 
@@ -465,16 +471,21 @@ static int led_classdev_next_name(const char *init_name, char *name,
 }
 
 /**
- * led_classdev_register_ext - register a new object of led_classdev class
- *			       with init data.
- *
- * @parent: parent of LED device
+ * led_classdev_init - Initialize a new LED class device.
+ * @parent: parent of LED device.
  * @led_cdev: the led_classdev structure for this device.
- * @init_data: LED class device initialization data
+ * @init_data: LED class device initialization data.
+ *
+ * Initializes a new LED class device, but does not register with the
+ * subsystem yet. Useful for performing additional initialization before
+ * adding the LED class device to the subsystem using led_classdev_add().
+ * After calling this function put_device() needs to be called on the dev
+ * field inside the LED class device in order to dispose it.
+ *
+ * Return: Zero on success or a negative error code on failure.
  */
-int led_classdev_register_ext(struct device *parent,
-			      struct led_classdev *led_cdev,
-			      struct led_init_data *init_data)
+int led_classdev_init(struct device *parent, struct led_classdev *led_cdev,
+		      struct led_init_data *init_data)
 {
 	char composed_name[LED_MAX_NAME_SIZE];
 	char final_name[LED_MAX_NAME_SIZE];
@@ -539,23 +550,54 @@ int led_classdev_register_ext(struct device *parent,
 	if (!led_cdev->max_brightness)
 		led_cdev->max_brightness = LED_FULL;
 
-	mutex_lock(&led_cdev->led_access);
-	led_cdev->dev = device_create_with_groups(&leds_class, parent, 0, led_cdev,
-						  led_cdev->groups, "%s", final_name);
-	if (IS_ERR(led_cdev->dev)) {
-		mutex_unlock(&led_cdev->led_access);
-		return PTR_ERR(led_cdev->dev);
+	led_cdev->dev = kzalloc_obj(*led_cdev->dev);
+	if (!led_cdev->dev)
+		return -ENOMEM;
+
+	device_initialize(led_cdev->dev);
+	led_cdev->dev->class = &leds_class;
+	led_cdev->dev->parent = parent;
+	led_cdev->dev->groups = led_cdev->groups;
+	dev_set_drvdata(led_cdev->dev, led_cdev);
+	ret = kobject_set_name(&led_cdev->dev->kobj, "%s", final_name);
+	if (ret < 0)
+	{
+		put_device(led_cdev->dev);
+		return ret;
 	}
 
 	if (init_data && init_data->fwnode)
 		device_set_node(led_cdev->dev, init_data->fwnode);
 
-	led_update_brightness(led_cdev);
-
 	led_cdev->wq = leds_wq;
 
 	led_init_core(led_cdev);
 
+	return 0;
+}
+EXPORT_SYMBOL_GPL(led_classdev_init);
+
+/**
+ * led_classdev_add - Add a new LED class device to the subsystem.
+ * @led_cdev: the led_classdev structure for this device.
+ *
+ * Adds a new LED class device to the subsystem, allowing userspace applications
+ * and/or kernel consumers to access it.
+ *
+ * Returns: Zero on success or a negative error code on failure.
+ */
+int led_classdev_add(struct led_classdev *led_cdev)
+{
+	int ret;
+
+	guard(mutex)(&led_cdev->led_access);
+
+	ret = device_add(led_cdev->dev);
+	if (ret < 0)
+		return ret;
+
+	led_update_brightness(led_cdev);
+
 	/* add to the list of leds */
 	down_write(&leds_list_lock);
 	list_add_tail(&led_cdev->node, &leds_list);
@@ -565,13 +607,36 @@ int led_classdev_register_ext(struct device *parent,
 	led_trigger_set_default(led_cdev);
 #endif
 
-	mutex_unlock(&led_cdev->led_access);
-
-	dev_dbg(parent, "Registered led device: %s\n",
-			led_cdev->name);
+	dev_dbg(led_cdev->dev->parent, "Registered led device: %s\n", led_cdev->name);
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(led_classdev_add);
+
+/**
+ * led_classdev_register_ext - register a new object of led_classdev class
+ *                             with init data.
+ *
+ * @parent: parent of LED device.
+ * @led_cdev: the led_classdev structure for this device.
+ * @init_data: LED class device initialization data.
+ */
+int led_classdev_register_ext(struct device *parent,
+                              struct led_classdev *led_cdev,
+                              struct led_init_data *init_data)
+{
+	int ret;
+
+	ret = led_classdev_init(parent, led_cdev, init_data);
+	if (ret < 0)
+		return ret;
+
+	ret = led_classdev_add(led_cdev);
+	if (ret < 0)
+		put_device(led_cdev->dev);
+
+	return ret;
+}
 EXPORT_SYMBOL_GPL(led_classdev_register_ext);
 
 /**
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index 6d00e6f44126..2d977c5a3520 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -18,6 +18,9 @@ static inline int led_get_brightness(struct led_classdev *led_cdev)
 }
 
 void led_init_core(struct led_classdev *led_cdev);
+int led_classdev_init(struct device *parent, struct led_classdev *led_cdev,
+		      struct led_init_data *init_data);
+int led_classdev_add(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);
-- 
2.56.0

Re: [PATCH 08/10] leds: class: Split led_classdev_register_ext()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] leds: dangling led_cdev->dev pointer on error in led_classdev_init()
- [High] leds: premature device_set_node() in led_classdev_init() breaks fw_devlink
--

commit 7efb333e9420ea13e705d9a114384e841e92d2b2
Author: Armin Wolf <W_Armin@gmx.de>

leds: class: Split led_classdev_register_ext()

This commit splits led_classdev_register_ext() into led_classdev_init() and
led_classdev_add() so that LED class devices can be initialized before
being added to the subsystem.

> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 77a0fce039528..ace417a2388e0 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c

[ ... ]

> @@ -539,23 +550,54 @@ int led_classdev_register_ext(struct device *parent,
>  	if (!led_cdev->max_brightness)
>  		led_cdev->max_brightness = LED_FULL;
>  
> -	mutex_lock(&led_cdev->led_access);
> -	led_cdev->dev = device_create_with_groups(&leds_class, parent, 0, led_cdev,
> -						  led_cdev->groups, "%s", final_name);
> -	if (IS_ERR(led_cdev->dev)) {
> -		mutex_unlock(&led_cdev->led_access);
> -		return PTR_ERR(led_cdev->dev);
> +	led_cdev->dev = kzalloc_obj(*led_cdev->dev);
> +	if (!led_cdev->dev)
> +		return -ENOMEM;
> +
> +	device_initialize(led_cdev->dev);
> +	led_cdev->dev->class = &leds_class;
> +	led_cdev->dev->parent = parent;
> +	led_cdev->dev->groups = led_cdev->groups;
> +	dev_set_drvdata(led_cdev->dev, led_cdev);
> +	ret = kobject_set_name(&led_cdev->dev->kobj, "%s", final_name);
> +	if (ret < 0)
> +	{
> +		put_device(led_cdev->dev);
> +		return ret;
>  	}

[Severity: High]
Does this leave led_cdev->dev as a dangling pointer when kobject_set_name()
fails here, or when led_classdev_add() fails in led_classdev_register_ext()
below?

Previously, led_cdev->dev was assigned the return value of
device_create_with_groups(), so on any failure it remained either NULL or an
ERR_PTR(). Commit 1dbb9fb4082ce2 ("leds: Allow to call
led_classdev_unregister() unconditionally") added an IS_ERR_OR_NULL() check
to led_classdev_unregister() so drivers can call it unconditionally in their
error and removal paths:

drivers/leds/led-class.c:led_classdev_unregister() {
	if (IS_ERR_OR_NULL(led_cdev->dev))
		return;
...
	led_stop_software_blink(led_cdev);
...
	flush_work(&led_cdev->set_brightness_work);

	device_unregister(led_cdev->dev);

	down_write(&leds_list_lock);
	list_del(&led_cdev->node);
	up_write(&leds_list_lock);
...
}

Many drivers rely on this guarantee during error unwind or removal, such as
asus_wmi_led_init() -> asus_wmi_led_exit() in drivers/platform/x86/asus-wmi.c,
asus_led_init() -> asus_led_exit() in drivers/platform/x86/asus-laptop.c, and
samsung_leds_init() -> samsung_leds_exit() in
drivers/platform/x86/samsung-laptop.c.

With this change, put_device(led_cdev->dev) frees the device via
led_classdev_release(), but led_cdev->dev is not reset to NULL. A subsequent
call to led_classdev_unregister() will bypass the IS_ERR_OR_NULL(led_cdev->dev)
check, operate on uninitialized blink_timer and set_brightness_work structures
if kobject_set_name() failed before led_init_core(), trigger a use-after-free
and double-free in device_unregister(led_cdev->dev), and crash in
list_del(&led_cdev->node).

>  
>  	if (init_data && init_data->fwnode)
>  		device_set_node(led_cdev->dev, init_data->fwnode);

[Severity: High]
Could calling device_set_node() in led_classdev_init() before device_add() is
called in led_classdev_add() break fw_devlink for child LED nodes?

Previously, device_create_with_groups() called device_add() while
led_cdev->dev->fwnode was still NULL, and device_set_node() was only called
after device_add() returned. When the parent LED controller finished probing,
device_links_driver_bound() -> fw_devlink_pickup_dangling_consumers() marked
the child LED fwnodes with FWNODE_FLAG_NOT_DEVICE and attached consumer links
to the parent LED controller device.

Now that led_cdev->dev->fwnode is populated before led_classdev_add() calls
device_add(led_cdev->dev), this block in device_add() will trigger during the
parent driver's probe:

drivers/base/core.c:device_add() {
...
	if (dev->fwnode && !dev->fwnode->dev) {
		dev->fwnode->dev = dev;
		fw_devlink_link_device(dev);
	}
...
}

Because led_cdev->dev is a class device that never binds to a driver:

1. __fw_devlink_link_to_consumers(led_cdev->dev) converts consumer fwnode_links
   on init_data->fwnode into managed device links with led_cdev->dev as the
   supplier instead of the parent LED controller. During boot (while
   fw_devlink_drv_reg_done is false), consumers remain blocked with
   -EPROBE_DEFER until fw_devlink_drivers_done() runs, and are never linked to
   the parent controller for runtime PM, suspend/resume, or unbind ordering.

2. __fw_devlink_link_to_suppliers(led_cdev->dev, fwnode) creates managed device
   links with led_cdev->dev as the consumer for any supplier properties on the
   child LED node (such as gpios in gpio-leds or pwms in pwm-leds). Since
   led_cdev->dev never probes a driver, those links stay in DL_STATE_AVAILABLE
   and prevent the GPIO or PWM suppliers from reaching sync_state().

> -
> -	led_update_brightness(led_cdev);
> -
>  	led_cdev->wq = leds_wq;
>  
>  	led_init_core(led_cdev);
>  
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(led_classdev_init);
> +
> +/**
> + * led_classdev_add - Add a new LED class device to the subsystem.
> + * @led_cdev: the led_classdev structure for this device.
> + *
> + * Adds a new LED class device to the subsystem, allowing userspace applications
> + * and/or kernel consumers to access it.
> + *
> + * Returns: Zero on success or a negative error code on failure.
> + */
> +int led_classdev_add(struct led_classdev *led_cdev)
> +{
> +	int ret;
> +
> +	guard(mutex)(&led_cdev->led_access);
> +
> +	ret = device_add(led_cdev->dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	led_update_brightness(led_cdev);
> +
>  	/* add to the list of leds */
>  	down_write(&leds_list_lock);
>  	list_add_tail(&led_cdev->node, &leds_list);
> @@ -565,13 +607,36 @@ int led_classdev_register_ext(struct device *parent,
>  	led_trigger_set_default(led_cdev);
>  #endif
>  
> -	mutex_unlock(&led_cdev->led_access);
> -
> -	dev_dbg(parent, "Registered led device: %s\n",
> -			led_cdev->name);
> +	dev_dbg(led_cdev->dev->parent, "Registered led device: %s\n", led_cdev->name);
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(led_classdev_add);
> +
> +/**
> + * led_classdev_register_ext - register a new object of led_classdev class
> + *                             with init data.
> + *
> + * @parent: parent of LED device.
> + * @led_cdev: the led_classdev structure for this device.
> + * @init_data: LED class device initialization data.
> + */
> +int led_classdev_register_ext(struct device *parent,
> +                              struct led_classdev *led_cdev,
> +                              struct led_init_data *init_data)
> +{
> +	int ret;
> +
> +	ret = led_classdev_init(parent, led_cdev, init_data);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = led_classdev_add(led_cdev);
> +	if (ret < 0)
> +		put_device(led_cdev->dev);
> +
> +	return ret;
> +}
>  EXPORT_SYMBOL_GPL(led_classdev_register_ext);

[ ... ]

>
> -- 
> 2.51.2
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018135339.1109271-1-W_Armin@gmx.de?part=8

Re: [PATCH 08/10] leds: class: Split led_classdev_register_ext()

[Armin Wolf]

Am 18.10.26 um 16:15 schrieb sashiko-bot@kernel.org:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] leds: dangling led_cdev->dev pointer on error in led_classdev_init()
> - [High] leds: premature device_set_node() in led_classdev_init() breaks fw_devlink
> --
>
> commit 7efb333e9420ea13e705d9a114384e841e92d2b2
> Author: Armin Wolf <W_Armin@gmx.de>
>
> leds: class: Split led_classdev_register_ext()
>
> This commit splits led_classdev_register_ext() into led_classdev_init() and
> led_classdev_add() so that LED class devices can be initialized before
> being added to the subsystem.
>
>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>> index 77a0fce039528..ace417a2388e0 100644
>> --- a/drivers/leds/led-class.c
>> +++ b/drivers/leds/led-class.c
> [ ... ]
>
>> @@ -539,23 +550,54 @@ int led_classdev_register_ext(struct device *parent,
>>   	if (!led_cdev->max_brightness)
>>   		led_cdev->max_brightness = LED_FULL;
>>   
>> -	mutex_lock(&led_cdev->led_access);
>> -	led_cdev->dev = device_create_with_groups(&leds_class, parent, 0, led_cdev,
>> -						  led_cdev->groups, "%s", final_name);
>> -	if (IS_ERR(led_cdev->dev)) {
>> -		mutex_unlock(&led_cdev->led_access);
>> -		return PTR_ERR(led_cdev->dev);
>> +	led_cdev->dev = kzalloc_obj(*led_cdev->dev);
>> +	if (!led_cdev->dev)
>> +		return -ENOMEM;
>> +
>> +	device_initialize(led_cdev->dev);
>> +	led_cdev->dev->class = &leds_class;
>> +	led_cdev->dev->parent = parent;
>> +	led_cdev->dev->groups = led_cdev->groups;
>> +	dev_set_drvdata(led_cdev->dev, led_cdev);
>> +	ret = kobject_set_name(&led_cdev->dev->kobj, "%s", final_name);
>> +	if (ret < 0)
>> +	{
>> +		put_device(led_cdev->dev);
>> +		return ret;
>>   	}
> [Severity: High]
> Does this leave led_cdev->dev as a dangling pointer when kobject_set_name()
> fails here, or when led_classdev_add() fails in led_classdev_register_ext()
> below?
>
> Previously, led_cdev->dev was assigned the return value of
> device_create_with_groups(), so on any failure it remained either NULL or an
> ERR_PTR(). Commit 1dbb9fb4082ce2 ("leds: Allow to call
> led_classdev_unregister() unconditionally") added an IS_ERR_OR_NULL() check
> to led_classdev_unregister() so drivers can call it unconditionally in their
> error and removal paths:
>
> drivers/leds/led-class.c:led_classdev_unregister() {
> 	if (IS_ERR_OR_NULL(led_cdev->dev))
> 		return;
> ...
> 	led_stop_software_blink(led_cdev);
> ...
> 	flush_work(&led_cdev->set_brightness_work);
>
> 	device_unregister(led_cdev->dev);
>
> 	down_write(&leds_list_lock);
> 	list_del(&led_cdev->node);
> 	up_write(&leds_list_lock);
> ...
> }
>
> Many drivers rely on this guarantee during error unwind or removal, such as
> asus_wmi_led_init() -> asus_wmi_led_exit() in drivers/platform/x86/asus-wmi.c,
> asus_led_init() -> asus_led_exit() in drivers/platform/x86/asus-laptop.c, and
> samsung_leds_init() -> samsung_leds_exit() in
> drivers/platform/x86/samsung-laptop.c.
>
> With this change, put_device(led_cdev->dev) frees the device via
> led_classdev_release(), but led_cdev->dev is not reset to NULL. A subsequent
> call to led_classdev_unregister() will bypass the IS_ERR_OR_NULL(led_cdev->dev)
> check, operate on uninitialized blink_timer and set_brightness_work structures
> if kobject_set_name() failed before led_init_core(), trigger a use-after-free
> and double-free in device_unregister(led_cdev->dev), and crash in
> list_del(&led_cdev->node).
>
>>   
>>   	if (init_data && init_data->fwnode)
>>   		device_set_node(led_cdev->dev, init_data->fwnode);
> [Severity: High]
> Could calling device_set_node() in led_classdev_init() before device_add() is
> called in led_classdev_add() break fw_devlink for child LED nodes?
>
> Previously, device_create_with_groups() called device_add() while
> led_cdev->dev->fwnode was still NULL, and device_set_node() was only called
> after device_add() returned. When the parent LED controller finished probing,
> device_links_driver_bound() -> fw_devlink_pickup_dangling_consumers() marked
> the child LED fwnodes with FWNODE_FLAG_NOT_DEVICE and attached consumer links
> to the parent LED controller device.
>
> Now that led_cdev->dev->fwnode is populated before led_classdev_add() calls
> device_add(led_cdev->dev), this block in device_add() will trigger during the
> parent driver's probe:
>
> drivers/base/core.c:device_add() {
> ...
> 	if (dev->fwnode && !dev->fwnode->dev) {
> 		dev->fwnode->dev = dev;
> 		fw_devlink_link_device(dev);
> 	}
> ...
> }
>
> Because led_cdev->dev is a class device that never binds to a driver:
>
> 1. __fw_devlink_link_to_consumers(led_cdev->dev) converts consumer fwnode_links
>     on init_data->fwnode into managed device links with led_cdev->dev as the
>     supplier instead of the parent LED controller. During boot (while
>     fw_devlink_drv_reg_done is false), consumers remain blocked with
>     -EPROBE_DEFER until fw_devlink_drivers_done() runs, and are never linked to
>     the parent controller for runtime PM, suspend/resume, or unbind ordering.
>
> 2. __fw_devlink_link_to_suppliers(led_cdev->dev, fwnode) creates managed device
>     links with led_cdev->dev as the consumer for any supplier properties on the
>     child LED node (such as gpios in gpio-leds or pwms in pwm-leds). Since
>     led_cdev->dev never probes a driver, those links stay in DL_STATE_AVAILABLE
>     and prevent the GPIO or PWM suppliers from reaching sync_state().

I have no experience with devicetree, so i cannot verify if this is true, why do we call
device_set_node() in the first place after having already registered the device?

Thanks,
Armin Wolf

>> -
>> -	led_update_brightness(led_cdev);
>> -
>>   	led_cdev->wq = leds_wq;
>>   
>>   	led_init_core(led_cdev);
>>   
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(led_classdev_init);
>> +
>> +/**
>> + * led_classdev_add - Add a new LED class device to the subsystem.
>> + * @led_cdev: the led_classdev structure for this device.
>> + *
>> + * Adds a new LED class device to the subsystem, allowing userspace applications
>> + * and/or kernel consumers to access it.
>> + *
>> + * Returns: Zero on success or a negative error code on failure.
>> + */
>> +int led_classdev_add(struct led_classdev *led_cdev)
>> +{
>> +	int ret;
>> +
>> +	guard(mutex)(&led_cdev->led_access);
>> +
>> +	ret = device_add(led_cdev->dev);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	led_update_brightness(led_cdev);
>> +
>>   	/* add to the list of leds */
>>   	down_write(&leds_list_lock);
>>   	list_add_tail(&led_cdev->node, &leds_list);
>> @@ -565,13 +607,36 @@ int led_classdev_register_ext(struct device *parent,
>>   	led_trigger_set_default(led_cdev);
>>   #endif
>>   
>> -	mutex_unlock(&led_cdev->led_access);
>> -
>> -	dev_dbg(parent, "Registered led device: %s\n",
>> -			led_cdev->name);
>> +	dev_dbg(led_cdev->dev->parent, "Registered led device: %s\n", led_cdev->name);
>>   
>>   	return 0;
>>   }
>> +EXPORT_SYMBOL_GPL(led_classdev_add);
>> +
>> +/**
>> + * led_classdev_register_ext - register a new object of led_classdev class
>> + *                             with init data.
>> + *
>> + * @parent: parent of LED device.
>> + * @led_cdev: the led_classdev structure for this device.
>> + * @init_data: LED class device initialization data.
>> + */
>> +int led_classdev_register_ext(struct device *parent,
>> +                              struct led_classdev *led_cdev,
>> +                              struct led_init_data *init_data)
>> +{
>> +	int ret;
>> +
>> +	ret = led_classdev_init(parent, led_cdev, init_data);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = led_classdev_add(led_cdev);
>> +	if (ret < 0)
>> +		put_device(led_cdev->dev);
>> +
>> +	return ret;
>> +}
>>   EXPORT_SYMBOL_GPL(led_classdev_register_ext);
> [ ... ]
>
>> -- 
>> 2.51.2
>>

[PATCH 09/10] leds: Support custom sysfs groups on multicolor and flash LEDs

[Armin Wolf]

Drivers registering multicolor/flash LEDs currently cannot use
led_cdev->groups due to said field being overwritten by the
multicolor/flash LED registration functions. Fix this by assigning
each LED type a seperate device type ("multicolor" and "flash")
that carries the sysfs attributes instead.

Also assign a device type for standard LEDs ("standard") for symmetry.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/leds/led-class-flash.c      | 108 ++++++++++++++--------------
 drivers/leds/led-class-multicolor.c |  18 ++++-
 drivers/leds/led-class.c            |   5 ++
 include/linux/led-class-flash.h     |   5 --
 4 files changed, 73 insertions(+), 63 deletions(-)

diff --git a/drivers/leds/led-class-flash.c b/drivers/leds/led-class-flash.c
index 953265fddb7d..e023511cea1c 100644
--- a/drivers/leds/led-class-flash.c
+++ b/drivers/leds/led-class-flash.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
+#include "leds.h"
 
 #define has_flash_op(fled_cdev, op)				\
 	(fled_cdev && fled_cdev->ops->op)
@@ -207,42 +208,56 @@ static ssize_t flash_fault_show(struct device *dev, const struct device_attribut
 }
 static const DEVICE_ATTR_RO(flash_fault);
 
-static const struct attribute *const led_flash_strobe_attrs[] = {
+static const struct attribute *const led_flash_attrs[] = {
 	&dev_attr_flash_strobe.attr,
-	NULL,
-};
-
-static const struct attribute *const led_flash_timeout_attrs[] = {
 	&dev_attr_flash_timeout.attr,
 	&dev_attr_max_flash_timeout.attr,
-	NULL,
-};
-
-static const struct attribute *const led_flash_brightness_attrs[] = {
 	&dev_attr_flash_brightness.attr,
 	&dev_attr_max_flash_brightness.attr,
-	NULL,
-};
-
-static const struct attribute *const led_flash_fault_attrs[] = {
 	&dev_attr_flash_fault.attr,
 	NULL,
 };
 
-static const struct attribute_group led_flash_strobe_group = {
-	.attrs_const = led_flash_strobe_attrs,
-};
+static umode_t led_flash_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);
+	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
+
+	if (attr == &dev_attr_flash_strobe.attr)
+		return attr->mode;
+
+        if (attr == &dev_attr_flash_timeout.attr && fled_cdev->ops->timeout_set)
+                return attr->mode;
 
-static const struct attribute_group led_flash_timeout_group = {
-	.attrs_const = led_flash_timeout_attrs,
+	if (attr == &dev_attr_max_flash_timeout.attr && fled_cdev->ops->timeout_set)
+		return attr->mode;
+
+	if (attr == &dev_attr_flash_brightness.attr && fled_cdev->ops->flash_brightness_set)
+		return attr->mode;
+
+	if (attr == &dev_attr_max_flash_brightness.attr && fled_cdev->ops->flash_brightness_set)
+		return attr->mode;
+
+	if (attr == &dev_attr_flash_fault.attr && fled_cdev->ops->fault_get)
+		return attr->mode;
+
+        return 0;
+}
+
+static const struct attribute_group led_flash_group = {
+	.is_visible_const = led_flash_is_visible,
+	.attrs_const = led_flash_attrs,
 };
 
-static const struct attribute_group led_flash_brightness_group = {
-	.attrs_const = led_flash_brightness_attrs,
+static const struct attribute_group *led_flash_groups[] = {
+	&led_flash_group,
+	NULL,
 };
 
-static const struct attribute_group led_flash_fault_group = {
-	.attrs_const = led_flash_fault_attrs,
+static const struct device_type led_flash_device_type = {
+	.name = "flash",
+	.groups = led_flash_groups,
 };
 
 static void led_flash_resume(struct led_classdev *led_cdev)
@@ -254,28 +269,6 @@ static void led_flash_resume(struct led_classdev *led_cdev)
 	call_flash_op(fled_cdev, timeout_set, fled_cdev->timeout.val);
 }
 
-static void led_flash_init_sysfs_groups(struct led_classdev_flash *fled_cdev)
-{
-	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
-	const struct led_flash_ops *ops = fled_cdev->ops;
-	const struct attribute_group **flash_groups = fled_cdev->sysfs_groups;
-
-	int num_sysfs_groups = 0;
-
-	flash_groups[num_sysfs_groups++] = &led_flash_strobe_group;
-
-	if (ops->flash_brightness_set)
-		flash_groups[num_sysfs_groups++] = &led_flash_brightness_group;
-
-	if (ops->timeout_set)
-		flash_groups[num_sysfs_groups++] = &led_flash_timeout_group;
-
-	if (ops->fault_get)
-		flash_groups[num_sysfs_groups++] = &led_flash_fault_group;
-
-	led_cdev->groups = flash_groups;
-}
-
 int led_classdev_flash_register_ext(struct device *parent,
 				    struct led_classdev_flash *fled_cdev,
 				    struct led_init_data *init_data)
@@ -289,26 +282,29 @@ int led_classdev_flash_register_ext(struct device *parent,
 
 	led_cdev = &fled_cdev->led_cdev;
 
-	if (led_cdev->flags & LED_DEV_CAP_FLASH) {
-		if (!led_cdev->brightness_set_blocking)
-			return -EINVAL;
+	if (!(led_cdev->flags & LED_DEV_CAP_FLASH))
+		return led_classdev_register_ext(parent, led_cdev, init_data);
 
-		ops = fled_cdev->ops;
-		if (!ops || !ops->strobe_set)
-			return -EINVAL;
+	if (!led_cdev->brightness_set_blocking)
+		return -EINVAL;
 
-		led_cdev->flash_resume = led_flash_resume;
+	ops = fled_cdev->ops;
+	if (!ops || !ops->strobe_set)
+		return -EINVAL;
 
-		/* Select the sysfs attributes to be created for the device */
-		led_flash_init_sysfs_groups(fled_cdev);
-	}
+	led_cdev->flash_resume = led_flash_resume;
 
 	/* Register led class device */
-	ret = led_classdev_register_ext(parent, led_cdev, init_data);
+	ret = led_classdev_init(parent, led_cdev, init_data);
 	if (ret < 0)
 		return ret;
 
-	return 0;
+	led_cdev->dev->type = &led_flash_device_type;
+	ret = led_classdev_add(led_cdev);
+	if (ret < 0)
+		put_device(led_cdev->dev);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(led_classdev_flash_register_ext);
 
diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
index 57df0b2e785b..9b8c37f7bdf4 100644
--- a/drivers/leds/led-class-multicolor.c
+++ b/drivers/leds/led-class-multicolor.c
@@ -12,6 +12,7 @@
 #include <linux/slab.h>
 #include <linux/sysfs.h>
 #include <linux/uaccess.h>
+#include "leds.h"
 
 static unsigned int led_mc_get_max_intensity(struct led_classdev_mc *mcled_cdev, size_t index)
 {
@@ -159,11 +160,17 @@ static const struct attribute *const led_multicolor_attrs[] = {
 };
 ATTRIBUTE_GROUPS(led_multicolor);
 
+static const struct device_type led_multicolor_device_type = {
+	.name = "multicolor",
+	.groups = led_multicolor_groups,
+};
+
 int led_classdev_multicolor_register_ext(struct device *parent,
 				     struct led_classdev_mc *mcled_cdev,
 				     struct led_init_data *init_data)
 {
 	struct led_classdev *led_cdev;
+	int ret;
 
 	if (!mcled_cdev)
 		return -EINVAL;
@@ -176,9 +183,16 @@ int led_classdev_multicolor_register_ext(struct device *parent,
 
 	led_cdev = &mcled_cdev->led_cdev;
 	led_cdev->flags |= LED_MULTI_COLOR;
-	mcled_cdev->led_cdev.groups = led_multicolor_groups;
+	ret = led_classdev_init(parent, led_cdev, init_data);
+	if (ret < 0)
+		return ret;
+
+	led_cdev->dev->type = &led_multicolor_device_type;
+	ret = led_classdev_add(led_cdev);
+	if (ret < 0)
+		put_device(led_cdev->dev);
 
-	return led_classdev_register_ext(parent, led_cdev, init_data);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(led_classdev_multicolor_register_ext);
 
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index ace417a2388e..49b321b3d863 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -613,6 +613,10 @@ int led_classdev_add(struct led_classdev *led_cdev)
 }
 EXPORT_SYMBOL_GPL(led_classdev_add);
 
+static const struct device_type led_device_type = {
+	.name = "standard",
+};
+
 /**
  * led_classdev_register_ext - register a new object of led_classdev class
  *                             with init data.
@@ -631,6 +635,7 @@ int led_classdev_register_ext(struct device *parent,
 	if (ret < 0)
 		return ret;
 
+	led_cdev->dev->type = &led_device_type;
 	ret = led_classdev_add(led_cdev);
 	if (ret < 0)
 		put_device(led_cdev->dev);
diff --git a/include/linux/led-class-flash.h b/include/linux/led-class-flash.h
index 775a96217518..5204ef60dc4b 100644
--- a/include/linux/led-class-flash.h
+++ b/include/linux/led-class-flash.h
@@ -28,8 +28,6 @@ struct led_classdev_flash;
 #define LED_FAULT_LED_OVER_TEMPERATURE	(1 << 8)
 #define LED_NUM_FLASH_FAULTS		9
 
-#define LED_FLASH_SYSFS_GROUPS_SIZE	5
-
 struct led_flash_ops {
 	/* set flash brightness */
 	int (*flash_brightness_set)(struct led_classdev_flash *fled_cdev,
@@ -79,9 +77,6 @@ struct led_classdev_flash {
 
 	/* flash timeout value in microseconds along with its constraints */
 	struct led_flash_setting duration;
-
-	/* LED Flash class sysfs groups */
-	const struct attribute_group *sysfs_groups[LED_FLASH_SYSFS_GROUPS_SIZE];
 };
 
 static inline struct led_classdev_flash *lcdev_to_flcdev(
-- 
2.56.0

Re: [PATCH 09/10] leds: Support custom sysfs groups on multicolor and flash LEDs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] leds: dangling led_cdev->dev pointer on registration failure causes UAF
--

commit fc841d87d7344c0d4e24b361394ed5d20afd7ffb
Author: Armin Wolf <W_Armin@gmx.de>

leds: Support custom sysfs groups on multicolor and flash LEDs

This commit moves the multicolor and flash LED sysfs attribute groups onto
dedicated device types ("multicolor", "flash", and "standard") so that drivers
can populate led_cdev->groups without having them overwritten during
registration.

> diff --git a/drivers/leds/led-class-flash.c b/drivers/leds/led-class-flash.c
> index 953265fddb7d9..e023511cea1c1 100644
> --- a/drivers/leds/led-class-flash.c
> +++ b/drivers/leds/led-class-flash.c

[ ... ]

> @@ -289,26 +282,29 @@ int led_classdev_flash_register_ext(struct device *parent,
>  
>  	led_cdev = &fled_cdev->led_cdev;
>  
> -	if (led_cdev->flags & LED_DEV_CAP_FLASH) {
> -		if (!led_cdev->brightness_set_blocking)
> -			return -EINVAL;
> +	if (!(led_cdev->flags & LED_DEV_CAP_FLASH))
> +		return led_classdev_register_ext(parent, led_cdev, init_data);
>  
> -		ops = fled_cdev->ops;
> -		if (!ops || !ops->strobe_set)
> -			return -EINVAL;
> +	if (!led_cdev->brightness_set_blocking)
> +		return -EINVAL;
>  
> -		led_cdev->flash_resume = led_flash_resume;
> +	ops = fled_cdev->ops;
> +	if (!ops || !ops->strobe_set)
> +		return -EINVAL;
>  
> -		/* Select the sysfs attributes to be created for the device */
> -		led_flash_init_sysfs_groups(fled_cdev);
> -	}
> +	led_cdev->flash_resume = led_flash_resume;
>  
>  	/* Register led class device */
> -	ret = led_classdev_register_ext(parent, led_cdev, init_data);
> +	ret = led_classdev_init(parent, led_cdev, init_data);
>  	if (ret < 0)
>  		return ret;
>  
> -	return 0;
> +	led_cdev->dev->type = &led_flash_device_type;
> +	ret = led_classdev_add(led_cdev);
> +	if (ret < 0)
> +		put_device(led_cdev->dev);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(led_classdev_flash_register_ext);
>  
> diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
> index 57df0b2e785b5..9b8c37f7bdf45 100644
> --- a/drivers/leds/led-class-multicolor.c
> +++ b/drivers/leds/led-class-multicolor.c

[ ... ]

> @@ -176,9 +183,16 @@ int led_classdev_multicolor_register_ext(struct device *parent,
>  
>  	led_cdev = &mcled_cdev->led_cdev;
>  	led_cdev->flags |= LED_MULTI_COLOR;
> -	mcled_cdev->led_cdev.groups = led_multicolor_groups;
> +	ret = led_classdev_init(parent, led_cdev, init_data);
> +	if (ret < 0)
> +		return ret;
> +
> +	led_cdev->dev->type = &led_multicolor_device_type;
> +	ret = led_classdev_add(led_cdev);
> +	if (ret < 0)
> +		put_device(led_cdev->dev);
>  
> -	return led_classdev_register_ext(parent, led_cdev, init_data);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(led_classdev_multicolor_register_ext);

[Severity: High]
Could this leave led_cdev->dev as a dangling pointer when registration fails,
leading to a use-after-free in led_classdev_unregister()?

In led_classdev_init(), led_cdev->dev is allocated and initialized via
device_initialize():

drivers/leds/led-class.c:led_classdev_init() {
	...
	led_cdev->dev = kzalloc_obj(*led_cdev->dev);
	if (!led_cdev->dev)
		return -ENOMEM;

	device_initialize(led_cdev->dev);
	...
	ret = kobject_set_name(&led_cdev->dev->kobj, "%s", final_name);
	if (ret < 0)
	{
		put_device(led_cdev->dev);
		return ret;
	}
	...
}

If kobject_set_name() fails in led_classdev_init(), or if led_classdev_add()
subsequently fails (for example when device_add() fails due to a sysfs group
conflict or -ENOMEM) in led_classdev_flash_register_ext(),
led_classdev_multicolor_register_ext(), or led_classdev_register_ext(),
put_device(led_cdev->dev) releases the final reference and frees the struct
device via led_classdev_release(), but led_cdev->dev is not reset to NULL and
led_cdev->node is never added to leds_list.

Since commit 1dbb9fb4082ce2 ("leds: Allow to call led_classdev_unregister()
unconditionally"), led_classdev_unregister() permits unconditional teardown by
checking:

drivers/leds/led-class.c:led_classdev_unregister() {
	if (IS_ERR_OR_NULL(led_cdev->dev))
		return;
	...
	device_unregister(led_cdev->dev);
	...
	down_write(&leds_list_lock);
	list_del(&led_cdev->node);
	up_write(&leds_list_lock);
	...
}

Several drivers rely on this contract when cleaning up after failed
registration or on driver removal, such as drivers/leds/leds-blinkm.c:

drivers/leds/leds-blinkm.c:register_multicolor() {
	...
	err = led_classdev_multicolor_register(&client->dev, &mc_led->cdev.mcled_cdev);
	if (err < 0) {
		dev_err(&client->dev, "couldn't register LED %s\n",
				mc_led->cdev.led_cdev.name);
		sysfs_remove_group(&client->dev.kobj, &blinkm_group);
	}
	return 0;
}

as well as drivers/platform/x86/asus-wmi.c (where asus_wmi_led_init() jumps to
asus_wmi_led_exit() on error), drivers/platform/x86/asus-laptop.c, and
drivers/platform/x86/toshiba_acpi.c.

Because led_cdev->dev still holds a non-NULL, non-ERR_PTR address after
put_device(led_cdev->dev), wouldn't a subsequent call to
led_classdev_unregister() (or its flash and multicolor wrappers) bypass the
IS_ERR_OR_NULL(led_cdev->dev) check, pass the freed pointer to
device_unregister(led_cdev->dev), and dereference NULL in
list_del(&led_cdev->node)?

Should led_cdev->dev be set to NULL after put_device(led_cdev->dev) in the
error paths of led_classdev_init(), led_classdev_register_ext(),
led_classdev_flash_register_ext(), and led_classdev_multicolor_register_ext()?

> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index ace417a2388e0..49b321b3d863b 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -613,6 +613,10 @@ int led_classdev_add(struct led_classdev *led_cdev)
>  }
>  EXPORT_SYMBOL_GPL(led_classdev_add);
>  
> +static const struct device_type led_device_type = {
> +	.name = "standard",
> +};
> +
>  /**
>   * led_classdev_register_ext - register a new object of led_classdev class
>   *                             with init data.
> @@ -631,6 +635,7 @@ int led_classdev_register_ext(struct device *parent,
>  	if (ret < 0)
>  		return ret;
>  
> +	led_cdev->dev->type = &led_device_type;
>  	ret = led_classdev_add(led_cdev);
>  	if (ret < 0)
>  		put_device(led_cdev->dev);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018135339.1109271-1-W_Armin@gmx.de?part=9

Re: [PATCH 09/10] leds: Support custom sysfs groups on multicolor and flash LEDs

[Rong Zhang]

Hi Armin,

Nice refactor!

On Sun, 2026-10-18 at 15:46 +0200, Armin Wolf wrote:
> Drivers registering multicolor/flash LEDs currently cannot use
> led_cdev->groups due to said field being overwritten by the
> multicolor/flash LED registration functions. Fix this by assigning
> each LED type a seperate device type ("multicolor" and "flash")
> that carries the sysfs attributes instead.
> 
> Also assign a device type for standard LEDs ("standard") for symmetry.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/leds/led-class-flash.c      | 108 ++++++++++++++--------------
>  drivers/leds/led-class-multicolor.c |  18 ++++-
>  drivers/leds/led-class.c            |   5 ++
>  include/linux/led-class-flash.h     |   5 --
>  4 files changed, 73 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/leds/led-class-flash.c b/drivers/leds/led-class-flash.c
> index 953265fddb7d..e023511cea1c 100644
> --- a/drivers/leds/led-class-flash.c
> +++ b/drivers/leds/led-class-flash.c
> @@ -13,6 +13,7 @@
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/sysfs.h>
> +#include "leds.h"
>  
>  #define has_flash_op(fled_cdev, op)				\
>  	(fled_cdev && fled_cdev->ops->op)
> @@ -207,42 +208,56 @@ static ssize_t flash_fault_show(struct device *dev, const struct device_attribut
>  }
>  static const DEVICE_ATTR_RO(flash_fault);
>  
> -static const struct attribute *const led_flash_strobe_attrs[] = {
> +static const struct attribute *const led_flash_attrs[] = {
>  	&dev_attr_flash_strobe.attr,
> -	NULL,
> -};
> -
> -static const struct attribute *const led_flash_timeout_attrs[] = {
>  	&dev_attr_flash_timeout.attr,
>  	&dev_attr_max_flash_timeout.attr,
> -	NULL,
> -};
> -
> -static const struct attribute *const led_flash_brightness_attrs[] = {
>  	&dev_attr_flash_brightness.attr,
>  	&dev_attr_max_flash_brightness.attr,
> -	NULL,
> -};
> -
> -static const struct attribute *const led_flash_fault_attrs[] = {
>  	&dev_attr_flash_fault.attr,
>  	NULL,
>  };
>  
> -static const struct attribute_group led_flash_strobe_group = {
> -	.attrs_const = led_flash_strobe_attrs,
> -};
> +static umode_t led_flash_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);
> +	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
> +
> +	if (attr == &dev_attr_flash_strobe.attr)
> +		return attr->mode;
> +
> +        if (attr == &dev_attr_flash_timeout.attr && fled_cdev->ops->timeout_set)
> +                return attr->mode;
>  
> -static const struct attribute_group led_flash_timeout_group = {
> -	.attrs_const = led_flash_timeout_attrs,
> +	if (attr == &dev_attr_max_flash_timeout.attr && fled_cdev->ops->timeout_set)
> +		return attr->mode;
> +
> +	if (attr == &dev_attr_flash_brightness.attr && fled_cdev->ops->flash_brightness_set)
> +		return attr->mode;
> +
> +	if (attr == &dev_attr_max_flash_brightness.attr && fled_cdev->ops->flash_brightness_set)
> +		return attr->mode;
> +
> +	if (attr == &dev_attr_flash_fault.attr && fled_cdev->ops->fault_get)
> +		return attr->mode;
> +
> +        return 0;
> +}
> +
> +static const struct attribute_group led_flash_group = {
> +	.is_visible_const = led_flash_is_visible,
> +	.attrs_const = led_flash_attrs,
>  };
>  
> -static const struct attribute_group led_flash_brightness_group = {
> -	.attrs_const = led_flash_brightness_attrs,
> +static const struct attribute_group *led_flash_groups[] = {
> +	&led_flash_group,
> +	NULL,
>  };
>  
> -static const struct attribute_group led_flash_fault_group = {
> -	.attrs_const = led_flash_fault_attrs,

Hmm, a bunch of back-and-forth changes. I suppose the is_visible_const
changes can be merged into PATCH 3, right?

Thanks,
Rong

> +static const struct device_type led_flash_device_type = {
> +	.name = "flash",
> +	.groups = led_flash_groups,
>  };
>  
>  static void led_flash_resume(struct led_classdev *led_cdev)
> @@ -254,28 +269,6 @@ static void led_flash_resume(struct led_classdev *led_cdev)
>  	call_flash_op(fled_cdev, timeout_set, fled_cdev->timeout.val);
>  }
>  
> -static void led_flash_init_sysfs_groups(struct led_classdev_flash *fled_cdev)
> -{
> -	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> -	const struct led_flash_ops *ops = fled_cdev->ops;
> -	const struct attribute_group **flash_groups = fled_cdev->sysfs_groups;
> -
> -	int num_sysfs_groups = 0;
> -
> -	flash_groups[num_sysfs_groups++] = &led_flash_strobe_group;
> -
> -	if (ops->flash_brightness_set)
> -		flash_groups[num_sysfs_groups++] = &led_flash_brightness_group;
> -
> -	if (ops->timeout_set)
> -		flash_groups[num_sysfs_groups++] = &led_flash_timeout_group;
> -
> -	if (ops->fault_get)
> -		flash_groups[num_sysfs_groups++] = &led_flash_fault_group;
> -
> -	led_cdev->groups = flash_groups;
> -}
> -
>  int led_classdev_flash_register_ext(struct device *parent,
>  				    struct led_classdev_flash *fled_cdev,
>  				    struct led_init_data *init_data)
> @@ -289,26 +282,29 @@ int led_classdev_flash_register_ext(struct device *parent,
>  
>  	led_cdev = &fled_cdev->led_cdev;
>  
> -	if (led_cdev->flags & LED_DEV_CAP_FLASH) {
> -		if (!led_cdev->brightness_set_blocking)
> -			return -EINVAL;
> +	if (!(led_cdev->flags & LED_DEV_CAP_FLASH))
> +		return led_classdev_register_ext(parent, led_cdev, init_data);
>  
> -		ops = fled_cdev->ops;
> -		if (!ops || !ops->strobe_set)
> -			return -EINVAL;
> +	if (!led_cdev->brightness_set_blocking)
> +		return -EINVAL;
>  
> -		led_cdev->flash_resume = led_flash_resume;
> +	ops = fled_cdev->ops;
> +	if (!ops || !ops->strobe_set)
> +		return -EINVAL;
>  
> -		/* Select the sysfs attributes to be created for the device */
> -		led_flash_init_sysfs_groups(fled_cdev);
> -	}
> +	led_cdev->flash_resume = led_flash_resume;
>  
>  	/* Register led class device */
> -	ret = led_classdev_register_ext(parent, led_cdev, init_data);
> +	ret = led_classdev_init(parent, led_cdev, init_data);
>  	if (ret < 0)
>  		return ret;
>  
> -	return 0;
> +	led_cdev->dev->type = &led_flash_device_type;
> +	ret = led_classdev_add(led_cdev);
> +	if (ret < 0)
> +		put_device(led_cdev->dev);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(led_classdev_flash_register_ext);
>  
> diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
> index 57df0b2e785b..9b8c37f7bdf4 100644
> --- a/drivers/leds/led-class-multicolor.c
> +++ b/drivers/leds/led-class-multicolor.c
> @@ -12,6 +12,7 @@
>  #include <linux/slab.h>
>  #include <linux/sysfs.h>
>  #include <linux/uaccess.h>
> +#include "leds.h"
>  
>  static unsigned int led_mc_get_max_intensity(struct led_classdev_mc *mcled_cdev, size_t index)
>  {
> @@ -159,11 +160,17 @@ static const struct attribute *const led_multicolor_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(led_multicolor);
>  
> +static const struct device_type led_multicolor_device_type = {
> +	.name = "multicolor",
> +	.groups = led_multicolor_groups,
> +};
> +
>  int led_classdev_multicolor_register_ext(struct device *parent,
>  				     struct led_classdev_mc *mcled_cdev,
>  				     struct led_init_data *init_data)
>  {
>  	struct led_classdev *led_cdev;
> +	int ret;
>  
>  	if (!mcled_cdev)
>  		return -EINVAL;
> @@ -176,9 +183,16 @@ int led_classdev_multicolor_register_ext(struct device *parent,
>  
>  	led_cdev = &mcled_cdev->led_cdev;
>  	led_cdev->flags |= LED_MULTI_COLOR;
> -	mcled_cdev->led_cdev.groups = led_multicolor_groups;
> +	ret = led_classdev_init(parent, led_cdev, init_data);
> +	if (ret < 0)
> +		return ret;
> +
> +	led_cdev->dev->type = &led_multicolor_device_type;
> +	ret = led_classdev_add(led_cdev);
> +	if (ret < 0)
> +		put_device(led_cdev->dev);
>  
> -	return led_classdev_register_ext(parent, led_cdev, init_data);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(led_classdev_multicolor_register_ext);
>  
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index ace417a2388e..49b321b3d863 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -613,6 +613,10 @@ int led_classdev_add(struct led_classdev *led_cdev)
>  }
>  EXPORT_SYMBOL_GPL(led_classdev_add);
>  
> +static const struct device_type led_device_type = {
> +	.name = "standard",
> +};
> +
>  /**
>   * led_classdev_register_ext - register a new object of led_classdev class
>   *                             with init data.
> @@ -631,6 +635,7 @@ int led_classdev_register_ext(struct device *parent,
>  	if (ret < 0)
>  		return ret;
>  
> +	led_cdev->dev->type = &led_device_type;
>  	ret = led_classdev_add(led_cdev);
>  	if (ret < 0)
>  		put_device(led_cdev->dev);
> diff --git a/include/linux/led-class-flash.h b/include/linux/led-class-flash.h
> index 775a96217518..5204ef60dc4b 100644
> --- a/include/linux/led-class-flash.h
> +++ b/include/linux/led-class-flash.h
> @@ -28,8 +28,6 @@ struct led_classdev_flash;
>  #define LED_FAULT_LED_OVER_TEMPERATURE	(1 << 8)
>  #define LED_NUM_FLASH_FAULTS		9
>  
> -#define LED_FLASH_SYSFS_GROUPS_SIZE	5
> -
>  struct led_flash_ops {
>  	/* set flash brightness */
>  	int (*flash_brightness_set)(struct led_classdev_flash *fled_cdev,
> @@ -79,9 +77,6 @@ struct led_classdev_flash {
>  
>  	/* flash timeout value in microseconds along with its constraints */
>  	struct led_flash_setting duration;
> -
> -	/* LED Flash class sysfs groups */
> -	const struct attribute_group *sysfs_groups[LED_FLASH_SYSFS_GROUPS_SIZE];
>  };
>  
>  static inline struct led_classdev_flash *lcdev_to_flcdev(

[PATCH 10/10] leds: multicolor: Adjust maximum subled intensity during initialization

[Armin Wolf]

The maximum brightness of a LED device is fixed after having called
led_classdev_init(), so performing the adjustment during runtime is
no longer necessary.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/leds/led-class-multicolor.c | 40 ++++++++++++-----------------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
index 9b8c37f7bdf4..875c615c33fc 100644
--- a/drivers/leds/led-class-multicolor.c
+++ b/drivers/leds/led-class-multicolor.c
@@ -14,24 +14,6 @@
 #include <linux/uaccess.h>
 #include "leds.h"
 
-static unsigned int led_mc_get_max_intensity(struct led_classdev_mc *mcled_cdev, size_t index)
-{
-	unsigned int max_intensity;
-
-	/*
-	 * The maximum global brightness value might still be changed by
-	 * led_classdev_register_ext() using devicetree properties. This
-	 * prevents us from changing subled_info[X].max_intensity when
-	 * registering a multicolor LED class device, so we have to do
-	 * this during runtime.
-	 */
-	max_intensity = mcled_cdev->subled_info[index].max_intensity;
-	if (max_intensity)
-		return max_intensity;
-
-	return mcled_cdev->led_cdev.max_brightness;
-}
-
 int led_mc_calc_color_components(struct led_classdev_mc *mcled_cdev,
 				 enum led_brightness brightness)
 {
@@ -53,12 +35,10 @@ static ssize_t multi_max_intensity_show(struct device *dev,
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
-	unsigned int max_intensity;
 	int len = 0;
 
-	for (int i = 0; i < mcled_cdev->num_colors; i++) {
-		max_intensity = led_mc_get_max_intensity(mcled_cdev, i);
-		len += sysfs_emit_at(buf, len, "%u", max_intensity);
+	for (unsigned int i = 0; i < mcled_cdev->num_colors; i++) {
+		len += sysfs_emit_at(buf, len, "%u", mcled_cdev->subled_info[i].max_intensity);
 		if (i < mcled_cdev->num_colors - 1)
 			len += sprintf(buf + len, " ");
 	}
@@ -98,8 +78,8 @@ static ssize_t multi_intensity_store(struct device *dev,
 		goto err_out;
 	}
 
-	for (int i = 0; i < mcled_cdev->num_colors; i++) {
-		max_intensity = led_mc_get_max_intensity(mcled_cdev, i);
+	for (unsigned int i = 0; i < mcled_cdev->num_colors; i++) {
+		max_intensity = mcled_cdev->subled_info[i].max_intensity;
 		mcled_cdev->subled_info[i].intensity = min(intensity_value[i], max_intensity);
 	}
 
@@ -187,6 +167,18 @@ int led_classdev_multicolor_register_ext(struct device *parent,
 	if (ret < 0)
 		return ret;
 
+	/*
+	 * The maximum global brightness value might still be changed by
+	 * led_classdev_init() using devicetree properties. Because of this
+	 * we can only adjust subled_info[X].max_intensity after said function
+	 * was called.
+	 */
+	for (unsigned int i = 0; i < mcled_cdev->num_colors; i++)
+	{
+		if (!mcled_cdev->subled_info[i].max_intensity)
+			mcled_cdev->subled_info[i].max_intensity = led_cdev->max_brightness;
+	}
+
 	led_cdev->dev->type = &led_multicolor_device_type;
 	ret = led_classdev_add(led_cdev);
 	if (ret < 0)
-- 
2.56.0

Re: [PATCH 10/10] leds: multicolor: Adjust maximum subled intensity during initialization

[sashiko-bot]

> The maximum brightness of a LED device is fixed after having called
> led_classdev_init(), so performing the adjustment during runtime is
> no longer necessary.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018135339.1109271-1-W_Armin@gmx.de?part=10

Re: [PATCH 10/10] leds: multicolor: Adjust maximum subled intensity during initialization

[Rong Zhang]

Hi Armin,

Nice cleanup!

On Sun, 2026-10-18 at 15:46 +0200, Armin Wolf wrote:
> The maximum brightness of a LED device is fixed after having called
> led_classdev_init(), so performing the adjustment during runtime is
> no longer necessary.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/leds/led-class-multicolor.c | 40 ++++++++++++-----------------
>  1 file changed, 16 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
> index 9b8c37f7bdf4..875c615c33fc 100644
> --- a/drivers/leds/led-class-multicolor.c
> +++ b/drivers/leds/led-class-multicolor.c
> @@ -14,24 +14,6 @@
>  #include <linux/uaccess.h>
>  #include "leds.h"
>  
> -static unsigned int led_mc_get_max_intensity(struct led_classdev_mc *mcled_cdev, size_t index)
> -{
> -	unsigned int max_intensity;
> -
> -	/*
> -	 * The maximum global brightness value might still be changed by
> -	 * led_classdev_register_ext() using devicetree properties. This
> -	 * prevents us from changing subled_info[X].max_intensity when
> -	 * registering a multicolor LED class device, so we have to do
> -	 * this during runtime.
> -	 */
> -	max_intensity = mcled_cdev->subled_info[index].max_intensity;
> -	if (max_intensity)
> -		return max_intensity;
> -
> -	return mcled_cdev->led_cdev.max_brightness;
> -}
> -
>  int led_mc_calc_color_components(struct led_classdev_mc *mcled_cdev,
>  				 enum led_brightness brightness)
>  {
> @@ -53,12 +35,10 @@ static ssize_t multi_max_intensity_show(struct device *dev,
>  {
>  	struct led_classdev *led_cdev = dev_get_drvdata(dev);
>  	struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
> -	unsigned int max_intensity;
>  	int len = 0;
>  
> -	for (int i = 0; i < mcled_cdev->num_colors; i++) {
> -		max_intensity = led_mc_get_max_intensity(mcled_cdev, i);
> -		len += sysfs_emit_at(buf, len, "%u", max_intensity);
> +	for (unsigned int i = 0; i < mcled_cdev->num_colors; i++) {
> +		len += sysfs_emit_at(buf, len, "%u", mcled_cdev->subled_info[i].max_intensity);
>  		if (i < mcled_cdev->num_colors - 1)
>  			len += sprintf(buf + len, " ");
>  	}
> @@ -98,8 +78,8 @@ static ssize_t multi_intensity_store(struct device *dev,
>  		goto err_out;
>  	}
>  
> -	for (int i = 0; i < mcled_cdev->num_colors; i++) {
> -		max_intensity = led_mc_get_max_intensity(mcled_cdev, i);
> +	for (unsigned int i = 0; i < mcled_cdev->num_colors; i++) {
> +		max_intensity = mcled_cdev->subled_info[i].max_intensity;
>  		mcled_cdev->subled_info[i].intensity = min(intensity_value[i], max_intensity);
>  	}
>  
> @@ -187,6 +167,18 @@ int led_classdev_multicolor_register_ext(struct device *parent,
>  	if (ret < 0)
>  		return ret;
>  
> +	/*
> +	 * The maximum global brightness value might still be changed by

s/still be/have been/ ?

Thanks,
Rong

> +	 * led_classdev_init() using devicetree properties. Because of this
> +	 * we can only adjust subled_info[X].max_intensity after said function
> +	 * was called.
> +	 */
> +	for (unsigned int i = 0; i < mcled_cdev->num_colors; i++)
> +	{
> +		if (!mcled_cdev->subled_info[i].max_intensity)
> +			mcled_cdev->subled_info[i].max_intensity = led_cdev->max_brightness;
> +	}
> +
>  	led_cdev->dev->type = &led_multicolor_device_type;
>  	ret = led_classdev_add(led_cdev);
>  	if (ret < 0)