On Tue Sep 1, 2026 at 8:32 AM JST, Matteo Kloiber wrote:
> SGTable::new() caps segment length at dma_max_mapping_size() only, which
> limits the DMA mapping path (e.g. swiotlb), not the device itself. The
> per-device limit from dma_set_max_seg_size() is ignored, so contiguous
> page segments can be longer than the declared max segment size,
> potentially causing problems for future drivers that use this
> abstraction.
>
> nova-core declares an unlimited segment size, so this does not change
> its behavior.
I guess what this last paragraph wants to state is that no user is
affected by this patch? There is another subtle user though: the Rust
DMA sample.
In any case, this patch without patch 1 wouldn't break either of those
(only waste a bit more memory in SG entries with nova-core), so maybe we
can skip it.
>
> Fixes: 05aa6fb1c21d ("rust: scatterlist: Add abstraction for sg_table")
> Signed-off-by: Matteo Kloiber <[email protected]>
> ---
> rust/helpers/dma.c | 5 +++++
> rust/kernel/scatterlist.rs | 12 ++++++++++--
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/rust/helpers/dma.c b/rust/helpers/dma.c
> index 9fbeb507b08c..ff8f24dae9df 100644
> --- a/rust/helpers/dma.c
> +++ b/rust/helpers/dma.c
> @@ -49,3 +49,8 @@ __rust_helper void rust_helper_dma_set_max_seg_size(struct
> device *dev,
> {
> dma_set_max_seg_size(dev, size);
> }
> +
> +__rust_helper unsigned int rust_helper_dma_get_max_seg_size(struct device
> *dev)
> +{
> + return dma_get_max_seg_size(dev);
> +}
> diff --git a/rust/kernel/scatterlist.rs b/rust/kernel/scatterlist.rs
> index b83c468b5c63..d677dcbe7aac 100644
> --- a/rust/kernel/scatterlist.rs
> +++ b/rust/kernel/scatterlist.rs
> @@ -350,15 +350,23 @@ fn new(
> page_vec.push(page.as_ptr(), flags)?;
> }
>
> + // Cap segments at both the DMA mapping-path limit and the device's
> declared
> + // max segment size.
> + //
> // `dma_max_mapping_size` returns `size_t`, but
> `sg_alloc_table_from_pages_segment()` takes
> // an `unsigned int`.
> //
> // SAFETY: `dev.as_raw()` is a valid pointer to a `struct device`.
> - let max_segment = match unsafe {
> bindings::dma_max_mapping_size(dev.as_raw()) } {
> + let max_mapping = match unsafe {
> bindings::dma_max_mapping_size(dev.as_raw()) } {
> 0 => u32::MAX,
> - max_segment => u32::try_from(max_segment).unwrap_or(u32::MAX),
> + max_mapping => u32::try_from(max_mapping).unwrap_or(u32::MAX),
nit: if we rename this variable, let's name it `max_mapping_size` for
consistency.
> };
>
> + // SAFETY: `dev.as_raw()` is a valid pointer to a `struct device`.
> + let max_seg_size = unsafe {
> bindings::dma_get_max_seg_size(dev.as_raw()) };
> +
> + let max_segment = max_mapping.min(max_seg_size);
This looks like the correct thing to do indeed.
Reviewed-by: Alexandre Courbot <[email protected]>
(the following is a related thought, not a request for this patch)
It also means that without patch 1, nova-core would split the firmware
into hundreds of 64KB SG entries, which is not breaking but still
something we want to avoid. The correct fix is to make sure that
`dma_set_max_seg_size` is called by the driver, and while we are at it
we also want every driver to call `dma_set_mask_and_coherent`. Ideally
we would use the type system to make sure that both functions are called
before any DMA operation can take place (using a safe interface), but
I'm not quite sure yet how we can do this.
