Re: [PATCH v1 2/3] mfd: aat2870: Convert to use OF bindings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] regulator: Missing DT Configuration for aat2870
- [High] regulator: Global state corruption causing UAF via of_regulator_match
- [High] backlight: Logic error scales max_current to zero due to unit mismatch
- [High] backlight: Missing default initialization causes max_current truncation to zero
- [High] mfd: Resource Leak and UAF due to unmanaged of_platform_populate
- [High] backlight: Divide by zero in aat2870_brightness()
- [Low] backlight: Type mismatch passing int pointer to u32 reader
--

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

mfd: aat2870: Convert to use OF bindings

This commit converts the AAT2870 driver to use OF bindings, replacing the
MFD registration API with of_platform_populate(). It updates the backlight
and regulator child drivers to parse device tree properties directly and
removes the legacy platform data initialization.

> diff --git a/drivers/mfd/aat2870-core.c b/drivers/mfd/aat2870-core.c
> --- a/drivers/mfd/aat2870-core.c
> +++ b/drivers/mfd/aat2870-core.c
[ ... ]
> @@ -348,60 +309,29 @@ static int aat2870_i2c_probe(struct i2c_client *client)
[ ... ]
> -	ret = mfd_add_devices(aat2870->dev, 0, aat2870_devs,
> -			      ARRAY_SIZE(aat2870_devs), NULL, 0, NULL);
> -	if (ret != 0) {
> -		dev_err(aat2870->dev, "Failed to add subdev: %d\n", ret);
> -		goto out_disable;
> +	gpiod_set_value(aat2870->en_pin, 1);
>  
> -	}
> +	ret = of_platform_populate(client->dev.of_node, NULL, NULL, &client->dev);
> +	if (ret) {

[Severity: High]
Are we missing an of_platform_depopulate() call here?

Since of_platform_populate() creates unmanaged child devices, if the parent
I2C driver unbinds, the child devices will remain active. If a child device
is accessed after the parent is removed, wouldn't it result in a
use-after-free when dereferencing the freed parent driver data? Using
devm_of_platform_populate() might resolve this leak.

> +		gpiod_set_value(aat2870->en_pin, 0);
> +		return dev_err_probe(&client->dev, ret, "Failed to populate cells\n");
>  	}
>  
>  	aat2870_init_debugfs(aat2870);
[ ... ]
> diff --git a/drivers/regulator/aat2870-regulator.c b/drivers/regulator/aat2870-regulator.c
> --- a/drivers/regulator/aat2870-regulator.c
> +++ b/drivers/regulator/aat2870-regulator.c
[ ... ]
> @@ -121,6 +130,13 @@ static struct aat2870_regulator aat2870_regulators[] = {
>  	AAT2870_LDO(LDOD),
>  };
>  
> +static struct of_regulator_match aat2870_regulator_matches[] = {
> +	{ .name = "ldo-a" },
> +	{ .name = "ldo-b" },
> +	{ .name = "ldo-c" },
> +	{ .name = "ldo-d" },
> +};

[Severity: High]
Does of_regulator_match() mutate this static array?

of_regulator_match() typically writes device-specific of_node and init_data
pointers into the matched array. If multiple AAT2870 devices are probed,
wouldn't the second probe overwrite the pointers from the first, leading to
data corruption and a potential use-after-free during devres cleanup?

> +
>  static struct aat2870_regulator *aat2870_get_regulator(int id)
>  {
[ ... ]
> @@ -152,33 +167,50 @@ static int aat2870_regulator_probe(struct platform_device *pdev)
[ ... ]
> +	for (int idx = 0; idx < ARRAY_SIZE(aat2870_regulator_matches); idx++) {
> +		if (!aat2870_regulator_matches[idx].of_node)
> +			continue;
>  
> -		return PTR_ERR(rdev);
> +		ri = aat2870_get_regulator(idx);
> +		if (!ri)
> +			return dev_err_probe(&pdev->dev, -EINVAL,
> +					     "Invalid device ID, %d\n", idx);
> +
> +		ri->aat2870 = dev_get_drvdata(pdev->dev.parent);
> +
> +		config.dev = &pdev->dev;
> +		config.driver_data = ri;
> +
> +		rdev = devm_regulator_register(&pdev->dev, &ri->desc, &config);

[Severity: High]
Is there a missing assignment for the Device Tree configuration?

of_regulator_match() parses the DT constraints into the match array, but
config.init_data and config.of_node don't seem to be populated from
aat2870_regulator_matches[idx] before calling devm_regulator_register().
Without these, do the regulators get registered without their DT constraints?

> +		if (IS_ERR(rdev))
> +			return dev_err_probe(&pdev->dev, PTR_ERR(rdev),
> +					     "Failed to register regulator %s\n",
> +					     ri->desc.name);
>  	}
[ ... ]
> diff --git a/drivers/video/backlight/aat2870_bl.c b/drivers/video/backlight/aat2870_bl.c
> --- a/drivers/video/backlight/aat2870_bl.c
> +++ b/drivers/video/backlight/aat2870_bl.c
[ ... ]
> @@ -140,20 +135,16 @@ static int aat2870_bl_probe(struct platform_device *pdev)
>  
>  	aat2870_bl->bd = bd;
>  
> -	if (pdata->channels > 0)
> -		aat2870_bl->channels = pdata->channels;
> -	else
> -		aat2870_bl->channels = AAT2870_BL_CH_ALL;
> +	aat2870_bl->channels = AAT2870_BL_CH_ALL;
> +	device_property_read_u8(&pdev->dev, "skyworks,channels", &aat2870_bl->channels);
>  
> -	if (pdata->max_current > 0)
> -		aat2870_bl->max_current = pdata->max_current;
> -	else
> -		aat2870_bl->max_current = AAT2870_CURRENT_27_9;
> +	device_property_read_u32(&pdev->dev, "led-max-microamp", &aat2870_bl->max_current);
> +	aat2870_bl->max_current = clamp(aat2870_bl->max_current, AAT2870_CURRENT_MIN,
> +					AAT2870_CURRENT_MAX);
> +	aat2870_bl->max_current /= AAT2870_CURRENT_STEP;

[Severity: High]
Can this sequence cause the max current to evaluate to 0?

If the led-max-microamp property is missing, aat2870_bl->max_current starts at
0 (due to devm_kzalloc) before entering the clamp logic.

Also, the clamp and divide logic seems to mix units. If the DT provides a
microamp value like 27900, it gets clamped up to AAT2870_CURRENT_MIN
(450,000) and then integer-divided by AAT2870_CURRENT_STEP (900,000). This
results in 0, which would lock the brightness at 0 when calculating
aat2870_brightness(). Does this need scaling adjustments?

>  
> -	if (pdata->max_brightness > 0)
> -		bd->props.max_brightness = pdata->max_brightness;
> -	else
> -		bd->props.max_brightness = 255;
> +	bd->props.max_brightness = 255;
> +	device_property_read_u32(&pdev->dev, "max-brightness", &bd->props.max_brightness);

[Severity: Low]
Is it safe to pass a pointer to a signed int here? bd->props.max_brightness
is an int, but device_property_read_u32() expects a u32 pointer. This might
cause signedness mismatch issues.

[Severity: High]
Could this lead to a division by zero in aat2870_brightness()?

If the device tree configures max-brightness = <0>, this property reader will
directly overwrite the default 255 with 0. Later, aat2870_brightness()
executes val /= bd->props.max_brightness, which would cause a kernel panic.

>  
>  	aat2870_bl->brightness = 0;
>  	bd->props.power = BACKLIGHT_POWER_ON;

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