On 2026-08-28 at 01:14:57 +0200, Andi Shyti wrote:
> Hi Krzysztof,
>
> ...
>
> > 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]>
>
> I think the right order should be:
>
> Co-developed-by: Krzysztof Karas <[email protected]>
> Signed-off-by: Krzysztof Karas <[email protected]>
> Signed-off-by: Krzysztof Niemiec <[email protected]>
>
> > 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);
>
> I think sashiko is right here, this should be removed.
>
> > + dev->registered = false;
>
> dev->registered = false should be set at the very beginning,
> maybe something like this:
>
> err_unload:
> dev->registered = false;
> if (dev->driver->unload)
> dev->driver->unload(dev);
> goto err_cleanup;
> err_minors:
> dev->registered = false;
> err_clenup:
> ...
>
I think we may need another label. Currently, dev->registered
state is ambiguous, because it is set to true before the last
goto err_minors. That would mean that putting it under
err_unload would still leave a gap, where registered == true and
is never unset on the error path, if driver->load() would fail.
How about:
if (driver->load) {
ret = driver->load(dev, flags);
if (ret)
goto err_registered;
}
...
err_unload:
if (dev->driver->unload)
dev->driver->unload(dev);
err_registered:
dev->registered = false;
err_minors:
...
--
Best Regards,
Krzysztof
