Re: [PATCH 06/16] clk: tests: Add clk_parse_clkspec() Kunit testing

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Brian,

>> @@ -5312,6 +5312,7 @@ struct clk_hw *of_clk_get_hw(struct device_node *np, int index,
>>  
>>  	return hw;
>>  }
>> +EXPORT_SYMBOL_GPL(of_clk_get_hw);
>
> So that we don't unnecessarily broaden the API that's available to the
> clk providers, you can use EXPORT_SYMBOL_IF_KUNIT so that this is only
> available to the kunit tests.

Ah, good idea.

> Note that Chen-Yu posted a separate patch to add the includes for a
> separate test. The two patches will conflict since Stephen hasn't picked
> this up yet.
>
> https://lore.kernel.org/linux-clk/20260225083413.3384950-1-wenst@chromium.org/

Thanks for the warning, I will synchronize with Chen-Yu.

>>  static struct clk *__of_clk_get(struct device_node *np,
>>  				int index, const char *dev_id,
>> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
>> index a268d7b5d4cb..b814b45f1f7e 100644
>> --- a/drivers/clk/clk_test.c
>> +++ b/drivers/clk/clk_test.c
>> @@ -3541,10 +3541,134 @@ static struct kunit_suite clk_hw_get_dev_of_node_test_suite = {
>>  	.test_cases = clk_hw_get_dev_of_node_test_cases,
>>  };
>>  
>> +static const struct clk_init_data clk_parse_clkspec_1_init_data = {
>> +	.name = "clk_parse_clkspec_1",
>> +	.ops = &empty_clk_ops,
>> +};
>> +
>> +static const struct clk_init_data clk_parse_clkspec_2_init_data = {
>> +	.name = "clk_parse_clkspec_2",
>> +	.ops = &empty_clk_ops,
>> +};
>> +
>> +static struct clk_hw *kunit_clk_get(struct of_phandle_args *clkspec, void *data)
>> +{
>> +	return (struct clk_hw *)data;
>> +}
>> +
>> +struct clk_parse_clkspec_ctx {
>> +	struct device_node *prov1_np;
>> +	struct device_node *prov2_np;
>> +	struct device_node *cons_np;
>> +};
>> +
>> +static int clk_parse_clkspec_init(struct kunit *test)
>> +{
>> +	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;
>> +
>> +	ctx->prov1_np = of_find_compatible_node(NULL, NULL, "test,clock-provider1");
>> +	KUNIT_ASSERT_NOT_NULL(test, ctx->prov1_np);
>> +
>> +	KUNIT_ASSERT_EQ(test, 0, of_clk_hw_register_kunit(test, ctx->prov1_np, hw1));
>> +	of_clk_add_hw_provider(ctx->prov1_np, kunit_clk_get, hw1);
>
> Can you just use of_clk_hw_simple_get() and drop kunit_clk_get()
> above?

I will try.

>> +	of_node_put(ctx->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;
>> +
>> +	ctx->prov2_np = of_find_compatible_node(NULL, NULL, "test,clock-provider2");
>> +	KUNIT_ASSERT_NOT_NULL(test, ctx->prov2_np);
>> +
>> +	KUNIT_ASSERT_EQ(test, 0, of_clk_hw_register_kunit(test, ctx->prov2_np, hw2));
>> +	of_clk_add_hw_provider(ctx->prov2_np, kunit_clk_get, hw2);
>> +	of_node_put(ctx->prov2_np);
>> +
>> +	ctx->cons_np = of_find_compatible_node(NULL, NULL, "test,clock-consumer");
>> +	KUNIT_ASSERT_NOT_NULL(test, ctx->cons_np);
>> +
>> +	return 0;
>> +}
>> +
>> +static void clk_parse_clkspec_exit(struct kunit *test)
>> +{
>> +	struct clk_parse_clkspec_ctx *ctx = test->priv;
>> +
>> +	of_node_put(ctx->prov1_np);
>> +	of_node_put(ctx->prov2_np);
>
> Is there a double free of prov1_np and prov2_np? If this is dropped from
> the test exit, then they should't need to be in the ctx struct.

These two calls increment the refcount on the node:
- of_find_compatible_node()
- of_clk_add_hw_provider()

However this makes me realize maybe I should call of_clk_del_provider()
in the exit() function. In any case, I believe keeping a reference over
the nodes during the test is correct and if there is an of_node_put()
call to remove, it should be the on in the _init().

Thanks for pointing this out!
Miquèl