Re: [PATCH v1 6/6] video: leds: backlight: lm3533: Support getting LED sources from DT

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] An off-by-one bounds check skips parsing `led-sources` when exactly the maximum number of items is provided.
- [High] `lm3533_update()` uses static global masks, clobbering the configuration of LEDs belonging to other control banks.
- [High] LVLED4 and LVLED5 configuration is written to the wrong register (`LM3533_REG_OUTPUT_CONF1`), corrupting backlight settings.
- [Medium] Assigning the negative error code of `device_property_count_u32` to an unsigned integer circumvents error checks.
- [Medium] Unvalidated device tree array values are used as bit shift amounts, risking Undefined Behavior.
--

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

video: leds: backlight: lm3533: Support getting LED sources from DT

Add Control Bank to HVLED/LVLED muxing support based on the led-sources
defined in the device tree.

> diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
> --- a/drivers/leds/leds-lm3533.c
> +++ b/drivers/leds/leds-lm3533.c
[ ... ]
> @@ -639,7 +649,30 @@ static const struct attribute_group *lm3533_led_attribute_groups[] = {
>  
>  static int lm3533_led_setup(struct lm3533_led *led)
>  {
> -	int ret;
> +	u32 output_cfg_shift = 0;
> +	u32 output_cfg_val = 0;
> +	int ret, i;
> +
> +	if (led->num_leds) {
> +		for (i = 0; i < led->num_leds; i++) {
> +			output_cfg_shift = led->leds[i] * 2;

Are the values in led->leds[i] validated against hardware bounds before
being used to calculate the bit shift? 

If the device tree provides excessively large values, the shift amount could
exceed the integer bit width.

> +			output_cfg_val |= led->id << output_cfg_shift;
> +		}
> +
> +		/* LVLED1, LVLED2 and LVLED3 */
> +		ret = lm3533_update(led->lm3533, LM3533_REG_OUTPUT_CONF1,
> +				    output_cfg_val << OUTPUT_CONF1_SHIFT,
> +				    OUTPUT_CONF1_MASK);

Since OUTPUT_CONF1_MASK spans all LVLED1-3 bits and control banks are
probed sequentially, will this update clear the routing configurations of
previously probed banks?

> +		if (ret)
> +			return ret;
> +
> +		/* LVLED4 and LVLED5 */
> +		ret = lm3533_update(led->lm3533, LM3533_REG_OUTPUT_CONF1,

Is LM3533_REG_OUTPUT_CONF1 the correct register for LVLED4 and LVLED5?

Writing to LM3533_REG_OUTPUT_CONF1 with OUTPUT_CONF2_MASK clears its lower
bits, which might overwrite the hardware mapping for the high-voltage
backlight and LVLED1 settings instead of configuring LVLED4 and LVLED5.

> +				    output_cfg_val >> OUTPUT_CONF2_SHIFT,
> +				    OUTPUT_CONF2_MASK);
> +		if (ret)
> +			return ret;
> +	}
[ ... ]
> @@ -713,6 +746,24 @@ static int lm3533_led_probe(struct platform_device *pdev)
>  	led->pwm = 0;
>  	device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &led->pwm);
>  
> +	led->num_leds = device_property_count_u32(&pdev->dev, "led-sources");
> +
> +	/*
> +	 * If led-sources property is not set then either this Control Bank uses
> +	 * its default LVLED or is not linked to any LVLED at all.
> +	 */
> +	if (led->num_leds <= 0 || led->num_leds > LM3533_MAX_LEDS)

Does assigning the result of device_property_count_u32() directly to the
unsigned led->num_leds cause negative error codes like -EINVAL to wrap into
large positive values?

This causes the led->num_leds <= 0 check to fail. Should a signed integer
be used for the return value check before assigning it?

> +		led->num_leds = 0;
> +
> +	if (led->num_leds > 0 && led->num_leds < LM3533_MAX_LEDS) {

Does the strict inequality led->num_leds < LM3533_MAX_LEDS prevent the
driver from reading the led-sources array when exactly the maximum allowed
number of items is specified?

> +		ret = device_property_read_u32_array(&pdev->dev, "led-sources",
> +						     led->leds, led->num_leds);
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to get led-sources\n");
> +			goto err_deregister;
> +		}
> +	}
> +
[ ... ]
> diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
> --- a/drivers/video/backlight/lm3533_bl.c
> +++ b/drivers/video/backlight/lm3533_bl.c
[ ... ]
> @@ -248,7 +255,8 @@ static struct attribute_group lm3533_bl_attribute_group = {
>  static int lm3533_bl_setup(struct lm3533_bl *bl)
>  {
>  	int id = lm3533_bl_get_ctrlbank_id(bl);
> -	int ret;
> +	u32 output_cfg_val = 0;
> +	int ret, i;
>  
>  	ret = lm3533_update(bl->lm3533, LM3533_REG_CTRLBANK_AB_BCONF,
>  			    bl->linear ? CTRLBANK_AB_BCONF_MODE(id) : 0,
> @@ -256,6 +264,16 @@ static int lm3533_bl_setup(struct lm3533_bl *bl)
>  	if (ret)
>  		return ret;
>  
> +	if (bl->num_leds) {
> +		for (i = 0; i < bl->num_leds; i++)
> +			output_cfg_val |= id << bl->led_strings[i];
> +
> +		ret = lm3533_update(bl->lm3533, LM3533_REG_OUTPUT_CONF1,
> +				    output_cfg_val, OUTPUT_CONF1_MASK);

Similar to the LED setup, will applying OUTPUT_CONF1_MASK without preserving
existing bit states overwrite the routing of other control banks?

> +		if (ret)
> +			return ret;
> +	}
> +
[ ... ]
> @@ -337,6 +355,25 @@ static int lm3533_bl_probe(struct platform_device *pdev)
>  	bl->linear = device_property_read_bool(&pdev->dev,
>  					       "ti,linear-mapping-mode");
>  
> +	bl->num_leds = device_property_count_u32(&pdev->dev, "led-sources");
> +
> +	/*
> +	 * If led-sources property is not set then either this Control Bank uses
> +	 * its default HVLED or is not linked to any HVLED at all.
> +	 */
> +	if (bl->num_leds <= 0 || bl->num_leds > LM3533_MAX_LED_STRINGS)

As with the LED probe function above, does this unsigned variable wrapper
prevent catching negative error codes from device_property_count_u32()?

> +		bl->num_leds = 0;
> +
> +	if (bl->num_leds > 0 && bl->num_leds < LM3533_MAX_LED_STRINGS) {

Does this also skip array parsing if exactly LM3533_MAX_LED_STRINGS items
are provided?

> +		ret = device_property_read_u32_array(&pdev->dev, "led-sources",
> +						     bl->led_strings,
> +						     bl->num_leds);
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to get led-sources\n");
> +			goto err_sysfs_remove;
> +		}
> +	}
> +

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