|SUCCESS| [v5,2/2] net/iavf: remove PHC polling from Rx datapath

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-20

I'll review this patch series focusing on correctness, coding style, API requirements, and guideline compliance.

commit sha: Not provided in patch (v5 1/2)
Author: Soumyadeep Hore <soumyadeep.hore at intel.com>
Subject: net/iavf: reuse device alarm for PHC sync

This patch consolidates PHC synchronization into the existing device
alarm handler instead of using a dedicated PHC alarm callback. The PHC
state is refreshed from the first active RX queue and propagated to all
active RX queues.

> diff --git a/drivers/net/intel/iavf/iavf.h b/drivers/net/intel/iavf/iavf.h
> index 403c61e2e8..9aad077621 100644
> --- a/drivers/net/intel/iavf/iavf.h
> +++ b/drivers/net/intel/iavf/iavf.h
> @@ -76,6 +76,7 @@
>  #define IAVF_QUEUE_ITR_INTERVAL_MAX     8160 /* 8160 us */
>  
>  #define IAVF_ALARM_INTERVAL 50000 /* us */
> +#define IAVF_PHC_SYNC_ALARM_INTERVAL_US 200000

Does this define require a comment explaining the chosen value and its
relationship to PHC drift tolerance?

> @@ -383,6 +384,9 @@ struct iavf_adapter {
>  	enum iavf_rx_func_type rx_func_type;
>  	enum iavf_tx_func_type tx_func_type;
>  	uint16_t fdir_ref_cnt;
> +	rte_spinlock_t phc_sync_lock;
> +	uint8_t phc_sync_ticks;
> +	bool phc_sync_paused;

Does this code follow the DPDK coding style rule that bool types should
be avoided in struct definitions (per section 3.2.1.2)?

> diff --git a/drivers/net/intel/iavf/iavf_ethdev.c b/drivers/net/intel/iavf/iavf_ethdev.c
> index 1eca20bc9a..3f49804aca 100644
> --- a/drivers/net/intel/iavf/iavf_ethdev.c
> +++ b/drivers/net/intel/iavf/iavf_ethdev.c

[ ... ]

> +static struct ci_rx_queue *
> +iavf_phc_sync_rxq_get(struct rte_eth_dev *dev)
> +{
> +	struct ci_rx_queue *rxq;
> +	uint16_t i;
> +
> +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> +		rxq = dev->data->rx_queues[i];
> +		if (rxq != NULL)

Does this code violate the implicit comparison rule (section 3.6.4)
which states "Do not explicitly compare a variable to 0 or NULL"?
Should this be written as "if (rxq)" ?

> +			return rxq;
> +	}
> +
> +	return NULL;
> +}

> +static void
> +iavf_phc_sync_update_all_rxq(struct rte_eth_dev *dev,
> +			     uint64_t phc_time,
> +			     uint64_t sw_cur_time)
> +{
> +	struct ci_rx_queue *rxq;
> +	uint16_t i;
> +
> +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> +		rxq = dev->data->rx_queues[i];
> +		if (rxq == NULL)

Same implicit comparison issue here - should this be "if (!rxq)" ?

> +			continue;
> +
> +		rxq->phc_time = phc_time;
> +		rxq->hw_time_update = sw_cur_time;
> +	}
> +}

> +static void
> +iavf_phc_sync_tick(struct rte_eth_dev *dev)
> +{
> +	struct iavf_adapter *adapter;
> +	const uint16_t phc_sync_ticks_max = RTE_MAX((uint16_t)1,
> +		(uint16_t)(IAVF_PHC_SYNC_ALARM_INTERVAL_US / IAVF_ALARM_INTERVAL));
> +	struct ci_rx_queue *sync_rxq;
> +	uint64_t sw_cur_time;
> +
> +	adapter = IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> +
> +	rte_spinlock_lock(&adapter->phc_sync_lock);
> +	if (adapter->phc_sync_paused || !iavf_phc_sync_alarm_needed(dev)) {
> +		adapter->phc_sync_ticks = 0;
> +		goto unlock;
> +	}
> +
> +	if (++adapter->phc_sync_ticks < phc_sync_ticks_max)
> +		goto unlock;
> +
> +	adapter->phc_sync_ticks = 0;
> +	sync_rxq = iavf_phc_sync_rxq_get(dev);
> +	if (sync_rxq == NULL)

Same implicit comparison style issue - should be "if (!sync_rxq)" ?

> +		goto unlock;
> +
> +	if (iavf_get_phc_time(sync_rxq) != 0) {

Does this code follow the implicit comparison style? Should this be
"if (iavf_get_phc_time(sync_rxq))" ?

> +		PMD_DRV_LOG(ERR, "get physical time failed");
> +		goto unlock;
> +	}
> +
> +	sw_cur_time = rte_get_timer_cycles() / (rte_get_timer_hz() / 1000);
> +	iavf_phc_sync_update_all_rxq(dev, sync_rxq->phc_time, sw_cur_time);
> +
> +unlock:
> +	rte_spinlock_unlock(&adapter->phc_sync_lock);
> +}

> @@ -2701,32 +2712,160 @@ iavf_dev_interrupt_handler(void *param)
>  	iavf_enable_irq0(hw);
>  }
>  
>  void
>  iavf_dev_alarm_handler(void *param)
>  {
>  	struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
> +	struct iavf_info *vf;
>  	if (dev == NULL || dev->data == NULL || dev->data->dev_private == NULL)

Does this code follow the implicit comparison style? Should these be
written as "if (!dev || !dev->data || !dev->data->dev_private)" ?

Also, does this declaration placement violate section 3.4.1 which
requires declarations before statements?

>  		return;
>  
> +	vf = IAVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
>  	struct iavf_hw *hw = IAVF_DEV_PRIVATE_TO_HW(dev->data->dev_private);

Does this code violate section 3.4.1 which requires all variable
declarations at the beginning of a block, not interspersed with
statements?

>  	uint32_t icr0;
>  
> -	iavf_disable_irq0(hw);
> +	if (!(vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_WB_ON_ITR)) {
> +		iavf_disable_irq0(hw);
>  
> -	/* read out interrupt causes */
> -	icr0 = IAVF_READ_REG(hw, IAVF_VFINT_ICR01);
> +		/* read out interrupt causes */
> +		icr0 = IAVF_READ_REG(hw, IAVF_VFINT_ICR01);
>  
> -	if (icr0 & IAVF_VFINT_ICR01_ADMINQ_MASK) {
> -		PMD_DRV_LOG(DEBUG, "ICR01_ADMINQ is reported");
> -		iavf_handle_virtchnl_msg(dev);
> +		if (icr0 & IAVF_VFINT_ICR01_ADMINQ_MASK) {
> +			PMD_DRV_LOG(DEBUG, "ICR01_ADMINQ is reported");
> +			iavf_handle_virtchnl_msg(dev);
> +		}
> +
> +		iavf_enable_irq0(hw);
>  	}
>  
> -	iavf_enable_irq0(hw);
> +	iavf_phc_sync_tick(dev);
>  
>  	rte_eal_alarm_set(IAVF_ALARM_INTERVAL,
>  			  iavf_dev_alarm_handler, dev);
>  }

> +static bool
> +iavf_phc_sync_alarm_needed(struct rte_eth_dev *dev)
> +{
> +	struct iavf_adapter *adapter;
> +
> +	adapter = IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> +
> +	if (adapter->closed || adapter->stopped)
> +		return false;
> +
> +	if (!(dev->data->dev_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP))
> +		return false;
> +
> +	if (dev->data->nb_rx_queues == 0)

Does this code follow implicit comparison style? Should be
"if (!dev->data->nb_rx_queues)" ?

> +		return false;
> +
> +	if (iavf_phc_sync_rxq_get(dev) == NULL)

Same implicit comparison style issue - should be
"if (!iavf_phc_sync_rxq_get(dev))" ?

> +		return false;
> +
> +	return true;
> +}

> +void
> +iavf_phc_sync_alarm_stop(struct rte_eth_dev *dev)
> +{
> +	struct iavf_adapter *adapter;
> +
> +	if (dev == NULL || dev->data == NULL || dev->data->dev_private == NULL)

Does this code follow implicit comparison style?

> +		return;
> +
> +	adapter = IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> +	rte_spinlock_lock(&adapter->phc_sync_lock);
> +	adapter->phc_sync_paused = true;
> +	adapter->phc_sync_ticks = 0;
> +	rte_spinlock_unlock(&adapter->phc_sync_lock);
> +}

> @@ -2808,6 +2947,7 @@ iavf_dev_init(struct rte_eth_dev *eth_dev)
>  	adapter->dev_data = eth_dev->data;
>  	adapter->stopped = 1;
>  	adapter->mac_primary_set = false;
> +	rte_spinlock_init(&adapter->phc_sync_lock);

Does this code properly handle cleanup if initialization fails after
this point? The spinlock does not need explicit cleanup, but should
this initialization order be documented?

> @@ -2847,9 +2987,9 @@ iavf_dev_init(struct rte_eth_dev *eth_dev)
>  
>  		/* enable uio intr after callback register */
>  		rte_intr_enable(pci_dev->intr_handle);
> -	else
> -		rte_eal_alarm_set(IAVF_ALARM_INTERVAL,
> -				  iavf_dev_alarm_handler, eth_dev);
> +
> +	rte_eal_alarm_set(IAVF_ALARM_INTERVAL,
> +			  iavf_dev_alarm_handler, eth_dev);

Does this code change the alarm setup logic intentionally? Previously
the alarm was only set in the else branch, now it's always set. Could
this cause duplicate alarms if interrupts are enabled?

---

commit sha: Not provided in patch (v5 2/2)
Author: Soumyadeep Hore <soumyadeep.hore at intel.com>
Subject: net/iavf: remove PHC polling from Rx datapath

This patch removes periodic PHC read/update checks from scalar and
vector flex RX paths, relying on control-path PHC sync.

> diff --git a/drivers/net/intel/iavf/iavf_rxtx.c b/drivers/net/intel/iavf/iavf_rxtx.c
> index 4ff6c18dc4..fabccc89bf 100644
> --- a/drivers/net/intel/iavf/iavf_rxtx.c
> +++ b/drivers/net/intel/iavf/iavf_rxtx.c
> @@ -1507,16 +1507,6 @@ iavf_recv_pkts_flex_rxd(void *rx_queue,
>  	rx_ring = rxq->rx_flex_ring;
>  	ptype_tbl = rxq->iavf_vsi->adapter->ptype_tbl;
>  
> -	if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
> -		uint64_t sw_cur_time = rte_get_timer_cycles() / (rte_get_timer_hz() / 1000);
> -
> -		if (sw_cur_time - rxq->hw_time_update > 4) {
> -			if (iavf_get_phc_time(rxq))
> -				PMD_DRV_LOG(ERR, "get physical time failed");
> -			rxq->hw_time_update = sw_cur_time;
> -		}
> -	}
> -

Does this removal ensure timestamp accuracy is maintained? The old code
updated PHC state every 4ms in the datapath, while patch 1/2 uses 200ms
intervals. Could this cause timestamp drift or inaccuracy?

[ ... ]

> @@ -1585,7 +1575,6 @@ iavf_recv_pkts_flex_rxd(void *rx_queue,
>  				rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high));
>  
>  			rxq->phc_time = ts_ns;
> -			rxq->hw_time_update = rte_get_timer_cycles() / (rte_get_timer_hz() / 1000);

Does this code still update hw_time_update anywhere? If not, does the
field become unused and should it be removed from the structure?

[ ... ]

> diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_avx2.c b/drivers/net/intel/iavf/iavf_rxtx_vec_avx2.c
> index db0462f0f5..9349646d55 100644
> --- a/drivers/net/intel/iavf/iavf_rxtx_vec_avx2.c
> +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_avx2.c
> @@ -514,18 +514,10 @