Message-ID: <oejxznxyvki6jg5rbrqcnwzmmygstypp24nbf3pn4nqgifmjpc@m3qqcgmuch6u>
Date:   Tue, 5 Dec 2023 16:13:47 +0100
From:   Michał Winiarski <michal.winiarski@...el.com>
To:     Maxime Ripard <mripard@...nel.org>
CC:     <intel-xe@...ts.freedesktop.org>,
        <dri-devel@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        Michal Wajdeczko <michal.wajdeczko@...el.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        "Javier Martinez Canillas" <javierm@...hat.com>,
        Maíra Canal <mcanal@...lia.com>
Subject: Re: [PATCH v2 3/3] drm/tests: managed: Add a simple test for
 drmm_managed_release

On Tue, Dec 05, 2023 at 02:50:22PM +0100, Maxime Ripard wrote:
> Hi,
> 
> Thanks for the rework
> 
> On Tue, Dec 05, 2023 at 02:22:10AM +0100, Michał Winiarski wrote:
> > Add a simple test that checks whether the action is indeed called right
> > away and that it is not called on the final drm_dev_put().
> > 
> > Signed-off-by: Michał Winiarski <michal.winiarski@...el.com>
> > ---
> >  drivers/gpu/drm/tests/drm_managed_test.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
> > index cabe6360aef71..8dfbea21c35c5 100644
> > --- a/drivers/gpu/drm/tests/drm_managed_test.c
> > +++ b/drivers/gpu/drm/tests/drm_managed_test.c
> > @@ -44,6 +44,29 @@ static void drm_test_managed_run_action(struct kunit *test)
> >  	KUNIT_EXPECT_GT(test, ret, 0);
> >  }
> >  
> 
> We should have a description of the intent of the test here.

The test checks that the release action is called immediately after
calling drmm_release_action, and that it is successfully removed from
the list of resources managed by DRMM (by verifying that release action
is not called upon device cleanup).
Would it be enough to expand the messages in KUNIT_EXPECT?

> 
> > +static void drm_test_managed_release_action(struct kunit *test)
> > +{
> > +	struct managed_test_priv *priv = test->priv;
> > +	int ret;
> > +
> > +	ret = drmm_add_action_or_reset(&priv->drm, drm_action, priv);
> > +	KUNIT_EXPECT_EQ(test, ret, 0);
> > +
> > +	ret = drm_dev_register(&priv->drm, 0);
> > +	KUNIT_ASSERT_EQ(test, ret, 0);
> > +
> > +	drmm_release_action(&priv->drm, drm_action, priv);
> > +	KUNIT_EXPECT_TRUE(test, priv->action_done);

KUNIT_EXPECT_TRUE_MSG(test, priv->action, "Release action was not called");

> > +	priv->action_done = false;
> > +
> > +	drm_dev_unregister(&priv->drm);
> > +	drm_kunit_helper_free_device(test, priv->drm.dev);
> > +
> > +	ret = wait_event_interruptible_timeout(priv->action_wq, priv->action_done,
> > +					       msecs_to_jiffies(TEST_TIMEOUT_MS));
> > +	KUNIT_EXPECT_EQ(test, ret, 0);

KUNIT_EXPECT_EQ_MSG(test, ret, 0, "Unexpected release action call during cleanup");

Or a comment on top? Kerneldoc? Not sure what the recommendation is, as
most (all?) tests in DRM don't have a description.

> 
> This tests that we have reached the timeout, thus the action never ran.

That's the intent, yes.

> It's not clear to me what your intent is here.
> 
> This test is:
> 
>   - Registering an action
>   - Registering the DRM device
>   - Calling drmm_release_action on the previously registered action
>   - Checking that the action has been run
>   - Clearing the flag saying the action has been run
>   - Unregistering the DRM device
>   - Freeing the DRM device
>   - Waiting for the action to run, but expecting it to never do?
> 
> I guess something like
> 
> static void drm_test_managed_release_action(struct kunit *test)
> {
> 	struct managed_test_priv *priv = test->priv;
> 	int ret;
> 
> 	ret = drmm_add_action_or_reset(&priv->drm, drm_action, priv);
> 	KUNIT_ASSERT_EQ(test, ret, 0);
> 
> 	KUNIT_ASSERT_FALSE(test, priv->action_done);
> 
> 	drmm_release_action(&priv->drm, drm_action, priv);
> 	ret = wait_event_interruptible_timeout(priv->action_wq, priv->action_done,
> 					       msecs_to_jiffies(TEST_TIMEOUT_MS));
> 	KUNIT_EXPECT_GT(test, ret, 0);
> 	KUNIT_EXPECT_TRUE(test, priv->action_done);
> }
> 
> would be enough?

It would only check that the action is called immediately, but not that
it was removed from the managed resources list. And we don't need to
wait for that - it should be called immediately.

We do need to wait when we expect it to be called (or not to be called -
in which case we expect a timeout) as part of device cleanup, otherwise
we would get a false positive when delayed release
(CONFIG_DEBUG_KOBJECT_RELEASE) is used.
I assumed that this was the intention behind introducing waitqueue in
drm_test_managed_run_action, is my understanding correct?

And BTW, now that I'm thinking about resource lifetimes. We can't really
tie priv lifetime with the device. It introduces use-after-free in both
tests, when checking if the action was called after
drm_kunit_helper_free_device() has a chance to invoke device cleanup.
Priv should be gone at that point, so I think we should go back to test
init from v1. Do you agree?

> 
> > +}
> > +
> >  static int drm_managed_test_init(struct kunit *test)
> >  {
> >  	struct managed_test_priv *priv;
> > @@ -65,6 +88,7 @@ static int drm_managed_test_init(struct kunit *test)
> >  
> >  static struct kunit_case drm_managed_tests[] = {
> >  	KUNIT_CASE(drm_test_managed_run_action),
> > +	KUNIT_CASE(drm_test_managed_release_action),
> 
> Also, tests should be organized by alphabetical order

Sure, I'll reorder it. I wasn't aware of that recommendation, as most of
the tests in DRM don't follow it.

Thanks,
-Michał

> 
> Maxime

Powered by blists - more mailing lists