Re: [PATCH v3 1/2] drm: Unwind drm device registration upon error

[Krzysztof Niemiec <krzysztof.niemiec@intel.com>]

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 <krzysztof.karas@intel.com>
> > Co-developed-by: Krzysztof Niemiec <krzysztof.niemiec@intel.com>
> > Signed-off-by: Krzysztof Niemiec <krzysztof.niemiec@intel.com>
> 
> I think the right order should be:
> 
> Co-developed-by: Krzysztof Karas <krzysztof.karas@intel.com>
> Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com>
> Signed-off-by: Krzysztof Niemiec <krzysztof.niemiec@intel.com>
> 
> > 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:

Why? Is .unload() supposed to expect dev->registered = false? I've seen
that moving the flag around might(?) make a bit of a mess (e0f32f78e51b),
and before this patch .unload() is being called with registered = true.
So changing it at the beginning of the unwind might mess up drivers that
rely on it, but it will make no difference for drivers that don't, so
I'd rather keep it this way. Unless we make explicit guarantees that
.unload() will be called with ->registered = false, in the middle of the
call to drm_dev_register().

> 
> err_unload:
> 	dev->registered = false;
> 	if (dev->driver->unload)
> 		dev->driver->unload(dev);
> 	goto err_cleanup;
> err_minors:
> 	dev->registered = false;
> err_clenup:
> 	...
> 
> Thanks,
> Andi