Re: [PATCH v2 1/5] net: ethernet: cortina: Fix budget accounting

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 (4) · Low: 0

- [Medium] At this commit, gmac_rx()
  (drivers/net/ethernet/cortina/gemini.c:1443-1571) increments the new…

Pre-existing issues:
- [Medium] gmac_napi_poll() at drivers/net/ethernet/cortina/gemini.c:1591
  does `port->freeq_refill += received;` and compares the result against…
- [Medium] gmac_napi_poll()
  (drivers/net/ethernet/cortina/gemini.c:1584-1589) discards the return…
- [Medium] gmac_napi_poll()
  (drivers/net/ethernet/cortina/gemini.c:1581-1598) opens the…
- [Medium] Duplicate view of the same ordering defect from a different
  stage: gmac_napi_poll()…

--

net: ethernet: cortina: Fix budget accounting

gmac_rx() used to return the leftover NAPI budget while gmac_napi_poll()
treated the value as a packet count, so an idle poll reported a full
budget. The patch adds a separate received counter, returns it, and
changes the free queue refill credit to add that count directly.

Link: https://lore.kernel.org/r/20260509-gemini-ethernet-fixes-v1-4-6c5d20ddc35b@kernel.org
Link: https://lore.kernel.org/r/20260512131456.189452-1-pabeni@redhat.com

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 4c762229ce420..1d9824d1716cf 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -1545,7 +1546,8 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget)
>  			napi_gro_frags(&port->napi);
>  			skb = NULL;
>  			frag_nr = 0;
> -			--budget;
> +			budget--;
> +			received++;
>  		}
>  		continue;

[Severity: Medium]
Should the drop paths in gmac_rx() also count towards received and
budget?  This is the only site that touches either counter, and it is
inside the successful GRO branch.

Descriptors that end up on a drop path still advance the ring read
pointer r but contribute nothing:

  - gmac_skb_if_good_frame() returning NULL (descriptor status error,
    short frame, RX status or checksum error, fragment allocation
    failure) jumps to err_drop
  - a zero DMA mapping jumps to err_drop
  - gmac_get_queue_page() failure does "continue"
  - a non-SOF descriptor with no in-progress skb does put_page() and
    "continue"
  - frag_nr == MAX_SKB_FRAGS jumps to err_drop

Since the loop condition is:

	while (budget && w != r) {

a sustained stream of malformed frames, or frag allocation failures
under memory pressure, lets one poll walk the whole available RX ring
while returning little or no work.  Does that defeat budget enforcement
in gmac_napi_poll() and allow softirq monopolization?

The later patch f27033c2d9b5 ("net: ethernet: cortina: Count dropped
frames as NAPI work") moves budget--/received++ to a common next_desc
EOF path, which appears to address exactly this, so this is noted for
the record.

> @@ -1565,7 +1567,7 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget)
>  	port->rx_skb = skb;
>  	port->rx_frag_nr = frag_nr;
>  	writew(r, ptr_reg);
> -	return budget;
> +	return received;
>  }
>  
>  static int gmac_napi_poll(struct napi_struct *napi, int budget)
> @@ -1586,7 +1588,7 @@ static int gmac_napi_poll(struct napi_struct *napi, int budget)
>  		++port->rx_napi_exits;
>  	}
                ^^^

[Severity: Medium]
This is a pre-existing issue and not introduced here, but the branch
above discards the return value of napi_complete_done() and then
re-arms the hardware RX interrupt unconditionally:

	if (received < budget) {
		napi_gro_flush(napi, false);
		napi_complete_done(napi, received);
		gmac_enable_rx_irq(napi->dev, 1);
		++port->rx_napi_exits;
	}

The kernel-doc in include/linux/netdevice.h says "Return: false if
device should avoid rearming interrupts", which is how
defer_hard_irqs / gro_flush_timeout / prefer_busy_poll deferral is
implemented in net/core/dev.c:napi_complete_done().

Does this patch make it more visible?  Before the change the branch was
entered based on the inverted value, whereas now every non-budget
exhausting poll, including a fully idle one, calls
napi_complete_done(napi, 0) and re-enables the IRQ regardless of the
answer.

The conditional form arrives later in the series in 8dd58e3d1305
("net: ethernet: cortina: Finish RX updates before NAPI completion") as
"if (received < budget && napi_complete_done(napi, received))", so this
is for the record.

>  
> -	port->freeq_refill += (budget - received);
> +	port->freeq_refill += received;
        ^^^

[Severity: Medium]
This isn't a regression from this patch since the old expression
evaluated to the same frame count, but are the units on the two sides
of this comparison the same?

	freeq_threshold = 1 << (geth->freeq_order - 1);
	...
	port->freeq_refill += received;
	if (port->freeq_refill > freeq_threshold) {

freeq_threshold is expressed in hardware free queue fragment entries,
and geth_fill_freeq() accounts one fragment reference per entry
("count += 1 << fpp_order").  Meanwhile received counts completed
frames only.

Each loop iteration in gmac_rx() consumes exactly one RX descriptor,
and therefore one free queue fragment, as the read pointer shows:

	r++;
	r &= m;
	...
	writew(r, ptr_reg);

Non-final fragments of multi-descriptor frames (freeq_frag_order is
fixed at DEFAULT_RX_BUF_ORDER, about 2 KB, and does not scale with MTU)
and all descriptors taken by the drop paths credit zero refill.  Can
that make the proactive half-empty refill fire late, or effectively
never during a fragment or drop storm, leaving only the SWFQ_EMPTY
threshold interrupt as a backstop and producing avoidable RX drops plus
"HW BUG: zero DMA desc" errors?

The later patch 230757ee03c5 ("net: ethernet: cortina: Count RX
descriptors for freeq refill") adds a freeq_consumed out-parameter that
counts every consumed descriptor, so this is noted for the record.

[Severity: Medium]
This is also pre-existing rather than introduced here, but the port
state updates around this line run after NAPI ownership has been
released.  gmac_napi_poll() opens the writer section first:

	u64_stats_update_begin(&port->rx_stats_syncp);

	received = gmac_rx(napi->dev, budget);
	if (received < budget) {
		napi_gro_flush(napi, false);
		napi_complete_done(napi, received);
		gmac_enable_rx_irq(napi->dev, 1);
		++port->rx_napi_exits;
	}

	port->freeq_refill += received;
	...
	u64_stats_update_end(&port->rx_stats_syncp);

Once napi_complete_done() clears NAPI_STATE_SCHED and
gmac_enable_rx_irq() unmasks the RX interrupt, can a new interrupt on
another CPU start a second poll for the same port while the first is
still in its epilogue?

CPU0 gmac_napi_poll()
	napi_complete_done()	/* clears NAPI_STATE_SCHED */
	gmac_enable_rx_irq()
	/* still inside the rx_stats_syncp write section */

CPU1 gmac_irq()
	napi_schedule(&port->napi)
	  -> net_rx_action -> gmac_napi_poll()
	       u64_stats_update_begin(&port->rx_stats_syncp);

u64_stats_sync requires writers to be mutually exclusive per syncp, and
this driver runs on 32-bit ARM where u64_stats_update_begin() is a real
write_seqcount_begin().  With two overlapping writers, can
gmac_get_stats64() / gmac_get_ethtool_stats() observe an even sequence
mid-update and return torn 64-bit counters?  And can the non-atomic
read-modify-write on port->freeq_refill lose updates and skip the
refill this patch is trying to preserve?

This patch does not create the structure, but it makes the completion
branch run on every non-budget-exhausting poll, including previously
skipped idle polls.  The ordering is corrected later in the series by
8dd58e3d1305 ("net: ethernet: cortina: Finish RX updates before NAPI
completion"), where the stats section and the freeq_refill block
complete before napi_complete_done().

[Severity: Medium]
Same pre-existing ordering point seen from the NAPI documentation side,
not something this patch introduces: the NAPI documentation asks drivers
not to touch their own data structures after releasing ownership,
because another poll instance or napi_disable() may proceed.  Here
port->rx_napi_exits, port->freeq_refill and the closing
u64_stats_update_end() all run after napi_complete_done() and
gmac_enable_rx_irq().  Would it be safer to finish all per-port
accounting before releasing NAPI ownership, as 8dd58e3d1305 ("net:
ethernet: cortina: Finish RX updates before NAPI completion") later
does?

>  	if (port->freeq_refill > freeq_threshold) {
>  		port->freeq_refill -= freeq_threshold;
>  		geth_fill_freeq(geth, true);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903-gemini-ethernet-fixes-v2-0-2bbbd598ca6e%40kernel.org