[RFC PATCH 0/2] KUnit device API proposal

[RFC PATCH 0/2] KUnit device API proposal

[David Gow]

Hi all,

This is a follow-up to the conversation[1] about adding helpers to create a
struct device for use in KUnit tests. At the moment, most tests are
using root_device_register(), which doesn't quite fit, and a few are
using platform_devices instead.

This adds a KUnit-specific equivalent: kunit_device_register(), which
creates a device which will be automatically cleaned up on test exit
(such as, for example, if an assertion fails).
It's also possible to unregister it earlier with
kunit_device_unregister().

This can replace the root_device_register() users pretty comfortably,
though doesn't resolve the issue with devm_ resources not being released
properly as laid out in [2]. Updating the implementation here to use a
'kunit' bus should, I think, be reasonably straightforward.

The first patch in the series is an in-progress implementation of a
separate new 'kunit_defer()' API, upon which this device implementation
is built.

If the overall idea seems good, I'll make sure to add better
tests/documentation, and patches converting existing tests to this API.

Cheers,
-- David

[1]: https://lore.kernel.org/linux-kselftest/bad670ee135391eb902bd34b8bcbe777afabc7fd.1679474247.git.mazziesaccount@gmail.com/
[2]: https://lore.kernel.org/linux-kselftest/20230324123157.bbwvfq4gsxnlnfwb@houat/

---
David Gow (2):
  kunit: resource: Add kunit_defer() functionality
  kunit: Add APIs for managing devices

 include/kunit/device.h   |  25 +++++++++
 include/kunit/resource.h |  87 +++++++++++++++++++++++++++++++
 lib/kunit/Makefile       |   1 +
 lib/kunit/device.c       |  68 ++++++++++++++++++++++++
 lib/kunit/resource.c     | 110 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 291 insertions(+)
 create mode 100644 include/kunit/device.h
 create mode 100644 lib/kunit/device.c

-- 
2.40.0.348.gf938b09366-goog

[RFC PATCH 1/2] kunit: resource: Add kunit_defer() functionality

[David Gow]

Many uses of the KUnit resource system are intended to simply defer
calling a function until the test exits (be it due to success or
failure). The existing kunit_alloc_resource() function is often used for
this, but was awkward to use (requiring passing NULL init functions, etc),
and returned a resource without incrementing its reference count, which
-- while okay for this use-case -- could cause problems in others.

Instead, introduce a simple kunit_defer() API: a simple function
(returning nothing, accepting a single void* argument) can be scheduled
to be called when the test exits. Deferred functions are called in the
opposite order to that which they were registered.

This is implemented as a resource under the hood, so the ordering
between resource cleanup and deferred functions is maintained.

Signed-off-by: David Gow <davidgow@google.com>
---
 include/kunit/resource.h |  87 +++++++++++++++++++++++++++++++
 lib/kunit/resource.c     | 110 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 197 insertions(+)

diff --git a/include/kunit/resource.h b/include/kunit/resource.h
index cf6fb8f2ac1b..6c4728ca9237 100644
--- a/include/kunit/resource.h
+++ b/include/kunit/resource.h
@@ -387,4 +387,91 @@ static inline int kunit_destroy_named_resource(struct kunit *test,
  */
 void kunit_remove_resource(struct kunit *test, struct kunit_resource *res);
 
+typedef void (*kunit_defer_function_t)(void *ctx);
+
+/**
+ * kunit_defer() - Defer a function call until the test ends.
+ * @test: Test case to associate the deferred function with.
+ * @func: The function to run on test exit
+ * @ctx: Data passed into @func
+ * @internal_gfp: gfp to use for internal allocations, if unsure, use GFP_KERNEL
+ *
+ * Defer the execution of a function until the test exits, either normally or
+ * due to a failure.  @ctx is passed as additional context. All functions
+ * registered with kunit_defer() will execute in the opposite order to that
+ * they were registered in.
+ *
+ * This is useful for cleaning up allocated memory and resources.
+ *
+ * RETURNS:
+ *   An opaque "cancellation token", or NULL on error. Pass this token to
+ *   kunit_defer_cancel() in order to cancel the deferred execution of func().
+ */
+void *kunit_defer(struct kunit *test, kunit_defer_function_t func,
+		  void *ctx, gfp_t internal_gfp);
+
+/**
+ * kunit_defer_cancel_token() - Cancel a deferred function call.
+ * @test: Test case the deferred function is associated with.
+ * @cancel_token: The cancellation token returned by kunit_defer()
+ *
+ * Prevent a function deferred using kunit_defer() from executing when the
+ * test ends.
+ *
+ * Prefer using this to kunit_defer_cancel() where possible.
+ */
+void kunit_defer_cancel_token(struct kunit *test, void *cancel_token);
+
+/**
+ * kunit_defer_trigger_token() - Run a deferred function call immediately.
+ * @test: Test case the deferred function is associated with.
+ * @cancel_token: The cancellation token returned by kunit_defer()
+ *
+ * Execute a deferred function call immediately, instead of waiting for the
+ * test to end.
+ *
+ * Prefer using this to kunit_defer_trigger() where possible.
+ */
+void kunit_defer_trigger_token(struct kunit *test, void *cancel_token);
+
+/**
+ * kunit_defer_cancel() - Cancel a matching deferred function call.
+ * @test: Test case the deferred function is associated with.
+ * @func: The deferred function to cancel.
+ * @ctx: The context passed to the deferred function to trigger.
+ *
+ * Prevent a function deferred via kunit_defer() from executing at shutdown.
+ * Unlike kunit_defer_cancel_token(), this takes the (func, ctx) pair instead of
+ * the cancellation token. If that function/context pair was deferred multiple
+ * times, only the most recent one will be cancelled.
+ *
+ * Prefer using kunit_defer_cancel_token() to this where possible.
+ */
+void kunit_defer_cancel(struct kunit *test,
+			kunit_defer_function_t func,
+			void *ctx);
+
+/**
+ * kunit_defer_trigger() - Run a matching deferred function call immediately.
+ * @test: Test case the deferred function is associated with.
+ * @func: The deferred function to trigger.
+ * @ctx: The context passed to the deferred function to trigger.
+ *
+ * Execute a function deferred via kunit_defer() immediately, rather than when
+ * the test ends.
+ * Unlike kunit_defer_trigger_token(), this takes the (func, ctx) pair instead of
+ * the cancellation token. If that function/context pair was deferred multiple
+ * times, it will only be executed once here. The most recent deferral will
+ * no longer execute when the test ends.
+ *
+ * kunit_defer_trigger(test, func, ctx);
+ * is equivalent to
+ * func(ctx);
+ * kunit_defer_cancel(test, func, ctx);
+ *
+ * Prefer using kunit_defer_trigger_token() to this where possible.
+ */
+void kunit_defer_trigger(struct kunit *test,
+			 kunit_defer_function_t func,
+			 void *ctx);
 #endif /* _KUNIT_RESOURCE_H */
diff --git a/lib/kunit/resource.c b/lib/kunit/resource.c
index c414df922f34..0d0c48054d45 100644
--- a/lib/kunit/resource.c
+++ b/lib/kunit/resource.c
@@ -77,3 +77,113 @@ int kunit_destroy_resource(struct kunit *test, kunit_resource_match_t match,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(kunit_destroy_resource);
+
+struct kunit_defer_ctx {
+	kunit_defer_function_t func;
+	void *ctx;
+};
+
+static void __kunit_defer_free(struct kunit_resource *res)
+{
+	struct kunit_defer_ctx *defer_ctx = (struct kunit_defer_ctx *)res->data;
+
+	defer_ctx->func(defer_ctx->ctx);
+
+	kfree(res->data);
+}
+
+void *kunit_defer(struct kunit *test, kunit_defer_function_t func,
+				void *ctx, gfp_t internal_gfp)
+{
+	struct kunit_resource *res;
+	struct kunit_defer_ctx *defer_ctx;
+
+	KUNIT_ASSERT_NOT_NULL_MSG(test, func, "Tried to defer a NULL function!");
+
+	res = kzalloc(sizeof(*res), internal_gfp);
+	if (!res)
+		return NULL;
+
+	defer_ctx = kzalloc(sizeof(*defer_ctx), internal_gfp);
+	if (!defer_ctx)
+		goto free_res;
+
+	defer_ctx->func = func;
+	defer_ctx->ctx = ctx;
+
+	res->should_kfree = true;
+	__kunit_add_resource(test, NULL, __kunit_defer_free, res, defer_ctx);
+
+	return (void *)res;
+
+free_res:
+	kfree(res);
+	return NULL;
+}
+
+void kunit_defer_cancel_token(struct kunit *test, void *cancel_token)
+{
+	struct kunit_resource *res = (struct kunit_resource *)cancel_token;
+
+	/* Remove the free function so we don't run the deferred function. */
+	res->free = NULL;
+
+	kunit_remove_resource(test, res);
+}
+
+void kunit_defer_trigger_token(struct kunit *test, void *cancel_token)
+{
+	struct kunit_resource *res = (struct kunit_resource *)cancel_token;
+
+	/* Removing the resource should trigger the res->free function. */
+	kunit_remove_resource(test, res);
+}
+
+static bool __kunit_defer_match(struct kunit *test,
+				struct kunit_resource *res, void *match_data)
+{
+	struct kunit_defer_ctx *match_ctx = (struct kunit_defer_ctx *)match_data;
+	struct kunit_defer_ctx *res_ctx = (struct kunit_defer_ctx *)res->data;
+
+	/* Make sure this is a free function. */
+	if (res->free != __kunit_defer_free)
+		return false;
+
+	/* Both the function and context data should match. */
+	return (match_ctx->func == res_ctx->func) && (match_ctx->ctx == res_ctx->ctx);
+}
+
+void kunit_defer_cancel(struct kunit *test,
+			kunit_defer_function_t func,
+			void *ctx)
+{
+	struct kunit_defer_ctx defer_ctx;
+	struct kunit_resource *res;
+
+	defer_ctx.func = func;
+	defer_ctx.ctx = ctx;
+
+	res = kunit_find_resource(test, __kunit_defer_match, &defer_ctx);
+	if (res) {
+		kunit_defer_cancel_token(test, res);
+		kunit_put_resource(res);
+	}
+}
+
+void kunit_defer_trigger(struct kunit *test,
+			 kunit_defer_function_t func,
+			 void *ctx)
+{
+	struct kunit_defer_ctx defer_ctx;
+	struct kunit_resource *res;
+
+	defer_ctx.func = func;
+	defer_ctx.ctx = ctx;
+
+	res = kunit_find_resource(test, __kunit_defer_match, &defer_ctx);
+	if (res) {
+		kunit_defer_trigger_token(test, res);
+		/* We have to put() this here, else free won't be called. */
+		kunit_put_resource(res);
+	}
+}
-- 
2.40.0.348.gf938b09366-goog

Re: [RFC PATCH 1/2] kunit: resource: Add kunit_defer() functionality

[Matti Vaittinen]

On 3/25/23 06:31, David Gow wrote:
> Many uses of the KUnit resource system are intended to simply defer
> calling a function until the test exits (be it due to success or
> failure). The existing kunit_alloc_resource() function is often used for
> this, but was awkward to use (requiring passing NULL init functions, etc),
> and returned a resource without incrementing its reference count, which
> -- while okay for this use-case -- could cause problems in others.
> 
> Instead, introduce a simple kunit_defer() API: a simple function
> (returning nothing, accepting a single void* argument) can be scheduled
> to be called when the test exits. Deferred functions are called in the
> opposite order to that which they were registered.
> 
> This is implemented as a resource under the hood, so the ordering
> between resource cleanup and deferred functions is maintained.
> 
> Signed-off-by: David Gow <davidgow@google.com>
> ---
>   include/kunit/resource.h |  87 +++++++++++++++++++++++++++++++
>   lib/kunit/resource.c     | 110 +++++++++++++++++++++++++++++++++++++++
>   2 files changed, 197 insertions(+)
> 
> diff --git a/include/kunit/resource.h b/include/kunit/resource.h
> index cf6fb8f2ac1b..6c4728ca9237 100644
> --- a/include/kunit/resource.h
> +++ b/include/kunit/resource.h
> @@ -387,4 +387,91 @@ static inline int kunit_destroy_named_resource(struct kunit *test,
>    */
>   void kunit_remove_resource(struct kunit *test, struct kunit_resource *res);
>   
> +typedef void (*kunit_defer_function_t)(void *ctx);
> +
> +/**
> + * kunit_defer() - Defer a function call until the test ends.
> + * @test: Test case to associate the deferred function with.
> + * @func: The function to run on test exit
> + * @ctx: Data passed into @func
> + * @internal_gfp: gfp to use for internal allocations, if unsure, use GFP_KERNEL
> + *
> + * Defer the execution of a function until the test exits, either normally or
> + * due to a failure.  @ctx is passed as additional context. All functions
> + * registered with kunit_defer() will execute in the opposite order to that
> + * they were registered in.
> + *
> + * This is useful for cleaning up allocated memory and resources.
> + *
> + * RETURNS:

Is this intentional? Unless I'm mistaken the kernel-doc return value 
specifier is 'Return:'.

> + *   An opaque "cancellation token", or NULL on error. Pass this token to
> + *   kunit_defer_cancel() in order to cancel the deferred execution of func().
> + */
> +void *kunit_defer(struct kunit *test, kunit_defer_function_t func,
> +		  void *ctx, gfp_t internal_gfp);
> +
> +/**
> + * kunit_defer_cancel_token() - Cancel a deferred function call.
> + * @test: Test case the deferred function is associated with.
> + * @cancel_token: The cancellation token returned by kunit_defer()
> + *
> + * Prevent a function deferred using kunit_defer() from executing when the
> + * test ends.
> + *
> + * Prefer using this to kunit_defer_cancel() where possible.
> + */
> +void kunit_defer_cancel_token(struct kunit *test, void *cancel_token);
> +
> +/**
> + * kunit_defer_trigger_token() - Run a deferred function call immediately.
> + * @test: Test case the deferred function is associated with.
> + * @cancel_token: The cancellation token returned by kunit_defer()
> + *
> + * Execute a deferred function call immediately, instead of waiting for the
> + * test to end.
> + *
> + * Prefer using this to kunit_defer_trigger() where possible.
> + */
> +void kunit_defer_trigger_token(struct kunit *test, void *cancel_token);
> +
> +/**
> + * kunit_defer_cancel() - Cancel a matching deferred function call.
> + * @test: Test case the deferred function is associated with.
> + * @func: The deferred function to cancel.
> + * @ctx: The context passed to the deferred function to trigger.
> + *
> + * Prevent a function deferred via kunit_defer() from executing at shutdown.
> + * Unlike kunit_defer_cancel_token(), this takes the (func, ctx) pair instead of
> + * the cancellation token. If that function/context pair was deferred multiple
> + * times, only the most recent one will be cancelled.
> + *
> + * Prefer using kunit_defer_cancel_token() to this where possible.
> + */
> +void kunit_defer_cancel(struct kunit *test,
> +			kunit_defer_function_t func,
> +			void *ctx);
> +
> +/**
> + * kunit_defer_trigger() - Run a matching deferred function call immediately.
> + * @test: Test case the deferred function is associated with.
> + * @func: The deferred function to trigger.
> + * @ctx: The context passed to the deferred function to trigger.
> + *
> + * Execute a function deferred via kunit_defer() immediately, rather than when
> + * the test ends.
> + * Unlike kunit_defer_trigger_token(), this takes the (func, ctx) pair instead of
> + * the cancellation token. If that function/context pair was deferred multiple
> + * times, it will only be executed once here. The most recent deferral will
> + * no longer execute when the test ends.
> + *
> + * kunit_defer_trigger(test, func, ctx);
> + * is equivalent to
> + * func(ctx);
> + * kunit_defer_cancel(test, func, ctx);
> + *
> + * Prefer using kunit_defer_trigger_token() to this where possible.
> + */
> +void kunit_defer_trigger(struct kunit *test,
> +			 kunit_defer_function_t func,
> +			 void *ctx);
>   #endif /* _KUNIT_RESOURCE_H */
> diff --git a/lib/kunit/resource.c b/lib/kunit/resource.c
> index c414df922f34..0d0c48054d45 100644
> --- a/lib/kunit/resource.c
> +++ b/lib/kunit/resource.c
> @@ -77,3 +77,113 @@ int kunit_destroy_resource(struct kunit *test, kunit_resource_match_t match,
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(kunit_destroy_resource);
> +
> +struct kunit_defer_ctx {
> +	kunit_defer_function_t func;
> +	void *ctx;
> +};
> +
> +static void __kunit_defer_free(struct kunit_resource *res)
> +{
> +	struct kunit_defer_ctx *defer_ctx = (struct kunit_defer_ctx *)res->data;
> +
> +	defer_ctx->func(defer_ctx->ctx);
> +
> +	kfree(res->data);
> +}
> +
> +void *kunit_defer(struct kunit *test, kunit_defer_function_t func,
> +				void *ctx, gfp_t internal_gfp)
> +{
> +	struct kunit_resource *res;
> +	struct kunit_defer_ctx *defer_ctx;
> +
> +	KUNIT_ASSERT_NOT_NULL_MSG(test, func, "Tried to defer a NULL function!");
> +
> +	res = kzalloc(sizeof(*res), internal_gfp);
> +	if (!res)
> +		return NULL;
> +
> +	defer_ctx = kzalloc(sizeof(*defer_ctx), internal_gfp);
> +	if (!defer_ctx)
> +		goto free_res;
> +
> +	defer_ctx->func = func;
> +	defer_ctx->ctx = ctx;
> +
> +	res->should_kfree = true;
> +	__kunit_add_resource(test, NULL, __kunit_defer_free, res, defer_ctx);
> +
> +	return (void *)res;
> +
> +free_res:
> +	kfree(res);
> +	return NULL;
> +}
> +
> +void kunit_defer_cancel_token(struct kunit *test, void *cancel_token)
> +{
> +	struct kunit_resource *res = (struct kunit_resource *)cancel_token;
> +
> +	/* Remove the free function so we don't run the deferred function. */
> +	res->free = NULL;
> +
> +	kunit_remove_resource(test, res);
> +}
> +
> +void kunit_defer_trigger_token(struct kunit *test, void *cancel_token)
> +{
> +	struct kunit_resource *res = (struct kunit_resource *)cancel_token;
> +
> +	/* Removing the resource should trigger the res->free function. */
> +	kunit_remove_resource(test, res);
> +}
> +
> +static bool __kunit_defer_match(struct kunit *test,
> +				struct kunit_resource *res, void *match_data)
> +{
> +	struct kunit_defer_ctx *match_ctx = (struct kunit_defer_ctx *)match_data;
> +	struct kunit_defer_ctx *res_ctx = (struct kunit_defer_ctx *)res->data;
> +
> +	/* Make sure this is a free function. */
> +	if (res->free != __kunit_defer_free)
> +		return false;
> +
> +	/* Both the function and context data should match. */
> +	return (match_ctx->func == res_ctx->func) && (match_ctx->ctx == res_ctx->ctx);
> +}
> +
> +void kunit_defer_cancel(struct kunit *test,
> +			kunit_defer_function_t func,
> +			void *ctx)
> +{
> +	struct kunit_defer_ctx defer_ctx;
> +	struct kunit_resource *res;
> +
> +	defer_ctx.func = func;
> +	defer_ctx.ctx = ctx;
> +
> +	res = kunit_find_resource(test, __kunit_defer_match, &defer_ctx);
> +	if (res) {
> +		kunit_defer_cancel_token(test, res);
> +		kunit_put_resource(res);
> +	}
> +}
> +
> +void kunit_defer_trigger(struct kunit *test,
> +			 kunit_defer_function_t func,
> +			 void *ctx)
> +{
> +	struct kunit_defer_ctx defer_ctx;
> +	struct kunit_resource *res;
> +
> +	defer_ctx.func = func;
> +	defer_ctx.ctx = ctx;
> +
> +	res = kunit_find_resource(test, __kunit_defer_match, &defer_ctx);
> +	if (res) {
> +		kunit_defer_trigger_token(test, res);
> +		/* We have to put() this here, else free won't be called. */
> +		kunit_put_resource(res);
> +	}
> +}

I am not really sure if you must provide the 'tokenless' cancel or 
trigger APIs. They just feel a bit much to me. Well, if you have a 
reason to do so then this looks good to me.

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [RFC PATCH 1/2] kunit: resource: Add kunit_defer() functionality

[Maxime Ripard]

Hi David,

Thanks a lot for doing this

On Sat, Mar 25, 2023 at 12:31:03PM +0800, David Gow wrote:
> Many uses of the KUnit resource system are intended to simply defer
> calling a function until the test exits (be it due to success or
> failure). The existing kunit_alloc_resource() function is often used for
> this, but was awkward to use (requiring passing NULL init functions, etc),
> and returned a resource without incrementing its reference count, which
> -- while okay for this use-case -- could cause problems in others.
> 
> Instead, introduce a simple kunit_defer() API: a simple function
> (returning nothing, accepting a single void* argument) can be scheduled
> to be called when the test exits. Deferred functions are called in the
> opposite order to that which they were registered.
> 
> This is implemented as a resource under the hood, so the ordering
> between resource cleanup and deferred functions is maintained.
> 
> Signed-off-by: David Gow <davidgow@google.com>
> ---
>  include/kunit/resource.h |  87 +++++++++++++++++++++++++++++++
>  lib/kunit/resource.c     | 110 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 197 insertions(+)
> 
> diff --git a/include/kunit/resource.h b/include/kunit/resource.h
> index cf6fb8f2ac1b..6c4728ca9237 100644
> --- a/include/kunit/resource.h
> +++ b/include/kunit/resource.h
> @@ -387,4 +387,91 @@ static inline int kunit_destroy_named_resource(struct kunit *test,
>   */
>  void kunit_remove_resource(struct kunit *test, struct kunit_resource *res);
>  
> +typedef void (*kunit_defer_function_t)(void *ctx);
> +
> +/**
> + * kunit_defer() - Defer a function call until the test ends.

devm (and drm resource management) uses devm_add_action for this, and
providing consistency across resource management API would be neat.

> + * @test: Test case to associate the deferred function with.
> + * @func: The function to run on test exit
> + * @ctx: Data passed into @func
> + * @internal_gfp: gfp to use for internal allocations, if unsure, use GFP_KERNEL

Do you really expect any GFP flag other than GFP_KERNEL?

> + * Defer the execution of a function until the test exits, either normally or
> + * due to a failure.  @ctx is passed as additional context. All functions
> + * registered with kunit_defer() will execute in the opposite order to that
> + * they were registered in.
> + *
> + * This is useful for cleaning up allocated memory and resources.
> + *
> + * RETURNS:
> + *   An opaque "cancellation token", or NULL on error. Pass this token to
> + *   kunit_defer_cancel() in order to cancel the deferred execution of func().
> + */

devm also gets away with this by (iirc) associating the function
pointer and context to the device. That triplet is pretty much
guaranteed to be unique across calls and thus you don't have to keep
(and pass!) a token around.

> +void *kunit_defer(struct kunit *test, kunit_defer_function_t func,
> +		  void *ctx, gfp_t internal_gfp);
> +
> +/**
> + * kunit_defer_cancel_token() - Cancel a deferred function call.
> + * @test: Test case the deferred function is associated with.
> + * @cancel_token: The cancellation token returned by kunit_defer()
> + *
> + * Prevent a function deferred using kunit_defer() from executing when the
> + * test ends.
> + *
> + * Prefer using this to kunit_defer_cancel() where possible.
> + */
> +void kunit_defer_cancel_token(struct kunit *test, void *cancel_token);
> +
> +/**
> + * kunit_defer_trigger_token() - Run a deferred function call immediately.
> + * @test: Test case the deferred function is associated with.
> + * @cancel_token: The cancellation token returned by kunit_defer()
> + *
> + * Execute a deferred function call immediately, instead of waiting for the
> + * test to end.
> + *
> + * Prefer using this to kunit_defer_trigger() where possible.
> + */
> +void kunit_defer_trigger_token(struct kunit *test, void *cancel_token);
> +
> +/**
> + * kunit_defer_cancel() - Cancel a matching deferred function call.
> + * @test: Test case the deferred function is associated with.
> + * @func: The deferred function to cancel.
> + * @ctx: The context passed to the deferred function to trigger.
> + *
> + * Prevent a function deferred via kunit_defer() from executing at shutdown.
> + * Unlike kunit_defer_cancel_token(), this takes the (func, ctx) pair instead of
> + * the cancellation token. If that function/context pair was deferred multiple
> + * times, only the most recent one will be cancelled.
> + *
> + * Prefer using kunit_defer_cancel_token() to this where possible.
> + */
> +void kunit_defer_cancel(struct kunit *test,
> +			kunit_defer_function_t func,
> +			void *ctx);
> +
> +/**
> + * kunit_defer_trigger() - Run a matching deferred function call immediately.
> + * @test: Test case the deferred function is associated with.
> + * @func: The deferred function to trigger.
> + * @ctx: The context passed to the deferred function to trigger.
> + *
> + * Execute a function deferred via kunit_defer() immediately, rather than when
> + * the test ends.
> + * Unlike kunit_defer_trigger_token(), this takes the (func, ctx) pair instead of
> + * the cancellation token. If that function/context pair was deferred multiple
> + * times, it will only be executed once here. The most recent deferral will
> + * no longer execute when the test ends.
> + *
> + * kunit_defer_trigger(test, func, ctx);
> + * is equivalent to
> + * func(ctx);
> + * kunit_defer_cancel(test, func, ctx);
> + *
> + * Prefer using kunit_defer_trigger_token() to this where possible.
> + */
> +void kunit_defer_trigger(struct kunit *test,
> +			 kunit_defer_function_t func,
> +			 void *ctx);
>  #endif /* _KUNIT_RESOURCE_H */

I think I agree with Matti here, you might want some function to
cancel an action/deferral but it's not clear to me why you would need
all the other functions there (for now at least)

Maxime

Re: [RFC PATCH 1/2] kunit: resource: Add kunit_defer() functionality

[Benjamin Berg]

Hi,

I do like having such an API for automatic cleanup.

On Sat, 2023-03-25 at 12:31 +0800, David Gow wrote:
> Many uses of the KUnit resource system are intended to simply defer
> calling a function until the test exits (be it due to success or
> failure). The existing kunit_alloc_resource() function is often used for
> this, but was awkward to use (requiring passing NULL init functions, etc),
> and returned a resource without incrementing its reference count, which
> -- while okay for this use-case -- could cause problems in others.
> 
> Instead, introduce a simple kunit_defer() API: a simple function
> (returning nothing, accepting a single void* argument) can be scheduled
> to be called when the test exits. Deferred functions are called in the
> opposite order to that which they were registered.
> 
> This is implemented as a resource under the hood, so the ordering
> between resource cleanup and deferred functions is maintained.
> 
> Signed-off-by: David Gow <davidgow@google.com>
> ---
>  include/kunit/resource.h |  87 +++++++++++++++++++++++++++++++
>  lib/kunit/resource.c     | 110 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 197 insertions(+)
> 
> diff --git a/include/kunit/resource.h b/include/kunit/resource.h
> index cf6fb8f2ac1b..6c4728ca9237 100644
> --- a/include/kunit/resource.h
> +++ b/include/kunit/resource.h
> @@ -387,4 +387,91 @@ static inline int kunit_destroy_named_resource(struct kunit *test,
>   */
>  void kunit_remove_resource(struct kunit *test, struct kunit_resource *res);
>  
> +typedef void (*kunit_defer_function_t)(void *ctx);
> +
> +/**
> + * kunit_defer() - Defer a function call until the test ends.

I separately proposed kunit_add_cleanup. Which I find quite descriptive
and it matches the name that the python unittest package uses.

> + * @test: Test case to associate the deferred function with.
> + * @func: The function to run on test exit
> + * @ctx: Data passed into @func
> + * @internal_gfp: gfp to use for internal allocations, if unsure, use GFP_KERNEL
> + *
> + * Defer the execution of a function until the test exits, either normally or
> + * due to a failure.  @ctx is passed as additional context. All functions
> + * registered with kunit_defer() will execute in the opposite order to that
> + * they were registered in.
> + *
> + * This is useful for cleaning up allocated memory and resources.
> + *
> + * RETURNS:
> + *   An opaque "cancellation token", or NULL on error. Pass this token to
> + *   kunit_defer_cancel() in order to cancel the deferred execution of func().
> + */
> +void *kunit_defer(struct kunit *test, kunit_defer_function_t func,
> +                 void *ctx, gfp_t internal_gfp);

Maybe return struct kunit_defer_ctx* instead of void? No need to have
the structure itself be visible.

> +/**
> + * kunit_defer_cancel_token() - Cancel a deferred function call.
> + * @test: Test case the deferred function is associated with.
> + * @cancel_token: The cancellation token returned by kunit_defer()
> + *
> + * Prevent a function deferred using kunit_defer() from executing when the
> + * test ends.
> + *
> + * Prefer using this to kunit_defer_cancel() where possible.
> + */
> +void kunit_defer_cancel_token(struct kunit *test, void *cancel_token);
> +
> +/**
> + * kunit_defer_trigger_token() - Run a deferred function call immediately.
> + * @test: Test case the deferred function is associated with.
> + * @cancel_token: The cancellation token returned by kunit_defer()
> + *
> + * Execute a deferred function call immediately, instead of waiting for the
> + * test to end.
> + *
> + * Prefer using this to kunit_defer_trigger() where possible.
> + */
> +void kunit_defer_trigger_token(struct kunit *test, void *cancel_token);
> +
> +/**
> + * kunit_defer_cancel() - Cancel a matching deferred function call.
> + * @test: Test case the deferred function is associated with.
> + * @func: The deferred function to cancel.
> + * @ctx: The context passed to the deferred function to trigger.
> + *
> + * Prevent a function deferred via kunit_defer() from executing at shutdown.
> + * Unlike kunit_defer_cancel_token(), this takes the (func, ctx) pair instead of
> + * the cancellation token. If that function/context pair was deferred multiple
> + * times, only the most recent one will be cancelled.
> + *
> + * Prefer using kunit_defer_cancel_token() to this where possible.
> + */
> +void kunit_defer_cancel(struct kunit *test,
> +                       kunit_defer_function_t func,
> +                       void *ctx);
> +
> +/**
> + * kunit_defer_trigger() - Run a matching deferred function call immediately.
> + * @test: Test case the deferred function is associated with.
> + * @func: The deferred function to trigger.
> + * @ctx: The context passed to the deferred function to trigger.
> + *
> + * Execute a function deferred via kunit_defer() immediately, rather than when
> + * the test ends.
> + * Unlike kunit_defer_trigger_token(), this takes the (func, ctx) pair instead of
> + * the cancellation token. If that function/context pair was deferred multiple
> + * times, it will only be executed once here. The most recent deferral will
> + * no longer execute when the test ends.
> + *
> + * kunit_defer_trigger(test, func, ctx);
> + * is equivalent to
> + * func(ctx);
> + * kunit_defer_cancel(test, func, ctx);
> + *
> + * Prefer using kunit_defer_trigger_token() to this where possible.
> + */
> +void kunit_defer_trigger(struct kunit *test,
> +                        kunit_defer_function_t func,
> +                        void *ctx);
>  #endif /* _KUNIT_RESOURCE_H */
> diff --git a/lib/kunit/resource.c b/lib/kunit/resource.c
> index c414df922f34..0d0c48054d45 100644
> --- a/lib/kunit/resource.c
> +++ b/lib/kunit/resource.c
> @@ -77,3 +77,113 @@ int kunit_destroy_resource(struct kunit *test, kunit_resource_match_t match,
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(kunit_destroy_resource);
> +
> +struct kunit_defer_ctx {
> +       kunit_defer_function_t func;
> +       void *ctx;
> +};

You could avoid an allocation by embedding struct kunit_resource.

> +
> +static void __kunit_defer_free(struct kunit_resource *res)
> +{
> +       struct kunit_defer_ctx *defer_ctx = (struct kunit_defer_ctx *)res->data;
> +
> +       defer_ctx->func(defer_ctx->ctx);
> +
> +       kfree(res->data);
> +}
> +
> +void *kunit_defer(struct kunit *test, kunit_defer_function_t func,
> +                               void *ctx, gfp_t internal_gfp)
> +{
> +       struct kunit_resource *res;
> +       struct kunit_defer_ctx *defer_ctx;
> +
> +       KUNIT_ASSERT_NOT_NULL_MSG(test, func, "Tried to defer a NULL function!");
> +
> +       res = kzalloc(sizeof(*res), internal_gfp);
> +       if (!res)
> +               return NULL;
> +
> +       defer_ctx = kzalloc(sizeof(*defer_ctx), internal_gfp);
> +       if (!defer_ctx)
> +               goto free_res;
> +
> +       defer_ctx->func = func;
> +       defer_ctx->ctx = ctx;
> +
> +       res->should_kfree = true;
> +       __kunit_add_resource(test, NULL, __kunit_defer_free, res, defer_ctx);
> +
> +       return (void *)res;
> +
> +free_res:
> +       kfree(res);
> +       return NULL;
> +}
> +
> +void kunit_defer_cancel_token(struct kunit *test, void *cancel_token)
> +{
> +       struct kunit_resource *res = (struct kunit_resource *)cancel_token;
> +
> +       /* Remove the free function so we don't run the deferred function. */
> +       res->free = NULL;
> +
> +       kunit_remove_resource(test, res);
> +}
> +
> +void kunit_defer_trigger_token(struct kunit *test, void *cancel_token)
> +{
> +       struct kunit_resource *res = (struct kunit_resource *)cancel_token;
> +
> +       /* Removing the resource should trigger the res->free function. */
> +       kunit_remove_resource(test, res);
> +}
> +
> +static bool __kunit_defer_match(struct kunit *test,
> +                               struct kunit_resource *res, void *match_data)
> +{
> +       struct kunit_defer_ctx *match_ctx = (struct kunit_defer_ctx *)match_data;
> +       struct kunit_defer_ctx *res_ctx = (struct kunit_defer_ctx *)res->data;
> +
> +       /* Make sure this is a free function. */
> +       if (res->free != __kunit_defer_free)
> +               return false;
> +
> +       /* Both the function and context data should match. */
> +       return (match_ctx->func == res_ctx->func) && (match_ctx->ctx == res_ctx->ctx);
> +}
> +
> +void kunit_defer_cancel(struct kunit *test,
> +                       kunit_defer_function_t func,
> +                       void *ctx)
> +{
> +       struct kunit_defer_ctx defer_ctx;
> +       struct kunit_resource *res;
> +
> +       defer_ctx.func = func;
> +       defer_ctx.ctx = ctx;
> +
> +       res = kunit_find_resource(test, __kunit_defer_match, &defer_ctx);
> +       if (res) {
> +               kunit_defer_cancel_token(test, res);
> +               kunit_put_resource(res);
> +       }
> +}
> +
> +void kunit_defer_trigger(struct kunit *test,
> +                        kunit_defer_function_t func,
> +                        void *ctx)
> +{
> +       struct kunit_defer_ctx defer_ctx;
> +       struct kunit_resource *res;
> +
> +       defer_ctx.func = func;
> +       defer_ctx.ctx = ctx;
> +
> +       res = kunit_find_resource(test, __kunit_defer_match, &defer_ctx);
> +       if (res) {
> +               kunit_defer_trigger_token(test, res);
> +               /* We have to put() this here, else free won't be called. */
> +               kunit_put_resource(res);
> +       }
> +}

[RFC PATCH 2/2] kunit: Add APIs for managing devices

[David Gow]

Tests for drivers often require a struct device to pass to other
functions. While it's possible to create these with
root_device_register(), or to use something like a platform device, this
is both a misuse of those APIs, and can be difficult to clean up after,
for example, a failed assertion.

Add two KUnit-specific functions for registering and unregistering a
struct device:
- kunit_device_register()
- kunit_device_unregister()

These behave similarly to root_device_register() and
root_device_unregister() except:
- They take a struct kunit pointer with a test context.
- They do not create a root device directory in sysfs.
- The device will automatically be unregistered when the test exits
  (unless it has already been unregistered using
  kunit_device_unregister())
- The device name is set to <test-name>.<device-name>.

This API can be extended in the future to, for example, add these
devices to a KUnit bus as tests begin to require that functionality.

Signed-off-by: David Gow <davidgow@google.com>
---
 include/kunit/device.h | 25 ++++++++++++++++
 lib/kunit/Makefile     |  1 +
 lib/kunit/device.c     | 68 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+)
 create mode 100644 include/kunit/device.h
 create mode 100644 lib/kunit/device.c

diff --git a/include/kunit/device.h b/include/kunit/device.h
new file mode 100644
index 000000000000..19a35b5e4e59
--- /dev/null
+++ b/include/kunit/device.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * KUnit basic device implementation
+ *
+ * Implementation of struct kunit_device helpers.
+ *
+ * Copyright (C) 2023, Google LLC.
+ * Author: David Gow <davidgow@google.com>
+ */
+
+#ifndef _KUNIT_DEVICE_H
+#define _KUNIT_DEVICE_H
+
+#if IS_ENABLED(CONFIG_KUNIT)
+
+#include <kunit/test.h>
+
+struct kunit_device;
+
+struct device *kunit_device_register(struct kunit *test, const char *name);
+void kunit_device_unregister(struct kunit *test, struct device *dev);
+
+#endif
+
+#endif
diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
index cb417f504996..b9bd059269ed 100644
--- a/lib/kunit/Makefile
+++ b/lib/kunit/Makefile
@@ -6,6 +6,7 @@ kunit-objs +=				test.o \
 					string-stream.o \
 					assert.o \
 					try-catch.o \
+					device.o \
 					executor.o
 
 ifeq ($(CONFIG_KUNIT_DEBUGFS),y)
diff --git a/lib/kunit/device.c b/lib/kunit/device.c
new file mode 100644
index 000000000000..ce87b7c40d9b
--- /dev/null
+++ b/lib/kunit/device.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit basic device implementation
+ *
+ * Implementation of struct kunit_device helpers.
+ *
+ * Copyright (C) 2023, Google LLC.
+ * Author: David Gow <davidgow@google.com>
+ */
+
+#include <linux/device.h>
+
+#include <kunit/test.h>
+#include <kunit/resource.h>
+
+/* A device owned by a KUnit test. */
+struct kunit_device {
+	struct device dev;
+	struct kunit *owner;
+};
+
+static inline struct kunit_device *to_kunit_device(struct device *d)
+{
+	return container_of(d, struct kunit_device, dev);
+}
+
+static void kunit_device_release(struct device *d)
+{
+	kfree(to_kunit_device(d));
+}
+
+struct device *kunit_device_register(struct kunit *test, const char *name)
+{
+	struct kunit_device *kunit_dev;
+	int err = -ENOMEM;
+
+	kunit_dev = kzalloc(sizeof(struct kunit_device), GFP_KERNEL);
+	if (!kunit_dev)
+		return ERR_PTR(err);
+
+	kunit_dev->owner = test;
+
+	err = dev_set_name(&kunit_dev->dev, "%s.%s", test->name, name);
+	if (err) {
+		kfree(kunit_dev);
+		return ERR_PTR(err);
+	}
+
+	kunit_dev->dev.release = kunit_device_release;
+
+	err = device_register(&kunit_dev->dev);
+	if (err) {
+		put_device(&kunit_dev->dev);
+		return ERR_PTR(err);
+	}
+
+	kunit_defer(test, (kunit_defer_function_t)device_unregister, &kunit_dev->dev, GFP_KERNEL);
+
+	return &kunit_dev->dev;
+}
+EXPORT_SYMBOL_GPL(kunit_device_register);
+
+void kunit_device_unregister(struct kunit *test, struct device *dev)
+{
+	kunit_defer_trigger(test, (kunit_defer_function_t)device_unregister, dev);
+}
+EXPORT_SYMBOL_GPL(kunit_device_unregister);
+
-- 
2.40.0.348.gf938b09366-goog

Re: [RFC PATCH 2/2] kunit: Add APIs for managing devices

[Greg Kroah-Hartman]

On Sat, Mar 25, 2023 at 12:31:04PM +0800, David Gow wrote:
> Tests for drivers often require a struct device to pass to other
> functions. While it's possible to create these with
> root_device_register(), or to use something like a platform device, this
> is both a misuse of those APIs, and can be difficult to clean up after,
> for example, a failed assertion.
> 
> Add two KUnit-specific functions for registering and unregistering a
> struct device:
> - kunit_device_register()
> - kunit_device_unregister()
> 
> These behave similarly to root_device_register() and
> root_device_unregister() except:
> - They take a struct kunit pointer with a test context.
> - They do not create a root device directory in sysfs.

But they show up in the root directory in sysfs, in /sys/devices/ which
is not a good place to be.  Why not make them part of a class, perhaps
called 'kunit', so that they will be in a contained place?

thanks,

greg k-h

Re: [RFC PATCH 2/2] kunit: Add APIs for managing devices

[Maxime Ripard]

Hi,

On Sat, Mar 25, 2023 at 12:31:04PM +0800, David Gow wrote:
> Tests for drivers often require a struct device to pass to other
> functions. While it's possible to create these with
> root_device_register(), or to use something like a platform device, this
> is both a misuse of those APIs, and can be difficult to clean up after,
> for example, a failed assertion.
> 
> Add two KUnit-specific functions for registering and unregistering a
> struct device:
> - kunit_device_register()
> - kunit_device_unregister()

If kunit_device_register() registers an action to clean up after the
test has ran, I'm not sure why do we need kunit_device_unregister()

I guess the typical test would just call kunit_device_register() and
be done with it, right?

Maxime