Re: [PATCH net-next v6] net: airoha: add LRO offload support

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

Critical: 0 · High: 0 · Medium: 1 · Low: 3

- [Medium] The new LRO drop path in `airoha_qdma_rx_process()` increments
  `netdev->stats.rx_dropped` (the legacy `struct net_device_stats`…
- [Low] The commit message claims "aggregated skbs are marked
  SKB_GSO_DODGY so that the stack does not rely on gso_size and does not…
- [Low] Teardown asymmetry for the shared FE/CDM LRO state:
  `airoha_dev_set_features()` arms the per-QDMA LRO block via…
- [Low] The new LRO error path performs a plain, non-atomic read-modify-
  write of the shared per-netdev counter `netdev->stats.rx_dropped++`…

--

net: airoha: add LRO offload support

This adds LRO offload to airoha_eth using the 8 dedicated LRO hardware
queues mapped to RX queues 24-31, bumping those queues' page_pool order
since LRO cannot use scatter-gather.  Aggregated skbs get GSO metadata
and a pseudo-header checksum so forwarded traffic can be re-segmented,
and NETIF_F_LRO is only offered while a single device uses the QDMA.

Link: https://lore.kernel.org/r/20260831-airoha-eth-lro-v5-1-6b0f50401121@kernel.org (local)

A few questions below.

[ ... ]

[ ... ]

+	shinfo = skb_shinfo(skb);
+	shinfo->gso_type = ipv4 ? SKB_GSO_TCPV4 : SKB_GSO_TCPV6;
+	shinfo->gso_type |= SKB_GSO_DODGY;
+	shinfo->gso_size = DIV_ROUND_UP(len - data_off, agg_count);
+	shinfo->gso_segs = agg_count;

[Severity: Low]
Is the commit message description of SKB_GSO_DODGY the right way round?
It says:

    "aggregated skbs are marked SKB_GSO_DODGY so that the stack does not
     rely on gso_size and does not merge the aggregate into the GRO
     engine"

SKB_GSO_DODGY means the opposite for gso_segs: it tells the stack that
gso_segs is untrusted and must be recomputed from gso_size.  In
qdisc_pkt_len_segs_init() in net/core/dev.c:

	if (unlikely(shinfo->gso_type & SKB_GSO_DODGY)) {
		int payload = skb->len - hdr_len;
		...
		gso_segs = DIV_ROUND_UP(payload, shinfo->gso_size);

and the TCP GSO output path uses gso_size as the MSS when re-segmenting,
so the approximated gso_size is authoritative rather than advisory.

The scheme still looks workable, since DIV_ROUND_UP(sum, agg_count) can
never exceed the largest merged segment and the re-segmented packets only
come out smaller.  Could the commit message state that rationale instead
of saying the stack does not rely on gso_size?

[ ... ]

                                ^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Does this counter ever reach userspace?  This driver registers
.ndo_get_stats64 = airoha_dev_get_stats64, and that callback assigns
rx_dropped from its own MIB-derived stats:

	storage->rx_dropped = dev->stats.rx_drops;

dev_get_stats() in net/core/dev.c memsets storage and calls
ndo_get_stats64 when it exists, only falling back to
netdev_stats_to_stats64(storage, &dev->stats) when there is no
ndo_get_stats64/ndo_get_stats, and afterwards only adds dev->core_stats.

So every packet dropped here (agg_count above
AIROHA_RXQ_LRO_MAX_AGG_COUNT, IPv6 with extension headers so nexthdr is
not NEXTHDR_TCP, iph->ihl below 5, th->doff below 5, or a frame shorter
than the headers) appears nowhere in ip -s link or rtnetlink.

Would dev_core_stats_rx_dropped_inc(netdev), or the driver's own
dev->stats.rx_drops under its u64_stats syncp, be the better sink here?

[Severity: Low]
Separately, can this increment lose counts?  netdev->stats.rx_dropped is
a plain unsigned long shared per netdev, and this is a non-atomic
read-modify-write from NAPI context.

Each RX ring has its own NAPI instance (q->napi, 32 rings across the IRQ
banks) and the target netdev is resolved per descriptor by
airoha_qdma_get_gdm_dev(), so two pollers on different CPUs, for example
LRO rings 24 and 31 both resolving to the same airoha_gdm_dev, can reach:

	airoha_qdma_rx_napi_poll()
	  airoha_qdma_rx_process()
	    netdev->stats.rx_dropped++;

concurrently with no lock and no per-CPU separation, which also tears on
32-bit.

+				dev_kfree_skb(q->skb);
+				q->skb = NULL;
+				continue;
+			}
+
+			q->skb->protocol = eth_type_trans(q->skb, netdev);
 		} else { /* scattered frame */
 			struct skb_shared_info *shinfo = skb_shinfo(q->skb);
 			int nr_frags = shinfo->nr_frags;

[ ... ]

[Severity: Low]
Should the enable path here be gated on netif_running()?  There is no
admin-state check, so ethtool -K ethX lro on arms the shared FE/CDM LRO
block of the QDMA while the interface is still down.

The only teardown sites for that hardware state are ndo_stop and a later
ndo_set_features.  unregister_netdev() does not call ndo_stop for an
already-closed device, and neither airoha_qdma_cleanup() nor
airoha_hw_cleanup() touches REG_CDM_LRO_EN, REG_CDM_LRO_RXQ or
REG_CDM_LRO_LIMIT, so after this sequence:

	ethtool -K ethX lro on      # link down, LRO armed anyway
	rmmod airoha_eth

the CDM LRO engine stays enabled and mapped to RX queues 24-31 after
those rings, descriptors and page pools are gone.

This looks like leftover hardware state rather than something worse:
airoha_qdma_cleanup() clears GLOBAL_CFG_RX_DMA_EN_MASK and polls
GLOBAL_CFG_RX_DMA_BUSY_MASK before the RX buffers are released, so no DMA
into freed pages, and airoha_hw_init() asserts and deasserts the fe reset
on the next probe before airoha_fe_init(), so the stale enable bits do not
survive a rebind and cannot make airoha_dev_open() return -EBUSY forever.

Would it make sense to call airoha_dev_lro_disable() from the remove path
as well, so enable and disable are symmetric?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906-airoha-eth-lro-v6-1-a6cc5179a8c5%40kernel.org