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

[Robin Murphy]

On 05/12/2024 4:41 pm, Daniel Almeida wrote:
> Hi Abdiel,
>
> > On 5 Dec 2024, at 11:25, Abdiel Janulgue <abdiel.janulgue@xxxxxxxxx> wrote:
> >
> > On 05/12/2024 15:07, Robin Murphy wrote:
> > > > +    pub fn alloc_coherent(
> > > > +        dev: &Device,
> > > > +        count: usize,
> > > > +        flags: kernel::alloc::Flags,
> > > Might it be worth adding at least a placeholder type and argument for "attrs" as well, so there's less churn involved in coming back and adding it later? Or is the intention to have a separate dma_alloc_attrs() constructor for such users?
> >
> > It certainly is possible to include attrs in the constructor using Option:
> > ```
> >     pub fn alloc_coherent(
> >         dev: &Device,
> >         count: usize,
> >         flags: kernel::alloc::Flags,
> >         attrs: Option<u64>,
> >     ) -> Result<CoherentAllocation<T>> {
> >
> > <cut>
> >         let ret = unsafe {
> >             bindings::dma_alloc_attrs(
> >                 dev.as_raw(),
> >                 size,
> >                 &mut dma_handle, flags.as_raw(),
> >                 attrs.unwrap_or(0),

Note that you'd also want to store the attrs in the CoherentAllocation 
itself, since they must be passed to the free and (eventually) mmap 
calls too.

> >             )
> >         };
> > <cut>
> >     }
> >
> > ```
> > So the signature of the constructor could be:
> >
> > let c = CoherentAllocation::alloc_coherent(dev, 4, GFP_KERNEL, None)?;
> >
> > Andreas, Miguel, Daniel, any thoughts?
> >
> > Regards,
> > Abdiel
>
> Instead of a raw u64, you should take inspiration from the work in the Alloc crate and elsewhere to define
> a proper `flags` type, which you can name `attr` in this case.

Indeed, some type-checking would be rather neat, especially given that 
there are actually different sets of DMA attributes which apply to the 
coherent vs. streaming APIs (and no, that isn't very well documented...)

Thanks,
Robin.