|SUCCESS| [v5, 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/163231

_AI Code Review_

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

I'll review this DPDK patch series for compliance with DPDK coding standards and contribution guidelines.

Git SHA: 20260422091609.1797686-2-ciara.loftus at intel.com
Author: Ciara Loftus <ciara.loftus at intel.com>
Subject: net/iavf: support LLDP Tx via mbuf ptype or dynfield

The patch series adds support for transmitting LLDP packets via mbuf
packet type in the iavf PMD, providing an alternative to the existing
dynamic mbuf field approach. It also implements AVX2 context descriptor
transmit paths.

> diff --git a/drivers/net/intel/iavf/iavf_ethdev.c b/drivers/net/intel/iavf/iavf_ethdev.c
> index 3126d9a644..288d701b7e 100644
> --- a/drivers/net/intel/iavf/iavf_ethdev.c
> +++ b/drivers/net/intel/iavf/iavf_ethdev.c

[ ... ]

> @@ -1016,6 +1018,26 @@ 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,
> +			"Using a dynamic mbuf field to identify LLDP packets is deprecated. "
> +			"Set the 'enable_ptype_lldp' driver option and mbuf LLDP ptypes instead.");
> +		if (adapter->devargs.enable_ptype_lldp)
> +			PMD_DRV_LOG(WARNING,
> +				"Both ptype and dynfield LLDP enabled; ptype takes precedence.");
> +	}
> +
> +	for (uint16_t i = 0; i < dev->data->nb_tx_queues; i++) {
> +		struct ci_tx_queue *txq = dev->data->tx_queues[i];
> +		if (txq) {

In iavf_dev_start, does this code properly handle the case where
dev->data->tx_queues[i] is NULL? The check for txq exists but if it is
NULL, the queue's lldp_mode remains uninitialized. Should there be
handling for this condition?

> +			if (adapter->devargs.enable_ptype_lldp)
> +				txq->lldp_mode = IAVF_LLDP_PTYPE;
> +			else if (rte_pmd_iavf_tx_lldp_dynfield_offset > 0)
> +				txq->lldp_mode = IAVF_LLDP_DYNFIELD;
> +			else
> +				txq->lldp_mode = IAVF_LLDP_DISABLED;
> +		}
> +	}

> diff --git a/drivers/net/intel/iavf/iavf_rxtx.c b/drivers/net/intel/iavf/iavf_rxtx.c
> index 3d9b49efd0..4828655ea7 100644
> --- a/drivers/net/intel/iavf/iavf_rxtx.c
> +++ b/drivers/net/intel/iavf/iavf_rxtx.c
> @@ -2343,7 +2343,7 @@ iavf_recv_pkts_bulk_alloc(void *rx_queue,
>  
>  /* Check if the context descriptor is needed for TX offloading */
>  static inline uint16_t
> -iavf_calc_context_desc(const struct rte_mbuf *mb, uint8_t vlan_flag)
> +iavf_calc_context_desc(const struct rte_mbuf *mb, uint8_t vlan_flag, uint8_t lldp_mode)

Does this line exceed 79 characters? This should be checked and wrapped
if necessary according to the coding style guide.

> @@ -2542,17 +2542,18 @@ iavf_get_context_desc(uint64_t ol_flags, const struct rte_mbuf *mbuf,
>  		      const struct ci_tx_queue *txq,
>  		      uint64_t *qw0, uint64_t *qw1)
>  {
> -	uint8_t iavf_vlan_flag;
> +	uint8_t iavf_vlan_flag, lldp_mode;

Multiple declarations should be on separate lines according to the
coding style guide. Should these be declared separately?

> diff --git a/drivers/net/intel/iavf/iavf_rxtx.h b/drivers/net/intel/iavf/iavf_rxtx.h
> index 80b06518b0..054cffd60c 100644
> --- a/drivers/net/intel/iavf/iavf_rxtx.h
> +++ b/drivers/net/intel/iavf/iavf_rxtx.h
> @@ -156,11 +156,18 @@
>  		(RTE_MBUF_F_TX_OFFLOAD_MASK ^ IAVF_TX_OFFLOAD_MASK)
>  
>  #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 *)))
> +
> +/* LLDP Tx modes */
> +#define IAVF_LLDP_DISABLED 0
> +#define IAVF_LLDP_PTYPE    1
> +#define IAVF_LLDP_DYNFIELD 2
> +
> +#define IAVF_CHECK_TX_LLDP(m, lldp_mode) \
> +	((lldp_mode) && \
> +	((((lldp_mode) == IAVF_LLDP_PTYPE) && \
> +	((m)->packet_type & RTE_PTYPE_L2_MASK) == RTE_PTYPE_L2_ETHER_LLDP) || \
> +	(((lldp_mode) == IAVF_LLDP_DYNFIELD) && \
> +	*RTE_MBUF_DYNFIELD((m), rte_pmd_iavf_tx_lldp_dynfield_offset, uint8_t *))))

In IAVF_CHECK_TX_LLDP, when lldp_mode is IAVF_LLDP_DYNFIELD, does this
code verify that rte_pmd_iavf_tx_lldp_dynfield_offset is valid before
dereferencing? The previous version checked if the offset was greater
than 0, but this appears to be missing now.

> diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
> index 4e8bf94fa0..c9422971b7 100644
> --- a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
> +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c

[ ... ]

> @@ -2080,7 +2080,7 @@ ctx_vtx1(volatile struct ci_tx_desc *txdp, struct rte_mbuf *pkt,
>  		}
>  #endif
>  	}
> -	if (IAVF_CHECK_TX_LLDP(pkt))
> +	if (IAVF_CHECK_TX_LLDP(pkt, lldp_mode))

In ctx_vtx1() within iavf_rxtx_vec_avx512.c, the function signature was
updated to include lldp_mode, but does the function properly handle all
three LLDP modes (DISABLED, PTYPE, DYNFIELD)?

> @@ -2258,6 +2258,7 @@ iavf_xmit_fixed_burst_vec_avx512_ctx(void *tx_queue, struct rte_mbuf **tx_pkts,
>  	/* bit2 is reserved and must be set to 1 according to Spec */
>  	uint64_t flags = CI_TX_DESC_CMD_EOP | CI_TX_DESC_CMD_ICRC;
>  	uint64_t rs = CI_TX_DESC_CMD_RS | flags;
> +	uint8_t lldp_mode = txq->lldp_mode;

Does this code ensure that txq->lldp_mode has been properly initialized
before use? If the queue was created before dev_start was called, could
lldp_mode contain an undefined value?

> diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_avx2.c b/drivers/net/intel/iavf/iavf_rxtx_vec_avx2.c
> index db0462f0f5..aa60e71857 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)
> +{

[ ... ]

> +static inline void
> +iavf_fill_ctx_desc_tunneling_field(volatile uint64_t *qw0,
> +		const struct rte_mbuf *m)
> +{

Does this patch introduce two separate but nearly identical
implementations of tunneling field filling
(iavf_fill_ctx_desc_tunneling_avx2 and
iavf_fill_ctx_desc_tunneling_field)? Could these be consolidated into a
single function to reduce code duplication?

> +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, uint8_t lldp_mode)

Does this line exceed 79 characters? Should it be wrapped?

> +{
> +	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);

In the AVX2 ctx_vtx1 function, is the function name
iavf_fill_ctx_desc_tunneling_avx2 appropriate given that it doesn't
actually use AVX2 intrinsics, while iavf_fill_ctx_desc_tunneling_field
appears to be functionally equivalent?

> +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, uint8_t lldp_mode)
> +{
> +	uint64_t hi_data_qw_tmpl = (IAVF_TX_DESC_DTYPE_DATA |
> +					((uint64_t)flags  << IAVF_TXD_QW1_CMD_SHIFT));

Does this code contain extra whitespace before the left shift operator?
According to the coding style guide, there should be a single space on
either side of binary operators.

> +	for (; nb_pkts > 1; txdp += 4, pkt += 2, nb_pkts -= 2) {
> +		uint64_t hi_ctx_qw1 = IAVF_TX_DESC_DTYPE_CONTEXT;
> +		uint64_t hi_ctx_qw0 = IAVF_TX_DESC_DTYPE_CONTEXT;
> +		uint64_t low_ctx_qw1 = 0;
> +		uint64_t low_ctx_qw0 = 0;
> +		uint64_t hi_data_qw1 = 0;
> +		uint64_t hi_data_qw0 = 0;

Does this loop process packets in pairs but increment txdp by 4? Does
this assume that each packet uses a context descriptor plus a data
descriptor? Is this assumption always valid?

> +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)
> +{

[ ... ]

> +	nb_commit = (uint16_t)RTE_MIN(txq->nb_tx_free, nb_pkts << 1);
> +	nb_commit &= 0xFFFE;

Does this code correctly handle the case where nb_commit becomes 0 after
the bitwise AND operation? The check for unlikely(nb_commit == 0) occurs
after this line, but should validation happen before the bitwise
operation?

Overall, the patch appears functionally correct but has several areas
that need attention regarding coding style compliance, potential code
duplication, and proper initialization checks.