[PATCH net 0/2] net: macb: two fixes for macb_tx_error_task()

[PATCH net 0/2] net: macb: two fixes for macb_tx_error_task()

[Nicolai Buchwitz]

Both pre-existing bugs were flagged during the clashiko review of my
software FCS patch [1]. These are the promised follow-up patches.

Patch 1 credits the frames dropped on a TX error to BQL. Patch 2 takes
tx_ptr_lock in the task, as macb_start_xmit() moves the same pointers
under it.

Tested on a Raspberry Pi CM5 (RP1 GEM). The bugs are hard to reproduce
and the error path never fired there, but they are worth fixing anyway.

[1] https://lore.kernel.org/netdev/d1f02ab923c0464fa73cafa75a21943d@tipi-net.de/

Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
Nicolai Buchwitz (2):
      net: macb: credit unsent frames to BQL in macb_tx_error_task()
      net: macb: take tx_ptr_lock in macb_tx_error_task()

 drivers/net/ethernet/cadence/macb_main.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
---
base-commit: 30c71476d842b094651bd7712f6c8ee8716147d0
change-id: 20261107-nb-macb-tx-error-task-net-e8a553b4d138

Best regards,
--  
Nicolai Buchwitz <nb@tipi-net.de>

[PATCH net 1/2] net: macb: credit unsent frames to BQL in macb_tx_error_task()

[Nicolai Buchwitz]

The frames dropped on a TX error are never returned to BQL. Each error
shrinks the BQL budget for good until the queue stops with an empty
ring.

netdev_tx_reset_queue() is not safe here as the task runs without
tx_ptr_lock, so macb_start_xmit() may still be queueing.

Fixes: e6a532185daa ("net: cadence: macb: Implement BQL")
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/cadence/macb_main.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index f087f278f9e56..b6105f915fe35 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1412,10 +1412,8 @@ static void macb_tx_error_task(struct work_struct *work)
 					    skb->data);
 				bp->netdev->stats.tx_packets++;
 				queue->stats.tx_packets++;
-				packets++;
 				bp->netdev->stats.tx_bytes += skb->len - tx_skb->fcs_len;
 				queue->stats.tx_bytes += skb->len - tx_skb->fcs_len;
-				bytes += skb->len;
 			}
 		} else {
 			/* "Buffers exhausted mid-frame" errors may only happen
@@ -1428,6 +1426,12 @@ static void macb_tx_error_task(struct work_struct *work)
 			desc->ctrl = ctrl | MACB_BIT(TX_USED);
 		}
 
+		/* BQL has to see every frame leaving the ring, sent or not */
+		if (skb) {
+			packets++;
+			bytes += skb->len;
+		}
+
 		macb_tx_unmap(&bp->info, tx_skb, 0);
 	}
 

-- 
2.53.0

[PATCH net 2/2] net: macb: take tx_ptr_lock in macb_tx_error_task()

[Nicolai Buchwitz]

The error task resets the TX ring under bp->lock while macb_start_xmit()
updates it under tx_ptr_lock. Stopping the queues does not wait for an
xmit already running on another CPU, so a frame mapped during the reset
is lost and its skb and DMA mapping leak.

Take tx_ptr_lock like macb_start_xmit() does.

Fixes: 138badbc21a0 ("net: macb: use NAPI for TX completion path")
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/cadence/macb_main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index b6105f915fe35..ff5e364e857e5 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1369,7 +1369,8 @@ static void macb_tx_error_task(struct work_struct *work)
 	 * network engine about the macb/gem being halted.
 	 */
 	napi_disable(&queue->napi_tx);
-	spin_lock_irqsave(&bp->lock, flags);
+	spin_lock_irqsave(&queue->tx_ptr_lock, flags);
+	spin_lock(&bp->lock);
 
 	/* Make sure nobody is trying to queue up new packets */
 	netif_tx_stop_all_queues(bp->netdev);
@@ -1463,7 +1464,8 @@ static void macb_tx_error_task(struct work_struct *work)
 	netif_tx_start_all_queues(bp->netdev);
 	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
 
-	spin_unlock_irqrestore(&bp->lock, flags);
+	spin_unlock(&bp->lock);
+	spin_unlock_irqrestore(&queue->tx_ptr_lock, flags);
 
 	napi_enable(&queue->napi_tx);
 

-- 
2.53.0

Re: [PATCH net 1/2] net: macb: credit unsent frames to BQL in macb_tx_error_task()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix leaves a second site with the same bug.

--

net: macb: credit unsent frames to BQL in macb_tx_error_task()

This commit moves the BQL packets/bytes accounting in macb_tx_error_task()
out of the "TX complete" branch. Every frame removed from the ring is now
credited through netdev_tx_completed_queue(), whether it was sent or not.
This stops the DQL in-flight count from leaking on each TX error.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index f087f278f9e56..b6105f915fe35 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

[ ... ]

> @@ -1428,6 +1426,12 @@ static void macb_tx_error_task(struct work_struct *work)
>  			desc->ctrl = ctrl | MACB_BIT(TX_USED);
>  		}
>  
> +		/* BQL has to see every frame leaving the ring, sent or not */
> +		if (skb) {
> +			packets++;
> +			bytes += skb->len;
> +		}
> +
>  		macb_tx_unmap(&bp->info, tx_skb, 0);
>  	}
>  

[Severity: High]
The commit message describes the problem in general terms: "The frames
dropped on a TX error are never returned to BQL." Does
macb_hresp_error_task() leak in the same way?

macb_interrupt_misc() queues it as BH work when the hardware reports
HRESP:

macb_interrupt_misc() {
    ...
	if (status & MACB_BIT(HRESP)) {
		queue_work(system_bh_wq, &bp->hresp_err_bh_work);
    ...
}

It then resets the TX rings and restarts the queues. It never calls
netdev_tx_completed_queue() or netdev_tx_reset_queue():

macb_hresp_error_task() {
    ...
	netif_tx_stop_all_queues(netdev);
	netif_carrier_off(netdev);

	bp->macbgem_ops.mog_init_rings(ctx);

	/* Initialize TX and RX buffers */
	macb_init_buffers(bp);
    ...
	netif_carrier_on(netdev);
	netif_tx_start_all_queues(netdev);
}

gem_init_rings() and macb_init_rings() mark every TX descriptor TX_USED
and set txq->head = txq->tail = 0. Every frame between tail and head was
already charged in macb_start_xmit():

	netdev_tx_sent_queue(netdev_get_tx_queue(bp->netdev, q),
			     skb->len);

So DQL would count those bytes as in flight forever. After enough HRESP
events, the queue would stay stopped with an empty ring. Only
macb_close() would recover it, through netdev_tx_reset_queue().

This commit carries the same Fixes: tag as the BQL introduction, and that
commit missed both paths. Could the HRESP ring reset path be covered here
too, or in a companion patch, so that backports get both fixes?

On a side note, this HRESP path also seems to reset head and tail
without unmapping the DMA buffers or freeing the tx_skb->skb entries. Are
those skbs and DMA mappings leaked as well?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107-nb-macb-tx-error-task-net-v1-0-a2aac5d64d52%40tipi-net.de