Re: [PATCH v5 3/4] mfd: aat2870: Convert to use OF bindings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] backlight: aat2870_bl: uninitialized max_current in aat2870_bl_probe()
--

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

mfd: aat2870: Convert to use OF bindings

This commit converts the AAT2870 MFD core, regulator, and backlight drivers to
use Device Tree bindings and modern GPIO and regulator frameworks, removing
legacy platform data support.

> diff --git a/drivers/video/backlight/aat2870_bl.c b/drivers/video/backlight/aat2870_bl.c
> index e85f2b4d81068..8e889e35621bb 100644
> --- a/drivers/video/backlight/aat2870_bl.c
> +++ b/drivers/video/backlight/aat2870_bl.c

[ ... ]

> @@ -84,24 +92,12 @@ static const struct backlight_ops aat2870_bl_ops = {
>  
>  static int aat2870_bl_probe(struct platform_device *pdev)
>  {
> -	struct aat2870_bl_platform_data *pdata = dev_get_platdata(&pdev->dev);
>  	struct aat2870_bl_driver_data *aat2870_bl;
>  	struct backlight_device *bd;
>  	struct backlight_properties props;
> +	u32 max_current;
>  	int ret = 0;

[ ... ]

> @@ -128,19 +124,19 @@ 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);
> +
> +	device_property_read_u32(&pdev->dev, "led-max-microamp", &max_current);
> +	max_current = clamp(max_current, AAT2870_CURRENT_MIN, AAT2870_CURRENT_MAX);
> +	max_current /= AAT2870_CURRENT_STEP;
>  
>  	/*
>  	 * The maximum number of brightness steps is determined by the max current.
> -	 * The aat2870_current enum maps directly to the allowed brightness steps.
> +	 * The max_current value calculated above maps to the allowed brightness
> +	 * steps with +1 offset since max_current 0 is still 450uA.
>  	 */
> -	if (pdata->max_current > 0)
> -		bd->props.max_brightness = pdata->max_current;
> -	else
> -		bd->props.max_brightness = AAT2870_CURRENT_27_9;
> +	bd->props.max_brightness = max_current + 1;

[Severity: High]
In aat2870_bl_probe(), max_current is declared on the stack without an
initial value. Unlike aat2870_bl->channels, which is initialized to
AAT2870_BL_CH_ALL before device_property_read_u8(), max_current is passed
directly to device_property_read_u32() without a default value or a return
value check.

When the optional led-max-microamp property is absent from the Device Tree
node, device_property_read_u32() returns -EINVAL without modifying
max_current.

Does this cause clamp() to read an uninitialized stack variable, resulting in
an arbitrary bd->props.max_brightness value that aat2870_bl_update_status()
then programs into the AAT2870_BLM register? Should max_current be
initialized to AAT2870_CURRENT_MAX prior to calling
device_property_read_u32()?

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