Re: [PATCH RFC net 2/6] net: macb: reprogram TBQP after shuffling the TX ring on link-up

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: macb: missing TX_USED bit on desc 0 in gem_shuffle_tx_one_ring()
--

commit ded0ab42c83afe9d93d9bc6c0ab9623a1dc038f5
Author: Christian Taedcke <christian.taedcke@xxxxxxxxxxxxxxx>

net: macb: reprogram TBQP after shuffling the TX ring on link-up

This commit reprograms the hardware transmit buffer queue pointer (TBQP) to
queue->tx_ring_dma in gem_shuffle_tx_one_ring() whenever tx_tail is reset to
0 during link-up, preventing TBQP from pointing at a stale descriptor.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 4b50378675378..97494933eb1ee 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -823,7 +823,7 @@ static void gem_shuffle_tx_one_ring(struct macb_queue *queue)
>  	if (!count) {
>  		queue->tx_head = 0;
>  		queue->tx_tail = 0;
> -		goto unlock;
> +		goto reset_hw_ptr;
>  	}
>  
>  	shift = tail % ring_size;
> @@ -872,6 +872,13 @@ static void gem_shuffle_tx_one_ring(struct macb_queue *queue)
>  	/* Make descriptor updates visible to hardware */
>  	wmb();
>  
> +reset_hw_ptr:
> +	/* tx_tail was reset to the ring base, so TBQP must be reprogrammed
> +	 * to match; otherwise it keeps pointing at a stale descriptor. Safe
> +	 * to write directly here as TX is still disabled (called from
> +	 * macb_mac_link_up() before TE is set).
> +	 */
> +	queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));

[Severity: High]
When count is 0 and gem_shuffle_tx_one_ring() in
drivers/net/ethernet/cadence/macb_main.c jumps from the !count branch to
reset_hw_ptr without rotating the ring, is descriptor 0 guaranteed to have
MACB_BIT(TX_USED) set before TBQP is reprogrammed to queue->tx_ring_dma?

When a multi-descriptor frame (such as scatter-gather or segmentation
offload) wraps around the end of tx_ring so that descriptor 0 is a non-first
descriptor of that frame, macb_tx_map() clears MACB_BIT(TX_USED) on all
descriptors of the frame and sets MACB_BIT(TX_USED) on the sentinel
descriptor at tx_head. On frame completion, the hardware only sets
MACB_BIT(TX_USED) on the first descriptor of the frame, and
macb_tx_complete() unmaps the buffers without setting MACB_BIT(TX_USED) on
the remaining descriptors:

drivers/net/ethernet/cadence/macb_main.c:macb_tx_complete() {
	...
		/* TX_USED bit is only set by hardware on the very first buffer
		 * descriptor of the transmitted frame.
		 */
		if (!(ctrl & MACB_BIT(TX_USED)))
			break;
	...
}

If the queue subsequently drains (count == 0) at a non-zero ring index
(tail % ring_size != 0) before link-up, the TX_USED sentinel sits at
desc[tail % ring_size] while desc[0]->ctrl still has TX_USED cleared and
desc[0]->addr holds an unmapped dma address.

Jumping to reset_hw_ptr points TBQP at desc[0] with TX_USED == 0. If
MACB_BIT(TSTART) is then written to NCR (for example when another queue
transmits in macb_start_xmit() or when TSTART is kicked on link-up) before a
new frame is mapped on this queue, can the controller read the stale unmapped
address in desc[0]->addr and advance TBQP past index 0 while tx_head and
tx_tail remain 0?

In comparison, macb_tx_error_task() marks descriptor 0 as used and issues a
write barrier before writing TBQP:

drivers/net/ethernet/cadence/macb_main.c:macb_tx_error_task() {
	...
	/* Set end of TX queue */
	desc = macb_tx_desc(queue, 0);
	macb_set_addr(bp, desc, 0);
	desc->ctrl = MACB_BIT(TX_USED);

	/* Make descriptor updates visible to hardware */
	wmb();

	/* Reinitialize the TX desc queue */
	queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
	...
}

Should the !count path also set MACB_BIT(TX_USED) on descriptor 0 and call
wmb() before reprogramming TBQP?

>  unlock:
>  	spin_unlock_irqrestore(&queue->tx_ptr_lock, flags);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-std-macb-v14-send-v1-0-9187bc545bf6@xxxxxxxxxxxx?part=2