[PATCH 0/3] drm/i915: Fix harmfull driver register/unregister assymetry

[Janusz Krzysztofik]

Hi Andi,

Thank you for review.

On Monday, 10 February 2025 14:01:19 CET Andi Shyti wrote:
> Hi Janusz,
> 
> On Thu, Feb 06, 2025 at 07:07:38PM +0100, Janusz Krzysztofik wrote:
> > We return immediately from i915_driver_register() if drm_dev_register()
> > fails, skipping remaining registration steps.  However, the _unregister()
> > counterpart called at device remove knows nothing about that skip and
> > executes reverts for all those steps.  For that to work correctly, those
> > revert functions must be resistant to being called even on uninitialized
> > objects, or we must not skip their initialization.
> > 
> > Three cases have been identified and fixes proposed.  Call traces are
> > taken from CI results of igt at i915_driver_load@reload-with-fault-injection
> > execution, reported to several separate Gitlab issues (links provided).
> > 
> > Immediate return was introduced to i915_driver_register() by commit
> > ec3e00b4ee27 ("drm/i915: stop registering if drm_dev_register() fails"),
> > however, quite a few things have changed since then.  That's why I haven't
> > mentioned it in a Fixes: tag to avoid it being picked up by stable, which
> > I haven't tested.
> 
> I'm not fully convinced about this series as I think that you are
> fixing a subset of what needs to be handled properly. What about
> hwmon? What about gt? what about debugfs?

For all of those, their _unregister() functions seem to be designed to be safe 
to call even if not registered.  Like e.g. kfree() -- you can call it safely 
even with NULL argument, you don't need to check for NULL and call it 
conditionally.  However, ...

> 
> In my opinion we need to check in _unregister whether the
> drm_dev_register has succeded 

I agree with you that it would be more clear if we skipped not only 
_register() but also _unregister() steps symmetrically, based on result of 
drm_dev_register().

> and one way would be, e.g., to
> check for the drm minor value, or even set the drm device tu NULL
> (first things that come to my mind, maybe there are smarter ways
> of doing it). 

As long as drm doesn't provide explicit support for checking if registration 
succeeded other than examining the return value of drm_dev_register(), I would 
rather store that value somewhere in our drm_i915_private structure instead of 
depending on drm internals.  What do you think?

Thanks,
Janusz


> This way we could skip some of the _unregister()
> steps.
> 
> Andi
>