[PATCH v3] drm/bridge: imx8qxp-pxl2dpi: avoid ERR_PTR with device_node cleanup

[PATCH v3] drm/bridge: imx8qxp-pxl2dpi: avoid ERR_PTR with device_node cleanup

[Guangshuo Li]

imx8qxp_pxl2dpi_get_available_ep_from_port() returns ERR_PTR()
on errors. imx8qxp_pxl2dpi_find_next_bridge() stores its return
value in a __free(device_node) variable before checking IS_ERR().
When the function returns on the error path, the cleanup action calls
of_node_put() on the ERR_PTR() value.

Do not let a device_node cleanup variable hold error pointers. Return
the error code from imx8qxp_pxl2dpi_get_available_ep_from_port()
directly and pass the endpoint node through an output argument. This
keeps the cleanup action operating only on NULL or a valid device_node,
while preserving the existing error codes.

This issue was found by a custom static analysis tool.

Fixes: ceea3f7806a10 ("drm/bridge: imx8qxp-pxl2dpi: simplify put of device_node pointers")
Cc: stable@vger.kernel.org
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
v3:
  - Do not change DEFINE_FREE(device_node, ...).
  - Fix the driver pattern by making
    imx8qxp_pxl2dpi_get_available_ep_from_port() return an int and
    pass the endpoint via an output argument.
  - Update both callers so __free(device_node) never holds ERR_PTR().

v2:
  - Fix DEFINE_FREE(device_node, ...) directly.

 drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c | 55 +++++++++-----------
 1 file changed, 26 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c b/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
index 441fd32dc91c..a82f10218707 100644
--- a/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
+++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
@@ -222,52 +222,52 @@ static const struct drm_bridge_funcs imx8qxp_pxl2dpi_bridge_funcs = {
 			imx8qxp_pxl2dpi_bridge_atomic_get_output_bus_fmts,
 };
 
-static struct device_node *
+static int
 imx8qxp_pxl2dpi_get_available_ep_from_port(struct imx8qxp_pxl2dpi *p2d,
-					   u32 port_id)
+					   u32 port_id,
+					   struct device_node **ep)
 {
-	struct device_node *port, *ep;
+	struct device_node *port __free(device_node) =
+		of_graph_get_port_by_id(p2d->dev->of_node, port_id);
 	int ep_cnt;
 
-	port = of_graph_get_port_by_id(p2d->dev->of_node, port_id);
+	*ep = NULL;
 	if (!port) {
 		DRM_DEV_ERROR(p2d->dev, "failed to get port@%u\n", port_id);
-		return ERR_PTR(-ENODEV);
+		return -ENODEV;
 	}
 
 	ep_cnt = of_get_available_child_count(port);
 	if (ep_cnt == 0) {
 		DRM_DEV_ERROR(p2d->dev, "no available endpoints of port@%u\n",
 			      port_id);
-		ep = ERR_PTR(-ENODEV);
-		goto out;
+		return -ENODEV;
 	} else if (ep_cnt > 1) {
 		DRM_DEV_ERROR(p2d->dev,
 			      "invalid available endpoints of port@%u\n",
 			      port_id);
-		ep = ERR_PTR(-EINVAL);
-		goto out;
+		return -EINVAL;
 	}
 
-	ep = of_get_next_available_child(port, NULL);
-	if (!ep) {
+	*ep = of_get_next_available_child(port, NULL);
+	if (!*ep) {
 		DRM_DEV_ERROR(p2d->dev,
 			      "failed to get available endpoint of port@%u\n",
 			      port_id);
-		ep = ERR_PTR(-ENODEV);
-		goto out;
+		return -ENODEV;
 	}
-out:
-	of_node_put(port);
-	return ep;
+
+	return 0;
 }
 
 static int imx8qxp_pxl2dpi_find_next_bridge(struct imx8qxp_pxl2dpi *p2d)
 {
-	struct device_node *ep __free(device_node) =
-		imx8qxp_pxl2dpi_get_available_ep_from_port(p2d, 1);
-	if (IS_ERR(ep))
-		return PTR_ERR(ep);
+	struct device_node *ep __free(device_node) = NULL;
+	int ret;
+
+	ret = imx8qxp_pxl2dpi_get_available_ep_from_port(p2d, 1, &ep);
+	if (ret)
+		return ret;
 
 	struct device_node *remote __free(device_node) = of_graph_get_remote_port_parent(ep);
 	if (!remote || !of_device_is_available(remote)) {
@@ -287,26 +287,23 @@ static int imx8qxp_pxl2dpi_find_next_bridge(struct imx8qxp_pxl2dpi *p2d)
 
 static int imx8qxp_pxl2dpi_set_pixel_link_sel(struct imx8qxp_pxl2dpi *p2d)
 {
-	struct device_node *ep;
+	struct device_node *ep __free(device_node) = NULL;
 	struct of_endpoint endpoint;
 	int ret;
 
-	ep = imx8qxp_pxl2dpi_get_available_ep_from_port(p2d, 0);
-	if (IS_ERR(ep))
-		return PTR_ERR(ep);
+	ret = imx8qxp_pxl2dpi_get_available_ep_from_port(p2d, 0, &ep);
+	if (ret)
+		return ret;
 
 	ret = of_graph_parse_endpoint(ep, &endpoint);
 	if (ret) {
 		DRM_DEV_ERROR(p2d->dev,
 			      "failed to parse endpoint of port@0: %d\n", ret);
-		goto out;
+		return ret;
 	}
 
 	p2d->pl_sel = endpoint.id;
-out:
-	of_node_put(ep);
-
-	return ret;
+	return 0;
 }
 
 static int imx8qxp_pxl2dpi_parse_dt_companion(struct imx8qxp_pxl2dpi *p2d)
-- 
2.43.0

Re: [PATCH v3] drm/bridge: imx8qxp-pxl2dpi: avoid ERR_PTR with device_node cleanup

[Frank Li]

On Sat, May 02, 2026 at 07:55:28PM +0800, Guangshuo Li wrote:
> imx8qxp_pxl2dpi_get_available_ep_from_port() returns ERR_PTR()
> on errors. imx8qxp_pxl2dpi_find_next_bridge() stores its return
> value in a __free(device_node) variable before checking IS_ERR().
> When the function returns on the error path, the cleanup action calls
> of_node_put() on the ERR_PTR() value.
>
> Do not let a device_node cleanup variable hold error pointers. Return
> the error code from imx8qxp_pxl2dpi_get_available_ep_from_port()
> directly and pass the endpoint node through an output argument. This
> keeps the cleanup action operating only on NULL or a valid device_node,
> while preserving the existing error codes.
>
> This issue was found by a custom static analysis tool.

Nit: needn't mention this.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
> Fixes: ceea3f7806a10 ("drm/bridge: imx8qxp-pxl2dpi: simplify put of device_node pointers")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> ---
> v3:
>   - Do not change DEFINE_FREE(device_node, ...).
>   - Fix the driver pattern by making
>     imx8qxp_pxl2dpi_get_available_ep_from_port() return an int and
>     pass the endpoint via an output argument.
>   - Update both callers so __free(device_node) never holds ERR_PTR().
>
> v2:
>   - Fix DEFINE_FREE(device_node, ...) directly.
>
>  drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c | 55 +++++++++-----------
>  1 file changed, 26 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c b/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
> index 441fd32dc91c..a82f10218707 100644
> --- a/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
> +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
> @@ -222,52 +222,52 @@ static const struct drm_bridge_funcs imx8qxp_pxl2dpi_bridge_funcs = {
>  			imx8qxp_pxl2dpi_bridge_atomic_get_output_bus_fmts,
>  };
>
> -static struct device_node *
> +static int
>  imx8qxp_pxl2dpi_get_available_ep_from_port(struct imx8qxp_pxl2dpi *p2d,
> -					   u32 port_id)
> +					   u32 port_id,
> +					   struct device_node **ep)
>  {
> -	struct device_node *port, *ep;
> +	struct device_node *port __free(device_node) =
> +		of_graph_get_port_by_id(p2d->dev->of_node, port_id);
>  	int ep_cnt;
>
> -	port = of_graph_get_port_by_id(p2d->dev->of_node, port_id);
> +	*ep = NULL;
>  	if (!port) {
>  		DRM_DEV_ERROR(p2d->dev, "failed to get port@%u\n", port_id);
> -		return ERR_PTR(-ENODEV);
> +		return -ENODEV;
>  	}
>
>  	ep_cnt = of_get_available_child_count(port);
>  	if (ep_cnt == 0) {
>  		DRM_DEV_ERROR(p2d->dev, "no available endpoints of port@%u\n",
>  			      port_id);
> -		ep = ERR_PTR(-ENODEV);
> -		goto out;
> +		return -ENODEV;
>  	} else if (ep_cnt > 1) {
>  		DRM_DEV_ERROR(p2d->dev,
>  			      "invalid available endpoints of port@%u\n",
>  			      port_id);
> -		ep = ERR_PTR(-EINVAL);
> -		goto out;
> +		return -EINVAL;
>  	}
>
> -	ep = of_get_next_available_child(port, NULL);
> -	if (!ep) {
> +	*ep = of_get_next_available_child(port, NULL);
> +	if (!*ep) {
>  		DRM_DEV_ERROR(p2d->dev,
>  			      "failed to get available endpoint of port@%u\n",
>  			      port_id);
> -		ep = ERR_PTR(-ENODEV);
> -		goto out;
> +		return -ENODEV;
>  	}
> -out:
> -	of_node_put(port);
> -	return ep;
> +
> +	return 0;
>  }
>
>  static int imx8qxp_pxl2dpi_find_next_bridge(struct imx8qxp_pxl2dpi *p2d)
>  {
> -	struct device_node *ep __free(device_node) =
> -		imx8qxp_pxl2dpi_get_available_ep_from_port(p2d, 1);
> -	if (IS_ERR(ep))
> -		return PTR_ERR(ep);
> +	struct device_node *ep __free(device_node) = NULL;
> +	int ret;
> +
> +	ret = imx8qxp_pxl2dpi_get_available_ep_from_port(p2d, 1, &ep);
> +	if (ret)
> +		return ret;
>
>  	struct device_node *remote __free(device_node) = of_graph_get_remote_port_parent(ep);
>  	if (!remote || !of_device_is_available(remote)) {
> @@ -287,26 +287,23 @@ static int imx8qxp_pxl2dpi_find_next_bridge(struct imx8qxp_pxl2dpi *p2d)
>
>  static int imx8qxp_pxl2dpi_set_pixel_link_sel(struct imx8qxp_pxl2dpi *p2d)
>  {
> -	struct device_node *ep;
> +	struct device_node *ep __free(device_node) = NULL;
>  	struct of_endpoint endpoint;
>  	int ret;
>
> -	ep = imx8qxp_pxl2dpi_get_available_ep_from_port(p2d, 0);
> -	if (IS_ERR(ep))
> -		return PTR_ERR(ep);
> +	ret = imx8qxp_pxl2dpi_get_available_ep_from_port(p2d, 0, &ep);
> +	if (ret)
> +		return ret;
>
>  	ret = of_graph_parse_endpoint(ep, &endpoint);
>  	if (ret) {
>  		DRM_DEV_ERROR(p2d->dev,
>  			      "failed to parse endpoint of port@0: %d\n", ret);
> -		goto out;
> +		return ret;
>  	}
>
>  	p2d->pl_sel = endpoint.id;
> -out:
> -	of_node_put(ep);
> -
> -	return ret;
> +	return 0;
>  }
>
>  static int imx8qxp_pxl2dpi_parse_dt_companion(struct imx8qxp_pxl2dpi *p2d)
> --
> 2.43.0
>

Claude review: drm/bridge: imx8qxp-pxl2dpi: avoid ERR_PTR with device_node cleanup

[Claude Code Review Bot]

Overall Series Review

Subject: drm/bridge: imx8qxp-pxl2dpi: avoid ERR_PTR with device_node cleanup
Author: Guangshuo Li <lgs201920130244@gmail.com>
Patches: 2
Reviewed: 2026-05-05T08:50:48.648719

---

This is a single-patch bugfix series (v3) addressing a real bug: `imx8qxp_pxl2dpi_get_available_ep_from_port()` returns `ERR_PTR()` on failure, but the caller `imx8qxp_pxl2dpi_find_next_bridge()` stores that value in a `__free(device_node)` variable. The `DEFINE_FREE(device_node, ...)` cleanup macro only checks `if (_T)` before calling `of_node_put()`, so an `ERR_PTR()` (which is non-NULL) will cause `of_node_put()` on a bogus pointer — a potential crash or memory corruption.

The fix is correct and well-structured: the helper function is refactored to return an `int` error code and pass the endpoint node through an output parameter, ensuring that `__free(device_node)` variables only ever hold `NULL` or a valid `device_node` pointer. Both callers are updated consistently.

**Verdict: The patch looks good and should be accepted.** There is one minor Fixes tag formatting nit (13-char SHA vs. the standard 12-char), but the substance of the fix is correct and clean.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/bridge: imx8qxp-pxl2dpi: avoid ERR_PTR with device_node cleanup

[Claude Code Review Bot]

Patch Review

**Bug analysis:** Correct. The existing code at line 267-268 of the current tree:
```c
struct device_node *ep __free(device_node) =
    imx8qxp_pxl2dpi_get_available_ep_from_port(p2d, 1);
```
stores an `ERR_PTR()` in a `__free(device_node)` variable. Since `DEFINE_FREE(device_node, ...)` is defined as:
```c
DEFINE_FREE(device_node, struct device_node *, if (_T) of_node_put(_T))
```
an `ERR_PTR()` passes the `if (_T)` check (it's non-NULL) and `of_node_put()` is called on a bogus error-encoding pointer.

**Fix approach (helper refactoring):** Clean and idiomatic. The function signature change from:
```c
static struct device_node *
imx8qxp_pxl2dpi_get_available_ep_from_port(struct imx8qxp_pxl2dpi *p2d,
                                           u32 port_id)
```
to:
```c
static int
imx8qxp_pxl2dpi_get_available_ep_from_port(struct imx8qxp_pxl2dpi *p2d,
                                           u32 port_id,
                                           struct device_node **ep)
```
is a reasonable pattern. The output parameter `*ep` is initialized to `NULL` at the top, and only set to a valid node on success.

**Helper function internals:** The `port` variable is now also managed with `__free(device_node)`:
```c
struct device_node *port __free(device_node) =
    of_graph_get_port_by_id(p2d->dev->of_node, port_id);
```
This is correct — `of_graph_get_port_by_id()` returns `NULL` on failure (not `ERR_PTR()`), so the cleanup is safe and eliminates the old `goto out` / `of_node_put(port)` pattern.

**Caller `imx8qxp_pxl2dpi_find_next_bridge()`:** Correctly updated:
```c
struct device_node *ep __free(device_node) = NULL;
int ret;

ret = imx8qxp_pxl2dpi_get_available_ep_from_port(p2d, 1, &ep);
if (ret)
    return ret;
```
The `ep` is initialized to `NULL`, so if the function fails (returning a negative error), `ep` remains `NULL` and the `__free(device_node)` cleanup is a safe no-op.

**Caller `imx8qxp_pxl2dpi_set_pixel_link_sel()`:** Also correctly updated. Additionally, this caller now uses `__free(device_node)` for `ep` where it previously used manual `of_node_put()`. The `goto out` / manual cleanup pattern is removed, and the function now returns early on errors:
```c
ret = of_graph_parse_endpoint(ep, &endpoint);
if (ret) {
    DRM_DEV_ERROR(p2d->dev,
                  "failed to parse endpoint of port@0: %d\n", ret);
    return ret;
}

p2d->pl_sel = endpoint.id;
return 0;
```
This is a nice improvement — it makes the second caller consistent with the first and simplifies the control flow.

**Minor nit — Fixes tag format:**
```
Fixes: ceea3f7806a10 ("drm/bridge: imx8qxp-pxl2dpi: ...")
```
The abbreviated SHA `ceea3f7806a10` is 13 hex characters. The kernel convention (`checkpatch.pl`) expects exactly 12. Should be `ceea3f7806a1`. This is cosmetic and wouldn't block acceptance, but is worth mentioning for a clean commit log.

**Overall:** The patch correctly identifies and fixes a real bug, the approach is clean, both callers are properly updated, and the commit message is clear and well-written. Reviewed-by-worthy with the optional Fixes tag nit.

---
Generated by Claude Code Patch Reviewer