Re: [PATCH v3 17/23] dmaengine: sdxi: Add completion status block API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Misplaced dma_rmb() and missing READ_ONCE()/endian conversion in sdxi_completion_signaled() allow stale DMA data reads.
- [High] Race condition in sdxi_completion_poll() can cause false timeouts if the thread is preempted.
- [High] Freeing the completion block after a timeout without aborting the hardware allows the device to write to freed DMA memory.
--

commit 7e5654d9158f4ac258b9deab508138cef7d4089d
Author: Nathan Lynch <nathan.lynch@amd.com>

dmaengine: sdxi: Add completion status block API

Introduce an API for managing completion status blocks. These are
DMA-coherent buffers that may be optionally attached to SDXI
descriptors to signal completion.

> diff --git a/drivers/dma/sdxi/completion.c b/drivers/dma/sdxi/completion.c
> new file mode 100644
> index 0000000000000..7ffd034b129b8
> --- /dev/null
> +++ b/drivers/dma/sdxi/completion.c
[ ... ]
> +int sdxi_completion_poll(const struct sdxi_completion *sc)
> +{
> +	unsigned long deadline = jiffies + msecs_to_jiffies(1000);
> +
> +	while (le64_to_cpu(READ_ONCE(sc->cst_blk->signal)) != 0) {
> +		if (time_after(jiffies, deadline))
> +			return -ETIMEDOUT;

[Severity: High]
Could this return a false timeout if the thread is preempted?

If the executing thread checks that the signal is not 0, gets preempted,
and then resumes after the deadline, time_after() will evaluate to true
and the function will return -ETIMEDOUT. Would it be safer to do a final
check of the hardware signal after detecting a timeout to ensure the
device didn't complete the operation during the preemption window?

[Severity: High]
When this loop times out and returns -ETIMEDOUT, the caller's cleanup
guard will free the completion block. Because the hardware operation is
never explicitly aborted, could the device eventually finish the stalled
descriptor and write to the freed DMA address?

This might corrupt memory if the DMA buffer has already been reallocated
from the pool for another descriptor or structure.

> +		cpu_relax();
> +	}
> +
> +	return sdxi_completion_errored(sc) ? -EIO : 0;
> +}
> +
> +bool sdxi_completion_signaled(const struct sdxi_completion *sc)
> +{
> +	dma_rmb();
> +	return (sc->cst_blk->signal == 0);

[Severity: High]
Should the read memory barrier be placed after the signal is successfully
verified as 0?

Executing dma_rmb() before checking the signal allows the CPU to
speculatively fetch payload memory before the DMA engine has finished
writing it.

Also, does reading sc->cst_blk->signal require READ_ONCE() and
le64_to_cpu() to prevent compiler optimization and endianness issues,
similar to how it is read in sdxi_completion_poll()?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605-sdxi-base-v3-0-4d38ca2bdffe@amd.com?part=17