[PATCH] net/iavf: fix VF reset race and stale ARQ message handling

[Loftus, Ciara]

> Subject: [PATCH] net/iavf: fix VF reset race and stale ARQ message handling
> 
> During VF reset, multiple issues can lead to initialization instability.
> 
> The first issue is a race condition in iavf_check_vf_reset_done(),
> where VFR state VFACTIVE is treated as both "reset not started" and
> "reset completed". This can allow the VF to proceed before the PF
> has acknowledged the reset, resulting in inconsistent initialization.
> 
> The second issue is the presence of stale messages in the Admin
> Receive Queue (ARQ) after VF reset. These may include opcode 0
> (VIRTCHNL_OP_UNKNOWN) or responses to commands issued before reset,
> which can interfere with API negotiation and cause command mismatch
> errors.
> 
> Additionally, opcode 0 messages generate excessive warning logs,
> causing unnecessary noise during initialization.
> 
> The solution involves:
> 1. Introducing a two-phase reset wait mechanism in
>    iavf_check_vf_reset_done():
>    - Wait for RSTAT to leave VFACTIVE to confirm reset start.
>    - Wait for RSTAT to return to VFACTIVE or COMPLETED to confirm
>      reset completion.
> 
> 2. Draining stale ARQ messages after admin queue initialization
>    and before issuing commands in iavf_init_vf().
> 
> 3. Downgrading opcode 0 message logging to DEBUG level while
>    preserving mismatch detection for other opcodes.
> 
> 4. Refactoring reset-start detection and ARQ drain logic into
>    helper functions (iavf_wait_for_reset_start() and
>    iavf_drain_arq()) to improve readability and reuse.
> 
> This ensures reliable VF reset handling and stable initialization.
> 
> Fixes: 28a1a72eac26 ("net/iavf: add VF initiated reset")

Is the fix only for the VF initiated reset sequence, or does it aim to
fix other reset sequences as well eg. PF initiated?

> Cc: stable at dpdk.org
> 
> Signed-off-by: Talluri Chaitanyababu <chaitanyababux.talluri at intel.com>
> ---
>  drivers/net/intel/iavf/iavf_ethdev.c | 54
> ++++++++++++++++++++++++++++
>  drivers/net/intel/iavf/iavf_vchnl.c  | 16 +++++++--
>  2 files changed, 67 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/intel/iavf/iavf_ethdev.c
> b/drivers/net/intel/iavf/iavf_ethdev.c
> index 1eca20bc9a..692e4455dc 100644
> --- a/drivers/net/intel/iavf/iavf_ethdev.c
> +++ b/drivers/net/intel/iavf/iavf_ethdev.c
> @@ -2002,11 +2002,36 @@ iavf_dev_rx_queue_intr_disable(struct
> rte_eth_dev *dev, uint16_t queue_id)
>  	return 0;
>  }
> 
> +/* Wait until PF acknowledges VF reset (RSTAT leaves VFACTIVE) */
> +static int
> +iavf_wait_for_reset_start(struct iavf_hw *hw)
> +{
> +	int i;
> +	uint32_t rstat;
> +
> +	for (i = 0; i < 100; i++) {
> +		rte_delay_ms(10);
> +
> +		rstat = IAVF_READ_REG(hw, IAVF_VFGEN_RSTAT);
> +		rstat &= IAVF_VFGEN_RSTAT_VFR_STATE_MASK;
> +		rstat >>= IAVF_VFGEN_RSTAT_VFR_STATE_SHIFT;
> +
> +		if (rstat != VIRTCHNL_VFR_VFACTIVE)
> +			return 0;
> +	}
> +
> +	return -1;
> +}
> +
>  static int
>  iavf_check_vf_reset_done(struct iavf_hw *hw)
>  {
>  	int i, reset;
> 
> +	/* Phase 1: wait for reset to start (leave VFACTIVE) */
> +	if (iavf_wait_for_reset_start(hw) != 0)
> +		PMD_DRV_LOG(DEBUG, "VF reset did not start within
> timeout");

There are multiple paths that lead to the iavf_wait_for_reset_start
function being called, including during device initialisation
iavf_init_vf -> iavf_check_vf_reset_done. In this case, we are not
expecting a reset event, just making sure we are not trying to
initialise a VF in the middle of a reset. I think the
iavf_wait_for_reset_start is not needed here, and the associated log
would be incorrect. Can you check that
iavf_wait_for_reset_start is only triggered when absolutely necessary.
Consider these events: device init, pf initiated reset, vf initiated
reset, queue pair change (iavf_queues_req_reset)

> +
>  	for (i = 0; i < IAVF_RESET_WAIT_CNT; i++) {
>  		reset = IAVF_READ_REG(hw, IAVF_VFGEN_RSTAT) &
>  			IAVF_VFGEN_RSTAT_VFR_STATE_MASK;
> @@ -2517,6 +2542,31 @@ iavf_init_proto_xtr(struct rte_eth_dev *dev)
>  	}
>  }
> 
> +/* Drain stale Admin Receive Queue messages after reset */
> +static void
> +iavf_drain_arq(struct iavf_hw *hw, struct iavf_info *vf)
> +{
> +	struct iavf_arq_event_info event;
> +	int drain_count = 0;
> +
> +	memset(&event, 0, sizeof(event));
> +	event.msg_buf = vf->aq_resp;
> +
> +	while (drain_count < IAVF_AQ_LEN) {
> +		event.buf_len = IAVF_AQ_BUF_SZ;
> +
> +		if (iavf_clean_arq_element(hw, &event, NULL) !=
> IAVF_SUCCESS)
> +			break;
> +
> +		drain_count++;
> +	}
> +
> +	if (drain_count > 0)
> +		PMD_INIT_LOG(DEBUG,
> +				"Drained %d stale ARQ messages",
> +				drain_count);
> +}
> +
>  static int
>  iavf_init_vf(struct rte_eth_dev *dev)
>  {
> @@ -2558,6 +2608,10 @@ iavf_init_vf(struct rte_eth_dev *dev)
>  		PMD_INIT_LOG(ERR, "unable to allocate vf_aq_resp
> memory");
>  		goto err_aq;
>  	}
> +
> +	/* Drain stale ARQ messages after VF reset */
> +	iavf_drain_arq(hw, vf);

Would we expect stale messages in the adminq here, it was only just
allocated a few lines above?

> +
>  	if (iavf_check_api_version(adapter) != 0) {
>  		PMD_INIT_LOG(ERR, "check_api version failed");
>  		goto err_api;
> diff --git a/drivers/net/intel/iavf/iavf_vchnl.c
> b/drivers/net/intel/iavf/iavf_vchnl.c
> index 08dd6f2d7f..a014be8b57 100644
> --- a/drivers/net/intel/iavf/iavf_vchnl.c
> +++ b/drivers/net/intel/iavf/iavf_vchnl.c
> @@ -296,11 +296,21 @@ iavf_read_msg_from_pf(struct iavf_adapter
> *adapter, uint16_t buf_len,
>  					__func__, vpe->event);
>  		}
>  	}  else {
> -		/* async reply msg on command issued by vf previously */
> +		/* Async reply for previously issued VF command.
> +		 * Stale messages from before reset are ignored, and polling
> +		 * continues until the expected response is received.
> +		 */
>  		result = IAVF_MSG_CMD;
>  		if (opcode != vf->pend_cmd) {
> -			PMD_DRV_LOG(WARNING, "command mismatch,
> expect %u, get %u",
> -					vf->pend_cmd, opcode);
> +			if (opcode == VIRTCHNL_OP_UNKNOWN)
> +				PMD_DRV_LOG(DEBUG,
> +						"Ignoring stale msg (opcode
> 0), pending cmd %u",
> +						vf->pend_cmd);
> +			else
> +				PMD_DRV_LOG(WARNING,
> +						"command mismatch, expect
> %u, get %u",
> +						vf->pend_cmd, opcode);
> +
>  			result = IAVF_MSG_ERR;
>  		}
>  	}
> --
> 2.43.0