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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Hardware-accelerated blink destructively alters the physical state of all other active LEDs by zeroing their PWM slots.
- [Medium] Hardware blink overrides user-configured brightness by hardcoding maximum intensity.
- [Medium] Updating LED brightness indiscriminately breaks an active hardware blink configuration.

Pre-existing issues:
- [High] The driver incorrectly assigns a sleeping function to the non-blocking `brightness_set` callback, causing 'scheduling while atomic' panics.
- [High] Dangling `fwnode` pointers due to un-incremented scoped device node reference.
--

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

leds: st1202: Add hardware-accelerated blink support

Implement blink_set() to enable hardware-accelerated blinking via the timer
trigger. Requested delays are clamped to the hardware range and the LED1202
pattern sequencer is configured globally.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
[ ... ]
> +static int st1202_blink_set(struct led_classdev *led_cdev,
> +			unsigned long *delay_on, unsigned long *delay_off)
> +{
[ ... ]
> +	/* 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);
> +		if (ret)
> +			return ret;
> +		ret = st1202_pwm_pattern_write(chip, i, 1, LED_OFF);
> +		if (ret)
> +			return ret;
> +	}

[Severity: High]
Does this loop destructively alter the physical state of all other active
LEDs?

Because the LED1202 hardware sequencer is a global resource, starting it
forces all channels to execute the sequence. Any LED that was previously set
to steady ON will output zero during these steps, causing it to go dark
while the kernel subsystem still believes it to be on.

> +
> +	ret = st1202_pwm_pattern_write(chip, led->led_num, 0, ST1202_PATTERN_PWM_FULL);

[Severity: Medium]
Can this override the user-configured brightness by hardcoding maximum
intensity?

The PWM slot is being forced to ST1202_PATTERN_PWM_FULL, and later the ILED
register is forced to U8_MAX, completely disregarding the
led_cdev->brightness value.

[ ... ]
> @@ -301,6 +380,7 @@ static int st1202_dt_init(struct st1202_chip *chip)
[ ... ]
>  		led = &chip->leds[reg];
>  		led->is_active = true;
>  		led->fwnode = of_fwnode_handle(child);

[Severity: High]
This is a pre-existing issue, but could this leave led->fwnode with a
dangling pointer?

The driver stores a pointer to this node from the scoped
for_each_available_child_of_node_scoped() loop, but fails to increment
its reference count via fwnode_handle_get(). When the scoped loop
drops the reference, the node may be freed, causing a use-after-free
when accessing the fwnode during class device registration or subsystem
operations.

>  
>  		led->led_cdev.max_brightness = U8_MAX;
>  		led->led_cdev.brightness_set_blocking = st1202_led_set;
>  		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;

[Severity: Medium]
Would updating the LED brightness indiscriminately break an active hardware
blink configuration?

If a user changes the brightness while the LED is hardware-blinking,
st1202_brightness_set() unconditionally forces all 8 PWM pattern slots to
ST1202_PATTERN_PWM_FULL:

st1202_brightness_set() {
        ...
        for (int patt = 0; patt < ST1202_MAX_PATTERNS; patt++)
                st1202_pwm_pattern_write(chip, led->led_num, patt, 
                                         ST1202_PATTERN_PWM_FULL);
        ...
}

This overwrites the OFF step established by st1202_blink_set(), turning the
LED steadily ON while the subsystem still considers it to be blinking.

>  		led->led_cdev.brightness_set = st1202_brightness_set;

[Severity: High]
This is a pre-existing issue, but is it safe to assign a sleeping function
to the non-blocking brightness_set callback? 

st1202_brightness_set() acquires a mutex and performs sleepable I2C
operations:

st1202_brightness_set() {
        ...
        guard(mutex)(&chip->lock);
        ...
        st1202_write_reg(...)
}

The LED core assumes led_cdev.brightness_set does not sleep, calling it
directly from atomic contexts. This can cause scheduling while atomic
panics.

>  		led->led_cdev.brightness_get = st1202_brightness_get;
>  	}
>  
>  	return 0;
>  }

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