Re: [PATCH v3 1/1] leds: st1202: Add hardware-accelerated blink support

[sashiko-bot@kernel.org]

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

New issues:
- [High] st1202_blink_set() forcibly zeroes out the active PWM pattern slots for all other LEDs, unintentionally destroying the state of independently configured steady-ON LEDs.
- [Medium] st1202_blink_set() ignores the LED's configured brightness and forces the hardware current limit to maximum (U8_MAX).

Pre-existing issues:
- [High] The driver assigns a sleeping function (st1202_brightness_set) to the non-blocking led_cdev.brightness_set callback, leading to kernel panics when called from atomic context.
--

--- Patch [1]: [PATCH v3 1/1] leds: st1202: Add hardware-accelerated blink support ---
commit d1f0e631f570c78c507b9a24936dea1d4c6fa4f3
Author: Manuel Fombuena <fombuena@outlook.com>

leds: st1202: Add hardware-accelerated blink support

This commit implements blink_set() to enable hardware-accelerated blinking
via the timer trigger on the ST1202 LED controller. It configures the
chip's pattern sequencer to alternate between full brightness and off for
the specified durations.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 168df5ecf27b7..3600409d4bc25 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
[ ... ]
> @@ -275,6 +276,88 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
[ ... ]
> +	/* Zero out PWM for all other active channels to prevent them from blinking */
> +	for (int i = 0; i < ST1202_MAX_LEDS; i++) {
> +		if (!chip->leds[i].is_active || i == led->led_num)
> +			continue;
> +		ret = st1202_pwm_pattern_write(chip, i, 0, LED_OFF);

[Severity: High]
Does this unintentionally destroy the state of other independently configured
steady-ON LEDs on the same chip? By forcing the PWM pattern slots to 0 for
all other active LEDs, they will be forced dark when the blink sequencer
starts, which seems to break their independent channel operation.

> +		if (ret)
> +			return ret;
> +		ret = st1202_pwm_pattern_write(chip, i, 1, LED_OFF);
> +		if (ret)
> +			return ret;
> +	}
[ ... ]
> +	ret = st1202_write_reg(chip, ST1202_PATTERN_REP, U8_MAX);
> +	if (ret)
> +		return ret;
> +
> +	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, U8_MAX);

[Severity: Medium]
Will this hardcoded maximum current limit (U8_MAX) overwrite the user's
previously configured brightness level? It looks like this ignores the LED's
configured brightness (or led_cdev->blink_brightness) and forces blinking
LEDs to output at 100% hardware intensity.

> +	if (ret)
> +		return ret;
> +
> +	ret = __st1202_channel_set(chip, led->led_num, true);
[ ... ]
> @@ -301,6 +384,7 @@ static int st1202_dt_init(struct st1202_chip *chip)
>  		led->led_cdev.pattern_set = st1202_led_pattern_set;
>  		led->led_cdev.pattern_clear = st1202_led_pattern_clear;
>  		led->led_cdev.default_trigger = "pattern";
> +		led->led_cdev.blink_set = st1202_blink_set;
>  		led->led_cdev.brightness_set = st1202_brightness_set;

[Severity: High]
This is a pre-existing issue, but does assigning st1202_brightness_set() to
the non-blocking led_cdev.brightness_set callback risk a kernel panic?

Looking at st1202_brightness_set(), it takes a sleepable mutex:

    guard(mutex)(&chip->lock);

If an atomic trigger (like a software timer or heartbeat) attempts to update
the LED brightness via this callback, which is strictly documented as
non-sleeping, wouldn't it result in a "scheduling while atomic" panic?

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