From: Krzysztof Karas <[email protected]>
The drm device registration is done via drm_dev_register().
This function attempts to undo some of the initiatlization steps
under err_unload and err_minors labels, but this process is
incomplete - debugfs entries remain and the dev->registered flag
is still set to true.
On the other hand, drm_dev_unregister() tears down everything
that was initialized during registration step. This is confusing
when considering a failure in drm_dev_register(), because some
setup steps will be undone and some settings will remain, so it
is unclear whether a call to drm_dev_unregister() is a
requirement or not.
This may trigger bugs in the error paths of drivers which rely on
functions that check for the value of dev->registered flag.
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.
Amend the problem by removing debugfs entries and marking
registered flag as false upon error in drm_dev_register().
Signed-off-by: Krzysztof Karas <[email protected]>
Co-developed-by: Krzysztof Niemiec <[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.
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 e51ed959da89..b98e8af6f1c1 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -1140,6 +1140,8 @@ int drm_dev_register(struct drm_device *dev, unsigned
long flags)
drm_minor_unregister(dev, DRM_MINOR_ACCEL);
drm_minor_unregister(dev, DRM_MINOR_PRIMARY);
drm_minor_unregister(dev, DRM_MINOR_RENDER);
+ drm_debugfs_dev_fini(dev);
+ dev->registered = false;
out_unlock:
if (drm_dev_needs_global_mutex(dev))
mutex_unlock(&drm_global_mutex);
--
2.55.0
