linux-kernel - Re: [05/11] drm/tests: helpers: Create an helper to allocate a locking ctx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <970f4a65-25dc-1805-3776-6447a61f77c5@loongson.cn>
Date:   Tue, 18 Jul 2023 00:50:16 +0800
From:   suijingfeng <suijingfeng@...ngson.cn>
To:     Maxime Ripard <mripard@...nel.org>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Emma Anholt <emma@...olt.net>
Cc:     linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org
Subject: Re: [05/11] drm/tests: helpers: Create an helper to allocate a
 locking ctx

Hi,

On 2023/7/10 15:47, Maxime Ripard wrote:
> As we get more and more tests, the locking context initialisation
> creates more and more boilerplate, both at creation and destruction.
>
> Let's create a helper that will allocate, initialise a context, and
> register kunit actions to clean up once the test is done.
>
> Signed-off-by: Maxime Ripard <mripard@...nel.org>
> ---
>   drivers/gpu/drm/tests/drm_kunit_helpers.c | 41 +++++++++++++++++++++++++++++++
>   include/drm/drm_kunit_helpers.h           |  2 ++
>   2 files changed, 43 insertions(+)
>
> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> index 38211fea9ae6..40a27c78d692 100644
> --- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> @@ -124,5 +124,46 @@ __drm_kunit_helper_alloc_drm_device_with_driver(struct kunit *test,
>   }
>   EXPORT_SYMBOL_GPL(__drm_kunit_helper_alloc_drm_device_with_driver);
>   
> +static void action_drm_release_context(void *ptr)
> +{
> +	struct drm_modeset_acquire_ctx *ctx = ptr;
> +
> +	drm_modeset_drop_locks(ctx);
> +	drm_modeset_acquire_fini(ctx);
> +}
> +
> +/**
> + * drm_kunit_helper_context_alloc - Allocates an acquire context
> + * @test: The test context object
> + *
> + * Allocates and initializes a modeset acquire context.
> + *
> + * The context is tied to the kunit test context, so we must not call
> + * drm_modeset_acquire_fini() on it, it will be done so automatically.
> + *
> + * Returns:
> + * An ERR_PTR on error, a pointer to the newly allocated context otherwise
> + */
> +struct drm_modeset_acquire_ctx *
> +drm_kunit_helper_acquire_ctx_alloc(struct kunit *test)
> +{
> +	struct drm_modeset_acquire_ctx *ctx;
> +	int ret;
> +
> +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, ctx);
> +
> +	drm_modeset_acquire_init(ctx, 0);
> +
> +	ret = kunit_add_action_or_reset(test,
> +					action_drm_release_context,
> +					ctx);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return ctx;
> +}
> +EXPORT_SYMBOL_GPL(drm_kunit_helper_acquire_ctx_alloc);
> +

I think all of the patch inside this series are quite well.

Personally, I can't find problems in it.


But I still want to ask a question:

Should the managed functions you introduced be prefixed with drmm_ 
(instead of drm_) ?

As mindless programmer may still want to call drm_modeset_acquire_fini() 
on the pointer returned by

drm_kunit_helper_acquire_ctx_alloc()?


>   MODULE_AUTHOR("Maxime Ripard <maxime@...no.tech>");
>   MODULE_LICENSE("GPL");
> diff --git a/include/drm/drm_kunit_helpers.h b/include/drm/drm_kunit_helpers.h
> index ed013fdcc1ff..4ba5e10653c6 100644
> --- a/include/drm/drm_kunit_helpers.h
> +++ b/include/drm/drm_kunit_helpers.h
> @@ -87,5 +87,7 @@ __drm_kunit_helper_alloc_drm_device(struct kunit *test,
>   						      sizeof(_type),		\
>   						      offsetof(_type, _member),	\
>   						      _feat))
> +struct drm_modeset_acquire_ctx *
> +drm_kunit_helper_acquire_ctx_alloc(struct kunit *test);
>   
>   #endif // DRM_KUNIT_HELPERS_H_