Re: [net] net: macb: Shuffle the tx ring before enabling tx

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: macb: Shuffle the tx ring before enabling tx

Quanyang observed that when using an NFS rootfs on an AMD ZynqMp board,
the rootfs may take an extended time to recover after a suspend. Upon
investigation, it was determined that the issue originates from a problem
in the macb driver. To resolve this issue, shuffle the tx ring and tx skb
array so that the first unsent packet is positioned at the start of the tx
ring.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 5bc35f651ebd..3ceeff22c12f 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

[ ... ]

> @@ -668,6 +669,88 @@ static void macb_mac_link_down(struct phylink_config *config, unsigned int mode,
>  	netif_tx_stop_all_queues(ndev);
>  }
>
> +/* Use juggling algorithm to left rotate tx ring and tx skb array */
> +static void gem_shuffle_tx_one_ring(struct macb_queue *queue)
> +{
> +	unsigned int i, head, tail, count, size, cycles, shift, curr, next;
> +	struct macb_dma_desc desc, *desc_curr, *desc_next;
> +	struct macb_tx_skb tx_skb, *skb_curr, *skb_next;
> +	struct macb *bp = queue->bp;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&queue->tx_ptr_lock, flags);
> +	head = queue->tx_head;
> +	tail = queue->tx_tail;
> +	size = bp->tx_ring_size;
> +	count = CIRC_CNT(head, tail, size);
> +
> +	if (!(tail % size))
> +		goto unlock;
> +
> +	if (!count) {
> +		queue->tx_head = 0;
> +		queue->tx_tail = 0;
> +		goto unlock;
> +	}
> +
> +	shift = tail % size;
> +	cycles = gcd(size, shift);
> +
> +	for (i = 0; i < cycles; i++) {
> +		memcpy(&desc, macb_tx_desc(queue, i), sizeof(struct macb_dma_desc));
> +		memcpy(&tx_skb, macb_tx_skb(queue, i), sizeof(struct macb_tx_skb));
> +
> +		curr = i;
> +		next = (curr + shift) % size;
> +
> +		while (next != i) {
> +			desc_curr = macb_tx_desc(queue, curr);
> +			desc_next = macb_tx_desc(queue, next);
> +
> +			memcpy(desc_curr, desc_next, sizeof(struct macb_dma_desc));
                                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Does this memcpy corrupt the descriptor fields on systems with 64-bit DMA?
The macb_tx_desc() macro uses macb_adj_dma_desc_idx() to properly space the
descriptor pointers in the ring according to the descriptor size reported by
macb_dma_desc_get_size(bp), which can be 8 bytes (base), 16 bytes (with
DMA64), or 24 bytes (with DMA64 + PTP). However, all three memcpy calls in
this function use sizeof(struct macb_dma_desc), which is only 8 bytes and
covers only the addr and ctrl fields.

On systems where macb_dma64(bp) == true, such as the AMD ZynqMp platform
this patch targets, each descriptor has an additional macb_dma_desc_64
struct containing the upper 32 bits of the DMA address (addrh field).

When the shuffle copies descriptors using only 8 bytes, it moves the lower
32-bit address (desc->addr) and control word (desc->ctrl) to new positions
but leaves the upper 32-bit address (addrh) unmoved in the old location.
After the shuffle, hardware reads a composite 64-bit DMA address from
mismatched descriptor slots: the old slot's addrh combined with the new
slot's addr.

This would cause DMA to a corrupted physical address, potentially leading
to memory corruption or bus errors.

The driver uses macb_dma_desc_get_size(bp) elsewhere for operations on the
descriptor ring:

drivers/net/ethernet/cadence/macb_main.c:macb_alloc_consistent() {
    ...
    size = bp->tx_ring_size * macb_dma_desc_get_size(bp);
    queue->tx_ring = dma_alloc_coherent(&bp->pdev->dev, size,
                                        &queue->tx_ring_dma, GFP_KERNEL);
    ...
}

and in:

drivers/net/ethernet/cadence/macb_main.c:macb_free_consistent() {
    ...
    size = bp->tx_ring_size * macb_dma_desc_get_size(bp);
    dma_free_coherent(&bp->pdev->dev, size, queue->tx_ring,
                      queue->tx_ring_dma);
    ...
}

Should the memcpy size here use macb_dma_desc_get_size(bp) instead of
sizeof(struct macb_dma_desc)?

The same issue appears at two other locations in this function (the initial
memcpy saving desc at the start of the loop, and the final memcpy restoring
desc at the end). Additionally, the stack-allocated "struct macb_dma_desc
desc" temporary buffer may be too small to hold the extended descriptor.

> +
> +			if (next == bp->tx_ring_size - 1)
> +				desc_curr->ctrl &= ~MACB_BIT(TX_WRAP);
> +			if (curr == bp->tx_ring_size - 1)
> +				desc_curr->ctrl |= MACB_BIT(TX_WRAP);
> +
> +			skb_curr = macb_tx_skb(queue, curr);
> +			skb_next = macb_tx_skb(queue, next);
> +			memcpy(skb_curr, skb_next, sizeof(struct macb_tx_skb));
> +
> +			curr = next;
> +			next = (curr + shift) % size;
> +		}
> +
> +		desc_curr = macb_tx_desc(queue, curr);
> +		memcpy(desc_curr, &desc, sizeof(struct macb_dma_desc));
> +		if (i == bp->tx_ring_size - 1)
> +			desc_curr->ctrl &= ~MACB_BIT(TX_WRAP);
> +		if (curr == bp->tx_ring_size - 1)
> +			desc_curr->ctrl |= MACB_BIT(TX_WRAP);
> +		memcpy(macb_tx_skb(queue, curr), &tx_skb, sizeof(struct macb_tx_skb));
> +	}
> +
> +	queue->tx_head = count;
> +	queue->tx_tail = 0;
> +
> +	/* Make descriptor updates visible to hardware */
> +	wmb();
> +
> +unlock:
> +	spin_unlock_irqrestore(&queue->tx_ptr_lock, flags);
> +}

[ ... ]
-- 
pw-bot: cr