Re: [PATCH net-next 06/10] dpll: add KUnit tests for core locking

[Ivan Vecera <ivecera@redhat.com>]

On 11/10/26 18:53, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] In drivers/dpll/dpll_core_kunit.c, some tests set a blocking
>    flag before dt_req_start() has registered dt_req_release().
> 
> --
> 
> dpll: add KUnit tests for core locking
> 
> This adds KUnit tests for DPLL core locking. They use a mock driver with
> several domains to check domain sharing, which locks callbacks hold,
> independence and serialization of domains, atomicity of multi-attribute
> pin sets, and notification ordering. The netlink pre/post doit and set
> handlers are exported for KUnit so the tests can call them directly.
> 
>> diff --git a/drivers/dpll/dpll_core_kunit.c b/drivers/dpll/dpll_core_kunit.c
>> new file mode 100644
>> index 0000000000000..a58800eda5ff5
>> --- /dev/null
>> +++ b/drivers/dpll/dpll_core_kunit.c
>> @@ -0,0 +1,1471 @@
> 
> [ ... ]
> 
>> +static void dt_block(struct dt_dom *dom, bool *block)
>> +{
>> +	complete(&dom->entered);
>> +	wait_event(dom->ctx->wq, !READ_ONCE(*block));
>> +}
> 
> [ ... ]
> 
>> +static struct dt_req *dt_req_start(struct kunit *test, int (*fn)(void *arg),
>> +				   void *arg)
>> +{
>> +	struct task_struct *task;
>> +	struct dt_req *req;
>> +
>> +	req = kunit_kzalloc(test, sizeof(*req), GFP_KERNEL);
>> +	KUNIT_ASSERT_NOT_NULL(test, req);
>> +
>> +	req->ctx = test->priv;
>> +	req->fn = fn;
>> +	req->arg = arg;
>> +	init_completion(&req->done);
>> +
>> +	task = kthread_run(dt_req_thread, req, "dpll-test");
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, task);
>> +	KUNIT_ASSERT_EQ(test, kunit_add_action_or_reset(test, dt_req_release,
>> +							req), 0);
>> +
>> +	return req;
>> +}
> 
> [ ... ]
> 
>> +/* Blocks a request of @dom in a callback, which holds the domain lock */
>> +static struct dt_req *dt_block_in(struct kunit *test, struct dt_dom *dom,
>> +				  int (*fn)(void *arg), void *arg)
>> +{
>> +	struct dt_req *req;
>> +
>> +	WRITE_ONCE(dom->block, true);
>> +	req = dt_req_start(test, fn, arg);
> 
> [Severity: Medium]
> Can the teardown hang here if dt_req_start() fails one of its assertions?

Yes. It takes an allocation or kthread_run() failure in the test, but
then the cleanup blocks under dpll_lock for good.

[...]
> One option is to register a cleanup action that calls dt_unblock() from
> dt_init(), before any flag can be set. Another is to set the flags only
> after dt_req_release() has been registered. Would either of these work?

The objects of the foreign and on-pin tests are added by the tests after
dt_init(), and KUnit runs the actions in reverse order, so an action
registered in dt_init() would run after their release. v2 clears the
flags in an exit callback of the suite instead. KUnit runs it before any
cleanup action, so it also covers dt_test_stuck_foreign_device().

Thanks,
Ivan
---
pw-bot: cr