Message-Id: <DG2YL94N216P.31GDSWQ0OD1PT@bootlin.com>
Date: Sat, 31 Jan 2026 18:43:08 +0100
From: "Luca Ceresoli" <luca.ceresoli@...tlin.com>
To: "Liu Ying" <victor.liu@....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>, "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>, "Shawn Guo"
 <shawnguo@...nel.org>, "Sascha Hauer" <s.hauer@...gutronix.de>,
 "Pengutronix Kernel Team" <kernel@...gutronix.de>, "Fabio Estevam"
 <festevam@...il.com>
Cc: "Hui Pu" <Hui.Pu@...ealthcare.com>, "Thomas Petazzoni"
 <thomas.petazzoni@...tlin.com>, <dri-devel@...ts.freedesktop.org>,
 <imx@...ts.linux.dev>, <linux-arm-kernel@...ts.infradead.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 4/4] drm/bridge: imx8qxp-pixel-link: get/put the next
 bridge

On Thu Jan 29, 2026 at 9:18 AM CET, Liu Ying wrote:
>
>
> On Thu, Jan 29, 2026 at 03:49:38PM +0800, Liu Ying wrote:
>>
>>
>> On Wed, Jan 28, 2026 at 04:58:18PM +0100, Luca Ceresoli wrote:
>>> On Tue Jan 27, 2026 at 4:54 AM CET, Liu Ying wrote:
>>>
>>> ...
>>>
>>>>>>> @@ -260,7 +259,7 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>>>>>>  {
>>>>>>>  	struct device_node *np = pl->dev->of_node;
>>>>>>>  	struct device_node *port;
>>>>>>> -	struct drm_bridge *selected_bridge = NULL;
>>>>>>> +	struct drm_bridge *selected_bridge __free(drm_bridge_put) = NULL;
>>>>>>>  	u32 port_id;
>>>>>>>  	bool found_port = false;
>>>>>>>  	int reg;
>>>>>>> @@ -297,7 +296,8 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>>>>>>  			continue;
>>>>>>>  		}
>>>>>>>
>>>>>>> -		struct drm_bridge *next_bridge = of_drm_find_bridge(remote);
>>>>>>> +		struct drm_bridge *next_bridge __free(drm_bridge_put) =
>>>>>>> +			of_drm_find_and_get_bridge(remote);
>>>>>>>  		if (!next_bridge)
>>>>>>>  			return -EPROBE_DEFER;
>>>>>>>
>>>>>>> @@ -305,12 +305,14 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>>>>>>  		 * Select the next bridge with companion PXL2DPI if
>>>>>>>  		 * present, otherwise default to the first bridge
>>>>>>>  		 */
>>>>>>> -		if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi"))
>>>>>>> -			selected_bridge = next_bridge;
>>>>>>> +		if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi")) {
>>>>>>> +			drm_bridge_put(selected_bridge);
>>>>>>> +			selected_bridge = drm_bridge_get(next_bridge);
>>>>>>
>>>>>> Considering selecting the first bridge without the companion pxl2dpi,
>>>>>> there would be a superfluous refcount for the selected bridge:
>>>>>>
>>>>>> 1) of_drm_find_and_get_bridge: refcount = 1
>>>>>> 2) drm_bridge_put: noop, since selected_bridge is NULL, refcount = 1
>>>>>> 3) drm_bridge_get: refcount = 2
>>>>>> 4) drm_bridge_put(__free): refcount = 1
>>>>>> 5) drm_bridge_get: for the pl->bridge.next_bridge, refcount = 2
>>>>>
>>>>> Here you are missing one put. There are two drm_bridge_put(__free), one for
>>>>> next_bridge and one for selected_bridge. So your list should rather be:
>>>>>
>>>>> 1) next_bridge = of_drm_find_and_get_bridge: refcount = 1
>>>>> 2) drm_bridge_put(selected_bridge): noop, since selected_bridge is NULL, refcount = 1
>>>>> 3) selected_bridge = drm_bridge_get: refcount = 2
>>>>> 4) drm_bridge_put(next_bridge) [__free at loop scope end]: refcount = 1
>>>>> 5) pl->bridge.next_bridge = drm_bridge_get(), refcount = 2
>>>>> 6) drm_bridge_put(selected_bridge) [__free at function scope end]: refcount = 1
>>>>
>>>> Ah, right, I did miss this last put because selected_bridge is declared with
>>>> __free a bit far away from the loop at the very beginning of
>>>> imx8qxp_pixel_link_find_next_bridge() - that's my problem I guess, but I'm
>>>> not even sure if I'll fall into this same pitfall again after a while, which
>>>> makes the driver difficult to maintain.
>>>>
>>>> Also, it seems that the refcount dance(back and forth bewteen 1 and 2) is not
>>>> something straightforward for driver readers to follow.
>>>
>>> I thing the whole logic becomes straightforward if you think it this way:
>>>
>>>  * when a pointer is assigned = a new reference starts existing -> refcount++
>>>  * when a pointer is cleared/overwritten or goes out of scope = a reference
>>>    stops existing -> refcount--
>>>
>>> In short: one pointer, one reference, one refcount.
>>>
>>> If you re-read the patch with this in mind, does it become clearer?
>>
>> Thanks for more explaination, maybe it becomes a bit clearer, I'm not sure:/
>>
>> Anyway, to simplify things with another try, I came up with the below
>> snippet in that loop, which drops the two intermediate bridges(local
>> next_bridge and selected_bridge) and uses pl->next_bridge only.
>
> Fix a typo:
> s/pl->next_bridge/pl->bridge.next_bridge/
>
>> It looks ok to me(at least, refcount dance is much simpler).
>>
>> -8<-
>> if (!pl->next_bridge || of_property_present(remote, "fsl,companion-pxl2dpi")) {
>>         drm_bridge_put(pl->next_bridge);
>>         pl->next_bridge = of_drm_find_and_get_bridge(remote);
>>         if (!pl->next_bridge)
>>                 return -EPROBE_DEFER;
>> }
>> -8<-
>
> -8<-
> if (!pl->bridge.next_bridge || of_property_present(remote, "fsl,companion-pxl2dpi")) {
>         drm_bridge_put(pl->bridge.next_bridge);
>         pl->bridge.next_bridge = of_drm_find_and_get_bridge(remote);
>         if (!pl->bridge.next_bridge)
>                 return -EPROBE_DEFER;
> }
> -8<-

It's OK enough, so in v5 I'm going to split the if() and skip the
intermediate selected_bridge variable.

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists