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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Probe returns 0 on failure during LED sources validation, causing a double-free on removal.
- [High] Out-of-bounds bitmask calculation for LM3533_REG_OUTPUT_CONF1 corrupts regcache and causes probe failures on SMBus.
- [Medium] Probe returns a positive integer on failure, violating the driver core API and generating an incorrect error code.
- [Low] Negative error codes from missing properties are evaluated as true, causing redundant regmap operations.
--

commit a17954bc9b86db17d86986ace3623a64fc46dd3b
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 a661aedcdc60a..fe3e01b265e10 100644
> --- a/drivers/leds/leds-lm3533.c
> +++ b/drivers/leds/leds-lm3533.c
[ ... ]
> @@ -623,7 +631,35 @@ 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;
> +	u32 output_cfg_mask = 0;
> +	int ret, i;
> +
> +	if (led->num_leds) {

[Severity: Low]
If the led-sources property is not present, device_property_count_u32()
returns a negative error code. Does this cause the condition to evaluate
to true, skipping the loop but triggering an unnecessary regmap_update_bits()
with a 0 mask?

> +		for (i = 0; i < led->num_leds; i++) {
> +			if (led->leds[i] >= LM3533_LVCTRLBANK_MAX)
> +				continue;
> +
> +			output_cfg_shift = led->leds[i] * 2;
> +			output_cfg_val |= led->id << output_cfg_shift;
> +			output_cfg_mask |= OUTPUT_LVLED_MASK << output_cfg_shift;
> +		}
> +
> +		/* LVLED1, LVLED2 and LVLED3 */
> +		ret = regmap_update_bits(led->regmap, LM3533_REG_OUTPUT_CONF1,
> +					 output_cfg_mask << OUTPUT_CONF1_SHIFT,
> +					 output_cfg_val << OUTPUT_CONF1_SHIFT);

[Severity: High]
For LVLED4 and LVLED5, output_cfg_mask could have bits 6 or 8 set. When
shifted left by OUTPUT_CONF1_SHIFT (2), the mask exceeds 8 bits. Since
LM3533_REG_OUTPUT_CONF1 is an 8-bit register, does this corrupt the regmap
cache or cause SMBus probe failures by passing a mask larger than 0xFF?

> +		if (ret)
> +			return ret;
> +
> +		/* LVLED4 and LVLED5 */
> +		ret = regmap_update_bits(led->regmap, LM3533_REG_OUTPUT_CONF2,
> +					 output_cfg_mask >> OUTPUT_CONF2_SHIFT,
> +					 output_cfg_val >> OUTPUT_CONF2_SHIFT);
> +		if (ret)
> +			return ret;
> +	}
[ ... ]
> @@ -700,6 +736,26 @@ 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->num_leds > LM3533_LVCTRLBANK_MAX) {
> +		dev_err(&pdev->dev, "num of LED sources exceeds max %d: %d\n",
> +			LM3533_LVCTRLBANK_MAX, led->num_leds);
> +		goto err_deregister;

[Severity: High]
Since led_classdev_register() was already called earlier in this function and
returned 0, ret is still 0 here. Will jumping to err_deregister return 0 on
failure, leading to a double-free on driver removal when
led_classdev_unregister() is called again and corrupting leds_list?

> +	}
[ ... ]
> diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
> index 2aa4b86042ff2..ed358fb3c7596 100644
> --- a/drivers/video/backlight/lm3533_bl.c
> +++ b/drivers/video/backlight/lm3533_bl.c
[ ... ]
> @@ -232,13 +237,30 @@ static struct attribute_group lm3533_bl_attribute_group = {
>  static int lm3533_bl_setup(struct lm3533_bl *bl)
>  {
>  	int ctrlbank = lm3533_bl_get_ctrlbank_id(bl);
> -	int ret;
> +	u32 output_cfg_val = 0;
> +	u32 output_cfg_mask = 0;
> +	int ret, i;
>  
>  	ret = regmap_assign_bits(bl->regmap, LM3533_REG_CTRLBANK_AB_BCONF,
>  				 CTRLBANK_AB_BCONF_MODE(ctrlbank), bl->linear);
>  	if (ret)
>  		return ret;
>  
> +	if (bl->num_leds) {

[Severity: Low]
Similar to lm3533_led_setup(), if the led-sources property is absent,
device_property_count_u32() returns a negative error code. Will this cause
the condition to evaluate to true and trigger an unnecessary I2C write with
a 0 mask?

> +		for (i = 0; i < bl->num_leds; i++) {
> +			if (bl->led_strings[i] >= LM3533_HVCTRLBANK_COUNT)
> +				continue;
[ ... ]
> @@ -316,6 +338,25 @@ static int lm3533_bl_probe(struct platform_device *pdev)
>  
>  	device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &bl->pwm);
>  
> +	bl->num_leds = device_property_count_u32(&pdev->dev, "led-sources");
> +	if (bl->num_leds > LM3533_HVCTRLBANK_COUNT)
> +		return dev_err_probe(&pdev->dev, bl->num_leds,
> +				     "num of LED sources exceeds max %d\n",
> +				     LM3533_HVCTRLBANK_COUNT);

[Severity: Medium]
Because bl->num_leds is strictly positive here, does returning it via
dev_err_probe() violate the driver core API which expects a negative error
code? The driver core negates positive return values, which could result in an
unrelated error code like -ESRCH.

> +
> +	/*

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