Re: [PATCH v4 4/4] drm/bridge: imx8qxp-pixel-link: get/put the next bridge

[Liu Ying <victor.liu@nxp.com>]

On Mon, Jan 26, 2026 at 07:18:47PM +0100, Luca Ceresoli wrote:
> Hello Liu,

Hello Luca,

> 
> On Mon Jan 26, 2026 at 9:06 AM CET, Liu Ying wrote:
>> Hi Luca,
>>
>> On Wed, Jan 07, 2026 at 10:56:29AM +0100, Luca Ceresoli wrote:
>>> This driver obtains a bridge pointer from of_drm_find_bridge() in the probe
>>> function and stores it until driver removal. of_drm_find_bridge() is
>>> deprecated. Move to of_drm_find_and_get_bridge() for the bridge to be
>>> refcounted and use bridge->next_bridge to put the reference on
>>> deallocation.
>>>
>>> This needs to be handled in various steps:
>>>
>>>  * the bridge returned of_drm_get_bridge() is stored in the local temporary
>>>    variable next_bridge whose scope is the for loop, so a cleanup action is
>>>    enough
>>>  * the value of next_bridge is copied into selected_bridge, potentially
>>>    more than once, so a cleanup action at function scope plus a
>>>    drm_bridge_put() in case of reassignment are enough
>>>  * on successful return selected_bridge is stored in bridge->next_bridge,
>>>    which ensures it is put when the bridge is deallocated
>>>
>>> Reviewed-by: Maxime Ripard <mripard@kernel.org>
>>> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> Thanks for having found the time to go into the details and provide a
> careful review of this series!
> 
>>> --- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
>>> +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
>>> @@ -23,7 +23,6 @@
>>>
>>>  struct imx8qxp_pixel_link {
>>>  	struct drm_bridge bridge;
>>> -	struct drm_bridge *next_bridge;
>>>  	struct device *dev;
>>>  	struct imx_sc_ipc *ipc_handle;
>>>  	u8 stream_id;
>>> @@ -140,7 +139,7 @@ static int imx8qxp_pixel_link_bridge_attach(struct drm_bridge *bridge,
>>>  	}
>>>
>>>  	return drm_bridge_attach(encoder,
>>> -				 pl->next_bridge, bridge,
>>> +				 pl->bridge.next_bridge, bridge,
>>>  				 DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>>  }
>>>
>>> @@ -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.

> 
> The idea is that for each pointer (which is a reference) we get a reference
> (refcount++) when the pointer is set and put the reference when that same
> pointer goes out of scope or is reset to NULL. "the pointer is set" can be
> either of_drm_find_and_get_bridge() or an assignment, as each of these
> operations creates another reference (pointer) to the same bridge.
> 
> Does it look correct?

Lol, I guess I need more coffee to read your logic of refcount get/put.

> 
>> 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.  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.

> 
>> ...
>> 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<-

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

-- 
Regards,
Liu Ying