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

[PATCH v2 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.

v2:
- Use plain WARN_ON_ONCE() instead of drm_WARN_ON_ONCE(NULL, ...) in
  drm_dev_release_barrier(), fixing a NULL pointer dereference on the
  warning path itself (patch 1, sashiko)
- Updated the commit message of patch 2 to describe the actual
  implementation (a single 20s wait_var_event_timeout() followed by
  one pr_warn() and an unbounded wait_var_event(), rather than a loop
  retrying with a diagnostic every 10s) and its uninterruptible-sleep
  tradeoff (sashiko)

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 v2 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.

v2:
- Use plain WARN_ON_ONCE() instead of drm_WARN_ON_ONCE(NULL, ...) in
  drm_dev_release_barrier(), since passing a NULL drm_device caused
  the warning path itself to dereference that NULL pointer inside
  dev_driver_string()/dev_name() (sashiko)

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..32a03170383c 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 (WARN_ON_ONCE(!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 v2 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() with a 20s
timeout, well above the typical maximum dma_fence signalling time, and
warn once if devices still remain by then, before falling back to an
unbounded wait_var_event() so a stuck rmmod is at least observable
instead of an indefinite, silent hang.

Note that if a reference genuinely leaks, this still ends up as an
indefinite uninterruptible sleep, which may eventually trip the
kernel's hung-task watchdog. The alternative would be for these bare
drm_device references to also take a module reference, which would
instead make the module unable to be unloaded unless all its devices are
manually unbound first. The wait-based approach is chosen here since it
keeps rmmod usable in the common case.

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.

v2:
- Updated commit message to describe the actual implementation (a
  single 20s wait_var_event_timeout() followed by one pr_warn() and an
  unbounded wait_var_event(), rather than a loop retrying with a
  diagnostic every 10s) and its uninterruptible-sleep tradeoff
  (sashiko)

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 v2 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 v2 1/3] drm: Provide a drm_dev_release_barrier() function to wait for device release callbacks

[Christian König]

On 9/24/26 10:04, Thomas Hellström wrote:
> 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.

That sounds like overkill to me.

I mean I can understand that you don't want to use RCU, but a single static SRCU for the DRM subsystem should pretty do the trick.

Regards,
Christian.

> 
> 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.
> 
> v2:
> - Use plain WARN_ON_ONCE() instead of drm_WARN_ON_ONCE(NULL, ...) in
>   drm_dev_release_barrier(), since passing a NULL drm_device caused
>   the warning path itself to dereference that NULL pointer inside
>   dev_driver_string()/dev_name() (sashiko)
> 
> 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..32a03170383c 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 (WARN_ON_ONCE(!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

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

[Thomas Hellström]

On Thu, 2026-09-24 at 13:19 +0200, Christian König wrote:
> On 9/24/26 10:04, Thomas Hellström wrote:
> > 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.
> 
> That sounds like overkill to me.
> 
> I mean I can understand that you don't want to use RCU, but a single
> static SRCU for the DRM subsystem should pretty do the trick.

Agreed. I'll change that in v3.

Thanks,
Thomas

> 
> Regards,
> Christian.
> 
> > 
> > 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.
> > 
> > v2:
> > - Use plain WARN_ON_ONCE() instead of drm_WARN_ON_ONCE(NULL, ...)
> > in
> >   drm_dev_release_barrier(), since passing a NULL drm_device caused
> >   the warning path itself to dereference that NULL pointer inside
> >   dev_driver_string()/dev_name() (sashiko)
> > 
> > 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..32a03170383c 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 (WARN_ON_ONCE(!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

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

[Matthew Brost]

On Thu, Sep 24, 2026 at 10:04:55AM +0200, Thomas Hellström wrote:
> 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>

Reviewed-by: Matthew Brost <matthew.brost@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 v2 2/3] drm/xe: Don't unload the driver until all drm devices are freed

[Matthew Brost]

On Thu, Sep 24, 2026 at 10:04:54AM +0200, Thomas Hellström wrote:
> 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() with a 20s
> timeout, well above the typical maximum dma_fence signalling time, and
> warn once if devices still remain by then, before falling back to an
> unbounded wait_var_event() so a stuck rmmod is at least observable
> instead of an indefinite, silent hang.
> 
> Note that if a reference genuinely leaks, this still ends up as an
> indefinite uninterruptible sleep, which may eventually trip the
> kernel's hung-task watchdog. The alternative would be for these bare
> drm_device references to also take a module reference, which would
> instead make the module unable to be unloaded unless all its devices are
> manually unbound first. The wait-based approach is chosen here since it
> keeps rmmod usable in the common case.
> 
> 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.
> 
> v2:
> - Updated commit message to describe the actual implementation (a
>   single 20s wait_var_event_timeout() followed by one pr_warn() and an
>   unbounded wait_var_event(), rather than a loop retrying with a
>   diagnostic every 10s) and its uninterruptible-sleep tradeoff
>   (sashiko)
> 
> 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,
>  };

I think everything above here will get moved to a DRM gloval srcu per
Christian's feedback?

Assuming the just dropped in favor of globlal
drm_dev_release_barrier(void), everything LGTM.

So feel free to carry this is in the next rev:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

>  
>  /**
> @@ -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
>