Re: [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Out-of-bounds array write in `lm3533_devices` when a DT child node matches multiple compatible strings.
- [High] The ALS driver expects the 'ti,resistor-value-ohms' property, but the DT binding defines 'ti,resistor-ohms'.
- [Medium] Skipped child nodes with invalid 'reg' properties will cause 'mfd_add_devices' to assign the wrong OF node to subsequent valid subdevices.
- [Medium] The check `if (!als->irq)` in `store_thresh_either_en` fails to handle negative error codes returned by `platform_get_irq_optional`, allowing invalid IRQ configuration.
- [Low] Incompatible pointer type passed to `device_property_read_u32()`, violating strict C typing.
--

commit 91427aa1400cfc0dbe1f634ca4391a5aa41811ec
Author: Svyatoslav Ryhel <clamor95@gmail.com>
Subject: mfd: lm3533: Convert to use OF bindings

This commit converts the LM3533 driver stack from using platform data to
using Device Tree bindings. It updates the core, als, backlight, and led
drivers to parse properties directly from the device hierarchy.

> diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> index 52136ca1abc94..55b35467a722d 100644
> --- a/drivers/iio/light/lm3533-als.c
> +++ b/drivers/iio/light/lm3533-als.c
[ ... ]
> @@ -716,30 +722,34 @@ static const struct attribute_group lm3533_als_attribute_group = {
>  	.attrs = lm3533_als_attributes
>  };
>  
> -static int lm3533_als_setup(struct lm3533_als *als,
> -			    const struct lm3533_als_platform_data *pdata)
> +static int lm3533_als_setup(struct lm3533_als *als)
>  {
[ ... ]
> -	if (!pdata->pwm_mode) {
> -		if (pdata->r_select < LM3533_ALS_RESISTOR_MIN ||
> -		    pdata->r_select > LM3533_ALS_RESISTOR_MAX) {
> -			dev_err(&als->pdev->dev, "invalid resistor value\n");
> -			return -EINVAL;
> -		}
> +	if (!als->pwm_mode) {
> +		ret = device_property_read_u32(dev, "ti,resistor-value-ohms",
> +					       &als->r_select);

[Severity: High]
Does this use the wrong property name? The device tree binding requires
"ti,resistor-ohms", but this attempts to read "ti,resistor-value-ohms". If a
conformant DT is provided, this read will fail and unconditionally abort the
driver probe when not in PWM mode.

> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to ger resistor value\n");
[ ... ]
> @@ -830,25 +833,28 @@ static int lm3533_als_probe(struct platform_device *pdev)
>  	indio_dev->channels = lm3533_als_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(lm3533_als_channels);
>  	indio_dev->name = dev_name(&pdev->dev);
> -	iio_device_set_parent(indio_dev, pdev->dev.parent);
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
>  	als = iio_priv(indio_dev);
>  	als->lm3533 = lm3533;
>  	als->pdev = pdev;
> -	als->irq = lm3533->irq;
> +	als->irq = platform_get_irq_optional(pdev, 0);
> +
> +	if (als->irq == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;

[Severity: Medium]
Since platform_get_irq_optional() returns a negative error code (like -ENXIO)
when an IRQ is absent, does the existing check in store_thresh_either_en()
need to be updated as well?

Because store_thresh_either_en() still checks:
    if (!als->irq)
        return -EBUSY;

A negative error code will bypass this check, allowing userspace to enable
the interrupt mode via sysfs without a registered interrupt handler.

> +
>  	atomic_set(&als->zone, 0);
>  	mutex_init(&als->thresh_mutex);
[ ... ]
> diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
> index 45e7f7481aa0f..d57c2cdf11c51 100644
> --- a/drivers/mfd/lm3533-core.c
> +++ b/drivers/mfd/lm3533-core.c
[ ... ]
> -static int lm3533_device_init(struct lm3533 *lm3533)
> +static int lm3533_device_init(struct lm3533 *lm3533)
>  {
[ ... ]
> +	lm3533_devices = devm_kcalloc(dev, nchilds, sizeof(*lm3533_devices),
> +				      GFP_KERNEL);
> +	if (!lm3533_devices)
> +		return -ENOMEM;
>  
> -	lm3533->have_leds = 1;
> +	device_for_each_child_node_scoped(lm3533->dev, child) {
> +		if (!fwnode_device_is_available(child))
> +			continue;
>  
> -	return 0;
> -}
> +		if (count >= nchilds)
> +			break;
>  
> -static int lm3533_device_init(struct lm3533 *lm3533)
> -{
> -	struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
> -	int ret;
> +		if (fwnode_device_is_compatible(child, "ti,lm3533-als")) {
> +			lm3533_devices[count].name = "lm3533-als";
> +			lm3533_devices[count].id = PLATFORM_DEVID_NONE;
> +			lm3533_devices[count].of_compatible = "ti,lm3533-als";
>  
> -	dev_dbg(lm3533->dev, "%s\n", __func__);
> +			lm3533->have_als = true;
> +			count++;
> +		}
>  
> -	if (!pdata) {
> -		dev_err(lm3533->dev, "no platform data\n");
> -		return -EINVAL;
> +		if (fwnode_device_is_compatible(child, "ti,lm3533-backlight")) {

[Severity: High]
Can a malformed device tree node cause a buffer overflow here? Since these
are independent if-statements rather than mutually exclusive else-if blocks,
a single node containing multiple matching compatible strings could execute
multiple blocks in the same iteration. Because the bounds check against nchilds
is only at the top of the loop, this would unconditionally increment count and
write out of bounds of the lm3533_devices array.

> +			ret = fwnode_property_read_u32(child, "reg", &reg);
> +			if (ret || reg >= LM3533_HVLED_ID_MAX) {
> +				dev_err(dev, "invalid backlight node %s\n",
> +					fwnode_get_name(child));
> +				continue;
> +			}
>  
> -	lm3533->hwen = devm_gpiod_get(lm3533->dev, NULL, GPIOD_OUT_LOW);
> -	if (IS_ERR(lm3533->hwen))
> -		return dev_err_probe(lm3533->dev, PTR_ERR(lm3533->hwen), "failed to request HWEN GPIO\n");
> -	gpiod_set_consumer_name(lm3533->hwen, "lm3533-hwen");
> -
> -	lm3533_enable(lm3533);
> +			lm3533_devices[count].name = "lm3533-backlight";
> +			lm3533_devices[count].id = reg;
> +			lm3533_devices[count].of_compatible = "ti,lm3533-backlight";

[Severity: Medium]
Will this assignment cause mfd_add_devices() to map the wrong OF node to
subdevices if an earlier node was skipped? Since cell->use_of_reg = true and
cell->of_reg = reg are not being set here, mfd_match_of_node_to_dev() will
simply assign the first available matching OF node to the first requested
mfd_cell. If an earlier invalid node was skipped, the subdevice may read
configurations from the wrong DT node.

>  
> -	ret = regmap_update_bits(lm3533->regmap, LM3533_REG_BOOST_PWM,
> -				 LM3533_BOOST_FREQ_MASK,
> -				 pdata->boost_freq << LM3533_BOOST_FREQ_SHIFT);
> +			lm3533->have_backlights = true;
> +			count++;
> +		}
[ ... ]
> diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
> index 5c8e690878609..c70b8a3bb3014 100644
> --- a/drivers/video/backlight/lm3533_bl.c
> +++ b/drivers/video/backlight/lm3533_bl.c
[ ... ]
> @@ -296,13 +294,20 @@ static int lm3533_bl_probe(struct platform_device *pdev)
>  	bl->cb.id = lm3533_bl_get_ctrlbank_id(bl);
>  	bl->cb.dev = NULL;			/* until registered */
>  
> +	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-%d",
> +			      pdev->name, pdev->id);
> +	if (!name)
> +		return -ENOMEM;
> +
>  	memset(&props, 0, sizeof(props));
>  	props.type = BACKLIGHT_RAW;
>  	props.max_brightness = LM3533_BL_MAX_BRIGHTNESS;
> -	props.brightness = pdata->default_brightness;
> -	bd = devm_backlight_device_register(&pdev->dev, pdata->name,
> -					pdev->dev.parent, bl, &lm3533_bl_ops,
> -					&props);
> +	props.brightness = LM3533_BL_MAX_BRIGHTNESS;
> +	device_property_read_u32(&pdev->dev, "default-brightness",
> +				 &props.brightness);

[Severity: Low]
Does passing &props.brightness violate strict C typing here? The brightness
field in struct backlight_properties is an int, but device_property_read_u32()
strictly expects a u32 pointer. This could cause build failures on modern
kernel trees that have -Werror=incompatible-pointer-types enabled.

> +
> +	bd = devm_backlight_device_register(&pdev->dev, name, &pdev->dev,
> +					    bl, &lm3533_bl_ops, &props);
>  	if (IS_ERR(bd)) {
>  		dev_err(&pdev->dev, "failed to register backlight device\n");
>  		return PTR_ERR(bd);
[ ... ]

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