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

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

[PATCH v6] 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. Change
imx8qxp_pxl2dpi_get_available_ep_from_port() to return an int and pass
the endpoint node through an output argument. Initialize the output
argument to NULL so callers using cleanup variables hold either NULL or
a valid device_node pointer on error paths.

Keep explicit of_node_put() usage in the helper and in
imx8qxp_pxl2dpi_set_pixel_link_sel() so the fix avoids adding more
cleanup action usage there.

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>
---
v6:
  - Change imx8qxp_pxl2dpi_get_available_ep_from_port() to return int
    and pass the endpoint through an output argument.
  - Keep using __free(device_node) in imx8qxp_pxl2dpi_find_next_bridge().
  - Keep ep initialized to NULL in imx8qxp_pxl2dpi_find_next_bridge()
    to satisfy the __free pointer initialization requirement.
  - Do not add cleanup action usage in
    imx8qxp_pxl2dpi_get_available_ep_from_port() or
    imx8qxp_pxl2dpi_set_pixel_link_sel().

v5:
  - Make the fix minimal for stable by avoiding __free(device_node)
    for the endpoint node in imx8qxp_pxl2dpi_find_next_bridge().
  - Keep imx8qxp_pxl2dpi_get_available_ep_from_port() unchanged.
  - Do not change imx8qxp_pxl2dpi_set_pixel_link_sel().
  - Drop Frank's Reviewed-by tag due to the implementation change.

v4:
  - Drop the sentence mentioning the custom static analysis tool.
  - Add Frank's Reviewed-by tag.
  - No functional code changes.

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 | 40 +++++++++++---------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c b/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
index 441fd32dc91c..d64e328bf542 100644
--- a/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
+++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
@@ -222,52 +222,58 @@ 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;
+	int ret = 0;
 	int ep_cnt;
 
+	*ep = NULL;
+
 	port = of_graph_get_port_by_id(p2d->dev->of_node, port_id);
 	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);
+		ret = -ENODEV;
 		goto out;
 	} else if (ep_cnt > 1) {
 		DRM_DEV_ERROR(p2d->dev,
 			      "invalid available endpoints of port@%u\n",
 			      port_id);
-		ep = ERR_PTR(-EINVAL);
+		ret = -EINVAL;
 		goto out;
 	}
 
-	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);
+		ret = -ENODEV;
 		goto out;
 	}
 out:
 	of_node_put(port);
-	return ep;
+	return ret;
 }
 
 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)) {
@@ -291,9 +297,9 @@ static int imx8qxp_pxl2dpi_set_pixel_link_sel(struct imx8qxp_pxl2dpi *p2d)
 	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) {
-- 
2.43.0

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

[Liu Ying]

On Wed, May 06, 2026 at 10:24:34PM +0800, Guangshuo Li wrote:

[...]

> Initialize the output
> argument to NULL so callers using cleanup variables hold either NULL or
> a valid device_node pointer on error paths.

I'd rephrase:
Initialize the output argument to NULL so callers hold either NULL on
error paths or a valid device_node pointer on successful path.

> 
> Keep explicit of_node_put() usage in the helper and in
> imx8qxp_pxl2dpi_set_pixel_link_sel() so the fix avoids adding more
> cleanup action usage there.

This sentence is not necessary, so I prefer to dropping it.

With these fixed:
Reviewed-by: Liu Ying <victor.liu@nxp.com>
Thanks!

-- 
Regards,
Liu Ying

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-07T13:23:41.500001

---

This is a single-patch bug fix for a real and valid problem. The `__free(device_node)` cleanup attribute calls `of_node_put()` on any non-NULL pointer, but `imx8qxp_pxl2dpi_get_available_ep_from_port()` returns `ERR_PTR()` values on error. When `imx8qxp_pxl2dpi_find_next_bridge()` stores such an `ERR_PTR()` in a `__free(device_node)` variable and then returns, the cleanup will call `of_node_put()` on the error pointer — corrupting memory or causing a crash.

The fix is correct and well-designed: change the helper to return `int` and pass the endpoint node via an output parameter initialized to `NULL`. This ensures the cleanup variable only ever holds `NULL` or a valid `device_node` pointer.

The patch is at v6 and has clearly been through careful review iteration. The approach is sound and the implementation is clean.

**Verdict: This patch looks good and should be accepted.**

---
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. Looking at the kernel tree, `DEFINE_FREE(device_node, ...)` at `include/linux/of.h:138` is:
```c
DEFINE_FREE(device_node, struct device_node *, if (_T) of_node_put(_T))
```
The `if (_T)` guard makes it safe for `NULL` but not for `ERR_PTR()` values (which are non-NULL). The original code in `imx8qxp_pxl2dpi_find_next_bridge()` stores the return value of the helper directly into a `__free(device_node)` variable, so an `ERR_PTR(-ENODEV)` would trigger `of_node_put()` on a bogus address.

**Function signature change:** Clean and appropriate.
```c
-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)
```
The output parameter pattern is idiomatic for kernel code that needs to distinguish between "no result" and "error."

**Output initialization:** Correct and important:
```c
+       *ep = NULL;
```
This ensures that on any error path the caller's variable stays `NULL`, which is safe with `__free(device_node)`.

**Error path changes in the helper:** All three `ERR_PTR()` returns are correctly converted:
```c
-               ep = ERR_PTR(-ENODEV);
+               ret = -ENODEV;
                goto out;
```
The `goto out` pattern is preserved, and `of_node_put(port)` at the `out:` label still runs correctly.

**Success path:** Correctly writes through the output pointer:
```c
-       ep = of_get_next_available_child(port, NULL);
-       if (!ep) {
+       *ep = of_get_next_available_child(port, NULL);
+       if (!*ep) {
```

**Caller update in `imx8qxp_pxl2dpi_find_next_bridge()`:**
```c
-       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;
```
The `__free(device_node)` variable is initialized to `NULL` and only gets a valid pointer on success. On error, `ep` remains `NULL` and cleanup is safe. Good.

**Caller update in `imx8qxp_pxl2dpi_set_pixel_link_sel()`:**
```c
-       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;
```
This caller does *not* use `__free(device_node)` for `ep` — it has an explicit `of_node_put(ep)` at its `out:` label (visible in the kernel tree at lines 307–308). The patch correctly leaves that manual cleanup in place. On the error path from the helper, `ep` is `NULL` and the explicit `of_node_put(NULL)` is a no-op, which is correct.

**Fixes tag and Cc: stable:** Both appropriate. The fix references `ceea3f7806a10` which introduced the `__free(device_node)` usage that created this bug.

**No issues found.** The patch is minimal, correct, well-documented in the commit message, and suitable for stable backport.

---
Generated by Claude Code Patch Reviewer

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: 4
Reviewed: 2026-05-07T13:48:12.371028

---

This is a correct and well-targeted single-patch fix for a real bug. The `DEFINE_FREE(device_node, ...)` cleanup macro (defined in `include/linux/of.h:138`) checks `if (_T)` before calling `of_node_put(_T)`, but does **not** filter out `IS_ERR()` values. Since ERR_PTR values are non-NULL, when `imx8qxp_pxl2dpi_get_available_ep_from_port()` returns an error, the `__free(device_node)` cleanup would call `of_node_put()` on a bogus kernel pointer — a use-after-free / invalid memory access.

The fix is minimal and correct: remove the `__free` annotation from `ep`, check `IS_ERR(ep)` before any cleanup can fire, then manually `of_node_put(ep)` once it's no longer needed. The approach is appropriate for a stable-targeted fix.

**Verdict: Looks good to me. Recommend applying.**

---
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 `DEFINE_FREE` for `device_node` at `include/linux/of.h:138`:
```c
DEFINE_FREE(device_node, struct device_node *, if (_T) of_node_put(_T))
```
only guards against NULL, not ERR_PTR. The function `imx8qxp_pxl2dpi_get_available_ep_from_port()` returns ERR_PTR on multiple error paths. So the original code:
```c
struct device_node *ep __free(device_node) =
    imx8qxp_pxl2dpi_get_available_ep_from_port(p2d, 1);
if (IS_ERR(ep))
    return PTR_ERR(ep);
```
would trigger `of_node_put()` on an ERR_PTR value when the function returns early on the error path, since `__free` runs at scope exit.

**Fix correctness:** The patch correctly:
1. Removes `__free(device_node)` from `ep`, making it a plain pointer.
2. Keeps the existing `IS_ERR(ep)` check — on this path, no cleanup is needed (ERR_PTR is not a real node).
3. Adds an explicit `of_node_put(ep)` after `of_graph_get_remote_port_parent(ep)` consumes the node.
4. Leaves `remote` with `__free(device_node)` — this is safe because `of_graph_get_remote_port_parent()` returns NULL on failure, never ERR_PTR.

```c
struct device_node *remote __free(device_node) = of_graph_get_remote_port_parent(ep);
of_node_put(ep);
```

The placement of `of_node_put(ep)` is correct: it's called after `ep` is consumed by `of_graph_get_remote_port_parent()` and before the function could return without releasing it.

**Minor observations:**
- The commit message is well-written and clearly explains the bug, the fix strategy, and why the approach was chosen (minimal for stable backport).
- The other caller, `imx8qxp_pxl2dpi_set_pixel_link_sel()`, already uses manual `of_node_put()` and is not affected by this bug.
- The Fixes tag correctly points to `ceea3f7806a10` which introduced the `__free(device_node)` annotation.
- The v5 changelog shows good iteration, with the author converging on the most minimal fix after exploring more invasive approaches in earlier versions.

**No issues found.** The patch is correct, minimal, and appropriate for stable.

---
Generated by Claude Code Patch Reviewer