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@xxxxxxxxxxxxxxx/ > > 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(-) > > >
