Re: [PATCH 2/2] rust: scatterlist: honor the device's maximum segment size

[Matteo Kloiber <kernel@matt3o12.de>]

First of all, thanks for merging my first two patches and sorry about my late reply.

What I think Alex meant and what I agree with is that the new API should focus on
making semantic bugs harder, not to necessarily make it easier to write safer
drivers. That being said, I think we can do both with this change.

The key change here is that we introduce a structure that defines dma
parameters for a device. This structure deliberately has no default
implementation and new fields should be added even if they require changes for
all dma drivers.

```
pub struct Params {
    pub streaming_mask: DmaMask,
    pub coherent_mask: DmaMask,
    pub max_seg_size: NonZeroU32,
}
```

Suppose, we didn't have `max_seg_size` before and only discovered it later. We
would add it and the compiler would guide us to any drivers that will need to
consider an appropriate value for `max_seg_size`.

If rust was bigger in the kernel, this could be quite a bit of work, requiring
coordination with other maintainers, but I would call this a feature because it
actually requires everyone to at least consider what the right value is instead
of forgetting to set it.

The same is also true for new drivers. Authors do not have to be omniscient of
all dma related settings and instead just fill out the struct and already have
some important ones set.

If a parameter is only important for some drivers but those drivers should not
forget it, we could make it an Option so that drivers that really don't need
a parameter can set it to `None`, but they had to think about it at least.

I think, instead of discussing different possibilities, it might be easier to
discuss code directly. Taking Alex's example, I went ahead and wrote a small
LLM assisted PoC. I'm happy to share it here or in another thread, but here are
the main changes:

1. The PCI and platform probe callbacks receive a unique setup token:

```rust
    fn probe<'bound>(
        dev: &'bound Device<device::Core<'_>>,
        dma: dma::Setup<'bound>,
        id_info: Option<&'bound Self::IdInfo>,
    ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound;
```

The bus driver creates this token and drivers cannot obtain another one from
the device. 

2. `setup.configure(params)` consumes the token, applies the parameters through
   the C DMA setters, and returns a `dma::Handle<'bound>` on success.  This
   combines the setters and `finish()` from Alex's outline into one call.

```
pub fn configure(self, params: Params) -> Result<Handle<'bound>> {
    let dev = self.dev.as_raw();

    // SAFETY: `dev` is valid and its DMA parameters are initialized by the bus. The `Setup`
    // invariants exclude concurrent DMA operations and other parameter setters. Consuming
    // the unique token prevents reconfiguration after a handle has been issued.
    unsafe {
        to_result(bindings::dma_set_mask(dev, params.streaming_mask.value()))?;
        to_result(bindings::dma_set_coherent_mask(dev, params.coherent_mask.value()))?;
        bindings::dma_set_max_seg_size(dev, params.max_seg_size.get());
    }

    Ok(Handle { dev: self.dev })
}
```

3. The handle contains a bound device reference and is copyable, but provides
   no way to reconfigure DMA:

```rust
#[derive(Clone, Copy)]
pub struct Handle<'bound> {
  dev: &'bound device::Device<Bound>,
}
```

4. Allocation and mapping APIs now require a Handle. 

For example, scatter list now takes:

```
pub fn new<'a>(
  dma: dma::Handle<'a>,
  pages: P,
  dir: dma::DataDirection,
  flags: alloc::Flags,
) -> impl PinInit<Self, Error> + 'a {
```

The PoC uses Handle for coherent allocations, owned scatterlists, and DRM
scatterlist mappings.

5. Each pci/platform driver that uses DMA configures its own `Params`. Let's
   look at `samples/rust/rust_dma.rs`:

```rust
fn probe<'bound>(
    pdev: &'bound pci::Device<Core<'_>>,
    setup: dma::Setup<'bound>,
    _info: Option<&'bound Self::IdInfo>,
) -> impl PinInit<Self::Data<'bound>, Error> + 'bound {
    pin_init::pin_init_scope(move || {
        dev_info!(pdev, "Probe DMA test driver.\n");

        let mask = DmaMask::new::<64>();

        let dma = setup.configure(dma::Params {
            streaming_mask: mask,
            coherent_mask: mask,
            max_seg_size: const { NonZeroU32::new(u32::SZ_64K).unwrap() },
        })?;

        let ca: Coherent<'_, [MyStruct]> =
            Coherent::zeroed_slice(dma, TEST_VALUES.len(), GFP_KERNEL)?;

        for (i, value) in TEST_VALUES.into_iter().enumerate() {
            io_project!(ca, [panic: i]).copy_write(MyStruct::new(value.0, value.1));
        }

        let size = 4 * page::PAGE_SIZE;
        let pages = VVec::with_capacity(size, GFP_KERNEL)?;

        let sgt = SGTable::new(dma, pages, DataDirection::ToDevice, GFP_KERNEL);

        Ok(try_pin_init!(Self::Data {
            pdev,
            ca,
            sgt <- sgt,
        }))
    })
}
```

What I am not 100% sure about is how we should handle drivers that don't use
dma. Currently I just discarded the setup token like suggested, for example
`samples/rust/rust_driver_auxiliary.rs`:

```rust
impl pci::Driver for ParentDriver {
  fn probe<'bound>(
    pdev: &'bound pci::Device<Core<'_>>,
    _dma: kernel::dma::Setup<'bound>,
    _info: Option<&'bound Self::IdInfo>,
  ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound { ... }
}
```

But maybe it would make sense to introduce a C API such as: `debug_no_dma` and
have the bus call it unless configure was called first. This would then trigger
a debug warning if a marked rust device calls any C APIs that use DMA
regardless. But this could also very well be a worthwhile follow up.