Re: [PATCH v7 6/9] leds: st1202: fix brightness having no effect while pattern mode is active

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

New issues:
- [Medium] The return value of `st1202_pwm_pattern_write` is ignored, resulting in missing error handling and I2C error log spam.

Pre-existing issues:
- [High] The driver assigns a sleeping function to the LED classdev `.brightness_set` callback, violating the atomic context constraint and exacerbating schedule-while-atomic bugs.
- [High] Per-channel pattern operations incorrectly overwrite global hardware state, breaking independent channel operation for all other active LEDs.
- [High] `st1202_led_pattern_set` does not scale the 8-bit brightness values to the 12-bit PWM register scale, resulting in extremely dim patterns.
- [High] Device node references fetched during `for_each_available_child_of_node_scoped` are not properly acquired, resulting in Use-After-Free when they are accessed later during LED registration.
--

commit c4f0635e8d1cc229cdb504a2e19927bd1b7b1d3d
Author: Manuel Fombuena <[email protected]>

leds: st1202: fix brightness having no effect while pattern mode is active

Once a hardware pattern is running (PATS=1), writing to the brightness
sysfs attribute only updates the ILED register. The visible output is
ILED x Pattern_PWM / 4095, so the change has little effect and the LED
never returns to steady static operation as the user expects.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index b8a5712829009..4909985ade04b 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -136,6 +136,8 @@ static void st1202_brightness_set(struct led_classdev *led_cdev,
>  
>  	guard(mutex)(&chip->lock);
>  
> +	for (int patt = 0; patt < ST1202_MAX_PATTERNS; patt++)
> +		st1202_pwm_pattern_write(chip, led->led_num, patt, ST1202_PATTERN_PWM_FULL);

[Severity: Medium]
Does this loop need to handle errors from st1202_pwm_pattern_write()? 

If the underlying I2C transaction fails, the function returns an error code,
but it is ignored here. This could result in multiple unnecessary writes and
I2C error log spam upon a hardware bus failure.

[Severity: High]
This is a pre-existing issue, but does assigning this function to the 
brightness_set callback cause schedule-while-atomic panics?

The LED core expects brightness_set to be safe to call from atomic context
(like the timer trigger for software blinking), but st1202_brightness_set()
acquires a mutex and performs sleeping I2C writes. Adding this loop of up
to 16 additional I2C writes inside the callback exacerbates the blocking
behavior.

Should this driver use brightness_set_blocking instead?

>  	st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, value);
>  }

[Severity: High]
This is a pre-existing issue, but does st1202_led_pattern_clear() break
patterns for all other active LEDs?

When clearing a pattern on a single channel, the code overwrites the global
config register and zeroes out the shared pattern duration array:

st1202_led_pattern_clear() {
        ...
        ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
        ...
        ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + patt, 0);
}

Since the LED1202 uses a single global sequencer and shared pattern duration 
registers, it seems this halts and corrupts running patterns on all other
channels.

[Severity: High]
This isn't a bug introduced by this patch, but does st1202_led_pattern_set()
fail to scale 8-bit brightness values?

The driver initializes led_cdev.max_brightness = U8_MAX, meaning the pattern
trigger passes brightness values in the range [0, 255]. However, these are
written directly to 12-bit PATTERN_PWM hardware registers:

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

Would this result in the patterns being visually very dim, as the maximum 
brightness of 255 only scales to roughly 6% duty cycle on the hardware?

[Severity: High]
This is a pre-existing issue, but does st1202_dt_init() cause a use-after-free
during LED registration?

The device node references fetched during the scoped iterator loop do not seem
to be properly acquired:

st1202_dt_init() {
        ...
        for_each_available_child_of_node_scoped(...) {
                led->fwnode = of_fwnode_handle(child);
        }
}

Because the scoped iterator drops its reference at the end of each iteration,
the raw pointer assigned to led->fwnode might drop to a zero refcount before 
devm_led_classdev_register_ext() is called later in probe.

Should this be using fwnode_handle_get() to safely increment the refcount?

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