Re: [PATCH v3 3/3] rust: kernel: add SgTable abstraction

[Martin Rodriguez Reboredo]

On 6/10/23 07:49, Qingsong Chen wrote:
> SgTable is similar to `struct sg_table`, consisted of
> scatterlist entries, and could be chained with each other.
>
> Signed-off-by: Qingsong Chen <changxian.cqs@xxxxxxxxxxxx>
> ---
> [...]
> +impl<'a, const N: usize> SgTable<'a, N> {
> +    /// Construct a new initializer.
> +    ///
> +    /// # Errors
> +    ///
> +    /// The length of `entries` should exactly be the available size of [`SgTable<N>`],
> +    /// or else an error is returned.
> +    ///
> +    /// If the table is `chainable`, the available size is `N - 1`, because one entry
> +    /// should be reserved for chaining.
> +    pub fn new(
> +        entries: &'a [Pin<&mut [u8]>],
> +        chainable: bool,
> +    ) -> impl PinInit<SgTable<'a, N>, Error> {
> +        build_assert!(N > 0);
> +        // SAFETY: `slot` is valid while the closure is called, the `entries` are
> +        // pinned and valid.
> +        unsafe {
> +            init::pin_init_from_closure(move |slot: *mut Self| {
> +                let mut nr_entry = N;
> +                if chainable {
> +                    nr_entry -= 1;
> +                }
> +                if nr_entry == 0 || entries.len() != nr_entry {
> +                    return Err(EINVAL);
> +                }
> +
> +                for i in 0..nr_entry {
> +                    // `slot` contains uninit memory, avoid creating a reference.
> +                    let opaque = addr_of!((*slot).entries[i].opaque);
> +                    let sgl = Opaque::raw_get(opaque);
> +
> +                    bindings::sg_set_buf(sgl, entries[i].as_ptr() as _, entries[i].len() as _);
> +                    if i + 1 == nr_entry {
> +                        (*sgl).page_link |= bindings::SG_END as u64;
> +                        (*sgl).page_link &= !(bindings::SG_CHAIN as u64);
> +                    }
> +                }
> +                Ok(())
> +            })
> +        }
> +    }
> +
> +    /// Chain two [`SgTable`] together.
> +    ///
> +    /// Transfer the last entry of this table as a chainable pointer to the
> +    /// first entry of `sgt` SgTable.
> +    ///
> +    /// # Errors
> +    ///
> +    /// Return an error if this table is not chainable or has been chained.
> +    pub fn chain_sgt<const M: usize>(&mut self, sgt: Pin<&mut SgTable<'_, M>>) -> Result {
> +        if self.count() != N - 1 {
> +            return Err(EINVAL);
> +        }
> +        self.entries[N - 2].unmark_end();
> +
> +        // SAFETY: `sgt.entries` are initialized by the `new` constructor,
> +        // so it's valid.
> +        let next = unsafe { ScatterList::as_mut(sgt.entries[0].opaque.get()).unwrap() };
> +        self.entries[N - 1].chain_sgl(next);
> +        Ok(())
> +    }
> +
> +    /// Chain [`SgTable`] and [`ScatterList`] together.
> +    ///
> +    /// Transfer the last entry of this table as a chainable pointer to `sgl`
> +    /// scatterlist.
> +    ///
> +    /// # Errors
> +    ///
> +    /// Return an error if this table is not chainable or has been chained.
> +    pub fn chain_sgl(&mut self, sgl: Pin<&mut ScatterList<'_>>) -> Result {
> +        if self.count() != N - 1 {
> +            return Err(EINVAL);
> +        }
> +        self.entries[N - 2].unmark_end();
> +        self.entries[N - 1].chain_sgl(sgl);
> +        Ok(())
> +    }
> +
> +    /// Split the first table from chained scatterlist.
> +    ///
> +    /// # Errors
> +    ///
> +    /// Return an error if this table is not chainable or has not been chained.
> +    pub fn split(&mut self) -> Result {
> +        if !self.entries[N - 1].is_chain() {
> +            return Err(EINVAL);
> +        }
> +        self.entries[N - 2].mark_end();
> +        Ok(())
> +    }
> +
> [...]
> +}

I'd suggest being more explicit in the `# Errors` section by
mentioning that `EINVAL` is returned, but do as you prefer.

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@xxxxxxxxx>