Message-Id: <DG0CHD0TAH9A.27UW4KKY2O9V7@bootlin.com>
Date: Wed, 28 Jan 2026 16:58:18 +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 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?

>>> I think the below snippet would be the right thing to do:
>>> -8<-
>>> {
>>> 	...
>>>
>>> 	struct drm_bridge *next_bridge __free(drm_bridge_put) =
>>> 		of_drm_find_and_get_bridge(remote);
>>>   		if (!next_bridge)
>>>   			return -EPROBE_DEFER;
>>>
>>> 	/*
>>> 	 * Select the next bridge with companion PXL2DPI if
>>> 	 * present, otherwise default to the first bridge
>>> 	 */
>>> 	if (!selected_bridge)
>>> 		selected_bridge = drm_bridge_get(next_bridge);
>>>
>>> 	if (of_property_present(remote, "fsl,companion-pxl2dpi")) {
>>> 		if (selected_bridge)
>>> 			drm_bridge_put(selected_bridge);
>>>
>>> 		selected_bridge = drm_bridge_get(next_bridge);
>>> 	}
>>> }
>>
>> Your version of the code looks OK as well so far, but totally equivalent to
>> what my patch proposes.
>>
>> Do you think splitting the if() into two if()s is clearer? Would you like
>> me to change this?
>
> Yes, please.  Two if()s are easier for me to read.

OK, will do.

> Also I think the
> "if (selected_bridge)" before "drm_bridge_put(selected_bridge)" improves
> readability, though I know drm_bridge_put() checks input parameter bridge
> for now.

I was about to reply "the NULL check in drm_bridge_put() is part of the API
contract as its documentation says", but then realized the documentation
does not say that. My bad, I was convinced I had documented that behaviour
to make it part of the contract so users can rely on it. I'm sending a
patch ASAP to document that.

>
>>
>>> ...
>>> pl->bridge.next_bridge = selected_bridge;
>>
>> Based on the logic above the drm_bridge_get() is still needed here (both
>> with the single if() or the split if()s) because at function exit the
>> selected_bridge reference will be put.
>
> Can the refcount dance be simplified a bit by dropping the put at
> function exit?  This snippet is what I'd propose if not too scary:
>
> -8<-
> 	struct drm_bridge *selected_bridge = NULL;
> 	...
>
> 	{
> 		...
>
> 		struct drm_bridge *next_bridge __free(drm_bridge_put) =
> 			of_drm_find_and_get_bridge(remote);
> 			if (!next_bridge)
> 				return -EPROBE_DEFER;
>
> 		/*
> 		 * Select the next bridge with companion PXL2DPI if
> 		 * present, otherwise default to the first bridge
> 		 */
> 		if (!selected_bridge)
> 			selected_bridge = drm_bridge_get(next_bridge);
>
> 		if (of_property_present(remote, "fsl,companion-pxl2dpi")) {
> 			if (selected_bridge)
> 				drm_bridge_put(selected_bridge);
>
> 			selected_bridge = drm_bridge_get(next_bridge);
> 		}
> 	}
>
> 	...
> 	pl->bridge.next_bridge = selected_bridge;
> -8<-

Based on the "one pointer, one reference, one refcount" logic I explained
above, I find this version more complex to understand. I read it as:
selected_bridge and pl->bridge.next_bridge are two pointers sharing a
single reference, and we know that would not create bugs because by careful
code inspection we realize that the life of the two references is
overlapped without a hole inbetween. I'm not a fan of this.

Luca

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

Powered by blists - more mailing lists