Re: [PATCH v2 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] Hardcoded register masks clobber sibling device configurations.
- [Medium] Missing bounds validation on device tree shift values causes undefined behavior.
- [Medium] Premature sysfs group removal in error path triggers kernel warnings.
- [Low] Incorrect boundary check allows an out-of-bounds LVLED index.
- [Low] Improper handling of negative error codes from device property count via unsigned wrapping.
--

commit e5414513aab84157f65b5403f08affee4717c3cb
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
> index d707d43d55260..07390bba9a48c 100644
> --- a/drivers/leds/leds-lm3533.c
> +++ b/drivers/leds/leds-lm3533.c
[ ... ]
> @@ -639,7 +649,33 @@ 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++) {
> +			if (led->leds[i] > LM3533_LVCTRLBANK_MAX)

[Severity: Low]
Is this boundary check correct? LM3533_LVCTRLBANK_MAX appears to be 5, so
using greater-than allows an index of 5 to pass.

Since there are only 5 LV LEDs mapped to indices 0 through 4, would an index
of 5 result in calculating a bit shift for a non-existent LVLED6?

> +				continue;
> +
> +			output_cfg_shift = led->leds[i] * 2;
> +			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);

[Severity: High]
If a mask like OUTPUT_CONF1_MASK is used for lm3533_update() and it covers all
LEDs, will it unconditionally clear the configuration bits for any LEDs not
assigned to the current bank?

For example, if Bank 0 configures one LED and then Bank 1 configures another,
does the second update overwrite and clear the first one's configuration since
the mask is static? Should the register mask be dynamically generated based on
the LEDs actually being configured?

> +		if (ret)
> +			return ret;
> +
> +		/* LVLED4 and LVLED5 */
> +		ret = lm3533_update(led->lm3533, LM3533_REG_OUTPUT_CONF2,
> +				    output_cfg_val >> OUTPUT_CONF2_SHIFT,
> +				    OUTPUT_CONF2_MASK);
> +		if (ret)
> +			return ret;
> +	}
[ ... ]
> diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
> index c03d0d1667e42..82b46a531dd27 100644
> --- a/drivers/video/backlight/lm3533_bl.c
> +++ b/drivers/video/backlight/lm3533_bl.c
[ ... ]
> @@ -248,7 +254,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 +263,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];

[Severity: Medium]
Since the values from bl->led_strings arrays are read directly from the device
tree property without bounds checking, could a malformed device tree provide a
value of 32 or greater and cause an out-of-bounds bitwise shift here?

> +
> +		ret = lm3533_update(bl->lm3533, LM3533_REG_OUTPUT_CONF1,
> +				    output_cfg_val, OUTPUT_CONF1_MASK);
> +		if (ret)
> +			return ret;
> +	}
[ ... ]
> @@ -329,6 +346,24 @@ 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");

[Severity: Low]
Looking at the struct definition, bl->num_leds is declared as a u32.

If device_property_count_u32() returns a negative error code like -EINVAL,
does that get implicitly cast to a very large positive unsigned integer?

The subsequent bounds check would safely fail, but could this be avoided by
using an int for the count, similar to what is done in lm3533_led_probe()?

> +
> +	/*
> +	 * 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_HVCTRLBANK_COUNT) {
> +		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;

[Severity: Medium]
If device_property_read_u32_array() fails, this jumps to err_sysfs_remove.

Looking at lm3533_bl_probe(), sysfs_create_group() is called later in the
probe sequence.

Does calling sysfs_remove_group() on an uncreated sysfs group here trigger a
kernel warning? Should the error path at this stage simply return the error
code instead?

> +		}
> +	} else {
> +		bl->num_leds = 0;
> +	}

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