[PATCH 0/3] drm, drm/xe: Protect against premature module unloads

[PATCH 0/3] drm, drm/xe: Protect against premature module unloads

[Thomas Hellström]

Driver and shared DRM helper code is increasingly relying on bare
drm_device references (drm_dev_get()/drm_dev_put()) to keep a device's
software state around, without also pairing that with a reference on
the owning kernel module. Xe itself does this in several places, and
so does drm_gpuvm for the lifetime of a GPU VM. None of these
references currently prevent the owning module from being unloaded
while they, or the teardown work they can still trigger, are
outstanding, meaning driver code can end up executing after its own
module's text has already been freed.

This series closes that gap for xe:

- Patch 1 adds core DRM infrastructure allowing a driver to wait for
  its outstanding device-release callbacks to finish before
  proceeding with module unload.

- Patch 2 makes xe use this infrastructure to hold up module unload
  until every xe_device instance has actually been released, rather
  than only until the module's own refcount happens to reach zero,
  with a diagnostic if this ends up taking an unexpectedly long time.

- Patch 3 fixes a related, previously unprotected case where the
  teardown of a GPU VM or its address space mappings can be deferred
  to run at an arbitrary later time, including after module unload has
  already completed.

Together, these changes ensure `rmmod xe` cannot free the module's
memory while any of its devices, or asynchronous work stemming from
them, might still be executing.

Thomas Hellström (3):
  drm: Provide a drm_dev_release_barrier() function to wait for device
    release callbacks
  drm/xe: Don't unload the driver until all drm devices are freed
  drm/xe: Route deferred xe_vma/xe_vm teardown off system_dfl_wq

 drivers/gpu/drm/drm_drv.c      | 56 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_device.c | 42 +++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_device.h |  2 ++
 drivers/gpu/drm/xe/xe_module.c | 24 +++++++++++++--
 drivers/gpu/drm/xe/xe_vm.c     |  5 +--
 include/drm/drm_drv.h          | 24 +++++++++++++++
 6 files changed, 148 insertions(+), 5 deletions(-)

-- 
2.55.0

[PATCH 1/3] drm: Provide a drm_dev_release_barrier() function to wait for device release callbacks

[Thomas Hellström]

Driver and drm helper code is increasingly relying on holding a bare
&drm_device reference (drm_dev_get()) without also holding a module
reference on the module that created the device.

For example drm_gpuvm_init() takes a drm_dev_get() reference on the
&drm_gpuvm's behalf with no accompanying module reference at all, and
drm_gpuvm_free() later calls the driver-supplied gpuvm->ops->vm_free()
callback, which lives in the driver module, before dropping that
reference. xe also takes bare drm_device references itself from
several asynchronous contexts, such as GuC submission fence workers,
EU stall, OA and PMU sampling code, relying only on those references
being dropped before the underlying xe_device, and eventually the
driver module, can be torn down.

If the module that created such a device is unloaded while one of
these bare references is still outstanding, and the corresponding
drm_dev_put() only completes after the module has already been
removed, the driver's ->release() callback, any drm managed release
actions, or a driver callback such as gpuvm->ops->vm_free(), all of
which live in that module's, by then freed, code, can end up being
invoked out of memory that no longer contains valid code.

Requiring every one of these bare drm_device references to also take a
module reference, as drm_pagemap does today via try_module_get(),
doesn't scale to shared helpers and driver-internal code with many
call sites, and is easy to get wrong.

Fix this properly by letting drivers keep a drm device-count and
ensure the module isn't unloaded until that count has dropped to zero
and until any release callback that had already started executing has
also finished executing.

To help with the latter, add a drm_dev_release_barrier() function.
The function ensures that any caller that has started executing
device release callbacks has also finished executing them.

Use SRCU for the implementation.

Rather than a single SRCU domain shared by all drivers, which would
mean drm_dev_release_barrier() could unnecessarily block a driver's
module unload on unrelated drivers' release callbacks, require each
driver that wants to use drm_dev_release_barrier() to supply its own
SRCU domain via a new &drm_driver.release_srcu field. Drivers should
define a static SRCU domain (DEFINE_STATIC_SRCU()) and set this field
to point at it. Leaving the field unset means @release and drm managed
release actions for that driver's devices simply aren't synchronized
against, and calling drm_dev_release_barrier() for such a driver is a
no-op that triggers a warning.

Since &drm_driver.release_srcu is a new field, every existing struct
drm_driver instance in the tree was scanned to confirm none of them
would leave it uninitialized with indeterminate content. All in-tree
instances have static storage duration (plain or const static
file-scope objects), or are members of a KUnit test fixture zeroed via
kunit_kzalloc(); no instance is stack-allocated or heap-allocated
without zeroing. Objects with static storage duration are guaranteed
by the C standard to have any member without an explicit initializer
zero-initialized, so the new field is reliably NULL, and
drm_dev_release() simply skips the SRCU critical section, for all
drivers that don't set it.

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Assisted-by: LLM
---
 drivers/gpu/drm/drm_drv.c | 56 +++++++++++++++++++++++++++++++++++++++
 include/drm/drm_drv.h     | 24 +++++++++++++++++
 2 files changed, 80 insertions(+)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 0cdc606af8d1..df32821e693a 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -921,18 +921,57 @@ EXPORT_SYMBOL(drm_dev_alloc);
 static void drm_dev_release(struct kref *ref)
 {
 	struct drm_device *dev = container_of(ref, struct drm_device, ref);
+	struct srcu_struct *srcu = dev->driver->release_srcu;
+	int idx = -1;
 
 	/* Just in case register/unregister was never called */
 	drm_debugfs_dev_fini(dev);
 
+	if (srcu)
+		idx = srcu_read_lock(srcu);
+
 	if (dev->driver->release)
 		dev->driver->release(dev);
 
 	drm_managed_release(dev);
 
+	if (srcu)
+		srcu_read_unlock(srcu, idx);
+
 	kfree(dev->managed.final_kfree);
 }
 
+/**
+ * drm_dev_release_barrier() - Ensure drm device release callbacks are finished
+ * @driver: driver whose release callbacks to wait for
+ *
+ * If a device release method or any of the drm managed release callbacks
+ * have been called for a device created with @driver, wait until all of
+ * them have finished executing. This function can be used to help determine
+ * whether it's safe to unload a driver module.
+ *
+ * Assume for example the driver maintains a device count which is decremented
+ * using a drmm callback or a device release callback. From a drm device
+ * lifetime POV, it's then safe to unload the driver when that device-count
+ * has reached zero and drm_dev_release_barrier() has been called.
+ *
+ * @driver must have &drm_driver.release_srcu set to a driver-owned
+ * &struct srcu_struct for this function to have anything to wait for.
+ *
+ * This function only waits for the &drm_driver.release callback and drm
+ * managed release actions to finish. It does not, by itself, guarantee that
+ * whoever called drm_dev_put() to drop the reference triggering that release
+ * has itself finished running. See drm_dev_put() for that invariant.
+ */
+void drm_dev_release_barrier(const struct drm_driver *driver)
+{
+	if (drm_WARN_ON_ONCE(NULL, !driver || !driver->release_srcu))
+		return;
+
+	synchronize_srcu(driver->release_srcu);
+}
+EXPORT_SYMBOL(drm_dev_release_barrier);
+
 /**
  * drm_dev_get - Take reference of a DRM device
  * @dev: device to take reference of or NULL
@@ -958,6 +997,23 @@ EXPORT_SYMBOL(drm_dev_get);
  *
  * This decreases the ref-count of @dev by one. The device is destroyed if the
  * ref-count drops to zero.
+ *
+ * If this call may drop the last reference, the calling code itself is
+ * responsible for ensuring it isn't unloaded (for example as part of a
+ * module) before this call has returned. This matters in particular for
+ * drivers relying on drm_dev_release_barrier() to determine when it's safe to
+ * unload, since that function only waits for the &drm_driver.release
+ * callback and drm managed release actions to finish, not for whoever calls
+ * drm_dev_put() to finish calling it.
+ *
+ * A common case is dropping the last reference from a deferred context, such
+ * as a workqueue item. In that case it's the responsibility of whoever
+ * queued that work item to guarantee it has run to completion before the
+ * module can unload, for example by draining a module-lifetime workqueue at
+ * module exit time. Holding a module reference only until the work item
+ * starts running is insufficient: that reference would already be dropped
+ * before this call runs, even though this call is what may still need the
+ * module's code to remain resident.
  */
 void drm_dev_put(struct drm_device *dev)
 {
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index b23830494ed4..30bb8727a896 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -48,6 +48,7 @@ struct drm_display_mode;
 struct drm_mode_create_dumb;
 struct drm_printer;
 struct sg_table;
+struct srcu_struct;
 
 /**
  * enum drm_driver_feature - feature flags
@@ -255,6 +256,28 @@ struct drm_driver {
 	 */
 	void (*release) (struct drm_device *);
 
+	/**
+	 * @release_srcu:
+	 *
+	 * Optional driver-owned SRCU domain used to synchronize completion of
+	 * the @release callback and drm managed release actions with
+	 * drm_dev_release_barrier().
+	 *
+	 * Left unset, @release and drm managed release actions for this
+	 * driver's devices aren't synchronized with drm_dev_release_barrier()
+	 * at all, and calling drm_dev_release_barrier() for this driver is a
+	 * no-op that triggers a warning.
+	 *
+	 * Drivers that want to use drm_dev_release_barrier(), for example to
+	 * help determine when it's safe to unload the driver module, should
+	 * define their own static SRCU domain (DEFINE_STATIC_SRCU()) and set
+	 * this field to point at it. Each driver should use its own domain,
+	 * so that drm_dev_release_barrier() only waits for that driver's own
+	 * release callbacks, rather than also for unrelated drivers sharing
+	 * the same domain.
+	 */
+	struct srcu_struct *release_srcu;
+
 	/**
 	 * @master_set:
 	 *
@@ -485,6 +508,7 @@ void drm_dev_exit(int idx);
 void drm_dev_unplug(struct drm_device *dev);
 int drm_dev_wedged_event(struct drm_device *dev, unsigned long method,
 			 struct drm_wedge_task_info *info);
+void drm_dev_release_barrier(const struct drm_driver *driver);
 
 /**
  * drm_dev_is_unplugged - is a DRM device unplugged
-- 
2.55.0

[PATCH 2/3] drm/xe: Don't unload the driver until all drm devices are freed

[Thomas Hellström]

xe, and shared helpers it uses such as drm_gpuvm, already take bare
drm_device references (drm_dev_get()) from several contexts, for
example GuC submission fence workers, EU stall, OA and PMU sampling
code, and drm_gpuvm's own vm object lifetime, without pairing them
with a module reference. Since xe_exit() is only invoked after the
module's own refcount has dropped to zero, none of these references
currently prevent `rmmod xe` from proceeding while they, or the
underlying xe_device release path they can trigger, are still
outstanding, i.e. driver code belonging to a module whose text is
being freed could still end up executing.

Close this gap by keeping a device-count and waiting for it to reach
zero at module unload, then waiting for any release callback that has
started executing to finish, using the drm_dev_release_barrier()
infrastructure introduced in the previous commit.

The wait for the device-count to reach zero at module unload is
unbounded and non-interruptible. Rather than blocking silently forever
if a reference is ever leaked, use wait_var_event_timeout() in a loop
and print a diagnostic every 10 seconds while devices remain, so a
stuck rmmod is at least observable instead of an indefinite, silent
hang.

Register a driver-private SRCU domain via the new
&drm_driver.release_srcu field on both xe drm_driver instances, and
pass the driver to drm_dev_release_barrier(). This keeps xe's wait for
its own release callbacks to complete from blocking on unrelated
drivers' release paths.

xe_device_exit() is added as a new module exit hook. Its entry in the
init_funcs[] table is placed between xe_destroy_wq_module_init and
xe_register_pci_driver, so that (exit functions run in reverse array
order) it executes after xe_unregister_pci_driver() has forced all
devices to unbind, but before xe_destroy_wq_module_exit() tears down
the module-lifetime xe_destroy_wq. This preserves xe_destroy_wq's
existing teardown ordering relative to xe_sched_job_module_exit() and
xe_hw_fence_module_exit(), which destroy kmem_caches that work drained
from xe_destroy_wq relies on, while ensuring xe_destroy_wq itself is
only torn down once xe_device_exit() has confirmed no more work can be
queued onto it.

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Assisted-by: LLM
---
 drivers/gpu/drm/xe/xe_device.c | 42 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_device.h |  2 ++
 drivers/gpu/drm/xe/xe_module.c | 18 ++++++++++++++-
 3 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index 205cb4e7f9e8..bfb1b482d83d 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -8,6 +8,7 @@
 #include <linux/aperture.h>
 #include <linux/delay.h>
 #include <linux/fault-inject.h>
+#include <linux/srcu.h>
 #include <linux/units.h>
 
 #include <drm/drm_client.h>
@@ -311,6 +312,13 @@ bool xe_is_xe_file(const struct file *file)
 	return file->f_op == &xe_driver_fops;
 }
 
+/*
+ * Driver-owned SRCU domain used to synchronize completion of driver release
+ * callbacks with drm_dev_release_barrier(), so that xe_device_exit() doesn't
+ * have to wait on unrelated drivers' release paths.
+ */
+DEFINE_STATIC_SRCU(xe_dev_release_srcu);
+
 static const struct drm_driver regular_driver = {
 	.driver_features =
 	    XE_DISPLAY_DRIVER_FEATURES |
@@ -335,6 +343,7 @@ static const struct drm_driver regular_driver = {
 	.major = DRIVER_MAJOR,
 	.minor = DRIVER_MINOR,
 	.patchlevel = DRIVER_PATCHLEVEL,
+	.release_srcu = &xe_dev_release_srcu,
 	XE_DISPLAY_DRIVER_OPS,
 };
 
@@ -357,6 +366,7 @@ static const struct drm_driver admin_only_driver = {
 	.major = DRIVER_MAJOR,
 	.minor = DRIVER_MINOR,
 	.patchlevel = DRIVER_PATCHLEVEL,
+	.release_srcu = &xe_dev_release_srcu,
 };
 
 /**
@@ -372,6 +382,9 @@ bool xe_device_is_admin_only(const struct xe_device *xe)
 }
 #endif
 
+/* Number of allocated struct xe_device */
+static atomic_t xe_device_count;
+
 static void xe_device_destroy(struct drm_device *dev, void *dummy)
 {
 	struct xe_device *xe = to_xe_device(dev);
@@ -391,6 +404,9 @@ static void xe_device_destroy(struct drm_device *dev, void *dummy)
 		destroy_workqueue(xe->destroy_wq);
 
 	ttm_device_fini(&xe->ttm);
+
+	if (atomic_dec_and_test(&xe_device_count))
+		wake_up_var(&xe_device_count);
 }
 
 /**
@@ -461,6 +477,7 @@ int xe_device_init_early(struct xe_device *xe)
 		return err;
 
 	xe_bo_dev_init(&xe->bo_device);
+	atomic_inc(&xe_device_count);
 	err = drmm_add_action_or_reset(&xe->drm, xe_device_destroy, NULL);
 	if (err)
 		return err;
@@ -1501,3 +1518,28 @@ struct xe_vm *xe_device_asid_to_vm(struct xe_device *xe, u32 asid)
 
 	return vm;
 }
+
+/**
+ * xe_device_exit() - Device subsystem exit function.
+ *
+ * Exit function to be called at module unload time.
+ */
+void xe_device_exit(void)
+{
+	/*
+	 * Wait for all devices to be freed. 20s is well above the typical
+	 * maximum dma_fence signalling time, so warn and keep waiting if
+	 * we're still not done by then, since it may indicate a leaked
+	 * xe_device reference is stalling module unload.
+	 */
+	if (!wait_var_event_timeout(&xe_device_count,
+				    !atomic_read(&xe_device_count),
+				    HZ * 20)) {
+		pr_warn("%s: Waiting for %d xe device(s) to be freed before unloading.\n",
+			DRIVER_NAME, atomic_read(&xe_device_count));
+		wait_var_event(&xe_device_count, !atomic_read(&xe_device_count));
+	}
+
+	/* Wait for any driver release callbacks to complete */
+	drm_dev_release_barrier(&regular_driver);
+}
diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
index 6d3d6d5eba29..83d6dafab53c 100644
--- a/drivers/gpu/drm/xe/xe_device.h
+++ b/drivers/gpu/drm/xe/xe_device.h
@@ -283,6 +283,8 @@ static inline bool xe_device_is_admin_only(const struct xe_device *xe)
 }
 #endif
 
+void xe_device_exit(void);
+
 /*
  * Occasionally it is seen that the G2H worker starts running after a delay of more than
  * a second even after being queued and activated by the Linux workqueue subsystem. This
diff --git a/drivers/gpu/drm/xe/xe_module.c b/drivers/gpu/drm/xe/xe_module.c
index 4bc28dfc1992..c61bd33546f2 100644
--- a/drivers/gpu/drm/xe/xe_module.c
+++ b/drivers/gpu/drm/xe/xe_module.c
@@ -12,7 +12,7 @@
 #include <drm/drm_module.h>
 
 #include "xe_defaults.h"
-#include "xe_device_types.h"
+#include "xe_device.h"
 #include "xe_drv.h"
 #include "xe_configfs.h"
 #include "xe_hw_fence.h"
@@ -162,6 +162,22 @@ static const struct init_funcs init_funcs[] = {
 		.init = xe_destroy_wq_module_init,
 		.exit = xe_destroy_wq_module_exit,
 	},
+	/*
+	 * xe_destroy_wq_module_exit() must run after xe_device_exit()
+	 * (below), since freeing a device can still queue work on
+	 * xe_destroy_wq that must be drained before the module can safely
+	 * unload. At the same time, xe_device_exit() must run after
+	 * xe_unregister_pci_driver() (below), and xe_destroy_wq_module_exit()
+	 * must run before xe_sched_job_module_exit() and
+	 * xe_hw_fence_module_exit() (above), whose kmem_caches are still used
+	 * by work drained from xe_destroy_wq. Exit functions run in reverse
+	 * array order, so this entry must sit between the
+	 * xe_destroy_wq_module_init entry above and the xe_register_pci_driver
+	 * entry below.
+	 */
+	{
+		.exit = xe_device_exit,
+	},
 	{
 		.init = xe_register_pci_driver,
 		.exit = xe_unregister_pci_driver,
-- 
2.55.0

[PATCH 3/3] drm/xe: Route deferred xe_vma/xe_vm teardown off system_dfl_wq

[Thomas Hellström]

xe_vma_destroy() can defer the final teardown of a struct xe_vma to a
dma_fence completion callback (vma_destroy_cb()), and xe_vm_free() (the
drm_gpuvm_ops.vm_free callback) always defers struct xe_vm teardown to
a work item, since destroying a VM needs to sleep. Both used to queue
their work on system_dfl_wq, a global, kernel-wide workqueue that xe
has no control over and never waits on during module unload.

drm_gpuvm_free() drops its drm_device reference immediately after
calling xe_vm_free(), without waiting for the deferred work to run.
The same applies one level down: whichever xe_vma or xe_vm reference
happens to be the last one can trigger this chain from a dma_fence
callback that may fire at an arbitrary time, including after the
owning file has already been closed and its own module reference
dropped. Since nothing tracks or waits for work queued on
system_dfl_wq, `rmmod xe` could succeed and free the module's text
while vma_destroy_work_func() or vm_destroy_work_func() is still
queued or running on it, jumping into freed code.

Fix this by queueing this work on xe_destroy_wq instead, the existing
module-lifetime workqueue already used for GuC exec queue teardown.
Unlike a per-device workqueue, this requires no dereference of a
struct xe_device that may already be gone by the time a deferred
callback fires, and unlike system_dfl_wq it is guaranteed to be
drained by xe_destroy_wq_module_exit() before the module is unloaded,
following the drm_pagemap_dev_hold()/unhold_work precedent of using a
workqueue that is waited on at module unload rather than a bare module
reference. The previous commit's reordering of xe_destroy_wq_exit()
to run after xe_device_exit() guarantees that xe_destroy_wq is only
torn down once the device-count has reached zero, i.e. after any
xe_vma or xe_vm whose teardown queues work here has already dropped
its drm_device reference and thus already queued that work.

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Assisted-by: LLM
---
 drivers/gpu/drm/xe/xe_module.c | 6 ++++--
 drivers/gpu/drm/xe/xe_vm.c     | 5 +++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_module.c b/drivers/gpu/drm/xe/xe_module.c
index c61bd33546f2..897724cb5cfb 100644
--- a/drivers/gpu/drm/xe/xe_module.c
+++ b/drivers/gpu/drm/xe/xe_module.c
@@ -114,8 +114,10 @@ static void xe_destroy_wq_module_exit(void)
  * xe_destroy_wq_queue() - Queue work on the destroy workqueue
  * @work: work item to queue
  *
- * The destroy workqueue has module lifetime and is used for GuC exec queue
- * teardown that can outlive a single xe_device. SVM pagemap destroy uses the
+ * The destroy workqueue has module lifetime, and is guaranteed to outlive
+ * any xe_device, and to be drained before the module is unloaded. It is used
+ * for GuC exec queue and xe_vm/xe_vma teardown that can be deferred past the
+ * lifetime of the xe_device that triggered it. SVM pagemap destroy uses the
  * per-device xe->destroy_wq instead.
  *
  * Return: %true if @work was queued, %false if it was already pending.
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 390da884c727..ee369e6c3b28 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -29,6 +29,7 @@
 #include "xe_exec_queue.h"
 #include "xe_gt.h"
 #include "xe_migrate.h"
+#include "xe_module.h"
 #include "xe_pagefault.h"
 #include "xe_pat.h"
 #include "xe_pm.h"
@@ -1249,7 +1250,7 @@ static void vma_destroy_cb(struct dma_fence *fence,
 	struct xe_vma *vma = container_of(cb, struct xe_vma, destroy_cb);
 
 	INIT_WORK(&vma->destroy_work, vma_destroy_work_func);
-	queue_work(system_dfl_wq, &vma->destroy_work);
+	xe_destroy_wq_queue(&vma->destroy_work);
 }
 
 static void xe_vm_assert_write_mode_or_garbage_collector(struct xe_vm *vm)
@@ -2059,7 +2060,7 @@ static void xe_vm_free(struct drm_gpuvm *gpuvm)
 	struct xe_vm *vm = container_of(gpuvm, struct xe_vm, gpuvm);
 
 	/* To destroy the VM we need to be able to sleep */
-	queue_work(system_dfl_wq, &vm->destroy_work);
+	xe_destroy_wq_queue(&vm->destroy_work);
 }
 
 struct xe_vm *xe_vm_lookup(struct xe_file *xef, u32 id)
-- 
2.55.0

Re: [PATCH 2/3] drm/xe: Don't unload the driver until all drm devices are freed

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] xe: logic mismatch in xe_device_exit wait causes indefinite uninterruptible hang
--

commit b47bb26ab24e9f99fb3dd76c67cadc55ddf082fa
Author: Thomas Hellström <thomas.hellstrom@linux.intel.com>

drm/xe: Don't unload the driver until all drm devices are freed

This commit ensures the driver module is not unloaded until all DRM device
references are dropped by tracking a device count. It implements a module
unload wait mechanism using drm_dev_release_barrier() and a timeout check to
prevent executing code from a freed module.

> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c

[ ... ]

> @@ -1501,3 +1518,28 @@ struct xe_vm *xe_device_asid_to_vm(struct xe_device *xe, u32 asid)
>  
>  	return vm;
>  }
> +
> +/**
> + * xe_device_exit() - Device subsystem exit function.
> + *
> + * Exit function to be called at module unload time.
> + */
> +void xe_device_exit(void)
> +{
> +	/*
> +	 * Wait for all devices to be freed. 20s is well above the typical
> +	 * maximum dma_fence signalling time, so warn and keep waiting if
> +	 * we're still not done by then, since it may indicate a leaked
> +	 * xe_device reference is stalling module unload.
> +	 */
> +	if (!wait_var_event_timeout(&xe_device_count,
> +				    !atomic_read(&xe_device_count),
> +				    HZ * 20)) {
> +		pr_warn("%s: Waiting for %d xe device(s) to be freed before unloading.\n",
> +			DRIVER_NAME, atomic_read(&xe_device_count));
> +		wait_var_event(&xe_device_count, !atomic_read(&xe_device_count));

[Severity: Medium]
The commit message mentions using a loop and printing a diagnostic every 10
seconds to avoid an indefinite, silent hang. However, this implementation
uses a single if block with a 20s timeout, followed by an unbounded
wait_var_event().

If an xe_device reference leak exists, won't this fallback to an indefinite
uninterruptible sleep and trigger the kernel's hung task watchdog?

> +	}
> +
> +	/* Wait for any driver release callbacks to complete */
> +	drm_dev_release_barrier(&regular_driver);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923140844.390822-1-thomas.hellstrom@linux.intel.com?part=2

Re: [PATCH 1/3] drm: Provide a drm_dev_release_barrier() function to wait for device release callbacks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm: NULL pointer dereference in drm_WARN_ON_ONCE on error path
--

commit 5d11ff8a335cc538f36d72b0645fb2d88466a4ce
Author: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Subject: drm: Provide a drm_dev_release_barrier() function to wait for device release callbacks

This commit adds a new drm_dev_release_barrier() function allowing drivers
to wait until all device release callbacks have finished executing. It uses 
a driver-owned SRCU domain to synchronize completion of release actions.

> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 0cdc606af8d18..df32821e693ac 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
[ ... ]
> +void drm_dev_release_barrier(const struct drm_driver *driver)
> +{
> +	if (drm_WARN_ON_ONCE(NULL, !driver || !driver->release_srcu))
> +		return;

[Severity: High]
Will passing NULL as the drm parameter to drm_WARN_ON_ONCE() cause a kernel 
panic here?

If the warning condition is met, drm_WARN_ON_ONCE() passes the NULL pointer 
to __drm_to_dev(), which returns NULL. This NULL pointer is then passed to 
dev_driver_string() and dev_name().

Because dev_driver_string() unconditionally dereferences the dev pointer via 
READ_ONCE(dev->driver), this transforms a non-fatal warning condition into an 
immediate kernel crash on the error path instead of gracefully returning.

Should a standard WARN_ON_ONCE() be used here instead since no drm_device is 
available in this context?

> +
> +	synchronize_srcu(driver->release_srcu);
> +}
> +EXPORT_SYMBOL(drm_dev_release_barrier);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923140844.390822-1-thomas.hellstrom@linux.intel.com?part=1