[PATCH] drm: Set dev->registered back to false in case of register failure

[PATCH] drm: Set dev->registered back to false in case of register failure

[Krzysztof Niemiec]

The dev->registered variable was initially added in such a way that it
is only set after all functions that could have failed have been called
and haven't returned an error. However, since then, the function
drm_modeset_register_all() is being checked for its return value, which
opens a possibility of failing the register after setting the registered
variable anyway. drm_dev_register() cleans up after itself in case of
failure, mimicking the functions called in drm_dev_unregister(), with
the exception of drm_client_sysrq_unregister() and
drm_panic_unregister() (_register() counterparts of which are not
checked for their return values), and the dev->registered flag.

This creates a situation in which after calling drm_dev_register()
nothing is registered (as the function entered an error path and cleaned
up), but the device is reported as being registered according to the
dev->registered variable.

This, for example, confuses the WARN_ON() in drm_mode_object_register(), which
is raised if a non-dynamic mode object is attempted to be unregistered
in between the drm_dev_register() and drm_dev_unregister() calls. If
drm_dev_register() fails - meaning the device *isn't* registered -
currently it still reports dev->registered = true, which triggers the
WARN_ON(), even though the usage is correct in that instance.

Set dev->registered back to false in the error path to prevent this.

Signed-off-by: Krzysztof Niemiec <krzysztof.niemiec@intel.com>
---
 drivers/gpu/drm/drm_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 985c283cf59f..92403d79bb64 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -1116,6 +1116,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
 	if (dev->driver->unload)
 		dev->driver->unload(dev);
 err_minors:
+	dev->registered = false;
 	remove_compat_control_link(dev);
 	drm_minor_unregister(dev, DRM_MINOR_ACCEL);
 	drm_minor_unregister(dev, DRM_MINOR_PRIMARY);
-- 
2.45.2

Re: [PATCH] drm: Set dev->registered back to false in case of register failure

[Janusz Krzysztofik]

Hi Krzysztof,

On Fri, 2026-05-22 at 17:40 +0200, Krzysztof Niemiec wrote:
> The dev->registered variable was initially added in such a way that it
> is only set after all functions that could have failed have been called
> and haven't returned an error. However, since then, the function
> drm_modeset_register_all() is being checked for its return value, which
> opens a possibility of failing the register after setting the registered
> variable anyway. drm_dev_register() cleans up after itself in case of
> failure, mimicking the functions called in drm_dev_unregister(), with
> the exception of drm_client_sysrq_unregister() and
> drm_panic_unregister() (_register() counterparts of which are not
> checked for their return values), and the dev->registered flag.
> 
> This creates a situation in which after calling drm_dev_register()
> nothing is registered (as the function entered an error path and cleaned
> up), but the device is reported as being registered according to the
> dev->registered variable.
> 
> This, for example, confuses the WARN_ON() in drm_mode_object_register(), which
> is raised if a non-dynamic mode object is attempted to be unregistered
> in between the drm_dev_register() and drm_dev_unregister() calls. If
> drm_dev_register() fails - meaning the device *isn't* registered -
> currently it still reports dev->registered = true, which triggers the
> WARN_ON(), even though the usage is correct in that instance.
> 
> Set dev->registered back to false in the error path to prevent this.

While you've also submitted a patch ("drm/i915: Remove drm_dev_unregister()
from the error path during i915_driver_register()") that adjusts i915
handling of register / unregister steps after this fix, have you checked if
other drivers are prepared for that and won't be affected?

Thanks,
Janusz


> 
> Signed-off-by: Krzysztof Niemiec <krzysztof.niemiec@intel.com>
> ---
>  drivers/gpu/drm/drm_drv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 985c283cf59f..92403d79bb64 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -1116,6 +1116,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
>  	if (dev->driver->unload)
>  		dev->driver->unload(dev);
>  err_minors:
> +	dev->registered = false;
>  	remove_compat_control_link(dev);
>  	drm_minor_unregister(dev, DRM_MINOR_ACCEL);
>  	drm_minor_unregister(dev, DRM_MINOR_PRIMARY);

Re: [PATCH] drm: Set dev->registered back to false in case of register failure

[Krzysztof Niemiec]

On 2026-05-25 at 10:16:32 GMT, Janusz Krzysztofik wrote:
> Hi Krzysztof,
> 
> On Fri, 2026-05-22 at 17:40 +0200, Krzysztof Niemiec wrote:
> > The dev->registered variable was initially added in such a way that it
> > is only set after all functions that could have failed have been called
> > and haven't returned an error. However, since then, the function
> > drm_modeset_register_all() is being checked for its return value, which
> > opens a possibility of failing the register after setting the registered
> > variable anyway. drm_dev_register() cleans up after itself in case of
> > failure, mimicking the functions called in drm_dev_unregister(), with
> > the exception of drm_client_sysrq_unregister() and
> > drm_panic_unregister() (_register() counterparts of which are not
> > checked for their return values), and the dev->registered flag.
> > 
> > This creates a situation in which after calling drm_dev_register()
> > nothing is registered (as the function entered an error path and cleaned
> > up), but the device is reported as being registered according to the
> > dev->registered variable.
> > 
> > This, for example, confuses the WARN_ON() in drm_mode_object_register(), which
> > is raised if a non-dynamic mode object is attempted to be unregistered
> > in between the drm_dev_register() and drm_dev_unregister() calls. If
> > drm_dev_register() fails - meaning the device *isn't* registered -
> > currently it still reports dev->registered = true, which triggers the
> > WARN_ON(), even though the usage is correct in that instance.
> > 
> > Set dev->registered back to false in the error path to prevent this.
> 
> While you've also submitted a patch ("drm/i915: Remove drm_dev_unregister()
> from the error path during i915_driver_register()") that adjusts i915
> handling of register / unregister steps after this fix, have you checked if
> other drivers are prepared for that and won't be affected?

I just did; there'd be a problem if in the error path triggered by
failing drm_dev_register, some driver relies on the dev->registered
variable to be true to do its work.

There are two main patterns of doing the error path in the DRM drivers
I've seen - one is just directly returning from the overarching function
with drm_dev_register()'s error return value. The other is invoking the
.unbind handler of the driver via component_unbind_all(), and most of
the time those call drm_dev_unregister() right away, which sets the
dev->registered value to false anyway. In these two cases (if
drm_dev_unregister() is called in the second one), the problem doesn't
affect the driver at all.

In the case of i915 not setting the variable to false on failure
triggered a WARN_ON() in drm_mode_object_unregister() because that is
invoked indirectly in the error path due to the complexity of our
init code (even xe just returns directly instead). If another driver
calls that function in its error path, it will just silence (correctly)
the WARN_ONs there.

The only driver that explicitly checks for dev->registerd in its error
path is msm, which does that to see if it needs to invoke
drm_dev_unregister() during unload. This patch actually fixes a bug there
because otherwise _unregister() would be called even if _register() fails,
which is not correct.

All other uses of the variable are outside the error path of their
respective drivers, so they should function as intended even after the
change.

Thanks
Krzysztof

> 
> Thanks,
> Janusz
> 
> 
> > 
> > Signed-off-by: Krzysztof Niemiec <krzysztof.niemiec@intel.com>
> > ---
> >  drivers/gpu/drm/drm_drv.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 985c283cf59f..92403d79bb64 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -1116,6 +1116,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
> >  	if (dev->driver->unload)
> >  		dev->driver->unload(dev);
> >  err_minors:
> > +	dev->registered = false;
> >  	remove_compat_control_link(dev);
> >  	drm_minor_unregister(dev, DRM_MINOR_ACCEL);
> >  	drm_minor_unregister(dev, DRM_MINOR_PRIMARY);

Re: [PATCH] drm: Set dev->registered back to false in case of register failure

[Krzysztof Niemiec]

On 2026-05-22 at 17:40:39 GMT, Krzysztof Niemiec wrote:
> The dev->registered variable was initially added in such a way that it
> is only set after all functions that could have failed have been called
> and haven't returned an error. However, since then, the function
> drm_modeset_register_all() is being checked for its return value, which
> opens a possibility of failing the register after setting the registered
> variable anyway. drm_dev_register() cleans up after itself in case of
> failure, mimicking the functions called in drm_dev_unregister(), with
> the exception of drm_client_sysrq_unregister() and
> drm_panic_unregister() (_register() counterparts of which are not
> checked for their return values), and the dev->registered flag.
> 
> This creates a situation in which after calling drm_dev_register()
> nothing is registered (as the function entered an error path and cleaned
> up), but the device is reported as being registered according to the
> dev->registered variable.
> 
> This, for example, confuses the WARN_ON() in drm_mode_object_register(), which

(typo, s/drm_mode_object_register/drm_mode_object_unregister)

> is raised if a non-dynamic mode object is attempted to be unregistered
> in between the drm_dev_register() and drm_dev_unregister() calls. If
> drm_dev_register() fails - meaning the device *isn't* registered -
> currently it still reports dev->registered = true, which triggers the
> WARN_ON(), even though the usage is correct in that instance.
> 
> Set dev->registered back to false in the error path to prevent this.
> 
> Signed-off-by: Krzysztof Niemiec <krzysztof.niemiec@intel.com>
> ---
>  drivers/gpu/drm/drm_drv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 985c283cf59f..92403d79bb64 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -1116,6 +1116,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
>  	if (dev->driver->unload)
>  		dev->driver->unload(dev);
>  err_minors:
> +	dev->registered = false;
>  	remove_compat_control_link(dev);
>  	drm_minor_unregister(dev, DRM_MINOR_ACCEL);
>  	drm_minor_unregister(dev, DRM_MINOR_PRIMARY);
> -- 
> 2.45.2
> 

Re: [PATCH] drm: Set dev->registered back to false in case of register failure

[Janusz Krzysztofik]

On Thu, 2026-05-28 at 17:24 +0200, Krzysztof Niemiec wrote:
> On 2026-05-25 at 10:16:32 GMT, Janusz Krzysztofik wrote:
> > Hi Krzysztof,
> > 
> > On Fri, 2026-05-22 at 17:40 +0200, Krzysztof Niemiec wrote:
> > > The dev->registered variable was initially added in such a way that it
> > > is only set after all functions that could have failed have been called
> > > and haven't returned an error. However, since then, the function
> > > drm_modeset_register_all() is being checked for its return value, which
> > > opens a possibility of failing the register after setting the registered
> > > variable anyway. drm_dev_register() cleans up after itself in case of
> > > failure, mimicking the functions called in drm_dev_unregister(), with
> > > the exception of drm_client_sysrq_unregister() and
> > > drm_panic_unregister() (_register() counterparts of which are not
> > > checked for their return values), and the dev->registered flag.
> > > 
> > > This creates a situation in which after calling drm_dev_register()
> > > nothing is registered (as the function entered an error path and cleaned
> > > up), but the device is reported as being registered according to the
> > > dev->registered variable.
> > > 
> > > This, for example, confuses the WARN_ON() in drm_mode_object_register(), which
> > > is raised if a non-dynamic mode object is attempted to be unregistered
> > > in between the drm_dev_register() and drm_dev_unregister() calls. If
> > > drm_dev_register() fails - meaning the device *isn't* registered -
> > > currently it still reports dev->registered = true, which triggers the
> > > WARN_ON(), even though the usage is correct in that instance.
> > > 
> > > Set dev->registered back to false in the error path to prevent this.
> > 
> > While you've also submitted a patch ("drm/i915: Remove drm_dev_unregister()
> > from the error path during i915_driver_register()") that adjusts i915
> > handling of register / unregister steps after this fix, have you checked if
> > other drivers are prepared for that and won't be affected?
> 
> I just did; there'd be a problem if in the error path triggered by
> failing drm_dev_register, some driver relies on the dev->registered
> variable to be true to do its work.

OK, maybe my doubts about other drivers were groundless.  The flag is
described as DRM internal after all, and cleaning it in the error unwind
path seems fully justified.  However, please review your commit
description.  Trying to understand why drm_mode_object_register() might
call WARN_ON() when dev->registered is true, I found it doesn't, and that's
probably its _unregister() counterpart that may do that.  With no stack
trace attached, that's confusing for a reviewer.  Such WARN_ON()s may be
hit not only due to inconsistencies internal to DRM, but also as a result
of drivers incorrectly / unexpectedly calling some DRM functions.

Thanks,
Janusz


> 
> There are two main patterns of doing the error path in the DRM drivers
> I've seen - one is just directly returning from the overarching function
> with drm_dev_register()'s error return value. The other is invoking the
> .unbind handler of the driver via component_unbind_all(), and most of
> the time those call drm_dev_unregister() right away, which sets the
> dev->registered value to false anyway. In these two cases (if
> drm_dev_unregister() is called in the second one), the problem doesn't
> affect the driver at all.
> 
> In the case of i915 not setting the variable to false on failure
> triggered a WARN_ON() in drm_mode_object_unregister() because that is
> invoked indirectly in the error path due to the complexity of our
> init code (even xe just returns directly instead). If another driver
> calls that function in its error path, it will just silence (correctly)
> the WARN_ONs there.
> 
> The only driver that explicitly checks for dev->registerd in its error
> path is msm, which does that to see if it needs to invoke
> drm_dev_unregister() during unload. This patch actually fixes a bug there
> because otherwise _unregister() would be called even if _register() fails,
> which is not correct.
> 
> All other uses of the variable are outside the error path of their
> respective drivers, so they should function as intended even after the
> change.
> 
> Thanks
> Krzysztof
> 
> > 
> > Thanks,
> > Janusz
> > 
> > 
> > > 
> > > Signed-off-by: Krzysztof Niemiec <krzysztof.niemiec@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_drv.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > > index 985c283cf59f..92403d79bb64 100644
> > > --- a/drivers/gpu/drm/drm_drv.c
> > > +++ b/drivers/gpu/drm/drm_drv.c
> > > @@ -1116,6 +1116,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
> > >  	if (dev->driver->unload)
> > >  		dev->driver->unload(dev);
> > >  err_minors:
> > > +	dev->registered = false;
> > >  	remove_compat_control_link(dev);
> > >  	drm_minor_unregister(dev, DRM_MINOR_ACCEL);
> > >  	drm_minor_unregister(dev, DRM_MINOR_PRIMARY);

Re: [PATCH] drm: Set dev->registered back to false in case of register failure

[Krzysztof Niemiec]

On 2026-05-29 at 14:49:29 GMT, Janusz Krzysztofik wrote:
> On Thu, 2026-05-28 at 17:24 +0200, Krzysztof Niemiec wrote:
> > On 2026-05-25 at 10:16:32 GMT, Janusz Krzysztofik wrote:
> > > Hi Krzysztof,
> > > 
> > > On Fri, 2026-05-22 at 17:40 +0200, Krzysztof Niemiec wrote:
> > > > The dev->registered variable was initially added in such a way that it
> > > > is only set after all functions that could have failed have been called
> > > > and haven't returned an error. However, since then, the function
> > > > drm_modeset_register_all() is being checked for its return value, which
> > > > opens a possibility of failing the register after setting the registered
> > > > variable anyway. drm_dev_register() cleans up after itself in case of
> > > > failure, mimicking the functions called in drm_dev_unregister(), with
> > > > the exception of drm_client_sysrq_unregister() and
> > > > drm_panic_unregister() (_register() counterparts of which are not
> > > > checked for their return values), and the dev->registered flag.
> > > > 
> > > > This creates a situation in which after calling drm_dev_register()
> > > > nothing is registered (as the function entered an error path and cleaned
> > > > up), but the device is reported as being registered according to the
> > > > dev->registered variable.
> > > > 
> > > > This, for example, confuses the WARN_ON() in drm_mode_object_register(), which
> > > > is raised if a non-dynamic mode object is attempted to be unregistered
> > > > in between the drm_dev_register() and drm_dev_unregister() calls. If
> > > > drm_dev_register() fails - meaning the device *isn't* registered -
> > > > currently it still reports dev->registered = true, which triggers the
> > > > WARN_ON(), even though the usage is correct in that instance.
> > > > 
> > > > Set dev->registered back to false in the error path to prevent this.
> > > 
> > > While you've also submitted a patch ("drm/i915: Remove drm_dev_unregister()
> > > from the error path during i915_driver_register()") that adjusts i915
> > > handling of register / unregister steps after this fix, have you checked if
> > > other drivers are prepared for that and won't be affected?
> > 
> > I just did; there'd be a problem if in the error path triggered by
> > failing drm_dev_register, some driver relies on the dev->registered
> > variable to be true to do its work.
> 
> OK, maybe my doubts about other drivers were groundless.  The flag is
> described as DRM internal after all, and cleaning it in the error unwind
> path seems fully justified.  However, please review your commit
> description.  Trying to understand why drm_mode_object_register() might
> call WARN_ON() when dev->registered is true, I found it doesn't, and that's
> probably its _unregister() counterpart that may do that.  With no stack
> trace attached, that's confusing for a reviewer.  Such WARN_ON()s may be
> hit not only due to inconsistencies internal to DRM, but also as a result
> of drivers incorrectly / unexpectedly calling some DRM functions.

Yes, I've made a mistake in the commit log, I meant _unregister() - see
[1]. Do you think going through how i915, specifically, hits this
WARN_ON() would be a sufficient example?

Thanks,
Krzysztof

[1] https://elixir.bootlin.com/linux/v7.0.10/source/drivers/gpu/drm/drm_mode_object.c#L106-L117

> 
> Thanks,
> Janusz
> 
> 
> > 
> > There are two main patterns of doing the error path in the DRM drivers
> > I've seen - one is just directly returning from the overarching function
> > with drm_dev_register()'s error return value. The other is invoking the
> > .unbind handler of the driver via component_unbind_all(), and most of
> > the time those call drm_dev_unregister() right away, which sets the
> > dev->registered value to false anyway. In these two cases (if
> > drm_dev_unregister() is called in the second one), the problem doesn't
> > affect the driver at all.
> > 
> > In the case of i915 not setting the variable to false on failure
> > triggered a WARN_ON() in drm_mode_object_unregister() because that is
> > invoked indirectly in the error path due to the complexity of our
> > init code (even xe just returns directly instead). If another driver
> > calls that function in its error path, it will just silence (correctly)
> > the WARN_ONs there.
> > 
> > The only driver that explicitly checks for dev->registerd in its error
> > path is msm, which does that to see if it needs to invoke
> > drm_dev_unregister() during unload. This patch actually fixes a bug there
> > because otherwise _unregister() would be called even if _register() fails,
> > which is not correct.
> > 
> > All other uses of the variable are outside the error path of their
> > respective drivers, so they should function as intended even after the
> > change.
> > 
> > Thanks
> > Krzysztof
> > 
> > > 
> > > Thanks,
> > > Janusz
> > > 
> > > 
> > > > 
> > > > Signed-off-by: Krzysztof Niemiec <krzysztof.niemiec@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_drv.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > > > index 985c283cf59f..92403d79bb64 100644
> > > > --- a/drivers/gpu/drm/drm_drv.c
> > > > +++ b/drivers/gpu/drm/drm_drv.c
> > > > @@ -1116,6 +1116,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
> > > >  	if (dev->driver->unload)
> > > >  		dev->driver->unload(dev);
> > > >  err_minors:
> > > > +	dev->registered = false;
> > > >  	remove_compat_control_link(dev);
> > > >  	drm_minor_unregister(dev, DRM_MINOR_ACCEL);
> > > >  	drm_minor_unregister(dev, DRM_MINOR_PRIMARY);

Re: [PATCH] drm: Set dev->registered back to false in case of register failure

[Janusz Krzysztofik]

On Fri, 2026-05-29 at 17:27 +0200, Krzysztof Niemiec wrote:
> On 2026-05-29 at 14:49:29 GMT, Janusz Krzysztofik wrote:
> > On Thu, 2026-05-28 at 17:24 +0200, Krzysztof Niemiec wrote:
> > > On 2026-05-25 at 10:16:32 GMT, Janusz Krzysztofik wrote:
> > > > Hi Krzysztof,
> > > > 
> > > > On Fri, 2026-05-22 at 17:40 +0200, Krzysztof Niemiec wrote:
> > > > > The dev->registered variable was initially added in such a way that it
> > > > > is only set after all functions that could have failed have been called
> > > > > and haven't returned an error. However, since then, the function
> > > > > drm_modeset_register_all() is being checked for its return value, which
> > > > > opens a possibility of failing the register after setting the registered
> > > > > variable anyway. drm_dev_register() cleans up after itself in case of
> > > > > failure, mimicking the functions called in drm_dev_unregister(), with
> > > > > the exception of drm_client_sysrq_unregister() and
> > > > > drm_panic_unregister() (_register() counterparts of which are not
> > > > > checked for their return values), and the dev->registered flag.
> > > > > 
> > > > > This creates a situation in which after calling drm_dev_register()
> > > > > nothing is registered (as the function entered an error path and cleaned
> > > > > up), but the device is reported as being registered according to the
> > > > > dev->registered variable.
> > > > > 
> > > > > This, for example, confuses the WARN_ON() in drm_mode_object_register(), which
> > > > > is raised if a non-dynamic mode object is attempted to be unregistered
> > > > > in between the drm_dev_register() and drm_dev_unregister() calls. If
> > > > > drm_dev_register() fails - meaning the device *isn't* registered -
> > > > > currently it still reports dev->registered = true, which triggers the
> > > > > WARN_ON(), even though the usage is correct in that instance.
> > > > > 
> > > > > Set dev->registered back to false in the error path to prevent this.
> > > > 
> > > > While you've also submitted a patch ("drm/i915: Remove drm_dev_unregister()
> > > > from the error path during i915_driver_register()") that adjusts i915
> > > > handling of register / unregister steps after this fix, have you checked if
> > > > other drivers are prepared for that and won't be affected?
> > > 
> > > I just did; there'd be a problem if in the error path triggered by
> > > failing drm_dev_register, some driver relies on the dev->registered
> > > variable to be true to do its work.
> > 
> > OK, maybe my doubts about other drivers were groundless.  The flag is
> > described as DRM internal after all, and cleaning it in the error unwind
> > path seems fully justified.  However, please review your commit
> > description.  Trying to understand why drm_mode_object_register() might
> > call WARN_ON() when dev->registered is true, I found it doesn't, and that's
> > probably its _unregister() counterpart that may do that.  With no stack
> > trace attached, that's confusing for a reviewer.  Such WARN_ON()s may be
> > hit not only due to inconsistencies internal to DRM, but also as a result
> > of drivers incorrectly / unexpectedly calling some DRM functions.
> 
> Yes, I've made a mistake in the commit log, I meant _unregister() - see
> [1]. Do you think going through how i915, specifically, hits this
> WARN_ON() would be a sufficient example?

Yes, especially if you are confident that's not i915 fault and you can tell
when and how i915 may legitimately trigger that path and why it may expect 
dev->registered not set by then.  I think that would evidently help to
convince reviewers that the change is really needed.

Thanks,
Janusz


> 
> Thanks,
> Krzysztof
> 
> [1] https://elixir.bootlin.com/linux/v7.0.10/source/drivers/gpu/drm/drm_mode_object.c#L106-L117
> 
> > 
> > Thanks,
> > Janusz
> > 
> > 
> > > 
> > > There are two main patterns of doing the error path in the DRM drivers
> > > I've seen - one is just directly returning from the overarching function
> > > with drm_dev_register()'s error return value. The other is invoking the
> > > .unbind handler of the driver via component_unbind_all(), and most of
> > > the time those call drm_dev_unregister() right away, which sets the
> > > dev->registered value to false anyway. In these two cases (if
> > > drm_dev_unregister() is called in the second one), the problem doesn't
> > > affect the driver at all.
> > > 
> > > In the case of i915 not setting the variable to false on failure
> > > triggered a WARN_ON() in drm_mode_object_unregister() because that is
> > > invoked indirectly in the error path due to the complexity of our
> > > init code (even xe just returns directly instead). If another driver
> > > calls that function in its error path, it will just silence (correctly)
> > > the WARN_ONs there.
> > > 
> > > The only driver that explicitly checks for dev->registerd in its error
> > > path is msm, which does that to see if it needs to invoke
> > > drm_dev_unregister() during unload. This patch actually fixes a bug there
> > > because otherwise _unregister() would be called even if _register() fails,
> > > which is not correct.
> > > 
> > > All other uses of the variable are outside the error path of their
> > > respective drivers, so they should function as intended even after the
> > > change.
> > > 
> > > Thanks
> > > Krzysztof
> > > 
> > > > 
> > > > Thanks,
> > > > Janusz
> > > > 
> > > > 
> > > > > 
> > > > > Signed-off-by: Krzysztof Niemiec <krzysztof.niemiec@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_drv.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > > > > index 985c283cf59f..92403d79bb64 100644
> > > > > --- a/drivers/gpu/drm/drm_drv.c
> > > > > +++ b/drivers/gpu/drm/drm_drv.c
> > > > > @@ -1116,6 +1116,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
> > > > >  	if (dev->driver->unload)
> > > > >  		dev->driver->unload(dev);
> > > > >  err_minors:
> > > > > +	dev->registered = false;
> > > > >  	remove_compat_control_link(dev);
> > > > >  	drm_minor_unregister(dev, DRM_MINOR_ACCEL);
> > > > >  	drm_minor_unregister(dev, DRM_MINOR_PRIMARY);