Re: [PATCH 06/10] leds: rgb: ktd202x: Add Etek ET6326 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] leds: ktd202x: delay_on truncation to 0 in ktd202x_blink_set() on ET6326
--

commit 7c2c5ad9958f725ed6f4b50a7cb2208921d4f6b4
Author: Joseph Telaak <joseph@telaak.dev>

leds: rgb: ktd202x: Add Etek ET6326 support

This commit moves the LED count and flash period parameters into per-chip
match data and adds support for the Etek ET6326 LED controller.

Link: https://www.etekmicro.com/wp-content/uploads/datasheets/ET6326_datasheet.pdf

> diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
> index feea33b7e8856..67e65c1fcb5fb 100644
> --- a/drivers/leds/rgb/leds-ktd202x.c
> +++ b/drivers/leds/rgb/leds-ktd202x.c
[ ... ]
> @@ -86,6 +85,33 @@ static const struct reg_default ktd202x_reg_defaults[] = {
>  	{ KTD202X_REG_LED_IOUT(3), KTD202X_LED_CURRENT_10_mA },
>  };
>  
> +struct ktd202x_info {
> +	unsigned long num_leds;
> +	/* Flash period encoded by a Reg1 value of 0 */
> +	unsigned int flash_period_min_ms;
> +	unsigned int flash_period_max_steps;
> +};
[ ... ]
> +/* The ET6326 flash period is (Reg1 + 1) * 128 ms, up to 16.38 s */
> +static const struct ktd202x_info et6326_info = {
> +	.num_leds = KTD2026_NUM_LEDS,
> +	.flash_period_min_ms = 128,
> +	.flash_period_max_steps = 127,
> +};
[ ... ]
> @@ -300,11 +328,11 @@ static int ktd202x_blink_set(struct ktd202x_led *led, unsigned long *delay_on,
>  	/* Integer representation of time of flash period */
>  	if (check_add_overflow(*delay_on, *delay_off, &delay_total_ms))
>  		delay_total_ms = ULONG_MAX;
> -	if (delay_total_ms < KTD202X_FLASH_PERIOD_MIN_MS)
> +	if (delay_total_ms < info->flash_period_min_ms)
>  		num_steps = 0;
>  	else
> -		num_steps = min_t(unsigned long, KTD202X_FLASH_PERIOD_MAX_STEPS,
> -				  (delay_total_ms - KTD202X_FLASH_PERIOD_MIN_MS) /
> +		num_steps = min_t(unsigned long, info->flash_period_max_steps,
> +				  (delay_total_ms - info->flash_period_min_ms) /
>  				  KTD202X_FLASH_PERIOD_STEP_MS);
>  
>  	/*
> @@ -315,7 +343,7 @@ static int ktd202x_blink_set(struct ktd202x_led *led, unsigned long *delay_on,
>  	on = clamp(on, 1, KTD202X_FLASH_ON_MAX - 1);
>  
>  	/* Actually used delay_{on,off} values */
> -	delay_total_ms = num_steps * KTD202X_FLASH_PERIOD_STEP_MS + KTD202X_FLASH_PERIOD_MIN_MS;
> +	delay_total_ms = num_steps * KTD202X_FLASH_PERIOD_STEP_MS + info->flash_period_min_ms;
>  	*delay_on = (delay_total_ms * on) / KTD202X_FLASH_ON_MAX;

[Severity: Medium]
Can this integer division truncate *delay_on to 0 on the ET6326?

On the KTD2026 and KTD2027, flash_period_min_ms is 256, which matches
KTD202X_FLASH_ON_MAX (256), so (delay_total_ms * on) / KTD202X_FLASH_ON_MAX
is always at least 1 when on is clamped to a minimum of 1.

With et6326_info setting flash_period_min_ms to 128, if a user configures
a short period where num_steps is 0 (delay_total_ms < 256 ms) and on is
clamped to 1 (for example, writing delay_on = 1 and delay_off = 127 via
the timer trigger), ktd202x_blink_set() computes:

	delay_total_ms = 0 * 128 + 128;
	*delay_on = (128 * 1) / 256;

which sets *delay_on to 0 while still programming the hardware to blink
with on = 1.

When led_blink_setup() stores blink_delay_on = 0, any subsequent write to
delay_off in sysfs calls led_delay_off_store() -> led_blink_set() with
*delay_on == 0, hitting the !*delay_on check in ktd202x_blink_single_set()
or ktd202x_blink_mc_set():

	/* Never on - just set to off */
	if (!*delay_on)
		return ktd202x_brightness_single_set(cdev, LED_OFF);

and turning the LED off instead of updating the blink period, which breaks
the invariant from commit fb5b897c7d3f ("leds: rgb: ktd202x: Never program
an on time of zero").

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101-ktd202x-et6326-v1-0-eb51eddad08c@telaak.dev?part=6