Re: [PATCH net-next v2 5/6] net: ftgmac100: Add AST2700 upper DMA address support

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] struct ftgmac100_txdes in
  drivers/net/ethernet/faraday/ftgmac100.h still says `__le32 txdes2; /*…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but TX
  completion has no consumer-side read barrier to pair with the…

--

net: ftgmac100: Add AST2700 upper DMA address support

This patch programs the high RX and TX descriptor ring base registers on
AST2700. It also stores DMA address bits 34:32 in rxdes2/txdes2 and
rebuilds the full address from both words before unmapping. A 35-bit DMA
mask is requested, and all of this is gated by the new
FTGMAC100_QUIRK_DMA_35BIT match-data quirk.

> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index 2d029d1262e6f..af2e272f32f2d 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c

[ ... ]

> @@ -672,9 +686,14 @@ static void ftgmac100_free_tx_packet(struct ftgmac100 *priv,
>  				     struct ftgmac100_txdes *txdes,
>  				     u32 ctl_stat)
>  {
> -	dma_addr_t map = le32_to_cpu(txdes->txdes3);
> +	dma_addr_t map;
>  	size_t len;
>  
> +	map = le32_to_cpu(txdes->txdes3);
> +	if (priv->quirks & FTGMAC100_QUIRK_DMA_35BIT)
> +		map |= (u64)FIELD_GET(FTGMAC100_TXDES2_TXBUF_BADR_HI,
> +				     le32_to_cpu(txdes->txdes2)) << 32;
> +

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. The new
txdes2 read goes through the same TX completion handoff as the existing
txdes0/txdes3 reads, and that handoff seems to have no read barrier on the
consumer side.

ftgmac100_hard_start_xmit() fills tx_skbs[] and txdes1/2/3, sets OWN in
txdes0, and then publishes the producer index:

	dma_wmb();
	first->txdes0 = cpu_to_le32(f_ctl_stat);
	...
	smp_wmb();
	...
	priv->tx_pointer = pointer;

On another CPU, ftgmac100_poll()->ftgmac100_tx_complete() runs:

	while (ftgmac100_tx_buf_cleanable(priv) &&
	       ftgmac100_tx_complete_packet(priv))
		;

ftgmac100_tx_buf_cleanable() loads priv->tx_pointer. After that,
ftgmac100_tx_complete_packet() loads txdes0 and tx_skbs[pointer], and
ftgmac100_free_tx_packet() loads txdes2/txdes3. Nothing like smp_rmb()
sits between the tx_pointer load and the descriptor loads. The smp_mb()
after the loop runs too late to help.

On weakly ordered SMP CPUs such as ARM/ARM64, can the txdes0 load be
satisfied before the tx_pointer load? If so, it could return the stale
OWN-clear value left by the previous completion, and the slot would be
reclaimed while the device still owns it.

That could unmap a buffer the device is still reading. It could also
dereference skb->len through a stale NULL tx_skbs[] entry.

ftgmac100_poll() keeps NAPI scheduled while ftgmac100_tx_buf_cleanable()
is true, so completion can run on one CPU while xmit runs on another.
Would an smp_rmb() after the tx_pointer check in the completion path be
needed here?

[ ... ]

> diff --git a/drivers/net/ethernet/faraday/ftgmac100.h b/drivers/net/ethernet/faraday/ftgmac100.h
> index 8c35c895b1465..0c244b1c0a351 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.h
> +++ b/drivers/net/ethernet/faraday/ftgmac100.h

[ ... ]

> @@ -226,6 +229,7 @@ struct ftgmac100_txdes {
>  #define FTGMAC100_TXDES1_TX2FIC		(1 << 30)
>  #define FTGMAC100_TXDES1_TXIC		(1 << 31)
>  
> +#define FTGMAC100_TXDES2_TXBUF_BADR_HI	GENMASK(18, 16)

[Severity: Low]
This isn't a bug, but struct ftgmac100_txdes still describes this word as
reserved:

	__le32	txdes2; /* Reserved */

With this patch, ftgmac100_hard_start_xmit() writes the upper buffer
address bits into txdes2 for the head descriptor and for every fragment
descriptor. ftgmac100_free_tx_packet() reads them back to rebuild the
address passed to dma_unmap_single()/dma_unmap_page().

Could the comment be updated to describe the new field? Otherwise a later
change might zero or reuse txdes2 based on that comment.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006-support_ast2700-v2-0-36de51fb8066%40aspeedtech.com