|SUCCESS| [v2] net/tap: use offsets provided by rte_net_get_ptype

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163307

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-27

Reviewing patch from Robin Jarry <rjarry at redhat.com>:

Subject: [PATCH dpdk v2] net/tap: use offsets provided by rte_net_get_ptype

This patch modifies tap_verify_csum() to use the rte_net_hdr_lens
structure populated by rte_net_get_ptype() instead of manually
calculating header offsets. This enables support for stacked VLAN/QinQ
tags and IPv6 extensions.

> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index a5d460a0b3cb..546bedea34df 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c

[ ... ]

> @@ -327,79 +327,66 @@ tun_alloc(struct pmd_internals *pmd, int is_keepalive, int persistent)
>  }
>  
>  static void
> -tap_verify_csum(struct rte_mbuf *mbuf)
> +tap_verify_csum(struct rte_mbuf *mbuf, const struct rte_net_hdr_lens *hlen)
>  {
> -	uint32_t l2 = mbuf->packet_type & RTE_PTYPE_L2_MASK;
>  	uint32_t l3 = mbuf->packet_type & RTE_PTYPE_L3_MASK;
>  	uint32_t l4 = mbuf->packet_type & RTE_PTYPE_L4_MASK;
> -	unsigned int l2_len = sizeof(struct rte_ether_hdr);
> -	unsigned int l3_len;
> +	uint32_t l4_off = hlen->l2_len + hlen->l3_len;
>  	uint16_t cksum = 0;
>  	void *l3_hdr;
>  	void *l4_hdr;
> -	struct rte_udp_hdr *udp_hdr;
>  
> -	if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
> -		l2_len += 4;
> -	else if (l2 == RTE_PTYPE_L2_ETHER_QINQ)
> -		l2_len += 8;
>  	/* Don't verify checksum for packets with discontinuous L2 header */
> -	if (unlikely(l2_len + sizeof(struct rte_ipv4_hdr) >
> -		     rte_pktmbuf_data_len(mbuf)))
> +	if (unlikely(l4_off > rte_pktmbuf_data_len(mbuf)))
>  		return;

The function dereferences hlen->l2_len and hlen->l3_len to compute
l4_off. Does this code properly handle the case where hlen is NULL? The
function signature declares hlen as const pointer, but can the caller
pass NULL?

> -	l3_hdr = rte_pktmbuf_mtod_offset(mbuf, void *, l2_len);
> +	l3_hdr = rte_pktmbuf_mtod_offset(mbuf, void *, hlen->l2_len);
>  	if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
>  		struct rte_ipv4_hdr *iph = l3_hdr;

[ ... ]

> @@ -561,10 +548,11 @@ pmd_rx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  			continue;
>  		}
>  
> -		mbuf->packet_type = rte_net_get_ptype(mbuf, NULL,
> +		struct rte_net_hdr_lens hlen = {0};

The coding style guide requires variable declarations at the beginning
of a block. Is this declaration properly placed according to the
guideline? The declaration appears after several statements within the
loop body.

Additionally, the style guide prefers explicit memset() calls over
initialization syntax for structures. Should this be memset(&hlen, 0,
sizeof(hlen)) instead?

> +		mbuf->packet_type = rte_net_get_ptype(mbuf, &hlen,
>  						      RTE_PTYPE_ALL_MASK);
>  		if (rxq->rxmode->offloads & RTE_ETH_RX_OFFLOAD_CHECKSUM)
> -			tap_verify_csum(mbuf);
> +			tap_verify_csum(mbuf, &hlen);
>  
>  		/* account for the receive frame */
>  		bufs[num_rx++] = mbuf;