Convert the tilcdc driver to use DRM managed resources (drmm_* APIs)
to eliminate resource lifetime issues, particularly in probe deferral
scenarios.

This conversion addresses potential use-after-free bugs by ensuring
proper cleanup ordering through the DRM managed resource framework.
The changes include:
- Replace drm_crtc_init_with_planes() with drmm_crtc_alloc_with_planes()
- Replace drm_universal_plane_init() with drmm_universal_plane_alloc()
- Replace drm_simple_encoder_init() with drmm_simple_encoder_alloc()
- Remove manual cleanup in tilcdc_crtc_destroy() and error paths
- Remove drm_encoder_cleanup() from encoder error handling paths
- Use drmm_add_action_or_reset() for remaining cleanup operations

This approach is recommended by the DRM subsystem for improved resource
lifetime management and is particularly important for drivers that may
experience probe deferral.

Signed-off-by: Kory Maincent (TI.com) <kory.maincent@bootlin.com>
---

Change in v4:
- Newt patch.
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c    | 54 +++++++++++++++++----------------
 drivers/gpu/drm/tilcdc/tilcdc_drv.c     |  5 +--
 drivers/gpu/drm/tilcdc/tilcdc_drv.h     | 13 ++++++--
 drivers/gpu/drm/tilcdc/tilcdc_encoder.c | 38 ++++++++---------------
 drivers/gpu/drm/tilcdc/tilcdc_plane.c   | 27 ++++++++---------
 5 files changed, 64 insertions(+), 73 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 0bd99a2efeeb4..1025643915052 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -16,6 +16,7 @@
 #include <drm/drm_fourcc.h>
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_dma_helper.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_print.h>
 #include <drm/drm_vblank.h>
@@ -30,7 +31,7 @@
 struct tilcdc_crtc {
 	struct drm_crtc base;
 
-	struct drm_plane primary;
+	struct tilcdc_plane *primary;
 	struct drm_pending_vblank_event *event;
 	struct mutex enable_lock;
 	bool enabled;
@@ -555,16 +556,15 @@ static void tilcdc_crtc_recover_work(struct work_struct *work)
 	drm_modeset_unlock(&crtc->mutex);
 }
 
-void tilcdc_crtc_destroy(struct drm_crtc *crtc)
+static void tilcdc_crtc_destroy(struct drm_device *dev, void *data)
 {
-	struct tilcdc_drm_private *priv = ddev_to_tilcdc_priv(crtc->dev);
+	struct tilcdc_drm_private *priv = (struct tilcdc_drm_private *)data;
 
-	tilcdc_crtc_shutdown(crtc);
+	tilcdc_crtc_shutdown(priv->crtc);
 
 	flush_workqueue(priv->wq);
 
-	of_node_put(crtc->port);
-	drm_crtc_cleanup(crtc);
+	of_node_put(priv->crtc->port);
 }
 
 int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
@@ -714,7 +714,6 @@ static void tilcdc_crtc_reset(struct drm_crtc *crtc)
 }
 
 static const struct drm_crtc_funcs tilcdc_crtc_funcs = {
-	.destroy        = tilcdc_crtc_destroy,
 	.set_config     = drm_atomic_helper_set_config,
 	.page_flip      = drm_atomic_helper_page_flip,
 	.reset		= tilcdc_crtc_reset,
@@ -960,12 +959,27 @@ int tilcdc_crtc_create(struct drm_device *dev)
 {
 	struct tilcdc_drm_private *priv = ddev_to_tilcdc_priv(dev);
 	struct tilcdc_crtc *tilcdc_crtc;
+	struct tilcdc_plane *primary;
 	struct drm_crtc *crtc;
 	int ret;
 
-	tilcdc_crtc = devm_kzalloc(dev->dev, sizeof(*tilcdc_crtc), GFP_KERNEL);
-	if (!tilcdc_crtc)
-		return -ENOMEM;
+	primary = tilcdc_plane_init(dev);
+	if (IS_ERR(primary)) {
+		dev_err(dev->dev, "Failed to initialize plane: %pe\n", primary);
+		return PTR_ERR(primary);
+	}
+
+	tilcdc_crtc = drmm_crtc_alloc_with_planes(dev, struct tilcdc_crtc, base,
+						  &primary->base,
+						  NULL,
+						  &tilcdc_crtc_funcs,
+						  "tilcdc crtc");
+	if (IS_ERR(tilcdc_crtc)) {
+		dev_err(dev->dev, "Failed to init CRTC: %pe\n", tilcdc_crtc);
+		return PTR_ERR(tilcdc_crtc);
+	}
+
+	tilcdc_crtc->primary = primary;
 
 	init_completion(&tilcdc_crtc->palette_loaded);
 	tilcdc_crtc->palette_base = dmam_alloc_coherent(dev->dev,
@@ -978,10 +992,6 @@ int tilcdc_crtc_create(struct drm_device *dev)
 
 	crtc = &tilcdc_crtc->base;
 
-	ret = tilcdc_plane_init(dev, &tilcdc_crtc->primary);
-	if (ret < 0)
-		goto fail;
-
 	mutex_init(&tilcdc_crtc->enable_lock);
 
 	init_waitqueue_head(&tilcdc_crtc->frame_done_wq);
@@ -989,20 +999,12 @@ int tilcdc_crtc_create(struct drm_device *dev)
 	spin_lock_init(&tilcdc_crtc->irq_lock);
 	INIT_WORK(&tilcdc_crtc->recover_work, tilcdc_crtc_recover_work);
 
-	ret = drm_crtc_init_with_planes(dev, crtc,
-					&tilcdc_crtc->primary,
-					NULL,
-					&tilcdc_crtc_funcs,
-					"tilcdc crtc");
-	if (ret < 0)
-		goto fail;
-
 	drm_crtc_helper_add(crtc, &tilcdc_crtc_helper_funcs);
 
+	ret = drmm_add_action_or_reset(dev, tilcdc_crtc_destroy, priv);
+	if (ret)
+		return ret;
+
 	priv->crtc = crtc;
 	return 0;
-
-fail:
-	tilcdc_crtc_destroy(crtc);
-	return ret;
 }
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 1a238a22309f4..3b11d296a7e91 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -392,7 +392,7 @@ static int tilcdc_pdev_probe(struct platform_device *pdev)
 	if (ret) {
 		dev_err(dev, "failed to register cpufreq notifier\n");
 		priv->freq_transition.notifier_call = NULL;
-		goto destroy_crtc;
+		goto disable_pm;
 	}
 #endif
 
@@ -442,9 +442,7 @@ static int tilcdc_pdev_probe(struct platform_device *pdev)
 #ifdef CONFIG_CPU_FREQ
 	cpufreq_unregister_notifier(&priv->freq_transition,
 				    CPUFREQ_TRANSITION_NOTIFIER);
-destroy_crtc:
 #endif
-	tilcdc_crtc_destroy(priv->crtc);
 disable_pm:
 	pm_runtime_disable(dev);
 	clk_put(priv->clk);
@@ -466,7 +464,6 @@ static void tilcdc_pdev_remove(struct platform_device *pdev)
 	cpufreq_unregister_notifier(&priv->freq_transition,
 				    CPUFREQ_TRANSITION_NOTIFIER);
 #endif
-	tilcdc_crtc_destroy(priv->crtc);
 	pm_runtime_disable(&pdev->dev);
 	clk_put(priv->clk);
 	destroy_workqueue(priv->wq);
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
index c69e279a2539d..17d152f9f0b69 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
@@ -77,7 +77,7 @@ struct tilcdc_drm_private {
 
 	struct drm_crtc *crtc;
 
-	struct drm_encoder *encoder;
+	struct tilcdc_encoder *encoder;
 	struct drm_connector *connector;
 
 	bool irq_enabled;
@@ -91,11 +91,18 @@ int tilcdc_crtc_create(struct drm_device *dev);
 irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc);
 void tilcdc_crtc_update_clk(struct drm_crtc *crtc);
 void tilcdc_crtc_shutdown(struct drm_crtc *crtc);
-void tilcdc_crtc_destroy(struct drm_crtc *crtc);
 int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
 		struct drm_framebuffer *fb,
 		struct drm_pending_vblank_event *event);
 
-int tilcdc_plane_init(struct drm_device *dev, struct drm_plane *plane);
+struct tilcdc_plane {
+	struct drm_plane base;
+};
+
+struct tilcdc_encoder {
+	struct drm_encoder base;
+};
+
+struct tilcdc_plane *tilcdc_plane_init(struct drm_device *dev);
 
 #endif /* __TILCDC_DRV_H__ */
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_encoder.c b/drivers/gpu/drm/tilcdc/tilcdc_encoder.c
index d42be3e16c536..1ee5761757a8c 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_encoder.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_encoder.c
@@ -37,13 +37,13 @@ int tilcdc_attach_bridge(struct drm_device *ddev, struct drm_bridge *bridge)
 	struct tilcdc_drm_private *priv = ddev_to_tilcdc_priv(ddev);
 	int ret;
 
-	priv->encoder->possible_crtcs = BIT(0);
+	priv->encoder->base.possible_crtcs = BIT(0);
 
-	ret = drm_bridge_attach(priv->encoder, bridge, NULL, 0);
+	ret = drm_bridge_attach(&priv->encoder->base, bridge, NULL, 0);
 	if (ret)
 		return ret;
 
-	priv->connector = tilcdc_encoder_find_connector(ddev, priv->encoder);
+	priv->connector = tilcdc_encoder_find_connector(ddev, &priv->encoder->base);
 	if (!priv->connector)
 		return -ENODEV;
 
@@ -53,6 +53,7 @@ int tilcdc_attach_bridge(struct drm_device *ddev, struct drm_bridge *bridge)
 int tilcdc_encoder_create(struct drm_device *ddev)
 {
 	struct tilcdc_drm_private *priv = ddev_to_tilcdc_priv(ddev);
+	struct tilcdc_encoder *encoder;
 	struct drm_bridge *bridge;
 	struct drm_panel *panel;
 	int ret;
@@ -64,33 +65,20 @@ int tilcdc_encoder_create(struct drm_device *ddev)
 	else if (ret)
 		return ret;
 
-	priv->encoder = devm_kzalloc(ddev->dev, sizeof(*priv->encoder), GFP_KERNEL);
-	if (!priv->encoder)
-		return -ENOMEM;
-
-	ret = drm_simple_encoder_init(ddev, priv->encoder,
-				      DRM_MODE_ENCODER_NONE);
-	if (ret) {
-		dev_err(ddev->dev, "drm_encoder_init() failed %d\n", ret);
-		return ret;
+	encoder = drmm_simple_encoder_alloc(ddev, struct tilcdc_encoder,
+					    base, DRM_MODE_ENCODER_NONE);
+	if (IS_ERR(encoder)) {
+		dev_err(ddev->dev, "drm_encoder_init() failed %pe\n", encoder);
+		return PTR_ERR(encoder);
 	}
+	priv->encoder = encoder;
 
 	if (panel) {
 		bridge = devm_drm_panel_bridge_add_typed(ddev->dev, panel,
 							 DRM_MODE_CONNECTOR_DPI);
-		if (IS_ERR(bridge)) {
-			ret = PTR_ERR(bridge);
-			goto err_encoder_cleanup;
-		}
+		if (IS_ERR(bridge))
+			return PTR_ERR(bridge);
 	}
 
-	ret = tilcdc_attach_bridge(ddev, bridge);
-	if (ret)
-		goto err_encoder_cleanup;
-
-	return 0;
-
-err_encoder_cleanup:
-	drm_encoder_cleanup(priv->encoder);
-	return ret;
+	return tilcdc_attach_bridge(ddev, bridge);
 }
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_plane.c b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
index a77a5b22ebd96..d98a1ae0e31f8 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_plane.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
@@ -14,7 +14,6 @@
 static const struct drm_plane_funcs tilcdc_plane_funcs = {
 	.update_plane	= drm_atomic_helper_update_plane,
 	.disable_plane	= drm_atomic_helper_disable_plane,
-	.destroy	= drm_plane_cleanup,
 	.reset		= drm_atomic_helper_plane_reset,
 	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
@@ -98,22 +97,20 @@ static const struct drm_plane_helper_funcs plane_helper_funcs = {
 	.atomic_update = tilcdc_plane_atomic_update,
 };
 
-int tilcdc_plane_init(struct drm_device *dev,
-		      struct drm_plane *plane)
+struct tilcdc_plane *tilcdc_plane_init(struct drm_device *dev)
 {
 	struct tilcdc_drm_private *priv = ddev_to_tilcdc_priv(dev);
-	int ret;
-
-	ret = drm_universal_plane_init(dev, plane, 1, &tilcdc_plane_funcs,
-				       priv->pixelformats,
-				       priv->num_pixelformats,
-				       NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
-	if (ret) {
-		dev_err(dev->dev, "Failed to initialize plane: %d\n", ret);
-		return ret;
-	}
+	struct tilcdc_plane *plane;
 
-	drm_plane_helper_add(plane, &plane_helper_funcs);
+	plane = drmm_universal_plane_alloc(dev, struct tilcdc_plane, base,
+					   1, &tilcdc_plane_funcs,
+					   priv->pixelformats,
+					   priv->num_pixelformats,
+					   NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (IS_ERR(plane))
+		return plane;
 
-	return 0;
+	drm_plane_helper_add(&plane->base, &plane_helper_funcs);
+
+	return plane;
 }

-- 
2.43.0

On Fri Jan 16, 2026 at 6:02 PM CET, Kory Maincent (TI.com) wrote:
> Convert the tilcdc driver to use DRM managed resources (drmm_* APIs)
> to eliminate resource lifetime issues, particularly in probe deferral
> scenarios.
>
> This conversion addresses potential use-after-free bugs by ensuring
> proper cleanup ordering through the DRM managed resource framework.
> The changes include:
> - Replace drm_crtc_init_with_planes() with drmm_crtc_alloc_with_planes()
> - Replace drm_universal_plane_init() with drmm_universal_plane_alloc()
> - Replace drm_simple_encoder_init() with drmm_simple_encoder_alloc()
> - Remove manual cleanup in tilcdc_crtc_destroy() and error paths
> - Remove drm_encoder_cleanup() from encoder error handling paths
> - Use drmm_add_action_or_reset() for remaining cleanup operations
>
> This approach is recommended by the DRM subsystem for improved resource
> lifetime management and is particularly important for drivers that may
> experience probe deferral.
>
> Signed-off-by: Kory Maincent (TI.com) <kory.maincent@bootlin.com>
> ---
>
> Change in v4:
> - Newt patch.

Why? Adding patches along the way does not help getting your series merged
timely. If there's a good reason for adding a new patch, please mention it
here.

> -void tilcdc_crtc_destroy(struct drm_crtc *crtc)
> +static void tilcdc_crtc_destroy(struct drm_device *dev, void *data)
>  {
> -	struct tilcdc_drm_private *priv = ddev_to_tilcdc_priv(crtc->dev);
> +	struct tilcdc_drm_private *priv = (struct tilcdc_drm_private *)data;
>
> -	tilcdc_crtc_shutdown(crtc);
> +	tilcdc_crtc_shutdown(priv->crtc);
>
>  	flush_workqueue(priv->wq);
>
> -	of_node_put(crtc->port);
> -	drm_crtc_cleanup(crtc);
> +	of_node_put(priv->crtc->port);
>  }
>
>  int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
> @@ -714,7 +714,6 @@ static void tilcdc_crtc_reset(struct drm_crtc *crtc)
>  }
>
>  static const struct drm_crtc_funcs tilcdc_crtc_funcs = {
> -	.destroy        = tilcdc_crtc_destroy,
>  	.set_config     = drm_atomic_helper_set_config,
>  	.page_flip      = drm_atomic_helper_page_flip,
>  	.reset		= tilcdc_crtc_reset,
> @@ -960,12 +959,27 @@ int tilcdc_crtc_create(struct drm_device *dev)
>  {
>  	struct tilcdc_drm_private *priv = ddev_to_tilcdc_priv(dev);
>  	struct tilcdc_crtc *tilcdc_crtc;
> +	struct tilcdc_plane *primary;
>  	struct drm_crtc *crtc;
>  	int ret;
>
> -	tilcdc_crtc = devm_kzalloc(dev->dev, sizeof(*tilcdc_crtc), GFP_KERNEL);
> -	if (!tilcdc_crtc)
> -		return -ENOMEM;
> +	primary = tilcdc_plane_init(dev);
> +	if (IS_ERR(primary)) {
> +		dev_err(dev->dev, "Failed to initialize plane: %pe\n", primary);
> +		return PTR_ERR(primary);
> +	}
> +
> +	tilcdc_crtc = drmm_crtc_alloc_with_planes(dev, struct tilcdc_crtc, base,
> +						  &primary->base,
> +						  NULL,
> +						  &tilcdc_crtc_funcs,
> +						  "tilcdc crtc");
> +	if (IS_ERR(tilcdc_crtc)) {
> +		dev_err(dev->dev, "Failed to init CRTC: %pe\n", tilcdc_crtc);
> +		return PTR_ERR(tilcdc_crtc);
> +	}
> +
> +	tilcdc_crtc->primary = primary;

(*) see below

>
>  	init_completion(&tilcdc_crtc->palette_loaded);
>  	tilcdc_crtc->palette_base = dmam_alloc_coherent(dev->dev,
> @@ -978,10 +992,6 @@ int tilcdc_crtc_create(struct drm_device *dev)
>
>  	crtc = &tilcdc_crtc->base;
>
> -	ret = tilcdc_plane_init(dev, &tilcdc_crtc->primary);
> -	if (ret < 0)
> -		goto fail;
> -
>  	mutex_init(&tilcdc_crtc->enable_lock);
>
>  	init_waitqueue_head(&tilcdc_crtc->frame_done_wq);
> @@ -989,20 +999,12 @@ int tilcdc_crtc_create(struct drm_device *dev)
>  	spin_lock_init(&tilcdc_crtc->irq_lock);
>  	INIT_WORK(&tilcdc_crtc->recover_work, tilcdc_crtc_recover_work);
>
> -	ret = drm_crtc_init_with_planes(dev, crtc,
> -					&tilcdc_crtc->primary,
> -					NULL,
> -					&tilcdc_crtc_funcs,
> -					"tilcdc crtc");
> -	if (ret < 0)
> -		goto fail;
> -
>  	drm_crtc_helper_add(crtc, &tilcdc_crtc_helper_funcs);
>
> +	ret = drmm_add_action_or_reset(dev, tilcdc_crtc_destroy, priv);
> +	if (ret)
> +		return ret;

Not related to your patch, but if the dmam_alloc_coherent() (not visible in
the diff) fails, tilcdc_crtc_destroy() won't be called. Is this intended?
At first sight this drmm_add_action_or_reset() should be moved at (*), just
after the allocation.

However being not related to your patch I'd leave this for another series
anyway, to avoid making this series a moving target.

> +
>  	priv->crtc = crtc;
>  	return 0;
> -
> -fail:
> -	tilcdc_crtc_destroy(crtc);
> -	return ret;
>  }

I find this patch hard to read and I think because it is converting
multiple things at once. Splitting it in small steps would have been nice,
even thought I'm not 100% sure it would have been doable.

Nevertheless it looks correct, so:

Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

On Mon, 19 Jan 2026 22:19:26 +0100
"Luca Ceresoli" <luca.ceresoli@bootlin.com> wrote:

> On Fri Jan 16, 2026 at 6:02 PM CET, Kory Maincent (TI.com) wrote:
> > Convert the tilcdc driver to use DRM managed resources (drmm_* APIs)
> > to eliminate resource lifetime issues, particularly in probe deferral
> > scenarios.
> >
> > This conversion addresses potential use-after-free bugs by ensuring
> > proper cleanup ordering through the DRM managed resource framework.
> > The changes include:
> > - Replace drm_crtc_init_with_planes() with drmm_crtc_alloc_with_planes()
> > - Replace drm_universal_plane_init() with drmm_universal_plane_alloc()
> > - Replace drm_simple_encoder_init() with drmm_simple_encoder_alloc()
> > - Remove manual cleanup in tilcdc_crtc_destroy() and error paths
> > - Remove drm_encoder_cleanup() from encoder error handling paths
> > - Use drmm_add_action_or_reset() for remaining cleanup operations
> >
> > This approach is recommended by the DRM subsystem for improved resource
> > lifetime management and is particularly important for drivers that may
> > experience probe deferral.
> >
> > Signed-off-by: Kory Maincent (TI.com) <kory.maincent@bootlin.com>
> > ---
> >
> > Change in v4:
> > - Newt patch.  
> 
> Why? Adding patches along the way does not help getting your series merged
> timely. If there's a good reason for adding a new patch, please mention it
> here.

Thanks for your review.

Sorry for that. The reason is that I faced a null pointer dereference koops if
for example the panel module is not installed. Then the
drm_of_find_panel_or_bridge() function return eprobe defer and something goes
wrong with the DRM resources. Using DRM managed resources solves it.
I will mention it for the v5.

> > +	tilcdc_crtc = drmm_crtc_alloc_with_planes(dev, struct tilcdc_crtc,
> > base,
> > +						  &primary->base,
> > +						  NULL,
> > +						  &tilcdc_crtc_funcs,
> > +						  "tilcdc crtc");
> > +	if (IS_ERR(tilcdc_crtc)) {
> > +		dev_err(dev->dev, "Failed to init CRTC: %pe\n",
> > tilcdc_crtc);
> > +		return PTR_ERR(tilcdc_crtc);
> > +	}
> > +
> > +	tilcdc_crtc->primary = primary;  
> 
> (*) see below
> 
> >
> >  	init_completion(&tilcdc_crtc->palette_loaded);
> >  	tilcdc_crtc->palette_base = dmam_alloc_coherent(dev->dev,
> > @@ -978,10 +992,6 @@ int tilcdc_crtc_create(struct drm_device *dev)
> >
> >  	crtc = &tilcdc_crtc->base;
> >
> > -	ret = tilcdc_plane_init(dev, &tilcdc_crtc->primary);
> > -	if (ret < 0)
> > -		goto fail;
> > -
> >  	mutex_init(&tilcdc_crtc->enable_lock);
> >
> >  	init_waitqueue_head(&tilcdc_crtc->frame_done_wq);
> > @@ -989,20 +999,12 @@ int tilcdc_crtc_create(struct drm_device *dev)
> >  	spin_lock_init(&tilcdc_crtc->irq_lock);
> >  	INIT_WORK(&tilcdc_crtc->recover_work, tilcdc_crtc_recover_work);
> >
> > -	ret = drm_crtc_init_with_planes(dev, crtc,
> > -					&tilcdc_crtc->primary,
> > -					NULL,
> > -					&tilcdc_crtc_funcs,
> > -					"tilcdc crtc");
> > -	if (ret < 0)
> > -		goto fail;
> > -
> >  	drm_crtc_helper_add(crtc, &tilcdc_crtc_helper_funcs);
> >
> > +	ret = drmm_add_action_or_reset(dev, tilcdc_crtc_destroy, priv);
> > +	if (ret)
> > +		return ret;  
> 
> Not related to your patch, but if the dmam_alloc_coherent() (not visible in
> the diff) fails, tilcdc_crtc_destroy() won't be called. Is this intended?
> At first sight this drmm_add_action_or_reset() should be moved at (*), just
> after the allocation.

You are totally right.

> However being not related to your patch I'd leave this for another series
> anyway, to avoid making this series a moving target.

I think it is related to this patch.
Before this patch there was no need for cleanup as the only action before the
dmam_alloc_coherent() was a devm_kzalloc().
Now the plane and the crtc are initialize before the dmam_alloc_coherent() so
the cleanup need to happen if it fails an error.

> I find this patch hard to read and I think because it is converting
> multiple things at once. Splitting it in small steps would have been nice,
> even thought I'm not 100% sure it would have been doable.

Yes, it brought more error when not converting the whole to DRM Managed
resources in one go.

> 
> Nevertheless it looks correct, so:
> 
> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

Thanks, but I will remove it due to the small change. 
Or maybe it is ok for you if I keep it with only the move of
drmm_add_action_or_reset().

Regards.
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

On Thu Jan 22, 2026 at 3:48 PM CET, Kory Maincent wrote:
> On Mon, 19 Jan 2026 22:19:26 +0100
> "Luca Ceresoli" <luca.ceresoli@bootlin.com> wrote:
>
>> On Fri Jan 16, 2026 at 6:02 PM CET, Kory Maincent (TI.com) wrote:
>> > Convert the tilcdc driver to use DRM managed resources (drmm_* APIs)
>> > to eliminate resource lifetime issues, particularly in probe deferral
>> > scenarios.
>> >
>> > This conversion addresses potential use-after-free bugs by ensuring
>> > proper cleanup ordering through the DRM managed resource framework.
>> > The changes include:
>> > - Replace drm_crtc_init_with_planes() with drmm_crtc_alloc_with_planes()
>> > - Replace drm_universal_plane_init() with drmm_universal_plane_alloc()
>> > - Replace drm_simple_encoder_init() with drmm_simple_encoder_alloc()
>> > - Remove manual cleanup in tilcdc_crtc_destroy() and error paths
>> > - Remove drm_encoder_cleanup() from encoder error handling paths
>> > - Use drmm_add_action_or_reset() for remaining cleanup operations
>> >
>> > This approach is recommended by the DRM subsystem for improved resource
>> > lifetime management and is particularly important for drivers that may
>> > experience probe deferral.
>> >
>> > Signed-off-by: Kory Maincent (TI.com) <kory.maincent@bootlin.com>
>> > ---
>> >
>> > Change in v4:
>> > - Newt patch.
>>
>> Why? Adding patches along the way does not help getting your series merged
>> timely. If there's a good reason for adding a new patch, please mention it
>> here.
>
> Thanks for your review.
>
> Sorry for that. The reason is that I faced a null pointer dereference koops if
> for example the panel module is not installed. Then the
> drm_of_find_panel_or_bridge() function return eprobe defer and something goes
> wrong with the DRM resources. Using DRM managed resources solves it.
> I will mention it for the v5.
>
>> > +	tilcdc_crtc = drmm_crtc_alloc_with_planes(dev, struct tilcdc_crtc,
>> > base,
>> > +						  &primary->base,
>> > +						  NULL,
>> > +						  &tilcdc_crtc_funcs,
>> > +						  "tilcdc crtc");
>> > +	if (IS_ERR(tilcdc_crtc)) {
>> > +		dev_err(dev->dev, "Failed to init CRTC: %pe\n",
>> > tilcdc_crtc);
>> > +		return PTR_ERR(tilcdc_crtc);
>> > +	}
>> > +
>> > +	tilcdc_crtc->primary = primary;
>>
>> (*) see below
>>
>> >
>> >  	init_completion(&tilcdc_crtc->palette_loaded);
>> >  	tilcdc_crtc->palette_base = dmam_alloc_coherent(dev->dev,
>> > @@ -978,10 +992,6 @@ int tilcdc_crtc_create(struct drm_device *dev)
>> >
>> >  	crtc = &tilcdc_crtc->base;
>> >
>> > -	ret = tilcdc_plane_init(dev, &tilcdc_crtc->primary);
>> > -	if (ret < 0)
>> > -		goto fail;
>> > -
>> >  	mutex_init(&tilcdc_crtc->enable_lock);
>> >
>> >  	init_waitqueue_head(&tilcdc_crtc->frame_done_wq);
>> > @@ -989,20 +999,12 @@ int tilcdc_crtc_create(struct drm_device *dev)
>> >  	spin_lock_init(&tilcdc_crtc->irq_lock);
>> >  	INIT_WORK(&tilcdc_crtc->recover_work, tilcdc_crtc_recover_work);
>> >
>> > -	ret = drm_crtc_init_with_planes(dev, crtc,
>> > -					&tilcdc_crtc->primary,
>> > -					NULL,
>> > -					&tilcdc_crtc_funcs,
>> > -					"tilcdc crtc");
>> > -	if (ret < 0)
>> > -		goto fail;
>> > -
>> >  	drm_crtc_helper_add(crtc, &tilcdc_crtc_helper_funcs);
>> >
>> > +	ret = drmm_add_action_or_reset(dev, tilcdc_crtc_destroy, priv);
>> > +	if (ret)
>> > +		return ret;
>>
>> Not related to your patch, but if the dmam_alloc_coherent() (not visible in
>> the diff) fails, tilcdc_crtc_destroy() won't be called. Is this intended?
>> At first sight this drmm_add_action_or_reset() should be moved at (*), just
>> after the allocation.
>
> You are totally right.
>
>> However being not related to your patch I'd leave this for another series
>> anyway, to avoid making this series a moving target.
>
> I think it is related to this patch.
> Before this patch there was no need for cleanup as the only action before the
> dmam_alloc_coherent() was a devm_kzalloc().
> Now the plane and the crtc are initialize before the dmam_alloc_coherent() so
> the cleanup need to happen if it fails an error.
>
>> I find this patch hard to read and I think because it is converting
>> multiple things at once. Splitting it in small steps would have been nice,
>> even thought I'm not 100% sure it would have been doable.
>
> Yes, it brought more error when not converting the whole to DRM Managed
> resources in one go.
>
>>
>> Nevertheless it looks correct, so:
>>
>> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>
> Thanks, but I will remove it due to the small change.
> Or maybe it is ok for you if I keep it with only the move of
> drmm_add_action_or_reset().

If you only move the drmm_add_action_or_reset() where I suggested you can
keep it.

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com