Re: [PATCH v12 2/2] leds: ltc3220: Add Support for LTC3220 18 channel LED Driver

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] In aggregated LED mode, regcache_sync() during resume incorrectly turns off all LEDs due to the hardware QUICK_WRITE broadcast feature.
- [High] Missing bounds checks in the second pass of child node iteration leads to an out-of-bounds memory write if the device tree is modified concurrently.
- [Medium] The `pattern_set` callback silently ignores the `repeat` argument, breaking repeating LED patterns.
- [Medium] Setting LED_CORE_SUSPENDRESUME destructively interferes with hardware blink and gradation state restoration on resume.
--

commit f1453b8dca779cc861037537a42cea5f29931bc3
Author: Edelweise Escala <[email protected]>

leds: ltc3220: Add Support for LTC3220 18 channel LED Driver

Add driver for the LTC3220 18-channel LED driver
with I2C interface, individual brightness control, and hardware-assisted
blink/gradation features.

> diff --git a/drivers/leds/leds-ltc3220.c b/drivers/leds/leds-ltc3220.c
> new file mode 100644
> index 0000000000000..f8e1f4bcd5529
> --- /dev/null
> +++ b/drivers/leds/leds-ltc3220.c

[ ... ]

> +static int ltc3220_pattern_set(struct led_classdev *led_cdev,
> +			       struct led_pattern *pattern,
> +			       u32 len, int repeat)
> +{
> +	struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg,
> +							 led_cdev);

[Severity: Medium]
Does this code safely handle repeating patterns? 

If a user requests a repeating pattern (e.g. repeat = -1 for infinite 
looping) via sysfs, this function does not check the repeat parameter 
and unconditionally returns 0. 

By returning 0 instead of -EINVAL or -EOPNOTSUPP, the driver falsely 
claims it natively handles the requested repeating pattern. The LED core 
consequently skips setting up a software fallback timer, causing the LED 
to perform a single ramp and permanently stop instead of looping.

> +	struct ltc3220 *ltc3220 = container_of(uled_cfg - uled_cfg->led_index, struct ltc3220,
> +					       uled_cfg[0]);

[ ... ]

> +static int ltc3220_resume(struct device *dev)
> +{
> +	struct ltc3220 *ltc3220 = i2c_get_clientdata(to_i2c_client(dev));
> +	int ret;
> +
> +	ret = regmap_update_bits(ltc3220->regmap, LTC3220_COMMAND_REG,
> +				 LTC3220_SHUTDOWN_MASK, 0);
> +	if (ret)
> +		return ret;
> +
> +	usleep_range(10000, 12000);
> +
> +	return regcache_sync(ltc3220->regmap);
> +}

[Severity: High]
Will this regcache_sync() turn off all LEDs in aggregated LED mode?

During ltc3220_reset(), all 18 ULED registers are written to 0, populating 
the regmap cache with 0s for unused channels.

On resume, regcache_sync() writes all dirty registers in address order. 
It first restores COMMAND_REG, enabling the QUICK_WRITE_MASK. It then 
restores ULED_REG(0) to the active brightness, which broadcasts to all LEDs. 

However, it subsequently syncs ULED_REG(1) through ULED_REG(17) using their 
cached 0s. Because QUICK_WRITE is active, each of these writes broadcasts 
0 to all 18 LEDs, turning them off and overwriting the restored state.

[ ... ]

> +static int ltc3220_probe(struct i2c_client *client)
> +{

[ ... ]

> +		led->led_cdev.blink_set = ltc3220_blink_set;
> +		led->led_cdev.pattern_set = ltc3220_pattern_set;
> +		led->led_cdev.pattern_clear = ltc3220_pattern_clear;
> +		led->led_cdev.flags = LED_CORE_SUSPENDRESUME;
> +	}

[Severity: Medium]
Does setting this flag break hardware blink and gradation state 
restoration on resume?

Because the driver sets the LED_CORE_SUSPENDRESUME flag, the LED core forces 
the brightness to 0 on suspend. ltc3220_set_led_data() executes this by 
clearing the mode bits (LTC3220_NORMAL_MODE) in the regmap cache.

On resume, the driver correctly syncs the regmap cache, but it now contains 
the wiped mode state. Finally, the LED core restores the previous brightness, 
but does so via the basic setter, leaving the LED permanently in normal 
(solid) mode instead of restoring the hardware blink/pattern. 

Since the driver natively manages power management via chip shutdown, is 
opting into the LED core's power management overrides necessary?

[ ... ]

> +	/* Second pass: register LEDs after validation */
> +	device_for_each_child_node_scoped(&client->dev, child) {
> +		struct led_init_data init_data = {};
> +		struct ltc3220_uled_cfg *led;
> +		u32 source;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &source);
> +		if (ret)
> +			return ret;
> +
> +		init_data.fwnode = child;
> +		init_data.devicename = "ltc3220";
> +
> +		led_index = source - 1;
> +		led = &ltc3220->uled_cfg[led_index];

[Severity: High]
Can this sequence cause an out-of-bounds write to the uled_cfg array?

The first pass validates that the reg property is within 1 to 
LTC3220_NUM_LEDS. However, the second pass re-iterates the device's 
children via device_for_each_child_node_scoped(), re-reads the reg 
property into source, but skips validation. 

If a device tree overlay adds a new child node to the device concurrently 
with the ltc3220_probe() execution, source could be out of bounds or 0. 
The driver computes led_index = source - 1 and uses it to index 
&ltc3220->uled_cfg[led_index]. 

Passing this invalid pointer to devm_led_classdev_register_ext() would 
corrupt memory.

> +
> +		ret = devm_led_classdev_register_ext(&client->dev, &led->led_cdev, &init_data);
> +		if (ret)
> +			return dev_err_probe(&client->dev, ret, "Failed to register LED class\n");
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2