Re: [PATCH 2/5] rust: dma: add DMA addressing capabilities

["Danilo Krummrich" <dakr@kernel.org>]

On Wed Jul 16, 2025 at 5:18 AM CEST, Alexandre Courbot wrote:
> On Fri Jul 11, 2025 at 4:45 AM JST, Danilo Krummrich wrote:
>> @@ -18,7 +18,83 @@
>>  /// The [`dma::Device`](Device) trait should be implemented by bus specific device representations,
>>  /// where the underlying bus is DMA capable, such as [`pci::Device`](::kernel::pci::Device) or
>>  /// [`platform::Device`](::kernel::platform::Device).
>> -pub trait Device: AsRef<device::Device<Core>> {}
>> +pub trait Device: AsRef<device::Device<Core>> {
>> +    /// Set up the device's DMA streaming addressing capabilities.
>> +    ///
>> +    /// This method is usually called once from `probe()` as soon as the device capabilities are
>> +    /// known.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// This method must not be called concurrently with any DMA allocation or mapping primitives,
>> +    /// such as [`CoherentAllocation::alloc_attrs`].
>
> I'm a bit confused by the use of "concurrently" in this sentence. Do you
> mean that it must be called *before* any DMA allocation of mapping
> primitives? In this case, wouldn't it be clearer to make the order
> explicit?

Setting the mask before any DMA allocations might only be relevant from a
semantical point of view, but not safety wise.

We need to prevent concurrent accesses to dev->dma_mask and
dev->coherent_dma_mask.

>> +    unsafe fn dma_set_mask(&self, mask: u64) -> Result {
>
> Do we want to allow any u64 as a valid mask? If not, shall we restrict
> the accepted values by taking either the parameter to give to
> `dma_bit_mask`, or a bit range (similarly to Daniel's bitmask series
> [1], which it might make sense to leverage)?
>
> [1]
> https://lore.kernel.org/rust-for-linux/20250714-topics-tyr-genmask2-v9-1-9e6422cbadb6@collabora.com/

I think it would make sense to make dma_bit_mask() return a new type, e.g.
DmaMask and take this instead.

Taking the parameter dma_bit_mask() takes directly in dma_set_mask() etc. makes
sense, but changes the semantics of the mask parameter *subtly* compared to the
C versions, which I want to avoid.

Using the infrastructure in [1] doesn't seem to provide much value, since we
don't want a range [M..N], but [0..N], so we should rather only ask for N.