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/client: Do not destroy NULL modes
Date: Fri, 27 Feb 2026 14:35:50 +1000
Message-ID: <review-patch1-20260224221227.69126-2-jonathan.cavitt@intel.com>
In-Reply-To: <20260224221227.69126-2-jonathan.cavitt@intel.com>
References: <20260224221227.69126-2-jonathan.cavitt@intel.com> <20260224221227.69126-2-jonathan.cavitt@intel.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

**Bug analysis:** Correct. Looking at lines 856=E2=80=93862 of `drm_client_=
modeset.c`:

```c
crtcs =3D kzalloc_objs(*crtcs, connector_count);
modes =3D kzalloc_objs(*modes, connector_count);
offsets =3D kzalloc_objs(*offsets, connector_count);
enabled =3D kzalloc_objs(bool, connector_count);
if (!crtcs || !modes || !enabled || !offsets) {
    ret =3D -ENOMEM;
    goto out;
}
```

If `crtcs` succeeds but `modes` fails, we jump to `out:` where `modes_destr=
oy()` is called with `modes =3D=3D NULL`. Inside `modes_destroy()`:

```c
static void modes_destroy(struct drm_device *dev,
                          const struct drm_display_mode *modes[],
                          int count)
{
    int i;

    for (i =3D 0; i < count; i++)
        mode_replace(dev, &modes[i], NULL);
}
```

The `modes[i]` dereference with `modes =3D=3D NULL` is indeed a NULL pointe=
r dereference. The fix is correct.

**Minor suggestion:** `kfree(NULL)` is safe in the kernel, and `kfree(modes=
)` already follows the `modes_destroy()` call. The guard added is:

```c
if (modes)
    modes_destroy(dev, modes, connector_count);
```

This is fine, but an alternative would be to add the NULL check inside `mod=
es_destroy()` itself, which would make it defensive at the source rather th=
an requiring every caller to remember the guard. That said, there is curren=
tly only one caller of `modes_destroy()`, so this approach is acceptable.

**Nit on the commit message:** The commit message says `modes` "may fail to=
 kcalloc", but the actual allocation macro used is `kzalloc_objs` (which wr=
aps `kzalloc`), not `kcalloc`. This is a very minor inaccuracy =E2=80=94 co=
nsider saying "may fail to allocate" instead.

**Verdict:** The fix is correct and the Fixes tag references the right comm=
it. The patch is ready to merge with at most a commit message wording tweak.

**Reviewed-by worthy:** Yes, with the minor commit message nit noted above.

---
Generated by Claude Code Patch Reviewer