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() tries to tear down
everything that was initialized during registration step.
This is confusing when considering a failure in
drm_dev_register(), because not all steps will be undone
before returning, so it is unclear whether a call to
drm_dev_unregister() is a requirement or not.

What is more, during the drm device registration client sysrqs
are added 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 attempts to clean up unsuccessful
registration with immediate unregistration.

Amend the problem by removing debugfs entries and setting
"registered" flag to false upon error in drm_dev_register().

Signed-off-by: Krzysztof Karas <[email protected]>
---
I noticed that some drivers use drm_dev_unregister() whenever
a call to drm_dev_register() and many do not. It is also a bit
confusing to me why drm_dev_register() does not completely
unwind all the initialization steps it performs, which leaves
me wondering if drm_dev_unregister() is required on the error
path or not.

The WARN_ON introduced in 6915190a50e8f7cf13dcbe534b02845be533b60a
exposed this problem, because previously there were no
notifications from these functions, so I noticed this mismatch
only thanks to the warnings.

I think the other way to solve this would be to require an
unregister call for each register call, but that would mean a
series of changes for the callers that currently do the cleanup
in their own way.
---
 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 2915118436ce..a502110696a3 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -1119,6 +1119,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.43.0

-- 
Best Regards,
Krzysztof

Reply via email to