Re: [PATCH 2/4] rust: scatterlist: Add type-state abstraction for sg_table

[Alice Ryhl <aliceryhl@google.com>]

On Mon, Aug 18, 2025 at 01:16:55PM +0200, Danilo Krummrich wrote:
> On Mon Aug 18, 2025 at 11:52 AM CEST, Alice Ryhl wrote:
> > On Fri, Aug 15, 2025 at 07:10:03PM +0200, Danilo Krummrich wrote:
> >> +/// A single entry in a scatter-gather list.
> >> +///
> >> +/// An `SGEntry` represents a single, physically contiguous segment of memory that has been mapped
> >> +/// for DMA.
> >> +///
> >> +/// Instances of this struct are obtained by iterating over an [`SGTable`]. Drivers do not create
> >> +/// or own [`SGEntry`] objects directly.
> >> +#[repr(transparent)]
> >> +pub struct SGEntry(Opaque<bindings::scatterlist>);
> >
> > Send/Sync?
> 
> I think SGEntry doesn't need it, but both would be valid.
> 
> For the other types, I simply forgot to impl Sync.

I would add them when it's valid to do so.

> >> +        // SAFETY: `self.as_raw()` is a valid pointer to a `struct scatterlist`.
> >> +        unsafe { bindings::sg_dma_address(self.as_raw()) }
> >> +    }
> >> +
> >> +    /// Returns the length of this SG entry in bytes.
> >> +    pub fn dma_len(&self) -> u32 {
> >
> > Is u32 really the right length type?
> 
> The C type uses unsigned int unfortunately, and SG entries larger than u32
> probably don't make sense.
> 
> Formally, bus addresses and hence this size, can exceed size_t. However, it
> obviously makes no sense for this to happen, so size_t would be a reasonable
> choice. ressource_size_t would be resonable too.

resource_size_t is what makes sense in my mind.

> >> +/// # Invariants
> >> +///
> >> +/// `sgt` is a valid pointer to a `struct sg_table` for the entire lifetime of an [`DmaMapSgt`].
> >
> > I think we probably want an invariant for why it's safe to call
> > dma_unmap_sgtable in Drop.
> 
> I assume you're suggesting that the invariant should say that the SG table is
> always mapped? And that we should add a safety requirement to DmaMapSgt::new()
> that the caller must guarantee that the SG table is never unmapped other than by
> dropping DmaMapSgt?
> 
> If so, that sounds reasonable.

Something like that, yeah.

> >> +struct DmaMapSgt {
> >> +    sgt: NonNull<bindings::sg_table>,
> >> +    dev: ARef<Device>,
> >> +    dir: dma::DataDirection,
> >> +}
> 
> <snip>
> 
> >> +impl RawSGTable {
> >> +    fn new(
> >> +        mut pages: KVec<*mut bindings::page>,
> >> +        size: usize,
> >> +        max_segment: u32,
> >> +        flags: alloc::Flags,
> >> +    ) -> impl PinInit<Self, Error> {
> >> +        try_pin_init!(Self {
> >> +            sgt <- Opaque::try_ffi_init(|slot: *mut bindings::sg_table| {
> >> +                // `sg_alloc_table_from_pages_segment()` expects at least one page, otherwise it
> >> +                // produces a NPE.
> >> +                if pages.is_empty() {
> >> +                    return Err(EINVAL);
> >> +                }
> >> +
> >> +                // SAFETY:
> >> +                // - `slot` is a valid pointer to uninitialized memory.
> >> +                // - As by the check above, `pages` is not empty.
> >> +                error::to_result(unsafe {
> >> +                    bindings::sg_alloc_table_from_pages_segment(
> >> +                        slot,
> >> +                        pages.as_mut_ptr(),
> >> +                        pages.len().try_into()?,
> >
> > The `pages` vector is dropped immediately after this call to
> > sg_alloc_table_from_pages_segment. Is that ok?
> 
> Yes, it's only needed during sg_alloc_table_from_pages_segment().
> 
> > If it's ok, then I would change `pages` to `&[*mut page]` so that the
> > caller can manage the allocation of the array.
> 
> We can immediately drop it after sg_alloc_table_from_pages_segmen(), so why do
> we want the caller to take care of the lifetime?

I think the API is easier to understand that way. With a slice, it's
clear that you only expect it to be alive for the duration of the
method. Whereas with a vector, it looks like you're going to keep the
allocation alive long-term, but that's not the case.

> >> +impl<P> Owned<P>
> >> +where
> >> +    for<'a> P: page::AsPageIter<Iter<'a> = VmallocPageIter<'a>> + 'static,
> >
> > If you specifically require the iterator type to be VmallocPageIter,
> > then I would hard-code that in the trait instead of specifying it here.
> 
> I do not follow, hard-code in which trait?

By hard-code, I meant that you refer to `VmallocPageIter` directly in
`trait AsPageIter`. But I don't think that's the correct solution.

> > But I think you just want `P: AsPageIter`.
> 
> Yeah, I thought for now it's probably good enough to require VmallocPageIter and
> revisit once we get more implementors of AsPageIter, but I think we can also do
> it right away.
> 
> I think we'd need a trait PageIterator, which implements page_count(), since we
> technically can't rely on Iterator::size_hint(). Though, in this case I think we
> can also just make AsPageIter::Iter: ExactSizeIterator?

I mean, ExactSizeIterator is not an unsafe trait, so it's allowed to lie
about the length. But it doesn't look like getting it wrong has any
problematic consequences here. At most we allocate too much in the
vector, or we have to reallocate it.

> >> +{
> >> +    fn new(
> >> +        dev: &Device<Bound>,
> >> +        mut pages: P,
> >> +        dir: dma::DataDirection,
> >> +        flags: alloc::Flags,
> >> +    ) -> Result<impl PinInit<Self, Error> + use<'_, P>> {
> >
> > We would probably want to move the logic into the initializer so that we
> > don't have the double Result here.
> 
> That'd be nice, but I think it's not possible.
> 
> We can't borrow from pages in the initializer closure while at the same time
> store pages with another initializer, can we?
> 
> Either way, it's not that big a deal I think, since this constructor is not
> exposed to the outside world. Which is also why it didn't bother me too much.

Ok. Shrug.

Alice