Re: [PATCH v1 03/11] leds: st1202: Name pattern index variables consistently

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete cleanup of remaining hardware pattern durations in st1202_led_pattern_set.
- [High] Global hardware duration registers are corrupted by independent LED pattern or blink requests.
- [High] st1202_blink_set inadvertently turns off continuously lit LEDs.
--

commit 46a022fdeb18fd45b309d2df4aa547f0116cf2c4
Author: Manuel Fombuena <fombuena@outlook.com>

leds: st1202: Name pattern index variables consistently

This commit renames the loop variables for pattern indices from "patt"
to "pattern" across several functions to maintain consistency. It also
renames the "pattern" array parameter to "patterns" in
st1202_led_pattern_set() to avoid shadowing.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index d8a04e96fb46c..ddb8c60277067 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c

[ ... ]

> @@ -251,12 +252,13 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
>  	if (ret != 0)
>  		return ret;
>  
> -	for (int patt = 0; patt < len; patt++) {
> -		ret = st1202_pwm_pattern_write(chip, led->led_num, patt, pattern[patt].brightness);
> +	for (int pattern = 0; pattern < len; pattern++) {
> +		ret = st1202_pwm_pattern_write(chip, led->led_num, pattern,
> +						patterns[pattern].brightness);
>  		if (ret != 0)
>  			return ret;
>  
> -		ret = st1202_duration_pattern_write(chip, patt, pattern[patt].delta_t);
> +		ret = st1202_duration_pattern_write(chip, pattern, patterns[pattern].delta_t);

[Severity: High]
This isn't a bug introduced by this patch, but will this write clobber
any pattern durations set by other LEDs?

Since the ST1202_PATTERN_DUR registers are global to the chip, if userspace
sets a pattern on one LED, and then sets a pattern or blink on another LED,
it seems the second LED's duration writes will overwrite the first LED's
timing.

The driver appears to expose shared hardware state as independent without
-EBUSY synchronization, and this behavior remains unresolved at the end of
the patch series.

>  		if (ret != 0)
>  			return ret;
>  	}

[Severity: High]
This is a pre-existing issue, but does this function leave durations for
trailing slots intact?

The loop sets durations up to 'len', but finishes without zeroing the global
duration registers for slots 'len' through ST1202_MAX_PATTERNS - 1.

Could this cause the hardware sequencer to execute trailing steps from a
previous longer pattern? During these ghost steps, the LED would be off,
which could lead to unexpected dark pauses that break the timing of the new
shorter pattern. This can be triggered if userspace sets a long pattern
followed by a short pattern via sysfs on the same LED.

This behavior persists through the entire patch series (including patch 5
which only clears the PWM registers, failing to clear the duration registers).


Also, looking at st1202_blink_set() which is not modified in this patch:

drivers/leds/leds-st1202.c:st1202_blink_set() {
    ...
	/* Zero out PWM for all other active channels to prevent them from blinking */
	for (int chan = 0; chan < ST1202_MAX_LEDS; chan++) {
		if (!chip->leds[chan].is_active || chan == led->led_num)
			continue;

		ret = st1202_pwm_pattern_write(chip, chan, 0, LED_OFF);
		if (ret)
			return ret;

		ret = st1202_pwm_pattern_write(chip, chan, 1, LED_OFF);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does st1202_blink_set()
inadvertently turn off continuously lit LEDs?

This unconditionally clears the first two pattern slots for all other channels,
which ruins the ST1202_PATTERN_PWM_FULL state set by st1202_led_set() and
forces them OFF.

If a second LED is set to a continuous brightness, and userspace requests a
blink on the first LED, st1202_blink_set() will overwrite the second LED's
slots 0 and 1 to LED_OFF. Since the global sequencer loops only over slots 0
and 1 during blink, the second LED will output 0 brightness.

This remains unresolved at the end of the patch series (including patch 10).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/GV1PR08MB8497422B7A44424C4968AEC1C5BD2@GV1PR08MB8497.eurprd08.prod.outlook.com?part=3