On Mon, 19 Jan 2026 22:19:26 +0100 "Luca Ceresoli" <[email protected]> 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) <[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. 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 <[email protected]> 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
