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

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.

[ ... ]

[ ... ]

[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;

[ ... ]

[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?

[ ... ]

[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