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

[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?

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