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 <[email protected]> > 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 <[email protected]> > > /** > @@ -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(®ular_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 >
