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

["Luca Ceresoli" <luca.ceresoli@bootlin.com>]

Hello Liu,

On Mon Jan 26, 2026 at 7:18 PM CET, Luca Ceresoli 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
>
> 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?

Based on this discussion I thought the commit message could be clearer, and
rewrote it as:

-----[no changes from here...]-----
    drm/bridge: imx8qxp-pixel-link: get/put the next bridge

    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.
-----[...to here]-----

    To keep the code as simple and reliable as possible, get a reference for
    each pointer that stores a drm_bridge address when it is stored and release
    when the pointer is set to NULL or goes out of scope. The involved pointers
    are:

     * next_bridge loop-local variable:
       - get reference by of_drm_find_and_get_bridge()
       - put reference at the end of the loop iteration (__free)

     * selected_bridge function-local variable:
       - get reference when written (by copy from next_bridge)
       - put reference at function exit (__free)
       - put reference before reassignment too

     * pl->bridge.next_bridge, tied to struct imx8qxp_pixel_link lifetime:
       - get reference when written (by copy from selected_bridge)
       - put reference when the struct imx8qxp_pixel_link embedding the
         struct drm_bridge is destroyed (struct drm_bridge::next_bridge)

Do you think it's better now?

Luca

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