Re: [PATCH] clk: divider: Fix overflow in clk_divider_bestdiv() for large rate requests

[Lad, Prabhakar]

Hi Stephen,

Thank you for the review.

On Sun, Apr 12, 2026 at 1:50 AM Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
>
> Quoting Prabhakar (2026-04-08 02:48:14)
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > clk_divider_bestdiv() clamps maxdiv using:
> >
> >     maxdiv = min(ULONG_MAX / rate, maxdiv);
> >
> > to avoid overflow in rate * i. However requests like
> > clk_round_rate(clk, ULONG_MAX), which are used to determine the maximum
> > supported rate of a clock, result in maxdiv being clamped to 1. If no
> > valid divider of 1 exists in the table the loop is never entered and
> > bestdiv falls back to the maximum divider with the minimum parent rate,
> > causing clk_round_rate(clk, ULONG_MAX) to incorrectly return the minimum
> > supported rate instead of the maximum.
> >
> > Fix this by replacing the maxdiv clamping and the unprotected rate * i
> > multiplications with check_mul_overflow(), clamping target_parent_rate
> > to ULONG_MAX on overflow. This allows the loop to iterate all valid
> > dividers regardless of the requested rate, and clk_hw_round_rate() with
> > ULONG_MAX will correctly return the maximum supported parent rate.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> >  drivers/clk/clk-divider.c | 14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
>
> Please add kunit tests to show the broken behavior that you're fixing.
> Make a clk-divider_test.c file for this instead of adding it to the
> clk_test.c file.
>
OK.

> >
> > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> > index 45e7ebde4a8b..dc486c2aa946 100644
> > --- a/drivers/clk/clk-divider.c
> > +++ b/drivers/clk/clk-divider.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/err.h>
> >  #include <linux/string.h>
> >  #include <linux/log2.h>
> > +#include <linux/overflow.h>
> >
> >  /*
> >   * DOC: basic adjustable divider clock that cannot gate
> > @@ -301,6 +302,7 @@ static int clk_divider_bestdiv(struct clk_hw *hw, struct clk_hw *parent,
> >         int i, bestdiv = 0;
> >         unsigned long parent_rate, best = 0, now, maxdiv;
> >         unsigned long parent_rate_saved = *best_parent_rate;
> > +       unsigned long target_parent_rate;
> >
> >         if (!rate)
> >                 rate = 1;
> > @@ -315,15 +317,11 @@ static int clk_divider_bestdiv(struct clk_hw *hw, struct clk_hw *parent,
> >                 return bestdiv;
> >         }
> >
> > -       /*
> > -        * The maximum divider we can use without overflowing
> > -        * unsigned long in rate * i below
> > -        */
> > -       maxdiv = min(ULONG_MAX / rate, maxdiv);
> > -
> >         for (i = _next_div(table, 0, flags); i <= maxdiv;
> >                                              i = _next_div(table, i, flags)) {
> > -               if (rate * i == parent_rate_saved) {
> > +               if (check_mul_overflow(rate, (unsigned long)i, &target_parent_rate))
>
> Please add some sort of comment above this if condition to tell us what
> a target_parent_rate of ULONG_MAX means and why overflowing we set the
> rate to that.
>
Ok, I will add a comment for and send a v2.

Cheers,
Prabhakar

> > +                       target_parent_rate = ULONG_MAX;
> > +               if (target_parent_rate == parent_rate_saved) {
> >                         /*
> >                          * It's the most ideal case if the requested rate can be
> >                          * divided from parent clock without needing to change