Re: [PATCH net-next v9 2/4] net: rnpgbe: Add basic TX packet transmission support

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260814111317.1741087-2-dong100@mucse.com?part=2
---

--- Patch [2]: [PATCH net-next v9 2/4] net: rnpgbe: Add basic TX packet transmission support ---

commit 8bff6db2aa4a278636a66ac5e08b9b7d42fd23a7
Author: Dong Yibo [off-list ref]

net: rnpgbe: Add basic TX packet transmission support

This adds the TX path for rnpgbe: the TX descriptor layout and buffer
management, rnpgbe_xmit_frame_ring(), TX ring setup/teardown, and TX
completion handling in rnpgbe_clean_tx_irq(). It also requires the 56-bit
DMA mask and enables NETIF_F_SG plus NETIF_F_HIGHDMA.

A few questions about the TX stop/wake logic and the TX DMA quiesce
handling below.

[ ... ]

[ ... ]

+	i += tx_ring->count;
+	tx_ring->next_to_clean = i;
+	u64_stats_update_begin(&tx_ring->syncp);
+	tx_ring->stats.bytes += total_bytes;
+	tx_ring->stats.packets += total_packets;
+	u64_stats_update_end(&tx_ring->syncp);
+
+#define TX_WAKE_THRESHOLD (DESC_NEEDED * 2)
+	__netif_txq_completed_wake(txring_txq(tx_ring),
+				   total_packets, total_bytes,
+				   mucse_desc_unused(tx_ring),
+				   TX_WAKE_THRESHOLD,
+				   !netif_carrier_ok(tx_ring->netdev) ||
+				   test_bit(__MUCSE_DOWN, &mucse->state));

[Severity: High]
[Finding: main-1-1]
[Sources: opus-5]
Can the TX queue end up stopped forever here?

rnpgbe_clean_tx_irq() is the only place in the driver that can clear
__QUEUE_STATE_DRV_XOFF after rnpgbe_xmit_frame_ring() stops the queue,
and the down_cond argument suppresses the wake whenever the carrier is
reported down:

	!netif_carrier_ok(tx_ring->netdev) ||
	test_bit(__MUCSE_DOWN, &mucse->state)

Since link state is polled by the service task, netif_carrier_off() can
lag the real link event. If the hardware finishes the queued descriptors
during that window, this call drains the ring but returns without waking,
and with an empty ring there is no further TX interrupt to re-evaluate
the condition.

What re-enables the queue after that? netif_tx_start_all_queues() is
called only from rnpgbe_up_complete() on ifup, and the runtime carrier-on
handler added later in the series only calls netif_carrier_on() without
touching the queues. dev_activate() does not clear DRV_XOFF either.

rnpgbe_netdev_ops also registers no .ndo_tx_timeout, and
netdev_watchdog_up() bails out early in that case:

net/sched/sch_generic.c:netdev_watchdog_up() {
	if (!dev->netdev_ops->ndo_tx_timeout)
		return;
}

so dev_watchdog() never fires and a genuine hardware TX stall (DD never
set, no TX interrupt) is equally invisible. Would it make sense to add
.ndo_tx_timeout, and to wake the queues from the carrier-on path?

+
+	return !!budget;
+}

[ ... ]

[Severity: Medium]
[Finding: gpt-5-6-sol-4-6]
[Sources: gpt-5-6-sol, opus-5]
Should the timeout here be propagated to the callers?

rnpgbe_wait_tx_dma_idle() returns void, so a timeout is only logged and
both callers continue as if the engine had confirmed idle. On the ifdown
side that means rnpgbe_clean_all_tx_rings() clears TX_AXI_RW_EN and then
rnpgbe_clean_tx_ring() calls dma_unmap_single()/dma_unmap_page() on every
in-flight TX buffer, and rnpgbe_close() -> rnpgbe_free_all_tx_resources()
-> rnpgbe_free_tx_resources() then dma_free_coherent()s the descriptor
ring.

The comment in rnpgbe_stop_tx_ring() states that DMA for descriptors
fetched before the stop may still be in flight, which is what this poll
is guarding. If the poll times out, can the device still be reading the
mapped buffers or writing back into the descriptor ring after they are
released?

The same unchecked wait sits at the head of rnpgbe_configure_tx(), where
RNPGBE_TX_BASE_ADDR_LO/HI, RNPGBE_TX_LEN and TAIL are reprogrammed and
TX_START is set to 1 again. Is a device reset or an error return needed
when the engine never reports idle?

+
+/**
+ * rnpgbe_clean_tx_ring - Free Tx Buffers
+ * @tx_ring: ring to be cleaned
+ **/
+static void rnpgbe_clean_tx_ring(struct mucse_ring *tx_ring)
+{

[ ... ]

+static void rnpgbe_clean_all_tx_rings(struct mucse *mucse)
+{
+	struct mucse_hw *hw = &mucse->hw;
+	u32 dma_axi_ctl;
+
+	for (int i = 0; i < mucse->num_tx_queues; i++)
+		rnpgbe_stop_tx_ring(mucse->tx_ring[i]);
+
+	if (mucse->num_tx_queues)
+		rnpgbe_wait_tx_dma_idle(mucse);
+
+	dma_axi_ctl = mucse_hw_rd32(hw, RNPGBE_DMA_AXI_EN);
+	dma_axi_ctl &= ~TX_AXI_RW_EN;
+	mucse_hw_wr32(hw, RNPGBE_DMA_AXI_EN, dma_axi_ctl);
+	/* Flush the posted write before releasing the DMA mappings. */
+	(void)mucse_hw_rd32(hw, RNPGBE_DMA_AXI_EN);
+
+	for (int i = 0; i < mucse->num_tx_queues; i++)
+		rnpgbe_clean_tx_ring(mucse->tx_ring[i]);
+}

[ ... ]

Simon says: I think we can skip the issue below.

[Severity: Low]
[Finding: main-2-3]
[Sources: opus-5]
This isn't a bug, but hw->pfvfnum is a u8 here and the shift is done on
the int-promoted value, while rnpgbe_tx_map() casts first for the same
field:

	fun_id = (u64)tx_ring->pfvfnum << 56;

With the kernel's -fno-strict-overflow the produced bit pattern is the
one the register wants in either form, so this is only a consistency
point; an explicit (u32) cast would match the other encoding site.

[ ... ]

+netdev_tx_t rnpgbe_xmit_frame_ring(struct sk_buff *skb,
+				   struct mucse_ring *tx_ring)
+{

[ ... ]

+	if (!netif_txq_maybe_stop(txring_txq(tx_ring),
+				  mucse_desc_unused(tx_ring),
+				  count + RESV_DESC_NEEDED,
+				  count + RESV_DESC_NEEDED))
+		return NETDEV_TX_BUSY;

[ ... ]

[ ... ]