Message-ID: <77mjv2s3ebgilijr76e3c5ajm7iafqfjwjwqmz3j2zsslby4q3@x6qncv3hwqo4>
Date: Wed, 9 Jul 2025 11:41:47 +1000
From: Alistair Popple <apopple@...dia.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: rust-for-linux@...r.kernel.org, Bjorn Helgaas <bhelgaas@...gle.com>, 
	Krzysztof Wilczyński <kwilczynski@...nel.org>, Miguel Ojeda <ojeda@...nel.org>, 
	Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, Benno Lossin <lossin@...nel.org>, 
	Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, 
	Trevor Gross <tmgross@...ch.edu>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	"Rafael J. Wysocki" <rafael@...nel.org>, John Hubbard <jhubbard@...dia.com>, 
	Alexandre Courbot <acourbot@...dia.com>, linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] rust: Add dma_set_mask() and dma_set_coherent_mask()
 bindings

On Tue, Jul 08, 2025 at 10:44:53PM +0200, 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.
> 
>       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.

Fine with me, and I agree with your conclusion that these should just be
declared as safe functions with proper documentation. I think there are cases
where the mask is dynamic and in practice I think basically all drivers just set
the mask in their probe routine before doing any DMA allocations anyway so it
does seem more theoretical.

> 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.

Sounds good. My motivation for sending this was simply that we will need it to
start initialising the GPU in nova-core.
 
> >> [1] https://lore.kernel.org/all/20250317185345.2608976-7-abdiel.janulgue@gmail.com/

Powered by blists - more mailing lists