[PATCH net v2] net: bcmasp: fix lost TX wakeup race with lockless queue API

[PATCH net v2] net: bcmasp: fix lost TX wakeup race with lockless queue API

[Justin Chen]

netif_stop_queue() in bcmasp_xmit() raced with netif_wake_queue() in
bcmasp_tx_poll(): a reclaim landing between the ring-full check and
the stop call left the queue stopped despite free descriptors,
hanging TX until timeout.

Switch to netif_txq_maybe_stop()/netif_txq_completed_wake() (the
lockless TX queue API, which also brings in BQL), replace
tx_spb_ring_full() with a live bcmasp_tx_avail() count.

Fixes: 490cb412007d ("net: bcmasp: Add support for ASP2.0 Ethernet controller")
Signed-off-by: Justin Chen <justin.chen@broadcom.com>
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Assisted-by: Claude:claude-sonnet-5
---
v2
	- replaced netif_tx_stop_queue() with netif_txq_try_stop()

 .../net/ethernet/broadcom/asp2/bcmasp_intf.c  | 69 +++++++++++--------
 1 file changed, 41 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
index f2176ef3a127..3369c45b49d5 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
@@ -15,11 +15,15 @@
 #include <linux/platform_device.h>
 #include <net/ip.h>
 #include <net/ipv6.h>
+#include <net/netdev_queues.h>
 #include <net/page_pool/helpers.h>
 
 #include "bcmasp.h"
 #include "bcmasp_intf_defs.h"
 
+#define BCMASP_TX_STOP_THRS	(MAX_SKB_FRAGS + 1)
+#define BCMASP_TX_START_THRS	(2 * MAX_SKB_FRAGS)
+
 static int incr_ring(int index, int ring_count)
 {
 	index++;
@@ -143,19 +147,13 @@ static void bcmasp_clean_txcb(struct bcmasp_intf *intf, int index)
 	txcb->last = false;
 }
 
-static int tx_spb_ring_full(struct bcmasp_intf *intf, int cnt)
+static int bcmasp_tx_avail(struct bcmasp_intf *intf)
 {
-	int next_index, i;
-
-	/* Check if we have enough room for cnt descriptors */
-	next_index = intf->tx_spb_index;
-	for (i = 0; i < cnt; i++) {
-		next_index = incr_ring(next_index, DESC_RING_COUNT);
-		if (next_index == intf->tx_spb_clean_index)
-			return 1;
-	}
+	int used = (READ_ONCE(intf->tx_spb_index) -
+		    READ_ONCE(intf->tx_spb_clean_index) + DESC_RING_COUNT) %
+		   DESC_RING_COUNT;
 
-	return 0;
+	return DESC_RING_COUNT - used - 1;
 }
 
 static struct sk_buff *bcmasp_csum_offload(struct net_device *dev,
@@ -241,16 +239,19 @@ static netdev_tx_t bcmasp_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct bcmasp_tx_cb *txcb;
 	dma_addr_t mapping, valid;
 	struct bcmasp_desc *desc;
+	struct netdev_queue *txq;
 	bool csum_hw = false;
 	struct device *kdev;
 	skb_frag_t *frag;
 
 	kdev = &intf->parent->pdev->dev;
+	txq = netdev_get_tx_queue(dev, 0);
 
 	nr_frags = skb_shinfo(skb)->nr_frags;
 
-	if (tx_spb_ring_full(intf, nr_frags + 1)) {
-		netif_stop_queue(dev);
+	if (unlikely(bcmasp_tx_avail(intf) < nr_frags + 1)) {
+		netif_txq_try_stop(txq, bcmasp_tx_avail(intf),
+				   BCMASP_TX_START_THRS);
 		if (net_ratelimit())
 			netdev_err(dev, "Tx Ring Full!\n");
 		return NETDEV_TX_BUSY;
@@ -333,15 +334,17 @@ static netdev_tx_t bcmasp_xmit(struct sk_buff *skb, struct net_device *dev)
 	 */
 	wmb();
 
-	intf->tx_spb_index = spb_index;
+	WRITE_ONCE(intf->tx_spb_index, spb_index);
 	intf->tx_spb_dma_valid = valid;
 
+	netdev_tx_sent_queue(txq, total_bytes);
+
 	skb_tx_timestamp(skb);
 
 	tx_spb_dma_wq(intf, intf->tx_spb_dma_valid, TX_SPB_DMA_VALID);
 
-	if (tx_spb_ring_full(intf, MAX_SKB_FRAGS + 1))
-		netif_stop_queue(dev);
+	netif_txq_maybe_stop(txq, bcmasp_tx_avail(intf),
+			     BCMASP_TX_STOP_THRS, BCMASP_TX_START_THRS);
 
 	return NETDEV_TX_OK;
 }
@@ -398,14 +401,16 @@ static void umac_enable_set(struct bcmasp_intf *intf, u32 mask,
 		usleep_range(1000, 2000);
 }
 
-static int bcmasp_tx_reclaim(struct bcmasp_intf *intf)
+static int bcmasp_tx_reclaim(struct bcmasp_intf *intf, unsigned int *bytes_out)
 {
 	struct bcmasp_intf_stats64 *stats = &intf->stats64;
 	struct device *kdev = &intf->parent->pdev->dev;
-	unsigned long read, released = 0;
+	unsigned int bytes_compl = 0;
 	struct bcmasp_tx_cb *txcb;
 	struct bcmasp_desc *desc;
+	unsigned long read;
 	dma_addr_t mapping;
+	int packets = 0;
 
 	read = tx_spb_dma_rq(intf, TX_SPB_DMA_READ);
 	while (intf->tx_spb_dma_read != read) {
@@ -423,6 +428,9 @@ static int bcmasp_tx_reclaim(struct bcmasp_intf *intf)
 			u64_stats_inc(&stats->tx_packets);
 			u64_stats_add(&stats->tx_bytes, txcb->bytes_sent);
 			u64_stats_update_end(&stats->syncp);
+
+			bytes_compl += txcb->bytes_sent;
+			packets++;
 		}
 
 		desc = &intf->tx_spb_cpu[intf->tx_spb_clean_index];
@@ -433,33 +441,37 @@ static int bcmasp_tx_reclaim(struct bcmasp_intf *intf)
 			  intf->tx_spb_clean_index);
 
 		bcmasp_clean_txcb(intf, intf->tx_spb_clean_index);
-		released++;
 
-		intf->tx_spb_clean_index = incr_ring(intf->tx_spb_clean_index,
-						     DESC_RING_COUNT);
+		WRITE_ONCE(intf->tx_spb_clean_index,
+			   incr_ring(intf->tx_spb_clean_index, DESC_RING_COUNT));
 		intf->tx_spb_dma_read = incr_first_byte(intf->tx_spb_dma_read,
 							intf->tx_spb_dma_addr,
 							DESC_RING_COUNT);
 	}
 
-	return released;
+	if (bytes_out)
+		*bytes_out = bytes_compl;
+
+	return packets;
 }
 
 static int bcmasp_tx_poll(struct napi_struct *napi, int budget)
 {
 	struct bcmasp_intf *intf =
 		container_of(napi, struct bcmasp_intf, tx_napi);
-	int released = 0;
+	struct netdev_queue *txq = netdev_get_tx_queue(intf->ndev, 0);
+	unsigned int bytes = 0;
+	int packets;
 
-	released = bcmasp_tx_reclaim(intf);
+	packets = bcmasp_tx_reclaim(intf, &bytes);
+
+	netif_txq_completed_wake(txq, packets, bytes,
+				 bcmasp_tx_avail(intf), BCMASP_TX_START_THRS);
 
 	napi_complete(&intf->tx_napi);
 
 	bcmasp_enable_tx_irq(intf, 1);
 
-	if (released)
-		netif_wake_queue(intf->ndev);
-
 	return 0;
 }
 
@@ -847,6 +859,7 @@ static void bcmasp_init_tx(struct bcmasp_intf *intf)
 	intf->tx_spb_index = 0;
 	intf->tx_spb_clean_index = 0;
 	memset(intf->tx_cbs, 0, sizeof(struct bcmasp_tx_cb) * DESC_RING_COUNT);
+	netdev_tx_reset_queue(netdev_get_tx_queue(intf->ndev, 0));
 
 	/* Make sure channels are disabled */
 	tx_spb_ctrl_wl(intf, 0x0, TX_SPB_CTRL_ENABLE);
@@ -944,7 +957,7 @@ static void bcmasp_netif_deinit(struct net_device *dev, bool stop_phy)
 	} while (timeout-- > 0);
 	tx_spb_dma_wl(intf, 0x0, TX_SPB_DMA_FIFO_CTRL);
 
-	bcmasp_tx_reclaim(intf);
+	bcmasp_tx_reclaim(intf, NULL);
 
 	umac_enable_set(intf, UMC_CMD_TX_EN, 0);
 
-- 
2.34.1

Re: [PATCH net v2] net: bcmasp: fix lost TX wakeup race with lockless queue API

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net v2] net: bcmasp: fix lost TX wakeup race with lockless queue API

[Justin Chen]

On 9/29/26 1:39 PM, netdev-bot+sinfo@kernel.org wrote:
> Hi!
> 
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
> 
>   - How the issue was discovered, e.g. hit in production, hit during
>     development, syzbot report, manual code inspection, LLM or static
>     analysis tool scan.
> 
>   - Whether the issue was actually triggered, or is only theoretical
>     (e.g. found by code inspection). If it was triggered please include
>     the symptoms, like the stack trace or error messages.
> 
>   - What hardware the change was tested on. For driver fixes please
>     mention the device (and if relevant firmware version) used for
>     testing, or say that the change was not tested on real hardware.
> 
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
> 
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

Apologies for ignoring these. Just found out they were landing in my 
spam box...

This issue was discovered using LLM and is only theoretical. I tested on 
a BCM74110 that no regressions were encountered.

Thanks,
Justin

Re: [PATCH net v2] net: bcmasp: fix lost TX wakeup race with lockless queue API

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (3) · 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).

- [Medium] New BQL accounting in bcmasp_xmit() is not ordered before the
  TX doorbell.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the DMA
  mapping-error unwind in bcmasp_xmit() leaks the DMA mappings already…
- [Medium] This problem wasn't introduced by this patch, but
  intf->stats64.syncp has TX and RX writers that are not mutually…
- [Medium] This problem wasn't introduced by this patch, but
  bcmasp_interface_suspend() stops TX with netif_device_detach() alone…

--

net: bcmasp: fix lost TX wakeup race with lockless queue API

This patch fixes a lost-wakeup race between netif_stop_queue() in
bcmasp_xmit() and netif_wake_queue() in bcmasp_tx_poll(). It switches both
paths to the netif_txq_maybe_stop()/netif_txq_completed_wake() helpers.
It also replaces tx_spb_ring_full() with a bcmasp_tx_avail() count and
adds BQL accounting.

> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> index f2176ef3a127b..3369c45b49d5b 100644
> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c

[ ... ]

> @@ -143,19 +147,13 @@ static void bcmasp_clean_txcb(struct bcmasp_intf *intf, int index)
>  	txcb->last = false;
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but bcmasp_clean_txcb() only
clears the tx_cbs[] metadata. It is also the only cleanup that the DMA
mapping error path in bcmasp_xmit() does for entries that are already
mapped:

		if (dma_mapping_error(kdev, mapping)) {
			intf->mib.tx_dma_failed++;
			spb_index = intf->tx_spb_index;
			for (j = 0; j < i; j++) {
				bcmasp_clean_txcb(intf, spb_index);
				spb_index = incr_ring(spb_index,
						      DESC_RING_COUNT);
			}
			/* Rewind so we do not have a hole */
			spb_index = intf->tx_spb_index;
			dev_kfree_skb(skb);
			return NETDEV_TX_OK;
		}

If a later fragment fails to map (i > 0), the head and the earlier
fragments stay mapped. Their dma_addr and dma_len are zeroed, and
tx_spb_index was never advanced. That means bcmasp_tx_reclaim() can't
unmap them later either.

Does this leak the DMA mappings (swiotlb slots or IOVA space) for those
entries?

[ ... ]

> @@ -333,15 +334,17 @@ static netdev_tx_t bcmasp_xmit(struct sk_buff *skb, struct net_device *dev)
>  	 */
>  	wmb();
>  
> -	intf->tx_spb_index = spb_index;
> +	WRITE_ONCE(intf->tx_spb_index, spb_index);
>  	intf->tx_spb_dma_valid = valid;
>  
> +	netdev_tx_sent_queue(txq, total_bytes);
> +
>  	skb_tx_timestamp(skb);
>  
>  	tx_spb_dma_wq(intf, intf->tx_spb_dma_valid, TX_SPB_DMA_VALID);

[Severity: Medium]
Is the new BQL accounting ordered before the doorbell write?

The wmb() above only orders stores made before it, and
netdev_tx_sent_queue() now comes after it. tx_spb_dma_wq() is a relaxed
MMIO write, from bcmasp.h:

	writeq_relaxed(val, intf->m + off);

When dql_avail() >= 0, dql_queued() does a plain store to
dql->num_queued with no CPU barrier.

On arm64, could the device see the new TX_SPB_DMA_VALID, transmit, and
raise the TX interrupt before that store is visible to another CPU?

In that case bcmasp_tx_poll() on the other CPU would reach:

bcmasp_tx_poll()
  netif_txq_completed_wake()
    netdev_tx_completed_queue()
      dql_completed()
        num_queued = READ_ONCE(dql->num_queued);
        ...
        BUG_ON(count > num_queued - dql->num_completed);

If only this packet is in flight, the stale num_queued would not include
total_bytes, and the BUG_ON() would fire. The smp_mb() in the completion
helper comes after dql_completed(), so it doesn't help here.

For this to happen, the store would have to stay invisible across the
whole DMA, transmit and interrupt round trip, so it may be mostly
theoretical. Would calling netdev_tx_sent_queue() before the wmb() close
the window?

[ ... ]

> @@ -433,33 +441,37 @@ static int bcmasp_tx_reclaim(struct bcmasp_intf *intf)

[ ... ]

>  static int bcmasp_tx_poll(struct napi_struct *napi, int budget)
>  {
>  	struct bcmasp_intf *intf =
>  		container_of(napi, struct bcmasp_intf, tx_napi);
> -	int released = 0;
> +	struct netdev_queue *txq = netdev_get_tx_queue(intf->ndev, 0);
> +	unsigned int bytes = 0;
> +	int packets;
>  
> -	released = bcmasp_tx_reclaim(intf);
> +	packets = bcmasp_tx_reclaim(intf, &bytes);
> +
> +	netif_txq_completed_wake(txq, packets, bytes,
> +				 bcmasp_tx_avail(intf), BCMASP_TX_START_THRS);

[Severity: Medium]
This is a pre-existing issue, but can this wake the queue after
bcmasp_interface_suspend() has detached the device?

bcmasp_interface_suspend() stops TX with only netif_device_detach():

	netif_device_detach(dev);

	wake = device_may_wakeup(kdev) && intf->wolopts;

	bcmasp_netif_deinit(dev, !wake);

netif_device_detach() sets DRV_XOFF without taking the TX lock, and the
qdisc stays active during suspend.

bcmasp_tx_poll() can keep running until napi_disable(&intf->tx_napi) in
bcmasp_netif_deinit(). Here it calls netif_txq_completed_wake() with
down_cond false, which calls netif_tx_wake_queue() once
avail >= BCMASP_TX_START_THRS.

The old netif_wake_queue() had the same race. This patch adds a second
restart point: netif_txq_try_stop() in bcmasp_xmit() calls
netif_tx_start_queue() when a concurrent reclaim frees enough
descriptors.

Once the queue is running again, bcmasp_xmit() could race with
bcmasp_tx_reclaim() in bcmasp_netif_deinit(). It could also write
TX_SPB_DMA_VALID after the block's clocks are gated. Any skbs queued this
way would be lost when bcmasp_init_tx() memsets tx_cbs[] on resume.

Should the suspend path synchronize with bcmasp_xmit() and tx_napi before
tearing down the TX ring?

[ ... ]

> @@ -944,7 +957,7 @@ static void bcmasp_netif_deinit(struct net_device *dev, bool stop_phy)
>  	} while (timeout-- > 0);
>  	tx_spb_dma_wl(intf, 0x0, TX_SPB_DMA_FIFO_CTRL);
>  
> -	bcmasp_tx_reclaim(intf);
> +	bcmasp_tx_reclaim(intf, NULL);

[Severity: Medium]
This isn't a bug introduced by this patch, but is intf->stats64.syncp
safe to update from here?

bcmasp_netif_deinit() disables only tx_napi before it calls
bcmasp_tx_reclaim() from sleepable process context. rx_napi stays enabled
until later:

bcmasp_netif_deinit()
    napi_disable(&intf->tx_napi);
    ...
    bcmasp_tx_reclaim(intf, NULL);
        u64_stats_update_begin(&stats->syncp);
        ...
    ...
    napi_disable(&intf->rx_napi);

bcmasp_rx_poll() updates the rx_* counters under the same syncp.

On 32-bit kernels, u64_stats_update_begin() expects preemption to be
disabled, and on non-RT kernels preempt_disable_nested() is only a lockdep
assertion. It also relies on writers being mutually exclusive.

Could this trigger a lockdep warning on interface down or suspend?

Could a concurrent RX NAPI poll race the seqcount increment? The same
question applies to threaded NAPI running the TX and RX polls on different
CPUs.

A lost increment would leave the sequence odd, and the
u64_stats_fetch_retry() loop in bcmasp_get_stats64() would then spin
forever.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929203400.1588868-1-justin.chen%40broadcom.com

Re: [PATCH net v2] net: bcmasp: fix lost TX wakeup race with lockless queue API

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 29 Sep 2026 13:34:00 -0700 you wrote:
> netif_stop_queue() in bcmasp_xmit() raced with netif_wake_queue() in
> bcmasp_tx_poll(): a reclaim landing between the ring-full check and
> the stop call left the queue stopped despite free descriptors,
> hanging TX until timeout.
> 
> Switch to netif_txq_maybe_stop()/netif_txq_completed_wake() (the
> lockless TX queue API, which also brings in BQL), replace
> tx_spb_ring_full() with a live bcmasp_tx_avail() count.
> 
> [...]

Here is the summary with links:
  - [net,v2] net: bcmasp: fix lost TX wakeup race with lockless queue API
    https://git.kernel.org/netdev/net-next/c/ea311fea3236

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html