Re: [PATCH v4 18/25] drm/tilcdc: Convert to DRM managed resources
From: "Luca Ceresoli" <luca.ceresoli@bootlin.com>
Date: 2026-01-19 21:19:35
Also in:
dri-devel, linux-arm-kernel, linux-omap, lkml
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.
quoted hunk ↗ jump to hunk
-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
quoted hunk ↗ jump to hunk
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