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 <[email protected]> >>> Signed-off-by: Luca Ceresoli <[email protected]> > > 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
