Re: [PATCH v7 5/9] leds: st1202: fix spurious pattern sequence start in setup

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver assigns a sleeping I2C function to the non-blocking `brightness_set` callback, and the blocking callback ignores brightness scaling.
- [Medium] `st1202_led_pattern_set` fails to truncate patterns, leaving trailing sequence steps active.
- [Medium] Per-LED pattern configuration in `st1202_led_pattern_clear` destructively overwrites shared global sequencer registers, breaking patterns running on other channels.
- [Medium] The driver leaves the hardware powered on upon probe failure or device unbind.
--

commit 892a1c21c022e44fcdad1ed04d340f90f20762de
Author: Manuel Fombuena <fombuena@outlook.com>

leds: st1202: fix spurious pattern sequence start in setup

This commit removes a configuration register write in st1202_setup() that
started the hardware pattern sequencer before any patterns were programmed.
By allowing the device reset to keep the pattern bits cleared, it prevents
the device from running a sequence with uninitialized values.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 3203b3768ec52..b8a5712829009 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -333,11 +333,6 @@ static int st1202_setup(struct st1202_chip *chip)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = st1202_write_reg(chip, ST1202_CONFIG_REG,
> -				ST1202_CONFIG_REG_PATS | ST1202_CONFIG_REG_PATSR);
> -	if (ret < 0)
> -		return ret;
> -
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but I noticed the driver assigns a sleeping
I2C function to the non-blocking brightness_set callback:

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

Will this cause a kernel panic (scheduling while atomic) if a hardware
trigger updates the LED brightness from softirq/hardirq context?

The subsystem calls brightness_set without deferring to a workqueue,
expecting it to be non-blocking, but st1202_brightness_set acquires a mutex
and performs sleeping I2C writes:

drivers/leds/leds-st1202.c:st1202_brightness_set() {
    ...
    guard(mutex)(&chip->lock);
    ...
    st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, value);
    ...
}

Additionally, does the blocking callback ignore actual brightness scaling?
It appears to only turn the channel on or off:

drivers/leds/leds-st1202.c:st1202_led_set() {
    ...
    return st1202_channel_set(led->chip, led->led_num, !!value);
}

[Severity: Medium]
This is a pre-existing issue, but does st1202_led_pattern_set() fail to
truncate patterns, leaving trailing sequence steps active?

drivers/leds/leds-st1202.c:st1202_led_pattern_set() {
    ...
    for (int patt = 0; patt < len; patt++) {
        ret = st1202_pwm_pattern_write(chip, led->led_num, patt,
                                       pattern[patt].brightness);
        if (ret != 0)
            return ret;

        ret = st1202_duration_pattern_write(chip, patt,
                                            pattern[patt].delta_t);
        if (ret != 0)
            return ret;
    }
    ...
}

If a shorter pattern is programmed over a longer one, the loop stops at
len, leaving the global pattern duration registers for trailing steps
unmodified.

Since the hardware sequencer runs up to 8 steps and uses a duration of 0 as
a skip marker, will the LED emit stale flashes from the previously
programmed pattern?

[Severity: Medium]
This is a pre-existing issue, but does st1202_led_pattern_clear()
destructively overwrite shared global sequencer 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 (ret != 0)
            return ret;
    }
    ...
}

Calling this function on one LED unconditionally clears the pattern duration
array. Since this array is shared globally across all channels on the chip,
will this break patterns running concurrently on other LEDs?

[Severity: Medium]
This is a pre-existing issue, but does the driver leave the hardware
powered on upon probe failure or device unbind?

drivers/leds/leds-st1202.c:st1202_probe() {
    ...
    ret = st1202_setup(chip);
    if (ret < 0)
        return ret;

    ret = st1202_dt_init(chip);
    if (ret < 0)
        return ret;
    ...
        ret = devm_led_classdev_register_ext(&client->dev, &led->led_cdev,
                                             &init_data);
        if (ret < 0)
            return dev_err_probe(&client->dev, ret,
                    "Failed to register LED class device\n");
    ...
}

The hardware is activated by writing the enable flag during st1202_setup().
If st1202_dt_init() or devm_led_classdev_register_ext() fails, or if the
driver is unbound, there is no devm_add_action_or_reset() or remove hook to
gracefully turn off the chip.

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