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.
What is more, commit 6915190a50e8 ("drm/client: Support emergency restore
via sysrq for all clients") introduced a new WARN_ON and added
drm_client_sysrq_register/unregister() calls to the drm device
register/unregister paths. Client sysrqs are registered only when
drm_dev_register() is sure to complete without failures, but
drm_client_sysrq_unregister() gets called every time during
drm_dev_unregister() and prints warning messages whenever the caller tries
to clean up the mess with immediate unregistration.
Set dev->registered back to false in the error path to prevent this.
Co-developed-by: Krzysztof Karas <[email protected]>
Signed-off-by: Krzysztof Karas <[email protected]>
Signed-off-by: Krzysztof Niemiec <[email protected]>
---
Changelog:
RFC -> v1:
* addedd title of commit 6915190a50e8 to commit message;
* rebased;
v1 -> v2:
* updated commit log slightly to mention possible bugs;
* updated authorship - the bug was found and fixed independently by
Krzysztof K. and Krzysztof N.; We decided to exchange the findings
and our comments and send the patch as co-developed-by.
v2 -> v3:
no functional changes
v3 -> v4:
* rebased
* removed the debugfs change (sashiko, Andi)
* put the =false in different places than v2/v3 (Andi). First driver I
sanity-checked that uses the .unload() callback calls
drm_mode_config_cleanup(), which expects registered=false down the
call stack, so =false needs to happen before the callback. Problem
is, the err_minors error path also needs to have the variable unset.
I'm not sure there's a way to do this cleanly given how the
.(un)load callbacks operate so we might be stuck with this, and it
smells, the way I did it smells less *to me* than what Andi suggested
with jumping between labels in the error path, but I'm not very
opinionated either
* spliced the v1 and v2/v3 commit logs together a bit to account for
the debugfs change removal
An instance of a bug caused by this was uncovered in i915 with fault
injection. During driver probe in the error path, after failing
drm_dev_register() there is a reasonable expectation of a full proper
teardown of whatever drm_dev_register() has done. This is, however, not
the case, as shown in the calls below:
i915_driver_probe()
i915_driver_register()
calls drm_dev_register()
drm_dev_register()
...
connector .late_register() callback
called from drm_dev_register(), and fails, leaving
dev->registered as true, propagates the error back up
...
drm_dev_register()
enters error path, but doesn't fully unwind
i915_driver_register()
enters error path
i915_driver_probe()
enters error path
intel_display_driver_remove_noirq()
called in _probe()'s error path
intel_mode_config_cleanup()
drm_mode_config_cleanup()
this triggers a WARN_ON(), dev->registered should be false
This is a bug on drm_dev_register()'s side.
drivers/gpu/drm/drm_drv.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index a2872c69b8c6..812add80831a 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -1135,9 +1135,11 @@ int drm_dev_register(struct drm_device *dev, unsigned
long flags)
goto out_unlock;
err_unload:
+ dev->registered = false;
if (dev->driver->unload)
dev->driver->unload(dev);
err_minors:
+ dev->registered = false; /* fallback for driver->load() error path */
/*
* If a minor was registered before the failure, userspace could have
* opened it and entered a drm_dev_enter() critical section. Ensure all
--
2.55.0
