linux-kernel - Re: [PATCH] clk: bcm: iproc: Fix overflow in clock calculation

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] [day] [month] [year] [list]

Message-ID: <CAJ8Y1dTNM-oXVK4bm0hCQf6Vb+QGUA_YL5T-L89Dv3HZYPzsFw@mail.gmail.com>
Date: Fri, 30 Jan 2026 08:52:45 -0800
From: Ray Jui <ray.jui@...adcom.com>
To: Mark Tomlinson <mark.tomlinson@...iedtelesis.co.nz>
Cc: sboyd@...nel.org, rjui@...adcom.com, bcm-kernel-feedback-list@...adcom.com, 
	linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] clk: bcm: iproc: Fix overflow in clock calculation

On Wed, Jan 28, 2026 at 4:41 PM Mark Tomlinson <
mark.tomlinson@...iedtelesis.co.nz> wrote:

> Some BCM iproc devices run at 1.25GHz. This comes from a 50MHZ crystal
> with a pre-divider of two, a post-divider of two and a PLL multiplier of
> 100. As the multiply is done first (50M * 100) this overflows a 32-bit
> value. The multiplication was done as a 64-bit multiply, but the result
> assigned to a 32-bit variable before the division was done. To fix this,
> use a 64-bit temporary variable and use the do_div() macro to perform a
> 64-bit/32-bit division.
>
> Fixes: 5fe225c105fd ("clk: iproc: add initial common clock support")
> Signed-off-by: Mark Tomlinson <mark.tomlinson@...iedtelesis.co.nz>
> ---
>  drivers/clk/bcm/clk-iproc-armpll.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/bcm/clk-iproc-armpll.c
> b/drivers/clk/bcm/clk-iproc-armpll.c
> index 9e86c0c10b57..665f1c531c4f 100644
> --- a/drivers/clk/bcm/clk-iproc-armpll.c
> +++ b/drivers/clk/bcm/clk-iproc-armpll.c
> @@ -180,7 +180,7 @@ static unsigned int __get_ndiv(struct iproc_arm_pll
> *pll)
>   *   mdiv = ARM PLL post divider
>   *
>   * The frequency is calculated by:
> - *   ((ndiv * parent clock rate) / pdiv) / mdiv
> + *   (ndiv * parent clock rate) / (pdiv * mdiv)
>   */
>  static unsigned long iproc_arm_pll_recalc_rate(struct clk_hw *hw,
>                 unsigned long parent_rate)
> @@ -189,6 +189,7 @@ static unsigned long iproc_arm_pll_recalc_rate(struct
> clk_hw *hw,
>         u32 val;
>         int mdiv;
>         u64 ndiv;
> +       u64 rate;
>         unsigned int pdiv;
>
>         /* in bypass mode, use parent rate */
> @@ -216,8 +217,9 @@ static unsigned long iproc_arm_pll_recalc_rate(struct
> clk_hw *hw,
>                 pll->rate = 0;
>                 return 0;
>         }
> -       pll->rate = (ndiv * parent_rate) >> 20;
>

50 MHz crystal * ndiv of 100 indeed > 32-bit, but the right shift by 20
would have ensured the result is way less than 32-bit.
I guess this is not a real world problem (you won't have a crystal as
reference clock of a PLL running at a frequency much higher than 50 MHz).
But just to be safe, using a 64-bit temporary variable for 'rate' before
further division below does not hurt.


> -       pll->rate = (pll->rate / pdiv) / mdiv;
> +       rate = (ndiv * parent_rate) >> 20;
> +       do_div(rate, pdiv * mdiv);
> +       pll->rate = rate;
>
>         pr_debug("%s: ARM PLL rate: %lu. parent rate: %lu\n", __func__,
>                  pll->rate, parent_rate);
> --
> 2.52.0
>
>
Acked-by: Ray Jui <ray.jui@...adcom.com>

Content of type "text/html" skipped

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5449 bytes)