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: dp_tunnel: use kzalloc_flex
Date: Mon, 09 Mar 2026 07:39:02 +1000
Message-ID: <review-patch1-20260308211220.33759-1-rosenp@gmail.com>
In-Reply-To: <20260308211220.33759-1-rosenp@gmail.com>
References: <20260308211220.33759-1-rosenp@gmail.com> <20260308211220.33759-1-rosenp@gmail.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

**The good:**

- The conversion from a separate `groups` pointer to a flexible array membe=
r `groups[]` with `__counted_by(group_count)` is clean and appropriate.
- Removing the second `kzalloc` for `mgr->groups` and the corresponding `kf=
ree(mgr->groups)` in `destroy_mgr` is correct =E2=80=94 with a flex array, =
a single `kfree(mgr)` suffices.
- Using `kzalloc_flex(*mgr, groups, max_group_count)` matches the macro's e=
xpected signature.

**Bug =E2=80=94 error handling dropped from `init_group` loop:**

The patch changes:
```c
-	for (i =3D 0; i < max_group_count; i++) {
-		if (!init_group(mgr, &mgr->groups[i])) {
-			destroy_mgr(mgr);
-
-			return ERR_PTR(-ENOMEM);
-		}
-
-		mgr->group_count++;
-	}
+	for (i =3D 0; i < max_group_count; i++)
+		init_group(mgr, &mgr->groups[i]);
```

The commit message claims "It always returns true. That is, max_group_count=
 is always equal to group_count." Looking at `init_group` (line 1597), this=
 is indeed **currently true** =E2=80=94 the function unconditionally return=
s `true`. So the behavioral change is safe today.

However, the patch also **silently changes the return type expectation**: b=
y removing the error check entirely and ignoring the return value, any futu=
re change to `init_group` that introduces a failure path would silently be =
ignored. The cleaner approach would be to also change `init_group`'s return=
 type from `bool` to `void` in the same patch to make the contract explicit=
. Otherwise a future developer seeing `init_group` returns `bool` might add=
 a failure path without realizing callers now ignore it.

**Nit =E2=80=94 `group_count` assignment before `init_group` loop:**

```c
+	mgr->group_count =3D max_group_count;
```

This is set before the `init_group` loop. In the old code, `mgr->group_coun=
t` was incremented one-by-one as each group was initialized, so `destroy_mg=
r` would only clean up successfully initialized groups. With the new code, =
if `init_group` were ever changed to fail, `destroy_mgr` would attempt to c=
lean up uninitialized groups.

This is fine given `init_group` can't fail, but it's another reason to **ch=
ange `init_group` to return `void`** in the same patch to make the invarian=
t clear.

**Struct field reordering:**

```c
-	int group_count;
-	struct drm_dp_tunnel_group *groups;
 	wait_queue_head_t bw_req_queue;
 ...
+	int group_count;
+	struct drm_dp_tunnel_group groups[] __counted_by(group_count);
```

Moving `group_count` and `groups` to the end of the struct is required sinc=
e flex arrays must be the last member. The `group_count` field is also move=
d to be adjacent to `groups[]` which is good for `__counted_by`. The reorde=
ring may introduce a small hole for padding between `wait_queue_head_t bw_r=
eq_queue` and the `#ifdef` block, but this is unlikely to matter in practic=
e.

**Recommendation:** The patch should also change `init_group` from `static =
bool` to `static void` to match the new calling convention. Without that, t=
he patch is technically correct but leaves a misleading function signature.

---
Generated by Claude Code Patch Reviewer