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

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

[justin.chen]

From: Justin Chen <justin.chen@broadcom.com>

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>
Assisted-by: Claude:claude-sonnet-5
---
 .../net/ethernet/broadcom/asp2/bcmasp_intf.c  | 68 +++++++++++--------
 1 file changed, 40 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..00e39a230676 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,18 @@ 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_tx_stop_queue(txq);
 		if (net_ratelimit())
 			netdev_err(dev, "Tx Ring Full!\n");
 		return NETDEV_TX_BUSY;
@@ -333,15 +333,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 +400,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 +427,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 +440,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 +858,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 +956,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] 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] net: bcmasp: fix lost TX wakeup race with lockless queue API

[Florian Fainelli]

On 9/28/26 13:18, justin.chen@broadcom.com wrote:
> From: Justin Chen <justin.chen@broadcom.com>
> 
> 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>
> Assisted-by: Claude:claude-sonnet-5

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

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

[Nicolai Buchwitz]

Hi Justin

On 28.9.2026 22:18, justin.chen@broadcom.com wrote:
> From: Justin Chen <justin.chen@broadcom.com>
> 
> 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>
> Assisted-by: Claude:claude-sonnet-5
> ---
>  .../net/ethernet/broadcom/asp2/bcmasp_intf.c  | 68 +++++++++++--------

> [...]

>  static struct sk_buff *bcmasp_csum_offload(struct net_device *dev,
> @@ -241,16 +239,18 @@ 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_tx_stop_queue(txq);

nit: AFAIU this is unreachable for now, but to prevent future races 
maybe
better use netif_txq_try_stop()?

> [...]

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

Thanks,
Nicolai

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

[Justin Chen]

On 9/28/26 2:35 PM, Nicolai Buchwitz wrote:
> Hi Justin
> 
> On 28.9.2026 22:18, justin.chen@broadcom.com wrote:
>> From: Justin Chen <justin.chen@broadcom.com>
>>
>> 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>
>> Assisted-by: Claude:claude-sonnet-5
>> ---
>>  .../net/ethernet/broadcom/asp2/bcmasp_intf.c  | 68 +++++++++++--------
> 
>> [...]
> 
>>  static struct sk_buff *bcmasp_csum_offload(struct net_device *dev,
>> @@ -241,16 +239,18 @@ 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_tx_stop_queue(txq);
> 
> nit: AFAIU this is unreachable for now, but to prevent future races maybe
> better use netif_txq_try_stop()?

Will use netif_txq_try_stop() in v2. My understanding is this is still 
reachable since the maybe_stop call at the end of the xmit may have 
false wake ups when the ring is full. That is what is documented in the 
function header description for netif_txq_try_stop().

Thanks,
Justin
> 
>> [...]
> 
> Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
> 
> Thanks,
> Nicolai

Re: [PATCH net] 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: 1 · Medium: 0 (2) · 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 is incomplete.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but in
  bcmasp_xmit() the dma_mapping_error() unwind leaks DMA mappings.
- [Medium] This problem wasn't introduced by this patch, but TX and RX
  update the same seqcount (intf->stats64.syncp) without being mutually…

--

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

This patch moves bcmasp TX flow control from netif_stop_queue() and
netif_wake_queue() to the lockless netif_txq_maybe_stop() and
netif_txq_completed_wake() helpers, which also adds BQL accounting.
tx_spb_ring_full() is replaced by a live bcmasp_tx_avail() count.

> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> index f2176ef3a127..00e39a230676 100644
> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c

[ ... ]

> @@ -241,16 +239,18 @@ static netdev_tx_t bcmasp_xmit(struct sk_buff *skb, struct net_device *dev)

[ ... ]

>  	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_tx_stop_queue(txq);
>  		if (net_ratelimit())
>  			netdev_err(dev, "Tx Ring Full!\n");
>  		return NETDEV_TX_BUSY;

[Severity: High]
Can the lost wakeup still happen through this ring-full pre-check in
bcmasp_xmit()?

This path still calls a bare netif_tx_stop_queue() and returns
NETDEV_TX_BUSY. After it sets __QUEUE_STATE_DRV_XOFF there is no
smp_mb__after_atomic() and no second look at bcmasp_tx_avail(). Only the
stop at the end of bcmasp_xmit() was converted to netif_txq_maybe_stop().

The pre-check looks reachable. include/net/netdev_queues.h says:

    The macros guarantee that rings will not remain stopped if there's
    space available, but they do *not* prevent false wake ups when the
    ring is full! Drivers should check for ring full at the start for
    the xmit handler.

In bcmasp_tx_poll()->netif_txq_completed_wake(), bcmasp_tx_avail() reads
tx_spb_index before netif_tx_queue_stopped() is checked. A stale producer
index can therefore wake the queue while the ring is nearly full. The next
bcmasp_xmit() with enough frags then takes this branch, and this can
happen:

CPU0 (bcmasp_xmit)                  CPU1 (bcmasp_tx_poll)
bcmasp_tx_avail() < nr_frags + 1
                                    bcmasp_tx_reclaim() frees the rest
                                    netif_txq_completed_wake()
                                      smp_mb()
                                      queue not stopped, no wake
netif_tx_stop_queue(txq)
return NETDEV_TX_BUSY

That looks like the same window the commit message describes as "a
reclaim landing between the ring-full check and the stop call".

After this, no descriptors are in flight, so no TX interrupt comes to wake
the queue. bcmasp_tx_timeout() only does:

    intf->mib.tx_timeout_cnt++;

so TX would stay stuck until the interface is restarted.

Could this branch use the stop/re-check protocol, as bnxt_start_xmit()
does? For example:

    if (!netif_txq_try_stop(txq, bcmasp_tx_avail(intf),
                            BCMASP_TX_START_THRS))
        return NETDEV_TX_BUSY;

[ ... ]

> @@ -333,15 +333,17 @@ static netdev_tx_t bcmasp_xmit(struct sk_buff *skb, struct net_device *dev)
>  	 */
>  	wmb();

[Severity: Medium]
This isn't a bug introduced by this patch, but does the dma_mapping_error()
unwind in the descriptor loop just above leak the DMA mappings that already
succeeded?

bcmasp_xmit() {
    ...
		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;
		}
    ...
}

bcmasp_clean_txcb() only clears txcb->skb, dma_addr, dma_len and last. It
never calls dma_unmap_single() or dma_unmap_page().

If skb_frag_dma_map() fails for a fragment with i > 0, the linear head
(mapped with dma_map_single()) and frags 1..i-1 stay mapped, and then the
skb is freed.

Can this leak IOVA or swiotlb slots and leave DMA_TO_DEVICE mappings that
point at freed memory?

[ ... ]

> @@ -423,6 +427,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);

[Severity: Medium]
This is a pre-existing issue, but can the TX and RX paths write
intf->stats64.syncp at the same time?

bcmasp_tx_reclaim() updates tx_packets and tx_bytes under stats->syncp.
bcmasp_rx_poll() updates rx_packets and rx_bytes under the same syncp:

bcmasp_rx_poll() {
    ...
		u64_stats_update_begin(&stats->syncp);
		u64_stats_inc(&stats->rx_packets);
		u64_stats_add(&stats->rx_bytes, len);
		u64_stats_update_end(&stats->syncp);
    ...
}

These run from separate NAPI instances. In bcmasp_netif_deinit(),
bcmasp_tx_reclaim() runs in process context while rx_napi is still active:

bcmasp_netif_deinit() {
	napi_disable(&intf->tx_napi);
	...
	bcmasp_tx_reclaim(intf, NULL);
	...
	bcmasp_enable_rx(intf, 0);

	napi_disable(&intf->rx_napi);
    ...
}

The two writers can also overlap at runtime with threaded NAPI, or when
IRQ affinity changes while one NAPI is still repolling.

u64_stats_sync writers have to be mutually exclusive on 32-bit kernels,
and ARCH_BRCMSTB includes arm. Could this let bcmasp_get_stats64() read
torn counters?

If both writers hit the sequence increment at the same moment, could one
increment be lost and the sequence left odd? Readers would then spin in
u64_stats_fetch_begin().

[ ... ]

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