Re: [PATCH v2 11/11] leds: rgb: ktd202x: Don't let a blinking LED retime another one

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] leds: rgb: ktd202x: on_timer cleared in
  ktd202x_brightness_set() when dimmed
- [Medium] leds: rgb: ktd202x: unlocked cdev->brightness race in
  ktd202x_blink_*_set()
--

commit 2bb78bf782d05191c786ef675dcadd7bc9160236
Author: Joseph Telaak <joseph@telaak.dev>

leds: rgb: ktd202x: Don't let a blinking LED retime another one

This commit tracks which LEDs use the shared PWM1 flash timer so that
starting a blink or hardware pattern on another LED with different timings
returns -EBUSY instead of retiming the active LEDs.

> diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
> index 0cc2ec3f6a7f6..e2f389d8888c9 100644
> --- a/drivers/leds/rgb/leds-ktd202x.c
> +++ b/drivers/leds/rgb/leds-ktd202x.c
[ ... ]
> @@ -212,24 +227,13 @@ static int ktd202x_brightness_set(struct ktd202x_led *led,
>  			return ret;
>  	}
>  
> -	ret = regmap_read(led->chip->regmap, KTD202X_REG_CHANNEL_CTRL, &state);
> -	if (ret)
> -		return ret;
> -
> -	/*
> -	 * In multicolor case, assume blink mode if PWM is set for at least one
> -	 * channel because another channel cannot be in state ON at the same time
> -	 */
> +	/* A LED that is turned off stops blinking and leaves the timer */
>  	for (i = 0; i < num_channels; i++) {
> -		int channel_state;
> -
> -		channel = subleds[i].channel;
> -		channel_state = (state >> 2 * channel) & KTD202X_CHANNEL_CTRL_MASK(0);
> -		if (channel_state == KTD202X_CHANNEL_CTRL_OFF)
> -			continue;
> -		mode_blink = channel_state == KTD202X_CHANNEL_CTRL_PWM1(0);
> -		break;
> +		if (subleds[i].brightness)
> +			lit = true;
>  	}
> +	if (!lit)
> +		led->on_timer = false;

[Severity: Medium]
Can this clear led->on_timer while a multicolor LED's overall brightness is
still non-zero?

In __ktd202x_brightness_mc_set(), led_mc_calc_color_components() scales
each subled[i].brightness with DIV_ROUND_CLOSEST(value * intensity,
max_brightness).  When a blinking or breathing multicolor LED is dimmed to
a small non-zero brightness where every channel rounds down to 0, or when
multi_intensity is set to 0 0 0, the LED core keeps the trigger active
because brightness is not LED_OFF, yet !lit clears led->on_timer.  A later
brightness or multi_intensity increase then programs
KTD202X_CHANNEL_CTRL_ON instead of KTD202X_CHANNEL_CTRL_PWM1, leaving the
LED steadily on and letting another LED overwrite the shared timer.

Conversely, ktd202x_timer_set() sets led->on_timer = true even when all
subleds[i].brightness are 0.

[ ... ]

> @@ -424,10 +522,26 @@ static int ktd202x_blink_single_set(struct led_classdev *cdev,
>  	if (!*delay_on)
>  		return ktd202x_brightness_single_set(cdev, LED_OFF);
>  
> +	mutex_lock(&led->chip->mutex);
> +
> +	/*
> +	 * An unlit LED blinks at the brightness of a software blink, which may
> +	 * be in its off phase, or else at full brightness.
> +	 */
> +	if (!cdev->brightness) {
> +		ret = __ktd202x_brightness_single_set(led, cdev->blink_brightness ?:
> +						      cdev->max_brightness);
> +		if (ret)
> +			goto out;
> +	}
> +
>  	info.brightness = cdev->brightness;
>  	info.channel = led->index;
>  
> -	return ktd202x_blink_set(led, delay_on, delay_off, &info, 1);
> +	ret = ktd202x_blink_set(led, delay_on, delay_off, &info, 1);
> +out:
> +	mutex_unlock(&led->chip->mutex);
> +	return ret;
>  }
>  
>  static int ktd202x_blink_mc_set(struct led_classdev *cdev,
> @@ -438,12 +552,6 @@ static int ktd202x_blink_mc_set(struct led_classdev *cdev,
>  	struct ktd202x_led *led = container_of(mc, struct ktd202x_led, mcdev);
>  	int ret;
>  
> -	if (!cdev->brightness) {
> -		ret = ktd202x_brightness_mc_set(cdev, KTD202X_MAX_BRIGHTNESS);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	/* If no blink specified, default to 1 Hz. */
>  	if (!*delay_off && !*delay_on) {
>  		*delay_off = 500;
> @@ -454,8 +562,24 @@ static int ktd202x_blink_mc_set(struct led_classdev *cdev,
>  	if (!*delay_on)
>  		return ktd202x_brightness_mc_set(cdev, LED_OFF);
>  
> -	return ktd202x_blink_set(led, delay_on, delay_off, mc->subled_info,
> -				 mc->num_colors);
> +	mutex_lock(&led->chip->mutex);
> +
> +	/*
> +	 * An unlit LED blinks at the brightness of a software blink, which may
> +	 * be in its off phase, or else at full brightness.
> +	 */
> +	if (!cdev->brightness) {
> +		ret = __ktd202x_brightness_mc_set(led, cdev->blink_brightness ?:
> +						  KTD202X_MAX_BRIGHTNESS);
> +		if (ret)
> +			goto out;
> +	}
> +
> +	ret = ktd202x_blink_set(led, delay_on, delay_off, mc->subled_info,
> +				mc->num_colors);
> +out:
> +	mutex_unlock(&led->chip->mutex);
> +	return ret;
>  }

[Severity: Medium]
Can reading cdev->brightness in ktd202x_blink_single_set() and
ktd202x_blink_mc_set() still race with led_set_brightness_nosleep()?

Because this driver uses brightness_set_blocking,
led_set_brightness_nosleep() updates cdev->brightness without holding
chip->mutex and defers the hardware update to set_brightness_work:

CPU0 (<softirq> / sysfs)          CPU1
led_set_brightness_nosleep()
  cdev->brightness = val
  queue_work(set_brightness_work)
                                  ktd202x_blink_mc_set()
                                    mutex_lock(&led->chip->mutex)
                                    if (!cdev->brightness) -> false
                                    ktd202x_blink_set()
                                      ktd202x_timer_set()  <-- chip disabled

When the LED was off in hardware (such as after the off phase of software
blinking), skipping __ktd202x_brightness_mc_set() or
__ktd202x_brightness_single_set() leaves the chip disabled when no other
LED is lit and leaves mc->subled_info[i].brightness at 0, so
ktd202x_timer_set() writes registers to a powered-down chip and leaves the
channels off while setting led->on_timer = true.

In ktd202x_blink_single_set(), cdev->brightness is also read a second time
for info.brightness, so a concurrent led_set_brightness_nosleep(cdev,
LED_OFF) can set cdev->brightness = 0 in between and pass info.brightness =
0 to ktd202x_blink_set().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-ktd202x-et6326-v2-0-56e97c226658@telaak.dev?part=11