Re: [PATCH 5/9] dmaengine: bcm2835: Add BCM2712 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dmaengine: bcm2835: missing STRIDE high bits in residue address helpers
--

commit 036b8292dd25c0046b851159c80cd9142c283fa1
Author: Peter Robinson <pbrobinson@gmail.com>

dmaengine: bcm2835: Add BCM2712 support

This commit adds support for the BCM2712 DMA controller with a 40-bit DMA
mask, configuring channels 6-11 as 40-bit DMA4 engines and setting
ext_addr for legacy layout channels so that address bits [39:32] are
stored in the control block STRIDE field.

> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
> index 35f8c47dea89f..f06609b04a098 100644
> --- a/drivers/dma/bcm2835-dma.c
> +++ b/drivers/dma/bcm2835-dma.c
[ ... ]
> @@ -349,6 +363,9 @@ static void bcm2835_dma_cb_set_src(struct bcm2835_chan *c,
>  	}
>  
>  	e->cb->src = lower_32_bits(addr);
> +	if (c->ext_addr)
> +		FIELD_MODIFY(BCM2712_DMA_STRIDE_SRC_HI, &e->cb->stride,
> +			     upper_32_bits(addr));
>  }
>  
>  static void bcm2835_dma_cb_set_dst(struct bcm2835_chan *c,
> @@ -362,6 +379,9 @@ static void bcm2835_dma_cb_set_dst(struct bcm2835_chan *c,
>  	}
>  
>  	e->cb->dst = lower_32_bits(addr);
> +	if (c->ext_addr)
> +		FIELD_MODIFY(BCM2712_DMA_STRIDE_DST_HI, &e->cb->stride,
> +			     upper_32_bits(addr));
>  }

[Severity: Medium]
Should bcm2835_dma_cb_mem_addr() and bcm2835_dma_get_pos() also account for
the upper address bits [39:32] stored in the STRIDE field when c->ext_addr
is set on legacy layout channels?

In drivers/dma/bcm2835-dma.c, bcm2835_dma_cb_set_src() and
bcm2835_dma_cb_set_dst() place upper_32_bits(addr) into e->cb->stride under
BCM2712_DMA_STRIDE_SRC_HI and BCM2712_DMA_STRIDE_DST_HI when c->ext_addr is
true on non-40-bit channels (such as channels 0-5 on BCM2712).

However, bcm2835_dma_cb_mem_addr() and bcm2835_dma_get_pos() only check
c->is_40bit_channel and otherwise return only the lower 32 bits:

drivers/dma/bcm2835-dma.c:bcm2835_dma_cb_mem_addr() {
	if (c->is_40bit_channel) {
		if (dir == DMA_DEV_TO_MEM)
			return bcm2711_dma40_addr(e->cb40->dst, e->cb40->dsti);
		return bcm2711_dma40_addr(e->cb40->src, e->cb40->srci);
	}

	return dir == DMA_DEV_TO_MEM ? e->cb->dst : e->cb->src;
}

drivers/dma/bcm2835-dma.c:bcm2835_dma_get_pos() {
	...
	if (dir == DMA_DEV_TO_MEM)
		return readl(base + BCM2835_DMA_DEST_AD);
	return readl(base + BCM2835_DMA_SOURCE_AD);
}

When bcm2835_dma_tx_status() calls bcm2835_dma_get_pos() and
bcm2835_dma_desc_size_pos() to compute transfer residue:

drivers/dma/bcm2835-dma.c:bcm2835_dma_desc_size_pos() {
	for (size = i = 0; i < d->frames; i++) {
		struct bcm2835_cb_entry *e = &d->cb_list[i];
		size_t this_size = bcm2835_dma_cb_get_len(d->c, e);
		dma_addr_t dma = bcm2835_dma_cb_mem_addr(d->c, e, d->dir);

		if (size)
			size += this_size;
		else if (addr >= dma && addr < dma + this_size)
			size += dma + this_size - addr;
	}
	return size;
}

If a transfer on a BCM2712 legacy layout channel crosses a 4 GiB boundary,
dma + this_size exceeds 0xffffffff while addr wraps in the 32-bit
BCM2835_DMA_SOURCE_AD or BCM2835_DMA_DEST_AD register, causing the range
check (addr >= dma && addr < dma + this_size) to fail and report zero
residue while the transfer is still running. Similarly, if scatter-gather
segments in different 4 GiB regions have overlapping low 32-bit addresses,
bcm2835_dma_desc_size_pos() can match the wrong control block.

Could bcm2835_dma_cb_mem_addr() and bcm2835_dma_get_pos() reconstruct the
full 40-bit address from the STRIDE field when c->ext_addr is true?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102090713.896324-1-pbrobinson@gmail.com?part=5