Re: [PATCH net v3 3/7] net: bcm4908_enet: release the TX ring on stop

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (3) · Medium: 1 · 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] The patch does not fix the TX skb leak when the TX channel
  times out, and on that path it leaves the hardware running.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it makes the
  new TX-ring release in bcm4908_enet_stop() unreachable when the DT has…
- [High] This problem wasn't introduced by this patch, but the patch
  applies the 'do not free buffers the DMA may still use' reasoning only…
- [High] This problem wasn't introduced by this patch, but the patch now
  depends on the code it bypasses.

--

net: bcm4908_enet: release the TX ring on stop

This patch makes bcm4908_enet_stop() wait for the TX DMA channel to stop
and then unmap and free the skbs left in the TX ring. If the channel does
not stop in time, the free is skipped. It also clears the TX descriptors
in bcm4908_enet_dma_ring_init(), drops the OWN check from
bcm4908_enet_start_xmit(), and fixes the RX timeout warning text.

> diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
> index b1738d954b91a..bb42b6ecde16a 100644
> --- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
> +++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c

[ ... ]

> @@ -313,6 +314,11 @@ static void bcm4908_enet_dma_ring_init(struct bcm4908_enet *enet,
>  	enet_write(enet, ring->st_ram_block + ENET_DMA_CH_STATE_RAM_BASE_DESC_PTR,
>  		   (uint32_t)ring->dma_addr);
>  
> +	/* A descriptor the stopped DMA never completed still carries OWN */
> +	if (ring->is_tx)
> +		memset(ring->buf_desc, 0,
> +		       ring->length * sizeof(*ring->buf_desc));
> +
>  	ring->read_idx = 0;
>  	ring->write_idx = 0;
>  }

[Severity: High]
This is a pre-existing issue, but the TX descriptor clear and index reset
added here only run if bcm4908_enet_dma_init() gets through the RX buffer
allocation loop:

bcm4908_enet_dma_init() {
	for (i = 0; i < rx_ring->length; i++) {
		err = bcm4908_enet_dma_alloc_rx_buf(enet, i);
		if (err) {
			dev_err(dev, "Failed to alloc RX buffer: %d\n", err);
			bcm4908_enet_dma_uninit(enet);
			return err;
		}
	}

	bcm4908_enet_dma_ring_init(enet, &enet->tx_ring);
	bcm4908_enet_dma_ring_init(enet, &enet->rx_ring);
	...
}

bcm4908_enet_open() ignores that return value:

	bcm4908_enet_dma_reset(enet);
	bcm4908_enet_dma_init(enet);

	enet_umac_set(enet, UMAC_CMD, CMD_TX_EN | CMD_RX_EN);

	enet_set(enet, ENET_DMA_CONTROLLER_CFG, ENET_DMA_CTRL_CFG_MASTER_EN);

If napi_alloc_frag() or dma_map_single() fails for any RX slot, the
interface still comes up with MASTER_EN set and the RX channel enabled.
bcm4908_enet_dma_reset() has just set ENET_DMA_CH_STATE_RAM_BASE_DESC_PTR
to 0, and nothing programs it again. ndo_open still returns 0.

The OWN check is now removed from bcm4908_enet_start_xmit(), so this
memset is the only thing that stops start_xmit() from reusing a
descriptor that still has OWN set. After an earlier timed-out stop,
wouldn't the stale read_idx/write_idx and OWN descriptors survive this
path?

Should bcm4908_enet_open() check the result of bcm4908_enet_dma_init()
and unwind on failure?

[ ... ]

> @@ -509,8 +546,10 @@ static int bcm4908_enet_stop(struct net_device *netdev)
>  	netdev_reset_queue(netdev);

[Severity: High]
This isn't a bug introduced by this patch, but can the new TX clean be
reached when the DT has no "tx" interrupt?

The binding allows a single interrupt (minItems: 1). In that case
enet->irq_tx is negative and bcm4908_enet_open() skips the TX
napi_enable():

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

bcm4908_enet_stop() calls napi_disable(&tx_ring->napi) unconditionally,
just above this hunk. netif_napi_add_weight_locked() sets
NAPI_STATE_SCHED and NAPI_STATE_NPSVC, and only napi_enable() clears
them. So napi_disable_locked() would loop forever:

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

That hangs ndo_stop with RTNL held, before bcm4908_enet_tx_ring_clean()
is reached.

The same configuration also breaks TX. The irq_tx < 0 branch in
bcm4908_enet_start_xmit() calls napi_schedule() on a NAPI that still has
SCHED set. napi_schedule_prep() then fails, so bcm4908_enet_poll_tx()
never runs. After 199 frames the queue stops and nothing wakes it.

>  
>  	bcm4908_enet_dma_rx_ring_disable(enet, &enet->rx_ring);
> -	bcm4908_enet_dma_tx_ring_disable(enet, &enet->tx_ring);
>  
> +	/* Leave the TX buffers alone if the channel may still read them */
> +	if (!bcm4908_enet_dma_tx_ring_disable(enet, &enet->tx_ring))
> +		bcm4908_enet_tx_ring_clean(enet);

[Severity: Medium]
What happens to the skbs between read_idx and write_idx when
bcm4908_enet_dma_tx_ring_disable() times out?

On that path bcm4908_enet_tx_ring_clean() is skipped and nothing else is
done to the hardware. The TX subchannel is not reset through
ENET_DMA_CTRL_CHANNEL_RESET, and both ENET_DMA_CTRL_CFG_MASTER_EN and
UMAC CMD_TX_EN stay set.

Nothing frees those slots later either. On the next open,
bcm4908_enet_dma_reset() clears MASTER_EN, and
bcm4908_enet_dma_ring_init() resets the channel, clears the descriptors
and zeroes the indexes. None of that unmaps or frees what is left in
tx_ring->slots[].

Doesn't that leak the skbs, their DMA_TO_DEVICE mappings and the socket
memory they hold? The commit message says this leak is fixed.

If the device is unbound instead, the path is bcm4908_enet_remove() ->
unregister_netdev() -> bcm4908_enet_stop(), then
bcm4908_enet_dma_free(). That kfree()s tx_ring->slots without freeing
the skbs. It also calls dma_free_coherent() on the TX descriptor ring,
although the TX channel was never confirmed stopped or reset.

The comment reads as if the free is only postponed. Could the timeout
path force the channel idle first (reset the TX subchannel and/or clear
MASTER_EN, as ring_init() and dma_reset() do) and then clean the ring?

>  	bcm4908_enet_dma_uninit(enet);

[Severity: High]
This is a pre-existing issue, but doesn't the same reasoning apply to RX?
There the device writes into the buffers.

bcm4908_enet_dma_rx_ring_disable() returns void and only warns on
timeout. bcm4908_enet_dma_uninit() is then called unconditionally:

bcm4908_enet_dma_uninit() {
	...
		dma_unmap_single(dev, slot->dma_addr, slot->len, DMA_FROM_DEVICE);
		skb_free_frag(slot->buf);
		slot->buf = NULL;
	...
}

The RX descriptors still have DMA_CTL_STATUS_OWN and the buffer bus
address set by bcm4908_enet_dma_alloc_rx_buf(), and MASTER_EN is not
cleared.

If the RX channel has not stopped, can received frames be DMA-written
into pages that have already gone back to the page allocator?

Later patches in this series only change the unmap size in
bcm4908_enet_dma_uninit() and do not add a guard here.

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