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

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

[Janusz Krzysztofik]

Starting with commit ec3e00b4ee27 ("drm/i915: stop registering if
drm_dev_register() fails"), 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 of all
those steps.  As a consequence, a number of kernel warnings that taint the
kernel are triggered:

<3> [525.823143] i915 0000:00:02.0: [drm] *ERROR* Failed to register driver for
userspace access!
...
<4> [525.831069] ------------[ cut here ]------------
<4> [525.831071] i915 0000:00:02.0: [drm] drm_WARN_ON(power_domains->init_wakere
f)
<4> [525.831095] WARNING: CPU: 6 PID: 3440 at drivers/gpu/drm/i915/display/intel
_display_power.c:2074 intel_power_domains_disable+0xc2/0xd0 [i915]
...
<4> [525.831328] CPU: 6 UID: 0 PID: 3440 Comm: i915_module_loa Tainted: G     U
            6.14.0-rc1-CI_DRM_16076-g7a632b6798b6+ #1
...
<4> [525.831334] RIP: 0010:intel_power_domains_disable+0xc2/0xd0 [i915]
...
<4> [525.831483] Call Trace:
<4> [525.831484]  <TASK>
...
<4> [525.831943]  i915_driver_remove+0x4b/0x140 [i915]
<4> [525.832028]  i915_pci_remove+0x1e/0x40 [i915]
<4> [525.832099]  pci_device_remove+0x3e/0xb0
<4> [525.832103]  device_remove+0x40/0x80
<4> [525.832107]  device_release_driver_internal+0x215/0x280
...
<4> [525.947666] ------------[ cut here ]------------
<4> [525.947669] kobject: '(null)' (ffff88814f62a218): is not initialized, yet kobject_put() is being called.
<4> [525.947707] WARNING: CPU: 6 PID: 3440 at lib/kobject.c:734 kobject_put+0xe4/0x200
...
<4> [525.947875] RIP: 0010:kobject_put+0xe4/0x200
...
<4> [525.947909] Call Trace:
<4> [525.947911]  <TASK>
...
<4> [525.947963]  intel_gt_sysfs_unregister+0x25/0x40 [i915]
<4> [525.948133]  intel_gt_driver_unregister+0x14/0x80 [i915]
<4> [525.948291]  i915_driver_remove+0x6c/0x140 [i915]
<4> [525.948411]  i915_pci_remove+0x1e/0x40 [i915]
...
<4> [526.441186] ------------[ cut here ]------------
<4> [526.441191] kernfs: can not remove 'error', no directory
<4> [526.441211] WARNING: CPU: 1 PID: 3440 at fs/kernfs/dir.c:1684 kernfs_remove_by_name_ns+0xbc/0xc0
...
<4> [526.441536] RIP: 0010:kernfs_remove_by_name_ns+0xbc/0xc0
...
<4> [526.441578] Call Trace:
<4> [526.441581]  <TASK>
...
<4> [526.441686]  sysfs_remove_bin_file+0x17/0x30
<4> [526.441691]  i915_gpu_error_sysfs_teardown+0x1d/0x30 [i915]
<4> [526.442226]  i915_teardown_sysfs+0x1c/0x60 [i915]
<4> [526.442369]  i915_driver_remove+0x9d/0x140 [i915]
<4> [526.442473]  i915_pci_remove+0x1e/0x40 [i915]
...
<4> [526.685700] ------------[ cut here ]------------
<4> [526.685706] i915 0000:00:02.0: [drm] i915 raw-wakerefs=1 wakelocks=1 on cle
anup
<4> [526.685734] WARNING: CPU: 1 PID: 3440 at drivers/gpu/drm/i915/intel_runtime
_pm.c:443 intel_runtime_pm_driver_release+0x75/0x90 [i915]
...
<4> [526.686090] RIP: 0010:intel_runtime_pm_driver_release+0x75/0x90 [i915]
...
<4> [526.686294] Call Trace:
<4> [526.686296]  <TASK>
...
<4> [526.687025]  i915_driver_release+0x7e/0xb0 [i915]
<4> [526.687243]  drm_dev_put.part.0+0x47/0x90
<4> [526.687250]  devm_drm_dev_init_release+0x13/0x30
<4> [526.687255]  devm_action_release+0x12/0x30
<4> [526.687261]  release_nodes+0x3a/0x120
<4> [526.687268]  devres_release_all+0x97/0xe0
<4> [526.687277]  device_unbind_cleanup+0x12/0x80
<4> [526.687282]  device_release_driver_internal+0x23a/0x280
...

Introduce a flag that indicates device registration status, raise it on
device registration success.  When that flag is found not set while
unregistering the driver, jump over reverts of registration steps omitted
on device registration failure.

To make it possible, move i915_pmu_unregister(), a counterpart of
i915_pmu_unregister() that is called unconditionally before registering
the device, down so it is not skipped in any case.  Also, move
intel_pxp_fini(), that apparently needs to be called on driver remove
whether the device was registered successfully or not, down right below
the jump target (though it doesn't look like a step that belongs to driver
unregistration, but that's beyond the scope of this patch).

v2: Check in _unregister whether the drm_dev_register has succeeded and
    skip some of the _unregister() steps. (Andi)

Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12817
Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9820
Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10047
Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10131
Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10887
Cc: Chris Wilson <chris.p.wilson@linux.intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
Cc: Andi Shyti <andi.shyti@linux.intel.com>
Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_driver.c | 13 ++++++++++---
 drivers/gpu/drm/i915/i915_drv.h    |  2 ++
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index 91a7748f44926..d3b30c3690bbe 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -639,6 +639,8 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
 		return;
 	}
 
+	dev_priv->registered = true;
+
 	i915_debugfs_register(dev_priv);
 	i915_setup_sysfs(dev_priv);
 
@@ -673,6 +675,9 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
 	struct intel_gt *gt;
 	unsigned int i;
 
+	if (!dev_priv->registered)
+		goto not_registered;
+
 	i915_switcheroo_unregister(dev_priv);
 
 	intel_unregister_dsm_handler();
@@ -682,17 +687,19 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
 
 	intel_display_driver_unregister(display);
 
-	intel_pxp_fini(dev_priv);
-
 	for_each_gt(gt, dev_priv, i)
 		intel_gt_driver_unregister(gt);
 
 	i915_hwmon_unregister(dev_priv);
 
 	i915_perf_unregister(dev_priv);
-	i915_pmu_unregister(dev_priv);
 
 	i915_teardown_sysfs(dev_priv);
+
+not_registered:
+	intel_pxp_fini(dev_priv);
+	i915_pmu_unregister(dev_priv);
+
 	drm_dev_unplug(&dev_priv->drm);
 
 	i915_gem_driver_unregister(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ffc346379cc2c..27a23b1eb9de0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -347,6 +347,8 @@ struct drm_i915_private {
 	/* The TTM device structure. */
 	struct ttm_device bdev;
 
+	bool registered;
+
 	I915_SELFTEST_DECLARE(struct i915_selftest_stash selftest;)
 
 	/*
-- 
2.48.1

Re: [PATCH v2] drm/i915: Fix harmfull driver register/unregister assymetry

[Jani Nikula]

On Wed, 19 Feb 2025, Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote:
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ffc346379cc2c..27a23b1eb9de0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -347,6 +347,8 @@ struct drm_i915_private {
>  	/* The TTM device structure. */
>  	struct ttm_device bdev;
>  
> +	bool registered;
> +

*must* find another way.

BR,
Jani.


>  	I915_SELFTEST_DECLARE(struct i915_selftest_stash selftest;)
>  
>  	/*

-- 
Jani Nikula, Intel

✗ Fi.CI.CHECKPATCH: warning for drm/i915: Fix harmfull driver register/unregister assymetry (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Fix harmfull driver register/unregister assymetry (rev3)
URL   : https://patchwork.freedesktop.org/series/144436/
State : warning

== Summary ==

Error: dim checkpatch failed
5b808e0ffe66 drm/i915: Fix harmfull driver register/unregister assymetry
-:14: WARNING:COMMIT_LOG_LONG_LINE: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
#14: 
<3> [525.823143] i915 0000:00:02.0: [drm] *ERROR* Failed to register driver for

-:172: WARNING:MISSING_FIXES_TAG: The commit message has 'Call Trace:', perhaps it also needs a 'Fixes:' tag?

total: 0 errors, 2 warnings, 0 checks, 47 lines checked

✗ Fi.CI.SPARSE: warning for drm/i915: Fix harmfull driver register/unregister assymetry (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Fix harmfull driver register/unregister assymetry (rev3)
URL   : https://patchwork.freedesktop.org/series/144436/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

✗ i915.CI.BAT: failure for drm/i915: Fix harmfull driver register/unregister assymetry (rev3)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 11545 bytes --]

== Series Details ==

Series: drm/i915: Fix harmfull driver register/unregister assymetry (rev3)
URL   : https://patchwork.freedesktop.org/series/144436/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_16161 -> Patchwork_144436v3
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_144436v3 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_144436v3, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/index.html

Participating hosts (43 -> 43)
------------------------------

  Additional (1): bat-arlh-2 
  Missing    (1): fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_144436v3:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@gem_contexts:
    - bat-arls-6:         [PASS][1] -> [DMESG-WARN][2] +40 other tests dmesg-warn
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16161/bat-arls-6/igt@i915_selftest@live@gem_contexts.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-arls-6/igt@i915_selftest@live@gem_contexts.html
    - bat-arlh-3:         [PASS][3] -> [DMESG-WARN][4] +40 other tests dmesg-warn
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16161/bat-arlh-3/igt@i915_selftest@live@gem_contexts.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-arlh-3/igt@i915_selftest@live@gem_contexts.html

  * igt@i915_selftest@live@hangcheck:
    - bat-mtlp-9:         [PASS][5] -> [DMESG-WARN][6] +37 other tests dmesg-warn
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16161/bat-mtlp-9/igt@i915_selftest@live@hangcheck.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-mtlp-9/igt@i915_selftest@live@hangcheck.html

  * igt@i915_selftest@live@late_gt_pm:
    - bat-mtlp-9:         NOTRUN -> [DMESG-WARN][7]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-mtlp-9/igt@i915_selftest@live@late_gt_pm.html

  * igt@i915_selftest@live@sanitycheck:
    - bat-arls-5:         [PASS][8] -> [DMESG-WARN][9] +40 other tests dmesg-warn
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16161/bat-arls-5/igt@i915_selftest@live@sanitycheck.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-arls-5/igt@i915_selftest@live@sanitycheck.html
    - bat-arlh-2:         NOTRUN -> [DMESG-WARN][10] +40 other tests dmesg-warn
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-arlh-2/igt@i915_selftest@live@sanitycheck.html

  * igt@i915_selftest@live@slpc:
    - bat-mtlp-6:         [PASS][11] -> [DMESG-WARN][12] +40 other tests dmesg-warn
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16161/bat-mtlp-6/igt@i915_selftest@live@slpc.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-mtlp-6/igt@i915_selftest@live@slpc.html

  
Known issues
------------

  Here are the changes found in Patchwork_144436v3 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@debugfs_test@basic-hwmon:
    - bat-arlh-2:         NOTRUN -> [SKIP][13] ([i915#11346] / [i915#9318])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-arlh-2/igt@debugfs_test@basic-hwmon.html

  * igt@fbdev@eof:
    - bat-arlh-2:         NOTRUN -> [SKIP][14] ([i915#11345] / [i915#11346]) +3 other tests skip
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-arlh-2/igt@fbdev@eof.html

  * igt@fbdev@info:
    - bat-arlh-2:         NOTRUN -> [SKIP][15] ([i915#11346] / [i915#1849])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-arlh-2/igt@fbdev@info.html

  * igt@gem_lmem_swapping@basic:
    - bat-arlh-2:         NOTRUN -> [SKIP][16] ([i915#10213] / [i915#11346] / [i915#11671]) +3 other tests skip
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-arlh-2/igt@gem_lmem_swapping@basic.html

  * igt@gem_mmap@basic:
    - bat-arlh-2:         NOTRUN -> [SKIP][17] ([i915#11343] / [i915#11346])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-arlh-2/igt@gem_mmap@basic.html

  * igt@gem_render_tiled_blits@basic:
    - bat-arlh-2:         NOTRUN -> [SKIP][18] ([i915#10197] / [i915#10211] / [i915#11346] / [i915#11725])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-arlh-2/igt@gem_render_tiled_blits@basic.html

  * igt@gem_tiled_blits@basic:
    - bat-arlh-2:         NOTRUN -> [SKIP][19] ([i915#11346] / [i915#12637]) +4 other tests skip
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-arlh-2/igt@gem_tiled_blits@basic.html

  * igt@gem_tiled_pread_basic:
    - bat-arlh-2:         NOTRUN -> [SKIP][20] ([i915#10206] / [i915#11346] / [i915#11724])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-arlh-2/igt@gem_tiled_pread_basic.html

  * igt@i915_pm_rpm@module-reload:
    - bat-dg1-7:          [PASS][21] -> [FAIL][22] ([i915#13633])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16161/bat-dg1-7/igt@i915_pm_rpm@module-reload.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-dg1-7/igt@i915_pm_rpm@module-reload.html
    - bat-rpls-4:         [PASS][23] -> [FAIL][24] ([i915#13633])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16161/bat-rpls-4/igt@i915_pm_rpm@module-reload.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-rpls-4/igt@i915_pm_rpm@module-reload.html

  * igt@i915_pm_rps@basic-api:
    - bat-arlh-2:         NOTRUN -> [SKIP][25] ([i915#10209] / [i915#11346] / [i915#11681])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-arlh-2/igt@i915_pm_rps@basic-api.html

  * igt@i915_selftest@live:
    - bat-mtlp-9:         NOTRUN -> [DMESG-FAIL][26] ([i915#12061])
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-mtlp-9/igt@i915_selftest@live.html

  * igt@i915_selftest@live@workarounds:
    - bat-mtlp-9:         [PASS][27] -> [DMESG-FAIL][28] ([i915#12061])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16161/bat-mtlp-9/igt@i915_selftest@live@workarounds.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-mtlp-9/igt@i915_selftest@live@workarounds.html

  * igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
    - bat-arlh-2:         NOTRUN -> [SKIP][29] ([i915#10200] / [i915#11346] / [i915#11666] / [i915#12203])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-arlh-2/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html

  * igt@kms_addfb_basic@basic-x-tiled-legacy:
    - bat-arlh-2:         NOTRUN -> [SKIP][30] ([i915#10200] / [i915#11346] / [i915#11666]) +8 other tests skip
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-arlh-2/igt@kms_addfb_basic@basic-x-tiled-legacy.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence:
    - bat-dg2-11:         [PASS][31] -> [SKIP][32] ([i915#9197]) +3 other tests skip
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16161/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html

  * igt@kms_psr@psr-primary-page-flip:
    - bat-arlh-2:         NOTRUN -> [SKIP][33] ([i915#11346]) +32 other tests skip
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-arlh-2/igt@kms_psr@psr-primary-page-flip.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-arlh-2:         NOTRUN -> [SKIP][34] ([i915#10208] / [i915#11346] / [i915#8809])
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-arlh-2/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-read:
    - bat-arlh-2:         NOTRUN -> [SKIP][35] ([i915#10212] / [i915#11346] / [i915#11726])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-arlh-2/igt@prime_vgem@basic-fence-read.html

  * igt@prime_vgem@basic-read:
    - bat-arlh-2:         NOTRUN -> [SKIP][36] ([i915#10214] / [i915#11346] / [i915#11726])
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-arlh-2/igt@prime_vgem@basic-read.html

  * igt@prime_vgem@basic-write:
    - bat-arlh-2:         NOTRUN -> [SKIP][37] ([i915#10216] / [i915#11346] / [i915#11723])
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/bat-arlh-2/igt@prime_vgem@basic-write.html

  
  [i915#10197]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10197
  [i915#10200]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10200
  [i915#10206]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10206
  [i915#10208]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10208
  [i915#10209]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10209
  [i915#10211]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10211
  [i915#10212]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10212
  [i915#10213]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10213
  [i915#10214]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10214
  [i915#10216]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10216
  [i915#11343]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11343
  [i915#11345]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11345
  [i915#11346]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11346
  [i915#11666]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11666
  [i915#11671]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11671
  [i915#11681]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11681
  [i915#11723]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11723
  [i915#11724]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11724
  [i915#11725]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11725
  [i915#11726]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11726
  [i915#12061]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12061
  [i915#12203]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12203
  [i915#12637]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12637
  [i915#13633]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13633
  [i915#1849]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/1849
  [i915#8809]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/8809
  [i915#9197]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9197
  [i915#9318]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9318


Build changes
-------------

  * Linux: CI_DRM_16161 -> Patchwork_144436v3

  CI-20190529: 20190529
  CI_DRM_16161: e19eb43c82480789a93ce0c452e22bbd851b93c6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_8238: 7df82badac410a7ce2505df6064d44f0e094650c @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_144436v3: e19eb43c82480789a93ce0c452e22bbd851b93c6 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144436v3/index.html

[-- Attachment #2: Type: text/html, Size: 14256 bytes --]

Re: [PATCH v2] drm/i915: Fix harmfull driver register/unregister assymetry

[Sebastian Brzezinka]

Hi Janusz

On Wed Feb 19, 2025 at 6:37 PM UTC, Janusz Krzysztofik wrote:
> Starting with commit ec3e00b4ee27 ("drm/i915: stop registering if
> drm_dev_register() fails"), 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 of all
> those steps.  As a consequence, a number of kernel warnings that taint the
> kernel are triggered:
>
> <3> [525.823143] i915 0000:00:02.0: [drm] *ERROR* Failed to register driver for
> userspace access!
> ...
> <4> [525.831069] ------------[ cut here ]------------
> <4> [525.831071] i915 0000:00:02.0: [drm] drm_WARN_ON(power_domains->init_wakere
> f)
> <4> [525.831095] WARNING: CPU: 6 PID: 3440 at drivers/gpu/drm/i915/display/intel
> _display_power.c:2074 intel_power_domains_disable+0xc2/0xd0 [i915]
> ...
> <4> [525.831328] CPU: 6 UID: 0 PID: 3440 Comm: i915_module_loa Tainted: G     U
>             6.14.0-rc1-CI_DRM_16076-g7a632b6798b6+ #1
> ...
> <4> [525.831334] RIP: 0010:intel_power_domains_disable+0xc2/0xd0 [i915]
> ...
> <4> [525.831483] Call Trace:
> <4> [525.831484]  <TASK>
> ...
> <4> [525.831943]  i915_driver_remove+0x4b/0x140 [i915]
> <4> [525.832028]  i915_pci_remove+0x1e/0x40 [i915]
> <4> [525.832099]  pci_device_remove+0x3e/0xb0
> <4> [525.832103]  device_remove+0x40/0x80
> <4> [525.832107]  device_release_driver_internal+0x215/0x280
> ...
> <4> [525.947666] ------------[ cut here ]------------
> <4> [525.947669] kobject: '(null)' (ffff88814f62a218): is not initialized, yet kobject_put() is being called.
> <4> [525.947707] WARNING: CPU: 6 PID: 3440 at lib/kobject.c:734 kobject_put+0xe4/0x200
> ...
> <4> [525.947875] RIP: 0010:kobject_put+0xe4/0x200
> ...
> <4> [525.947909] Call Trace:
> <4> [525.947911]  <TASK>
> ...
> <4> [525.947963]  intel_gt_sysfs_unregister+0x25/0x40 [i915]
> <4> [525.948133]  intel_gt_driver_unregister+0x14/0x80 [i915]
> <4> [525.948291]  i915_driver_remove+0x6c/0x140 [i915]
> <4> [525.948411]  i915_pci_remove+0x1e/0x40 [i915]
> ...
> <4> [526.441186] ------------[ cut here ]------------
> <4> [526.441191] kernfs: can not remove 'error', no directory
> <4> [526.441211] WARNING: CPU: 1 PID: 3440 at fs/kernfs/dir.c:1684 kernfs_remove_by_name_ns+0xbc/0xc0
> ...
> <4> [526.441536] RIP: 0010:kernfs_remove_by_name_ns+0xbc/0xc0
> ...
> <4> [526.441578] Call Trace:
> <4> [526.441581]  <TASK>
> ...
> <4> [526.441686]  sysfs_remove_bin_file+0x17/0x30
> <4> [526.441691]  i915_gpu_error_sysfs_teardown+0x1d/0x30 [i915]
> <4> [526.442226]  i915_teardown_sysfs+0x1c/0x60 [i915]
> <4> [526.442369]  i915_driver_remove+0x9d/0x140 [i915]
> <4> [526.442473]  i915_pci_remove+0x1e/0x40 [i915]
> ...
> <4> [526.685700] ------------[ cut here ]------------
> <4> [526.685706] i915 0000:00:02.0: [drm] i915 raw-wakerefs=1 wakelocks=1 on cle
> anup
> <4> [526.685734] WARNING: CPU: 1 PID: 3440 at drivers/gpu/drm/i915/intel_runtime
> _pm.c:443 intel_runtime_pm_driver_release+0x75/0x90 [i915]
> ...
> <4> [526.686090] RIP: 0010:intel_runtime_pm_driver_release+0x75/0x90 [i915]
> ...
> <4> [526.686294] Call Trace:
> <4> [526.686296]  <TASK>
> ...
> <4> [526.687025]  i915_driver_release+0x7e/0xb0 [i915]
> <4> [526.687243]  drm_dev_put.part.0+0x47/0x90
> <4> [526.687250]  devm_drm_dev_init_release+0x13/0x30
> <4> [526.687255]  devm_action_release+0x12/0x30
> <4> [526.687261]  release_nodes+0x3a/0x120
> <4> [526.687268]  devres_release_all+0x97/0xe0
> <4> [526.687277]  device_unbind_cleanup+0x12/0x80
> <4> [526.687282]  device_release_driver_internal+0x23a/0x280
> ...
>
> Introduce a flag that indicates device registration status, raise it on
> device registration success.  When that flag is found not set while
> unregistering the driver, jump over reverts of registration steps omitted
> on device registration failure.
>
> To make it possible, move i915_pmu_unregister(), a counterpart of
> i915_pmu_unregister() that is called unconditionally before registering
> the device, down so it is not skipped in any case.  Also, move
> intel_pxp_fini(), that apparently needs to be called on driver remove
> whether the device was registered successfully or not, down right below
> the jump target (though it doesn't look like a step that belongs to driver
> unregistration, but that's beyond the scope of this patch).
>
> v2: Check in _unregister whether the drm_dev_register has succeeded and
>     skip some of the _unregister() steps. (Andi)
>
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12817
> Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9820
> Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10047
> Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10131
> Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10887
> Cc: Chris Wilson <chris.p.wilson@linux.intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_driver.c | 13 ++++++++++---
>  drivers/gpu/drm/i915/i915_drv.h    |  2 ++
>  2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index 91a7748f44926..d3b30c3690bbe 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -639,6 +639,8 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
>  		return;
>  	}
>  
> +	dev_priv->registered = true;
> +
>  	i915_debugfs_register(dev_priv);
>  	i915_setup_sysfs(dev_priv);
>  
> @@ -673,6 +675,9 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
>  	struct intel_gt *gt;
>  	unsigned int i;
>  
> +	if (!dev_priv->registered)
> +		goto not_registered;
> +
>  	i915_switcheroo_unregister(dev_priv);
>  
>  	intel_unregister_dsm_handler();
> @@ -682,17 +687,19 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
>  
>  	intel_display_driver_unregister(display);
>  
> -	intel_pxp_fini(dev_priv);
> -
>  	for_each_gt(gt, dev_priv, i)
>  		intel_gt_driver_unregister(gt);
>  
>  	i915_hwmon_unregister(dev_priv);
>  
>  	i915_perf_unregister(dev_priv);
> -	i915_pmu_unregister(dev_priv);
>  
>  	i915_teardown_sysfs(dev_priv);
> +
> +not_registered:
> +	intel_pxp_fini(dev_priv);
> +	i915_pmu_unregister(dev_priv);
> +
>  	drm_dev_unplug(&dev_priv->drm);
>  
>  	i915_gem_driver_unregister(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ffc346379cc2c..27a23b1eb9de0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -347,6 +347,8 @@ struct drm_i915_private {
>  	/* The TTM device structure. */
>  	struct ttm_device bdev;
>  
> +	bool registered;
Isn't `struct·drm_device` already has a registered field that could
be used, instead of introducing new variable. It's set in
`int·drm_dev_register(struct·drm_device·*dev,·unsigned·long·flags)`

-- 
Best regards,
Sebastian

Re: [PATCH v2] drm/i915: Fix harmfull driver register/unregister assymetry

[Janusz Krzysztofik]

Hi Sebastian,

On Thursday, 20 February 2025 17:10:57 CET Sebastian Brzezinka wrote:
> Hi Janusz
> 
> On Wed Feb 19, 2025 at 6:37 PM UTC, Janusz Krzysztofik wrote:
> > Starting with commit ec3e00b4ee27 ("drm/i915: stop registering if
> > drm_dev_register() fails"), 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 of all
> > those steps.  As a consequence, a number of kernel warnings that taint the
> > kernel are triggered:
> >
> > <3> [525.823143] i915 0000:00:02.0: [drm] *ERROR* Failed to register driver for
> > userspace access!
> > ...
> > <4> [525.831069] ------------[ cut here ]------------
> > <4> [525.831071] i915 0000:00:02.0: [drm] drm_WARN_ON(power_domains->init_wakere
> > f)
> > <4> [525.831095] WARNING: CPU: 6 PID: 3440 at drivers/gpu/drm/i915/display/intel
> > _display_power.c:2074 intel_power_domains_disable+0xc2/0xd0 [i915]
> > ...
> > <4> [525.831328] CPU: 6 UID: 0 PID: 3440 Comm: i915_module_loa Tainted: G     U
> >             6.14.0-rc1-CI_DRM_16076-g7a632b6798b6+ #1
> > ...
> > <4> [525.831334] RIP: 0010:intel_power_domains_disable+0xc2/0xd0 [i915]
> > ...
> > <4> [525.831483] Call Trace:
> > <4> [525.831484]  <TASK>
> > ...
> > <4> [525.831943]  i915_driver_remove+0x4b/0x140 [i915]
> > <4> [525.832028]  i915_pci_remove+0x1e/0x40 [i915]
> > <4> [525.832099]  pci_device_remove+0x3e/0xb0
> > <4> [525.832103]  device_remove+0x40/0x80
> > <4> [525.832107]  device_release_driver_internal+0x215/0x280
> > ...
> > <4> [525.947666] ------------[ cut here ]------------
> > <4> [525.947669] kobject: '(null)' (ffff88814f62a218): is not initialized, yet kobject_put() is being called.
> > <4> [525.947707] WARNING: CPU: 6 PID: 3440 at lib/kobject.c:734 kobject_put+0xe4/0x200
> > ...
> > <4> [525.947875] RIP: 0010:kobject_put+0xe4/0x200
> > ...
> > <4> [525.947909] Call Trace:
> > <4> [525.947911]  <TASK>
> > ...
> > <4> [525.947963]  intel_gt_sysfs_unregister+0x25/0x40 [i915]
> > <4> [525.948133]  intel_gt_driver_unregister+0x14/0x80 [i915]
> > <4> [525.948291]  i915_driver_remove+0x6c/0x140 [i915]
> > <4> [525.948411]  i915_pci_remove+0x1e/0x40 [i915]
> > ...
> > <4> [526.441186] ------------[ cut here ]------------
> > <4> [526.441191] kernfs: can not remove 'error', no directory
> > <4> [526.441211] WARNING: CPU: 1 PID: 3440 at fs/kernfs/dir.c:1684 kernfs_remove_by_name_ns+0xbc/0xc0
> > ...
> > <4> [526.441536] RIP: 0010:kernfs_remove_by_name_ns+0xbc/0xc0
> > ...
> > <4> [526.441578] Call Trace:
> > <4> [526.441581]  <TASK>
> > ...
> > <4> [526.441686]  sysfs_remove_bin_file+0x17/0x30
> > <4> [526.441691]  i915_gpu_error_sysfs_teardown+0x1d/0x30 [i915]
> > <4> [526.442226]  i915_teardown_sysfs+0x1c/0x60 [i915]
> > <4> [526.442369]  i915_driver_remove+0x9d/0x140 [i915]
> > <4> [526.442473]  i915_pci_remove+0x1e/0x40 [i915]
> > ...
> > <4> [526.685700] ------------[ cut here ]------------
> > <4> [526.685706] i915 0000:00:02.0: [drm] i915 raw-wakerefs=1 wakelocks=1 on cle
> > anup
> > <4> [526.685734] WARNING: CPU: 1 PID: 3440 at drivers/gpu/drm/i915/intel_runtime
> > _pm.c:443 intel_runtime_pm_driver_release+0x75/0x90 [i915]
> > ...
> > <4> [526.686090] RIP: 0010:intel_runtime_pm_driver_release+0x75/0x90 [i915]
> > ...
> > <4> [526.686294] Call Trace:
> > <4> [526.686296]  <TASK>
> > ...
> > <4> [526.687025]  i915_driver_release+0x7e/0xb0 [i915]
> > <4> [526.687243]  drm_dev_put.part.0+0x47/0x90
> > <4> [526.687250]  devm_drm_dev_init_release+0x13/0x30
> > <4> [526.687255]  devm_action_release+0x12/0x30
> > <4> [526.687261]  release_nodes+0x3a/0x120
> > <4> [526.687268]  devres_release_all+0x97/0xe0
> > <4> [526.687277]  device_unbind_cleanup+0x12/0x80
> > <4> [526.687282]  device_release_driver_internal+0x23a/0x280
> > ...
> >
> > Introduce a flag that indicates device registration status, raise it on
> > device registration success.  When that flag is found not set while
> > unregistering the driver, jump over reverts of registration steps omitted
> > on device registration failure.
> >
> > To make it possible, move i915_pmu_unregister(), a counterpart of
> > i915_pmu_unregister() that is called unconditionally before registering
> > the device, down so it is not skipped in any case.  Also, move
> > intel_pxp_fini(), that apparently needs to be called on driver remove
> > whether the device was registered successfully or not, down right below
> > the jump target (though it doesn't look like a step that belongs to driver
> > unregistration, but that's beyond the scope of this patch).
> >
> > v2: Check in _unregister whether the drm_dev_register has succeeded and
> >     skip some of the _unregister() steps. (Andi)
> >
> > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12817
> > Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9820
> > Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10047
> > Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10131
> > Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10887
> > Cc: Chris Wilson <chris.p.wilson@linux.intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
> > Cc: Andi Shyti <andi.shyti@linux.intel.com>
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_driver.c | 13 ++++++++++---
> >  drivers/gpu/drm/i915/i915_drv.h    |  2 ++
> >  2 files changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > index 91a7748f44926..d3b30c3690bbe 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -639,6 +639,8 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
> >  		return;
> >  	}
> >  
> > +	dev_priv->registered = true;
> > +
> >  	i915_debugfs_register(dev_priv);
> >  	i915_setup_sysfs(dev_priv);
> >  
> > @@ -673,6 +675,9 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
> >  	struct intel_gt *gt;
> >  	unsigned int i;
> >  
> > +	if (!dev_priv->registered)
> > +		goto not_registered;
> > +
> >  	i915_switcheroo_unregister(dev_priv);
> >  
> >  	intel_unregister_dsm_handler();
> > @@ -682,17 +687,19 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
> >  
> >  	intel_display_driver_unregister(display);
> >  
> > -	intel_pxp_fini(dev_priv);
> > -
> >  	for_each_gt(gt, dev_priv, i)
> >  		intel_gt_driver_unregister(gt);
> >  
> >  	i915_hwmon_unregister(dev_priv);
> >  
> >  	i915_perf_unregister(dev_priv);
> > -	i915_pmu_unregister(dev_priv);
> >  
> >  	i915_teardown_sysfs(dev_priv);
> > +
> > +not_registered:
> > +	intel_pxp_fini(dev_priv);
> > +	i915_pmu_unregister(dev_priv);
> > +
> >  	drm_dev_unplug(&dev_priv->drm);
> >  
> >  	i915_gem_driver_unregister(dev_priv);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index ffc346379cc2c..27a23b1eb9de0 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -347,6 +347,8 @@ struct drm_i915_private {
> >  	/* The TTM device structure. */
> >  	struct ttm_device bdev;
> >  
> > +	bool registered;
> Isn't `struct·drm_device` already has a registered field that could
> be used, instead of introducing new variable. It's set in
> `int·drm_dev_register(struct·drm_device·*dev,·unsigned·long·flags)`

I'm aware of that field, but I think that would be an abuse because it is 
described as "Internally used by drm_dev_register() and 
drm_connector_register()".  IOW, that field shouldn't be considered a 
stable component of DRM API, with well defined semantics, I believe.

I'm not sure what exactly Jani had on mind when rejecting my addition of a 
field to struct drm_i915_private, but maybe I'll try to move it up alongside 
do_release field in hope that reject was related to the note on not adding 
stuff to the tail of the structure considered a "dri1/ums dungeon".

But anyway, CI results show that the approach of skipping all unregister steps 
that correspond to steps skipped on unsuccessful register attempt needs much 
more work.  For me it looks like at least some of the functions called from 
i915_driver_unregister(), the purpose of which, I think, should be limited to 
tearing down external interfaces, also do other things, then simply skipping 
or even reordering them introduces new issues.  I think my initial approach of 
skipping selectively those steps or their sub-steps that actually trigger the 
kernel warnings, rejected before by Andi, was more cost-effective.

Thanks,
Janusz

> 
>