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

On Thu Jan 29, 2026 at 9:18 AM CET, Liu Ying wrote:

On Thu, Jan 29, 2026 at 03:49:38PM +0800, Liu Ying wrote:

quoted

On Wed, Jan 28, 2026 at 04:58:18PM +0100, Luca Ceresoli wrote:

quoted

On Tue Jan 27, 2026 at 4:54 AM CET, Liu Ying wrote:

...

quoted

quoted

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

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?

Thanks for more explaination, maybe it becomes a bit clearer, I'm not sure:/

Anyway, to simplify things with another try, I came up with the below
snippet in that loop, which drops the two intermediate bridges(local
next_bridge and selected_bridge) and uses pl->next_bridge only.

Fix a typo:
s/pl->next_bridge/pl->bridge.next_bridge/

quoted

It looks ok to me(at least, refcount dance is much simpler).

-8<-
if (!pl->next_bridge || of_property_present(remote, "fsl,companion-pxl2dpi")) {
        drm_bridge_put(pl->next_bridge);
        pl->next_bridge = of_drm_find_and_get_bridge(remote);
        if (!pl->next_bridge)
                return -EPROBE_DEFER;
}
-8<-

-8<-
if (!pl->bridge.next_bridge || of_property_present(remote, "fsl,companion-pxl2dpi")) {
        drm_bridge_put(pl->bridge.next_bridge);
        pl->bridge.next_bridge = of_drm_find_and_get_bridge(remote);
        if (!pl->bridge.next_bridge)
                return -EPROBE_DEFER;
}
-8<-

It's OK enough, so in v5 I'm going to split the if() and skip the
intermediate selected_bridge variable.

Luca

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