Re: [PATCH v2 3/3] backlight: qcom-wled: Read back the programmed brightness at probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [Critical] wled_remove() dereferences a NULL pointer because platform_set_drvdata() is never called in wled_probe().
- [High] wled_auto_string_detection() hardcodes wled4_set_brightness() instead of using the wled_set_brightness() function pointer, corrupting WLED5 hardware registers.
- [High] The asynchronous delayed work wled->ovp_work can execute after the wled struct is freed on the probe error path.
- [High] Incorrect teardown sequence in wled_remove() causes use-after-destroy on mutex and Use-After-Free on workqueue.
- [High] WLED3_SINK_REG_BRIGHT macro is incorrectly defined with a 16-byte stride instead of a 2-byte stride, causing brightness updates to corrupt unrelated registers.
--

commit 410b83154149a165bcd4c13790f405a3c5f2ac9d
Author: David Heidelberg <david@ixit.cz>

backlight: qcom-wled: Read back the programmed brightness at probe

This commit adds support to read the hardware brightness at probe time to avoid
visible stepping if the bootloader left the backlight on. It introduces
wled_read_brightness() and wires it up to the backlight_ops get_brightness.

> A module that is enabled with the brightness registers at zero, which
> wled4_setup() and wled_auto_string_detection() can leave behind on a
> cold boot, now reads back as off.

[Severity: High]
This is a pre-existing issue, but since the commit message mentions
wled_auto_string_detection(), does this function incorrectly hardcode the WLED4
brightness setter for all versions?

Looking at wled_auto_string_detection(), it bypasses the
wled->wled_set_brightness() pointer and directly calls wled4_set_brightness():

drivers/video/backlight/qcom-wled.c:wled_auto_string_detection() {
    ...
    rc = wled4_set_brightness(wled, AUTO_DETECT_BRIGHTNESS);
    ...
    rc = wled4_set_brightness(wled, wled->brightness);
}

On WLED5 hardware, this writes to the WLED4 register instead of the proper
WLED5 registers, potentially corrupting hardware state when an OVP fault
occurs. Could this be updated to use the generic function pointer?

> diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> index 43e9f6f755582..1acb3ee86878f 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c

[ ... ]

> @@ -290,6 +290,50 @@ static int wled5_set_brightness(struct wled *wled, u16 brightness)
>  	return rc;
>  }
>  
> +static int wled_read_brightness(struct wled *wled)
> +{
> +	u16 addr, mask;
> +	__le16 v;
> +	u32 val;
> +	int rc;
> +
> +	rc = regmap_read(wled->regmap, wled->ctrl_addr + WLED3_CTRL_REG_MOD_EN,
> +			 &val);
> +	if (rc < 0)
> +		return rc;
> +
> +	if (!(val & WLED3_CTRL_REG_MOD_EN_MASK))
> +		return 0;
> +
> +	switch (wled->version) {
> +	case 3:
> +		addr = wled->sink_addr +
> +		       WLED3_SINK_REG_BRIGHT(wled->cfg.enabled_strings[0]);

[Severity: High]
This is a pre-existing issue, but does WLED3_SINK_REG_BRIGHT use an incorrect
stride that corrupts other registers here and in wled3_set_brightness()?

Looking at the macro definition:
#define WLED3_SINK_REG_BRIGHT(n)            (0x40 + (n * 0x10))

The macro applies a 16-byte stride instead of a 2-byte stride. Because
WLED3_SINK_REG_STR_MOD_EN(n) also uses a 16-byte stride starting at 0x60,
evaluating WLED3_SINK_REG_BRIGHT(2) results in 0x60.

This aliases exactly with WLED3_SINK_REG_STR_MOD_EN(0), meaning brightness
updates for string 2 can accidentally overwrite the modulator enable for
string 0.

> +		mask = WLED3_SINK_REG_BRIGHT_MAX;
> +		break;

[ ... ]

> @@ -480,6 +524,18 @@ static int wled_update_status(struct backlight_device *bl)
>  	return rc;
>  }
>  
> +static int wled_get_brightness(struct backlight_device *bl)
> +{
> +	struct wled *wled = bl_get_data(bl);
> +	int rc;
> +
> +	mutex_lock(&wled->lock);

[Severity: High]
This isn't a bug introduced by this patch, but does the teardown sequence in
wled_remove() allow a use-after-destroy here if this new sysfs method is
called concurrently?

wled_remove() prematurely destroys the mutex:

drivers/video/backlight/qcom-wled.c:wled_remove() {
    ...
    mutex_destroy(&wled->lock);
    cancel_delayed_work_sync(&wled->ovp_work);
    ...
}

Because the backlight device is registered with devm_backlight_device_register,
it remains accessible in sysfs until devres cleanup runs after wled_remove()
finishes.

If a user reads the brightness via sysfs during this window,
wled_get_brightness() will lock a destroyed mutex. Could we move the resource
cleanup to a devm action or unregister the backlight explicitly?

> +	rc = wled_read_brightness(wled);
> +	mutex_unlock(&wled->lock);
> +
> +	return rc;
> +}

[ ... ]

> @@ -1757,7 +1809,7 @@ static int wled_probe(struct platform_device *pdev)
>  
>  	memset(&props, 0, sizeof(struct backlight_properties));
>  	props.type = BACKLIGHT_RAW;
> -	props.brightness = val;
> +	props.brightness = wled->brightness ?: val;
>  	props.max_brightness = wled->max_brightness;
>  	bl = devm_backlight_device_register(&pdev->dev, wled->name,
>  					    &pdev->dev, wled,
>  					    &wled_ops, &props);
>  	return PTR_ERR_OR_ZERO(bl);
>  }

[Severity: Critical]
This is a pre-existing issue, but does wled_remove() dereference a NULL pointer
because we missed calling platform_set_drvdata() in probe?

If the driver is unbound, wled_remove() does:

drivers/video/backlight/qcom-wled.c:wled_remove() {
    struct wled *wled = platform_get_drvdata(pdev);
    mutex_destroy(&wled->lock);
    ...
}

Since platform_set_drvdata(pdev, wled) is never called in wled_probe(),
platform_get_drvdata() will return NULL, causing a crash on module removal.

[Severity: High]
This is also a pre-existing issue, but can the wled->ovp_work delayed work
execute after the wled structure is freed if this backlight registration fails?

If the short circuit interrupt fires during probe after
wled_configure_short_irq() enables it, the handler can call
wled_module_enable() which does:

drivers/video/backlight/qcom-wled.c:wled_module_enable() {
    ...
    schedule_delayed_work(&wled->ovp_work, HZ / 100);
    ...
}

If devm_backlight_device_register() subsequently fails, wled_probe() returns
an error. Devres will free the wled structure, but the delayed work is never
cancelled on the probe error path, resulting in a use-after-free when the
work executes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-qcom-wled-backlight-v2-0-a908d9233e93@ixit.cz?part=3