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

[Brian Masney <bmasney@redhat.com>]

Hi Lad,

On Tue, Apr 21, 2026 at 07:25:25PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> 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@bp.renesas.com>
> ---
> 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.

> +
> +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.

> +
> +/*
> + * 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.

> +
> +	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.

Brian