Re: [PATCH RFC net 5/6] net: macb: kick the transmitter on link-up

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] net: macb: DMA of freed skb in macb_mac_link_up() when empty ring resets TBQP
- [High] net: macb: unbased TBQP index in macb_tx_restart() breaks link-up kick
--

commit 19c83c729b5e6d7a587a6b6ecc879a9a247907de
Author: Steffen Dirkwinkel <s.dirkwinkel@beckhoff.com>

net: macb: kick the transmitter on link-up

This commit calls macb_tx_restart() for each queue in macb_mac_link_up()
after enabling transmit so that pending frames in the ring are transmitted
even if byte queue limits previously stopped the queue.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 6e7ead23d0b74..3dfc7ffdd5985 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

[ ... ]

> @@ -969,6 +971,10 @@ static void macb_mac_link_up(struct phylink_config *config,
>  	spin_unlock_irqrestore(&bp->lock, flags);
>  
>  	if (!(bp->caps & MACB_CAPS_MACB_IS_EMAC)) {
> +		/* With BQL holding a queue stopped no xmit would issue TSTART */
> +		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
> +			macb_tx_restart(queue);

[Severity: Critical]
Can kicking the transmitter here in macb_mac_link_up() cause the controller
to transmit an already unmapped and freed skb buffer on an empty queue?

When a multi-descriptor frame wraps across the end of the ring,
macb_tx_map() clears the used bit on all descriptors of the frame and
places the sentinel used bit at the new queue->tx_head. The controller only
sets the used bit on the first descriptor of a transmitted frame, and
macb_tx_complete() unmaps the buffers via macb_tx_unmap() and frees the skb
without setting the used bit on the remaining descriptors. As a result,
descriptor 0 can retain a cleared used bit and a stale buffer address while
the sentinel sits at start % ring_size != 0.

When that queue is empty (count == 0) with start % ring_size != 0 during
link-up, macb_mac_link_up() -> gem_shuffle_tx_rings() ->
gem_shuffle_tx_one_ring() skips the ring rotation and reprograms hardware
to descriptor 0:

drivers/net/ethernet/cadence/macb_main.c:gem_shuffle_tx_one_ring() {
	if (!(start % ring_size))
		goto reset_hw_ptr;

	if (!count) {
		queue->tx_head = 0;
		queue->tx_tail = 0;
		goto reset_hw_ptr;
	}
	...
reset_hw_ptr:
	queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
}

Unlike macb_tx_error_task(), which explicitly marks descriptor 0 as used
before resetting the hardware pointer:

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);
}

gem_shuffle_tx_one_ring() leaves descriptor 0 with its stale control word.
When macb_tx_restart() then sets MACB_BIT(TSTART) in the global NCR
register because another queue has pending frames, won't the controller
start reading from descriptor 0 on the empty queue, transmit the freed
buffer onto the network, and advance the hardware pointer past index 0
while queue->tx_head and queue->tx_tail remain at 0?

> +
>  		/* Reap the frames completed while link-down had TCOMP masked */
>  		local_bh_disable();
>  		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)

[Severity: High]
Does macb_tx_restart() compute the descriptor index from queue_readl(queue,
TBQP) without subtracting the ring base address, causing missed kicks or
spurious restarts on link-up?

Earlier in macb_mac_link_up(), gem_shuffle_tx_one_ring() rotates the ring
so that done completed frames sit at [ring_size - done, ring_size), unsent
frames sit at [0, count - done), and the hardware pointer is reset to the
ring base:

drivers/net/ethernet/cadence/macb_main.c:gem_shuffle_tx_one_ring() {
	queue->tx_tail = done ? ring_size - done : 0;
	queue->tx_head = queue->tx_tail + count;

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

reset_hw_ptr:
	queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
}

Then macb_mac_link_up() calls macb_tx_restart(queue) before scheduling
queue->napi_tx to reap those completed frames:

drivers/net/ethernet/cadence/macb_main.c:macb_tx_restart() {
	if (queue->tx_head == queue->tx_tail)
		goto out_tx_ptr_unlock;

	tbqp = queue_readl(queue, TBQP) / macb_dma_desc_get_size(bp);
	tbqp = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp, tbqp));
	head_idx = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp, queue->tx_head));

	if (tbqp == head_idx)
		goto out_tx_ptr_unlock;
}

Because queue_readl(queue, TBQP) returns the raw 32-bit address
lower_32_bits(queue->tx_ring_dma) rather than a byte offset from the start
of the ring, dividing it directly by macb_dma_desc_get_size(bp) only wraps
to index 0 if lower_32_bits(queue->tx_ring_dma) is an exact multiple of
bp->tx_ring_size * macb_dma_desc_get_size(bp).

In macb_alloc(), queue->tx_ring_dma is not aligned to that product in
several configurations:

drivers/net/ethernet/cadence/macb_main.c:macb_alloc() {
	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
		queue->tx_ring = tx + macb_tx_ring_size_per_queue(bp) * q;
		queue->tx_ring_dma = tx_dma + macb_tx_ring_size_per_queue(bp) * q;
}

Specifically:
- On multi-queue controllers with descriptor prefetch,
  macb_tx_ring_size_per_queue(bp) adds bp->tx_bd_rd_prefetch bytes per
  queue, shifting queue->tx_ring_dma for queues >= 1.
- When 64-bit addressing and ptp timestamping are both enabled,
  macb_dma_desc_get_size(bp) is 24 bytes, which does not evenly divide a
  page-aligned base address even on queue 0.
- When bp->tx_ring_size * macb_dma_desc_get_size(bp) exceeds page size and
  dma_alloc_coherent() returns an address aligned only to page size.

When tbqp at the ring base wraps to a non-zero base_idx instead of 0:
- If count - done == base_idx unsent frames are waiting at [0, base_idx),
  head_idx equals tbqp, so macb_tx_restart() skips writing MACB_BIT(TSTART)
  and leaves the stopped queue stuck.
- If all queued frames completed while link-down masked completion
  interrupts (done == count > 0), queue->tx_head != queue->tx_tail is still
  true before napi_tx runs, and head_idx is 0 while tbqp is non-zero, so
  macb_tx_restart() writes MACB_BIT(TSTART) onto a used descriptor at index
  0 and triggers a spurious used-bit-read interrupt.

Should macb_tx_restart() subtract lower_32_bits(queue->tx_ring_dma) from
queue_readl(queue, TBQP) before dividing by macb_dma_desc_get_size(bp)?

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