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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 2

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] When rnpgbe_wait_tx_dma_idle() times out, the driver only
  latches __MUCSE_AXI_FAULT and logs an error.
- [Low] The commit message does not cover everything the patch changes.
- [Low] In rnpgbe_clean_tx_irq() the comment says it clears next_to_watch
  "to prevent false hangs", but the driver has no TX hang detection.

--

net: rnpgbe: Add basic TX packet transmission support

This commit adds the basic TX path for the rnpgbe driver. It covers TX
descriptor and buffer management, rnpgbe_xmit_frame_ring(), TX ring
setup and teardown, and NAPI TX completion in rnpgbe_clean_tx_irq(). It
also sets a 56-bit DMA mask, reports TX statistics through
ndo_get_stats64(), and treats a TX DMA idle timeout as a terminal fault.

> Introduce the TX descriptor structure and buffer management, the
> rnpgbe_xmit_frame_ring() transmit function, TX ring allocation and
> teardown, and TX completion handling in rnpgbe_clean_tx_irq(). Track TX
> packet and byte statistics, and report dropped and tx_dropped counts
> through ndo_get_stats64().

[ ... ]

> Configure streaming and coherent DMA masks for the hardware's 56-bit
> DMA addressing capability. Add cycles_per_us to struct mucse_hw to scale
> the TX interrupt timer, and enable NETIF_F_HIGHDMA when the DMA mask
> allows it.

[Severity: Low]
Is "report dropped and tx_dropped counts" accurate?

rnpgbe_get_stats64() fills only stats->tx_packets, stats->tx_bytes and
stats->tx_dropped. The per-queue dropped counter is added into tx_dropped
and is not reported on its own.

The message mentions NETIF_F_HIGHDMA, but not the NETIF_F_SG change right
next to it. rnpgbe_add_adapter() now sets NETIF_F_SG in both
netdev->features and netdev->hw_features, so non-linear skbs will go
through the frag-mapping loop in rnpgbe_tx_map().

The patch also adds BQL accounting, which the message doesn't mention:
netdev_tx_sent_queue() in rnpgbe_tx_map(), __netif_txq_completed_wake()
in rnpgbe_clean_tx_irq(), and netdev_tx_reset_queue() in
rnpgbe_clean_tx_ring().

Could the commit message describe the SG and BQL changes too?

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> index 81c3ec63a9a0c..21cd578f75323 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c

[ ... ]

> @@ -65,26 +67,143 @@ static void rnpgbe_irq_enable_queues(struct mucse_q_vector *q_vector)
>  	}
>  }
>  
> +/**
> + * rnpgbe_clean_tx_irq - Reclaim resources after transmit completes

[ ... ]

> +		/* if eop DD is not set pending work has not been completed */
> +		if (!(eop_desc->vlan_cmd & cpu_to_le32(M_TXD_STAT_DD)))
> +			break;
> +		/* clear next_to_watch to prevent false hangs */
> +		tx_buffer->next_to_watch = NULL;

[Severity: Low]
This isn't a bug, but is the "prevent false hangs" comment accurate for
this driver?

rnpgbe_netdev_ops has no ndo_tx_timeout, and nothing in the driver checks
next_to_watch for hangs. Here next_to_watch only marks a pending packet
for rnpgbe_clean_tx_irq() and rnpgbe_clean_tx_ring(). The comment seems
to have been carried over from ixgbe.

[ ... ]

> @@ -556,12 +680,173 @@ static void rnpgbe_napi_disable_all(struct mucse *mucse)
>  		napi_disable(&mucse->q_vector[i]->napi);
>  }
>  

[ ... ]

> +static int rnpgbe_wait_tx_dma_idle(struct mucse *mucse)
> +{
> +	struct mucse_hw *hw = &mucse->hw;
> +	u32 dma_status;
> +	int err;
> +
> +	/* A timeout indicates a terminal AXI fault. Hardware stops all PCIe
> +	 * DMA requests in this state, including requests accepted before the
> +	 * fault. Teardown may therefore release mappings and descriptor memory.
> +	 * Recovery requires a chip-level reset.
> +	 */
> +	err = readl_poll_timeout(hw->hw_addr + RNPGBE_DMA_STATUS,
> +				 dma_status,
> +				 (dma_status & RNPGBE_DMA_TX_STATUS) ==
> +				 RNPGBE_DMA_TX_STATUS,
> +				 10, 100000);
> +	if (err) {
> +		set_bit(__MUCSE_AXI_FAULT, &mucse->state);
> +		dev_err(&mucse->pdev->dev,
> +			"TX DMA failed to quiesce, status %#x\n", dma_status);
> +	}
> +
> +	return err;
> +}

[ ... ]

> +	if (mucse->num_tx_queues &&
> +	    !test_bit(__MUCSE_AXI_FAULT, &mucse->state))
> +		err = 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 continuing. */
> +	(void)mucse_hw_rd32(hw, RNPGBE_DMA_AXI_EN);

[ ... ]

> +void rnpgbe_clean_all_tx_rings(struct mucse *mucse)
> +{
> +	/* rnpgbe_stop_all_tx_rings() logs and latches any AXI fault. Hardware
> +	 * stops DMA requests in this state, so cleanup intentionally ignores
> +	 * an error and releases the mappings and descriptor memory.
> +	 */
> +	rnpgbe_stop_all_tx_rings(mucse);
> +
> +	for (int i = 0; i < mucse->num_tx_queues; i++)
> +		rnpgbe_clean_tx_ring(mucse->tx_ring[i]);
> +}

[Severity: Medium]
After rnpgbe_wait_tx_dma_idle() times out, what guarantees that the
device has actually stopped DMA?

On a timeout the driver only sets __MUCSE_AXI_FAULT and clears the
internal TX_AXI_RW_EN gate in rnpgbe_stop_all_tx_rings().
rnpgbe_clean_all_tx_rings() then ignores the error. It unmaps the TX
buffers, frees the skbs and memsets the descriptor ring.

After that, rnpgbe_close() or the err_free_tx path in rnpgbe_open()
continues with:

rnpgbe_free_all_tx_resources()
  rnpgbe_free_tx_resources()
    dma_free_coherent(tx_ring->dev, tx_ring->size, tx_ring->desc,
                      tx_ring->dma);

The only reason this would be safe is the hardware claim in the comment
and the commit message, that the hardware "stops all PCIe DMA requests in
this state". Nothing on the host side enforces that:

- rnpgbe_probe() calls pci_set_master(), but the driver never calls
  pci_clear_master() and never resets the function.
- TX_BASE_ADDR_HI/LO keep pointing at the freed ring. Once the bit is
  set, rnpgbe_open() returns -EIO, so nothing reprograms them.
- Bus Master Enable is only cleared by pci_disable_device() in
  rnpgbe_remove() or rnpgbe_dev_shutdown(), long after the memory was
  freed.

The timeout also only means the done bits weren't seen within 100ms. A
slow drain looks the same as a real fault.

If the device does write DD status back late, or reads a buffer after
the unmap, wouldn't that land in freed and possibly reused memory?

Would it be safer to call pci_clear_master() (or reset the function) on
the fault path before releasing DMA memory, or to leak the ring and
buffers in this case instead of freeing them?

The later patch "net: rnpgbe: Add basic RX data path support" in this
series uses the same approach for RX.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/3B42BE25B3696EAE%2B20261013022920.63529-1-dong100%40mucse.com