Re: [PATCH v12 2/3] rust: add dma coherent allocator abstraction.

[Abdiel Janulgue]

On 24/02/2025 15:21, Alice Ryhl wrote:
> On Mon, Feb 24, 2025 at 12:50 PM Abdiel Janulgue
> <abdiel.janulgue@xxxxxxxxx> wrote:
> > Add a simple dma coherent allocator rust abstraction. Based on
> > Andreas Hindborg's dma abstractions from the rnvme driver, which
> > was also based on earlier work by Wedson Almeida Filho.
> >
> > Nacked-by: Christoph Hellwig <hch@xxxxxx>
> > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@xxxxxxxxx>
>
> > +    /// Create a duplicate of the `CoherentAllocation` object but prevent it from being dropped.
> > +    pub fn skip_drop(self) -> CoherentAllocation<T> {
> > +        let me = core::mem::ManuallyDrop::new(self);
> > +        Self {
> > +            // SAFETY: The refcount of `dev` will not be decremented because this doesn't actually
> > +            // duplicafe `ARef` and the use of `ManuallyDrop` forgets the originals.
> > +            dev: unsafe { core::ptr::read(&me.dev) },
> > +            dma_handle: me.dma_handle,
> > +            count: me.count,
> > +            cpu_addr: me.cpu_addr,
> > +            dma_attrs: me.dma_attrs,
> > +        }
> > +    }
>
> The skip_drop pattern requires the return value to use a different
> struct with the same fields, because otherwise you don't really skip
> the destructor. But I don't think you have the user for this method
> anymore so maybe just drop it.

Ah, yep. I agree.

> > +    /// Retrieve a single entry from the region with bounds checking. `offset` is in units of `T`,
> > +    /// not the number of bytes.
> > +    pub fn item_from_index(&self, offset: usize) -> Result<*mut T> {
> > +        if offset >= self.count {
> > +            return Err(EINVAL);
> > +        }
> > +        // SAFETY:
> > +        // - The pointer is valid due to type invariant on `CoherentAllocation`
> > +        // and we've just checked that the range and index is within bounds.
> > +        // - `offset` can't overflow since it is smaller than `self.count` and we've checked
> > +        // that `self.count` won't overflow early in the constructor.
> > +        Ok(unsafe { &mut *self.cpu_addr.add(offset) })
>
> The point of the dma_read/dma_write macros is to avoid creating
> references to the dma memory, so don't create a reference here.

This is embarrassing, I thought I had changed this already in the patch. 
My local tree had this already fixed. Anyways thanks for catching this!

/Abdiel