Re: [PATCH v4 18/25] drm/tilcdc: Convert to DRM managed resources

On Thu Jan 22, 2026 at 3:48 PM CET, Kory Maincent wrote:

On Mon, 19 Jan 2026 22:19:26 +0100
"Luca Ceresoli" [off-list ref] wrote:

quoted

On Fri Jan 16, 2026 at 6:02 PM CET, Kory Maincent (TI.com) wrote:

quoted

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.

quoted

quoted

+	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

quoted

 	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.

quoted

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.

quoted

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.

quoted

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