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

[Yibo Dong]

On Wed, Oct 14, 2026 at 02:59:13AM +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> [ ... ]
> 
> > @@ -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.
This is illegitimate.
Once START=0 has taken effect, hardware does not initiate any new
PCIe memory read or write requests, including for prefetched descriptors.
The 100 ms wait only covers transactions already in flight.
For this chip, failure to reach the done state within that window
indicates a terminal AXI fault, not a recoverable slow drain. As
documented in the code comments, this fault prevents further host-memory
accesses, including from previously accepted requests, and requires a
chip-level reset. Cleanup therefore relies on this hardware guarantee.
The same to RX.
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/3B42BE25B3696EAE%2B20261013022920.63529-1-dong100%40mucse.com
>