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/gem: Remove unneeded ret check in change handle ioctl
Date: Sat, 16 May 2026 15:11:29 +1000
Message-ID: <review-patch1-20260511142540.2989187-1-jonathan.cavitt@intel.com>
In-Reply-To: <20260511142540.2989187-1-jonathan.cavitt@intel.com>
References: <20260511142540.2989187-1-jonathan.cavitt@intel.com> <20260511142540.2989187-1-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

**Base mismatch =E2=80=94 patch is obsolete.**

The patch removes these lines (from its base revision `abc8e41d6652`):

```c
	spin_unlock(&file_priv->table_lock);

-	if (ret < 0)
-		goto out_unlock;
-
	if (obj->dma_buf) {
```

The commit message states this `if (ret < 0)` check after `idr_replace` is =
dead code because "in the only place we modify the return value between the=
re and the previous return value check, we also immediately jump to out_unl=
ock, bypassing the check entirely." For an `idr_replace`-based implementati=
on, this analysis is likely correct =E2=80=94 `idr_replace` returns `void *=
` (not `int`), so any error from it would be caught via `IS_ERR()` and woul=
d immediately `goto out_unlock`, meaning `ret` is never set to a negative v=
alue in a path that falls through to this check.

**However, the current drm-next code at `drivers/gpu/drm/drm_gem.c:1044-105=
0` has been rewritten:**

```c
	spin_lock(&file_priv->table_lock);
	ret =3D idr_alloc(&file_priv->object_idr, obj, handle, handle + 1,
			GFP_NOWAIT);
	spin_unlock(&file_priv->table_lock);

	if (ret < 0)
		goto out_unlock;
```

In the current code, `ret` is set directly by `idr_alloc()`, which returns =
a negative errno on failure (e.g., `-ENOSPC` if the handle is already in us=
e, `-ENOMEM`). The `if (ret < 0)` check is **not dead code** =E2=80=94 it i=
s essential for error handling. Removing it in the current code would cause=
 the function to proceed with a failed allocation, leading to incorrect beh=
avior.

**Issues:**
1. The patch does not apply =E2=80=94 the pre-image hash `abc8e41d6652` doe=
s not match the current drm-next tree.
2. The commit message references `idr_replace`, but drm-next uses `idr_allo=
c` =E2=80=94 the function has been refactored since the patch was written.
3. Naively adapting this patch to the current code would introduce a bug by=
 removing a necessary error check on `idr_alloc`'s return value.

**Verdict:** The underlying dead-code observation was probably valid for th=
e `idr_replace` version, but the code has moved on. This patch should be dr=
opped. If the author rebases onto drm-next, they will find the `idr_replace=
` call is gone and the analogous `ret` check is no longer dead.

---
Generated by Claude Code Patch Reviewer