[PATCH v3 0/3] drm/managed: Add drmm_release_action

[PATCH v3 0/3] drm/managed: Add drmm_release_action

[Michał Winiarski]

Upcoming Intel Xe driver will need to have a more fine-grained control
over DRM managed actions - namely, the ability to release a given
action, triggering it manually at a different point in time than the
final drm_dev_put().
This series adds a drmm_release_action function (which is similar to
devres devm_release_action) and a simple test that uses it.

v1 -> v2:
- Split the test changes (Maxime)
- Simplify priv lifetime management (Maxime)

v2 -> v3:
- Order tests alphabetically (Maxime)
- Add comments explaining the intention behind the tests and the reason
  why DRM device can't be embedded inside test priv (Maxime)
- Bring back priv lifetime management from v1 to avoid use-after-free

Michał Winiarski (3):
  drm/managed: Add drmm_release_action
  drm/tests: managed: Extract device initialization into test init
  drm/tests: managed: Add a simple test for drmm_managed_release

 drivers/gpu/drm/drm_managed.c            | 39 ++++++++++++
 drivers/gpu/drm/tests/drm_managed_test.c | 80 +++++++++++++++++++-----
 include/drm/drm_managed.h                |  4 ++
 3 files changed, 108 insertions(+), 15 deletions(-)

-- 
2.43.0

[PATCH v3 1/3] drm/managed: Add drmm_release_action

[Michał Winiarski]

Similar to devres equivalent, it allows to call the "release" action
directly and remove the resource from the managed resources list.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/drm_managed.c | 39 +++++++++++++++++++++++++++++++++++
 include/drm/drm_managed.h     |  4 ++++
 2 files changed, 43 insertions(+)

diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c
index bcd111404b128..7646f67bda4e4 100644
--- a/drivers/gpu/drm/drm_managed.c
+++ b/drivers/gpu/drm/drm_managed.c
@@ -176,6 +176,45 @@ int __drmm_add_action_or_reset(struct drm_device *dev,
 }
 EXPORT_SYMBOL(__drmm_add_action_or_reset);
 
+/**
+ * drmm_release_action - release a managed action from a &drm_device
+ * @dev: DRM device
+ * @action: function which would be called when @dev is released
+ * @data: opaque pointer, passed to @action
+ *
+ * This function calls the @action previously added by drmm_add_action()
+ * immediately.
+ * The @action is removed from the list of cleanup actions for @dev,
+ * which means that it won't be called in the final drm_dev_put().
+ */
+void drmm_release_action(struct drm_device *dev,
+			 drmres_release_t action,
+			 void *data)
+{
+	struct drmres *dr_match = NULL, *dr;
+	unsigned long flags;
+
+	spin_lock_irqsave(&dev->managed.lock, flags);
+	list_for_each_entry_reverse(dr, &dev->managed.resources, node.entry) {
+		if (dr->node.release == action) {
+			if (!data || (data && *(void **)dr->data == data)) {
+				dr_match = dr;
+				del_dr(dev, dr_match);
+				break;
+			}
+		}
+	}
+	spin_unlock_irqrestore(&dev->managed.lock, flags);
+
+	if (WARN_ON(!dr_match))
+		return;
+
+	action(dev, data);
+
+	free_dr(dr_match);
+}
+EXPORT_SYMBOL(drmm_release_action);
+
 /**
  * drmm_kmalloc - &drm_device managed kmalloc()
  * @dev: DRM device
diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h
index ad08f834af408..f547b09ca0239 100644
--- a/include/drm/drm_managed.h
+++ b/include/drm/drm_managed.h
@@ -45,6 +45,10 @@ int __must_check __drmm_add_action_or_reset(struct drm_device *dev,
 					    drmres_release_t action,
 					    void *data, const char *name);
 
+void drmm_release_action(struct drm_device *dev,
+			 drmres_release_t action,
+			 void *data);
+
 void *drmm_kmalloc(struct drm_device *dev, size_t size, gfp_t gfp) __malloc;
 
 /**
-- 
2.43.0

Re: [PATCH v3 1/3] drm/managed: Add drmm_release_action

[Maxime Ripard]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 342 bytes --]

On Mon, 11 Dec 2023 23:09:37 +0100, Michał Winiarski wrote:
> Similar to devres equivalent, it allows to call the "release" action
> directly and remove the resource from the managed resources list.
> 
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[PATCH v3 2/3] drm/tests: managed: Extract device initialization into test init

[Michał Winiarski]

It simplifies the process of extending the test suite with additional
test cases without unnecessary duplication.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/tests/drm_managed_test.c | 51 +++++++++++++++++-------
 1 file changed, 36 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
index 1652dca11d30c..15bd2474440b5 100644
--- a/drivers/gpu/drm/tests/drm_managed_test.c
+++ b/drivers/gpu/drm/tests/drm_managed_test.c
@@ -12,6 +12,7 @@
 #define TEST_TIMEOUT_MS	100
 
 struct managed_test_priv {
+	struct drm_device *drm;
 	bool action_done;
 	wait_queue_head_t action_wq;
 };
@@ -24,35 +25,54 @@ static void drm_action(struct drm_device *drm, void *ptr)
 	wake_up_interruptible(&priv->action_wq);
 }
 
+/*
+ * The test verifies that the release action is called automatically when the
+ * device is released.
+ */
 static void drm_test_managed_run_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);
+
+	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_GT_MSG(test, ret, 0, "Release action was not called");
+}
+
+static int drm_managed_test_init(struct kunit *test)
 {
 	struct managed_test_priv *priv;
-	struct drm_device *drm;
 	struct device *dev;
-	int ret;
 
 	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
-	init_waitqueue_head(&priv->action_wq);
 
 	dev = drm_kunit_helper_alloc_device(test);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
 
-	drm = __drm_kunit_helper_alloc_drm_device(test, dev, sizeof(*drm), 0, DRIVER_MODESET);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, drm);
+	/*
+	 * DRM device can't be embedded in priv, since priv->action_done needs
+	 * to remain allocated beyond both parent device and drm_device
+	 * lifetime.
+	 */
+	priv->drm = __drm_kunit_helper_alloc_drm_device(test, dev, sizeof(*priv->drm), 0,
+							DRIVER_MODESET);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->drm);
 
-	ret = drmm_add_action_or_reset(drm, drm_action, priv);
-	KUNIT_EXPECT_EQ(test, ret, 0);
-
-	ret = drm_dev_register(drm, 0);
-	KUNIT_ASSERT_EQ(test, ret, 0);
+	init_waitqueue_head(&priv->action_wq);
 
-	drm_dev_unregister(drm);
-	drm_kunit_helper_free_device(test, dev);
+	test->priv = priv;
 
-	ret = wait_event_interruptible_timeout(priv->action_wq, priv->action_done,
-					       msecs_to_jiffies(TEST_TIMEOUT_MS));
-	KUNIT_EXPECT_GT(test, ret, 0);
+	return 0;
 }
 
 static struct kunit_case drm_managed_tests[] = {
@@ -62,6 +82,7 @@ static struct kunit_case drm_managed_tests[] = {
 
 static struct kunit_suite drm_managed_test_suite = {
 	.name = "drm-test-managed",
+	.init = drm_managed_test_init,
 	.test_cases = drm_managed_tests
 };
 
-- 
2.43.0

Re: [PATCH v3 2/3] drm/tests: managed: Extract device initialization into test init

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3557 bytes --]

On Mon, Dec 11, 2023 at 11:09:38PM +0100, Michał Winiarski wrote:
> It simplifies the process of extending the test suite with additional
> test cases without unnecessary duplication.
> 
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>  drivers/gpu/drm/tests/drm_managed_test.c | 51 +++++++++++++++++-------
>  1 file changed, 36 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
> index 1652dca11d30c..15bd2474440b5 100644
> --- a/drivers/gpu/drm/tests/drm_managed_test.c
> +++ b/drivers/gpu/drm/tests/drm_managed_test.c
> @@ -12,6 +12,7 @@
>  #define TEST_TIMEOUT_MS	100
>  
>  struct managed_test_priv {
> +	struct drm_device *drm;
>  	bool action_done;
>  	wait_queue_head_t action_wq;
>  };
> @@ -24,35 +25,54 @@ static void drm_action(struct drm_device *drm, void *ptr)
>  	wake_up_interruptible(&priv->action_wq);
>  }
>  
> +/*
> + * The test verifies that the release action is called automatically when the
> + * device is released.
> + */
>  static void drm_test_managed_run_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);
> +
> +	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_GT_MSG(test, ret, 0, "Release action was not called");

The addition of the message should be in a separate patch

> +}
> +
> +static int drm_managed_test_init(struct kunit *test)
>  {
>  	struct managed_test_priv *priv;
> -	struct drm_device *drm;
>  	struct device *dev;
> -	int ret;
>  
>  	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
>  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
> -	init_waitqueue_head(&priv->action_wq);
>  
>  	dev = drm_kunit_helper_alloc_device(test);
>  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
>  
> -	drm = __drm_kunit_helper_alloc_drm_device(test, dev, sizeof(*drm), 0, DRIVER_MODESET);
> -	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, drm);
> +	/*
> +	 * DRM device can't be embedded in priv, since priv->action_done needs
> +	 * to remain allocated beyond both parent device and drm_device
> +	 * lifetime.
> +	 */
> +	priv->drm = __drm_kunit_helper_alloc_drm_device(test, dev, sizeof(*priv->drm), 0,
> +							DRIVER_MODESET);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->drm);
>  
> -	ret = drmm_add_action_or_reset(drm, drm_action, priv);
> -	KUNIT_EXPECT_EQ(test, ret, 0);
> -
> -	ret = drm_dev_register(drm, 0);
> -	KUNIT_ASSERT_EQ(test, ret, 0);
> +	init_waitqueue_head(&priv->action_wq);
>  
> -	drm_dev_unregister(drm);
> -	drm_kunit_helper_free_device(test, dev);
> +	test->priv = priv;
>  
> -	ret = wait_event_interruptible_timeout(priv->action_wq, priv->action_done,
> -					       msecs_to_jiffies(TEST_TIMEOUT_MS));
> -	KUNIT_EXPECT_GT(test, ret, 0);
> +	return 0;
>  }
>  
>  static struct kunit_case drm_managed_tests[] = {
> @@ -62,6 +82,7 @@ static struct kunit_case drm_managed_tests[] = {
>  
>  static struct kunit_suite drm_managed_test_suite = {
>  	.name = "drm-test-managed",
> +	.init = drm_managed_test_init,
>  	.test_cases = drm_managed_tests
>  };
>  
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v3 3/3] drm/tests: managed: Add a simple test for drmm_managed_release

[Michał Winiarski]

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@intel.com>
---
 drivers/gpu/drm/tests/drm_managed_test.c | 29 ++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
index 15bd2474440b5..ef5e784afbc6d 100644
--- a/drivers/gpu/drm/tests/drm_managed_test.c
+++ b/drivers/gpu/drm/tests/drm_managed_test.c
@@ -48,6 +48,34 @@ static void drm_test_managed_run_action(struct kunit *test)
 	KUNIT_EXPECT_GT_MSG(test, ret, 0, "Release action was not called");
 }
 
+/*
+ * The test verifies that the release action is called immediately when
+ * drmm_release_action is called and that it is not called for a second time
+ * when the device is released.
+ */
+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_MSG(test, priv->action_done, "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_MSG(test, ret, 0, "Unexpected release action call during cleanup");
+}
+
 static int drm_managed_test_init(struct kunit *test)
 {
 	struct managed_test_priv *priv;
@@ -76,6 +104,7 @@ static int drm_managed_test_init(struct kunit *test)
 }
 
 static struct kunit_case drm_managed_tests[] = {
+	KUNIT_CASE(drm_test_managed_release_action),
 	KUNIT_CASE(drm_test_managed_run_action),
 	{}
 };
-- 
2.43.0

Re: [PATCH v3 3/3] drm/tests: managed: Add a simple test for drmm_managed_release

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2707 bytes --]

Hi,

On Mon, Dec 11, 2023 at 11:09:39PM +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@intel.com>
> ---
>  drivers/gpu/drm/tests/drm_managed_test.c | 29 ++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
> index 15bd2474440b5..ef5e784afbc6d 100644
> --- a/drivers/gpu/drm/tests/drm_managed_test.c
> +++ b/drivers/gpu/drm/tests/drm_managed_test.c
> @@ -48,6 +48,34 @@ static void drm_test_managed_run_action(struct kunit *test)
>  	KUNIT_EXPECT_GT_MSG(test, ret, 0, "Release action was not called");
>  }
>  
> +/*
> + * The test verifies that the release action is called immediately when
> + * drmm_release_action is called and that it is not called for a second time
> + * when the device is released.
> + */

Thanks, it's much clearer now.

> +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_MSG(test, priv->action_done, "Release action was not called");
> +	priv->action_done = false;

We should call wait_event_* here.

> +
> +	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_MSG(test, ret, 0, "Unexpected release action call during cleanup");
> +}
> +

Tests should in general be as fast as possible. Waiting for 100ms for
the success case is not ok. We have ~500 tests at the moment, if every
test was doing that it would take at least 50s to run all our unit
tests, while it takes less than a second at the moment on a capable
machine.

And also, I'm not sure we actually need to make sure it never happened.
If only because nothing actually guarantees it wouldn't have happened
after the timeout anyway, so the test isn't definitive.

I guess what we could test is whether the action is still in the actions
list through a function only exported to tests. If it's no longer in the
action list, then it won't be run.

But unless we ever have a bug, I'm not sure it's worth testing for that.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 3/3] drm/tests: managed: Add a simple test for drmm_managed_release

[Michał Winiarski]

On Fri, Dec 15, 2023 at 05:31:38PM +0100, Maxime Ripard wrote:
> Hi,
> 
> On Mon, Dec 11, 2023 at 11:09:39PM +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@intel.com>
> > ---
> >  drivers/gpu/drm/tests/drm_managed_test.c | 29 ++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
> > index 15bd2474440b5..ef5e784afbc6d 100644
> > --- a/drivers/gpu/drm/tests/drm_managed_test.c
> > +++ b/drivers/gpu/drm/tests/drm_managed_test.c
> > @@ -48,6 +48,34 @@ static void drm_test_managed_run_action(struct kunit *test)
> >  	KUNIT_EXPECT_GT_MSG(test, ret, 0, "Release action was not called");
> >  }
> >  
> > +/*
> > + * The test verifies that the release action is called immediately when
> > + * drmm_release_action is called and that it is not called for a second time
> > + * when the device is released.
> > + */
> 
> Thanks, it's much clearer now.
> 
> > +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_MSG(test, priv->action_done, "Release action was not called");
> > +	priv->action_done = false;
> 
> We should call wait_event_* here.
> 
> > +
> > +	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_MSG(test, ret, 0, "Unexpected release action call during cleanup");
> > +}
> > +
> 
> Tests should in general be as fast as possible. Waiting for 100ms for
> the success case is not ok. We have ~500 tests at the moment, if every
> test was doing that it would take at least 50s to run all our unit
> tests, while it takes less than a second at the moment on a capable
> machine.
> 
> And also, I'm not sure we actually need to make sure it never happened.
> If only because nothing actually guarantees it wouldn't have happened
> after the timeout anyway, so the test isn't definitive.

There's no difference in that regard (test being definitive) between
drm_test_managed_release_action and pre-existing
drm_test_managed_run_action.

If the action is executed after the timeout, with run_action we're going
to get a false-negative result, and with release_action we're going to
get a false-positive result.

> I guess what we could test is whether the action is still in the actions
> list through a function only exported to tests. If it's no longer in the
> action list, then it won't be run.

That would require digging into implementation details rather than
focusing on the interface, which, in my opinion, is not a very good
approach.

In the next revision I'll drop the waitqueue completely. If that won't
work, I also have a variant that uses bus notifier to make both tests
more definitive.

Thanks,
-Michał

> But unless we ever have a bug, I'm not sure it's worth testing for that.
> 
> Maxime