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

[netdev-bot+sashiko@kernel.org]

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?

dom->block is set before dt_req_start() registers dt_req_release(), and
dt_req_release() is the only cleanup action that calls dt_unblock(). If
kunit_kzalloc() or kthread_run() in dt_req_start() fails, the
KUNIT_ASSERT_NOT_NULL() or KUNIT_ASSERT_NOT_ERR_OR_NULL() check aborts the
test and the flag stays set.

The cleanup actions that were already registered then run, for example:

dt_reg_release()
  dpll_pin_unregister()
    down_write(&dpll_lock)
    __dpll_pin_unregister()
      dpll_pin_delete_ntf()
        mutex_lock(&pin->domain->lock)
        dpll_pin_event_send()
          dpll_cmd_pin_get_one()
            pin get callbacks
              dt_pin_enter()->dt_enter()->dt_block()

dt_block() then waits with:

    wait_event(dom->ctx->wq, !READ_ONCE(*block));

Nothing is left that can clear the flag. The KUnit cleanup thread would
sleep uninterruptibly while holding dpll_lock for write and a domain
mutex. Every other DPLL user in the system would then block behind it.

dt_test_stuck_foreign_device() has the same problem:

    WRITE_ONCE(ctx->f0.block, true);
    blocked = dt_req_start(test, dt_device_ntf, &ctx->f0);

There, dt_dev_release()->dpll_device_unregister()->dpll_device_delete_ntf()
->dpll_device_get_one()->dt_mode_get()->dt_dev_enter() would reach
dt_block() on f0.block.

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?

Setting block_prio_set early looks harmless, since no teardown path calls
prio_set.

> +	KUNIT_ASSERT_TRUE(test, wait_for_completion_timeout(&dom->entered,
> +							    DT_TIMEOUT));
> +
> +	return req;
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106121005.1327603-1-ivecera%40redhat.com