Re: [PATCH v2 3/3] leds: aat1290: Add support for AAT1270 Current Regulator for Flash LEDs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] leds: aat1290: division by zero due to unvalidated flash-max-timeout-us
- [Medium] leds: aat1290: missing T_OFF reset pulse for AAT1270 SSC protocol
--

commit edf7870e4592e0289f95e937e9de77118e50731c
Author: Svyatoslav Ryhel <clamor95@gmail.com>

leds: aat1290: Add support for AAT1270 Current Regulator for Flash LEDs

The AAT1270 is quite similar to the AAT1290, but it does not support the
Advanced Simple Serial Control (AS2C) used in the AAT1290. The AAT1270
uses an earlier version of AS2C, namely Simple Serial Control, which lacks
address-sending capabilities.

> diff --git a/drivers/leds/flash/leds-aat1290.c b/drivers/leds/flash/leds-aat1290.c
> --- a/drivers/leds/flash/leds-aat1290.c
> +++ b/drivers/leds/flash/leds-aat1290.c
> @@ -101,15 +113,17 @@ static void aat1290_as2cwire_write(struct aat1290_led *led, int addr, int value)
>  
>  	udelay(AAT1290_FLEN_OFF_DELAY_TIME_US);

[Severity: Medium]
Is this delay sufficient to reset the AAT1270? The EN/SET pin is pulled
low for only 10us (AAT1290_FLEN_OFF_DELAY_TIME_US) before sending pulses.
For the AAT1270 SSC protocol, wouldn't a T_OFF duration of >500us be
required to reset the internal counter to 0?

If a user changes the brightness of the AAT1270 LED via sysfs while it is
already on, the driver directly calls aat1290_as2cwire_write() without fully
turning the device off first. Will the Simple Serial Control counter fail
to reset, causing new pulses to accumulate or be ignored?

> -	/* write address */
> -	for (i = 0; i < addr; ++i) {
> -		udelay(AAT1290_EN_SET_TICK_TIME_US);
> -		gpiod_direction_output(led->gpio_en_set, 0);
> -		udelay(AAT1290_EN_SET_TICK_TIME_US);
> -		gpiod_direction_output(led->gpio_en_set, 1);
> -	}
> +	if (led->info->as2c) {
> +		/* write address */
> +		for (i = 0; i < addr; ++i) {
> +			udelay(AAT1290_EN_SET_TICK_TIME_US);
> +			gpiod_direction_output(led->gpio_en_set, 0);
> +			udelay(AAT1290_EN_SET_TICK_TIME_US);
> +			gpiod_direction_output(led->gpio_en_set, 1);
> +		}

[ ... ]

> @@ -360,7 +379,12 @@ static void aat1290_init_flash_timeout(struct aat1290_led *led,
>  
>  	/* Init flash timeout setting */
>  	setting = &fled_cdev->timeout;
> -	setting->min = cfg->max_flash_tm / AAT1290_FLASH_TM_NUM_LEVELS;
> +
> +	if (!led->info->fixed_timeout)
> +		setting->min = cfg->max_flash_tm / AAT1290_FLASH_TM_NUM_LEVELS;
> +	else
> +		setting->min = cfg->max_flash_tm;
> +
>  	setting->max = cfg->max_flash_tm;
>  	setting->step = setting->min;

[Severity: High]
Could this lead to a division by zero in led_clamp_align()?

If the device tree provides a flash-max-timeout-us less than 16 for the
AAT1290, or 0 for the AAT1270, setting->min evaluates to 0. Since
setting->step is assigned setting->min right below this, step also becomes 0.

When user-space writes to the flash_timeout sysfs attribute or triggers a
strobe, will led_clamp_align() compute v % s->step and crash the kernel?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013181803.496627-1-clamor95@gmail.com?part=3