Re: [PATCH v2 1/3] rust: kernel: add ScatterList abstraction

[Martin Rodriguez Reboredo]

On 6/2/23 07:18, Qingsong Chen wrote:
> Add a wrapper for `struct scatterlist`.
>
> Signed-off-by: Qingsong Chen <changxian.cqs@xxxxxxxxxxxx>
> ---
> [...]
> +/// Wrap the kernel's `struct scatterlist`.
> +///
> +/// According to the design of kernel's `struct sg_table`, the `page_link`
> +/// field of one `scatterlist` may contain a pointer to another. That is,
> +/// there could exist external pointers to it, making it necessary to pin
> +/// this struct.
> +///
> +/// # Invirants
> +///
> +/// All instances should be valid, either created by the `new` constructor
> +/// (see [`pin_init`]), or transmuted from raw pointers (which could be used
> +/// to reference a valid entry of `sg_table`).
> +///
> +/// # Examples
> +///
> +/// The following is some use cases of [`ScatterList`].
> +///
> +/// ```rust
> +/// use core::pin::pin;
> +/// # use kernel::error::Result;
> +/// # use kernel::scatterlist::ScatterList;
> +///
> +/// // Prepare memory buffers.
> +/// let buf0: Pin<&mut [u8]> = pin!([0u8; 512]);
> +/// let buf1: Pin<&mut [u8]> = pin!([1u8; 512]);
> +///
> +/// // Allocates an instance on stack.
> +/// kernel::stack_pin_init!(let foo = ScatterList::new(&buf0));
> +/// let mut foo: Pin<&mut ScatterList<'_>> = foo;
> +/// assert_eq!(foo.length(), 512);
> +///
> +/// // Alloccate an instance by Box::pin_init.
> +/// let bar: Pin<Box<ScatterList<'_>>> = Box::pin_init(ScatterList::new(&buf1))?;
> +/// assert_eq!(bar.length(), 512);
> +///
> +/// // Assert other attributes of a instance.
> +/// assert_eq!(foo.is_dma_bus_address(), false);
> +/// assert_eq!(foo.is_chain(), false);
> +/// assert_eq!(foo.is_last(), true);
> +/// assert_eq!(foo.count(), 1);
> +///
> +/// // Get a immutable reference to memory buffer.
> +/// assert_eq!(foo.get_buf(), [0u8; 512]);
> +///
> +/// // Reset the memory buffer.
> +/// foo.set_buf(&buf1);
> +/// assert_eq!(foo.get_buf(), [1u8; 512]);
> +///
> +/// // Get a mutable reference to memory buffer.
> +/// foo.get_mut_buf().fill(2);
> +/// assert_eq!(foo.get_buf(), [2u8; 512]);
> +/// ```

This comment has some typos, but luckily there's the `typos` tool [1]
out there to help us.

> +#[pin_data]
> +pub struct ScatterList<'a> {
> +    #[pin]
> +    opaque: Opaque<bindings::scatterlist>,
> +    _p: PhantomData<&'a mut bindings::scatterlist>,
> +}
> +
> +impl<'a> ScatterList<'a> {
> [...]
> +
> +    /// Obtain a pinned reference to an immutable scatterlist from a raw pointer.
> +    pub fn as_ref(ptr: *mut bindings::scatterlist) -> Option<Pin<&'a Self>> {
> +        // SAFETY: `sgl` is non-null and valid.
> +        NonNull::new(ptr).map(|sgl| Pin::new(unsafe { &*(sgl.as_ptr() as *const ScatterList<'_>) }))
> +    }
> +
> +    /// Obtain a pinned reference to a mutable scatterlist from a raw pointer.
> +    pub fn as_mut(ptr: *mut bindings::scatterlist) -> Option<Pin<&'a mut Self>> {
> +        // SAFETY: `sgl` is non-null and valid.
> +        NonNull::new(ptr)
> +            .map(|sgl| Pin::new(unsafe { &mut *(sgl.as_ptr() as *mut ScatterList<'_>) }))
> +    }

Please mark both of these as `unsafe` as you can still pass an invalid
pointer to them. Also, if there's no other user of those methods other
than this module then I'd suggest to make them private.

> +}
> +
> [...]

Link: https://github.com/crate-ci/typos [1]