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: Tue, 05 May 2026 08:50:48 +1000	[thread overview]
Message-ID: <review-patch1-20260502115528.530401-1-lgs201920130244@gmail.com> (raw)
In-Reply-To: <20260502115528.530401-1-lgs201920130244@gmail.com>

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

      parent reply	other threads:[~2026-05-04 22:50 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-05-02 11:55 [PATCH v3] drm/bridge: imx8qxp-pxl2dpi: avoid ERR_PTR with device_node cleanup Guangshuo Li
2026-05-04 19:09 ` Frank Li
2026-05-04 22:50 ` Claude review: " Claude Code Review Bot
2026-05-04 22:50 ` Claude Code Review Bot [this message]

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