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

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

New issues:
- [High] `st1202_blink_set` breaks all other active LEDs by destructively zeroing their PWM registers and clobbering global sequencer durations.
- [Medium] `st1202_blink_set` unconditionally forces maximum brightness, ignoring the user's configured brightness level.
- [Medium] Arithmetic overflow in delay calculations leads to clamping extremely large delays to the minimum duration instead of the maximum.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the driver incorrectly registers a sleeping function (`st1202_brightness_set`) as the non-blocking `brightness_set` callback, leading to 'scheduling while atomic' panics.
--

--- Patch [1]: [PATCH v2 1/1] leds: st1202: Add hardware-accelerated blink support ---
commit 0c45ca2d77a60a4118919a5f1d1afac3386caf4b
Author: Manuel Fombuena <[email protected]>

leds: st1202: Add hardware-accelerated blink support

Implement blink_set() to enable hardware-accelerated blinking via the
timer trigger. The LED1202 pattern engine is used to produce a two-step
sequence: full brightness for delay_on, off for delay_off, repeating
indefinitely.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 168df5ecf27b7..fff42fc164412 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c

[ ... ]

> @@ -275,6 +276,86 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
>  	return 0;
>  }
>  
> +static int st1202_blink_set(struct led_classdev *led_cdev,
> +			unsigned long *delay_on, unsigned long *delay_off)
> +{
> +	struct st1202_led *led = cdev_to_st1202_led(led_cdev);
> +	struct st1202_chip *chip = led->chip;
> +	unsigned long on, off;
> +	int ret;
> +
> +	on = *delay_on ?: ST1202_BLINK_DEFAULT_DELAY;
> +	off = *delay_off ?: ST1202_BLINK_DEFAULT_DELAY;
> +
> +	on = roundup(on, ST1202_MILLIS_PATTERN_DUR_MIN);
> +	off = roundup(off, ST1202_MILLIS_PATTERN_DUR_MIN);

[Severity: Medium]
Will this cause an integer overflow if userspace provides a very large delay
near ULONG_MAX for delay_on or delay_off?

The roundup() macro performs an addition of the value and the divisor minus
one. If the delay is extremely large, this addition can wrap around to a
small number, which then mathematically rounds to 0.

The subsequent clamp_val() will then erroneously clamp this 0 to the minimum
hardware delay (22ms) instead of the maximum.

Should the inputs be clamped before rounding?

> +	on = clamp_val(on, ST1202_MILLIS_PATTERN_DUR_MIN, ST1202_MILLIS_PATTERN_DUR_MAX);
> +	off = clamp_val(off, ST1202_MILLIS_PATTERN_DUR_MIN, ST1202_MILLIS_PATTERN_DUR_MAX);
> +
> +	guard(mutex)(&chip->lock);
> +
> +	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
> +	if (ret)
> +		return ret;
> +
> +	/* 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 inadvertently break independent channels that are configured to be
solid-ON?

This blindly zeroes the PWM slots of all other active LEDs. Since the
hardware sequencer is global, when it runs for the blinking LED, it applies
the 0 PWM to the solid-ON LEDs during steps 0 and 1, forcing them dark.

[ ... ]

> +	ret = st1202_write_reg(chip, ST1202_PATTERN_DUR,
> +				st1202_prescalar_to_miliseconds(on));
> +	if (ret)
> +		return ret;
> +	ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + 1,
> +				st1202_prescalar_to_miliseconds(off));
> +	if (ret)
> +		return ret;
> +
> +	for (int patt = 2; patt < ST1202_MAX_PATTERNS; patt++) {
> +		ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + patt, 0);

[Severity: High]
Are global duration registers unconditionally overwritten here without
tracking sequencer ownership or concurrency?

[ ... ]

> +	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]
Is this bypassing and corrupting the subsystem's tracked brightness state?

This unconditionally forces maximum brightness by writing U8_MAX to the
ST1202_ILED_REG0 register, completely ignoring any user's previously
configured brightness level.

Furthermore, since st1202_brightness_get() reads directly from this register,
does this permanently corrupt the internal sysfs state if the user had
configured a lower brightness before activating the blink trigger?

[ ... ]

> @@ -301,6 +382,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 brightness_set callback lead to scheduling while atomic panics?

The LED subsystem expects brightness_set to never sleep, calling it directly
from atomic softirq contexts for software-based timer triggers. 

Because st1202_brightness_set() acquires a mutex and performs I2C writes, it
will sleep in an atomic context.

Should this driver leave brightness_set unassigned and rely solely on
brightness_set_blocking to safely defer writes to a workqueue?

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