Re: [PATCH v3 1/2] clk: divider: Add KUnit tests for clk_divider_bestdiv() ULONG_MAX handling

[Lad, Prabhakar]

Hi Brian,

Thank you for the review.

On Fri, Apr 24, 2026 at 8:35 PM Brian Masney <bmasney@xxxxxxxxxx> wrote:
>
> Hi Lad,
>
> On Tue, Apr 21, 2026 at 07:25:25PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > Add KUnit tests to verify the behaviour of clk_divider_bestdiv() when
> > clk_round_rate() is called with ULONG_MAX, which is the canonical way
> > to probe the maximum rate a clock can produce.
> >
> > Two test cases are introduced:
> >
> > - clk_divider_bestdiv_ulong_max_returns_max_rate: registers a 1 GHz
> >   fixed-rate parent driving a table-based divider whose smallest entry
> >   is div=2 (entries: 2, 4, 8). Calls clk_hw_round_rate(div_hw, ULONG_MAX)
> >   and checks the result.
> >
> > - clk_divider_bestdiv_mux_ulong_max_returns_max_rate: places a two-input
> >   mux (4 GHz and 2 GHz fixed-rate parents, CLK_SET_RATE_PARENT) ahead of
> >   the same table-based divider to verify correct parent selection under
> >   ULONG_MAX.
> >
> > Both tests use an explicit clk_div_table with a minimum divider of 2 so
> > that the pre-loop maxdiv clamping in clk_divider_bestdiv():
> >
> >     maxdiv = min(ULONG_MAX / rate, maxdiv);
> >
> > clamps maxdiv to 1, causing _next_div() to return 2 on the first
> > iteration and skip the loop body entirely. This makes bestdiv fall back
> > to the maximum divider, returning the minimum rate rather than the
> > maximum.
> >
> > The expected values intentionally reflect the buggy output:
> >   - test 1: PARENT_RATE_1GHZ / 8  (minimum rate, not maximum)
> >   - test 2: 0                      (invalid, loop never populated bestdiv)
> >
> > These will be corrected to PARENT_RATE_1GHZ / 2 and PARENT_RATE_4GHZ / 2
> > respectively once the fix to clk_divider_bestdiv() is applied.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > v2->v3:
> > - Added false positive expected values
> > - Updated the commit message
> > - Added dependency on !S390 in Kconfig
> > ---
> >  drivers/clk/Kconfig            |   8 ++
> >  drivers/clk/Makefile           |   1 +
> >  drivers/clk/clk-divider_test.c | 158 +++++++++++++++++++++++++++++++++
> >  3 files changed, 167 insertions(+)
> >  create mode 100644 drivers/clk/clk-divider_test.c
>
> CONFIG_CLK_DIVIDER_KUNIT_TEST=y needs to be added to
> drivers/clk/.kunitconfig.
>
> >
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > index b2efbe9f6acb..fdeb87ff8fd9 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -573,4 +573,12 @@ config CLK_FD_KUNIT_TEST
> >       help
> >         Kunit test for the clk-fractional-divider type.
> >
> > +config CLK_DIVIDER_KUNIT_TEST
> > +     tristate "KUnit tests for clk divider bestdiv" if !KUNIT_ALL_TESTS
> > +     depends on KUNIT
> > +     depends on !S390
> > +     default KUNIT_ALL_TESTS
> > +     help
> > +       Kunit test for the clk-divider type.
> > +
> >  endif
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> > index a3e2862ebd7e..dc653b458f56 100644
> > --- a/drivers/clk/Makefile
> > +++ b/drivers/clk/Makefile
> > @@ -21,6 +21,7 @@ clk-test-y                  := clk_test.o \
> >                                  kunit_clk_hw_get_dev_of_node.dtbo.o \
> >                                  kunit_clk_parent_data_test.dtbo.o
> >  obj-$(CONFIG_COMMON_CLK)     += clk-divider.o
> > +obj-$(CONFIG_CLK_DIVIDER_KUNIT_TEST) += clk-divider_test.o
> >  obj-$(CONFIG_COMMON_CLK)     += clk-fixed-factor.o
> >  obj-$(CONFIG_COMMON_CLK)     += clk-fixed-rate.o
> >  obj-$(CONFIG_CLK_FIXED_RATE_KUNIT_TEST)      += clk-fixed-rate-test.o
> > diff --git a/drivers/clk/clk-divider_test.c b/drivers/clk/clk-divider_test.c
> > new file mode 100644
> > index 000000000000..3087c77fb157
> > --- /dev/null
> > +++ b/drivers/clk/clk-divider_test.c
> > @@ -0,0 +1,158 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * KUnit tests for clk_divider_bestdiv()
> > + */
> > +#include <kunit/test.h>
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/limits.h>
> > +#include <linux/units.h>
> > +
> > +#define PARENT_RATE_1GHZ     GIGA
> > +#define PARENT_RATE_2GHZ     (2 * GIGA)
> > +#define PARENT_RATE_4GHZ     (4 * GIGA)
> > +
> > +static const struct clk_div_table bestdiv_table[] = {
> > +     { .val = 0, .div = 2 },
> > +     { .val = 1, .div = 4 },
> > +     { .val = 2, .div = 8 },
> > +     { /* sentinel */ }
> > +};
>
> Is it possible to drop the table like I suggested in v2? You can use
> CLK_DIVIDER_ONE_BASED. (See docs in include/linux/clk-provider.h).
> You can also use CLK_DIVIDER_POWER_OF_TWO if you want the divider to be
> larger.
>
When using CLK_DIVIDER_ONE_BASED/CLK_DIVIDER_POWER_OF_TWO I dont get the issue.

For example for a parent of 1GHz and when using
CLK_DIVIDER_POWER_OF_TWO, Im getting expected result of 1GHz for
`clk_hw_round_rate(div_hw, ULONG_MAX)`

Note with CLK_DIVIDER_POWER_OF_TWO flag Im using
    div_hw = clk_hw_register_divider(NULL, "bestdiv-div",
                     "bestdiv-parent",
                     CLK_SET_RATE_PARENT,
                     (void __iomem __force *)fake_reg,
                     0, 2, CLK_DIVIDER_POWER_OF_TWO, NULL);

Case #1 With bestdiv_table
_get_maxdiv() --> walks table, finds max div = 8
maxdiv = min(ULONG_MAX / ULONG_MAX, 8) = min(1, 8) = 1

_next_div(table, 0) --> first table entry --> i = 2

Loop: 2 <= 1 --> FALSE, never enters

Fallback (!bestdiv):
    bestdiv = _get_maxdiv() = 8
    *best_parent_rate = clk_hw_round_rate(parent, 1) = 1GHz  <-- fixed
rate ignores request

return 8 --> 1GHz / 8 = 125MHz

Case #2 With CLK_DIVIDER_POWER_OF_TWO

_get_maxdiv() --> 1 << clk_div_mask(2) = 1 << 3 = 8
maxdiv = min(ULONG_MAX / ULONG_MAX, 8) = min(1, 8) = 1

_next_div(NULL, 0, CLK_DIVIDER_POWER_OF_TWO) --> 1 << 0 = i = 1

Loop: 1 <= 1 --> TRUE  <-- enters because min div is 1, not 2

    rate * i = ULONG_MAX * 1 = ULONG_MAX (no overflow)
    parent_rate = clk_hw_round_rate(parent, ULONG_MAX) = 1GHz
    now = 1GHz / 1 = 1GHz
    bestdiv = 1, best = 1GHz

    next i = 2 > 1 --> loop ends

return 1 --> 1GHz / 1 = 1GHz

> > +
> > +static void unregister_fixed_rate(void *hw)
> > +{
> > +     clk_hw_unregister_fixed_rate(hw);
> > +}
> > +
> > +static void unregister_divider(void *hw)
> > +{
> > +     clk_hw_unregister_divider(hw);
> > +}
> > +
> > +static void unregister_mux(void *hw)
> > +{
> > +     clk_hw_unregister_mux(hw);
> > +}
>
> Can these 3 wrapper functions be dropped, and you just call the inner
> function in the call to kunit_add_action_or_reset()? For example,
> instead of:
>
>     static void unregister_fixed_rate(void *hw)
>     {
>         clk_hw_unregister_fixed_rate(hw);
>     }
>     ...
>     KUNIT_ASSERT_EQ(test, 0,
>                     kunit_add_action_or_reset(test, unregister_fixed_rate,
>                                               parent_hw));
>
> You should be able to do this:
>
>     KUNIT_ASSERT_EQ(test, 0,
>                     kunit_add_action_or_reset(test, clk_hw_unregister_fixed_rate,
>                                               parent_hw));
>
> I think we should just drop the wrappers completely if they aren't
> needed. If more functionality is needed in the future, then the wrappers
> can be added then.
>
Ok, I willl need to cast and use it like below:

+       KUNIT_ASSERT_EQ(test, 0,
+                       kunit_add_action_or_reset(test,
+                                                 (kunit_action_t
*)clk_hw_unregister_divider,
+                                                 div_hw));

> > +
> > +/*
> > + * Test that clk_round_rate(clk, ULONG_MAX) returns the maximum achievable
> > + * rate for a divider clock.
> > + */
> > +static void clk_divider_bestdiv_ulong_max_returns_max_rate(struct kunit *test)
> > +{
> > +     struct clk_hw *parent_hw, *div_hw;
> > +     u32 *fake_reg;
> > +     unsigned long rate;
>
> Reverse Christmas tree order.
>
OK.

> > +
> > +     fake_reg = kunit_kzalloc(test, sizeof(*fake_reg), GFP_KERNEL);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fake_reg);
> > +
> > +     parent_hw = clk_hw_register_fixed_rate(NULL, "bestdiv-parent",
> > +                                            NULL, 0, PARENT_RATE_1GHZ);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent_hw);
> > +     KUNIT_ASSERT_EQ(test, 0,
> > +                     kunit_add_action_or_reset(test, unregister_fixed_rate,
> > +                                               parent_hw));
> > +
> > +     div_hw = clk_hw_register_divider_table(NULL, "bestdiv-div",
> > +                                            "bestdiv-parent",
> > +                                            CLK_SET_RATE_PARENT,
> > +                                            (void __iomem __force *)fake_reg,
> > +                                            0, 2, 0, bestdiv_table, NULL);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, div_hw);
> > +     KUNIT_ASSERT_EQ(test, 0,
> > +                     kunit_add_action_or_reset(test, unregister_divider, div_hw));
> > +
> > +     /*
> > +      * ULONG_MAX is the canonical way to probe the maximum rate a clock
> > +      * can produce.
> > +      */
> > +     rate = clk_hw_round_rate(div_hw, ULONG_MAX);
> > +     KUNIT_EXPECT_EQ(test, rate, PARENT_RATE_1GHZ / 8);
> > +}
> > +
> > +/*
> > + * Test that clk_round_rate(clk, ULONG_MAX) returns the correct maximum rate
> > + * when a mux clock sits between a divider and its parent candidates.
> > + *
> > + * Topology:
> > + *
> > + *   [fixed 4 GHz] --\
> > + *                    +--> [mux CLK_SET_RATE_PARENT] --> [div {2,4,8} CLK_SET_RATE_PARENT]
> > + *   [fixed 2 GHz] --/
> > + *
> > + */
> > +static void clk_divider_bestdiv_mux_ulong_max_returns_max_rate(struct kunit *test)
> > +{
> > +     static const char * const mux_parents[] = {
> > +             "bestdiv-mux-parent-a",
> > +             "bestdiv-mux-parent-b",
> > +     };
> > +     struct clk_hw *parent_a_hw, *parent_b_hw, *mux_hw, *div_hw;
> > +     u32 *fake_reg_mux, *fake_reg_div;
> > +     unsigned long rate;
> > +
> > +     fake_reg_mux = kunit_kzalloc(test, sizeof(*fake_reg_mux), GFP_KERNEL);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fake_reg_mux);
> > +
> > +     fake_reg_div = kunit_kzalloc(test, sizeof(*fake_reg_div), GFP_KERNEL);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fake_reg_div);
> > +
> > +     /* Higher-rate parent: the mux should select this for ULONG_MAX. */
> > +     parent_a_hw = clk_hw_register_fixed_rate(NULL, "bestdiv-mux-parent-a",
> > +                                              NULL, 0, PARENT_RATE_4GHZ);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent_a_hw);
> > +     KUNIT_ASSERT_EQ(test, 0,
> > +                     kunit_add_action_or_reset(test, unregister_fixed_rate,
> > +                                               parent_a_hw));
> > +
> > +     /* Lower-rate parent: should not be selected. */
> > +     parent_b_hw = clk_hw_register_fixed_rate(NULL, "bestdiv-mux-parent-b",
> > +                                              NULL, 0, PARENT_RATE_2GHZ);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent_b_hw);
> > +     KUNIT_ASSERT_EQ(test, 0,
> > +                     kunit_add_action_or_reset(test, unregister_fixed_rate,
> > +                                               parent_b_hw));
> > +
> > +     /*
> > +      * 1-bit mux register selects between the two parents.
> > +      * CLK_SET_RATE_PARENT allows the divider's rate request to
> > +      * propagate into clk_mux_determine_rate().
> > +      */
> > +     mux_hw = clk_hw_register_mux(NULL, "bestdiv-mux",
> > +                                  mux_parents, ARRAY_SIZE(mux_parents),
> > +                                  CLK_SET_RATE_PARENT,
> > +                                  (void __iomem __force *)fake_reg_mux,
> > +                                  0, 1, 0, NULL);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mux_hw);
> > +     KUNIT_ASSERT_EQ(test, 0,
> > +                     kunit_add_action_or_reset(test, unregister_mux, mux_hw));
> > +
> > +     div_hw = clk_hw_register_divider_table(NULL, "bestdiv-mux-div",
> > +                                            "bestdiv-mux",
> > +                                            CLK_SET_RATE_PARENT,
> > +                                            (void __iomem __force *)fake_reg_div,
> > +                                            0, 2, 0, bestdiv_table, NULL);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, div_hw);
> > +     KUNIT_ASSERT_EQ(test, 0,
> > +                     kunit_add_action_or_reset(test, unregister_divider, div_hw));
> > +
> > +     rate = clk_hw_round_rate(div_hw, ULONG_MAX);
> > +     KUNIT_EXPECT_EQ(test, rate, 0);
> > +}
> > +
> > +static struct kunit_case clk_divider_bestdiv_test_cases[] = {
> > +     KUNIT_CASE(clk_divider_bestdiv_ulong_max_returns_max_rate),
> > +     KUNIT_CASE(clk_divider_bestdiv_mux_ulong_max_returns_max_rate),
> > +     {}
> > +};
> > +
> > +static struct kunit_suite clk_divider_bestdiv_test_suite = {
> > +     .name = "clk_divider_bestdiv",
> > +     .test_cases = clk_divider_bestdiv_test_cases,
> > +};
> > +
> > +kunit_test_suite(clk_divider_bestdiv_test_suite);
> > +
> > +MODULE_DESCRIPTION("KUnit tests for clk_divider_bestdiv()");
>
> KUnit tests for clk divider.
>
> I have another tests that will eventually be added to this file.
>
Ok.

Cheers,
Prabhakar