Re: [PATCH v5 2/2] leds: ltc3208: Add driver for LTC3208 Multidisplay LED Driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Duplicate `reg` property values in the device tree cause global `leds_list` corruption due to registering the exact same `led_classdev` object multiple times.
- [Medium] The `aux_channels` stack array can be left uninitialized if device tree properties fail to match due to errors other than `-EINVAL` (e.g., `-ENOENT` or `-ENODATA`), leading to uninitialized memory being used to configure the register.
- [Medium] Falsely successful probe and premature loop abortion when an invalid LED ID is encountered because `ret` is strictly 0.
--

commit b568fbfe98ac0a2fda2a035cd233d75353b8718c
Author: Jan Carlo Roleda <jancarlo.roleda@analog.com>
Subject: leds: ltc3208: Add driver for LTC3208 Multidisplay LED Driver

Kernel driver implementation for LTC3208 Multidisplay LED Driver.
The LTC3208 is a Multi-display LED driver, designed to control up to
7 distinct LED channels.

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

[ ... ]

> +	/* Initialize aux channel configurations */
> +	for (int i = 0; i < LTC3208_NUM_AUX_LEDS; i++) {
> +		ret = device_property_match_property_string(
> +			&client->dev, ltc3208_dt_aux_channels[i],
> +			ltc3208_aux_opt, LTC3208_NUM_AUX_OPT);
> +		/* Fallback to default value (AUX) if not found */
> +		if (ret == -EINVAL)
> +			aux_channels[i] = LTC3208_AUX_CHAN_AUX;
> +		else if (ret >= 0)
> +			aux_channels[i] = ret;
> +	}
> +
> +	reg_val = FIELD_PREP(LTC3208_AUX1_MASK, aux_channels[0]) |
> +		  FIELD_PREP(LTC3208_AUX2_MASK, aux_channels[1]) |
> +		  FIELD_PREP(LTC3208_AUX3_MASK, aux_channels[2]) |
> +		  FIELD_PREP(LTC3208_AUX4_MASK, aux_channels[3]);

[Severity: Medium]
What happens to aux_channels[i] if device_property_match_property_string()
returns an error code other than -EINVAL, such as -ENODATA?

It appears aux_channels[i] would remain uninitialized on the stack, and
this uninitialized memory would then be passed to FIELD_PREP() and
written to the LTC3208_REG_E_AUX_SELECT register.

Would it be safer to have a default catch-all else branch, or to fully
initialize the aux_channels array when it is declared?

[ ... ]

> +	device_for_each_child_node_scoped(&client->dev, child) {
> +		struct ltc3208_led *led;
> +		struct led_init_data init_data = {};
> +		u32 chan;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &chan);
> +		if (ret)
> +			return dev_err_probe(&client->dev, ret,
> +					    "Failed to get reg value of LED\n");
> +		else if (chan >= LTC3208_NUM_LED_GRPS)
> +			return dev_err_probe(&client->dev, ret,
> +					     "%d is an invalid LED ID\n", chan);

[Severity: Medium]
Does this return a success code of 0 instead of an error?

Because the preceding fwnode_property_read_u32() call succeeded, ret is
exactly 0 in this else if branch.

Calling dev_err_probe() with a 0 return code will cause the probe function
to abort iterating through the children but return a success status to the
driver core, masking the probe failure.

Should this return an explicit error like -EINVAL instead?

> +
> +		led = &ddata->leds[chan];
> +
> +		led->rfield =
> +			devm_regmap_field_alloc(&client->dev, ddata->regmap,
> +						ltc3208_led_reg_field[chan]);

[ ... ]

> +		ret = devm_led_classdev_register_ext(&client->dev, &led->cdev,
> +						     &init_data);

[Severity: High]
Can duplicate reg properties in the device tree corrupt the led_classdev
list?

If a malformed device tree provides multiple children with the same reg
value, the driver will reuse the exact same struct ltc3208_led object
from the ddata->leds array without checking if it is already in use.

Calling devm_led_classdev_register_ext() multiple times with the identical
&led->cdev pointer would cause the LED core to add the same node to the
global leds_list twice, corrupting the doubly-linked list.

Could we add a check to verify if the channel has already been populated
before proceeding with registration?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619-upstream-ltc3208-v5-0-075d18060606@analog.com?part=2