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

[Alexandre Courbot]

On Wed Jul 16, 2025 at 5:04 PM JST, Danilo Krummrich wrote:
> 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@xxxxxxxxxxxxx/
>
> 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.

I agree that a dedicated type limiting the possible values to inputs
that make sense would be nice.