Re: [PATCH v8 5/6] leds: is31fl32xx: Fix missing brightness_steps for is31fl3236

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing IS31FL32XX_REG_NONE initialization for output_frequency_setting_reg in is31fl3293_cdef leads to arbitrary register corruption.
- [High] I2C writes are sent to a powered-down hardware device during driver removal.

Pre-existing issues:
- [High] Incorrect bound checking and inverted logic for current limit calculation ignores DT-specified safety limits and causes register overflow.
- [High] LED class devices are registered and exposed to userspace before the hardware is initialized and taken out of reset.
--

commit 19aea3a466357ecc0f41f0475513b5da802e6488
Author: Jun Yan <jerrysteve1101@gmail.com>

leds: is31fl32xx: Fix missing brightness_steps for is31fl3236

This commit adds the missing brightness_steps value for the is31fl3236
LED driver to fix brightness control issues.

> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index f9d3b9a707ca5..0c811aebcbcfe 100644
> --- a/drivers/leds/leds-is31fl32xx.c
> +++ b/drivers/leds/leds-is31fl32xx.c
> @@ -492,6 +492,7 @@ static const struct is31fl32xx_chipdef is31fl3236_cdef = {
>  	.pwm_register_base			= 0x01,
>  	.led_control_register_base		= 0x26,
>  	.enable_bits_per_led_control_register	= 1,
> +	.brightness_steps			= 256,
>  };
>  
>  static const struct is31fl32xx_chipdef is31fl3236a_cdef = {

While reviewing this file, I also noticed a few other regressions in the same
driver:

drivers/leds/leds-is31fl32xx.c:is31fl3293_reset() {
...
	for (i = 0; i < priv->num_leds; i++) {
		struct is31fl32xx_led_data *led_data = &priv->leds[i];
		int current_level_reg = IS31FL3293_CL_REG + led_data->channel - 1;
		int microamp = max(led_data->max_microamp, IS31FL3293_MAX_MICROAMP);
		int current_level = (microamp * 0xff) / IS31FL3293_MAX_MICROAMP;

		ret = is31fl32xx_write(priv, current_level_reg, current_level);
...
}

[Severity: High]
This isn't a bug introduced by this patch, but does this calculation handle the
current limit bounds correctly?

It appears that max(led_data->max_microamp, IS31FL3293_MAX_MICROAMP) forces
microamp to be at least 20,000 (the value of IS31FL3293_MAX_MICROAMP). If a
device tree requires a lower safety limit (e.g., 5000), it seems it would be
ignored.

Conversely, if the device tree specifies a value larger than 20,000 (e.g.,
30,000), the calculated current_level evaluates to 382. When passed to
is31fl32xx_write(), which takes a u8 parameter, won't this overflow and
truncate to 126, silently programming a drastically lower current limit?


drivers/leds/leds-is31fl32xx.c:is31fl32xx_probe() {
...
	ret = is31fl32xx_parse_dt(dev, priv);
	if (ret)
		return ret;

	ret = is31fl32xx_init_regs(priv);
...
}

[Severity: High]
This is a pre-existing issue, but could this ordering lead to a race condition
with userspace?

is31fl32xx_parse_dt() calls devm_led_classdev_register_ext(), which
immediately publishes the LED devices. The LED subsystem might apply
default-state triggers, or userspace might set the brightness, which sends
PWM writes to the device over I2C.

However, the hardware registers are only initialized and reset in
is31fl32xx_init_regs() afterwards. Won't this reset wipe out any brightness
state configured during or immediately after LED registration? Should the
hardware be fully initialized before device registration?


drivers/leds/leds-is31fl32xx.c:
...
static const struct is31fl32xx_chipdef is31fl3293_cdef = {
	.channels				= 3,
	.shutdown_reg				= IS31FL32XX_REG_NONE,
	.pwm_update_reg				= 0x28,
	.pwm_update_value			= 0xc5,
	.global_control_reg			= IS31FL32XX_REG_NONE,
	.reset_reg				= IS31FL32XX_REG_NONE,
	.pwm_register_base			= 0x19,
	.led_control_register_base		= IS31FL32XX_REG_NONE,
	.brightness_steps			= 4096,
	.reset_func				= is31fl3293_reset,
	.sw_shutdown_func			= is31fl3293_software_shutdown,
};
...

[Severity: High]
Is is31fl3293_cdef missing the initialization for output_frequency_setting_reg?

Since this field isn't set to IS31FL32XX_REG_NONE, it is implicitly initialized
to 0x00. During probe, is31fl32xx_init_regs() checks if this field is
!= IS31FL32XX_REG_NONE (which is 0xFF). Since 0x00 != 0xFF, if the DT
contains the issi,22khz-pwm property, the driver will erroneously write 0x01
to register 0x00.

For the IS31FL3293, register 0x00 is undefined. Can this silently corrupt the
chip state?


drivers/leds/leds-is31fl32xx.c:is31fl32xx_remove() {
...
static void is31fl32xx_remove(struct i2c_client *client)
{
	struct is31fl32xx_priv *priv = i2c_get_clientdata(client);
	int ret;

	ret = is31fl32xx_reset_regs(priv);
...
	if (priv->powerdown_gpio)
		gpiod_set_value_cansleep(priv->powerdown_gpio, 1);
}
...

[Severity: High]
Will the device acknowledge I2C writes after being powered down during removal?

In is31fl32xx_remove(), the driver asserts powerdown_gpio to place the chip
into a hardware shutdown mode. After remove() completes, devres automatically
unregisters the LED class devices that were allocated in is31fl32xx_parse_dt().

During unregistration, the LED subsystem turns off the LEDs by calling
is31fl32xx_brightness_set(), which attempts to write to the chip over I2C.
Because the chip is already physically powered down, won't this result in I2C
timeouts or EREMOTEIO errors on every driver removal?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260607152002.446617-1-jerrysteve1101@gmail.com?part=5