Re: [PATCH v2 3/5] net: ethernet: cortina: Count dropped frames as NAPI work

[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 · Low: 0

- [Medium] Incomplete fix: in gmac_rx()
  (drivers/net/ethernet/cortina/gemini.c) the new common accounting…

--

net: ethernet: cortina: Count dropped frames as NAPI work

This moves the budget-- and received++ accounting in gmac_rx() out of the
successful delivery path and into a new common next_desc label, so that
frames which are dropped also consume NAPI budget. The error paths now
jump to next_desc instead of using continue.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 6502220362cbd..33e9763b32fe8 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -1523,7 +1523,7 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget)
>  
>  		} else if (!skb) {
>  			put_page(page);
> -			continue;
> +			goto next_desc;
>  		}
>  
>  		if (word3.bits32 & EOF_BIT)
> @@ -1546,10 +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--;
> -			received++;
>  		}
> -		continue;
> +		goto next_desc;
>  
>  err_drop:
>  		if (skb) {
> @@ -1562,6 +1560,13 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget)
>  			put_page(page);
>  
>  		port->stats.rx_dropped++;
> +
> +next_desc:
> +		/* Final or single-descriptor fragment, advance things */
> +		if (word3.bits32 & EOF_BIT) {
> +			budget--;
> +			received++;
> +		}

[Severity: Medium]
Since the accounting is gated on EOF_BIT, does this fully close the case
described in the commit message ("a stream of bad frames can process the
entire receive ring in one poll")?

The loop guard is the only per-descriptor limiter:

	while (budget && w != r) {
		...
		r++;
		r &= m;

so any descriptor that reaches next_desc with EOF_BIT clear still advances
r at no cost to budget. Paths that can do that in gmac_rx() are:

	if (!mapping) {
		netdev_err(netdev,
			   "rxq[%u]: HW BUG: zero DMA desc\n", r);
		goto err_drop;
	}

Here the driver has just concluded the device never wrote the descriptor,
and word3 comes from the same dma_alloc_coherent()-zeroed slot, so EOF_BIT
is clear and err_drop falls through to next_desc. Each such descriptor also
emits one unratelimited netdev_err() from softirq context.

The other uncharged paths are the "could not find mapping" gpage failure on
a non-final fragment, the orphan-continuation case:

		} else if (!skb) {
			put_page(page);
			goto next_desc;
		}

and the frag_nr == MAX_SKB_FRAGS drop when the descriptor is not the final
fragment.

Also, is the frame abandoned by an unexpected SOF counted anywhere?

		if (word3.bits32 & SOF_BIT) {
			if (skb) {
				napi_free_frags(&port->napi);
				port->stats.rx_dropped++;

That bumps rx_dropped and frees the partial frame, so a frame was
definitively dropped, but neither budget nor received changes, which reads
differently from "counts each completed frame as NAPI work whether it was
delivered or dropped".

For a descriptor stream that never presents EOF (zeroed or never-written
descriptors, repeated SOF without EOF), one poll can still walk the whole
snapshot, up to 1 << port->rxq_order entries: 512 with
DEFAULT_GMAC_RXQ_ORDER of 9, and up to 32768 after

		port->rxq_order = min(15, ilog2(rp->rx_pending - 1) + 1);

in gmac_set_ringparam(). The loop does terminate since r converges on the
snapshotted w, and received == 0 lets gmac_napi_poll() call
napi_complete_done(), so this is softirq residency and log flooding rather
than a hang.

Would charging work per descriptor consumed be more robust than inferring
frame completion from a field of a descriptor that may itself be invalid?
Note the later patch in this series that adds the freeq_consumed counter
already tracks a per-descriptor count, and the EOF_BIT gating at next_desc
is still present at the end of the series (230757ee03c5).

The related concern about port->freeq_refill += received diverging from the
number of descriptors the MAC consumed is addressed by the later commit
"net: ethernet: cortina: Count RX descriptors for freeq refill", so that
part is not raised here.

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