|SUCCESS| [2/2] net/i40e: fix blocking link wait on device start

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-03

Reviewing patch series: net/ice and net/i40e link update changes

Commit 1: 6c76b76dc64183eb2f24a52b90d4ff9feb4872f4
Author: Ciara Loftus <ciara.loftus at intel.com>
Subject: net/ice: revert fix link up when starting device

This commit reverts a previous workaround that used blocking link status
update during device start. The commit message explains that an adminq
notification should handle link status updates correctly, making the
blocking delay unnecessary.

> diff --git a/drivers/net/intel/ice/ice_ethdev.c b/drivers/net/intel/ice/ice_ethdev.c
> index d2734b6688..da570d03cb 100644
> --- a/drivers/net/intel/ice/ice_ethdev.c
> +++ b/drivers/net/intel/ice/ice_ethdev.c
> @@ -4464,7 +4464,7 @@ ice_dev_start(struct rte_eth_dev *dev)
>  	ice_dev_set_link_up(dev);
>  
>  	/* Call get_link_info aq command to enable/disable LSE */
> -	ice_link_update(dev, 1);
> +	ice_link_update(dev, 0);
>  
>  	pf->adapter_stopped = false;

The change from blocking (1) to non-blocking (0) link update appears
straightforward. However, the commit message states "an adminq
notification should arrive once the link is up" which requires interrupt
handling to be enabled. Does the ice driver ensure that LSE interrupts
are properly configured before ice_dev_start() calls ice_link_update()?
If interrupts aren't registered at this point in device start, could
this revert cause link status to remain incorrectly down?

---

Commit 2: (new commit)
Author: Ciara Loftus <ciara.loftus at intel.com>
Subject: net/i40e: fix blocking link wait on device start

This commit removes a blocking wait during device start for i40e and
defers MAC config to the first link-up event when needed.

> diff --git a/drivers/net/intel/i40e/i40e_ethdev.c b/drivers/net/intel/i40e/i40e_ethdev.c
> index e818b6fc7c..a7c00033e5 100644
> --- a/drivers/net/intel/i40e/i40e_ethdev.c
> +++ b/drivers/net/intel/i40e/i40e_ethdev.c
> @@ -2565,15 +2565,19 @@ i40e_dev_start(struct rte_eth_dev *dev)
>  					       I40E_AQ_EVENT_MEDIA_NA), NULL);
>  		if (ret != I40E_SUCCESS)
>  			PMD_DRV_LOG(WARNING, "Fail to set phy mask");
> -
> -		/* Call get_link_info aq command to enable/disable LSE */
> -		i40e_dev_link_update(dev, 1);
>  	}
>  
>  	if (dev->data->dev_conf.intr_conf.rxq == 0) {
> +		i40e_dev_link_update(dev, 0);
> +		pf->mac_config_on_link_up = !dev->data->dev_link.link_status;
>  		rte_eal_alarm_set(I40E_ALARM_INTERVAL,
>  				  i40e_dev_alarm_handler, dev);
>  	} else {
> +		/* Block if no interrupt handler can process link-up events, to help
> +		 * ensure that MAC config is applied when the link is up.
> +		 */
> +		i40e_dev_link_update(dev, !rte_intr_allow_others(intr_handle));
> +		pf->mac_config_on_link_up = !dev->data->dev_link.link_status;

Does rte_intr_allow_others() guarantee that link-state-change interrupts
will be processed? The comment says "if no interrupt handler can process
link-up events" but rte_intr_allow_others() checks for availability of
vectors beyond RX/TX queues. Could there be cases where
rte_intr_allow_others() returns true but LSC interrupts are disabled,
causing mac_config_on_link_up to remain true indefinitely?

Also, in both branches pf->mac_config_on_link_up is set to
!dev->data->dev_link.link_status. If link_status hasn't been updated
yet at this point in device start, could this incorrectly initialize
mac_config_on_link_up? For instance, if the link becomes up between when
the struct is memset and when link_status is read, would
mac_config_on_link_up be set incorrectly?

[ ... ]

> @@ -6899,9 +6904,21 @@ i40e_dev_handle_aq_msg(struct rte_eth_dev *dev)
>  			break;
>  		case i40e_aqc_opc_get_link_status:
>  			ret = i40e_dev_link_update(dev, 0);
> -			if (!ret)
> +			/* Some devices require MAC config to be applied when the link comes up. */
> +			if (!ret) {
> +				if (pf->mac_config_on_link_up && dev->data->dev_link.link_status) {
> +					uint16_t max_frame_size;
> +
> +					max_frame_size = dev->data->mtu ?
> +						dev->data->mtu + I40E_ETH_OVERHEAD :
> +						I40E_FRAME_SIZE_MAX;
> +					i40e_aq_set_mac_config(hw, max_frame_size, TRUE,
> +								false, 0, NULL);

Is the return value from i40e_aq_set_mac_config() checked? If this
command fails, does pf->mac_config_on_link_up remain true so it can be
retried, or will it be cleared even on failure?

The coding style guide requires explicit comparison with zero. Does
"max_frame_size = dev->data->mtu ? ..." violate this? Should it be
"dev->data->mtu != 0"?

Also, style guide section 3.1 states that boolean values should use
lowercase true/false. This code uses uppercase "TRUE" and mixed case
"false" in the same function call. Should these be consistent lowercase?

> +					pf->mac_config_on_link_up = false;
> +				}
>  				rte_eth_dev_callback_process(dev,
>  					RTE_ETH_EVENT_INTR_LSC, NULL);
> +			}
>  			break;

If pf->mac_config_on_link_up was true and link_status is up, but then
ret is non-zero from i40e_dev_link_update(), does this code skip the MAC
config and callback? Would mac_config_on_link_up remain set incorrectly?

> diff --git a/drivers/net/intel/i40e/i40e_ethdev.h b/drivers/net/intel/i40e/i40e_ethdev.h
> index d57c53f661..7524defdb7 100644
> --- a/drivers/net/intel/i40e/i40e_ethdev.h
> +++ b/drivers/net/intel/i40e/i40e_ethdev.h
> @@ -1191,6 +1191,8 @@ struct i40e_pf {
>  
>  	/* When firmware > 8.3, the enable flag for outer VLAN processing */
>  	bool fw8_3gt;
> +	/* MAC config needs re-applying when link first comes up */
> +	bool mac_config_on_link_up;

The struct i40e_pf is allocated but is this new field explicitly
initialized to false? If the struct uses rte_zmalloc() or similar, it
should be zeroed, but does i40e driver initialization guarantee this new
field starts as false?