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

Abdiel Janulgue
2024-10-31T09:45:43Z

Hi,

On 28/10/2024 17:38, Daniel Almeida wrote:
>
> This patch is not a v2, so was anybody against using a raw pointer at some time?

The original idea behind this was to improve a little bit from raw
pointer manipulation when indexing the data. But yeah, for the proper v2
I now reintroduced the raw pointer but dynamically created slice from it
as you suggested below. :)

>
> Not sure why there’s ’static here. The lifetime of `cpu_addr` is the lifetime of the object.
>
> This is why keeping a pointer and building the slice as needed is actually a better approach, IMHO.
> That will correctly express the lifetime we want to enforce, i.e.:
>
> ```
> pub fn cpu(&’a self) -> &’a mut [T];
> ```
>
> Where ‘a is automatically filled in, of course.
>

>> + /// # Examples
>> + ///
>> + /// ```
>> + /// use kernel::device::Device;
>> + /// use kernel::dma::CoherentAllocation;
>> + ///
>> + /// # fn dox(dev: &Device) -> Result<()> {
>> + /// let c: CoherentAllocation<u64> = CoherentAllocation::alloc_coherent(dev, 4, GFP_KERNEL)?;
>
> Have you considered ZSTs? What happens if someone writes down:
>
> ```
> let c = CoherentAllocation<()> = …
> ```
>
> This doesn’t really make sense and should be forbidden.

Would restricting it to a primitive type make sense?

e.g:

pub struct CoherentAllocation<T: BitAnd + BitOr>

I'm not sure, but are there other ways to enforce that restriction?

>
>> 2.43.0
>>
>>
>
> Everything else looks good to me!

Thanks,
Abdiel

>
> — Daniel
>