[PATCH net v2 0/4] Drop in-flight Tx SKBs on MACB close

[PATCH net v2 0/4] Drop in-flight Tx SKBs on MACB close

[Théo Lebrun]

The first patch is here to allow giving a drop reason.
We dissociate consumed packets from dropped ones that way.

Second patch is the main one: it drops unsent packets on close.
MACB driver forgot freeing its SKBs (and associated DMA mappings).

Last two patches are to fix stats reporting on dropped codepaths.

---
Changes in v2:
- Increment tx_dropped stat once per SKB, not once per frame.
- Reset tx_head & tx_tail to avoid keeping stalled cursors.
- Fix SKB dropped reasons throughout by adding the reason as parameter
  to macb_tx_unmap(). This is a new patch. Then the drop-all-on-close
  fix can use this ability to report we are not consuming SKBs.
- Add increment to stats->tx_dropped on DMA mapping failure and
  tx_error_task. Done as separate patches (3 and 4).
- Rebase upon net/main @ 46f74a3f7d57, nothing to report.
- Link to v1: https://patch.msgid.link/20260424-macb-drop-tx-v1-1-b3ecb787d84d@bootlin.com

To: Nicolas Ferre <nicolas.ferre@microchip.com>
To: Claudiu Beznea <claudiu.beznea@tuxon.dev>
To: Andrew Lunn <andrew+netdev@lunn.ch>
To: "David S. Miller" <davem@davemloft.net>
To: Eric Dumazet <edumazet@google.com>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
To: Haavard Skinnemoen <hskinnemoen@atmel.com>
To: Jeff Garzik <jeff@garzik.org>
Cc: Paolo Valerio <pvalerio@redhat.com>
Cc: Conor Dooley <conor@kernel.org>
Cc: Nicolai Buchwitz <nb@tipi-net.de>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
Cc: Gregory CLEMENT <gregory.clement@bootlin.com>
Cc: Benoît Monin <benoit.monin@bootlin.com>
Cc: Tawfik Bayouk <tawfik.bayouk@mobileye.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

---
Théo Lebrun (4):
      net: macb: give reasons for Tx SKB kfree
      net: macb: drop in-flight Tx SKBs on close
      net: macb: increment stats.tx_dropped on tx error
      net: macb: increment stats.tx_dropped on DMA map error

 drivers/net/ethernet/cadence/macb_main.c | 57 +++++++++++++++++++++++++++-----
 1 file changed, 48 insertions(+), 9 deletions(-)
---
base-commit: e3684df8e778a9988b7f7a84e08daea8019b661e
change-id: 20260423-macb-drop-tx-f9ce72720d05

Best regards,
--  
Théo Lebrun <theo.lebrun@bootlin.com>

[PATCH net v2 1/4] net: macb: give reasons for Tx SKB kfree

[Théo Lebrun]

Using dev_consume_skb_any() marks the drop reason as SKB_CONSUMED every
time we free a Tx SKB. Instead, replace by SKB_DROP_REASON_NOT_SPECIFIED
when packet has been dropped without sending.

It is not precise but at least differs from SKB_CONSUMED and is used by
many drivers for their error codepaths through dev_kfree_skb_{any,irq}().

Pass a reason around rather than call dev_consume_skb_any() or
dev_kfree_skb_any() because macb_tx_unmap() is called for cleanup in
all cases.

macb_tx_error_task() is made complex because some SKBs encountered have
been successfully sent.

Fixes: 89e5785fc8a6 ("[PATCH] Atmel MACB ethernet driver")
Cc: stable@vger.kernel.org
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index a12aa21244e8..9caae1ef52b1 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1201,7 +1201,8 @@ static int macb_halt_tx(struct macb *bp)
 					bp, TSR);
 }
 
-static void macb_tx_unmap(struct macb *bp, struct macb_tx_skb *tx_skb, int budget)
+static void macb_tx_unmap(struct macb *bp, struct macb_tx_skb *tx_skb,
+			  int budget, enum skb_drop_reason reason)
 {
 	if (tx_skb->mapping) {
 		if (tx_skb->mapped_as_page)
@@ -1214,7 +1215,7 @@ static void macb_tx_unmap(struct macb *bp, struct macb_tx_skb *tx_skb, int budge
 	}
 
 	if (tx_skb->skb) {
-		dev_consume_skb_any(tx_skb->skb);
+		dev_kfree_skb_any_reason(tx_skb->skb, reason);
 		tx_skb->skb = NULL;
 	}
 }
@@ -1297,7 +1298,8 @@ static void macb_tx_error_task(struct work_struct *work)
 	 * Free transmit buffers in upper layer.
 	 */
 	for (tail = queue->tx_tail; tail != queue->tx_head; tail++) {
-		u32	ctrl;
+		enum skb_drop_reason reason = SKB_DROP_REASON_NOT_SPECIFIED;
+		u32 ctrl;
 
 		desc = macb_tx_desc(queue, tail);
 		ctrl = desc->ctrl;
@@ -1307,7 +1309,10 @@ static void macb_tx_error_task(struct work_struct *work)
 		if (ctrl & MACB_BIT(TX_USED)) {
 			/* skb is set for the last buffer of the frame */
 			while (!skb) {
-				macb_tx_unmap(bp, tx_skb, 0);
+				/* The reason parameter is unused because it
+				 * only matters when skb is valid.
+				 */
+				macb_tx_unmap(bp, tx_skb, 0, SKB_CONSUMED);
 				tail++;
 				tx_skb = macb_tx_skb(queue, tail);
 				skb = tx_skb->skb;
@@ -1326,6 +1331,7 @@ static void macb_tx_error_task(struct work_struct *work)
 				bp->dev->stats.tx_bytes += skb->len;
 				queue->stats.tx_bytes += skb->len;
 				bytes += skb->len;
+				reason = SKB_CONSUMED;
 			}
 		} else {
 			/* "Buffers exhausted mid-frame" errors may only happen
@@ -1339,7 +1345,7 @@ static void macb_tx_error_task(struct work_struct *work)
 			desc->ctrl = ctrl | MACB_BIT(TX_USED);
 		}
 
-		macb_tx_unmap(bp, tx_skb, 0);
+		macb_tx_unmap(bp, tx_skb, 0, reason);
 	}
 
 	netdev_tx_completed_queue(netdev_get_tx_queue(bp->dev, queue_index),
@@ -1458,7 +1464,7 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
 			}
 
 			/* Now we can safely release resources */
-			macb_tx_unmap(bp, tx_skb, budget);
+			macb_tx_unmap(bp, tx_skb, budget, SKB_CONSUMED);
 
 			/* skb is set only for the last buffer of the frame.
 			 * WARNING: at this point skb has been freed by
@@ -2357,7 +2363,11 @@ static unsigned int macb_tx_map(struct macb *bp,
 	for (i = queue->tx_head; i != tx_head; i++) {
 		tx_skb = macb_tx_skb(queue, i);
 
-		macb_tx_unmap(bp, tx_skb, 0);
+		/* The reason parameter is unused, tx_skb->skb has not yet
+		 * been assigned. Parent caller is responsible for freeing
+		 * the SKB.
+		 */
+		macb_tx_unmap(bp, tx_skb, 0, SKB_DROP_REASON_NOT_SPECIFIED);
 	}
 
 	return -ENOMEM;

-- 
2.54.0

[PATCH net v2 2/4] net: macb: drop in-flight Tx SKBs on close

[Théo Lebrun]

The MACB driver has since forever leaked the outgoing SKBs that
have not yet been marked as completed. They live in queue->tx_skb
which gets freed without remorse nor checking.

macb_free_consistent() gets called in a few codepaths, but only
close will trigger the added expressions. In macb_open() and
macb_alloc_consistent() failure cases, tx_skb just got allocated
and is empty.

Use the new macb_tx_unmap() prototype to report our error as
SKB_DROP_REASON_NOT_SPECIFIED rather than SKB_CONSUMED which makes it
sound like no error occurred. Equivalent to dev_kfree_skb_any().

Fixes: 89e5785fc8a6 ("[PATCH] Atmel MACB ethernet driver")
Cc: stable@vger.kernel.org
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 9caae1ef52b1..5a2500bd59a6 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -2678,8 +2678,26 @@ static void macb_free_consistent(struct macb *bp)
 	dma_free_coherent(dev, size, bp->queues[0].rx_ring, bp->queues[0].rx_ring_dma);
 
 	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
-		kfree(queue->tx_skb);
-		queue->tx_skb = NULL;
+		if (queue->tx_skb) {
+			unsigned int dropped = 0, tail;
+
+			for (tail = queue->tx_tail; tail != queue->tx_head;
+			     tail++) {
+				if (macb_tx_skb(queue, tail)->skb)
+					dropped++;
+				macb_tx_unmap(bp, macb_tx_skb(queue, tail), 0,
+					      SKB_DROP_REASON_NOT_SPECIFIED);
+			}
+
+			queue->stats.tx_dropped += dropped;
+			bp->dev->stats.tx_dropped += dropped;
+
+			kfree(queue->tx_skb);
+			queue->tx_skb = NULL;
+		}
+
+		queue->tx_head = 0;
+		queue->tx_tail = 0;
 		queue->tx_ring = NULL;
 		queue->rx_ring = NULL;
 	}

-- 
2.54.0

[PATCH net v2 3/4] net: macb: increment stats.tx_dropped on tx error

[Théo Lebrun]

macb_tx_error_task() is the workqueue callback on Tx errors interrupts
(MACB_TX_ERR_FLAGS). Count number of errored SKBs and increment the Tx
dropped stat by that amount.

Two types of dropped (not consumed) packets:
 - Those that have been TX_USED but with an error.
 - Those that have not been TX_USED but that'll we drop to reset.

Fixes: 89e5785fc8a6 ("[PATCH] Atmel MACB ethernet driver")
Cc: stable@vger.kernel.org
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 5a2500bd59a6..ba7cbb10dc2c 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1261,6 +1261,7 @@ static void macb_tx_error_task(struct work_struct *work)
 	struct macb		*bp = queue->bp;
 	u32			queue_index;
 	u32			packets = 0;
+	unsigned int		dropped = 0;
 	u32			bytes = 0;
 	struct macb_tx_skb	*tx_skb;
 	struct macb_dma_desc	*desc;
@@ -1332,6 +1333,8 @@ static void macb_tx_error_task(struct work_struct *work)
 				queue->stats.tx_bytes += skb->len;
 				bytes += skb->len;
 				reason = SKB_CONSUMED;
+			} else {
+				dropped++;
 			}
 		} else {
 			/* "Buffers exhausted mid-frame" errors may only happen
@@ -1343,11 +1346,17 @@ static void macb_tx_error_task(struct work_struct *work)
 					   "BUG: TX buffers exhausted mid-frame\n");
 
 			desc->ctrl = ctrl | MACB_BIT(TX_USED);
+
+			if (skb)
+				dropped++;
 		}
 
 		macb_tx_unmap(bp, tx_skb, 0, reason);
 	}
 
+	queue->stats.tx_dropped += dropped;
+	bp->dev->stats.tx_dropped += dropped;
+
 	netdev_tx_completed_queue(netdev_get_tx_queue(bp->dev, queue_index),
 				  packets, bytes);
 

-- 
2.54.0

[PATCH net v2 4/4] net: macb: increment stats.tx_dropped on DMA map error

[Théo Lebrun]

On .ndo_start_xmit() and DMA mapping failure, increment the Tx dropped
statistics counter by one.

Fixes: 89e5785fc8a6 ("[PATCH] Atmel MACB ethernet driver")
Cc: stable@vger.kernel.org
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index ba7cbb10dc2c..353b4bd61aae 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -2567,6 +2567,8 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* Map socket buffer for DMA transfer */
 	if (macb_tx_map(bp, queue, skb, hdrlen)) {
 		dev_kfree_skb_any(skb);
+		queue->stats.tx_dropped++;
+		bp->dev->stats.tx_dropped++;
 		goto unlock;
 	}
 

-- 
2.54.0

Re: [PATCH net v2 1/4] net: macb: give reasons for Tx SKB kfree

[Nicolai Buchwitz]

On 28.4.2026 18:32, Théo Lebrun wrote:
> Using dev_consume_skb_any() marks the drop reason as SKB_CONSUMED every
> time we free a Tx SKB. Instead, replace by 
> SKB_DROP_REASON_NOT_SPECIFIED
> when packet has been dropped without sending.
> 
> It is not precise but at least differs from SKB_CONSUMED and is used by
> many drivers for their error codepaths through 
> dev_kfree_skb_{any,irq}().
> 
> Pass a reason around rather than call dev_consume_skb_any() or
> dev_kfree_skb_any() because macb_tx_unmap() is called for cleanup in
> all cases.
> 
> macb_tx_error_task() is made complex because some SKBs encountered have
> been successfully sent.
> 
> Fixes: 89e5785fc8a6 ("[PATCH] Atmel MACB ethernet driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---

> [...]

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Re: [PATCH net v2 3/4] net: macb: increment stats.tx_dropped on tx error

[Nicolai Buchwitz]

On 28.4.2026 18:32, Théo Lebrun wrote:
> macb_tx_error_task() is the workqueue callback on Tx errors interrupts
> (MACB_TX_ERR_FLAGS). Count number of errored SKBs and increment the Tx
> dropped stat by that amount.
> 
> Two types of dropped (not consumed) packets:
>  - Those that have been TX_USED but with an error.
>  - Those that have not been TX_USED but that'll we drop to reset.
> 
> Fixes: 89e5785fc8a6 ("[PATCH] Atmel MACB ethernet driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 9 +++++++++
>  1 file changed, 9 insertions(+)

> [...]

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Re: [PATCH net v2 4/4] net: macb: increment stats.tx_dropped on DMA map error

[Nicolai Buchwitz]

On 28.4.2026 18:33, Théo Lebrun wrote:
> On .ndo_start_xmit() and DMA mapping failure, increment the Tx dropped
> statistics counter by one.
> 
> Fixes: 89e5785fc8a6 ("[PATCH] Atmel MACB ethernet driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---

> [...]

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Re: [PATCH net v2 2/4] net: macb: drop in-flight Tx SKBs on close

[Nicolai Buchwitz]

On 28.4.2026 18:32, Théo Lebrun wrote:
> The MACB driver has since forever leaked the outgoing SKBs that
> have not yet been marked as completed. They live in queue->tx_skb
> which gets freed without remorse nor checking.
> 
> macb_free_consistent() gets called in a few codepaths, but only
> close will trigger the added expressions. In macb_open() and
> macb_alloc_consistent() failure cases, tx_skb just got allocated
> and is empty.
> 
> Use the new macb_tx_unmap() prototype to report our error as
> SKB_DROP_REASON_NOT_SPECIFIED rather than SKB_CONSUMED which makes it
> sound like no error occurred. Equivalent to dev_kfree_skb_any().
> 
> Fixes: 89e5785fc8a6 ("[PATCH] Atmel MACB ethernet driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c 
> b/drivers/net/ethernet/cadence/macb_main.c
> index 9caae1ef52b1..5a2500bd59a6 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -2678,8 +2678,26 @@ static void macb_free_consistent(struct macb 
> *bp)
>  	dma_free_coherent(dev, size, bp->queues[0].rx_ring, 
> bp->queues[0].rx_ring_dma);
> 
>  	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> -		kfree(queue->tx_skb);
> -		queue->tx_skb = NULL;
> +		if (queue->tx_skb) {
> +			unsigned int dropped = 0, tail;
> +
> +			for (tail = queue->tx_tail; tail != queue->tx_head;
> +			     tail++) {
> +				if (macb_tx_skb(queue, tail)->skb)
> +					dropped++;
> +				macb_tx_unmap(bp, macb_tx_skb(queue, tail), 0,
> +					      SKB_DROP_REASON_NOT_SPECIFIED);
> +			}

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Side note, not blocking: macb_close() doesn't cancel tx_error_task,
so the workqueue handler can race with this loop on tx_skb[]. The
exposure is pre-existing, but maybe worth a follow-up adding
cancel_work_sync() between napi_disable() and macb_free_consistent().

> [...]

Thanks,
Nicolai

Re: [PATCH net v2 2/4] net: macb: drop in-flight Tx SKBs on close

[Théo Lebrun]

Hello Nicolai,

On Tue Apr 28, 2026 at 11:30 PM CEST, Nicolai Buchwitz wrote:
> On 28.4.2026 18:32, Théo Lebrun wrote:
>> The MACB driver has since forever leaked the outgoing SKBs that
>> have not yet been marked as completed. They live in queue->tx_skb
>> which gets freed without remorse nor checking.
>> 
>> macb_free_consistent() gets called in a few codepaths, but only
>> close will trigger the added expressions. In macb_open() and
>> macb_alloc_consistent() failure cases, tx_skb just got allocated
>> and is empty.
>> 
>> Use the new macb_tx_unmap() prototype to report our error as
>> SKB_DROP_REASON_NOT_SPECIFIED rather than SKB_CONSUMED which makes it
>> sound like no error occurred. Equivalent to dev_kfree_skb_any().
>> 
>> Fixes: 89e5785fc8a6 ("[PATCH] Atmel MACB ethernet driver")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>> ---
>>  drivers/net/ethernet/cadence/macb_main.c | 22 ++++++++++++++++++++--
>>  1 file changed, 20 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c 
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index 9caae1ef52b1..5a2500bd59a6 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -2678,8 +2678,26 @@ static void macb_free_consistent(struct macb 
>> *bp)
>>  	dma_free_coherent(dev, size, bp->queues[0].rx_ring, 
>> bp->queues[0].rx_ring_dma);
>> 
>>  	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
>> -		kfree(queue->tx_skb);
>> -		queue->tx_skb = NULL;
>> +		if (queue->tx_skb) {
>> +			unsigned int dropped = 0, tail;
>> +
>> +			for (tail = queue->tx_tail; tail != queue->tx_head;
>> +			     tail++) {
>> +				if (macb_tx_skb(queue, tail)->skb)
>> +					dropped++;
>> +				macb_tx_unmap(bp, macb_tx_skb(queue, tail), 0,
>> +					      SKB_DROP_REASON_NOT_SPECIFIED);
>> +			}
>
> Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks for the review!
We are quite a few caring about MACB which is nice.

> Side note, not blocking: macb_close() doesn't cancel tx_error_task,
> so the workqueue handler can race with this loop on tx_skb[]. The
> exposure is pre-existing, but maybe worth a follow-up adding
> cancel_work_sync() between napi_disable() and macb_free_consistent().

Yes, noticed that while working on the context swapping series [0].
The goal here is to improve MACB piecewise, so I won't take that on in
the current series.

[0]: https://lore.kernel.org/all/90f843aa3940bdbabadddce27314c1f1@tipi-net.de/t/#mda18f759c27a4d833084b23605463994632d97e3
     (and the two replies)

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net v2 2/4] net: macb: drop in-flight Tx SKBs on close

[Nicolai Buchwitz]

Hi Théo

On 29.4.2026 11:26, Théo Lebrun wrote:
> [...]

>> Side note, not blocking: macb_close() doesn't cancel tx_error_task,
>> so the workqueue handler can race with this loop on tx_skb[]. The
>> exposure is pre-existing, but maybe worth a follow-up adding
>> cancel_work_sync() between napi_disable() and macb_free_consistent().
> 
> Yes, noticed that while working on the context swapping series [0].
> The goal here is to improve MACB piecewise, so I won't take that on in
> the current series.
> 
> [0]: 
> https://lore.kernel.org/all/90f843aa3940bdbabadddce27314c1f1@tipi-net.de/t/#mda18f759c27a4d833084b23605463994632d97e3
>      (and the two replies)

Yes, flagged it on that series too. Wasn't my intention to fold it
into this one either, just wanted to make sure it doesn't get lost
with so much in flight on macb right now, which I really appreciate!

> [...]

Cheers,
Nicolai

Re: [PATCH net v2 2/4] net: macb: drop in-flight Tx SKBs on close

[Jakub Kicinski]

On Tue, 28 Apr 2026 18:32:58 +0200 Théo Lebrun wrote:
>  	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> -		kfree(queue->tx_skb);
> -		queue->tx_skb = NULL;
> +		if (queue->tx_skb) {
> +			unsigned int dropped = 0, tail;
> +
> +			for (tail = queue->tx_tail; tail != queue->tx_head;
> +			     tail++) {
> +				if (macb_tx_skb(queue, tail)->skb)
> +					dropped++;
> +				macb_tx_unmap(bp, macb_tx_skb(queue, tail), 0,
> +					      SKB_DROP_REASON_NOT_SPECIFIED);
> +			}
> +
> +			queue->stats.tx_dropped += dropped;
> +			bp->dev->stats.tx_dropped += dropped;

I'm slightly baffled by the stats in this driver.

Incrementing of both device and queue stats is highly unusual.
The driver seems to already have the values for the per-queue drops
but currently never increments it (did I miss it?) It does for Rx
stats but not for Tx stats.

As sashiko correctly points out incrementing dev stats will lead
to races and lass of increments for multi-queue devices.

Since there are no increments for tx_dropped stat today - could you
please delete it from ethtool -S, migrate the only existing
dev->stats.tx_dropped++; to increment the per-queue stat and make 
macb_get_stats() collect the tx_dropped from all queues, instead
of relying on the device-level stat?

This should be patch 2 in this series, and then subsequent patches
don't have to do this double-counting dance.

I suppose you may want to migrate the byte and packet counters
while at it, and add a u64 sync...

Re: [PATCH net v2 2/4] net: macb: drop in-flight Tx SKBs on close

[Nicolai Buchwitz]

Hi Théo and Jacub

On 30.4.2026 04:34, Jakub Kicinski wrote:
> On Tue, 28 Apr 2026 18:32:58 +0200 Théo Lebrun wrote:
>>  	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
>> -		kfree(queue->tx_skb);
>> -		queue->tx_skb = NULL;
>> +		if (queue->tx_skb) {
>> +			unsigned int dropped = 0, tail;
>> +
>> +			for (tail = queue->tx_tail; tail != queue->tx_head;
>> +			     tail++) {
>> +				if (macb_tx_skb(queue, tail)->skb)
>> +					dropped++;
>> +				macb_tx_unmap(bp, macb_tx_skb(queue, tail), 0,
>> +					      SKB_DROP_REASON_NOT_SPECIFIED);
>> +			}
>> +
>> +			queue->stats.tx_dropped += dropped;
>> +			bp->dev->stats.tx_dropped += dropped;
> 
> I'm slightly baffled by the stats in this driver.
> 
> Incrementing of both device and queue stats is highly unusual.
> The driver seems to already have the values for the per-queue drops
> but currently never increments it (did I miss it?) It does for Rx
> stats but not for Tx stats.
> 
> As sashiko correctly points out incrementing dev stats will lead
> to races and lass of increments for multi-queue devices.
> 
> Since there are no increments for tx_dropped stat today - could you
> please delete it from ethtool -S, migrate the only existing
> dev->stats.tx_dropped++; to increment the per-queue stat and make
> macb_get_stats() collect the tx_dropped from all queues, instead
> of relying on the device-level stat?

Would make sense, yes. While we're already cleaning this up, two
more things possibly worth touching:

1. macb_start_xmit() drops the skb on macb_clear_csum() and
    macb_pad_and_fcs() failures without counting it. Both could
    use a tx_dropped++.

2. tx_packets / tx_bytes already increment per-queue but never
    rach nstat (rx side too). Could just pick them up in the same
    loop.

> [...]

Thanks,
Nicolai

Re: [PATCH net v2 2/4] net: macb: drop in-flight Tx SKBs on close

[Théo Lebrun]

Hello Jakub,

On Thu Apr 30, 2026 at 4:34 AM CEST, Jakub Kicinski wrote:
> On Tue, 28 Apr 2026 18:32:58 +0200 Théo Lebrun wrote:
>>  	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
>> -		kfree(queue->tx_skb);
>> -		queue->tx_skb = NULL;
>> +		if (queue->tx_skb) {
>> +			unsigned int dropped = 0, tail;
>> +
>> +			for (tail = queue->tx_tail; tail != queue->tx_head;
>> +			     tail++) {
>> +				if (macb_tx_skb(queue, tail)->skb)
>> +					dropped++;
>> +				macb_tx_unmap(bp, macb_tx_skb(queue, tail), 0,
>> +					      SKB_DROP_REASON_NOT_SPECIFIED);
>> +			}
>> +
>> +			queue->stats.tx_dropped += dropped;
>> +			bp->dev->stats.tx_dropped += dropped;
>
> I'm slightly baffled by the stats in this driver.
>
> Incrementing of both device and queue stats is highly unusual.
> The driver seems to already have the values for the per-queue drops
> but currently never increments it (did I miss it?) It does for Rx
> stats but not for Tx stats.
>
> As sashiko correctly points out incrementing dev stats will lead
> to races and lass of increments for multi-queue devices.
>
> Since there are no increments for tx_dropped stat today - could you
> please delete it from ethtool -S, migrate the only existing
> dev->stats.tx_dropped++; to increment the per-queue stat and make 
> macb_get_stats() collect the tx_dropped from all queues, instead
> of relying on the device-level stat?
>
> This should be patch 2 in this series, and then subsequent patches
> don't have to do this double-counting dance.
>
> I suppose you may want to migrate the byte and packet counters
> while at it, and add a u64 sync...

Agreed. Here is the plan for next revision. It goes further than your
proposal on some aspects and less so on others.

 - Stop using `netdev->stats`. Not even on MACB (single queue) or from
   at91 code (single queue, custom functions for a lot of things). This
   will drop all the double-counting; I added some in this series but
   there is lot in the driver to drop.

   sed 's/netdev->stats/queue->stats/' **/macb_main.c  # -ish

 - All stats that used to land in netdev->stats will instead land in
   queue->stats. For that we need to add two fields:
    - multicast, incremented by at91ether_rx()
    - tx_errors, incremented by at91ether_interrupt()

 - Make queue->stats u64 values (getting inspiration from nstat).

 - In macb_get_stats(), replace:

      netdev_stats_to_stats64(nstat, &bp->dev->stats);

   by:

      for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
         u64_stats_fetch_begin(...);
         nstat->rx_packets += queue->stats.rx_packets;
         nstat->tx_packets += queue->stats.tx_packets;
         // ... same for all stats ...
      }

 - Also the struct name (struct queue_stats) deserves a driver prefix.

Notice we don't drop tx_dropped from `ethtool -S`. It might be useful to
get per-queue stats and it doesn't cost much. We need per-queue
counters anyway, let's keep exposing them.

I don't have time to test enough, next revision will wait next week.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net v2 2/4] net: macb: drop in-flight Tx SKBs on close

[Jakub Kicinski]

On Thu, 30 Apr 2026 18:20:01 +0200 Théo Lebrun wrote:
>  - In macb_get_stats(), replace:
> 
>       netdev_stats_to_stats64(nstat, &bp->dev->stats);
> 
>    by:
> 
>       for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
>          u64_stats_fetch_begin(...);
>          nstat->rx_packets += queue->stats.rx_packets;
>          nstat->tx_packets += queue->stats.tx_packets;

you'd probably catch this when doing the real implementation but beware
of updating nstat directly in the fetch loop since the loop may retry

>          // ... same for all stats ...
>       }
> 
>  - Also the struct name (struct queue_stats) deserves a driver prefix.
> 
> Notice we don't drop tx_dropped from `ethtool -S`. It might be useful to
> get per-queue stats and it doesn't cost much. We need per-queue
> counters anyway, let's keep exposing them.

There's a dedicated API now for exposing pre-queue stats. 
Since tx_dropped was always zero we can as well delete it from ethtool
-S and think about adding netdev queue stats in net-next