From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm: rcar-du: Don't leak device_link to CMM
Date: Wed, 25 Mar 2026 07:42:40 +1000
Message-ID: <review-patch4-20260323164526.2292491-5-laurent.pinchart+renesas@ideasonboard.com>
In-Reply-To: <20260323164526.2292491-5-laurent.pinchart+renesas@ideasonboard.com>
References: <20260323164526.2292491-1-laurent.pinchart+renesas@ideasonboard.com> <20260323164526.2292491-5-laurent.pinchart+renesas@ideasonboard.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Status: Bug =E2=80=94 NULL pointer dereference**

The core idea is sound: introduce `struct rcar_du_cmm` to group the device =
and link, and delete links at cleanup. However, this patch introduces a NUL=
L pointer dereference.

In `rcar_du_crtc_create()`, `rcrtc->cmm` is only assigned when the CMM devi=
ce exists:

```c
if (rcdu->cmms[swindex].dev) {
    rcrtc->cmm =3D &rcdu->cmms[swindex];
    ...
}
```

If CMM is not available for a CRTC, `rcrtc->cmm` remains **NULL**. But the =
patch changes checks from `if (!rcrtc->cmm)` to `if (!rcrtc->cmm->dev)` in =
three places (`rcar_du_cmm_setup`, `rcar_du_crtc_stop`, `rcar_du_crtc_atomi=
c_enable`), which will dereference the NULL pointer:

```c
// rcar_du_crtc.c, rcar_du_cmm_setup():
if (!rcrtc->cmm->dev)    // BUG: rcrtc->cmm is NULL when no CMM
    return;

// rcar_du_crtc.c, rcar_du_crtc_stop():
if (rcrtc->cmm->dev)     // BUG: same issue
    rcar_cmm_disable(rcrtc->cmm->dev);

// rcar_du_crtc.c, rcar_du_crtc_atomic_enable():
if (rcrtc->cmm->dev)     // BUG: same issue
    rcar_cmm_enable(rcrtc->cmm->dev);
```

**Fix options:**
1. Always set `rcrtc->cmm =3D &rcdu->cmms[swindex]` (even when `.dev` is NU=
LL), so the pointer is always valid and only `.dev` needs checking. This is=
 the simplest fix.
2. Add a two-level NULL check: `if (rcrtc->cmm && rcrtc->cmm->dev)` =E2=80=
=94 but this is ugly and defeats the purpose of the refactoring.

Option 1 would look like:

```c
/* In rcar_du_crtc_create(): */
rcrtc->cmm =3D &rcdu->cmms[swindex];
if (rcrtc->cmm->dev) {
    rgrp->cmms_mask |=3D BIT(hwindex % 2);
    drm_mode_crtc_set_gamma_size(crtc, CM2_LUT_SIZE);
    drm_crtc_enable_color_mgmt(crtc, 0, false, CM2_LUT_SIZE);
}
```

---
Generated by Claude Code Patch Reviewer