[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