Re: [PATCH v7 2/9] leds: st1202: validate pattern input before stopping the sequence

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] st1202_led_pattern_set() fails to clear or mark the remaining pattern slots as skipped when len < ST1202_MAX_PATTERNS.
- [High] st1202_led_pattern_clear() unconditionally zeroes the shared global duration registers, destroying the timing of active sequences on all other LEDs.
--

commit 9126a03ff29680e83b737c78e164f544294aa400
Author: Manuel Fombuena <fombuena@outlook.com>

leds: st1202: validate pattern input before stopping the sequence

This commit moves input validation for pattern duration before taking the
mutex and interacting with the hardware. This prevents the chip from being
left in a stopped and partially written state if validation fails mid-loop.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 316ed8eb054f9..a750c95ef3f89 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
[ ... ]
> @@ -235,10 +241,6 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
>  		return ret;
>  
>  	for (int patt = 0; patt < len; patt++) {
> -		if (pattern[patt].delta_t < ST1202_MILLIS_PATTERN_DUR_MIN ||
> -				pattern[patt].delta_t > ST1202_MILLIS_PATTERN_DUR_MAX)
> -			return -EINVAL;
> -
>  		ret = st1202_pwm_pattern_write(chip, led->led_num, patt, pattern[patt].brightness);
>  		if (ret != 0)
>  			return ret;

[Severity: High]
This is a pre-existing issue, but does st1202_led_pattern_set() fail to clear
or mark the remaining pattern slots as skipped when len < ST1202_MAX_PATTERNS?

If a user writes a pattern of length L (where L < 8) to the sysfs pattern
attribute, this loop only iterates up to len, leaving the hardware duration
registers for slots len through 7 unmodified.

The ST1202 executes a fixed 8-slot sequence unless unused slots are explicitly
marked to skip by setting duration to 0.

Because the driver only programs the first L slots and abandons the remaining
slots without zeroing their duration registers, will the hardware sequencer
continue playing the stale durations and PWM values for the remaining steps
instead of truncating the pattern?

[Severity: High]
This isn't a bug introduced by this patch, but does st1202_led_pattern_clear()
unconditionally zero the shared global duration registers?

drivers/leds/leds-st1202.c:st1202_led_pattern_clear() {
    ...
    for (int patt = 0; patt < ST1202_MAX_PATTERNS; patt++) {
        ...
        ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + patt, 0);
    }
    ...
}

If a user sets a pattern on LED A, then clears the pattern on LED B using sysfs,
this unconditional write to the global duration registers might destroy the timing
of active sequences on all other LEDs.

The ST1202 uses a single global sequencer with chip-wide duration registers.
When a pattern is cleared on a single LED, writing 0 to all ST1202_PATTERN_DUR
registers marks the slots as skipped globally.

This instantly freezes or destroys the running pattern on all other LEDs sharing
the chip. Should the driver only modify the target LED's PWM registers to visually
clear it, without touching the shared timing?

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