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

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

Pre-existing issues:
- [High] The driver improperly writes 8-bit brightness values directly into a 12-bit PWM register without scaling, resulting in extremely dim pattern output.
- [High] The driver implements the `.brightness_set` callback with a function that sleeps, violating LED subsystem requirements and causing a panic in atomic context.
- [High] Clearing one LED's pattern destructively resets the globally shared pattern sequencer and duration registers for all other LEDs.
--

commit 9797d63cea6ed2c2a29dd33d73d899810acde5ba
Author: Manuel Fombuena <[email protected]>

leds: st1202: validate pattern input before stopping the sequence

Input validation for pattern duration is performed inside the write
loop, after the pattern sequence has already been stopped. This patch
moves all input validation before the mutex and before any hardware
interaction, so an invalid input leaves the chip state unchanged.

> 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
> @@ -228,6 +228,12 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
>  	if (len > ST1202_MAX_PATTERNS)
>  		return -EINVAL;
>  
> +	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;
> +	}
> +
>  	guard(mutex)(&chip->lock);
>  
>  	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
> @@ -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);

[Severity: High]
This isn't a bug introduced by this patch, but does this function improperly
write 8-bit brightness values directly into a 12-bit PWM register without
scaling?

Because the input brightness value is bounded to max_brightness (255) by
the subsystem, the upper 8 bits in st1202_pwm_pattern_write() appear to
always be 0:

drivers/leds/leds-st1202.c:st1202_pwm_pattern_write() {
    ...
    value_h = (u8)(value >> 8);
    ...
}

Will this result in extremely dim pattern output (maximum of 6.2% brightness)
and render the pattern feature functionally broken?

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

[ ... ]

Additionally, there are a few other pre-existing issues in this driver
that I noticed:

[Severity: High]
This is a pre-existing issue, but does the driver inappropriately register a
sleeping function as the non-blocking .brightness_set callback?

drivers/leds/leds-st1202.c:st1202_dt_init() {
    ...
    led->led_cdev.brightness_set = st1202_brightness_set;
    ...
}

When a trigger (like the timer trigger) calls this from atomic context,
st1202_brightness_set() takes a mutex and performs I2C operations which can
sleep:

drivers/leds/leds-st1202.c:st1202_brightness_set() {
    ...
    guard(mutex)(&chip->lock);
    ...
}

Will this cause a panic in atomic context? Should the driver only implement
.brightness_set_blocking to ensure the LED core defers the work to a
workqueue?


[Severity: High]
This is a pre-existing issue, but does clearing one LED's pattern
destructively reset the globally shared pattern sequencer for all other LEDs?

In st1202_led_pattern_clear(), it writes directly to the global configuration
register, disabling the pattern sequencer for all LEDs on the chip:

drivers/leds/leds-st1202.c:st1202_led_pattern_clear() {
    ...
    ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
    ...
}

It also appears to blindly zero out the globally shared duration registers:

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

Will this corrupt any sequences currently running on other channels on the
same chip?

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