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

["Luca Ceresoli" <luca.ceresoli@bootlin.com>]

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