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

[Abdiel Janulgue <abdiel.janulgue@gmail.com>]

Hi Daniel,

Thanks for the feedback!

On 06/11/2024 18:40, Daniel Almeida wrote:
> Calling `get/get_mut()` for each T seems a bit of too much overhead.
> 
> If T is u32, for example, I expect a driver to want to read or write
> a significant number of them at once. This should be common
> if you want to read or write raw bytes.
> 
> You should probably make the CPU-addressable region available
> to users after validating if the range asked is within bounds.
> 
> Luckily, cpu_buf() will already tie the lifetime of the slice to &self,
> so they cannot touch that memory if the mapping is gone.

You mean the read/write helpers could be removed and we expose the 
cpu_buf() helper instead that takes a valid range, returns a checked 
slice and let the user have the flexibility in implementing read/writes 
themselves. Is this what you had in mind?

> 
> The fact that reads/writes will copy the memory is also a bit problematic
> given that you can offer access to the underlying buffer without copying
> anything. Also, the bound on `FromBytes/AsBytes` is, for now, a de facto
> bound on `Copy`, since it is only implemented for types which themselves
> implement Copy, i.e.:
> 
> ```
> // SAFETY: All bit patterns are acceptable values of the types below.
> unsafe impl FromBytes for u8 {}
> unsafe impl FromBytes for u16 {}
> unsafe impl FromBytes for u32 {}
> unsafe impl FromBytes for u64 {}
> unsafe impl FromBytes for usize {}
> unsafe impl FromBytes for i8 {}
> unsafe impl FromBytes for i16 {}
> unsafe impl FromBytes for i32 {}
> unsafe impl FromBytes for i64 {}
> unsafe impl FromBytes for isize {}
> // SAFETY: If all bit patterns are acceptable for individual values in an array, then all bit
> // patterns are also acceptable for arrays of that type.
> unsafe impl<T: FromBytes> FromBytes for [T] {}
> unsafe impl<T: FromBytes, const N: usize> FromBytes for [T; N] {}
> ```
> 
> Also, I think FromBytes is a bit restrictive here. In a lot of places, drivers
> expect to be able to cast the mapping to a given struct whose definition
> was sourced from some technical document or vendor code somewhere.
> 
> For example, here is some code for the H.264 decoder in the `rkvdec`
> driver:
> 
> ```
> 	struct rkvdec_h264_priv_tbl *priv_tbl;
> 
> <cut>
> 
> 	priv_tbl = dma_alloc_coherent(rkvdec->dev, sizeof(*priv_tbl),
> 				      &h264_ctx->priv_tbl.dma, GFP_KERNEL);
> ```

We could just remove the FromBytes/AsBytes bound and let
the constructor handle the ZST restriction there (like in the current 
submission)? If that is the case, there would be no need to wrestle with 
this issue then?

/Abdiel