Re: [PATCH v6 3/3] clk: cs2600: Add KUnit test for CS2600 driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Medium] 32-bit integer overflow in the calculation of `fdelta` tightens the acceptable error bounds, potentially causing false test failures.
- [Medium] `clk_prepare_enable_kunit()` is repeatedly invoked inside loops, bypassing hardware enable paths after the first iteration and accumulating redundant cleanup actions.
- [Medium] The test verifies the ratio slot using a stale copy of the PLL configuration registers read prior to `clk_set_rate()`.
- [Low] Evaluating a device tree node pointer value after dropping its reference with `of_node_put()`.
- [Low] The Kconfig entry for `COMMON_CLK_CS2600_KUNIT_TEST` fails to `select DTC`, leading to potential build failures.
- [Low] The mock I2C transfer function `cs2600_kunit_i2c_xfer` unconditionally accesses message buffers without validating message count or length.
--

commit da440c9cd81c522da5fe16912468ea26cbaf9db1
Author: Richard Fitzgerald <rf@opensource.cirrus.com>
Date:   Tue Jun 30 15:55:49 2026 +0000

    clk: cs2600: Add KUnit test for CS2600 driver
    
This patch adds a KUnit test for the CS2600 clock driver using a mock I2C host
driver and regmap to partially emulate the hardware. The test relies on hardcoded
tables to validate the frequency ratio and PLL configuration settings, and uses 
devicetree overlays to validate different hardware properties.

> diff --git a/drivers/clk/cirrus/Kconfig b/drivers/clk/cirrus/Kconfig
> index 5674d6554f966..a6807cab4b945 100644
> --- a/drivers/clk/cirrus/Kconfig
> +++ b/drivers/clk/cirrus/Kconfig
> @@ -8,3 +8,16 @@ config COMMON_CLK_CS2600
>  	help
>  	  If you say yes here you get support for the CS2600 clock synthesizer
>  	  and multiplier.
> +
> +config COMMON_CLK_CS2600_KUNIT_TEST
> +	tristate "KUnit test for CS2600 clock driver" if !KUNIT_ALL_TESTS
> +	depends on COMMON_CLK_CS2600
> +	depends on KUNIT
> +	depends on REGULATOR
> +	depends on REGULATOR_FIXED_VOLTAGE
> +	default KUNIT_ALL_TESTS

[Severity: Low]
Does this Kconfig entry need to select DTC? The test's Makefile builds Device
Tree overlays (.dtbo files), which could cause build failures if CONFIG_DTC
is disabled.

> +	help
> +		Build the KUnit tests for the Cirrus Logic CS2600
> +		clock driver.
> +
> +		If unsure say N.

[ ... ]

> diff --git a/drivers/clk/cirrus/clk-cs2600-kunit.c b/drivers/clk/cirrus/clk-cs2600-kunit.c
> new file mode 100644
> index 0000000000000..9631801ac553b
> --- /dev/null
> +++ b/drivers/clk/cirrus/clk-cs2600-kunit.c

[ ... ]

> +static void cs2600_kunit_synth_ratio(struct kunit *test)
> +{
> +	const struct cs2600_kunit_params *params = test->param_value;
> +	const struct cs2600_kunit_ratio_config *ratios = params->s_ratios;
> +	struct cs2600_kunit_priv *priv = test->priv;
> +	unsigned int ratio_sel, pll_cfg2, ratio;
> +	int i;
> +
> +	KUNIT_ASSERT_NOT_NULL(test, ratios);
> +
> +	for (i = 0; ratios[i].clk_out != 0; i++) {
> +		/* 6 MHz is out-of-range for some input clocks */
> +		if (ratios[i].r == 0) {
> +			KUNIT_ASSERT_EQ(test, ratios[i].clk_out, 6000000);
> +			continue;
> +		}
> +
> +		KUNIT_EXPECT_EQ_MSG(test, 0,
> +				    clk_set_rate(priv->clkout, ratios[i].clk_out),
> +				    "clk_out:%u s:%#x\n",
> +				    ratios[i].clk_out, ratios[i].r);
> +		KUNIT_EXPECT_EQ_MSG(test, 0,
> +				    clk_prepare_enable_kunit(test, priv->clkout),

[Severity: Medium]
Is it intentional to call clk_prepare_enable_kunit() in every loop iteration
without a matching disable? This will repeatedly increment the clock's reference
count and accumulate deferred cleanup actions, which might bypass the actual
hardware enable paths on subsequent loop iterations. This also appears to happen
in cs2600_kunit_mult_ratio() and cs2600_kunit_smart_ratio().

> +				    "clk_out:%u s:%#x\n",
> +				    ratios[i].clk_out, ratios[i].r);
> +
> +		KUNIT_ASSERT_EQ(test, 0, regmap_read(priv->regmap, CS2600_PLL_CFG1, &ratio_sel));

[ ... ]

> +static void cs2600_kunit_change_parent_same_clkout(struct kunit *test)
> +{
> +	const struct cs2600_kunit_params *params = test->param_value;
> +	struct cs2600_kunit_priv *priv = test->priv;
> +	struct clk *refclk = clk_hw_get_clk_kunit(test, priv->refclk_hw, NULL);
> +	struct clk *clkin = clk_hw_get_clk_kunit(test, priv->clkin_hw, NULL);
> +	unsigned int ratio_sel, pll_cfg1, pll_cfg2, pll_cfg3, ratio;
> +	int i;

[ ... ]

> +		/* clk_set_rate() should recalculate based on new parent frequency */
> +		KUNIT_EXPECT_EQ_MSG(test, 0,
> +				    clk_set_rate(priv->clkout, params->m_ratios[i].clk_out),
> +				    "clk_out:%u\n", params->m_ratios[i].clk_out);
> +
> +		ratio_sel = FIELD_GET(CS2600_M_RATIO_SEL_MASK, pll_cfg2);

[Severity: Medium]
Does pll_cfg2 contain the updated register state here? The test appears to
validate the ratio slot using a stale copy of the PLL configuration registers
(pll_cfg2, and later pll_cfg1) read prior to the clk_set_rate() call, which
might fail to verify the newly updated hardware state.

> +		ratio = cs2600_kunit_get_ratio_val(test, ratio_sel);
> +		KUNIT_EXPECT_EQ_MSG(test, params->m_ratios[i].r, ratio,
> +				    "clk_out:%u\n", params->m_ratios[i].clk_out);
> +
> +		/*
> +		 * Change back to default parent and it should be synth mode again
> +		 */
> +		KUNIT_EXPECT_EQ(test, 0, clk_set_parent(priv->pllout, refclk));
> +
> +		KUNIT_ASSERT_EQ(test, 0, regmap_read(priv->regmap, CS2600_PLL_CFG1, &pll_cfg1));
> +		KUNIT_ASSERT_EQ(test, 0, regmap_read(priv->regmap, CS2600_PLL_CFG2, &pll_cfg2));
> +		KUNIT_ASSERT_EQ(test, 0, regmap_read(priv->regmap, CS2600_PLL_CFG3, &pll_cfg3));
> +
> +		KUNIT_EXPECT_EQ(test, 0, FIELD_GET(CS2600_PLL_MODE_SEL, pll_cfg2));
> +		KUNIT_EXPECT_EQ(test, FIELD_GET(CS2600_S_RATIO_SEL_MASK, pll_cfg1),
> +				FIELD_GET(CS2600_M_RATIO_SEL_MASK, pll_cfg2));
> +
> +		/* Ratio should have changed if reference frequency has changed */
> +		if (clk_get_rate(refclk) != clk_get_rate(clkin)) {
> +			ratio_sel = FIELD_GET(CS2600_S_RATIO_SEL_MASK, pll_cfg1);
> +			ratio = cs2600_kunit_get_ratio_val(test, ratio_sel);
> +			KUNIT_EXPECT_NE_MSG(test, params->m_ratios[i].r, ratio,
> +					    "clk_out:%u\n", params->s_ratios[i].clk_out);
> +		}
> +
> +		/* clk_set_rate() should recalculate based on new parent frequency */
> +		KUNIT_EXPECT_EQ_MSG(test, 0,
> +				    clk_set_rate(priv->clkout, params->s_ratios[i].clk_out),
> +				    "clk_out:%u\n", params->s_ratios[i].clk_out);
> +
> +		ratio_sel = FIELD_GET(CS2600_S_RATIO_SEL_MASK, pll_cfg1);

[Severity: Medium]
Similar to the issue above, is pll_cfg1 stale here because it was read before
clk_set_rate() updated the hardware?

> +		ratio = cs2600_kunit_get_ratio_val(test, ratio_sel);
> +		KUNIT_EXPECT_EQ_MSG(test, params->s_ratios[i].r, ratio,
> +				    "clk_out:%u\n", params->m_ratios[i].clk_out);
> +	}
> +}

[ ... ]

> +static void cs2600_kunit_bclk_divider(struct kunit *test)
> +{
> +	struct cs2600_kunit_priv *priv = test->priv;
> +	unsigned int out_cfg1, bclk_div, fdelta;
> +	int i;
> +
> +	for (i = 0; cs2600_kunit_bclk_divs[i].pll_out != 0; i++) {

[ ... ]

> +		/*
> +		 * Actual reported frequency can be slightly out because of
> +		 * PLL resolution, but should be within worst-case PPM.
> +		 */
> +		fdelta = (cs2600_kunit_bclk_divs[i].bf_out * CS2600_20_12_PPM) >> 20;

[Severity: Medium]
Can this multiplication overflow a 32-bit integer? The maximum bf_out value is
24,576,000, and CS2600_20_12_PPM is 224, making their product 5,505,024,000,
which exceeds the 32-bit UINT_MAX. This could evaluate to an artificially tight
tolerance range, potentially causing false test failures.

> +		KUNIT_EXPECT_GE_MSG(test, clk_get_rate(priv->bclk),
> +				    cs2600_kunit_bclk_divs[i].bf_out - fdelta,
> +				    "clk_out:%u bclk:%u\n",
> +				    cs2600_kunit_bclk_divs[i].pll_out,
> +				    cs2600_kunit_bclk_divs[i].bf_out);

[ ... ]

> +static int cs2600_kunit_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> +	struct cs2600_kunit_priv *priv = i2c_get_adapdata(adap);
> +	unsigned int addr, val;
> +	__be16 *bebuf;
> +	int ret;
> +
> +	/* First word of first message is always the register address */
> +	bebuf = (__force __be16 *)msgs[0].buf;
> +	addr = be16_to_cpu(bebuf[0]);

[Severity: Low]
Could this unconditionally dereference memory? The code appears to access
msgs[0].buf without first verifying that num >= 1 and msgs[0].len >=
sizeof(__be16).

> +
> +	/* A second word in the first message is a data write */
> +	if (msgs[0].len >= 2 * sizeof(__be16)) {
> +		val = be16_to_cpu(bebuf[1]);
> +		ret = cs2600_kunit_i2c_write(priv, addr, val);
> +		if (ret < 0)
> +			return ret;
> +
> +		return 1;
> +	}

[ ... ]

> +static int cs2600_kunit_case_common_init(struct kunit *test)
> +{
> +	const struct cs2600_kunit_params *params = test->param_value;
> +	struct cs2600_kunit_priv *priv;
> +	struct device_node *np;

[ ... ]

> +	/* Wait for probes if the test case init already loaded the overlay */
> +	np = of_find_node_by_name(NULL, "cs2600-test-consumer");
> +	of_node_put(np);
> +	if (!IS_ERR_OR_NULL(np))

[Severity: Low]
Is it safe to evaluate np with IS_ERR_OR_NULL() after its reference has been
dropped by of_node_put()? Although IS_ERR_OR_NULL() only performs address
evaluation, relying on a conceptually dangling pointer value might lead to
unintended behavior.

> +		cs2600_kunit_wait_for_probes(test);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630155549.824059-1-rf@opensource.cirrus.com?part=3