Message-ID: <67252c36-8b31-4c40-9d89-4f502da4a087@nxp.com>
Date: Thu, 22 May 2025 11:01:13 +0800
From: Liu Ying <victor.liu@....com>
To: Luca Ceresoli <luca.ceresoli@...tlin.com>
Cc: 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>,
 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>,
 Jagan Teki <jagan@...rulasolutions.com>, Shawn Guo <shawnguo@...nel.org>,
 Sascha Hauer <s.hauer@...gutronix.de>,
 Pengutronix Kernel Team <kernel@...gutronix.de>,
 Fabio Estevam <festevam@...il.com>, Douglas Anderson
 <dianders@...omium.org>, Chun-Kuang Hu <chunkuang.hu@...nel.org>,
 Krzysztof Kozlowski <krzk@...nel.org>, Anusha Srivatsa
 <asrivats@...hat.com>, Paul Kocialkowski <paulk@...-base.io>,
 Dmitry Baryshkov <lumag@...nel.org>, Hui Pu <Hui.Pu@...ealthcare.com>,
 Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 dri-devel@...ts.freedesktop.org, asahi@...ts.linux.dev,
 linux-kernel@...r.kernel.org, chrome-platform@...ts.linux.dev,
 imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org, linux-amlogic@...ts.infradead.org,
 linux-renesas-soc@...r.kernel.org, platform-driver-x86@...r.kernel.org,
 linux-samsung-soc@...r.kernel.org, linux-arm-msm@...r.kernel.org,
 freedreno@...ts.freedesktop.org, linux-stm32@...md-mailman.stormreply.com
Subject: Re: [PATCH v2 30/34] drm/bridge: imx8qxp-pixel-combiner: convert to
 devm_drm_bridge_alloc() API

On 05/07/2025, Luca Ceresoli wrote:

[...]

>> After looking into this patch and patch 31(though I've already provided my A-b)
>> more closely, I think the imx8qxp_pc and imx8{qm,qxp}_ldb main structures
>> should have the same life time with the embedded DRM bridges, because for
>> example the clk_apb clock in struct imx8qxp_pc would be accessed by the
>> imx8qxp_pc_bridge_mode_set DRM bridge callback.  But, IIUC, your patches extend
>> the life time for the embedded channel/bridge structures only, but not for the
>> main structures.  What do you think ?
> 
> I see you concern, but I'm sure the change I'm introducing is not
> creating the problem you are concerned about.
> 
> The key aspect is that my patch is merely changing the lifetime of the
> _allocation_ of the drm_bridge, not its usage. On drm_bridge_remove()
> the bridge is removed from its encoder chain and it is completely not
> reachable, both before and after my patch. With my patch it is not

drm_bridge_remove() only removes a bridge from the global bridge_list defined
in drm_bridge.c.  drm_bridge_detach() is the one which removes a bridge from
it's encoder chain.  It looks like you wrongly thought drm_bridge_remove()
is drm_bridge_detach().  So, even if drm_bridge_remove() is called, the removed
bridge could still be in it's encoder chain, hence an atomic commit could still
access the allocated bridge(with lifetime extended) and the clock_apb clock
for example in struct imx8qxp_pc could also be accessed.  That's why I think
the main structures should have the same lifetime with the allocated bridge.

> freed immediately, but it's just a piece of "wasted" memory that is
> still allocated until elsewhere in the kernel there are pointers to it,
> to avoid use-after-free.
> 
> With this explanation, do you think my patch is correct (after fixing
> the bug we already discussed of course)?
> 
> Best regards,
> Luca
> 

-- 
Regards,
Liu Ying

Powered by blists - more mailing lists