Re: [PATCH v14 4/5] reset: Add KUnit test cases

[Tzung-Bi Shih]

On Mon, Oct 19, 2026 at 03:32:12PM +0200, Philipp Zabel wrote:
> On So, 2026-10-11 at 20:12 +0800, Tzung-Bi Shih wrote:
> > Since commit 9035073d0ef1 ("reset: convert reset core to using firmware
> > nodes"), reset suppliers and consumers can be wired up using software
> > nodes without requiring Device Tree or hardware devices.  Leverage this
> > to add KUnit test cases for the reset controller framework.
> > 
> > The test cases cover the following scenarios:
> > 
> > - Basic: Verifies that a consumer can acquire a reset control via
> >   fwnode and perform reset, assert, deassert, status, release, and
> >   acquire operations.
> > - Use After Unregister: Verifies that after the supplier unregisters
> >   the reset_controller_dev while a consumer still holds a reset_control
> >   handle, subsequent consumer operations fail safely with -ENODEV.
> > - Concurrent Unregister: Verifies that unregistering the
> >   reset_controller_dev concurrently with an in-flight reset operation
> >   waits for the active operation to complete before returning.
> > 
> > Signed-off-by: Tzung-Bi Shih <tzungbi@xxxxxxxxxx>
> 
> It looks like the Kunit tests are independent from the reset of the
> series and could be sent individually.
> 
> Is there any dependency that I am missing?

No, but I was using it to verify if [PATCH v14 5/5] breaks anything.  I'll
send it out independently.

> > +static int reset_test_init(struct kunit *test)
> > +{
...
> > +	ctx->rcdev.ops = &reset_test_ops;
> > +	ctx->rcdev.nr_resets = 1;
> > +	ctx->rcdev.fwnode = ctx->supp_fwnode;
> > +
> > +	ret = reset_controller_register(&ctx->rcdev);
> > +	KUNIT_ASSERT_EQ(test, ret, 0);
> > +
> > +	ctx->rstc = __fwnode_reset_control_get(ctx->cons_fwnode, NULL, 0,
> > +					       RESET_CONTROL_EXCLUSIVE);
> > +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->rstc);
> > +
> > +	test->priv = ctx;
> 
> The rcdev and rstc could be cleaned up in .exit for the error cases.

Ack, will add a .exit() callback to clean up `rstc` and `rcdev`.

> > +static void reset_test_basic(struct kunit *test)
> > +{
...
> > +	reset_control_release(rstc);
> > +	KUNIT_EXPECT_EQ(test, reset_control_acquire(rstc), 0);
> > +
> > +	reset_control_put(rstc);
> > +	reset_controller_unregister(&ctx->rcdev);
> 
> Could be cleaned up by .exit.

Ack, will let .exit() handle them.

> > +static void reset_test_use_after_unregister(struct kunit *test)
> > +{
> > +	struct reset_test_context *ctx = test->priv;
> > +	struct reset_control *rstc = ctx->rstc;
> > +
> > +	KUNIT_EXPECT_EQ(test, ctx->reset_count, 0);
> > +	KUNIT_EXPECT_EQ(test, reset_control_reset(rstc), 0);
> > +	KUNIT_EXPECT_EQ(test, ctx->reset_count, 1);
> > +
> > +	/* Unregister supplier while consumer still holds rstc. */
> > +	reset_controller_unregister(&ctx->rcdev);
> 
> If you allocate rcdev on its own instead of embedding it in the
> context, you could
> 
> 	reset_controller_unregister(ctx->rcdev);
> 	ctx->rcdev = NULL;
> 
> here and still have normal cleanup if not NULL in .exit.

The `reset_control_ops` callbacks use `container_of(...)` to obtain `ctx`,
so keeping `rcdev` embedded in `ctx` is convenient.  Will add a bool flag
so .exit() only unregisters `rcdev` if it is still registered.

> > +	KUNIT_EXPECT_EQ(test, ctx->reset_count, 1);
> > +	KUNIT_EXPECT_EQ(test, reset_control_reset(rstc), -ENODEV);
> > +	KUNIT_EXPECT_EQ(test, ctx->reset_count, 1);
> > +
> > +	KUNIT_EXPECT_EQ(test, reset_control_assert(rstc), -ENODEV);
> > +	KUNIT_EXPECT_EQ(test, reset_control_deassert(rstc), -ENODEV);
> > +	KUNIT_EXPECT_EQ(test, reset_control_status(rstc), -ENODEV);
> > +
> > +	reset_control_release(rstc);
> > +	KUNIT_EXPECT_EQ(test, reset_control_acquire(rstc), -ENODEV);
> > +
> > +	reset_control_put(rstc);
> 
> This would not be required either.

Ack, will let .exit() handle it.

> > +static void reset_test_concurrent_unregister(struct kunit *test)
> > +{
> > +	struct reset_test_context *rtc = test->priv;
> > +	struct test_concurrent_unregister_context *ctx;
> > +	int i, ret, val;
> > +
> > +	rtc->rcdev.ops = &reset_test_concurrent_unregister_ops;
> > +
> > +	ctx = kunit_kmalloc_array(test, 3, sizeof(*ctx), GFP_KERNEL);
> > +	KUNIT_ASSERT_NOT_NULL(test, ctx);
> > +
> > +	for (i = 0; i < 3; ++i) {
> > +		ctx[i].test = test;
> > +		init_completion(&ctx[i].started);
> > +		init_completion(&ctx[i].enter);
> > +
> > +		if (i == 0) {
> > +			ctx[i].thread = kthread_run(
> > +				test_concurrent_unregister_supplier, ctx + i,
> > +				"supplier%d", i);
> > +		} else {
> > +			init_completion(&ctx[i].entered);
> > +			init_completion(&ctx[i].exit);
> > +
> > +			ctx[i].thread = kthread_run(
> > +				test_concurrent_unregister_consumer, ctx + i,
> > +				"consumer%d", i);
> > +		}
> > +		KUNIT_ASSERT_FALSE(test, IS_ERR(ctx[i].thread));
> 
> Isn't this missing kthread cleanup in the error cases?

Correct, will fix it.

> > +	/* Subsequent access must see -ENODEV. */
> > +	KUNIT_EXPECT_EQ(test, reset_control_reset(rtc->rstc), -ENODEV);
> > +	reset_control_put(rtc->rstc);
> 
> reset_control_put() could be done by .exit.

Ack, will let .exit() handle it.