Re: [PATCH 10/10] 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 7 potential issue(s) to consider:
- [Medium] leds: ktd202x: double truncation in ktd202x_blink_set() breaks timer sharing
- [Medium] leds: ktd202x: ktd202x_timer_set() leaves on_timer set on register write error
- [Medium] leds: ktd202x: regmap_update_bits() skips timer setup after regulator disable
- [Medium] leds: rgb: ktd202x: race on cdev.brightness in ktd202x_brightness_set()
- [Medium] leds: rgb: ktd202x: blink_set() clobbers brightness after -EBUSY fallback
- [Medium] leds: ktd202x: race and glitch from unlocked gap before ktd202x_timer_set()
- [Low] leds: ktd202x: pattern_set turns LED on in hardware before timer busy check
--

commit 06fc1f38dbec3466d62b49ae0ee28d54bc03cc75
Author: Joseph Telaak <joseph@telaak.dev>

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

This commit tracks which LEDs are actively using the shared PWM1 hardware
flash timer so that a newly blinking or breathing LED only joins the hardware
timer if it is unused or already configured with the same period, on time,
and ramp timings, returning -EBUSY otherwise.

> diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
> index 5f51df9930cb2..f4544ba337d17 100644
> --- a/drivers/leds/rgb/leds-ktd202x.c
> +++ b/drivers/leds/rgb/leds-ktd202x.c
[ ... ]
> @@ -212,24 +225,9 @@ 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
> -	 */
> -	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;
> -	}
> +	/* A LED that is turned off stops blinking and leaves the timer */
> +	if (!led->cdev.brightness)
> +		led->on_timer = false;

[Severity: Medium]
Can checking led->cdev.brightness here race with concurrent brightness
updates and fail to clear led->on_timer?

In ktd202x_brightness_single_set() and ktd202x_brightness_mc_set(),
cdev->brightness is written before acquiring led->chip->mutex:

ktd202x_brightness_single_set()
    cdev->brightness = value;
    mutex_lock(&led->chip->mutex);
    info.brightness = value;
    info.channel = led->index;
    ret = ktd202x_brightness_set(led, &info, 1);

In addition, the LED core updates led_cdev->brightness synchronously in
led_set_brightness_nosleep() before scheduling set_brightness_work. When a
blinking LED is turned off and immediately set to a new steady brightness
(for example via LED_SET_BRIGHTNESS_OFF followed by LED_SET_BRIGHTNESS in
set_brightness_delayed(), or two concurrent updates), led->cdev.brightness
can already be non-zero by the time ktd202x_brightness_set() runs for the
LED_OFF transition.

In that case, subleds[i].brightness is 0, yet led->cdev.brightness is
non-zero, so led->on_timer stays true and the subsequent non-zero brightness
write puts the channel back into KTD202X_CHANNEL_CTRL_PWM1 instead of steady
KTD202X_CHANNEL_CTRL_ON.

Also, because led_set_brightness(cdev, LED_OFF) defers brightness_set_blocking
to set_brightness_work, turning off a blinking LED and immediately setting a
blink or pattern on a second LED can still see other->on_timer set in
ktd202x_timer_busy() before the work item runs. Should ktd202x_brightness_set()
determine whether all channels are off from the subled brightness values
passed under led->chip->mutex?

[ ... ]
> @@ -320,6 +315,89 @@ static unsigned long ktd202x_flash_period_ms(const struct ktd202x_info *info,
>  	return num_steps * KTD202X_FLASH_PERIOD_STEP_MS + info->flash_period_min_ms;
>  }
>  
[ ... ]
> +/* Called with the chip mutex held */
> +static int ktd202x_timer_set(struct ktd202x_led *led, struct mc_subled *subleds,
> +			     unsigned int num_channels, u8 flash_period, u8 flash_on,
> +			     u8 ramp_times)
> +{
> +	struct ktd202x *chip = led->chip;
> +	u8 ctrl_mask = 0;
> +	u8 ctrl_mode = 0;
> +	bool busy;
> +	int ret;
> +	int i;
> +
> +	busy = ktd202x_timer_busy(led, flash_period, flash_on, ramp_times);
> +	led->on_timer = !busy;
> +
> +	for (i = 0; i < num_channels; i++) {
> +		int channel = subleds[i].channel;
> +
> +		ctrl_mask |= KTD202X_CHANNEL_CTRL_MASK(channel);
> +		ctrl_mode |= ktd202x_channel_mode(led, channel);
> +	}
> +
> +	/* Without the timer, leave the LED steadily on */
> +	if (busy) {
> +		ret = regmap_update_bits(chip->regmap, KTD202X_REG_CHANNEL_CTRL,
> +					 ctrl_mask, ctrl_mode);
> +		return ret ? ret : -EBUSY;
> +	}
> +
> +	chip->flash_period = flash_period;
> +	chip->flash_on = flash_on;
> +	chip->ramp_times = ramp_times;
> +
> +	/*
> +	 * Update the timer registers only when they change, so that the
> +	 * blink of another LED on the timer is not restarted.
> +	 */
> +	ret = regmap_update_bits(chip->regmap, KTD202X_REG_FLASH_PERIOD, 0xff,
> +				 flash_period);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(chip->regmap, KTD202X_REG_PWM1_TIMER, 0xff,
> +				 flash_on);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(chip->regmap, KTD202X_REG_TRISE_FALL, 0xff,
> +				 ramp_times);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(chip->regmap, KTD202X_REG_CHANNEL_CTRL,
> +				  ctrl_mask, ctrl_mode);
> +}

[Severity: Medium]
If one of the four regmap_update_bits() calls in the !busy path of
ktd202x_timer_set() fails, won't led->on_timer remain true?

When called from ktd202x_blink_set(), returning a negative error code causes
led_blink_setup() in the LED core to fall back to software blinking without
first setting brightness to LED_OFF. With led->on_timer left set to true,
each software blink ON tick in ktd202x_brightness_set() will program
KTD202X_CHANNEL_CTRL_PWM1 instead of KTD202X_CHANNEL_CTRL_ON, and
ktd202x_timer_busy() will treat this LED as occupying the hardware timer
with the partially updated chip->flash_period, chip->flash_on, and
chip->ramp_times values.

Should led->on_timer and the chip timer fields only be committed once the
register writes succeed (or rolled back on error)?

[Severity: Medium]
Also in ktd202x_timer_set(), could switching KTD202X_REG_FLASH_PERIOD,
KTD202X_REG_PWM1_TIMER, and KTD202X_REG_TRISE_FALL from regmap_write() to
regmap_update_bits() skip programming the hardware timer after the chip has
been powered down?

When all LEDs are turned off, ktd202x_brightness_set() calls
ktd202x_chip_disable(), which disables the vin and vio regulators without
invalidating the REGCACHE_FLAT cache:

ktd202x_brightness_set()
    ktd202x_chip_disable()
        regmap_write(chip->regmap, KTD202X_REG_RESET_CONTROL, KTD202X_ENABLE_CTRL_SLEEP);
        regulator_bulk_disable(ARRAY_SIZE(chip->regulators), chip->regulators);

If disabling the regulators removes power from the chip, the hardware timer
registers reset to their power-on defaults, while REGCACHE_FLAT retains the
last values written before power-down. Previously regmap_write() always
transmitted the timer registers over I2C, whereas regmap_update_bits() skips
the write when the cached value matches. If an LED is later set to blink or
breathe with the same timings as before the chip was disabled, won't
regmap_update_bits() elide the register writes and leave the chip running at
its reset defaults?

> @@ -363,28 +440,19 @@ static int ktd202x_blink_set(struct ktd202x_led *led, unsigned long *delay_on,
>  	on = mul_u64_u64_div_u64(*delay_on, KTD202X_FLASH_ON_MAX, delay_total_ms);
>  	on = clamp(on, 1, KTD202X_FLASH_ON_MAX - 1);
>  
> +	/*
> +	 * Without the ramps a hardware pattern may have set. On -EBUSY the
> +	 * LED core falls back to software blinking.
> +	 */
> +	ret = ktd202x_timer_set(led, subleds, num_channels, num_steps, on,
> +				KTD202X_RAMP_TIMES_2_MS);
> +	if (ret)
> +		goto out;
> +
>  	/* Actually used delay_{on,off} values */
>  	delay_total_ms = ktd202x_flash_period_ms(info, num_steps);
>  	*delay_on = (delay_total_ms * on) / KTD202X_FLASH_ON_MAX;
>  	*delay_off = delay_total_ms - *delay_on;

[Severity: Medium]
Documentation/leds/leds-ktd202x.rst notes:

"To share the timer, give the second LED the delays that the first one reads
back."

Does round-tripping the read-back delay_on and delay_off values through
ktd202x_blink_set() actually produce the same flash_on value when both
(delay_total_ms / 128) and on are odd?

Because delay_total_ms is a multiple of 128 ms (128 * k) and
KTD202X_FLASH_ON_MAX is 256, computing the read-back delay_on on the first
LED truncates towards zero:

    *delay_on = (delay_total_ms * on) / KTD202X_FLASH_ON_MAX;

which is (k * on) / 2. Whenever k and on are both odd (for example, the
default 500 ms / 500 ms blink on KTD2026/KTD2027 gives num_steps = 5,
delay_total_ms = 896 ms with k = 7, and any odd on step such as on = 129
reads back as delay_on = 451 ms, delay_off = 445 ms), *delay_on truncates
to (k * on - 1) / 2.

When those read-back delays are written to the second LED,
ktd202x_blink_set() computes:

    on = mul_u64_u64_div_u64(*delay_on, KTD202X_FLASH_ON_MAX, delay_total_ms);

which evaluates to ((k * chip->flash_on - 1) / 2 * 256) / (128 * k) =
chip->flash_on - 1. That causes ktd202x_timer_busy() to see flash_on !=
chip->flash_on and reject the second LED with -EBUSY. Should the conversion
from *delay_on to on (or both conversions) round to nearest instead of
truncating twice?

[Severity: Medium]
When ktd202x_timer_set() returns -EBUSY here and the LED core falls back to
software blinking, can a subsequent update to delay_on or delay_off clobber
the LED's configured brightness?

In drivers/leds/led-core.c, led_blink_setup() falls back to
led_set_software_blink(), where led_timer_function() alternates
cdev->brightness between cdev->blink_brightness and LED_OFF (0). If the user
later writes delay_on or delay_off via sysfs while the software blink timer
happens to be in its OFF half-cycle (so cdev->brightness is 0 while
cdev->blink_brightness holds the configured brightness),
ktd202x_blink_single_set() and ktd202x_blink_mc_set() do:

ktd202x_blink_single_set()
    if (!cdev->brightness) {
        ret = ktd202x_brightness_single_set(cdev, cdev->max_brightness);
        if (ret)
            return ret;
    }

Wouldn't this overwrite a lower configured brightness with max_brightness (or
KTD202X_MAX_BRIGHTNESS in ktd202x_blink_mc_set()) instead of preserving
cdev->blink_brightness?

[ ... ]
> @@ -556,7 +601,11 @@ static int ktd202x_pattern_single_set(struct led_classdev *cdev,
>  
>  	info.channel = led->index;
>  
> -	return ktd202x_pattern_program(led, pattern, &info, 1);
> +	ret = ktd202x_pattern_program(led, pattern, &info, 1);
> +	if (ret)
> +		ktd202x_brightness_single_set(cdev, LED_OFF);
> +
> +	return ret;
>  }
>  
>  static int ktd202x_pattern_single_clear(struct led_classdev *cdev)
> @@ -580,8 +629,12 @@ static int ktd202x_pattern_mc_set(struct led_classdev *cdev,
>  	if (ret)
>  		return ret;
>  
> -	return ktd202x_pattern_program(led, pattern, mc->subled_info,
> -				       mc->num_colors);
> +	ret = ktd202x_pattern_program(led, pattern, mc->subled_info,
> +				      mc->num_colors);
> +	if (ret)
> +		ktd202x_brightness_mc_set(cdev, LED_OFF);
> +
> +	return ret;
>  }

[Severity: Low]
In ktd202x_pattern_single_set() and ktd202x_pattern_mc_set(),
ktd202x_brightness_single_set() or ktd202x_brightness_mc_set() is called
before ktd202x_pattern_program().

When another LED is already using the timer with different parameters, won't
this turn the LED physically on in hardware over I2C at pattern[1].brightness
(and write KTD202X_CHANNEL_CTRL_ON again in the busy branch of
ktd202x_timer_set()), only to turn it back off over I2C when
ktd202x_pattern_program() returns -EBUSY, causing a brief flash on an off LED
(or turning off an LED that was previously lit)?

[Severity: Medium]
Relatedly, because ktd202x_brightness_single_set() and
ktd202x_brightness_mc_set() release led->chip->mutex before
ktd202x_blink_set() or ktd202x_pattern_program() re-acquires it to call
ktd202x_timer_set(), can an intervening LED_OFF update race in that unlocked
window?

For example, if a queued set_brightness_work item (from a prior software
blink or software pattern timer tick, or multi_intensity_store() when
brightness is 0) or a concurrent delay_on=0 write runs
ktd202x_brightness_set() with LED_OFF between the brightness call and
ktd202x_timer_set():

CPU0                                     CPU1
ktd202x_pattern_single_set()
  ktd202x_brightness_single_set()
    mutex_lock(&chip->mutex)
    ...
    mutex_unlock(&chip->mutex)
                                         set_brightness_delayed()
                                           ktd202x_brightness_single_set(cdev, LED_OFF)
                                             mutex_lock(&chip->mutex)
                                             led->on_timer = false;
                                             /* channel OFF, chip disabled */
                                             mutex_unlock(&chip->mutex)
  ktd202x_pattern_program()
    mutex_lock(&chip->mutex)
    ktd202x_timer_set()
      led->on_timer = true;
      /* sets channel to PWM1 */
    mutex_unlock(&chip->mutex)

Wouldn't ktd202x_timer_set() set led->on_timer = true and enable PWM1 on the
channel even though cdev->brightness is now 0 (and the chip may have been
disabled by ktd202x_chip_disable())?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101-ktd202x-et6326-v1-0-eb51eddad08c@telaak.dev?part=10