public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/bridge: imx8qxp-pxl2dpi: avoid ERR_PTR with device_node cleanup
Date: Thu, 07 May 2026 13:23:41 +1000	[thread overview]
Message-ID: <review-patch1-20260506142434.643523-1-lgs201920130244@gmail.com> (raw)
In-Reply-To: <20260506142434.643523-1-lgs201920130244@gmail.com>

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

  parent reply	other threads:[~2026-05-07  3:23 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-05-06 14:24 [PATCH v6] drm/bridge: imx8qxp-pxl2dpi: avoid ERR_PTR with device_node cleanup Guangshuo Li
2026-05-07  2:20 ` Liu Ying
2026-05-07  3:23 ` Claude review: " Claude Code Review Bot
2026-05-07  3:23 ` Claude Code Review Bot [this message]
  -- strict thread matches above, loose matches on Subject: below --
2026-05-06  9:23 [PATCH v5] " Guangshuo Li
2026-05-07  3:48 ` Claude review: " Claude Code Review Bot
2026-05-07  3:48 ` Claude Code Review Bot
2026-05-02 11:55 [PATCH v3] " Guangshuo Li
2026-05-04 22:50 ` Claude review: " Claude Code Review Bot
2026-05-04 22:50 ` Claude Code Review Bot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=review-patch1-20260506142434.643523-1-lgs201920130244@gmail.com \
    --to=claude-review@example.com \
    --cc=dri-devel-reviews@example.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox