Hello Dmitry, thanks for reviewing!
On Mon, 23 Jun 2025 14:06:43 +0300 Dmitry Baryshkov <dmitry.barysh...@oss.qualcomm.com> wrote: > On 20/06/2025 18:59, Luca Ceresoli wrote: > > To the best of my knowledge, all drivers in the mainline kernel adding a > > DRM bridge are now converted to using devm_drm_bridge_alloc() for > > allocation and initialization. Among others this ensures initialization of > > the bridge refcount, allowing dynamic allocation lifetime. > > > > devm_drm_bridge_alloc() is now mandatory for all new bridges. Code using > > the old pattern ([devm_]kzalloc + filling the struct fields + > > drm_bridge_add) is not allowed anymore. > > > > Any drivers that might have been missed during the conversion, patches in > > flight towards mainline and out-of-tre drivers still using the old pattern > > will already be caught by a warning looking like: > > > > ------------[ cut here ]------------ > > refcount_t: addition on 0; use-after-free. > > WARNING: CPU: 2 PID: 83 at lib/refcount.c:25 > > refcount_warn_saturate+0x120/0x148 > > [...] > > Call trace: > > refcount_warn_saturate+0x120/0x148 (P) > > drm_bridge_get.part.0+0x70/0x98 [drm] > > drm_bridge_add+0x34/0x108 [drm] > > sn65dsi83_probe+0x200/0x480 [ti_sn65dsi83] > > [...] > > > > This warning comes from the refcount code and happens because > > drm_bridge_add() is increasing the refcount, which is uninitialized and > > thus initially zero. > > > > Having a warning and the corresponding stack trace is surely useful, but > > the warning text does not clarify the root problem nor how to fix it. > > > > Add a DRM_WARN() just before increasing the refcount, so the log will be > > much more readable: > > > > [drm] DRM bridge corrupted or not allocated by devm_drm_bridge_alloc() > > ------------[ cut here ]------------ > > refcount_t: addition on 0; use-after-free. > > [...etc...] > > > > A DRM_WARN is used because drm_warn and drm_WARN require a struct > > drm_device pointer which is not yet available when adding a bridge. > > > > Do not print the dev_name() in the warning because struct drm_bridge has no > > pointer to the struct device. The affected driver should be easy to catch > > based on the following stack trace however. > > > > Signed-off-by: Luca Ceresoli <luca.ceres...@bootlin.com> > > > > --- > > > > Changes in v9: > > - change warning trigger from "refcount != 1" to "container not NULL" > > > > This patch was added in v8 > > --- > > drivers/gpu/drm/drm_bridge.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > > index > > f001bbe95559aabf0aac9f25f89250ad4e1ad9c8..0b450b334afd82e0460f18fdd248f79d0a2b153d > > 100644 > > --- a/drivers/gpu/drm/drm_bridge.c > > +++ b/drivers/gpu/drm/drm_bridge.c > > @@ -295,6 +295,9 @@ EXPORT_SYMBOL(__devm_drm_bridge_alloc); > > */ > > void drm_bridge_add(struct drm_bridge *bridge) > > Can we rename this function, making sure that noone can call it > directly? E.g. adding two underscores might point out that is is an > internal API. I'm afraid I don't think this would be correct. Every bridge driver is expected to call drm_bridge_add() at the end of probe/initialization, to make the DRM subsystem aware that this bridge is ready for being used. The point of this patch, which is a completion to [1], is to ensure that all drivers use the "new" process: 1. bridge = devm_drm_bridge_alloc() 2. drm_bridge_add(bridge) and there are no users of the old process: 1. bridge = [devm_]kzalloc() 2. drm_bridge_add(bridge) Does this addresses your concern? [1] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=b3f13e00a8de351832df7d628a15ca4db49ca94f Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
