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) <[email protected]>
> ---
>
> 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 <[email protected]>
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
