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: Set dev->registered back to false in case of register failure
Date: Mon, 25 May 2026 18:27:45 +1000
Message-ID: <review-patch1-20260522154037.46892-3-krzysztof.niemiec@intel.com>
In-Reply-To: <20260522154037.46892-3-krzysztof.niemiec@intel.com>
References: <20260522154037.46892-3-krzysztof.niemiec@intel.com> <20260522154037.46892-3-krzysztof.niemiec@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

**Correctness: Good.** The patch places `dev->registered =3D false` at the =
`err_minors` label, which is reached by both error paths (`err_unload` fall=
s through to `err_minors`). This means the flag is cleared regardless of wh=
ether the failure came from `driver->load()` or `drm_modeset_register_all()=
`.

Critically, `err_minors` is also the target for failures *before* `dev->reg=
istered =3D true` is set (lines 1075=E2=80=931089), so setting it to `false=
` there is a no-op in those cases (it was already `false`). This is harmles=
s and correct.

**Placement within `err_minors`:** The patch places `dev->registered =3D fa=
lse` at the top of `err_minors`, before `remove_compat_control_link()`:

```c
err_minors:
+	dev->registered =3D false;
	remove_compat_control_link(dev);
```

This mirrors `drm_dev_unregister()` which sets `dev->registered =3D false` =
as its first action (line 1148). The placement is consistent and correct =
=E2=80=94 clearing the flag first ensures that any code triggered during te=
ardown (e.g., `drm_minor_unregister` =E2=86=92 eventual mode object cleanup=
) sees the device as unregistered.

**Commit message quality:** The commit message is well-written. It explains=
 the history of how the bug was introduced (the later addition of `drm_mode=
set_register_all()` error checking), describes the concrete symptom (spurio=
us `WARN_ON` in `drm_mode_object_register()`), and is clear about the fix.

**One minor note on the commit message:** The message says the WARN_ON is i=
n `drm_mode_object_register()`, but looking at the current tree, `drm_mode_=
object_register()` doesn't contain a WARN_ON =E2=80=94 it's a simple `idr_r=
eplace()` under a mutex. The WARN_ON the author is likely referring to may =
be elsewhere (possibly in `drm_mode_object_unregister()` or in connector/fr=
amebuffer registration paths that check `dev->registered`). This doesn't af=
fect the correctness of the fix, but the commit message could be slightly m=
ore precise about which WARN_ON is triggered.

**Missing cleanup consideration:** Looking at the `err_unload` path: when `=
drm_modeset_register_all()` fails, the error path calls `driver->unload()` =
and then falls through to `err_minors`. However, `drm_modeset_register_all(=
)` success would be followed by `drm_panic_register()` and `drm_client_sysr=
q_register()` on the happy path. In the `err_unload` path, `drm_modeset_unr=
egister_all()` is *not* called to undo the partial modeset registration =E2=
=80=94 this is a pre-existing issue and out of scope for this patch, but wo=
rth noting.

**Verdict: The patch is correct and should be applied.** It's a clear, mini=
mal fix for a real inconsistency in the error handling.

---
Generated by Claude Code Patch Reviewer