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

[Yibo Dong]

On Fri, Sep 04, 2026 at 02:57:15PM -0700, Jakub Kicinski wrote:
> 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?
> 
> [ ... ]
> 
Got it, I will move mod_timer after NAPI is enabled.
> > +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?
The RX descriptor write-back format guarantees that each descriptor is at
least 33 bytes, so the first descriptor always contains the Ethernet
header, including for multi-descriptor packets.
Would it be acceptable to rely on this hardware invariant rather than add
an additional defensive length check before eth_type_trans()?
> 
> [ ... ]
> 
> > @@ -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?
> 
> [ ... ]
> 
I plan to keep the normal poll path unchanged and fix this in the down path:

        rnpgbe_irq_disable(mucse);
        rnpgbe_napi_disable_all(mucse);
        synchronize_net();
        rnpgbe_irq_disable(mucse);

The first mask prevents new hard IRQs from scheduling NAPI and
synchronizes any running IRQ handlers. After NAPI is disabled,
synchronize_net() waits for an in-flight NAPI poll tail which may already
have cleared NAPI_STATE_SCHED but has not yet returned. The final mask is
therefore not subject to a later rnpgbe_irq_enable_queues() write.
Would this teardown ordering be an acceptable way to close the race while
keeping the normal RX/TX poll path unchanged?

Thanks for your feedback.