Hi Danilo, On Fri, Mar 28, 2025 at 03:50:13PM +0100, Danilo Krummrich wrote: > On Fri, Mar 28, 2025 at 03:28:04PM +0100, Maxime Ripard wrote: > > On Wed, Mar 26, 2025 at 11:46:54AM +0100, Danilo Krummrich wrote: > > > On Wed, Mar 26, 2025 at 10:24:43AM +0100, Maxime Ripard wrote: > > > > On Wed, Mar 26, 2025 at 12:54:32AM +0100, Danilo Krummrich wrote: > > > > > > drm_dev_unregister also have an hotplug-aware variant in > > > > drm_dev_unplug(). However, most devices are hotpluggable, even if only > > > > through sysfs. So drm_dev_unplug() is generally a better option. Should > > > > we use it here, and / or should we provide multiple options still? > > > > > > > > Another thing worth mentioning I think is that drm_dev_unplug() works by > > > > setting a flag, and drivers are expected to check that their access to > > > > device-bound resources (so registers mapping, clocks, regulators, etc.) > > > > are still there through drm_dev_enter/drm_dev_exit. It's pretty fragile > > > > overall, so I wonder if it's something we could abstract away in Rust. > > > > > > DRM should not have to take care of the lifetime of device resources of > > > the > > > parent device. This is the responsibility of the driver core abstractions. > > > > > > At least for the device resources we directly give out to drivers, it has > > > to be, > > > since otherwise it would mean that the driver core abstraction is unsound. > > > Drivers could otherwise extend the lifetime of device resources > > > arbitrarily. > > > > Sure. > > > > > For instance, I/O memory is only ever given out by bus abstractions > > > embedded in > > > a Devres container (e.g. Devres<pci::Bar>). Once the parent device is > > > unbound > > > the pci::Bar within the Devres container won't be accessible any more and > > > will > > > be dropped internally. So, that's nothing DRM has to take care of. > > > > > > However, there are cases where it's better to let subsystem abstractions > > > manage > > > the lifetime of device resources, (e.g. DMA mappings). The relevant thing > > > is, > > > that we never hand out device resources to a driver in a way that the > > > driver can > > > extend their lifetime arbitrarily. > > > > I was talking about the opposite though: when the driver is still around > > but the device (and its resources) aren't anymore. > > Well, that's what I was talking about too. :) > > > It makes total sense to tie the lifetime of the device resources to the > > device. However, the DRM device and driver will far outlive the device > > it was bound to so it needs to deal with that kind of degraded "the DRM > > driver can still be called by userspace, but it doesn't have a device > > anymore" scenario. That's what I was talking about. > > This scenario should be covered by the things I mentioned above. Let's take > the > I/O memory example. > > If you call into the DRM driver from userspace when the underlying bus device > has already been unbound, the DRM driver may still hold a Devres<pci::Bar> > instance. > > If the DRM driver then calls bar.try_access() (in order to subsequently call > writeX() or readX()) the try_access() call will fail, since the corresponding > PCI device has been unbound already. > > In other words the pci::Bar instance within the Devres container will be > dropped > (which includes unmapping the bar and releasing the resource region) when the > PCI device is unbound internally and the Devres container will restrict > subsequent accesses from drivers. > > It pretty much does the same thing as drm_dev_enter() / drm_dev_exit(), but > additionally prevents access to the (meanwhile) invalid pointer to the device > resource and ensures that the driver can't make device resources out-live > device > unbind. > > As mentioned above, the Devres container is just one example of how we prevent > such things; it depends on the exact scenario. In either case, I don't want > the > driver itself to be responsible to setup the corresponding guards, that would > make the corresponding abstractions unsound.
Thanks for the explanation :) It makes total sense now. Maxime
signature.asc
Description: PGP signature
