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
