Hey, Den 2025-12-08 kl. 13:09, skrev Krzysztof Karas: > 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);
I'm not convinced anything should depend on dev->registered being set to false, as we should only free the struct afterwards, but clearing out drm_debugfs would be a a good step. All in all looks sane. Reviewed-by: Maarten Lankhorst <[email protected]> Kind regards, ~Maarten Lankhorst
