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

[Javier Martinez Canillas <javierm@redhat.com>]

Maxime Ripard <mripard@kernel.org> writes:

Hello Maxime,

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

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

> Signed-off-by: Maxime Ripard <mripard@kernel.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