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 v5] 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 store the endpoint node in a cleanup variable before checking
whether it is an error pointer. Use a regular device_node pointer for
the endpoint node, check it with IS_ERR() first, and release it
explicitly with of_node_put() after getting the remote port parent.

This keeps the fix minimal and avoids changing
imx8qxp_pxl2dpi_get_available_ep_from_port().

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>
---
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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c b/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
index 441fd32dc91c..f64f57a33c62 100644
--- a/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
+++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
@@ -264,12 +264,13 @@ imx8qxp_pxl2dpi_get_available_ep_from_port(struct imx8qxp_pxl2dpi *p2d,
 
 static int imx8qxp_pxl2dpi_find_next_bridge(struct imx8qxp_pxl2dpi *p2d)
 {
-	struct device_node *ep __free(device_node) =
+	struct device_node *ep =
 		imx8qxp_pxl2dpi_get_available_ep_from_port(p2d, 1);
 	if (IS_ERR(ep))
 		return PTR_ERR(ep);
 
 	struct device_node *remote __free(device_node) = of_graph_get_remote_port_parent(ep);
+	of_node_put(ep);
 	if (!remote || !of_device_is_available(remote)) {
 		DRM_DEV_ERROR(p2d->dev, "no available remote\n");
 		return -ENODEV;
-- 
2.43.0

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

[Liu Ying]

On Wed, May 06, 2026 at 05:23:24PM +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 store the endpoint node in a cleanup variable before checking
> whether it is an error pointer. Use a regular device_node pointer for
> the endpoint node, check it with IS_ERR() first, and release it
> explicitly with of_node_put() after getting the remote port parent.
> 
> This keeps the fix minimal and avoids changing
> imx8qxp_pxl2dpi_get_available_ep_from_port().
> 
> 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>
> ---
> v5:
>   - Make the fix minimal for stable by avoiding __free(device_node)
>     for the endpoint node in imx8qxp_pxl2dpi_find_next_bridge().

By "minimal" in v4 comment, I meant not to use __free(device_node)
in imx8qxp_pxl2dpi_get_available_ep_from_port() and
imx8qxp_pxl2dpi_set_pixel_link_sel() - please keep using
__free(device_node) in imx8qxp_pxl2dpi_find_next_bridge().


>   - Keep imx8qxp_pxl2dpi_get_available_ep_from_port() unchanged.

No, please fix imx8qxp_pxl2dpi_get_available_ep_from_port() to make it
return int.

>   - Do not change imx8qxp_pxl2dpi_set_pixel_link_sel().

No, you need to change it.

-- 
Regards,
Liu Ying

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

[Guangshuo Li]

Hi Liu,

Thanks for the clarification.

On Wed, 6 May 2026 at 17:49, Liu Ying <victor.liu@nxp.com> wrote:
>

>
> By "minimal" in v4 comment, I meant not to use __free(device_node)
> in imx8qxp_pxl2dpi_get_available_ep_from_port() and
> imx8qxp_pxl2dpi_set_pixel_link_sel() - please keep using
> __free(device_node) in imx8qxp_pxl2dpi_find_next_bridge().
>
>
> >   - Keep imx8qxp_pxl2dpi_get_available_ep_from_port() unchanged.
>
> No, please fix imx8qxp_pxl2dpi_get_available_ep_from_port() to make it
> return int.
>
> >   - Do not change imx8qxp_pxl2dpi_set_pixel_link_sel().
>
> No, you need to change it.
>
> --
> Regards,
> Liu Ying

I misunderstood your previous comment. I will update the patch to keep
using __free(device_node) in imx8qxp_pxl2dpi_find_next_bridge(),
change imx8qxp_pxl2dpi_get_available_ep_from_port() to return int and
pass the endpoint through an output argument, but avoid adding cleanup
action usage in imx8qxp_pxl2dpi_get_available_ep_from_port() and
imx8qxp_pxl2dpi_set_pixel_link_sel().

I will also drop the unnecessary local NULL initialization for ep,
since the helper initializes the output argument to NULL.

I will send a v6.

Best regards,
Guangshuo

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

[Guangshuo Li]

Hi Liu,

On Wed, 6 May 2026 at 21:58, Guangshuo Li <lgs201920130244@gmail.com> wrote:
>
> I will also drop the unnecessary local NULL initialization for ep,
> since the helper initializes the output argument to NULL.
>
> I will send a v6.
>
> Best regards,
> Guangshuo

One correction to my previous reply: I will keep the local ep variable
in imx8qxp_pxl2dpi_find_next_bridge() initialized to NULL, since
checkpatch requires pointers with the __free attribute to be
initialized.

Best regards,
Guangshuo

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