[PATCH] drm: Set dev->registered back to false in case of register failure

[PATCH] drm: Set dev->registered back to false in case of register failure

[Krzysztof Niemiec]

The dev->registered variable was initially added in such a way that it
is only set after all functions that could have failed have been called
and haven't returned an error. However, since then, the function
drm_modeset_register_all() is being checked for its return value, which
opens a possibility of failing the register after setting the registered
variable anyway. drm_dev_register() cleans up after itself in case of
failure, mimicking the functions called in drm_dev_unregister(), with
the exception of drm_client_sysrq_unregister() and
drm_panic_unregister() (_register() counterparts of which are not
checked for their return values), and the dev->registered flag.

This creates a situation in which after calling drm_dev_register()
nothing is registered (as the function entered an error path and cleaned
up), but the device is reported as being registered according to the
dev->registered variable.

This, for example, confuses the WARN_ON() in drm_mode_object_register(), which
is raised if a non-dynamic mode object is attempted to be unregistered
in between the drm_dev_register() and drm_dev_unregister() calls. If
drm_dev_register() fails - meaning the device *isn't* registered -
currently it still reports dev->registered = true, which triggers the
WARN_ON(), even though the usage is correct in that instance.

Set dev->registered back to false in the error path to prevent this.

Signed-off-by: Krzysztof Niemiec <krzysztof.niemiec@intel.com>
---
 drivers/gpu/drm/drm_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 985c283cf59f..92403d79bb64 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -1116,6 +1116,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
 	if (dev->driver->unload)
 		dev->driver->unload(dev);
 err_minors:
+	dev->registered = false;
 	remove_compat_control_link(dev);
 	drm_minor_unregister(dev, DRM_MINOR_ACCEL);
 	drm_minor_unregister(dev, DRM_MINOR_PRIMARY);
-- 
2.45.2

Re: [PATCH] drm: Set dev->registered back to false in case of register failure

[Janusz Krzysztofik]

Hi Krzysztof,

On Fri, 2026-05-22 at 17:40 +0200, Krzysztof Niemiec wrote:
> The dev->registered variable was initially added in such a way that it
> is only set after all functions that could have failed have been called
> and haven't returned an error. However, since then, the function
> drm_modeset_register_all() is being checked for its return value, which
> opens a possibility of failing the register after setting the registered
> variable anyway. drm_dev_register() cleans up after itself in case of
> failure, mimicking the functions called in drm_dev_unregister(), with
> the exception of drm_client_sysrq_unregister() and
> drm_panic_unregister() (_register() counterparts of which are not
> checked for their return values), and the dev->registered flag.
> 
> This creates a situation in which after calling drm_dev_register()
> nothing is registered (as the function entered an error path and cleaned
> up), but the device is reported as being registered according to the
> dev->registered variable.
> 
> This, for example, confuses the WARN_ON() in drm_mode_object_register(), which
> is raised if a non-dynamic mode object is attempted to be unregistered
> in between the drm_dev_register() and drm_dev_unregister() calls. If
> drm_dev_register() fails - meaning the device *isn't* registered -
> currently it still reports dev->registered = true, which triggers the
> WARN_ON(), even though the usage is correct in that instance.
> 
> Set dev->registered back to false in the error path to prevent this.

While you've also submitted a patch ("drm/i915: Remove drm_dev_unregister()
from the error path during i915_driver_register()") that adjusts i915
handling of register / unregister steps after this fix, have you checked if
other drivers are prepared for that and won't be affected?

Thanks,
Janusz


> 
> Signed-off-by: Krzysztof Niemiec <krzysztof.niemiec@intel.com>
> ---
>  drivers/gpu/drm/drm_drv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 985c283cf59f..92403d79bb64 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -1116,6 +1116,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
>  	if (dev->driver->unload)
>  		dev->driver->unload(dev);
>  err_minors:
> +	dev->registered = false;
>  	remove_compat_control_link(dev);
>  	drm_minor_unregister(dev, DRM_MINOR_ACCEL);
>  	drm_minor_unregister(dev, DRM_MINOR_PRIMARY);

Claude review: drm: Set dev->registered back to false in case of register failure

[Claude Code Review Bot]

Overall Series Review

Subject: drm: Set dev->registered back to false in case of register failure
Author: Krzysztof Niemiec <krzysztof.niemiec@intel.com>
Patches: 2
Reviewed: 2026-05-25T18:27:44.910345

---

This is a single-patch fix for a real bug in the `drm_dev_register()` error path. The analysis is correct and the fix is appropriate.

The problem: `dev->registered` is set to `true` at line 1091, before `driver->load()` (line 1094) and `drm_modeset_register_all()` (line 1100) are called. If either of these fails, the error path cleans up the registration but never resets `dev->registered` back to `false`. This leaves the device in an inconsistent state — all registration side-effects have been unwound, but the flag still claims the device is registered.

The fix is a one-line addition that is both minimal and correct.

---
Generated by Claude Code Patch Reviewer

Claude review: drm: Set dev->registered back to false in case of register failure

[Claude Code Review Bot]

Patch Review

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

Critically, `err_minors` is also the target for failures *before* `dev->registered = true` is set (lines 1075–1089), so setting it to `false` there is a no-op in those cases (it was already `false`). This is harmless and correct.

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

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

This mirrors `drm_dev_unregister()` which sets `dev->registered = false` as its first action (line 1148). The placement is consistent and correct — clearing the flag first ensures that any code triggered during teardown (e.g., `drm_minor_unregister` → 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_modeset_register_all()` error checking), describes the concrete symptom (spurious `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 in `drm_mode_object_register()`, but looking at the current tree, `drm_mode_object_register()` doesn't contain a WARN_ON — it's a simple `idr_replace()` under a mutex. The WARN_ON the author is likely referring to may be elsewhere (possibly in `drm_mode_object_unregister()` or in connector/framebuffer registration paths that check `dev->registered`). This doesn't affect the correctness of the fix, but the commit message could be slightly more 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_sysrq_register()` on the happy path. In the `err_unload` path, `drm_modeset_unregister_all()` is *not* called to undo the partial modeset registration — this is a pre-existing issue and out of scope for this patch, but worth noting.

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

---
Generated by Claude Code Patch Reviewer