Re: [PATCH v2 01/13] clk: scmi: Fix clock rate rounding

[Cristian Marussi]

On Wed, Mar 11, 2026 at 12:30:34PM +0100, Geert Uytterhoeven wrote:
> Hi Cristian,
> 
> On Tue, 10 Mar 2026 at 19:40, Cristian Marussi <cristian.marussi@xxxxxxx> wrote:
> > While the do_div() helper used for rounding expects its divisor argument
> > to be a 32bits quantity, the currently provided divisor parameter is a
> > 64bit value that, as a consequence, is silently truncated and a possible
> > source of bugs.
> >
> > Fix by using the proper div64_ul helper.
> >
> > Cc: Michael Turquette <mturquette@xxxxxxxxxxxx>
> > Cc: Stephen Boyd <sboyd@xxxxxxxxxx>
> > Cc: linux-clk@xxxxxxxxxxxxxxx
> > Fixes: 7a8655e19bdb ("clk: scmi: Fix the rounding of clock rate")
> > Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
> 
> Thanks for your patch!
> 
> > --- a/drivers/clk/clk-scmi.c
> > +++ b/drivers/clk/clk-scmi.c
> 
> > @@ -83,7 +83,7 @@ static int scmi_clk_determine_rate(struct clk_hw *hw,
> >
> >         ftmp = req->rate - fmin;
> >         ftmp += clk->info->range.step_size - 1; /* to round up */
> > -       do_div(ftmp, clk->info->range.step_size);
> > +       ftmp = div64_ul(ftmp, clk->info->range.step_size);
> 
> include/linux/math64.h has:
> 
>     #if BITS_PER_LONG == 64
>     #define div64_ul(x, y)   div64_u64((x), (y))
>     #elif BITS_PER_LONG == 32
>     #define div64_ul(x, y)   div_u64((x), (y))
>     #endif
> 
> I.e. div64_ul() is meant for the case where the divisor is unsigned
> long.  Hence div64_ul() may still truncate step_size on 32-bit
> platforms, and thus should use div64_u64() unconditionally.
> 

Ah...my bad, I missed that...

> I am aware clock rates are "unsigned long" on 32-bit platforms, and
> thus cannot support rates that do not fit in a 32-bit value.
> If that is the reason you are using div64_ul(), it should be documented
> properly.  And probably the SCMI core code should reject any rate values
> (incl. min, max, step) that do not fit in unsigned long, as such clocks
> cannot be used on 32-bit platforms.

As per the SCMI spec Clock rates are reported as 64bit, so I suppose
that, yes I will have to add the checks you mentioned to avoid trying to
fit a reported clock rate where the upper 32bits are not zero into an
unsigned long on a 32bit machine...

Seems like there will be a V3 (together with your other reports..)

Thanks for your reviews !

Cristian