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

  Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/tests/drm_kunit_helpers.c | 32 +++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)
  

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?
  

Hi Javier,

Thanks for reviewing the series

On Wed, Jul 19, 2023 at 08:42:51PM +0200, Javier Martinez Canillas wrote:
> > 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().

It's not so much that it's not needed after platform_device_unregister,
but rather that once you've called paltform_device_add my understanding
was that you didn't need it anymore.

I can't find where I got that from though, so I might be wrong. It also
looks like I could just use platform_device_del directly and not cancel
platform_device_put which will make it more obvious.

> 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?

You're right

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

We still need it for the drmm execution test where we would force the
removal of the device. All other tests at the moment shouldn't call it.

Maxime