On Tue Jul 8, 2025 at 10:44 PM CEST, Danilo Krummrich wrote: > On Tue Jul 8, 2025 at 11:48 AM CEST, Danilo Krummrich wrote: >> On Tue Jul 8, 2025 at 10:40 AM CEST, Danilo Krummrich wrote: >>> On Tue Jul 8, 2025 at 8:04 AM CEST, Alistair Popple wrote: >>>> Add bindings to allow setting the DMA masks for both a generic device >>>> and a PCI device. >>> >>> Nice coincidence, I was about to get back to this. I already implemented this in >>> a previous patch [1], but didn't apply it yet. >>> >>> I think the approach below is thought a bit too simple: >>> >>> (1) We want the DMA mask methods to be implemented by a trait in dma.rs. >>> Subsequently, the trait should only be implemented by bus devices where >>> the bus actually supports DMA. Allowing to set the DMA mask on any device >>> doesn't make sense. >> >> Forgot to mention, another reason for a trait is that we can also use it as a >> trait bound on dma::CoherentAllocation::new(), such that people can't pass >> arbitrary devices to dma::CoherentAllocation::new(), but only those that >> actually sit on a DMA capable bus. >> >>> >>> (2) We need to consider that with this we do no prevent >>> dma_set_coherent_mask() to concurrently with dma_alloc_coherent() (not >>> even if we'd add a new `Probe` device context). >>> >>> (2) is the main reason why I didn't follow up yet. So far I haven't found a nice >>> solution for a sound API that doesn't need unsafe. >>> >>> One thing I did consider was to have some kind of per device table (similar to >>> the device ID table) for drivers to specify the DMA mask already at compile >>> time. However, I'm pretty sure there are cases where the DMA mask has to derived >>> dynamically from probe(). >>> >>> I think I have to think a bit more about it. > > Ok, there are multiple things to consider in the context of (2) above. > > (a) We have to ensure that the dev->dma_mask pointer is properly initialized, > which happens when the corresponding bus device is initialized. This is > definitely the case when probe() is called, i.e. when the device is bound. > > So the solutions here is simple, we just implement the dma::Device trait > (which implements dma_set_mask() and dma_set_coherent_mask()) for > &Device<Bound>. > > (b) When dma_set_mask() or dma_set_coherent_mask() are called concurrently > with e.g. dma_alloc_coherent(), there is a data race with dev->dma_mask, > dev->coherent_dma_mask and dev->dma_skip_sync (also set by > dma_set_mask()). > > However, AFAICT, this does not necessarily make the Rust API unsafe in the > sense of Rust's requirements. I.e. a potential data race does not lead to > undefined behavior on the CPU side of things, but may result into a not > properly functioning device. Apparently, this is wrong, and it might indeed result in undefined behavior on the CPU side of things. :( > > It would be possible to declare dma_set_mask() and dma_set_coherent_mask() > Rust accessors as safe with the caveat that the device may not be able to > use the memory concurrently allocated with e.g. > dma::CoherentAllocation::new() properly. > > The alternative would be to make dma_set_mask() and > dma_set_coherent_mask() unsafe to begin with. > > I don't think there's a reasonable alternative given that the mask may be > derived on runtime in probe() by probing the device itself. > > I guess we could do something with type states and cookie values etc., but > that's unreasonable overhead for something that is clearly more a > theoretical than a practical concern. > > My conclusion is that we should just declare dma_set_mask() and > dma_set_coherent_mask() as safe functions (with proper documentation on > the pitfalls), given that the device is equally malfunctioning if they're > not called at all. > > @Alistair: If that is fine for you I'll pick up my old patches ([1] and related > ones) and re-send them. > > If there is more discussion on (b) I'm happy to follow up either here or in the > mentioned patches once I re-visited and re-sent them. > >>> [1] https://lore.kernel.org/all/20250317185345.2608976-7-abdiel.janulgue@xxxxxxxxx/
