Re: [PATCH net-next v14 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] rnpgbe_clean_rx_irq() counts descriptors with an invalid hardware
  length (`!size || size > RNPGBE_RX_DESC_DATA_LEN`) only in the per-…
- [Low] mucse_alloc_page_pool() creates the pool with
  PP_FLAG_DMA_SYNC_DEV, .offset = 0 and .max_len = PAGE_SIZE.
- [Low] The comment in rnpgbe_configure_rx_ring() says 'Any allocation
  shortfall is retried after NAPI is enabled by rnpgbe_up_complete()'.
- [Low] The rnpgbe_is_non_eop() kernel-doc says that for a non-EOP buffer
  the function 'will place the sk_buff in the next buffer to be chained…
- [Low] When rnpgbe_alloc_rx_buffers() fails inside
  rnpgbe_clean_rx_irq(), cleaned_count is set again from…

--

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 at one page per descriptor, and the commit adds a NAPI poll
callback and RX ring setup and teardown. It keeps a 16-descriptor gap in the
512-entry ring, retries failed page allocations with a per q_vector timer,
and treats an RX DMA quiesce timeout 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 e23fc2db1aab8..7beec27991d35 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c

[ ... ]

> +/**
> + * rnpgbe_is_non_eop - Process handling of non-EOP buffers
> + * @rx_ring: rx ring being processed
> + * @rx_desc: rx descriptor for current buffer
> + * @skb: current socket buffer containing buffer in progress
> + *
> + * This function updates next to clean.  If the buffer is an EOP buffer
> + * this function exits returning false, otherwise it will place the
> + * sk_buff in the next buffer to be chained and return true indicating
> + * that this is in fact a non-EOP buffer.
> + *
> + * Return: true for not end of packet
> + **/
> +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;
> +	}

[Severity: Low]
This isn't a bug, but the kernel-doc for rnpgbe_is_non_eop() says that for a
non-EOP buffer the skb is placed in the next buffer. When nr_frags has
reached MAX_SKB_FRAGS, the skb is freed here instead, and the function still
returns true.

Could the kernel-doc say that the caller's skb may already be freed when the
function returns true? The only caller, rnpgbe_clean_rx_irq(), just does
continue, so nothing is affected at runtime today.

[ ... ]

> +static int rnpgbe_clean_rx_irq(struct mucse_q_vector *q_vector,
> +			       struct mucse_ring *rx_ring,
> +			       int budget)
> +{
> +	unsigned int total_rx_bytes = 0, total_rx_packets = 0;
> +	u16 cleaned_count = mucse_desc_unused_rx(rx_ring);
> +	unsigned int work_done = 0;
> +
> +	while (likely(work_done < budget)) {
> +		struct mucse_rx_buffer *rx_buffer;
> +		union rnpgbe_rx_desc *rx_desc;
> +		struct sk_buff *skb;
> +		unsigned int size;
> +
> +		if (cleaned_count >= M_RX_BUFFER_WRITE) {
> +			if (rnpgbe_alloc_rx_buffers(rx_ring, cleaned_count)) {
> +				mod_timer(&q_vector->rx_alloc_timer,
> +					  jiffies + msecs_to_jiffies(500));
> +				cleaned_count = mucse_desc_unused_rx(rx_ring);
> +			} else {
> +				cleaned_count = 0;
> +			}
> +		}

[Severity: Low]
After a failed refill in rnpgbe_clean_rx_irq(), cleaned_count is recomputed
with mucse_desc_unused_rx(). If the first page allocation failed,
next_to_use has not moved. The value is then still >= M_RX_BUFFER_WRITE, and
it grows as more descriptors are consumed.

Does every later iteration of the same poll then retry the full refill? Each
retry calls page_pool_dev_alloc_pages(), which will likely fail again under
memory pressure. It then does a dma_wmb() and a writel() of the unchanged
tail in rnpgbe_alloc_rx_buffers():

	dma_wmb();
	mucse_update_rx_tail(rx_ring, i);

and calls mod_timer() again, even though the retry timer is already armed.

ixgbe-style drivers reset cleaned_count to 0 after one attempt. Would that
be enough here, since the retry timer is already armed?

> +		rx_desc = M_RX_DESC(rx_ring, rx_ring->next_to_clean);
> +

[ ... ]

> +		size = le16_to_cpu(rx_desc->wb.len);
> +
> +		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++;
> +			if (!dropping)
> +				atomic64_inc(&rx_ring->stats->dropped);

[Severity: Low]
Should descriptors with an invalid hardware length be counted as length
errors rather than drops?

This branch only increments the per-queue dropped counter, and
rnpgbe_get_stats64() reports that counter as rx_dropped:

	stats->rx_packets += packets;
	stats->rx_dropped += dropped;
	stats->rx_bytes += bytes;

rx_errors and rx_length_errors are never set.

include/uapi/linux/if_link.h describes rx_length_errors as "Number of
packets dropped due to invalid length". It also says rx_errors must include
the events counted by rx_length_errors. rx_dropped is meant for packets that
were received but not processed, for example due to lack of resources.

As written, malformed-length completions show up as resource drops, and
rx_errors always reads 0. struct mucse_queue_stats also has no error counter
to hold these events.

[ ... ]

> @@ -1263,4 +1863,223 @@ void rnpgbe_get_stats64(struct net_device *netdev,

[ ... ]

> +static int mucse_alloc_page_pool(struct mucse_ring *rx_ring)
> +{
> +	struct page_pool_params pp_params = {
> +		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
> +		.order = 0,
> +		.pool_size = rx_ring->count,
> +		.nid = dev_to_node(rx_ring->dev),
> +		.dev = rx_ring->dev,
> +		.napi = &rx_ring->q_vector->napi,
> +		.dma_dir = DMA_FROM_DEVICE,
> +		.offset = 0,
> +		.max_len = PAGE_SIZE,

[Severity: Low]
This isn't a bug, but does this sync window in mucse_alloc_page_pool() match
the buffer layout? mucse_alloc_mapped_page() posts buffers at an offset:

	bi->page_offset = RNPGBE_SKB_PAD;

The hardware writes at most RNPGBE_RX_DESC_DATA_LEN (1536) bytes per
descriptor.

With .offset = 0 and .max_len = PAGE_SIZE, every recycled page is synced for
the device across the whole page. That includes the headroom and the
skb_shared_info area, which the device never writes.

The reuse path in mucse_alloc_mapped_page() uses a third window:

	dma_sync_single_range_for_device(rx_ring->dev, bi->dma,
					 bi->page_offset,
					 PAGE_SIZE - bi->page_offset,
					 DMA_FROM_DEVICE);

On non-coherent or swiotlb platforms, this adds unnecessary cache
maintenance to the hot path. Would .offset = RNPGBE_SKB_PAD and
.max_len = RNPGBE_RX_DESC_DATA_LEN match what the device actually writes?

[ ... ]

> +static void rnpgbe_configure_rx_ring(struct mucse *mucse,
> +				     struct mucse_ring *ring)
> +{

[ ... ]

> +	/* Fill the ring before RX DMA is enabled. Any allocation shortfall is
> +	 * retried after NAPI is enabled by rnpgbe_up_complete().
> +	 */
> +	rnpgbe_alloc_rx_buffers(ring, mucse_desc_unused_rx(ring));
> +}

[Severity: Low]
Is this comment in rnpgbe_configure_rx_ring() accurate for small shortfalls?
rnpgbe_schedule_rx_retry() arms rx_alloc_timer whenever
mucse_desc_unused_rx() is non-zero:

	if (mucse_desc_unused_rx(ring)) {
		mod_timer(&q_vector->rx_alloc_timer,
			  jiffies + msecs_to_jiffies(500));

The NAPI poll that the timer schedules only refills when this holds in
rnpgbe_clean_rx_irq():

	if (cleaned_count >= M_RX_BUFFER_WRITE) {

rnpgbe_alloc_rx_buffers() stops at the first page_pool failure, so a
shortfall of 1 to 15 descriptors can happen. In that case the timer fires,
rnpgbe_clean_rx_irq() skips the refill, and the timer is not re-armed. The
retry the comment promises never happens.

The ring still has at least 481 descriptors posted, and refill resumes once
traffic frees 16 slots. So the practical effect is a misleading comment and
a timer that does nothing. The comment on mucse_desc_unused_rx() in
rnpgbe.h ("Refill is attempted once at least this many descriptors are
available") matches what the code does.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/0A4D45AD9F6A0F14%2B20260928033701.1033196-1-dong100%40mucse.com