Re: [PATCH v2 2/7] clk: tests: Add clk_parse_clkspec() Kunit testing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Ignored return value of `of_clk_add_hw_provider_kunit()`.
--

--- Patch [2]: [PATCH v2 2/7] clk: tests: Add clk_parse_clkspec() Kunit testing ---
commit 4b569be51db687b32469c5799d300307be42e0b7
Author: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>

clk: tests: Add clk_parse_clkspec() Kunit testing

This commit introduces a new set of KUnit tests to verify that
clk_parse_clkspec() functions properly. It validates whether valid lookups
by index and name return the correct devices, and checks that out-of-bounds
requests properly result in an error.

> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> index b1961daac5e22..fddb2bb9618fd 100644
> --- a/drivers/clk/clk_test.c
> +++ b/drivers/clk/clk_test.c
[ ... ]
> @@ -3541,10 +3541,126 @@ static struct kunit_suite clk_hw_get_dev_of_node_test_suite = {
[ ... ]
> +static int clk_parse_clkspec_init(struct kunit *test)
> +{
> +	struct device_node *prov1_np, *prov2_np;
> +	struct clk_parse_clkspec_ctx *ctx;
> +	struct clk_hw *hw1, *hw2;
> +
> +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> +	test->priv = ctx;
> +
> +	KUNIT_ASSERT_EQ(test, 0, of_overlay_apply_kunit(test, kunit_clk_parse_clkspec));
> +
> +	/* Register provider 1 */
> +	hw1 = kunit_kzalloc(test, sizeof(*hw1), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw1);
> +	hw1->init = &clk_parse_clkspec_1_init_data;
> +
> +	prov1_np = of_find_compatible_node(NULL, NULL, "test,clock-provider1");
> +	KUNIT_ASSERT_NOT_NULL(test, prov1_np);
> +
> +	KUNIT_ASSERT_EQ(test, 0, of_clk_hw_register_kunit(test, prov1_np, hw1));
> +	of_clk_add_hw_provider_kunit(test, prov1_np, of_clk_hw_simple_get, hw1);

[Severity: Medium]
Should the return value of of_clk_add_hw_provider_kunit() be verified here?

If the provider fails to register during setup, test initialization will 
continue as if it succeeded. This could mask the actual setup failure and
cause downstream tests to fail unpredictably. Adding a KUNIT_ASSERT_EQ check
similar to the of_clk_hw_register_kunit() call right above it might prevent 
this.

> +	of_node_put(prov1_np);
> +
> +	/* Register provider 2 */
> +	hw2 = kunit_kzalloc(test, sizeof(*hw2), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw2);
> +	hw2->init = &clk_parse_clkspec_2_init_data;
> +
> +	prov2_np = of_find_compatible_node(NULL, NULL, "test,clock-provider2");
> +	KUNIT_ASSERT_NOT_NULL(test, prov2_np);
> +
> +	KUNIT_ASSERT_EQ(test, 0, of_clk_hw_register_kunit(test, prov2_np, hw2));
> +	of_clk_add_hw_provider_kunit(test, prov2_np, of_clk_hw_simple_get, hw2);

[Severity: Medium]
Is it also necessary to check the return value of this second call to
of_clk_add_hw_provider_kunit() for the same reason?

> +	of_node_put(prov2_np);
> +
> +	ctx->cons_np = of_find_compatible_node(NULL, NULL, "test,clock-consumer");
> +	KUNIT_ASSERT_NOT_NULL(test, ctx->cons_np);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-schneider-v7-2-rc1-eip201-upstream-v2-0-b4680787377e@bootlin.com?part=2