Re: [PATCH v2] leds: rgb: leds-qcom-lpg: Fix LED color balancing in HW pattern mode

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unprotected call to lpg_brightness_set() from lpg_pattern_mc_set() leads to data races on shared chip state.
- [High] Short palindrome patterns trigger an edge case that permanently leaks LUT slots and silently fails to display the pattern.
--

commit 8e19f4c1d616341c9f4d4953aa70f990483e7985
Author: Fenglin Wu <fenglin.wu@oss.qualcomm.com>
Subject: leds: rgb: leds-qcom-lpg: Fix LED color balancing in HW pattern mode

This commit fixes an issue where the LED device operates in a white-balanced
mode regardless of intensity settings when triggered by a HW pattern. It scales
the pattern data according to the sub-led intensity and programs the HW
pattern separately for each LPG channel.

> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> index d7d6518de30f2..daf796df5706c 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
[ ... ]
> @@ -1087,20 +1098,48 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
>  	 * duration of all steps.
>  	 */
>  	if (lpg->lut_base || lpg->lut_sdam) {
> -		lo_pause = pattern[0].delta_t;
> -		hi_pause = pattern[actual_len - 1].delta_t;
> +		prep->lo_pause = pattern[0].delta_t;
> +		prep->hi_pause = pattern[actual_len - 1].delta_t;

[Severity: High]
This is a pre-existing issue, but does the pattern preparation logic
handle short palindrome patterns correctly without leaking LUT slots?

When a minimal 4-element HW palindrome pattern is processed internally
(translating to a len of 2 internally), the ping_pong mode sets
actual_len = (2 + 1) / 2 = 1.

lpg_lut_store() then successfully allocates exactly 1 slot, setting
lo_idx == hi_idx. However, lpg_lut_free() has an early exit:

static void lpg_lut_free(struct lpg *lpg, unsigned int lo_idx,
                         unsigned int hi_idx)
{
    ...
    len = hi_idx - lo_idx + 1;
    if (len == 1)
        return;
    ...
}

Because lpg_brightness_set() checks for pattern_lo_idx != pattern_hi_idx
before applying it, the pattern itself is ignored and not displayed. 
Furthermore, the early return in lpg_lut_free() prevents the bitmap from ever
being cleared, permanently leaking the allocated slot.

Could this lead to eventual resource exhaustion for pattern assignments?

>  	} else {
>  		if (delta_t != pattern[0].delta_t || delta_t != pattern[actual_len - 1].delta_t)
> -			goto out_free_pattern;
> +			goto out_free;
[ ... ]
> @@ -1144,23 +1182,88 @@ static int lpg_pattern_single_set(struct led_classdev *cdev,
>  }
>  
>  static int lpg_pattern_mc_set(struct led_classdev *cdev,
[ ... ]
> +out_unlock:
> +	mutex_unlock(&lpg->lock);
> +	kfree(scaled);
> +out_free_prep:
> +	kfree(pattern.data);
>  
> -	ret = lpg_pattern_set(led, pattern, len, repeat);
>  	if (ret < 0)
>  		return ret;
>  
> -	led_mc_calc_color_components(mc, LED_FULL);
>  	lpg_brightness_set(led, cdev, mc->subled_info);

[Severity: High]
This isn't a bug introduced by this patch, but does calling
lpg_brightness_set() here outside of the lpg->lock lead to data races?

The lpg->lock mutex is explicitly released just prior to this call. However,
lpg_brightness_set() updates chip-wide shared state. For instance, it calls
lpg_set_pbs_trigger() which performs an unprotected read-modify-write on
lpg->pbs_en_bitmap:

static int lpg_set_pbs_trigger(...)
{
    ...
    lpg->pbs_en_bitmap |= lut_mask;
    ...
}

Concurrent sysfs accesses or pattern triggers invoking lpg_pattern_mc_set()
could race here alongside other brightness updates, potentially causing
data corruption in the shared state. Is it safe to leave this invocation
unprotected?

>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624-lpg-rgb-color-balance-fix-v2-1-c01b0e50caf6@oss.qualcomm.com?part=1