Re: [PATCH net 1/7] net: bcm4908_enet: fix the TX ring wrap

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 1

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] The early ring-full path at the top of
  bcm4908_enet_start_xmit() (`if…
- [Low] The commit message leaves out a second symptom of the same index
  mismatch, which this patch also fixes.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch's
  new stop/wake logic still depends on it working.

--

net: bcm4908_enet: fix the TX ring wrap

The TX ring now uses all descriptors, with WRAP on the last one. Both
read_idx and write_idx wrap at length, and one descriptor is always left
free, so read_idx == write_idx can only mean an empty ring.
bcm4908_enet_start_xmit() now stops the queue once the ring is full.
bcm4908_enet_poll_tx() wakes it again, with smp_mb() on both sides.

> diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
> index fd2715f84af97..01a3e1d0192f8 100644
> --- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
> +++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c

[ ... ]

> @@ -525,20 +535,14 @@ static netdev_tx_t bcm4908_enet_start_xmit(struct sk_buff *skb, struct net_devic

[ ... ]

>  	/* Free transmitted skbs */
>  	if (enet->irq_tx < 0 &&
> -	    !(le32_to_cpu(ring->buf_desc[ring->read_idx].ctl) & DMA_CTL_STATUS_OWN))
> +	    !(le32_to_cpu(ring->buf_desc[READ_ONCE(ring->read_idx)].ctl) & DMA_CTL_STATUS_OWN))
>  		napi_schedule(&enet->tx_ring.napi);

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but
does the new stop/wake logic work when the optional TX interrupt is
missing?

In Documentation/devicetree/bindings/net/brcm,bcm4908-enet.yaml, both
interrupts and interrupt-names have minItems: 1. A DT node that lists
only "rx" is therefore valid, and probe carries on with
enet->irq_tx < 0.

In that case bcm4908_enet_open() never enables the TX NAPI:

	if (enet->irq_tx > 0) {
		napi_enable(&tx_ring->napi);

The TX NAPI still has NAPI_STATE_SCHED set from netif_napi_add, so
napi_schedule() here always fails in napi_schedule_prep(), and
bcm4908_enet_poll_tx() never runs.

Nothing then advances read_idx, unmaps or frees the TX skbs, or wakes
the queue. After length - 1 frames, the new stop-on-full branch at the
end of bcm4908_enet_start_xmit() stops the queue. Nothing wakes it
again, although the new comment there says "poll_tx() wakes the queue
once there is room".

On ifdown, bcm4908_enet_stop() calls napi_disable(&tx_ring->napi) on
the NAPI that was never enabled. Wouldn't napi_disable_locked() then
spin here forever while holding RTNL and the netdev instance lock?

		while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
			usleep_range(20, 200);
			val = READ_ONCE(n->state);
		}

bcm4908_enet_stop() also calls free_irq(enet->irq_tx, enet) with a
negative IRQ.

This goes back to 12bb508bfe5a ("net: broadcom: bcm4908_enet: support
TX interrupt"). The in-tree bcm4908.dtsi provides both "rx" and "tx",
so only DTs that use the binding's optional-TX form are affected.

>  
> -	/* Don't use the last empty buf descriptor */
> -	if (ring->read_idx <= ring->write_idx)
> -		free_buf_descs = ring->read_idx - ring->write_idx + ring->length;
> -	else
> -		free_buf_descs = ring->read_idx - ring->write_idx;
> -	if (free_buf_descs < 2) {
> +	if (unlikely(bcm4908_enet_tx_free_descs(ring) < 1)) {
>  		netif_stop_queue(netdev);
>  		return NETDEV_TX_BUSY;
>  	}

[Severity: Medium]
Can this early ring-full path in bcm4908_enet_start_xmit() lose a
wakeup?

It stops the queue without the smp_mb() and re-check that the patch
adds at the end of bcm4908_enet_start_xmit(). include/net/netdev_queues.h
says the standard stop/wake pairing does not prevent false wake ups
when the ring is full, and that drivers should check for ring full at
the start of the xmit handler. So this path can be reached.

Here is one way to get a false wake:

  - bcm4908_enet_poll_tx() finds netif_queue_stopped() true and free
    descriptors >= 1.
  - Meanwhile the stop/smp_mb/re-check at the end of start_xmit
    restarts the queue.
  - poll_tx is delayed, for example by a hardirq, before it calls
    netif_wake_queue().
  - start_xmit fills the last free descriptor and stops the queue
    again.
  - poll_tx's late netif_wake_queue() then clears XOFF on a full ring.

There is no read barrier in poll_tx between the XOFF test and the
write_idx load, so load-load reordering is another way to get there.

On the next xmit, the early check reads read_idx and sees a full ring.
Suppose the poll_tx() runs that reap the remaining descriptors finish
inside the window before netif_stop_queue() here:

CPU0 (start_xmit)                    CPU1 (poll_tx)
bcm4908_enet_tx_free_descs() == 0
                                     reap all outstanding descriptors
                                     WRITE_ONCE(tx_ring->read_idx, ...)
                                     smp_mb()
                                     netif_queue_stopped() == false
netif_stop_queue()
return NETDEV_TX_BUSY

No descriptors are outstanding at that point, so no TX interrupt or
NAPI poll follows, and the driver has no ndo_tx_timeout. Wouldn't the
queue then stay stopped until ifdown/ifup? That is the "queue stops for
good with nothing left to wake it" symptom the commit message describes.

The window is narrow. It needs the xmit CPU to be preempted, as on
PREEMPT_RT, or held by a long hardirq.

Could this path use netif_txq_try_stop() instead, or the same
stop/smp_mb/re-check/restart sequence used at the end of
bcm4908_enet_start_xmit()?

[ ... ]

> @@ -572,8 +576,17 @@ static netdev_tx_t bcm4908_enet_start_xmit(struct sk_buff *skb, struct net_devic
>  
>  	bcm4908_enet_dma_tx_ring_enable(enet, &enet->tx_ring);
>  
> -	if (++ring->write_idx == ring->length - 1)
> -		ring->write_idx = 0;
> +	WRITE_ONCE(ring->write_idx,
> +		   ring->write_idx == ring->length - 1 ? 0 : ring->write_idx + 1);

[Severity: Low]
This isn't a bug, but the commit message leaves out a second symptom of
the same index mismatch, and this change fixes it too.

With the old wrap at length - 1, slot length - 1 was never filled.
bcm4908_enet_poll_tx() wrapped read_idx at length. After it reaped
slot length - 2, read_idx became length - 1, which write_idx could
never equal.

The loop then ran on the unused slot, whose descriptor ctl is still
zero, so OWN is clear. Once per ring lap it called dma_unmap_single()
with slot->dma_addr 0 and slot->len 0, then dev_kfree_skb(NULL) and
handled++. This over-counted tx_packets and caused CONFIG_DMA_API_DEBUG
warnings about unmapping memory that was never mapped.

The changelog only says:

    while poll_tx() wraps read_idx at length,

Could it also name this symptom, so stable backporters can see that it
is covered?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109235700.2586821-1-kudzo51%40gmail.com