Re: [PATCH v2 16/23] dmaengine: sdxi: Generic descriptor manipulation helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Data race and TOCTOU race in descriptor ownership validation defeats the safety check.
- [Medium] Severe performance degradation due to Read-Modify-Write (RMW) on Write-Combining (WC) DMA memory.
--

commit 54eb4223ce2b3ad6354bf153f2d7fc8aa53474e0
Author: Nathan Lynch <nathan.lynch@amd.com>

dmaengine: sdxi: Generic descriptor manipulation helpers

Introduce small helper functions for manipulating certain common properties
of descriptors after their operation-specific encoding has been performed but
before they are submitted.

> diff --git a/drivers/dma/sdxi/descriptor.h b/drivers/dma/sdxi/descriptor.h
> new file mode 100644
> index 0000000000000..c0f01b1be726d
> --- /dev/null
> +++ b/drivers/dma/sdxi/descriptor.h
[ ... ]
> +static inline void sdxi_desc_vl_expect(const struct sdxi_desc *desc, bool expected)
> +{
> +	u8 vl = FIELD_GET(SDXI_DSC_VL, le32_to_cpu(desc->opcode));
> +
> +	WARN_RATELIMIT(vl != expected, "expected vl=%u but got %u\n", expected, vl);
> +}

The descriptor ring is often mapped as DMA coherent memory, which on many
architectures is Write-Combining or uncached. Does reading desc->opcode
here and in the calling functions trigger synchronous, non-posted PCIe reads?

If these helpers are called directly on the DMA ring, this read-modify-write
pattern might cause multiple PCIe reads per descriptor and artificially cap
the DMA engine's throughput. Is there a way to redesign the API to avoid
reading from the ring, perhaps by returning the modified opcode or passing
it as state?

[ ... ]

> +static inline void sdxi_desc_set_fence(struct sdxi_desc *desc)
> +{
> +	u32 opcode = le32_to_cpu(desc->opcode);
> +
> +	sdxi_desc_vl_expect(desc, 0);

Can this double-read sequence lead to a time-of-check to time-of-use race?

If the driver erroneously calls this helper on an active descriptor (vl=1),
the first read of desc->opcode into the local opcode variable fetches vl=1.
If the hardware then completes the operation and writes vl=0 to desc->opcode
in memory, the second read inside sdxi_desc_vl_expect() fetches vl=0.

The validation check would then succeed, bypassing the WARN_RATELIMIT safety
net. The helper then writes back the stale local opcode (which still has
vl=1) to memory, silently re-submitting the completed descriptor.

Could desc->opcode be read exactly once using READ_ONCE(), and that local
value passed into sdxi_desc_vl_expect() to avoid this race?

> +	FIELD_MODIFY(SDXI_DSC_FE, &opcode, 1);
> +	desc->opcode = cpu_to_le32(opcode);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-sdxi-base-v2-0-889cfed17e3f@amd.com?part=16