Re: [PATCH] leds: rgb: leds-qcom-lpg: Add PWM frequency mode support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The FM mode frequency calculation uses truncated clock periods, causing the driver to select configurations that generate a hardware period longer than requested, which violates the PWM API contract.
- [Medium] The driver unconditionally logs a `dev_err` when the opportunistic FM mode fallback fails, allowing unprivileged userspace to spam the kernel log (DoS vector).
- [Medium] Disabling the PWM unconditionally clears the FM mode hardware bit, causing subsequent state readbacks to return severely corrupted period and duty cycle values.
--

--- Patch [1]: [PATCH] leds: rgb: leds-qcom-lpg: Add PWM frequency mode support ---
commit 7ebdd4f9eacf70734e7346f7608c74a404663422
Author: Fenglin Wu <fenglin.wu@oss.qualcomm.com>

leds: rgb: leds-qcom-lpg: Add PWM frequency mode support
    
Certain PWM channels on a PMIC (e.g. PM8350C PWM4) support a Frequency Mode
(FM) that can generate waveforms with more frequency points than the standard
LPG PWM mode. Add FM support so that when the duty cycle is requested to be
exactly 50%, FM mode is used by default for finer-grained frequency resolution.

> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> index d7d6518de30f2..166009ef53af8 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
[ ... ]
> @@ -413,10 +425,105 @@ static int lpg_lut_sync(struct lpg *lpg, unsigned int mask)
>  
>  static const unsigned int lpg_clk_rates[] = {0, 1024, 32768, 19200000};
>  static const unsigned int lpg_clk_rates_hi_res[] = {0, 1024, 32768, 19200000, 76800000};
> +static const unsigned int lpg_clk_period_ns[] = {0, 976562, 30517, 52};
>  static const unsigned int lpg_pre_divs[] = {1, 3, 5, 6};
>  static const unsigned int lpg_pwm_resolution[] = {6, 9};
>  static const unsigned int lpg_pwm_resolution_hi_res[] = {8, 9, 10, 11, 12, 13, 14, 15};
>  
> +static int lpg_calc_freq_fm(struct lpg_channel *chan, uint64_t period_ns)
> +{
[ ... ]
> +	for (clk = 1; clk < clk_len; clk++) {
> +		last_err = U64_MAX;
> +
> +		for (exp = 0; exp <= LPG_MAX_M; exp++) {
> +			/* Calculate pwm_value_lsb for this (clk, exp) pair */
> +			lsb_tmp = period_ns;
> +			lsb_tmp = div64_u64(lsb_tmp, clk_period_arr[clk]);

[Severity: Medium]
Will this division by a truncated nanosecond value (e.g. 52 ns instead of
52.0833 ns for the 19.2MHz clock) artificially increase the computed LSB?

The PWM framework API contract expects the actual generated period to be
equal to or less than the requested period, but this calculation causes the
hardware period to exceed the requested period.

> +			lsb_tmp >>= (exp + 1);
[ ... ]
> +	if (!found) {
> +		dev_err(chan->lpg->dev,
> +			"FM: Cannot generate period %llu ns\n", period_ns);
> +		return -EINVAL;
> +	}

[Severity: Medium]
Since the caller lpg_pwm_apply() gracefully falls back to standard mode
when this returns an error, will unconditionally logging a dev_err allow
unprivileged userspace to spam the kernel log with period requests outside
the FM mode bounds?

> +
> +	chan->clk_sel = best_clk;
> +	chan->pre_div_exp = best_exp;
[ ... ]
> +static void lpg_apply_frequency_mode(struct lpg_channel *chan)
> +{
> +	struct lpg *lpg = chan->lpg;
> +
> +	if (!chan->fm_capable)
> +		return;
> +
> +	regmap_write(lpg->map, chan->base + PWM_FM_MODE_REG,
> +		     chan->use_fm ? PWM_FM_ENABLE : 0);
> +}

[Severity: Medium]
When disabling the PWM in lpg_pwm_apply(), state->enabled is false, which
causes chan->use_fm to be set to false.

Will this unconditionally write 0 to the register, clearing the FM mode
status bit while leaving the clock values intact as FM values?

If the driver is unbound and rebound, or if the hardware state is read back
upon export, this could cause the driver to incorrectly interpret the FM
values using standard mode formulas, yielding corrupted period and duty cycle
values.

[ ... ]
> @@ -1230,28 +1349,42 @@ static int lpg_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
[ ... ]
>  	if (state->enabled) {
> -		ret = lpg_calc_freq(chan, state->period);
> -		if (ret < 0)
> -			goto out_unlock;
> +		use_fm = chan->fm_capable && (state->duty_cycle == state->period / 2);
>  
> -		lpg_calc_duty(chan, state->duty_cycle);
> +		if (use_fm) {
> +			ret = lpg_calc_freq_fm(chan, state->period);
> +			/*
> +			 * fallback to use the standard mode if frequency
> +			 * mode couldn't satisfy the requested period
> +			 */
> +			if (ret < 0)
> +				use_fm = false;
> +		}
> +
> +		if (!use_fm) {
> +			ret = lpg_calc_freq(chan, state->period);
> +			if (ret < 0)
> +				return ret;
> +
> +			lpg_calc_duty(chan, state->duty_cycle);
> +		}
>  	}
> +
> +	chan->use_fm = use_fm;
>  	chan->enabled = state->enabled;
>  
>  	lpg_apply(chan);
[ ... ]
> @@ -1296,9 +1432,32 @@ static int lpg_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
[ ... ]
> +		if (fm_active) {
> +			if (clk_idx >= ARRAY_SIZE(lpg_clk_period_ns))
> +				return -EINVAL;
> +
> +			/*
> +			 * FM period formula:
> +			 *   period = 2 * (pwm_value_lsb + 1) * (2^exp) * clk_period_ns
> +			 */
> +			pwm_value &= 0xff;
> +			state->period = (u64)2 * (pwm_value + 1) * (1 << m) *
> +					lpg_clk_period_ns[clk_idx];

[Severity: Medium]
Does calculating the state back into nanoseconds using the same lossy clock
period array also cause inaccurate state readbacks here?

> +			state->duty_cycle = state->period / 2;
> +		} else {
> +			state->period = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC *
> +					((1 << resolution) - 1) * pre_div * (1 << m), refclk);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-lpg-pwm-fm-support-v1-1-16d3c72a9921@oss.qualcomm.com?part=1