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

Hello Liu,

On Mon Jan 26, 2026 at 7:18 PM CET, Luca Ceresoli wrote:

quoted

quoted

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