Re: [PATCH v6 01/10] 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] Unsigned subtraction underflow causes out-of-bounds hardware pointer values to return a large positive index instead of a negative error code.
--

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

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

This commit introduces the dw_edma_core_ll_cur_idx() function to retrieve
the current linked list entry index. This prepares the driver for the
dynamic addition of DMA requests while the DMA 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
> index abc1bd4c0be2e..cc80591829403 100644
> --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
> @@ -616,6 +616,25 @@ static void dw_edma_v0_core_ch_doorbell(struct dw_edma_chan *chan)
>  		  FIELD_PREP(EDMA_V0_DOORBELL_CH_MASK, chan->id));
>  }
>  
> +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));
> +
> +	/*
> +	 * LL regions stay within one 4 GiB address window. Reject an all-ones
> +	 * MMIO value. If the low word is zero, use the high word to distinguish
> +	 * a nonzero boundary address from an unprogrammed all-zero context.
> +	 */
> +	if (val == U32_MAX ||
> +	    (!val && !GET_CH_32(chan->dw, chan->dir, chan->id, llp.msb)))
> +		return -EINVAL;
> +
> +	return (val - base) / EDMA_LL_SZ;

[Severity: Medium]
Is it possible for val to be less than base if the channel context is
uninitialized and holds a stale linked-list pointer from a previous
allocation at a lower physical address?

Since val and base are unsigned 32-bit integers, if val < base, the operation
val - base underflows to a large unsigned integer. Dividing this by
EDMA_LL_SZ yields a massive positive index that fits into the signed int
return type, rather than returning a negative error code as intended.

Would it make sense to verify that val is >= base, or to bounds check the
resulting index against chan->ll_max to prevent potential out-of-bounds
reads or writes?

> +}
> +

[ ... ]

> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> index 36ee72efcd317..19da1c8fd6159 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> @@ -384,6 +384,25 @@ static void dw_hdma_v0_core_ch_doorbell(struct dw_edma_chan *chan)
>  	SET_CH_32(dw, chan->dir, chan->id, doorbell, HDMA_V0_DOORBELL_START);
>  }
>  
> +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));
> +
> +	/*
> +	 * LL regions stay within one 4 GiB address window. Reject an all-ones
> +	 * MMIO value. If the low word is zero, use the high word to distinguish
> +	 * a nonzero boundary address from an unprogrammed all-zero context.
> +	 */
> +	if (val == U32_MAX ||
> +	    (!val && !GET_CH_32(chan->dw, chan->dir, chan->id, llp.msb)))
> +		return -EINVAL;
> +
> +	return (val - base) / EDMA_LL_SZ;

[Severity: Medium]
Could this suffer from the same underflow issue as the v0 core implementation
if val is less than base?

> +}
> +

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