On Mon, 2026-09-28 at 12:13 +0200, Danilo Krummrich wrote: > On Mon Sep 28, 2026 at 10:46 AM CEST, Thomas Hellström wrote: > > 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. > > Correct. > > > 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. > > There's two lifetimes we have to deal with in drivers: the lifetime > of (bus / > physical) device resources, which are managed by the driver and the > software > state that is represented through the class device to userspace (e.g. > file > handles). > > The former is bounded to the scope where the driver is bound to the > device and > the latter is unbounded and indeed depends on userspace. > > Either the subsystem or the driver has to decouple those lifetimes. > I.e. if the > driver is unbound it should clean up all GPUVMs as they represent the > GPU's > virtual address space and hence are associated with the hardware. > However, the > driver should not operated the hardware anymore after driver unbind.
I disagree here. At unbind time we decouple the HW and SW state, The device no longer uses it's pointers to the page-table so, for example VRAM page-tables can be torn down, system page-tables lose their dma- mappings, but in xe we don't tear down the page-table structure itself. If HW accesses are properly protected by drm_dev_enter() / drm_dev_exit(), Hw won't be accessed after unbind. > > So, in your case it seems that file lifetime and VM lifetime are > conflated > although they should be separate. I view the VM as software state, page-table pointers, dma-mappings and VRAM storage as HW state. It seems like what we're not agreeing on is where to separate those. I see no reason as to why we would complicate the driver to remove more than necessary at unbind time? > > We can't have userspace to decide when we drop device resources, such > as DMA > mappings, I/O memory mappings, etc. We don't (Unless we have bugs, and you may have stumbled on those below?) Those should be removed at unbind time. We should also revoke dma-buf mappings and SVM migrates all dma-buf mappings to system. > > > 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. > > Right, I forgot about them for a second. However, they are similar to > the > release() callbacks as in they are the wrong cleanup model for driver > private > structures. > > drmm is a great tool for common subsystem structures that lifetime > wise tie to > the drm_device. But it is the wrong lifetime model for stuff that is > used to > operate the device, as this should be torn down on device unbind. The current model used by xe (and amdgpu AFACT, that also ties vm lifetime to file lifetime) is to block all hardware access and dma at unbind time. The rest is state that doesn't necessarily need to be torn down at unbind time. I believe the current separation is mostly done with drm_dev_enter() / drm_dev_exit() and why should we enforce a change of that? I'd say the drivers should be free to release what's convenient. Also if drm_gpuvms are designed to not outlive the struct device, why do they need to take a struct drm_device reference in the first place, I mean I brought this problem up then and IIRC I think you argued the reference was needed and punted any problems it caused to the drivers? > > I had a quick look at Xe and found this for instance: > > drmm_add_action_or_reset(&xe->drm, control_fini_action, gt) > > control_fini_action() stops a worker that writes device registeres, > which must > not be done after driver unbind anymore. > > Now, there's two options, either after driver unbind this work is > never running > (which would be correct), but then this could have been > devm_add_action_or_reset(), or it does actually run after driver > unbind, but > this would violate the driver model. Agreed, Unless there is something protecting the hardware access after unplug in that control subsystem, that's a genuine bug, but that's separate from this discussion > > > > 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. > > I know it is not true in practice, but we are doing the wrong thing. > We are > conflating the unbounded userspace lifetime with the bounded lifetime > from the > driver model. I don't think we are. As long as all HW access is given up or blocked, we're fine. > > I also know that we can't fix this easily, but I want to create some > awareness, > especially when we introduce more band aid for the status quo, such > that we can > subsequently address the fundamental lifetime problems. > > > > 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. > > Well, but the workqueue itself has a bounded lifetime, which should > either be > driver unbind or module unload (when shared between driver > instances). The work > items themselves should ideally not extend beyond driver unbind, > because there > shouldn't be anything to do for the driver after unbind, because all > the > hardware should be torn down and not touched at this point anymore. Hardware isn't touched but I don't think there is any reason for cleanup tasks to stop executing at unbind time? > > I had another brief look at Xe and found this: > > drmm_add_action_or_reset(&xe->drm, ggtt_fini_early, ggtt) > > where ggtt_fini_early() destroys a workqueue. It also calls > > drm_mm_takedown(&ggtt->mm); > > which IIUC is the range allocator for the global GTT. (The hardware > is gone on > driver unbind (i.e. no more GGTT is available for the driver), so > there's > shouldn't be a need for this drm_mm to live longer than driver > unbind). Yes, this *can* probably be taken down at unbind time at the expense of subsystem and structure validity checking, but it doesn't have to. > > This model ties the lifetime of all shorter lived resources that are > bounded to > the driver unload scope to the unbounded lifetime of the drm_device > that is > controlled by userspace. > > I.e. the model is backwards and hence also extends your driver > structures and > callback entry points not only beyond driver unbind, but also > potentially beyond > module unload. > > If it would be done the other way around, tear down everything on > driver unload, > and then use default trampolines for userspace still trying to call > into the > driver (which is also what DMA fence does and other subsystem do), > all those > issues go away. But that is arguably something that's never going to happen, and even if it is, I think that's mostly orthogonal to code keeping references to drm_device. > > > > > - 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. > > GPUVM shouldn't be lifetime wise tied to a software state, it > represents > hardware resoruces. So then we can remove it's drm device reference? Why would it need to keep a reference to the software state guaranteed to outlive it? Regardless, I can remove all mentions of GPUVM in the documentation, but that doesn't remove the fact that keeping a reference to a struct drm_device without a module reference or a module presence guarantee is extremely fragile, and will be also in a model that removes a larger part of its software state at unbind time. IMO removing that fragility is a good thing. Thanks, Thomas
