Re: [PATCH net-next v12 3/5] net: rnpgbe: Add RX packet reception support

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 4

- [Low] Commit claims to add RX packet reception, but at this commit no
  MAC-level receive enable or address filtering exists (verified: `git…
- [Low] In rnpgbe_configure_rx_ring() the comment "/* Start an initial
  retry after NAPI is enabled.
- [Low] rnpgbe_stop_all_rx_rings() is documented "Return: 0 when RX DMA
  was quiesced, negative errno otherwise" and rnpgbe_configure_rx()…
- [Low] rnpgbe_free_rx_resources() is not self-contained even though its
  kernel-doc says "Free all receive software resources": it vfree()s…

--

net: rnpgbe: Add RX packet reception support

This adds the RX descriptor format, page_pool based RX buffer management,
the NAPI poll callback with RX cleanup, RX ring setup/teardown and RX
statistics for the rnpgbe driver.  A per q_vector timer retries failed
page allocations, and an RX DMA quiesce timeout is treated as a terminal
AXI fault.

A few questions below about the new RX teardown paths and some of the
new comments.

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> index fed07d53e5fe1..e4b7ff992c4d0 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c

[ ... ]

> @@ -719,6 +1165,31 @@ static int rnpgbe_wait_tx_dma_idle(struct mucse *mucse)
>  	return err;
>  }
>  
> +static int rnpgbe_wait_rx_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_RX_STATUS) ==
> +				 RNPGBE_DMA_RX_STATUS,
> +				 10, 100000);
> +	if (err) {
> +		set_bit(__MUCSE_AXI_FAULT, &mucse->state);
> +		dev_err(&mucse->pdev->dev,
> +			"RX DMA failed to quiesce, status %#x\n", dma_status);
> +	}
> +
> +	return err;
> +}
> +

[ ... ]

> @@ -838,6 +1309,66 @@ void rnpgbe_clean_all_tx_rings(struct mucse *mucse)
>  		rnpgbe_clean_tx_ring(mucse->tx_ring[i]);
>  }
>  
> +static void rnpgbe_stop_rx_ring(struct mucse_ring *rx_ring)
> +{
> +	/* Stop hw. hardware design guarantees:
> +	 * - No new descriptors will be fetched after RX_START=0
> +	 * - No DMA will be initiated for already-fetched descriptors
> +	 */
> +	mucse_ring_wr32(rx_ring, RNPGBE_RX_START, 0);
> +	/* Flush posted write to ensure hardware sees RX_START=0 */
> +	(void)mucse_ring_rd32(rx_ring, RNPGBE_RX_START);
> +}
> +
> +/**
> + * rnpgbe_stop_all_rx_rings - Stop RX DMA for all queues
> + * @mucse: board private structure
> + *
> + * Return: 0 when RX DMA was quiesced, negative errno otherwise
> + **/
> +static int rnpgbe_stop_all_rx_rings(struct mucse *mucse)
> +{
> +	struct mucse_hw *hw = &mucse->hw;
> +	u32 dma_axi_ctl;
> +	int err = 0;
> +
> +	for (int i = 0; i < mucse->num_rx_queues; i++)
> +		rnpgbe_stop_rx_ring(mucse->rx_ring[i]);
> +
> +	if (mucse->num_rx_queues &&
> +	    !test_bit(__MUCSE_AXI_FAULT, &mucse->state))
> +		err = rnpgbe_wait_rx_dma_idle(mucse);
> +
> +	dma_axi_ctl = mucse_hw_rd32(hw, RNPGBE_DMA_AXI_EN);
> +	dma_axi_ctl &= ~RX_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);
> +
> +	if (!test_bit(__MUCSE_AXI_FAULT, &mucse->state))
> +		return 0;
> +
> +	return err ? err : -EIO;
> +}

[Severity: Low]
Does the return value here match the kernel-doc "Return: 0 when RX DMA was
quiesced, negative errno otherwise"?

If __MUCSE_AXI_FAULT is already latched, for example set earlier by
rnpgbe_wait_tx_dma_idle(), then the RX wait is skipped:

	if (mucse->num_rx_queues &&
	    !test_bit(__MUCSE_AXI_FAULT, &mucse->state))
		err = rnpgbe_wait_rx_dma_idle(mucse);

err stays 0, yet the tail of the function still does:

	return err ? err : -EIO;

So -EIO is reported as "RX DMA did not quiesce" without RNPGBE_DMA_RX_STATUS
ever having been read.  rnpgbe_configure_rx() then forwards that -EIO to
rnpgbe_open() under the same wording in its own kernel-doc.

Refusing to bring the interface up after a terminal AXI fault looks
intentional, so would it be clearer to describe the sticky-fault case in
the kernel-doc of both functions rather than have it read as an RX quiesce
timeout?

[ ... ]

> @@ -1270,5 +1807,255 @@ void rnpgbe_get_stats64(struct net_device *netdev,

[ ... ]

> +/**
> + * rnpgbe_clean_rx_ring - Free Rx Buffers per Queue
> + * @rx_ring: ring to free buffers from
> + **/
> +static void rnpgbe_clean_rx_ring(struct mucse_ring *rx_ring)
> +{
> +	struct mucse_rx_buffer *rx_buffer;
> +	u16 i;
> +
> +	/* ring already cleared, nothing to do */
> +	if (!rx_ring->rx_buffer_info)
> +		return;
> +	/* Free all the Rx ring sk_buffs */
> +	for (i = 0; i < rx_ring->count; i++) {
> +		rx_buffer = &rx_ring->rx_buffer_info[i];
> +
> +		if (rx_buffer->skb) {
> +			struct sk_buff *skb = rx_buffer->skb;
> +
> +			dev_kfree_skb(skb);
> +			rx_buffer->skb = NULL;
> +		}
> +
> +		if (rx_buffer->page) {
> +			page_pool_put_full_page(rx_ring->page_pool,
> +						rx_buffer->page, false);
> +			rx_buffer->page = NULL;
> +		}
> +	}
> +
> +	rx_ring->next_to_clean = 0;
> +	rx_ring->next_to_use = 0;
> +}
> +
> +/**
> + * rnpgbe_free_rx_resources - Free Rx Resources
> + * @rx_ring: ring to clean the resources from
> + *
> + * Free all receive software resources
> + **/
> +static void rnpgbe_free_rx_resources(struct mucse_ring *rx_ring)
> +{
> +	vfree(rx_ring->rx_buffer_info);
> +	rx_ring->rx_buffer_info = NULL;
> +	/* if not set, then don't free */
> +	if (!rx_ring->desc)
> +		return;
> +
> +	dma_free_coherent(rx_ring->dev, rx_ring->size, rx_ring->desc,
> +			  rx_ring->dma);
> +	rx_ring->desc = NULL;
> +	if (rx_ring->page_pool) {
> +		page_pool_destroy(rx_ring->page_pool);
> +		rx_ring->page_pool = NULL;
> +	}
> +}

[Severity: Low]
Can this leak the page_pool pages held in rx_buffer_info?

rnpgbe_free_rx_resources() is documented as "Free all receive software
resources", but it vfree()s rx_buffer_info first, which discards every
page pointer and every chained skb pointer that rnpgbe_clean_rx_ring()
would have released:

	vfree(rx_ring->rx_buffer_info);
	rx_ring->rx_buffer_info = NULL;

page_pool_destroy() is also placed after the early bail-out:

	if (!rx_ring->desc)
		return;

so a ring that has a pool but no descriptor memory keeps the pool.

That makes rnpgbe_free_all_rx_resources() correct only when the caller has
already run rnpgbe_clean_all_rx_rings(), which is not stated anywhere.
rnpgbe_close() gets that ordering through rnpgbe_down(), but the new
err_free_rx label in rnpgbe_open() calls it with no preceding RX clean,
while the adjacent err_free_tx label does clean-then-free.

Since rnpgbe_configure_rx_ring() already posts up to ring->count -
M_RX_BUFFER_WRITE (496) pages per queue, any future failure return added at
or after that fill would reach err_free_rx with the page pointers gone, and
page_pool_destroy() would then run with inflight pages and keep warning
about a stalled pool shutdown.

Would it be safer to call rnpgbe_clean_rx_ring() from
rnpgbe_free_rx_resources() and destroy the pool before the desc check, so
the free path is self-contained?

[ ... ]

> +/**
> + * rnpgbe_configure_rx_ring - Configure Rx ring info to hw
> + * @mucse: pointer to private structure
> + * @ring: structure containing ring specific data
> + *
> + * Configure the Rx descriptor ring after a reset.
> + **/
> +static void rnpgbe_configure_rx_ring(struct mucse *mucse,
> +				     struct mucse_ring *ring)
> +{

[ ... ]

> +	mucse_ring_wr32(ring, RNPGBE_RX_INT_PKTCNT, M_DEFAULT_RX_INT_PKTCNT);
> +	/* Start an initial retry after NAPI is enabled. */
> +	rnpgbe_alloc_rx_buffers(ring, mucse_desc_unused_rx(ring));
> +}

[Severity: Low]
Is this comment describing the right call?  The call below it is the
initial synchronous fill, not a retry, and it runs before NAPI is enabled:

  rnpgbe_open() -> rnpgbe_configure() -> rnpgbe_configure_rx()
    -> rnpgbe_configure_rx_ring()

while napi_enable happens later in rnpgbe_up_complete() ->
rnpgbe_napi_enable_all().  The behaviour the comment describes looks like
rnpgbe_schedule_rx_retry(), which up_complete() calls after NAPI is
enabled.

This is also the only call site that discards the documented return value
of rnpgbe_alloc_rx_buffers() ("Return: true if alloc failed"), so a fully
failed initial fill leaves the ring empty with no rx_alloc_timer armed
until rnpgbe_schedule_rx_retry() re-checks.  Was that intended?

> +
> +/**
> + * rnpgbe_configure_rx - Configure Receive Unit after Reset
> + * @mucse: pointer to private structure
> + *
> + * Configure the Rx unit after a reset.
> + *
> + * Return: 0 on success, negative errno if RX DMA did not quiesce
> + **/
> +int rnpgbe_configure_rx(struct mucse *mucse)
> +{

[ ... ]

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> index 21136abe12317..6e006fbd17ad5 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> @@ -31,13 +31,19 @@ static struct pci_device_id rnpgbe_pci_tbl[] = {
>   * rnpgbe_configure - Configure the hardware
>   * @mucse: pointer to private structure
>   *
> - * Configure Tx registers in hardware.
> + * Configure Tx and Rx registers in hardware.
>   *
>   * Return: 0 on success, negative errno if hardware configuration fails
>   **/
>  static int rnpgbe_configure(struct mucse *mucse)
>  {
> -	return rnpgbe_configure_tx(mucse);
> +	int err;
> +
> +	err = rnpgbe_configure_tx(mucse);
> +	if (err)
> +		return err;
> +
> +	return rnpgbe_configure_rx(mucse);
>  }

[Severity: Low]
The changelog says "Add basic RX packet reception for the RNPGBE driver",
but at this commit only the DMA side is programmed.  There is no MAC-level
receive enable and no address filtering yet:

  git grep -E 'GMAC|set_rx_mode|set_link' drivers/net/ethernet/mucse/

returns nothing at this commit.  The pieces arrive later in the same
series, where rnpgbe_set_rx_mode() programs the RAR and multicast hash and
is registered as .ndo_set_rx_mode, and rnpgbe_set_link() sets
GMAC_CONTROL_RE plus GMAC_FRAME_FILTER = GMAC_RX_ALL on link up.

Could the changelog mention that MAC receive enable and filtering come in
follow-up patches, so it is clear packets are not yet received at this
step?

> @@ -69,13 +75,18 @@ static int rnpgbe_open(struct net_device *netdev)
>  	err = rnpgbe_setup_all_tx_resources(mucse);
>  	if (err)
>  		goto err_free_irqs;
> +	err = rnpgbe_setup_all_rx_resources(mucse);
> +	if (err)
> +		goto err_free_tx;
>  
>  	err = rnpgbe_configure(mucse);
>  	if (err)
> -		goto err_free_tx;
> +		goto err_free_rx;
>  	rnpgbe_up_complete(mucse);
>  
>  	return 0;
> +err_free_rx:
> +	rnpgbe_free_all_rx_resources(mucse);
>  err_free_tx:
>  	rnpgbe_clean_all_tx_rings(mucse);
>  	rnpgbe_free_all_tx_resources(mucse);

[Severity: Low]
Same point as above, from the caller side: should err_free_rx mirror
err_free_tx and clean the RX rings before freeing their resources?

err_free_tx does:

	rnpgbe_clean_all_tx_rings(mucse);
	rnpgbe_free_all_tx_resources(mucse);

while err_free_rx only frees.  Today every failure return reaches this
label before rnpgbe_configure_rx_ring() has posted any page, so nothing
leaks yet, but the asymmetry makes the path fragile if a later failure
return is added after the RX buffer fill.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/64BE41C2D2C0836A%2B20260914013019.2262317-1-dong100%40mucse.com