linux-kernel - Re: [PATCH] clk: qcom: clk-rcg2: Fix wrong RCG clock rate for high parent frequencies

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: <v2jtnqomqr3bmewtollty2wlu73ymtbobt3pd5aypprmezhow2@edrb3w3kz56b>
Date:   Thu, 20 Jul 2023 21:07:41 +0200
From:   Marijn Suijten <marijn.suijten@...ainline.org>
To:     Devi Priya <quic_devipriy@...cinc.com>
Cc:     andersson@...nel.org, agross@...nel.org, konrad.dybcio@...aro.org,
        mturquette@...libre.com, sboyd@...nel.org,
        linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org, quic_saahtoma@...cinc.com
Subject: Re: [PATCH] clk: qcom: clk-rcg2: Fix wrong RCG clock rate for high
 parent frequencies

Can you retitle this to state "overflow" rather than just "wrong"?
That's more descriptive.

E.g "Fix clockrate overflow for high parent frequencies"

On 2023-07-20 14:03:04, Devi Priya wrote:
> If the parent clock rate is greater than unsigned long max/2 then
> integer overflow happens when calculating the clock rate on 32-bit systems.
> As RCG2 uses half integer dividers, the clock rate is first being
> multiplied by 2 which will overflow the unsigned long max value. So, use
> unsigned long long for rate computations to avoid overflow.
> 
> Signed-off-by: Devi Priya <quic_devipriy@...cinc.com>
> ---
>  drivers/clk/qcom/clk-rcg2.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> index e22baf3a7112..42d00b134975 100644
> --- a/drivers/clk/qcom/clk-rcg2.c
> +++ b/drivers/clk/qcom/clk-rcg2.c
> @@ -156,18 +156,18 @@ static int clk_rcg2_set_parent(struct clk_hw *hw, u8 index)
>   *            hid_div       n
>   */
>  static unsigned long
> -calc_rate(unsigned long rate, u32 m, u32 n, u32 mode, u32 hid_div)
> +calc_rate(unsigned long parent_rate, u32 m, u32 n, u32 mode, u32 hid_div)
>  {
> +	u64 rate = parent_rate;
> +
>  	if (hid_div) {
>  		rate *= 2;
> -		rate /= hid_div + 1;
> +		do_div(rate, hid_div + 1);

I'm pretty sure mult_frac() could have solved this as well, without
temporarily going to u64?

    mult_frac(rate, 2, hid_div + 1)

>  	}
>  
>  	if (mode) {
> -		u64 tmp = rate;
> -		tmp *= m;
> -		do_div(tmp, n);
> -		rate = tmp;
> +		rate *= m;
> +		do_div(rate, n);

    mult_frac(rate, m, n)

Or am I totally wrong?

- Marijn

>  	}
>  
>  	return rate;
> -- 
> 2.17.1
>