Re: [PATCH v3 net 1/7] net: enetc: consistenly track dropped frames in enetc_xdp_xmit()

[netdev-bot+sashiko]

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

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

- [Medium] enetc_xdp_xmit()
  (drivers/net/ethernet/freescale/enetc/enetc.c:1817-1820) still returns…

Pre-existing issues:
- [Medium] The accumulation added by this patch,
  `tx_ring->stats.xdp_tx_drops += num_frames - xdp_tx_frm_cnt;`…

--

net: enetc: consistenly track dropped frames in enetc_xdp_xmit()

This makes enetc_xdp_xmit() account for every frame in the batch that was
not handed to hardware, instead of only counting a single drop on
enetc_xdp_tx() failure.  The drop counter is now bumped by the remainder
num_frames - xdp_tx_frm_cnt, which also covers the too-many-fragments and
enetc_xdp_frame_to_xdp_tx_swbd() failure cases.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 80f0082f6c633..e7ecdf8ef67c8 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c

[ ... ]

> @@ -1854,6 +1853,7 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
>  		enetc_update_tx_ring_tail(tx_ring);
>  
>  	tx_ring->stats.xdp_tx += xdp_tx_frm_cnt;
> +	tx_ring->stats.xdp_tx_drops += num_frames - xdp_tx_frm_cnt;

[Severity: Medium]
Should the early exit at the top of enetc_xdp_xmit() also be counted here?
That path returns before this new accumulation runs:

drivers/net/ethernet/freescale/enetc/enetc.c:enetc_xdp_xmit() {
	if (unlikely(test_bit(ENETC_TX_DOWN, &priv->flags) ||
		     !netif_carrier_ok(ndev)))
		return -ENETDOWN;
	...
}

On a negative return, bq_xmit_all() in kernel/bpf/devmap.c discards the
whole batch:

	sent = dev->netdev_ops->ndo_xdp_xmit(dev, to_send, bq->q, flags);
	if (sent < 0) {
		err = sent;
		sent = 0;
	}
	for (i = sent; unlikely(i < to_send); i++)
		xdp_return_frame_rx_napi(bq->q[i]);

So all num_frames frames are dropped, but "Tx ring N XDP drops" in
ethtool -S stays unchanged.

The XDP_TX producer of the same counter in enetc_clean_rx_ring_xdp() does
count the identical condition:

		case XDP_TX:
			tx_ring = priv->xdp_tx_ring[rx_ring->index];
			if (unlikely(test_bit(ENETC_TX_DOWN, &priv->flags))) {
				enetc_xdp_drop(rx_ring, orig_i, i);
				tx_ring->stats.xdp_tx_drops++;
				break;
			}

Given that the commit message says frames "will not be transmitted" should
be tracked by drop counters, does leaving the -ENETDOWN case uncounted give
the same counter two different meanings depending on which in-driver path
produced the drop?  tx_ring is derivable there as
priv->xdp_tx_ring[smp_processor_id()], so the early return could bump the
counter by num_frames before returning.

[Severity: Medium]
This isn't a bug introduced by this patch, but the new accumulation inherits
an unsynchronized read-modify-write on tx_ring->stats.xdp_tx_drops, the same
way the existing xdp_tx += line does.  The fields in struct
enetc_ring_stats are plain unsigned long, with no per-CPU separation and no
u64_stats_sync.

The same ring's counters are also written from NAPI in
enetc_clean_rx_ring_xdp():

			tx_ring = priv->xdp_tx_ring[rx_ring->index];
			...
				tx_ring->stats.xdp_tx_drops++;

enetc_xdp_xmit() picks the ring with smp_processor_id() while XDP_TX picks it
with rx_ring->index, so both can target the same tx_ring from different CPUs
once RX IRQ affinity is not the identity mapping (enetc_setup_irqs() only
installs a hint via irq_set_affinity_hint(), and userspace can rewrite
/proc/irq/*/smp_affinity).

The only lock held is enetc_lock_mdio(), which is a shared read_lock() and
compiles away unless the errata static branch is enabled:

drivers/net/ethernet/freescale/enetc/enetc_hw.h:enetc_lock_mdio() {
	if (static_branch_unlikely(&enetc_has_err050089))
		read_lock(&enetc_mdio_lock);
}

Can increments be lost this way, and is it worth converting these ring stats
to u64_stats_sync while touching the accounting?

>  
>  	enetc_unlock_mdio();
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915222735.1016937-1-vladimir.oltean%40nxp.com