Re: [PATCH 1/2] rust: kernel: add scatterlist wrapper

[Martin Rodriguez Reboredo]

On 5/30/23 03:48, Qingsong Chen wrote:
> Add abstractions for single `struct scatterlist` and chainable
> `struct sg_table`.
>
> Signed-off-by: Qingsong Chen <changxian.cqs@xxxxxxxxxxxx>
> ---
>   rust/bindings/bindings_helper.h |   1 +
>   rust/helpers.c                  |  14 +
>   rust/kernel/lib.rs              |   1 +
>   rust/kernel/scatterlist.rs      | 478 ++++++++++++++++++++++++++++++++

I think that 478 line changes are a bit too much for a commit, I'd
suggest to split this patch into smaller parts for easier reviews,
like one for ScatterList, another for SgTable, one for iterator
implementations, etc.

>   4 files changed, 494 insertions(+)
>   create mode 100644 rust/kernel/scatterlist.rs
>
> [...]
> +
> +/// A [`ScatterList`] table of fixed `N` entries.
> +///
> +/// According to the SG table design (form kernel ), the `page_link` field may contain
> +/// a pointer to the next sg table list, so this struct should be pinned. If the table
> +/// is chainable, the last entry will be reserved for chainning. The recommended way to

This comment has some orthographic, coherence (like "SG table",
"sg table") and grammatical, though, it's correct on its meaning.

> +/// create such instances is with the [`pin_init`].
This ([`pin_init`]) might point to nothing in `rustdoc`, though I'd
recommend to test it with `make rustdoc` and opening it in a browser
nonetheless.

> +///
> +/// # Examples
> +///
> +/// The following is examples of creating [`SgTable<N>`] instances> +///
> [...]> +
> +impl<'a, const N: usize> SgTable<'a, N> {
> [...]

This `impl` block has the same definition as the one above it. And
thus, wouldn't be worth to have the iter functions inside the same
block?

> +}
> +
> +/// Wrap the kernel's `struct scatterlist`.
> +///
> +/// According to the SG table design (from kernel), the `page_link` field may contain
> +/// a pointer to the next sg table list, so this struct should be pinned.
> +///
> +/// # Invirants
> +///
> +/// All instances are valid, either created by the `new` constructor (see [`pin_init`]),
> +/// or transmuted from raw pointers by the `as_ref` or `as_mut` function (usually used
> +/// to get an entry of [`SgTable`]).
> +///
> +/// # Examples
> +///
> +/// The following is examples of creating [`ScatterList`] instances.

Same as `SgTable` above.

> +///
> +/// ```rust
> +/// use core::pin::pin;
> +/// # use kernel::error::Result;
> +/// # use kernel::scatterlist::ScatterList;
> +///
> +/// // Prepare memory buffer.
> +/// let buf: Pin<&mut [u8]> = pin!([0u8; 512]);
> +///
> +/// // Allocates an instance on stack.
> +/// kernel::stack_pin_init!(let foo = ScatterList::new(&buf));
> +/// let foo: Pin<&mut ScatterList<'_>> = foo;
> +/// assert_eq!(foo.length(), 512);
> +/// assert_eq!(foo.count(), 1);
> +///
> +/// // Alloccate an instance by Box::pin_init.
> +/// let bar: Result<Pin<Box<ScatterList<'_>>>> = Box::pin_init(ScatterList::new(&buf));
> +/// assert_eq!(bar.as_ref().unwrap().length(), 512);
> +/// assert_eq!(bar.as_ref().unwrap().count(), 1);
> +/// ```
> +#[pin_data]
> +pub struct ScatterList<'a> {
> +    #[pin]
> +    opaque: Opaque<bindings::scatterlist>,
> +    _p: PhantomData<&'a mut bindings::scatterlist>,
> +}
> +
> +impl<'a> ScatterList<'a> {
> +    /// Construct a new initializer.
> +    pub fn new(buf: &'a Pin<&mut [u8]>) -> impl PinInit<ScatterList<'a>> {
> +        // SAFETY: `slot` is valid while the closure is called, the memory buffer is
> +        // pinned and valid.
> +        unsafe {
> +            init::pin_init_from_closure(move |slot: *mut Self| {
> +                (*slot).set_buf(buf);
> +                (*slot).mark_end();
> +                Ok(())
> +            })
> +        }
> +    }
> +
> +    /// Obtain [`Pin<&ScatterList>`] from raw pointer.

I'd rather say "Obtain a pinned reference to a scatter list from a raw
pointer.

> +    pub fn as_ref(entry: *mut bindings::scatterlist) -> Option<Pin<&'a Self>> {
> +        match entry.is_null() {
> +            true => None,
> +            // SAFETY: `entry` is non-null and valid.
> +            false => Some(Pin::new(unsafe { &*(entry as *const ScatterList<'_>) })),
> +        }

Another approach could be `NonNull::new` with `Option::map`, though if
you don't use it, no problem.

> +    }
> +
> +    /// Obtain [`Pin<&mut ScatterList>`] from raw pointer.

Same as `ScatterList::as_mut`.

> +    pub fn as_mut(entry: *mut bindings::scatterlist) -> Option<Pin<&'a mut Self>> {
> +        match entry.is_null() {
> +            true => None,
> +            // SAFETY: `entry` is non-null and valid.
> +            false => Some(Pin::new(unsafe { &mut *(entry as *mut ScatterList<'_>) })),
> +        }
> +    }
> +}
> +
> +impl ScatterList<'_> {
> [...]
> +
> +    /// Return the mapped DMA length.
> +    ///
> +    /// # Safety
> +    ///
> +    /// It is only valid after this scatterlist has been mapped to some bus address
> +    /// and then called `set_dma` method to setup it.
> +    #[cfg(CONFIG_NEED_SG_DMA_LENGTH)]
> +    pub fn dma_length(&self) -> usize {
> +        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
> +        unsafe { (*self.opaque.get()).dma_length as _ }
> +    }
> +
> +    /// Return the mapped DMA length.
> +    ///
> +    /// # Safety
> +    ///
> +    /// It is only valid after this scatterlist has been mapped to some bus address
> +    /// and then called `set_dma` method to setup it.
> +    #[cfg(not(CONFIG_NEED_SG_DMA_LENGTH))]
> +    pub fn dma_length(&self) -> usize {
> +        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
> +        unsafe { (*self.opaque.get()).length as _ }
> +    }
> +
> +    /// Setup the DMA address and length.
> +    #[cfg(CONFIG_NEED_SG_DMA_LENGTH)]
> +    pub fn set_dma(&mut self, addr: usize, len: usize) {
> +        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
> +        unsafe {
> +            (*self.opaque.get()).dma_address = addr as _;
> +            (*self.opaque.get()).dma_length = len as _;
> +        }
> +        self.dma_mark_bus_address();
> +    }
> +
> +    /// Setup the DMA address and length.
> +    #[cfg(not(CONFIG_NEED_SG_DMA_LENGTH))]
> +    pub fn set_dma(&mut self, addr: usize, len: usize) {
> +        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
> +        unsafe {
> +            (*self.opaque.get()).dma_address = addr as _;
> +            (*self.opaque.get()).length = len as _;
> +        }
> +        self.dma_mark_bus_address();
> +    }

Please avoid boilerplate by putting those `cfgs` inside the function.
For example you can turn this...

    /// Setup the DMA address and length.
    #[cfg(CONFIG_NEED_SG_DMA_LENGTH)]
    pub fn set_dma(&mut self, addr: usize, len: usize) {
        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
        unsafe {
            (*self.opaque.get()).dma_address = addr as _;
            (*self.opaque.get()).dma_length = len as _;
        }
        self.dma_mark_bus_address();
    }

    /// Setup the DMA address and length.
    #[cfg(CONFIG_NEED_SG_DMA_LENGTH)]
    pub fn set_dma(&mut self, addr: usize, len: usize) {
        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
        unsafe {
            (*self.opaque.get()).dma_address = addr as _;
            (*self.opaque.get()).length = len as _;
        }
        self.dma_mark_bus_address();
    }

...into this...

    /// Setup the DMA address and length.
    pub fn set_dma(&mut self, addr: usize, len: usize) {
        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
        #[cfg(CONFIG_NEED_SG_DMA_LENGTH)]
        unsafe {
            (*self.opaque.get()).dma_address = addr as _;
            (*self.opaque.get()).dma_length = len as _;
        }
        #[cfg(not(CONFIG_NEED_SG_DMA_LENGTH))]
        unsafe {
            (*self.opaque.get()).dma_address = addr as _;
            (*self.opaque.get()).length = len as _;
        }
        self.dma_mark_bus_address();
    }

> [...]
> +}
> [...]