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

[Janusz Krzysztofik]

Hi Krzysztof,

Thank you for looking at it.

On Thursday, 6 March 2025 12:00:40 CET Krzysztof Karas wrote:
> Hi Janusz,
> 
> throughout the series you modify the code right after
> introducing it. 

Yes, that split among patches reflects my way of getting to a solution that 
not only resolves the issue but also tries to address comments I got and take 
care of resulting code clarity.  That's why I mentioned the possibility of 
squashing one or more follow-ups with the initial patch.  Patch 1/4 alone is a 
minimal fix that actually resolves the issues.  The rest is only about 
satisfying Andi's comments (patch 2/4) and simplifying the code (patches 
3-4/4) that we may or may not want to apply or squash.

> How about changing the order of things a bit:
>  1) order the functions in a symmetrical way between
>   register/unregister steps and group them as you see necessary,
>   (At that point you would not be fixing the issue yet, but
>   prepare the code for further changes)

Please note that I still haven't achieved full symmetry.  If I only had clues 
from authors of patches that introduced asymmetry on why they did it that way 
then I would think of reordering the steps to achieve full symmetry, each in a 
separate patch, together with meaningful justification and possibly 
alternative solutions to issues that asymmetry was trying to address.  Without 
those clues, more work on analysis and more testing is needed, I believe, and 
that would be still more beyond the scope of a quick fix I initially intended.

> 
>  2) then introduce the new flag along with all the labels needed
>   for clean unregistration.

The flag, or a single global point of indication if device registration 
succeeded or not, was an idea suggested by Andi, and now objected by Jani from 
the display code PoV, so not a final solution.

BTW, have you seen v1 of the series[1]?  How do you find it, compared to v2/3?

Thanks,
Janusz

[1] https://lore.kernel.org/dri-devel/20250206180927.2237256-5-janusz.krzysztofik@linux.intel.com/

> 
> I think that way you could reduce number of patches (and changes
> in code needing review) while also fixing the original issue.
> 
> Overall, I believe this is a good effort and much needed change
> in registration and unregistering process.
> 
> Best Regards,
> Krzysztof
> 
> > Starting with commit ec3e00b4ee27 ("drm/i915: stop registering if
> > drm_dev_register() fails"), we may return from i915_driver_register()
> > immediately, skipping remaining registration steps.  However, the
> > _unregister() counterpart called at device remove knows nothing about that
> > skip and executes reverts of all those steps.  As a consequence, a number
> > of kernel warnings that taint the kernel are triggered.
> > 
> > Introduce a flag that indicates device registration status and raise it on
> > device registration success.  As a minimum (first patch), when that flag
> > is found not set while unregistering the driver, jump over those reverts
> > of registration steps omitted after device registration failure that are
> > not ready for being called unconditionally (and trigger the kernel
> > warnings).
> > 
> > With the second patch, also jump over reverts of other driver registration
> > steps that were not called due to device registration failure.  Some
> > unregister function calls, found implementing additional steps beyond the
> > register reverts, are still executed.
> > 
> > To simplify i915_driver_unregister() code, the third patch makes sure
> > reverts of driver registration steps executed before potentially
> > unsuccessful device registration are symmetrically called after
> > the device unplug.
> > 
> > Finally, the last patch further simplifies the i915_driver_unregister()
> > code by moving two required steps down, right after device unplug.
> > 
> > The first patch may be squashed with one or more of its follow-ups if so
> > decided.
> > 
> > Janusz Krzysztofik (4):
> >   drm/i915: Skip harmful unregister steps if not registered
> >   drm/i915: Omit unnecessary driver unregister steps
> >   drm/i915: Fix asymmetry in PMU register/unregister step order
> >   drm/i915: Group not skipped unregister steps
> > 
> >  drivers/gpu/drm/i915/gt/intel_gt.c |  6 ++++++
> >  drivers/gpu/drm/i915/i915_driver.c | 18 ++++++++++++------
> >  drivers/gpu/drm/i915/i915_drv.h    |  2 ++
> >  3 files changed, 20 insertions(+), 6 deletions(-)
> > 
>