Re: [PATCH net-next v15 3/5] net: rnpgbe: Add basic RX data path support

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The RX drop state machine in `rnpgbe_clean_rx_irq()` keeps only a
  boolean `drop_status`, not the reason a drop is in progress.
- [Low] `rnpgbe_poll()` calls `rnpgbe_rx_refill_pending(q_vector)` after
  `napi_complete_done()` has returned true.

--

net: rnpgbe: Add basic RX data path support

This commit adds the RX DMA data path for the rnpgbe driver. RX buffers
come from page_pool with one page per descriptor, and the commit adds a
NAPI poll callback, RX ring setup/teardown and RX statistics. A per
q_vector timer retries failed page allocations by scheduling NAPI, and an
RX DMA quiesce timeout is treated as a terminal AXI fault.

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

[ ... ]

> +static bool rnpgbe_is_non_eop(struct mucse_ring *rx_ring,
> +			      union rnpgbe_rx_desc *rx_desc,
> +			      struct sk_buff *skb)
> +{

[ ... ]

> +	if (skb_shinfo(skb)->nr_frags < MAX_SKB_FRAGS) {
> +		/* place skb in next buffer to be received */
> +		rx_ring->rx_buffer_info[ntc].skb = skb;
> +	} else {
> +		atomic64_inc(&rx_ring->stats->dropped);
> +		rx_ring->drop_bytes = skb->len;
> +		/* too much frags, force free */
> +		dev_kfree_skb_any(skb);
> +		rx_ring->drop_status = true;
> +	}

[ ... ]

> +static int rnpgbe_clean_rx_irq(struct mucse_q_vector *q_vector,
> +			       struct mucse_ring *rx_ring,
> +			       int budget)
> +{

[ ... ]

> +		if (unlikely(!size || size > RNPGBE_RX_DESC_DATA_LEN)) {
> +			struct mucse_rx_buffer *err_rx_buffer;
> +			bool dropping = rx_ring->drop_status;
> +			u16 idx = rx_ring->next_to_clean;
> +
> +			cleaned_count++;
> +			/* Count one length error for the complete packet. */
> +			if (!dropping)
> +				total_rx_length_errors++;
> +			rx_ring->drop_bytes = 0;

[Severity: Low]
Can this branch undercount rx_length_errors?

drop_status only records that a drop is in progress, not why. Two paths
start a drop without counting a length error.

The first is the skb build failure on a non-EOP descriptor, later in
rnpgbe_clean_rx_irq():

    if (!skb) {
        cleaned_count++;
        rx_ring->drop_bytes = size;
        atomic64_inc(&rx_ring->stats->dropped);
        ...
        } else {
            rx_ring->drop_status = true;
        }

The second is the MAX_SKB_FRAGS case in rnpgbe_is_non_eop() above.

Suppose a later descriptor of the same packet reports a length of 0 or
more than RNPGBE_RX_DESC_DATA_LEN. This branch sees dropping == true and
skips total_rx_length_errors++. It also clears drop_bytes, so the
drop_status branch does not account the packet at EOP.

As a result the packet is counted only in rx_dropped. It never shows up
in rx_errors or rx_length_errors, which goes against the "Count one length
error for the complete packet" comment.

Could the ring track the reason for the drop, for example with a separate
"length error already counted" state, instead of a single bool?

[ ... ]

> @@ -201,11 +653,29 @@ static int rnpgbe_poll(struct napi_struct *napi, int budget)

[ ... ]

>  	if (!clean_complete)
>  		return budget;
>  
> -	if (likely(napi_complete_done(napi, work_done)))
> +	if (likely(napi_complete_done(napi, work_done))) {
> +		if (rnpgbe_rx_refill_pending(q_vector))
> +			mod_timer(&q_vector->rx_alloc_timer,
> +				  jiffies + msecs_to_jiffies(500));
>  		rnpgbe_irq_enable_queues(q_vector);
> +	}

[Severity: Low]
Is it safe for rnpgbe_poll() to read the ring indices after
napi_complete_done() has returned true?

Once NAPI_STATE_SCHED is cleared, this context no longer owns the NAPI
instance. rx_alloc_timer can still be pending, either from an earlier
poll or from rnpgbe_schedule_rx_retry() in rnpgbe_up_complete(). If it
fires on another CPU in this window:

CPU0                       CPU1
rnpgbe_poll()
  napi_complete_done()
                           rnpgbe_rx_alloc_retry()
                             napi_schedule()
                           rnpgbe_poll()
                             rnpgbe_clean_rx_irq()
                               writes next_to_clean
                               rnpgbe_alloc_rx_buffers()
                                 writes next_to_use
  rnpgbe_rx_refill_pending()
    mucse_desc_unused_rx()
      reads next_to_clean, next_to_use

These are plain loads and stores with no locking and no
READ_ONCE()/WRITE_ONCE(), so this is a data race that KCSAN can report.
The practical effect seems limited to arming the timer when it isn't
needed, or skipping an arm that was.

Could the refill decision be computed before napi_complete_done(), while
NAPI is still owned?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/B438B0A83ACCA2DE%2B20261004065604.224873-1-dong100%40mucse.com