[PATCH v5 0/5] drm/managed: Add drmm_release_action

[PATCH v5 0/5] 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

v3 -> v4:
- Split test changes into smaller patches (Maxime)
- Remove the waitqueue usage in tests
- Rename the test suite to match other DRM tests

v4 -> v5:
- Simplify the release test (Maxime)
- Rename suite to "drm_managed" (Maxime)
- Drop redundant messages from asserts (Maxime)

Michał Winiarski (5):
  drm/managed: Add drmm_release_action
  drm/tests: managed: Rename the suite name to match other DRM tests
  drm/tests: managed: Add comments about test intent
  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 | 77 +++++++++++++++++++-----
 include/drm/drm_managed.h                |  4 ++
 3 files changed, 104 insertions(+), 16 deletions(-)

-- 
2.43.0

[PATCH v5 1/5] 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>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
---
 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

[PATCH v5 2/5] drm/tests: managed: Rename the suite name to match other DRM tests

[Michał Winiarski]

DRM tests use "_" rather than "-" as word separator. Rename the test
suite to match other tests.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/tests/drm_managed_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
index 1652dca11d30c..f85dada4de0a9 100644
--- a/drivers/gpu/drm/tests/drm_managed_test.c
+++ b/drivers/gpu/drm/tests/drm_managed_test.c
@@ -61,7 +61,7 @@ static struct kunit_case drm_managed_tests[] = {
 };
 
 static struct kunit_suite drm_managed_test_suite = {
-	.name = "drm-test-managed",
+	.name = "drm_managed",
 	.test_cases = drm_managed_tests
 };
 
-- 
2.43.0

[PATCH v5 3/5] drm/tests: managed: Add comments about test intent

[Michał Winiarski]

Add comments explaining the intention behind the test and certain
implementation details related to device lifetime.

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

diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
index f85dada4de0a9..b5cf46d9f5cf8 100644
--- a/drivers/gpu/drm/tests/drm_managed_test.c
+++ b/drivers/gpu/drm/tests/drm_managed_test.c
@@ -24,6 +24,10 @@ 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;
@@ -38,6 +42,11 @@ static void drm_test_managed_run_action(struct kunit *test)
 	dev = drm_kunit_helper_alloc_device(test);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
 
+	/*
+	 * DRM device can't be embedded in priv, since priv->action_done needs
+	 * to remain allocated beyond both parent device and drm_device
+	 * lifetime.
+	 */
 	drm = __drm_kunit_helper_alloc_drm_device(test, dev, sizeof(*drm), 0, DRIVER_MODESET);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, drm);
 
-- 
2.43.0

[PATCH v5 4/5] 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>
Acked-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/tests/drm_managed_test.c | 41 +++++++++++++++---------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
index b5cf46d9f5cf8..d936c879a4a30 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;
 };
@@ -29,11 +30,28 @@ static void drm_action(struct drm_device *drm, void *ptr)
  * 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(test, ret, 0);
+}
+
+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);
@@ -47,21 +65,13 @@ static void drm_test_managed_run_action(struct kunit *test)
 	 * to remain allocated beyond both parent device and drm_device
 	 * lifetime.
 	 */
-	drm = __drm_kunit_helper_alloc_drm_device(test, dev, sizeof(*drm), 0, DRIVER_MODESET);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, drm);
+	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);
+	test->priv = priv;
 
-	ret = drm_dev_register(drm, 0);
-	KUNIT_ASSERT_EQ(test, ret, 0);
-
-	drm_dev_unregister(drm);
-	drm_kunit_helper_free_device(test, dev);
-
-	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[] = {
@@ -71,6 +81,7 @@ static struct kunit_case drm_managed_tests[] = {
 
 static struct kunit_suite drm_managed_test_suite = {
 	.name = "drm_managed",
+	.init = drm_managed_test_init,
 	.test_cases = drm_managed_tests
 };
 
-- 
2.43.0

[PATCH v5 5/5] drm/tests: managed: Add a simple test for drmm_managed_release

[Michał Winiarski]

Add a simple test that checks whether the action is called when
drmm_managed_release is called.

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

diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
index d936c879a4a30..76eb273c9b364 100644
--- a/drivers/gpu/drm/tests/drm_managed_test.c
+++ b/drivers/gpu/drm/tests/drm_managed_test.c
@@ -25,6 +25,30 @@ 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 when
+ * drmm_release_action is called.
+ */
+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);
+	ret = wait_event_interruptible_timeout(priv->action_wq, priv->action_done,
+					       msecs_to_jiffies(TEST_TIMEOUT_MS));
+	KUNIT_EXPECT_GT(test, ret, 0);
+
+	drm_dev_unregister(priv->drm);
+	drm_kunit_helper_free_device(test, priv->drm->dev);
+}
+
 /*
  * The test verifies that the release action is called automatically when the
  * device is released.
@@ -75,6 +99,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: (subset) [PATCH v5 1/5] drm/managed: Add drmm_release_action

[Maxime Ripard]

On Mon, 15 Jan 2024 18:13:47 +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.
> 
> 

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

Re: (subset) [PATCH v5 2/5] drm/tests: managed: Rename the suite name to match other DRM tests

[Maxime Ripard]

On Mon, 15 Jan 2024 18:13:48 +0100, Michał Winiarski wrote:
> DRM tests use "_" rather than "-" as word separator. Rename the test
> suite to match other tests.
> 
> 

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

Re: (subset) [PATCH v5 3/5] drm/tests: managed: Add comments about test intent

[Maxime Ripard]

On Mon, 15 Jan 2024 18:13:49 +0100, Michał Winiarski wrote:
> Add comments explaining the intention behind the test and certain
> implementation details related to device lifetime.
> 
> 

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

Re: (subset) [PATCH v5 4/5] drm/tests: managed: Extract device initialization into test init

[Maxime Ripard]

On Mon, 15 Jan 2024 18:13:50 +0100, Michał Winiarski wrote:
> It simplifies the process of extending the test suite with additional
> test cases without unnecessary duplication.
> 
> 

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

Re: (subset) [PATCH v5 5/5] drm/tests: managed: Add a simple test for drmm_managed_release

[Maxime Ripard]

On Mon, 15 Jan 2024 18:13:51 +0100, Michał Winiarski wrote:
> Add a simple test that checks whether the action is called when
> drmm_managed_release is called.
> 
> 

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime