linux-kernel - Re: [PATCH 01/11] drm/tests: helpers: Switch to kunit actions

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]

Date:   Wed, 19 Jul 2023 20:42:51 +0200
From:   Javier Martinez Canillas <javierm@...hat.com>
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:     Maxime Ripard <mripard@...nel.org>, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH 01/11] drm/tests: helpers: Switch to kunit actions

Maxime Ripard <mripard@...nel.org> writes:

Hello Maxime,

The patch looks good to me. I've two questions below though.

Reviewed-by: Javier Martinez Canillas <javierm@...hat.com>

> Signed-off-by: Maxime Ripard <mripard@...nel.org>
> ---
>  drivers/gpu/drm/tests/drm_kunit_helpers.c | 32 +++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> index 4df47071dc88..38211fea9ae6 100644
> --- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> @@ -35,8 +35,8 @@ static struct platform_driver fake_platform_driver = {
>   * able to leverage the usual infrastructure and most notably the
>   * device-managed resources just like a "real" device.
>   *
> - * Callers need to make sure drm_kunit_helper_free_device() on the
> - * device when done.
> + * Resources will be cleaned up automatically, but the removal can be
> + * forced using @drm_kunit_helper_free_device.
>   *
>   * Returns:
>   * A pointer to the new device, or an ERR_PTR() otherwise.
> @@ -49,12 +49,31 @@ struct device *drm_kunit_helper_alloc_device(struct kunit *test)
>  	ret = platform_driver_register(&fake_platform_driver);
>  	KUNIT_ASSERT_EQ(test, ret, 0);
>  
> +	ret = kunit_add_action_or_reset(test,
> +					(kunit_action_t *)platform_driver_unregister,
> +					&fake_platform_driver);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
>  	pdev = platform_device_alloc(KUNIT_DEVICE_NAME, PLATFORM_DEVID_NONE);
>  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
>  
> +	ret = kunit_add_action_or_reset(test,
> +					(kunit_action_t *)platform_device_put,
> +					pdev);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
>  	ret = platform_device_add(pdev);
>  	KUNIT_ASSERT_EQ(test, ret, 0);
>  
> +	kunit_remove_action(test,
> +			    (kunit_action_t *)platform_device_put,
> +			    pdev);
> +

I understand that this action removal is because platform_device_put() is
not needed anymore after the platform_device_unregister() remove action is
registered since that already takes care of the platform_device_put().

But maybe add a comment to make more clear for someone who is not familiar
with these details of the platform core ?

>  EXPORT_SYMBOL_GPL(drm_kunit_helper_alloc_device);
> @@ -70,8 +89,13 @@ void drm_kunit_helper_free_device(struct kunit *test, struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
>  
> -	platform_device_unregister(pdev);
> -	platform_driver_unregister(&fake_platform_driver);
> +	kunit_release_action(test,
> +			     (kunit_action_t *)platform_device_unregister,
> +			     pdev);
> +
> +	kunit_release_action(test,
> +			     (kunit_action_t *)platform_driver_unregister,
> +			     &fake_platform_driver);
>  }
>  EXPORT_SYMBOL_GPL(drm_kunit_helper_free_device);
>

I thought the point of using the kunit cleanup actions was that you could
just make the kunit framework handle the release of resources and not do
it manually?

Can you just remove this function helper or is still needed in some cases?

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat