linux-kernel - Re: [PATCH RFC] leds: rgb: leds-qcom-lpg: Compute PWM value based on period instead

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ylnkjxnukss7askv7ip5htrb4tyjzhpw7jim2se6rloleq5h6w@ngk7lbk26hxj>
Date: Tue, 4 Mar 2025 07:24:32 +0100
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Abel Vesa <abel.vesa@...aro.org>
Cc: Lee Jones <lee@...nel.org>, Pavel Machek <pavel@...nel.org>, 
	Anjelique Melendez <anjelique.melendez@....qualcomm.com>, Kamal Wadhwa <quic_kamalw@...cinc.com>, 
	Jishnu Prakash <jishnu.prakash@....qualcomm.com>, Bjorn Andersson <andersson@...nel.org>, 
	Konrad Dybcio <konradybcio@...nel.org>, Johan Hovold <johan@...nel.org>, 
	Sebastian Reichel <sre@...nel.org>, Pavel Machek <pavel@....cz>, linux-leds@...r.kernel.org, 
	linux-pwm@...r.kernel.org, linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC] leds: rgb: leds-qcom-lpg: Compute PWM value based on
 period instead

Hello Abel,

On Mon, Mar 03, 2025 at 06:14:36PM +0200, Abel Vesa wrote:
> Currently, the implementation computes the best matched period based
> on the requested one, by looping through all possible register
> configurations. The best matched period is below the requested period.

The best matched period *isn't above* the requested one. An exact match
is fine.

> This means the PWM consumer could request duty cycle values between
> the best matched period and the requested period, which with the current
> implementation for computing the PWM value, it will result in values out
> of range with respect to the selected resolution.

I still don't understand what you mean with resolution here.

I guess you spend some time understanding the workings of the driver and
you also have an example request that results in a hardware
configuration you don't like. Please share the latter to a) support your
case and b) make it easier for your reviewers to judge if your change is
indeed an improvement.

> So change the way the PWM value is determined as a ratio between the
> requested period and duty cycle, mapped on the resolution interval.

Is the intention here that (for the picked period) a duty_cycle is
selected that approximates the requested relative duty_cycle (i.e.
.duty_cycle / .period)?

If it's that: Nack. This might be the right thing for your use case, but
it's wrong for others, it complicates the driver because you have spend
more effort in the calculation and (from my POV even worse) the driver's
behaviour deviates from the usual one for pwm drivers. I admit there are
some other lowlevel pwm drivers that are not aligned to the procedure I
described that should be used to determine the register settings for a
given request. But I target a common behaviour of all pwm drivers
because that is the only way the pwm API functions can make a promise to
its consumers about the resulting behaviour. Reaching this is difficult,
because some consumers might depend on the "broken" behaviour of a given
lowlevel driver (and also because analysing a driver to check and fix
its behaviour is an effort). But "fixing" a driver to deviate from the
declared right behaviour is wrong and includes all downsides that make
me hesitate to align the old drivers to the common policy.

The policy to pick a hardware setting is a compromise between consumer
needs and what is straight forward to implement for (most) hardware
drivers. Please stick to that. If you want more flexibility and
precision in your consumer, please consider converting the pwm driver to
the waveform API.

> [...]
> @@ -532,13 +534,15 @@ static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty)
>  	if (chan->subtype == LPG_SUBTYPE_HI_RES_PWM) {
>  		max = BIT(lpg_pwm_resolution_hi_res[chan->pwm_resolution_sel]) - 1;
>  		clk_rate = lpg_clk_rates_hi_res[chan->clk_sel];
> +		pwm_resolution_arr = lpg_pwm_resolution_hi_res;
>  	} else {
>  		max = BIT(lpg_pwm_resolution[chan->pwm_resolution_sel]) - 1;
>  		clk_rate = lpg_clk_rates[chan->clk_sel];
> +		pwm_resolution_arr = lpg_pwm_resolution;
>  	}
>  
> -	val = div64_u64(duty * clk_rate,
> -			(u64)NSEC_PER_SEC * lpg_pre_divs[chan->pre_div_sel] * (1 << chan->pre_div_exp));
> +	step = div64_u64(period, max);
> +	val = div64_u64(duty, step);

Such a pair of divisions is bound to loose precision. I didn't try to
determine values that can happen in practise here, but consider:

	period =  999996
	max =     500000
	duty =    499998

The exact value for val is 250000. However with integer division you
get 499998. What you can do about that is calculating

	val = duty * max / period

instead which gives you a more exact result. The challenge here however
is that the multiplication might overflow. If you know that the result
fits into a u64, mul_u64_u64_div_u64() is the function that gets this
right for you.

>  	chan->pwm_value = min(val, max);
>  }
> [...]
> ---
> base-commit: 0067a4b21c9ab441bbe6bf3635b3ddd21f6ca7c3

My git repo doesn't know that commit. Given that you said your patch
bases on that other series, this isn't surprising. Please use a publicly
available commit as base parameter, otherwise you (and I) don't benefit
from the armada of build bots because they just silently fail to test in
this case.

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)