|SUCCESS| [v3, 3/3] doc: announce change to LLDP packet detection in iavf PMD

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

git sha: 20260417100804.1143121-2-ciara.loftus at intel.com
Author: Ciara Loftus <ciara.loftus at intel.com>
Subject: [PATCH v3 1/3] net/iavf: support LLDP Tx via mbuf ptype or dynfield

This commit adds an alternative method for transmitting LLDP packets via
the iavf PMD using the mbuf packet type RTE_PTYPE_L2_ETHER_LLDP instead
of the dynamic mbuf field. The new approach is enabled via the
enable_ptype_lldp devarg.

> diff --git a/drivers/net/intel/iavf/iavf.h b/drivers/net/intel/iavf/iavf.h
> index f8008d0fda..ef503a1b64 100644
> --- a/drivers/net/intel/iavf/iavf.h
> +++ b/drivers/net/intel/iavf/iavf.h
> @@ -323,6 +323,7 @@ struct iavf_devargs {
>  	int auto_reconfig;
>  	int no_poll_on_link_down;
>  	uint64_t mbuf_check;
> +	int enable_ptype_lldp;
>  };

[ ... ]

> diff --git a/drivers/net/intel/iavf/iavf_ethdev.c b/drivers/net/intel/iavf/iavf_ethdev.c
> index 3126d9b644..77e8c6b54b 100644
> --- a/drivers/net/intel/iavf/iavf_ethdev.c
> +++ b/drivers/net/intel/iavf/iavf_ethdev.c

[ ... ]

> @@ -1016,6 +1019,11 @@ iavf_dev_start(struct rte_eth_dev *dev)
>  	/* Check Tx LLDP dynfield */
>  	rte_pmd_iavf_tx_lldp_dynfield_offset =
>  		rte_mbuf_dynfield_lookup(IAVF_TX_LLDP_DYNFIELD, NULL);
> +	if (rte_pmd_iavf_tx_lldp_dynfield_offset > 0)
> +		PMD_DRV_LOG(WARNING,
> +			"The LLDP Tx dynamic mbuf field will be removed in a future release.");
> +	iavf_ptype_lldp_enabled = adapter->devargs.enable_ptype_lldp ||
> +					rte_pmd_iavf_tx_lldp_dynfield_offset > 0;

Does this logic correctly handle the case where the user has not set
enable_ptype_lldp and has not registered the dynfield? When both are
zero/negative, will iavf_ptype_lldp_enabled become false, even though
the dynfield offset being negative means it wasn't registered?

[ ... ]

> @@ -2795,6 +2808,8 @@ iavf_dev_init(struct rte_eth_dev *eth_dev)
>  		 */
>  		rte_pmd_iavf_tx_lldp_dynfield_offset =
>  			rte_mbuf_dynfield_lookup(IAVF_TX_LLDP_DYNFIELD, NULL);
> +		iavf_ptype_lldp_enabled = adapter->devargs.enable_ptype_lldp ||
> +						rte_pmd_iavf_tx_lldp_dynfield_offset > 0;

The same logic pattern appears here. Does this handle all initialization
paths correctly?

> diff --git a/drivers/net/intel/iavf/iavf_rxtx.h b/drivers/net/intel/iavf/iavf_rxtx.h
> index 80b06518b0..0157c4c37e 100644
> --- a/drivers/net/intel/iavf/iavf_rxtx.h
> +++ b/drivers/net/intel/iavf/iavf_rxtx.h
> @@ -157,14 +157,15 @@
>  
>  #define IAVF_TX_LLDP_DYNFIELD "intel_pmd_dynfield_tx_lldp"
>  #define IAVF_CHECK_TX_LLDP(m) \
> -	((rte_pmd_iavf_tx_lldp_dynfield_offset > 0) && \
> -	(*RTE_MBUF_DYNFIELD((m), \
> -			rte_pmd_iavf_tx_lldp_dynfield_offset, \
> -			uint8_t *)))
> +	(iavf_ptype_lldp_enabled && \
> +	(((m)->packet_type & RTE_PTYPE_L2_MASK) == RTE_PTYPE_L2_ETHER_LLDP || \
> +	(rte_pmd_iavf_tx_lldp_dynfield_offset > 0 && \
> +	*RTE_MBUF_DYNFIELD((m), rte_pmd_iavf_tx_lldp_dynfield_offset, uint8_t *))))

Does this macro preserve the original behavior when neither method is
enabled? When iavf_ptype_lldp_enabled is false, the entire expression
becomes false, which might not match the original logic.

Also, does dereferencing RTE_MBUF_DYNFIELD when
rte_pmd_iavf_tx_lldp_dynfield_offset is -1 cause undefined behavior in
the second part of the OR?


git sha: 20260417100804.1143121-3-ciara.loftus at intel.com
Author: Ciara Loftus <ciara.loftus at intel.com>
Subject: [PATCH v3 2/3] net/iavf: add AVX2 context descriptor Tx paths

This commit implements new AVX2 transmit paths that support using context
descriptors, enabling features like LLDP transmission on AVX2 paths.

> diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_avx2.c b/drivers/net/intel/iavf/iavf_rxtx_vec_avx2.c
> index db0462f0f5..2e7fe96d2b 100644
> --- a/drivers/net/intel/iavf/iavf_rxtx_vec_avx2.c
> +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_avx2.c

[ ... ]

> +static inline void
> +iavf_fill_ctx_desc_tunneling_avx2(uint64_t *low_ctx_qw, struct rte_mbuf *pkt)
> +{
> +	if (pkt->ol_flags & RTE_MBUF_F_TX_TUNNEL_MASK) {
> +		uint64_t eip_typ = IAVF_TX_CTX_DESC_EIPT_NONE;
> +		uint64_t eip_len = 0;
> +		uint64_t eip_noinc = 0;
> +		/* Default - IP_ID is increment in each segment of LSO */
> +
> +		switch (pkt->ol_flags & (RTE_MBUF_F_TX_OUTER_IPV4 |
> +				RTE_MBUF_F_TX_OUTER_IPV6 |
> +				RTE_MBUF_F_TX_OUTER_IP_CKSUM)) {
> +		case RTE_MBUF_F_TX_OUTER_IPV4:
> +			eip_typ = IAVF_TX_CTX_DESC_EIPT_IPV4_NO_CHECKSUM_OFFLOAD;
> +			eip_len = pkt->outer_l3_len >> 2;
> +		break;

Does pkt->outer_l3_len contain a valid value when
RTE_MBUF_F_TX_OUTER_IPV4 is set? Could this result in using an
uninitialized field if the application did not set outer_l3_len?

[ ... ]

> +static inline void
> +iavf_fill_ctx_desc_tunneling_field(volatile uint64_t *qw0,
> +		const struct rte_mbuf *m)
> +{
> +	uint64_t eip_typ = IAVF_TX_CTX_DESC_EIPT_NONE;
> +	uint64_t eip_len = 0;
> +	uint64_t eip_noinc = 0;
> +	/* Default - IP_ID is increment in each segment of LSO */
> +
> +	switch (m->ol_flags & (RTE_MBUF_F_TX_OUTER_IPV4 |
> +			RTE_MBUF_F_TX_OUTER_IPV6 |
> +			RTE_MBUF_F_TX_OUTER_IP_CKSUM)) {

This function appears to duplicate the logic from
iavf_fill_ctx_desc_tunneling_avx2. Does one function take const mbuf
while the other does not for a specific reason? Could these be unified?

[ ... ]

> +static __rte_always_inline void
> +ctx_vtx1(volatile struct ci_tx_desc *txdp, struct rte_mbuf *pkt,
> +		uint64_t flags, bool offload, uint8_t vlan_flag)
> +{
> +	uint64_t high_ctx_qw = IAVF_TX_DESC_DTYPE_CONTEXT;
> +	uint64_t low_ctx_qw = 0;
> +
> +	if (offload) {
> +		iavf_fill_ctx_desc_tunneling_avx2(&low_ctx_qw, pkt);

Does this code validate that the tunneling fields are properly set in the
mbuf before calling this function? What happens if the application provides
invalid tunnel information?

> +#ifdef IAVF_TX_VLAN_QINQ_OFFLOAD
> +		if (pkt->ol_flags & RTE_MBUF_F_TX_QINQ) {
> +			uint64_t qinq_tag = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2 ?
> +				(uint64_t)pkt->vlan_tci_outer :
> +				(uint64_t)pkt->vlan_tci;
> +			high_ctx_qw |= IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT;
> +			low_ctx_qw |= qinq_tag << IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> +		} else if ((pkt->ol_flags & RTE_MBUF_F_TX_VLAN) &&
> +				vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2) {
> +			high_ctx_qw |= IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT;
> +			low_ctx_qw |= (uint64_t)pkt->vlan_tci << IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> +		}
> +#endif

Is IAVF_TX_VLAN_QINQ_OFFLOAD defined somewhere in the codebase? This
looks like dead code if that symbol is never defined.

> +	}
> +	if (IAVF_CHECK_TX_LLDP(pkt))
> +		high_ctx_qw |= IAVF_TX_CTX_DESC_SWTCH_UPLINK << IAVF_TXD_CTX_QW1_CMD_SHIFT;
> +	uint64_t high_data_qw = (IAVF_TX_DESC_DTYPE_DATA |
> +				((uint64_t)flags  << IAVF_TXD_QW1_CMD_SHIFT) |
> +				((uint64_t)pkt->data_len << IAVF_TXD_QW1_TX_BUF_SZ_SHIFT));

Does this logic verify that pkt->data_len fits within the available bits
after the shift? Could large data_len values cause the shifted result to
overflow?

[ ... ]

> +static __rte_always_inline void
> +ctx_vtx(volatile struct ci_tx_desc *txdp,
> +		struct rte_mbuf **pkt, uint16_t nb_pkts, uint64_t flags,
> +		bool offload, uint8_t vlan_flag)
> +{

[ ... ]

> +	for (; nb_pkts > 1; txdp += 4, pkt += 2, nb_pkts -= 2) {

Does this loop correctly handle alignment requirements? The loop processes
packets in pairs and increments txdp by 4 descriptors, which is 2
descriptors per packet. Does this match the ctx_vtx1 behavior which uses 2
descriptors per packet?

[ ... ]

> +static __rte_always_inline uint16_t
> +iavf_xmit_fixed_burst_vec_avx2_ctx(void *tx_queue, struct rte_mbuf **tx_pkts,
> +				 uint16_t nb_pkts, bool offload)
> +{
> +	struct ci_tx_queue *txq = (struct ci_tx_queue *)tx_queue;
> +	volatile struct ci_tx_desc *txdp;
> +	struct ci_tx_entry_vec *txep;
> +	uint16_t n, nb_commit, nb_mbuf, tx_id;
> +	/* bit2 is reserved and must be set to 1 according to Spec */
> +	uint64_t flags = IAVF_TX_DESC_CMD_EOP | IAVF_TX_DESC_CMD_ICRC;
> +	uint64_t rs = IAVF_TX_DESC_CMD_RS | flags;
> +
> +	if (txq->nb_tx_free < txq->tx_free_thresh)
> +		ci_tx_free_bufs_vec(txq, iavf_tx_desc_done, true);
> +
> +	nb_commit = (uint16_t)RTE_MIN(txq->nb_tx_free, nb_pkts << 1);
> +	nb_commit &= 0xFFFE;

Does this mask operation guarantee correct behavior? The code left-shifts
nb_pkts by 1 to account for context descriptors, then masks the low bit.
Could this result in transmitting one fewer packet than requested in some
cases?