Re: [PATCH v3 01/24] dmaengine: dw-edma: Add dw_edma_core_ll_cur_idx() to get current LL entry index

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Torn read of 64-bit hardware register 'llp' leads to false -EINVAL when the DMA is running.
--

--- Patch [1]: [PATCH v3 01/24] dmaengine: dw-edma: Add dw_edma_core_ll_cur_idx() to get current LL entry index ---
commit 8302239cbe55e73285af81992cde8e677977d567
Author: Frank Li <[email protected]>

dmaengine: dw-edma: Add dw_edma_core_ll_cur_idx() to get current LL entry index

Add dw_edma_core_ll_cur_idx() to get the current LL entry index. This
prepares the driver for dynamically adding DMA requests while the engine
is running.

> diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
> @@ -616,6 +616,24 @@ static void dw_edma_v0_core_ch_doorbell(struct dw_edma_chan *chan)

[ ... ]

> +static int dw_edma_v0_core_ll_cur_idx(struct dw_edma_chan *chan)
> +{
> +	u32 base, val;
> +
> +	val = GET_CH_32(chan->dw, chan->dir, chan->id, llp.lsb);
> +	base = lower_32_bits(dw_edma_core_get_ll_paddr(chan));
> +
> +	/*
> +	 * An all-zero pointer means that the channel context is unavailable,
> +	 * so an LL region starting at DMA address zero is not supported.
> +	 */
> +	if (!val && !GET_CH_32(chan->dw, chan->dir, chan->id, llp.msb))

[Severity: Medium]
Does this conditional read of llp.msb risk a torn read if the DMA engine
is actively running?

If the linked list ring crosses a 4 GiB boundary, a valid descriptor can reside
at a physical address with a zero LSB (e.g., 0x00000001_00000000). 

Could the following sequence occur:

CPU reads llp.lsb as 0.
CPU is interrupted.
DMA engine advances the ring to an address below 4 GiB (e.g. 0x00000000_FFFFD000).
CPU resumes and reads llp.msb as 0.

Will this cause the function to falsely evaluate the pointer as all-zeros
and incorrectly return -EINVAL?

> +		return -EINVAL;
> +
> +	/* u32 subtraction handles a low-word wrap at a 4 GiB boundary. */
> +	return (val - base) / EDMA_LL_SZ;
> +}

[ ... ]

> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> @@ -384,6 +384,24 @@ static void dw_hdma_v0_core_ch_doorbell(struct dw_edma_chan *chan)

[ ... ]

> +static int dw_hdma_v0_core_ll_cur_idx(struct dw_edma_chan *chan)
> +{
> +	u32 base, val;
> +
> +	val = GET_CH_32(chan->dw, chan->dir, chan->id, llp.lsb);
> +	base = lower_32_bits(dw_edma_core_get_ll_paddr(chan));
> +
> +	/*
> +	 * An all-zero pointer means that the channel context is unavailable,
> +	 * so an LL region starting at DMA address zero is not supported.
> +	 */
> +	if (!val && !GET_CH_32(chan->dw, chan->dir, chan->id, llp.msb))

[Severity: Medium]
This shares the same logic as dw_edma_v0_core_ll_cur_idx(). Can this also
suffer from the identical torn read scenario described above, leading to a
false -EINVAL?

> +		return -EINVAL;
> +
> +	/* u32 subtraction handles a low-word wrap at a 4 GiB boundary. */
> +	return (val - base) / EDMA_LL_SZ;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1