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

[Lee Jones <lee@kernel.org>]

On Tue, 25 Aug 2026, Fenglin Wu wrote:

> 
> 
> On 7/17/2026 12:55 PM, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > 
> > Pre-existing issues:
> > - [High] The driver permanently leaks a hardware LUT slot and fails to trigger patterns when a 2-step palindrome pattern evaluates to an `actual_len` of 1.
> > --
> > 
> > --- Patch [1]: [PATCH v6] leds: rgb: leds-qcom-lpg: Fix LED color balancing in HW pattern mode ---
> > commit c5b003f7bc9326bf3784a70ad883c39b215ae496
> > Author: Fenglin Wu <fenglin.wu@oss.qualcomm.com>
> > Subject: leds: rgb: leds-qcom-lpg: Fix LED color balancing in HW pattern mode
> > 
> > This commit splits out pattern preparation into lpg_prepare_pattern() and
> > adds support for per-channel scaling so the hardware pattern respects
> > multi-color LED balances. It also updates the LUT storage paths to handle
> > the dynamically allocated scaled patterns.
> > 
> >> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> >> index d7d6518de30f2..24b1f570f5244 100644
> >> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> >> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> > 
> > [ ... ]
> > 
> >> @@ -1087,43 +1098,64 @@ 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;
> >>  	} else {
> >>  		if (delta_t != pattern[0].delta_t || delta_t != pattern[actual_len - 1].delta_t)
> >> -			goto out_free_pattern;
> >> +			return -EINVAL;
> >> +		prep->lo_pause = 0;
> >> +		prep->hi_pause = 0;
> >>  	}
> >>  
> >> +	prep->data = no_free_ptr(pattern);
> >> +	prep->len = actual_len;
> > 
> > [Severity: High]
> > This is a pre-existing issue, but does this logic permanently leak a
> > hardware LUT slot if a 2-step palindrome pattern is provided?
> > 
> > If a user-space process provides a 2-step pattern where both steps have the
> > same brightness, the ping_pong logic earlier in lpg_prepare_pattern() will
> > calculate actual_len = (2 + 1) / 2, which evaluates to 1.
> > 
> > When this length-1 pattern is subsequently stored, it allocates 1 slot where
> > lo_idx and hi_idx are equal. Because they are equal, lpg_brightness_set()
> > treats the configuration as a static brightness instead of playing the
> > pattern.
> > 
> > Later, when the pattern is cleared and lpg_lut_free() is called with this
> > lo_idx and hi_idx, it hits this early return:
> > 
> > static void lpg_lut_free(struct lpg *lpg, unsigned int lo_idx, unsigned int hi_idx)
> > {
> > 	int len;
> > 
> > 	len = hi_idx - lo_idx + 1;
> > 	if (len == 1)
> > 		return;
> > 
> > 	bitmap_clear(lpg->lut_bitmap, lo_idx, len);
> > }
> > 
> > Does this early return bypass bitmap_clear() and permanently leak the LUT
> > slot for the remainder of the device lifecycle?
> > 
> 
> I think this should go into a separate patch, since it isn’t introduced
> by this change. On the fix itself, rather than checking "(len == 1)" to
> clear the bitmap, we should add a new flag to track whether the bitmap
> has been allocated and only free it when that flag is true.
> 
> @Jones, what do you think? Would you prefer that I include this fix in
> the current change, or should I push a separate fix after this one?

It's Lee! =:-)

I don't tend to look at existing issues, but if you'd like to submit a
patch for it, great!

-- 
Lee Jones