[PATCH v4 0/4] drm/bridge: add of_drm_find_and_get_bridge() and a managed *next_bridge, deprecate of_drm_find_bridge()

[PATCH v4 0/4] drm/bridge: add of_drm_find_and_get_bridge() and a managed *next_bridge, deprecate of_drm_find_bridge()

[Luca Ceresoli]

This v4 contains the leftovers of the v3 series that deprecated
of_drm_find_bridge(), added a replacement which handles bridge refcounting,
and converted some of the direct users. All patches have been applied
except the imx8qxp-pixel-link ones, which still need a review for the first
patch.

@Liu, patch 1 is waiting for a review and it's non-trivial, it would be
nice if you could have a look at that one at least.

This is part of the work to support hotplug of DRM bridges. The grand plan
was discussed in [0].

Here's the work breakdown (➜ marks the current series):

 1. ➜ add refcounting to DRM bridges struct drm_bridge,
      based on devm_drm_bridge_alloc()
    A. ✔ add new alloc API and refcounting (v6.16)
    B. ✔ convert all bridge drivers to new API (v6.17)
    C. ✔ kunit tests (v6.17)
    D. ✔ add get/put to drm_bridge_add/remove() + attach/detach()
         and warn on old allocation pattern (v6.17)
    E. ➜ add get/put on drm_bridge accessors
       1. ✔ drm_bridge_chain_get_first_bridge(), add cleanup action (v6.18)
       2. ✔ drm_bridge_get_prev_bridge() (v6.18)
       3. ✔ drm_bridge_get_next_bridge() (v6.19)
       4. ✔ drm_for_each_bridge_in_chain() (v6.19)
       5. ✔ drm_bridge_connector_init (v6.19)
       6. … protect encoder bridge chain with a mutex
       7. ➜ of_drm_find_bridge()
          a. ➜✔… add of_drm_get_bridge(), convert basic direct users
	         (v6.20?, one driver still pending)
	  b. convert other direct users
	  c. convert bridge-only drm_of_find_panel_or_bridge() users
       8. drm_of_find_panel_or_bridge, *_of_get_bridge
       9. ✔ enforce drm_bridge_add before drm_bridge_attach (v6.19)
    F. ✔ debugfs improvements
       1. ✔ add top-level 'bridges' file (v6.16)
       2. ✔ show refcount and list lingering bridges (v6.19)
 2. … handle gracefully atomic updates during bridge removal
    A. ✔ Add drm_dev_enter/exit() to protect device resources (v6.20?)
    B. … protect private_obj removal from list
 3. … DSI host-device driver interaction
 4. ✔ removing the need for the "always-disconnected" connector
 5. finish the hotplug bridge work, moving code to the core and potentially
    removing the hotplug-bridge itself (this needs to be clarified as
    points 1-3 are developed)

[0] https://lore.kernel.org/lkml/20250206-hotplug-drm-bridge-v6-0-9d6f2c9c3058@bootlin.com/#t

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Changes in v4:
- Added review trailers
- Link to v3: https://lore.kernel.org/r/20251216-drm-bridge-alloc-getput-drm_of_find_bridge-v3-0-b5165fab8058@bootlin.com

Changes in v3:
- Completely rewrote using the __drm_bridge_free() idea to prevent
  use-after-free of the next_bridge for the common cases
- Added needed cleanups to the imx8qxp-pixel-link and imx8qxp-pxl2dpi
  drivers
- Removed various patches converting simple cases, to reduce the number of
  e-mails sent; will be moved to the follow-up series
- Link to v2: https://lore.kernel.org/r/20251128-drm-bridge-alloc-getput-drm_of_find_bridge-v2-0-88f8a107eca2@bootlin.com

Changes in v2:
- All patches: renamed drm_of_find_bridge() -> of_drm_get_bridge()
- Various fixes and improvements to patches 1-6, see individual patches
  changelog
- Removed bouncing recipient: Edmund Dea <edmund.j.dea@intel.com>
- Link to v1: https://lore.kernel.org/r/20251119-drm-bridge-alloc-getput-drm_of_find_bridge-v1-0-0db98a7fe474@bootlin.com

---
Luca Ceresoli (4):
      drm/bridge: imx8qxp-pixel-link: simplify logic to find next bridge
      drm/bridge: imx8qxp-pixel-link: simplify freeing of the remote device_node
      drm/bridge: imx8qxp-pixel-link: imx8qxp_pixel_link_find_next_bridge: return int, not ERR_PTR
      drm/bridge: imx8qxp-pixel-link: get/put the next bridge

 drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c | 53 ++++++++++++-------------
 1 file changed, 25 insertions(+), 28 deletions(-)
---
base-commit: f12ad2e5233a1a30b3bd6fe1e784b3544caa2383
change-id: 20251117-drm-bridge-alloc-getput-drm_of_find_bridge-74903367448d

Best regards,
-- 
Luca Ceresoli <luca.ceresoli@bootlin.com>

[PATCH v4 1/4] drm/bridge: imx8qxp-pixel-link: simplify logic to find next bridge

[Luca Ceresoli]

imx8qxp_pixel_link_find_next_bridge() uses a sophisticated logic to find
the preferred next bridge, using an array with two supporting index
variables. This is more sophisticated than required because we only ever
need a pointer to the "current" bridge and to the "best so far" bridge.

Additionally this logic is going to make the addition of proper refcounting
quite complex.

Rewrite the logic using two drm_bridge pointers, which is by itself
slightly simpler and is a preparation step for introducing bridge
refcounting in a later commit.

Also reword a comment to make it clearer.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
index 433c080197a2..4f84825fddca 100644
--- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
+++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
@@ -261,12 +261,10 @@ imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
 {
 	struct device_node *np = pl->dev->of_node;
 	struct device_node *port, *remote;
-	struct drm_bridge *next_bridge[PL_MAX_NEXT_BRIDGES];
+	struct drm_bridge *selected_bridge = NULL;
 	u32 port_id;
 	bool found_port = false;
-	int reg, ep_cnt = 0;
-	/* select the first next bridge by default */
-	int bridge_sel = 0;
+	int reg;
 
 	for (port_id = 1; port_id <= PL_MAX_MST_ADDR + 1; port_id++) {
 		port = of_graph_get_port_by_id(np, port_id);
@@ -300,24 +298,25 @@ imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
 			continue;
 		}
 
-		next_bridge[ep_cnt] = of_drm_find_bridge(remote);
-		if (!next_bridge[ep_cnt]) {
+		struct drm_bridge *next_bridge = of_drm_find_bridge(remote);
+		if (!next_bridge) {
 			of_node_put(remote);
 			return ERR_PTR(-EPROBE_DEFER);
 		}
 
-		/* specially select the next bridge with companion PXL2DPI */
-		if (of_property_present(remote, "fsl,companion-pxl2dpi"))
-			bridge_sel = ep_cnt;
-
-		ep_cnt++;
+		/*
+		 * 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;
 
 		of_node_put(remote);
 	}
 
 	pl->mst_addr = port_id - 1;
 
-	return next_bridge[bridge_sel];
+	return selected_bridge;
 }
 
 static int imx8qxp_pixel_link_bridge_probe(struct platform_device *pdev)

-- 
2.52.0

Re: [PATCH v4 1/4] drm/bridge: imx8qxp-pixel-link: simplify logic to find next bridge

[Liu Ying]

On Wed, Jan 07, 2026 at 10:56:26AM +0100, Luca Ceresoli wrote:
> imx8qxp_pixel_link_find_next_bridge() uses a sophisticated logic to find
> the preferred next bridge, using an array with two supporting index
> variables. This is more sophisticated than required because we only ever
> need a pointer to the "current" bridge and to the "best so far" bridge.
> 
> Additionally this logic is going to make the addition of proper refcounting
> quite complex.
> 
> Rewrite the logic using two drm_bridge pointers, which is by itself
> slightly simpler and is a preparation step for introducing bridge
> refcounting in a later commit.
> 
> Also reword a comment to make it clearer.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
>  drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)

Reviewed-by: Liu Ying <victor.liu@nxp.com>

[PATCH v4 2/4] drm/bridge: imx8qxp-pixel-link: simplify freeing of the remote device_node

[Luca Ceresoli]

The main loop in imx8qxp_pixel_link_find_next_bridge() requires calling
of_node_put() in multiple places, complicating code flow. Simplify it by
using a cleanup action and making the 'remote' variable scope local to the
loop.

Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
index 4f84825fddca..0c5ed06eee1b 100644
--- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
+++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
@@ -260,7 +260,7 @@ static struct drm_bridge *
 imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
 {
 	struct device_node *np = pl->dev->of_node;
-	struct device_node *port, *remote;
+	struct device_node *port;
 	struct drm_bridge *selected_bridge = NULL;
 	u32 port_id;
 	bool found_port = false;
@@ -286,7 +286,8 @@ imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
 	}
 
 	for (reg = 0; reg < PL_MAX_NEXT_BRIDGES; reg++) {
-		remote = of_graph_get_remote_node(np, port_id, reg);
+		struct device_node *remote __free(device_node) =
+			of_graph_get_remote_node(np, port_id, reg);
 		if (!remote)
 			continue;
 
@@ -294,15 +295,12 @@ imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
 			DRM_DEV_DEBUG(pl->dev,
 				      "port%u endpoint%u remote parent is not available\n",
 				      port_id, reg);
-			of_node_put(remote);
 			continue;
 		}
 
 		struct drm_bridge *next_bridge = of_drm_find_bridge(remote);
-		if (!next_bridge) {
-			of_node_put(remote);
+		if (!next_bridge)
 			return ERR_PTR(-EPROBE_DEFER);
-		}
 
 		/*
 		 * Select the next bridge with companion PXL2DPI if
@@ -310,8 +308,6 @@ imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
 		 */
 		if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi"))
 			selected_bridge = next_bridge;
-
-		of_node_put(remote);
 	}
 
 	pl->mst_addr = port_id - 1;

-- 
2.52.0

Re: [PATCH v4 2/4] drm/bridge: imx8qxp-pixel-link: simplify freeing of the remote device_node

[Liu Ying]

On Wed, Jan 07, 2026 at 10:56:27AM +0100, Luca Ceresoli wrote:
> The main loop in imx8qxp_pixel_link_find_next_bridge() requires calling
> of_node_put() in multiple places, complicating code flow. Simplify it by
> using a cleanup action and making the 'remote' variable scope local to the
> loop.
> 
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
>  drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)

Reviewed-by: Liu Ying <victor.liu@nxp.com>

[PATCH v4 3/4] drm/bridge: imx8qxp-pixel-link: imx8qxp_pixel_link_find_next_bridge: return int, not ERR_PTR

[Luca Ceresoli]

In preparation for using bridge->next_bridge, we need to ensure that it
will never contain anything but NULL or a valid bridge pointer. Current
code stores an ERR_PTR when imx8qxp_pixel_link_find_next_bridge() errors
out. Instead of fixing that after the facts in the caller, change the
function to internally set pl->next_bridge and just return an int error
value.

No functional changes.

Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
index 0c5ed06eee1b..91e4f4d55469 100644
--- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
+++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
@@ -256,8 +256,7 @@ static int imx8qxp_pixel_link_disable_all_controls(struct imx8qxp_pixel_link *pl
 	return imx8qxp_pixel_link_disable_sync(pl);
 }
 
-static struct drm_bridge *
-imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
+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;
@@ -282,7 +281,7 @@ imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
 
 	if (!found_port) {
 		DRM_DEV_ERROR(pl->dev, "no available output port\n");
-		return ERR_PTR(-ENODEV);
+		return -ENODEV;
 	}
 
 	for (reg = 0; reg < PL_MAX_NEXT_BRIDGES; reg++) {
@@ -300,7 +299,7 @@ imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
 
 		struct drm_bridge *next_bridge = of_drm_find_bridge(remote);
 		if (!next_bridge)
-			return ERR_PTR(-EPROBE_DEFER);
+			return -EPROBE_DEFER;
 
 		/*
 		 * Select the next bridge with companion PXL2DPI if
@@ -311,8 +310,9 @@ imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
 	}
 
 	pl->mst_addr = port_id - 1;
+	pl->next_bridge = selected_bridge;
 
-	return selected_bridge;
+	return 0;
 }
 
 static int imx8qxp_pixel_link_bridge_probe(struct platform_device *pdev)
@@ -368,9 +368,9 @@ static int imx8qxp_pixel_link_bridge_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	pl->next_bridge = imx8qxp_pixel_link_find_next_bridge(pl);
-	if (IS_ERR(pl->next_bridge))
-		return PTR_ERR(pl->next_bridge);
+	ret = imx8qxp_pixel_link_find_next_bridge(pl);
+	if (ret)
+		return ret;
 
 	platform_set_drvdata(pdev, pl);
 

-- 
2.52.0

Re: [PATCH v4 3/4] drm/bridge: imx8qxp-pixel-link: imx8qxp_pixel_link_find_next_bridge: return int, not ERR_PTR

[Liu Ying]

On Wed, Jan 07, 2026 at 10:56:28AM +0100, Luca Ceresoli wrote:
> In preparation for using bridge->next_bridge, we need to ensure that it
> will never contain anything but NULL or a valid bridge pointer. Current
> code stores an ERR_PTR when imx8qxp_pixel_link_find_next_bridge() errors
> out. Instead of fixing that after the facts in the caller, change the
> function to internally set pl->next_bridge and just return an int error
> value.
> 
> No functional changes.
> 
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
>  drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

Acked-by: Liu Ying <victor.liu@nxp.com>

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

[Luca Ceresoli]

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 <mripard@kernel.org>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
index 91e4f4d55469..b3050310a7f0 100644
--- 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);
+		}
 	}
 
 	pl->mst_addr = port_id - 1;
-	pl->next_bridge = selected_bridge;
+	pl->bridge.next_bridge = drm_bridge_get(selected_bridge);
 
 	return 0;
 }

-- 
2.52.0

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

[Liu Ying]

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 <mripard@kernel.org>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
>  drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
> index 91e4f4d55469..b3050310a7f0 100644
> --- 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

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);
	}
}

...
pl->bridge.next_bridge = selected_bridge;
-8<-

Make sense?

> +		}
>  	}
>  
>  	pl->mst_addr = port_id - 1;
> -	pl->next_bridge = selected_bridge;
> +	pl->bridge.next_bridge = drm_bridge_get(selected_bridge);
>  
>  	return 0;
>  }
> 

-- 
Regards,
Liu Ying

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

[Luca Ceresoli]

Hello Liu,

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 <mripard@kernel.org>
>> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

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

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?

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

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

Luca

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

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

[Luca Ceresoli]

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

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

[Liu Ying]

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 <mripard@kernel.org>
>>> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> 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

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

[Luca Ceresoli]

On Tue Jan 27, 2026 at 4:54 AM CET, Liu Ying 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
>
> 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?

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

OK, will do.

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

I was about to reply "the NULL check in drm_bridge_put() is part of the API
contract as its documentation says", but then realized the documentation
does not say that. My bad, I was convinced I had documented that behaviour
to make it part of the contract so users can rely on it. I'm sending a
patch ASAP to document that.

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

Based on the "one pointer, one reference, one refcount" logic I explained
above, I find this version more complex to understand. I read it as:
selected_bridge and pl->bridge.next_bridge are two pointers sharing a
single reference, and we know that would not create bugs because by careful
code inspection we realize that the life of the two references is
overlapped without a hole inbetween. I'm not a fan of this.

Luca

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

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

[Liu Ying]

On Wed, Jan 28, 2026 at 04:58:18PM +0100, Luca Ceresoli wrote:
> On Tue Jan 27, 2026 at 4:54 AM CET, Liu Ying 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
>>
>> 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.
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<-

What do you think?

-- 
Regards,
Liu Ying

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

[Liu Ying]

On Thu, Jan 29, 2026 at 03:49:38PM +0800, Liu Ying wrote:
> 
> 
> On Wed, Jan 28, 2026 at 04:58:18PM +0100, Luca Ceresoli wrote:
>> On Tue Jan 27, 2026 at 4:54 AM CET, Liu Ying 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
>>>
>>> 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/

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

> 
> What do you think?
> 

-- 
Regards,
Liu Ying

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

[Luca Ceresoli]

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:
>>
>>
>> On Wed, Jan 28, 2026 at 04:58:18PM +0100, Luca Ceresoli wrote:
>>> On Tue Jan 27, 2026 at 4:54 AM CET, Liu Ying 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
>>>>
>>>> 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/
>
>> 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

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

[Liu Ying]

On Wed, Jan 07, 2026 at 10:56:29AM +0100, Luca Ceresoli wrote:

[...]

>  * the bridge returned of_drm_get_bridge() is stored in the local temporary

There is no of_drm_get_bridge() defined, so
s/of_drm_get_bridge/of_drm_find_and_get_bridge/ ?

-- 
Regards,
Liu Ying

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

[Luca Ceresoli]

On Mon Jan 26, 2026 at 9:14 AM CET, Liu Ying wrote:
>
>
> On Wed, Jan 07, 2026 at 10:56:29AM +0100, Luca Ceresoli wrote:
>
> [...]
>
>>  * the bridge returned of_drm_get_bridge() is stored in the local temporary
>
> There is no of_drm_get_bridge() defined, so
> s/of_drm_get_bridge/of_drm_find_and_get_bridge/ ?

Ah, well spotted, fixing that.

Luca

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

Re: [PATCH v4 0/4] drm/bridge: add of_drm_find_and_get_bridge() and a managed *next_bridge, deprecate of_drm_find_bridge()

[Liu Ying]

On 1/7/26 17:56, Luca Ceresoli wrote:
> This v4 contains the leftovers of the v3 series that deprecated
> of_drm_find_bridge(), added a replacement which handles bridge refcounting,
> and converted some of the direct users. All patches have been applied
> except the imx8qxp-pixel-link ones, which still need a review for the first
> patch.
> 
> @Liu, patch 1 is waiting for a review and it's non-trivial, it would be
> nice if you could have a look at that one at least.

Luca, I'v been too busy recently for tasks assigned by my employer to
give an in-time review, sorry about that.  I'll try to review this late
next week if not sooner.

> 
> This is part of the work to support hotplug of DRM bridges. The grand plan
> was discussed in [0].
> 
> Here's the work breakdown (➜ marks the current series):
> 
>  1. ➜ add refcounting to DRM bridges struct drm_bridge,
>       based on devm_drm_bridge_alloc()
>     A. ✔ add new alloc API and refcounting (v6.16)
>     B. ✔ convert all bridge drivers to new API (v6.17)
>     C. ✔ kunit tests (v6.17)
>     D. ✔ add get/put to drm_bridge_add/remove() + attach/detach()
>          and warn on old allocation pattern (v6.17)
>     E. ➜ add get/put on drm_bridge accessors
>        1. ✔ drm_bridge_chain_get_first_bridge(), add cleanup action (v6.18)
>        2. ✔ drm_bridge_get_prev_bridge() (v6.18)
>        3. ✔ drm_bridge_get_next_bridge() (v6.19)
>        4. ✔ drm_for_each_bridge_in_chain() (v6.19)
>        5. ✔ drm_bridge_connector_init (v6.19)
>        6. … protect encoder bridge chain with a mutex
>        7. ➜ of_drm_find_bridge()
>           a. ➜✔… add of_drm_get_bridge(), convert basic direct users
> 	         (v6.20?, one driver still pending)
> 	  b. convert other direct users
> 	  c. convert bridge-only drm_of_find_panel_or_bridge() users
>        8. drm_of_find_panel_or_bridge, *_of_get_bridge
>        9. ✔ enforce drm_bridge_add before drm_bridge_attach (v6.19)
>     F. ✔ debugfs improvements
>        1. ✔ add top-level 'bridges' file (v6.16)
>        2. ✔ show refcount and list lingering bridges (v6.19)
>  2. … handle gracefully atomic updates during bridge removal
>     A. ✔ Add drm_dev_enter/exit() to protect device resources (v6.20?)
>     B. … protect private_obj removal from list
>  3. … DSI host-device driver interaction
>  4. ✔ removing the need for the "always-disconnected" connector
>  5. finish the hotplug bridge work, moving code to the core and potentially
>     removing the hotplug-bridge itself (this needs to be clarified as
>     points 1-3 are developed)
> 
> [0] https://lore.kernel.org/lkml/20250206-hotplug-drm-bridge-v6-0-9d6f2c9c3058@bootlin.com/#t
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
> Changes in v4:
> - Added review trailers
> - Link to v3: https://lore.kernel.org/r/20251216-drm-bridge-alloc-getput-drm_of_find_bridge-v3-0-b5165fab8058@bootlin.com
> 
> Changes in v3:
> - Completely rewrote using the __drm_bridge_free() idea to prevent
>   use-after-free of the next_bridge for the common cases
> - Added needed cleanups to the imx8qxp-pixel-link and imx8qxp-pxl2dpi
>   drivers
> - Removed various patches converting simple cases, to reduce the number of
>   e-mails sent; will be moved to the follow-up series
> - Link to v2: https://lore.kernel.org/r/20251128-drm-bridge-alloc-getput-drm_of_find_bridge-v2-0-88f8a107eca2@bootlin.com
> 
> Changes in v2:
> - All patches: renamed drm_of_find_bridge() -> of_drm_get_bridge()
> - Various fixes and improvements to patches 1-6, see individual patches
>   changelog
> - Removed bouncing recipient: Edmund Dea <edmund.j.dea@intel.com>
> - Link to v1: https://lore.kernel.org/r/20251119-drm-bridge-alloc-getput-drm_of_find_bridge-v1-0-0db98a7fe474@bootlin.com
> 
> ---
> Luca Ceresoli (4):
>       drm/bridge: imx8qxp-pixel-link: simplify logic to find next bridge
>       drm/bridge: imx8qxp-pixel-link: simplify freeing of the remote device_node
>       drm/bridge: imx8qxp-pixel-link: imx8qxp_pixel_link_find_next_bridge: return int, not ERR_PTR
>       drm/bridge: imx8qxp-pixel-link: get/put the next bridge
> 
>  drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c | 53 ++++++++++++-------------
>  1 file changed, 25 insertions(+), 28 deletions(-)
> ---
> base-commit: f12ad2e5233a1a30b3bd6fe1e784b3544caa2383
> change-id: 20251117-drm-bridge-alloc-getput-drm_of_find_bridge-74903367448d
> 
> Best regards,

-- 
Regards,
Liu Ying

Re: [PATCH v4 0/4] drm/bridge: add of_drm_find_and_get_bridge() and a managed *next_bridge, deprecate of_drm_find_bridge()

[Luca Ceresoli]

Hello Liu,

On Fri Jan 9, 2026 at 10:53 AM CET, Liu Ying wrote:
>
>
> On 1/7/26 17:56, Luca Ceresoli wrote:
>> This v4 contains the leftovers of the v3 series that deprecated
>> of_drm_find_bridge(), added a replacement which handles bridge refcounting,
>> and converted some of the direct users. All patches have been applied
>> except the imx8qxp-pixel-link ones, which still need a review for the first
>> patch.
>>
>> @Liu, patch 1 is waiting for a review and it's non-trivial, it would be
>> nice if you could have a look at that one at least.
>
> Luca, I'v been too busy recently for tasks assigned by my employer to
> give an in-time review, sorry about that.  I'll try to review this late
> next week if not sooner.

That would be great, thanks! And I understand, aren't we all busy? :)

I have resent as v4 just to ensure it's clear that these patches are not
yet applied. My e-mail [0] might have suggested the opposite, due to my
total noobness in using the b4 auto-thankanator, sorry if that was the
case.

Having patch 1 reviewed is the most important thing here. Other patches are
already reviewed by Maxime, so your review would of course be very useful
but not strictly necessary for this series to go forward.

[0] https://lore.kernel.org/lkml/176708623356.127863.8623917477288453126.b4-ty@bootlin.com/

Best regards,
Luca

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

Re: (subset) [PATCH v4 0/4] drm/bridge: add of_drm_find_and_get_bridge() and a managed *next_bridge, deprecate of_drm_find_bridge()

[Luca Ceresoli]

On Wed, 07 Jan 2026 10:56:25 +0100, Luca Ceresoli wrote:
> This v4 contains the leftovers of the v3 series that deprecated
> of_drm_find_bridge(), added a replacement which handles bridge refcounting,
> and converted some of the direct users. All patches have been applied
> except the imx8qxp-pixel-link ones, which still need a review for the first
> patch.
> 
> @Liu, patch 1 is waiting for a review and it's non-trivial, it would be
> nice if you could have a look at that one at least.
> 
> [...]

Applied, thanks!

[1/4] drm/bridge: imx8qxp-pixel-link: simplify logic to find next bridge
      commit: 96476ab8690290aa27084b12a481e48f3af7afb2
[2/4] drm/bridge: imx8qxp-pixel-link: simplify freeing of the remote device_node
      commit: 42bb487369e56f8f07c82ac11fe771ba2b70cd68
[3/4] drm/bridge: imx8qxp-pixel-link: imx8qxp_pixel_link_find_next_bridge: return int, not ERR_PTR
      commit: 4eda1d5fe99db2a71da0156387b7b69e8e0dc32f

Best regards,
-- 
Luca Ceresoli <luca.ceresoli@bootlin.com>