Re: [PATCH] leds: class: Don't expose color sysfs entry

[Hans de Goede]

Hi,

On 11/21/23 17:23, Takashi Iwai wrote:
> The commit c7d80059b086 ("leds: class: Store the color index in struct
> led_classdev") introduced a new sysfs entry "color" that is commonly
> created for the led classdev.  Unfortunately, this conflicts with the
> "color" sysfs entry of already existing drivers such as Logitech HID
> or System76 ACPI drivers.  The driver probe fails due to the conflict,
> hence it leads to a severe regression with the missing keyboard, for
> example.
> 
> This patch reverts partially the change in the commit above for
> removing the led class color sysfs entries again for addressing the
> regressions.  The newly introduced led_classdev.color field is kept as
> it's already used by other driver.
> 
> Fixes: c7d80059b086 ("leds: class: Store the color index in struct led_classdev")
> Reported-by: Johannes Penßel <johannes.penssel@xxxxxxxxx>
> Closes: https://lore.kernel.org/r/b5646db3-acff-45aa-baef-df3f660486fb@xxxxxxxxx
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218045
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218155
> Link: https://bugzilla.suse.com/show_bug.cgi?id=1217172
> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>

Thank you for taking care of this, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>

Regards,

Hans

> ---
> 
> This is a sort of v2 patch, as it turned out that the full revert
> leads to a build error.
> 
>  Documentation/ABI/testing/sysfs-class-led |  9 ---------
>  drivers/leds/led-class.c                  | 14 --------------
>  2 files changed, 23 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
> index b2ff0012c0f2..2e24ac3bd7ef 100644
> --- a/Documentation/ABI/testing/sysfs-class-led
> +++ b/Documentation/ABI/testing/sysfs-class-led
> @@ -59,15 +59,6 @@ Description:
>  		brightness. Reading this file when no hw brightness change
>  		event has happened will return an ENODATA error.
>  
> -What:		/sys/class/leds/<led>/color
> -Date:		June 2023
> -KernelVersion:	6.5
> -Description:
> -		Color of the LED.
> -
> -		This is a read-only file. Reading this file returns the color
> -		of the LED as a string (e.g: "red", "green", "multicolor").
> -
>  What:		/sys/class/leds/<led>/trigger
>  Date:		March 2006
>  KernelVersion:	2.6.17
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 974b84f6bd6a..ba1be15cfd8e 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -75,19 +75,6 @@ static ssize_t max_brightness_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(max_brightness);
>  
> -static ssize_t color_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	const char *color_text = "invalid";
> -	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> -
> -	if (led_cdev->color < LED_COLOR_ID_MAX)
> -		color_text = led_colors[led_cdev->color];
> -
> -	return sysfs_emit(buf, "%s\n", color_text);
> -}
> -static DEVICE_ATTR_RO(color);
> -
>  #ifdef CONFIG_LEDS_TRIGGERS
>  static BIN_ATTR(trigger, 0644, led_trigger_read, led_trigger_write, 0);
>  static struct bin_attribute *led_trigger_bin_attrs[] = {
> @@ -102,7 +89,6 @@ static const struct attribute_group led_trigger_group = {
>  static struct attribute *led_class_attrs[] = {
>  	&dev_attr_brightness.attr,
>  	&dev_attr_max_brightness.attr,
> -	&dev_attr_color.attr,
>  	NULL,
>  };
>