Re: [PATCH v6 4/9] leds: st1202: set all pattern PWM slots to full after clearing pattern

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The commit message hallucinates details about the baseline code behavior, falsely claiming it previously restored Pattern0 to full duty cycle.

Pre-existing issues:
- [High] Clearing a pattern on one LED destructively halts the global sequencer and zeroes shared duration registers, breaking active patterns on all other channels.
- [High] The driver provides a non-blocking `brightness_set` callback that incorrectly performs sleeping operations, causing panics in atomic contexts.
- [Medium] Programming a shorter pattern fails to clear trailing sequence steps, causing leftover pattern steps to execute.
- [Medium] The user-provided 8-bit brightness is written directly to a 12-bit PWM register without scaling, causing patterns to be extremely dim.
--

commit 030b013dca5ca1bf862e53e8c7cfe3288cb86967
Author: Manuel Fombuena <fombuena@outlook.com>
Subject: leds: st1202: set all pattern PWM slots to full after clearing pattern

The commit alters st1202_led_pattern_clear() to set all 8 PWM slots
to ST1202_PATTERN_PWM_FULL. This aims to prevent unintended blinks
if another channel starts the global sequencer.

> pattern_clear() zeros all PWM registers then restores only Pattern0 to
> full duty cycle. Patterns 1-7 are left at zero.

[Severity: Medium]
Is this description of the previous behavior accurate?

The baseline code appears to iterate over all patterns and set them to
LED_OFF, without restoring Pattern0 to full duty cycle. Could the commit
message be updated to precisely reflect the original baseline behavior?

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index b4adade49898e..3203b3768ec52 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
[ ... ]
> @@ -205,7 +206,7 @@ static int st1202_led_pattern_clear(struct led_classdev *ldev)
>  		return ret;
>  
>  	for (int patt = 0; patt < ST1202_MAX_PATTERNS; patt++) {
> -		ret = st1202_pwm_pattern_write(chip, led->led_num, patt, LED_OFF);
> +		ret = st1202_pwm_pattern_write(chip, led->led_num, patt, ST1202_PATTERN_PWM_FULL);
>  		if (ret != 0)
>  			return ret;
>  

[Severity: High]
This is a pre-existing issue, but does clearing a pattern on one LED
destructively halt the global sequencer for all channels?

If a user configures a pattern on LED0, and then clears the pattern on LED1,
st1202_led_pattern_clear() writes to the global ST1202_CONFIG_REG:

    ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);

And then zeroes the shared ST1202_PATTERN_DUR registers:

    ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + patt, 0);

Since this patch now sets all PWM slots to ST1202_PATTERN_PWM_FULL for the
cleared channel, making it immune to the sequencer state, is it still
necessary to stop the sequencer and zero the shared durations in this
per-channel callback?


[Severity: High]
This is a pre-existing issue, but is it possible for the brightness_set
callback to perform sleeping operations in an atomic context?

The driver assigns st1202_brightness_set to the non-blocking callback
pointer in st1202_dt_init():

    led->led_cdev.brightness_set = st1202_brightness_set;

However, st1202_brightness_set() acquires a mutex which can sleep:

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

Since the LED core can call brightness_set from atomic contexts (like
softirqs used by the timer trigger), could this cause a scheduling while
atomic kernel panic? Would it be better to rely solely on
brightness_set_blocking for this driver?


[Severity: Medium]
This is a pre-existing issue, but would programming a shorter pattern fail
to clear the trailing sequence steps?

In st1202_led_pattern_set(), the loop terminates exactly at len:

    for (int patt = 0; patt < len; patt++) {

If a user programs a pattern of length 8, and later programs a shorter
pattern of length 4, the hardware will continue executing the leftover
non-zero steps from the previous pattern. Should the driver explicitly
zero the duration register of the len-th step to mark the end of the
sequence?


[Severity: Medium]
This is a pre-existing issue, but could the user-provided 8-bit brightness
be written directly to a 12-bit PWM register without scaling?

In st1202_led_pattern_set():

    ret = st1202_pwm_pattern_write(chip, led->led_num, patt,
                                   pattern[patt].brightness);

Since the 8-bit value (0-255) is written directly into the 12-bit register
(0-4095), the maximum possible pattern brightness is only about 6 percent of
the LED capability. Should this brightness value be scaled before writing it
to the hardware?

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