Re: [PATCH v2 2/8] rust: dma: add generalized container for types other than slices

[Andreas Hindborg]

"Gary Guo" <gary@xxxxxxxxxxx> writes:

> On Tue Mar 24, 2026 at 1:42 PM GMT, Andreas Hindborg wrote:
>> "Danilo Krummrich" <dakr@xxxxxxxxxx> writes:
>>
>>> +    /// Returns a reference to the data in the region.
>>>      ///
>>> -    /// # Examples
>>> +    /// # Safety
>>>      ///
>>> -    /// ```
>>> -    /// # use kernel::device::{Bound, Device};
>>> -    /// use kernel::dma::{attrs::*, CoherentAllocation};
>>> +    /// * Callers must ensure that the device does not read/write to/from memory while the returned
>>> +    ///   slice is live.
>>> +    /// * Callers must ensure that this call does not race with a write to the same region while
>>> +    ///   the returned slice is live.
>>> +    #[inline]
>>> +    pub unsafe fn as_ref(&self) -> &T {
>>> +        // SAFETY: per safety requirement.
>>
>> Is this enough? Don't you need to assert a valid bit pattern? I assume
>> you get this from `FromBytes`, but that bound is not on `T` in this impl
>> block.
>
> Concetually `Coherent<T>` is a container of `T` so it should always hold a valid
> bit pattern for the type. Perhaps this should be added to the type invariant.
>
> FromBytes is only needed at the constructor level as that's the only place where
> you're asserting that initial bit pattern is valid.

Makes sense to add the invariant and assert at construction time.

>
>
>>
>>> +        unsafe { &*self.as_ptr() }
>>> +    }
>>> +
>>> +    /// Returns a mutable reference to the data in the region.
>>>      ///
>>> -    /// # fn test(dev: &Device<Bound>) -> Result {
>>> -    /// let c: CoherentAllocation<u64> =
>>> -    ///     CoherentAllocation::alloc_attrs(dev, 4, GFP_KERNEL, DMA_ATTR_NO_WARN)?;
>>> -    /// # Ok::<(), Error>(()) }
>>> -    /// ```
>>> -    pub fn alloc_attrs(
>>> +    /// # Safety
>>> +    ///
>>> +    /// * Callers must ensure that the device does not read/write to/from memory while the returned
>>> +    ///   slice is live.
>>> +    /// * Callers must ensure that this call does not race with a read or write to the same region
>>> +    ///   while the returned slice is live.
>>> +    #[expect(clippy::mut_from_ref, reason = "unsafe to use API")]
>>> +    #[inline]
>>> +    pub unsafe fn as_mut(&self) -> &mut T {
>>> +        // SAFETY: per safety requirement.
>>
>> Same.
>>
>>> +        unsafe { &mut *self.as_mut_ptr() }
>>> +    }
>>> +
>>> +    /// Reads the value of `field` and ensures that its type is [`FromBytes`].
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// This must be called from the [`dma_read`] macro which ensures that the `field` pointer is
>>> +    /// validated beforehand.
>>> +    ///
>>> [...]
>>> +
>>> +impl<T: AsBytes + FromBytes> Coherent<T> {
>>> +    /// Allocates a region of `T` of coherent memory.
>>> +    #[expect(unused)]
>>> +    fn alloc_with_attrs(
>>>          dev: &device::Device<Bound>,
>>> -        count: usize,
>>>          gfp_flags: kernel::alloc::Flags,
>>>          dma_attrs: Attrs,
>>> -    ) -> Result<CoherentAllocation<T>> {
>>> -        build_assert!(
>>> -            core::mem::size_of::<T>() > 0,
>>> -            "It doesn't make sense for the allocated type to be a ZST"
>>> -        );
>>> +    ) -> Result<Self> {
>>> +        const {
>>> +            assert!(
>>> +                core::mem::size_of::<T>() > 0,
>>> +                "It doesn't make sense for the allocated type to be a ZST"
>>> +            );
>>> +        }
>>>
>>> -        let size = count
>>> -            .checked_mul(core::mem::size_of::<T>())
>>> -            .ok_or(EOVERFLOW)?;
>>>          let mut dma_handle = 0;
>>>          // SAFETY: Device pointer is guaranteed as valid by the type invariant on `Device`.
>>>          let addr = unsafe {
>>>              bindings::dma_alloc_attrs(
>>>                  dev.as_raw(),
>>> -                size,
>>> +                core::mem::size_of::<T>(),
>>>                  &mut dma_handle,
>>>                  gfp_flags.as_raw(),
>>>                  dma_attrs.as_raw(),
>>>              )
>>>          };
>>> -        let addr = NonNull::new(addr).ok_or(ENOMEM)?;
>>> +        let cpu_addr = NonNull::new(addr.cast()).ok_or(ENOMEM)?;
>>>          // INVARIANT:
>>> -        // - We just successfully allocated a coherent region which is accessible for
>>> -        //   `count` elements, hence the cpu address is valid. We also hold a refcounted reference
>>> -        //   to the device.
>>> -        // - The allocated `size` is equal to `size_of::<T> * count`.
>>> -        // - The allocated `size` fits into a `usize`.
>>> +        // - We just successfully allocated a coherent region which is adequately sized for `T`,
>>> +        //   hence the cpu address is valid.
>>
>> The invariant says "valid for the lifetime", do you need to argue for
>> the duration of the validity?
>
> It says "For the lifetime of an instance of ..." which is basically equal to
> saying nothing. I think that part should just be removed.

Sounds good.

>
>>
>>> +        // - We also hold a refcounted reference to the device.
>>
>> This does not relate to the second invariant of `Self`.
>
> The second invariant is about size, which is addressed in the first bullet
> point. This is about the third invariant, which the doc-comemnt just says "TODO"
> currently.

So it is not an invariant, and it should not be covered here.

I would suggest that if you use bullet list both in definition and
justification, please make sure the justification bullets match the
definition bullets. If you need to, you can make sub-lists by indenting.


Best regards,
Andreas Hindborg