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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: rnpgbe: Add RX packet reception support

This adds the RX path for rnpgbe: the RX descriptor union, page_pool
backed RX buffers, the NAPI poll callback, RX ring setup/teardown and RX
statistics. It also adds a per-q_vector timer that retries failed page
allocations by scheduling NAPI, and RX DMA quiesce handling on teardown.

A few questions about the new RX code below.

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> index ff81d222edd3..346e9a1cca8a 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c

[ ... ]

> @@ -198,15 +200,419 @@ static bool rnpgbe_clean_tx_irq(struct mucse_q_vector *q_vector,
>  	return !!budget;
>  }
>  

[ ... ]

> +/**
> + * rnpgbe_rx_alloc_retry - Retry RX buffer allocation
> + * @timer: RX allocation retry timer
> + *
> + * Schedules NAPI after RX buffer allocation fails during polling or
> + * initial ring configuration.
> + **/
> +static void rnpgbe_rx_alloc_retry(struct timer_list *timer)
> +{
> +	struct mucse_q_vector *q_vector =
> +		timer_container_of(q_vector, timer, rx_alloc_timer);
> +
> +	napi_schedule(&q_vector->napi);
> +}

[Severity: Medium]
Can this retry be silently lost when the timer is armed from
rnpgbe_configure_rx_ring()?

At that point NAPI has not been enabled yet.  The open path is:

rnpgbe_open()
  rnpgbe_configure()
    rnpgbe_configure_rx()
      rnpgbe_configure_rx_ring()
        if (rnpgbe_alloc_rx_buffers(ring, mucse_desc_unused_rx(ring))) {
                mod_timer(&ring->q_vector->rx_alloc_timer,
                          jiffies + msecs_to_jiffies(500));
        }
  rnpgbe_up_complete()
    rnpgbe_napi_enable_all()
    clear_bit(__MUCSE_DOWN, &mucse->state);

netif_napi_add_weight() leaves NAPI_STATE_SCHED and NAPI_STATE_NPSVC set
until napi_enable_locked() runs, so napi_schedule_prep() returns false and
only sets NAPIF_STATE_MISSED, which napi_enable_locked() does not act on.

rnpgbe_rx_alloc_retry() ignores the return value of napi_schedule() and does
not re-arm, so if the timer expires in the window before
rnpgbe_napi_enable_all() the retry is dropped.  If the initial refill failed
for every descriptor, no descriptors are posted, no RX interrupt can arrive
and the ring stays empty until an administrative down/up.

Would arming the timer only after NAPI is enabled, or re-arming from the
callback when napi_schedule() returns false, be more robust?

[ ... ]

> +static int rnpgbe_clean_rx_irq(struct mucse_q_vector *q_vector,
> +			       struct mucse_ring *rx_ring,
> +			       int budget)
> +{
> +	unsigned int max_size = SKB_WITH_OVERHEAD(PAGE_SIZE) - RNPGBE_SKB_PAD;

[ ... ]

> +		dma_rmb();
> +		/* Hardware enforces: minimum 33-bytes descriptor(no 1-13 byte
> +		 * size), multi-descriptors only for jumbo frames > 1536 bytes
> +		 * (controlled by M_DEFAULT_SG=96, each descriptor no more than
> +		 * 1536 bytes). Small packets use single descriptor.
> +		 */
> +		size = le16_to_cpu(rx_desc->wb.len);

[ ... ]

> +		if (unlikely(!size || size > max_size)) {

[Severity: Medium]
Is there anything that prevents a first descriptor with size in the 1..13
range from starting a fragmented packet here?

The only length check is the one above, which accepts any size from 1 to
max_size.  If such a short descriptor is non-EOP, rnpgbe_is_non_eop() parks
the skb and the following descriptors are attached with skb_add_rx_frag(),
leaving an skb whose linear head is shorter than ETH_HLEN while data_len is
non-zero.

rnpgbe_cleanup_headers() only calls eth_skb_pad(), which does nothing once
skb->len is already at least ETH_ZLEN, so the short head survives.
rnpgbe_process_skb_fields() then does:

	skb->protocol = eth_type_trans(skb, dev);

and eth_type_trans() -> eth_skb_pull_mac() -> skb_pull_inline(ETH_HLEN)
reaches the __skb_pull() check:

include/linux/skbuff.h:__skb_pull() {
	...
	BUG_ON(skb->len < skb->data_len);
	...
}

The comment above states the hardware enforces a 33-byte minimum, but the
code does not enforce it.  Would rejecting descriptors that cannot supply an
ETH_HLEN linear head, or a pskb_may_pull(skb, ETH_HLEN) before
eth_type_trans(), be worth adding?

[ ... ]

> @@ -225,11 +632,27 @@ static int rnpgbe_poll(struct napi_struct *napi, int budget)
>  	if (unlikely(!budget))
>  		return 0;
>  
> +	if (q_vector->rx.count > 1)
> +		per_ring_budget = max(budget / q_vector->rx.count, 1);
> +	else
> +		per_ring_budget = budget;
> +
> +	mucse_for_each_ring(ring, q_vector->rx) {
> +		int cleaned = 0;
> +
> +		cleaned = rnpgbe_clean_rx_irq(q_vector, ring, per_ring_budget);
> +		work_done += cleaned;
> +		if (cleaned >= per_ring_budget)
> +			clean_complete = false;
> +	}
> +
>  	if (!clean_complete)
>  		return budget;
>  
> -	if (likely(napi_complete_done(napi, work_done)))
> -		rnpgbe_irq_enable_queues(q_vector);
> +	if (likely(napi_complete_done(napi, work_done))) {
> +		if (!test_bit(__MUCSE_DOWN, &q_vector->mucse->state))
> +			rnpgbe_irq_enable_queues(q_vector);
> +	}

[Severity: Medium]
Does this __MUCSE_DOWN test actually close the window the commit message
describes with "Do not re-enable queue interrupts while the device is
down"?

napi_complete_done() clears NAPI_STATE_SCHED before the test_bit() and the
MMIO unmask run, and napi_disable_locked() only waits for SCHED/NPSVC to
clear:

net/core/dev.c:napi_disable_locked() {
	...
	while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
		usleep_range(20, 200);
		val = READ_ONCE(n->state);
	}
	...
}

So this interleaving looks possible:

CPU0 rnpgbe_poll()
	napi_complete_done()		/* SCHED released */
	test_bit(__MUCSE_DOWN) == 0

CPU1 rnpgbe_down()
	test_and_set_bit(__MUCSE_DOWN, &mucse->state);
	rnpgbe_napi_disable_all();
	rnpgbe_irq_disable();		/* includes synchronize_irq() */
	rnpgbe_stop_all_rx_rings();
	rnpgbe_clean_all_rx_rings();

CPU0 (continues)
	rnpgbe_irq_enable_queues(q_vector);	/* unmask + trig write */

After rnpgbe_down() returns the queue interrupt would be left unmasked and
explicitly re-triggered, after RX DMA was quiesced and the RX buffers were
released, and just before rnpgbe_free_irq().

Would unmasking before napi_complete_done() releases SCHED, or re-masking
after rnpgbe_napi_disable_all() in rnpgbe_down(), give the guarantee the
changelog claims?

[ ... ]