Re: [net-next,v5] net: airoha: add HW GRO offload support

[netdev-bot+sashiko@kernel.org]

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

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

- [High] LRO RX ring 31 is programmed as an active hardware-LRO
  destination but is bound to no IRQ bank, so it is never polled.
- [Medium] The driver advertises NETIF_F_GRO_HW while it cannot satisfy
  the documented rx-gro-hw contract, because the hardware does not…

--

net: airoha: add HW GRO offload support

This adds hardware GRO (LRO) offload to airoha_eth, mapping the 8 LRO
hardware contexts onto RX queues 24-31, bumping the page_pool order for
those queues and disabling scatter-gather on them.  Aggregated frames get
their network headers copied into a linear skb, the payload attached as a
frag, CHECKSUM_PARTIAL with a pseudo-header checksum, and GSO metadata
derived from the descriptor aggregation count.

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 64619e9a704dd..57b19d7b5bd79 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c

[ ... ]

> @@ -491,6 +493,88 @@ static void airoha_fe_crsn_qsel_init(struct airoha_eth *eth)
>  				 CDM_CRSN_QSEL_Q1));
>  }
>
> +static void airoha_fe_lro_rxq_enable(struct airoha_eth *eth, int qdma_id,
> +				     int lro_queue_index, int qid,
> +				     int buf_size)
> +{

[ ... ]

> +	airoha_fe_rmw(eth, REG_CDM_LRO_RXQ(id, lro_queue_index),
> +		      LRO_RXQ_MASK(lro_queue_index),
> +		      __field_prep(LRO_RXQ_MASK(lro_queue_index), qid));
> +	airoha_fe_set(eth, REG_CDM_LRO_EN(id), BIT(lro_queue_index));
> +}

[ ... ]

> +static void airoha_dev_lro_enable(struct airoha_gdm_dev *dev)
> +{
> +	struct airoha_qdma *qdma = airoha_qdma_deref(dev);
> +	struct airoha_eth *eth = qdma->eth;
> +	int qdma_id = qdma - &eth->qdma[0];
> +	int i, lro_queue_index = 0;
> +
> +	if (airoha_fe_lro_is_enabled(eth, qdma_id))
> +		return;
> +
> +	for (i = 0; i < ARRAY_SIZE(qdma->q_rx); i++) {
> +		struct airoha_queue *q = &qdma->q_rx[i];
> +		u32 size;
> +
> +		if (!q->ndesc)
> +			continue;
> +
> +		if (!airoha_qdma_is_lro_queue(q))
> +			continue;
> +
> +		size = SKB_WITH_OVERHEAD(AIROHA_RX_LEN(q->buf_size));
> +		size = min_t(u32, size, AIROHA_MAX_RX_SIZE);
> +		airoha_fe_lro_rxq_enable(eth, qdma_id, lro_queue_index, i,
> +					 size);
> +		lro_queue_index++;
> +	}
> +}

[Severity: High]
Can traffic steered to LRO context 7 be dropped here?  This loop enables an
LRO context for every RX ring selected by airoha_qdma_is_lro_queue(), and
AIROHA_RXQ_LRO_EN_MASK is GENMASK(31, 24), so ring 31 gets LRO context 7 and
BIT(7) is set in REG_CDM_LRO_EN.

RX ring 31 does not appear in any IRQ bank pin mask in airoha_eth.h:

	#define RX_IRQ0_BANK_PIN_MASK			0x839f
	#define RX_IRQ1_BANK_PIN_MASK			0x7fe00000
	#define RX_IRQ2_BANK_PIN_MASK			0x20
	#define RX_IRQ3_BANK_PIN_MASK			0x40

The union of those four masks covers queues 0-4, 7, 8, 15 and 21-30, but not
BIT(31).  Since airoha_qdma_hw_init() derives the RX interrupt enables from
those masks:

	airoha_qdma_irq_enable(&qdma->irq_banks[i], QDMA_INT_REG_IDX2,
			       INT_RX2_MASK(RX_IRQ_BANK_PIN_MASK(i)));

RX31_DONE is never enabled in any bank, and airoha_irq_handler() masks the
status with irq_bank->irqmask, so q_rx[31]'s NAPI is never scheduled.  The
re-arm loop in airoha_qdma_rx_napi_poll() is also a no-op for that qid:

	if (!(BIT(qid) & RX_IRQ_BANK_PIN_MASK(i)))
		continue;

The ring is still created and filled by airoha_qdma_fill_rx_queue(), so once
its descriptors are consumed by the LRO engine it is never drained or
refilled again.  Would either restricting the mask to GENMASK(30, 24) or
adding BIT(31) to one of the bank pin masks be the intended fix?  The v4
changelog in this posting lists "Enable rx interrupt for queue 31", but that
change does not seem to be present in this version of the patch.

[ ... ]

> @@ -657,12 +742,191 @@ airoha_qdma_get_gdm_dev(struct airoha_eth *eth, struct airoha_qdma_desc *desc)
>  	return port->devs[d] ? port->devs[d] : ERR_PTR(-ENODEV);
>  }
>
> +static struct sk_buff *airoha_qdma_lro_rx_skb(struct airoha_queue *q,
> +					      struct airoha_qdma_desc *desc,
> +					      struct airoha_queue_entry *e,
> +					      u32 msg1, u32 msg2, u32 len,
> +					      u32 agg_count)
> +{

[ ... ]

> +	page = virt_to_head_page(e->buf);
> +	data_len = len - data_off;
> +	shinfo = skb_shinfo(skb);
> +	skb_add_rx_frag(skb, shinfo->nr_frags, page,
> +			e->buf + data_off - page_address(page), data_len,
> +			q->buf_size);
> +
> +	shinfo->gso_type = ipv4 ? SKB_GSO_TCPV4 : SKB_GSO_TCPV6;
> +	/* HW does not report the per-segment MSS, so approximate it as
> +	 * the average segment size. The LRO engine always aggregates
> +	 * n-1 MSS-sized TCP segments plus one shorter tail for an
> +	 * aggregate of n segments, so this still re-segments into
> +	 * exactly agg_count segments, keeping gso_size/gso_segs and the
> +	 * packet accounting coherent, while never overestimating the MSS.
> +	 */
> +	shinfo->gso_size = DIV_ROUND_UP(data_len, agg_count);
> +	shinfo->gso_segs = agg_count;

[Severity: Medium]
Is NETIF_F_GRO_HW the right feature bit given this approximation?
Documentation/networking/netdev-features.rst says for rx-gro-hw:

	A packet stream merged by Hardware GRO must
	be re-segmentable by GSO or TSO back to the exact original packet stream.

With gso_size computed as the average payload size, the aggregate cannot be
re-segmented into the original stream whenever the tail segment is shorter
than the MSS, which the commit message describes as the normal case:

	The LRO engine always aggregates n-1 MSS-sized TCP segments plus one
	shorter tail for an aggregate of n segments

For a two-segment aggregate of 1460 + 1000 bytes, gso_size becomes 1230, and
GSO/TSO emits 1230 + 1230 rather than 1460 + 1000.  On the L3-forwarding path
this patch targets, airoha_dev_xmit() writes that value straight into the TX
descriptor MSS field:

	__be16 csum = cpu_to_be16(skb_shinfo(skb)->gso_size);

	tcp_hdr(skb)->check = (__force __sum16)csum;
	msg0 |= FIELD_PREP(QDMA_ETH_TXMSG_TSO_MASK, 1);

so forwarded flows end up with a reduced MSS.  Would NETIF_F_LRO be the more
accurate advertisement for an irreversible merge?

A related question on gso_segs: agg_count comes from
FIELD_GET(QDMA_ETH_RXMSG_AGG_COUNT_MASK, msg2) and is only bounded above by
AIROHA_RXQ_LRO_MAX_AGG_COUNT, while data_len has no lower bound beyond 1.  If
the engine ever aggregates segments that are not uniformly MSS-sized (for
example 64 segments totalling 100 payload bytes, giving gso_size 2 and 50
emitted segments), gso_segs would overstate the number of segments actually
produced, which feeds qdisc pkt_len accounting.  Is the uniform-segment
property guaranteed by the hardware for all aggregates?

[ ... ]

> @@ -3430,11 +3781,9 @@ static int airoha_alloc_gdm_device(struct airoha_eth *eth,
>  	netdev->ethtool_ops = &airoha_ethtool_ops;
>  	netdev->max_mtu = AIROHA_MAX_MTU;
>  	netdev->watchdog_timeo = 5 * HZ;
> -	netdev->hw_features = NETIF_F_IP_CSUM | NETIF_F_RXCSUM | NETIF_F_TSO6 |
> -			      NETIF_F_IPV6_CSUM | NETIF_F_SG | NETIF_F_TSO |
> -			      NETIF_F_HW_TC;
> -	netdev->features |= netdev->hw_features;
> -	netdev->vlan_features = netdev->hw_features;
> +	netdev->hw_features = AIROHA_HW_FEATURES | NETIF_F_GRO_HW;
> +	netdev->features |= AIROHA_HW_FEATURES;
> +	netdev->vlan_features = AIROHA_HW_FEATURES;
>  	SET_NETDEV_DEV(netdev, eth->dev);

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> index fa9a8edce22ff..84cadbb538666 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.h
> +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> @@ -45,6 +45,18 @@
>  	 (_n) == 15 ? 128 :		\
>  	 (_n) ==  0 ? 1024 : 16)
>
> +#define AIROHA_LRO_PAGE_ORDER		get_order(SZ_16K)
> +#define AIROHA_MAX_NUM_LRO_QUEUES	8
> +#define AIROHA_RXQ_LRO_EN_MASK		GENMASK(31, 24)

[ ... ]

>  #define PSE_RSV_PAGES			128
>  #define PSE_QUEUE_RSV_PAGES		64

[ ... ]

> @@ -714,6 +728,18 @@ static inline bool airoha_is_7583(struct airoha_eth *eth)
>  	return eth->soc->version == 0x7583;
>  }
>
> +static inline bool airoha_qdma_is_lro_queue(struct airoha_queue *q)
> +{
> +	struct airoha_qdma *qdma = q->qdma;
> +	int qid = q - &qdma->q_rx[0];
> +
> +	/* EN7581 SoC supports at most 8 LRO rx queues */
> +	BUILD_BUG_ON(hweight32(AIROHA_RXQ_LRO_EN_MASK) >
> +		     AIROHA_MAX_NUM_LRO_QUEUES);
> +
> +	return !!(AIROHA_RXQ_LRO_EN_MASK & BIT(qid));
> +}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831-airoha-eth-lro-v5-1-6b0f50401121%40kernel.org