Message-ID: <20260122154840.5185671a@kmaincent-XPS-13-7390>
Date: Thu, 22 Jan 2026 15:48:40 +0100
From: Kory Maincent <kory.maincent@...tlin.com>
To: "Luca Ceresoli" <luca.ceresoli@...tlin.com>
Cc: "Jyri Sarha" <jyri.sarha@....fi>, "Tomi Valkeinen"
 <tomi.valkeinen@...asonboard.com>, "Maarten Lankhorst"
 <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard" <mripard@...nel.org>,
 "Thomas Zimmermann" <tzimmermann@...e.de>, "David Airlie"
 <airlied@...il.com>, "Simona Vetter" <simona@...ll.ch>, "Rob Herring"
 <robh@...nel.org>, "Krzysztof Kozlowski" <krzk+dt@...nel.org>, "Conor
 Dooley" <conor+dt@...nel.org>, "Russell King" <linux@...linux.org.uk>,
 "Bartosz Golaszewski" <brgl@...ev.pl>, "Tony Lindgren" <tony@...mide.com>,
 "Andrzej Hajda" <andrzej.hajda@...el.com>, "Neil Armstrong"
 <neil.armstrong@...aro.org>, "Robert Foss" <rfoss@...nel.org>, "Laurent
 Pinchart" <Laurent.pinchart@...asonboard.com>, "Jonas Karlman"
 <jonas@...boo.se>, "Jernej Skrabec" <jernej.skrabec@...il.com>, "Markus
 Schneider-Pargmann" <msp@...libre.com>, "Bajjuri Praneeth"
 <praneeth@...com>, "Louis Chauvet" <louis.chauvet@...tlin.com>, "Thomas
 Petazzoni" <thomas.petazzoni@...tlin.com>, "Miguel Gazquez"
 <miguel.gazquez@...tlin.com>, "Herve Codina" <herve.codina@...tlin.com>,
 <dri-devel@...ts.freedesktop.org>, <devicetree@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
 <linux-omap@...r.kernel.org>
Subject: Re: [PATCH v4 18/25] drm/tilcdc: Convert to DRM managed resources

On Mon, 19 Jan 2026 22:19:26 +0100
"Luca Ceresoli" <luca.ceresoli@...tlin.com> 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) <kory.maincent@...tlin.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.

> > +	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 <luca.ceresoli@...tlin.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().

Regards.
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Powered by blists - more mailing lists