On Fri, 2026-09-25 at 18:18 +0200, Danilo Krummrich wrote: > On Fri Sep 25, 2026 at 3:33 PM CEST, Thomas Hellström wrote: > > 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. > > Since you mention DRM GPUVM in a couple of places, how can this ever > happen? It > wouldn't make sense to keep a VM alive beyond driver unbind. I.e. it > can't make > its drm_device reference count reach module unload in the first > place.
There seems to be a bit of misunderstanding here. Driver unbind removes the struct device from the driver, triggers device unplug, and eventually the devres release actions. IIRC the last devres action removes a *single reference* on the struct drm_device. Hence if there exists other reference holders on the struct drm_device (open files, exported dma-bufs, exported drm_pagemaps as an example), the drm_device will survive the driver unbind. So will open files and thus drm_gpuvms until user-space decides to remove them. Files, dma-bufs and drm_pagemaps all hold a driver module reference until they have successfully released the drm_device. The requirement is "If a drm_device reference is held, a module reference of the driver providing the drm_device must also be held, or if it's held by the driver itself, it must ensure at driver unload time that any drm_device references it holds are released and drmm release callbacks have finished executing." What this series in effect does is to change this to to "The driver won't unload until all drm_device references are gone, and all drmm release callbacks have finished executing." I agree that the use of drm_gpuvm in the documentation is a bit unfair. Since the code calling drm_dev_get() and drm_dev_put() is intended to be called from the driver, the reference in effect becomes the driver's responsibility, but if someone would, in the future change that so that those references are put from a worker from within the driver or even within drm_gpuvm itself, things would break. If a future code reviewer, developer or AI agent knows about the new drm_device reference guarantee, then that will lessen the review scope and code will become more rubost. > > Besides that, can you please remind me whether there are any other > reasons than > the release() callback why a DRM device must not outlive module > unload? The drmm release callbacks. > > I don't think the correct solution is to constrain module unload. The > release() > callback shouldn't really do anything other than free the memory of > the > drm_device allocation. All other resources a driver may have should > be released > on driver unbind. That is not true. See above. The alternative would be, as mentioned in the commit message of patch 2, IIRC that each drm_device itself hold a module reference of the creating module. But then you wouldn't be able to execute rmmod with devices alive, You would have to manually unbind all devices first. > > There may be shared resources, such as e.g. a common workqueue, but > those are > module level things that have nothing to do with the DRM device. I don't think that is correct either. Take a look at Matt Brost reply to patch 3 there where he points out that the drm device (a base class of the xe device) is acually referenced in a work item after the struct drm_device reference is put. (There is a workqueue naming confusion in xe, but I do believe that patch needs a fix). With the poposed series in place a simple fix would be to hold a drm_device reference across the workqueue item. The unload process would then block until all devices are unreferenced, and then again at destroy_worqueue time waiting for the work item epilogue to finish executing. > > I am aware that a few drivers implement release(), but TBH it looks > pretty > broken. qxl_drm_release() is a great example, and it already calls it > out > itself. drm_release is not of major interest. > > > 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. > > Please see above; I also wonder why Xe cares in the first place. Xe > doesn't > implement release(), no? See above. > > > - 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. > > Huh? GPUVM tracks the GPU's VA space mappings, but after driver > unbind there's > no access to the GPU to manage anything anymore. How can this even > work? > > > As previously mentioned, software device state may well outlive a driver unbind. This is all about its cleanup. Thanks, Thomas > > Thanks, > Danilo
