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

[Loftus, Ciara]

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

Some comments below. I agree there is an issue here but I'm not sure about this
approach to fixing it. I've explained why inline.

One alternative solution could be to call the existing iavf_is_reset_detected helper
immediately after iavf_vf_reset() in iavf_dev_close() (on the reset path only). That
would ensure that the reset has started before before proceeding with tearing down
and reinitialising the adminq etc.

[snip]

> +
>  static int
>  iavf_init_vf(struct rte_eth_dev *dev)
>  {
> @@ -2654,6 +2706,10 @@ iavf_init_vf(struct rte_eth_dev *dev)
>  		goto err;
>  	}
> 
> +	/* Drain stale ARQ messages only during reset recovery */
> +	if (vf->in_reset_recovery)
> +		iavf_drain_arq(hw, vf);

Even with the guard, I still think this draining is unnecessary.
The adminq is initialised in the code block immediately before this.
I think it will be empty and there will be nothing to drain here.

> +
>  	if (iavf_check_api_version(adapter) != 0) {
>  		PMD_INIT_LOG(ERR, "check_api version failed");
>  		goto err_api;
> @@ -3289,6 +3345,8 @@ iavf_dev_reset(struct rte_eth_dev *dev)
>  	ret = iavf_dev_uninit(dev);
>  	if (ret)
>  		return ret;
> +	/* Add delay before re-initialization */
> +	rte_delay_ms(50);

Although it's a short delay it feels a bit heavy handed to unconditionally
delay here for every reset.

> 
>  	return iavf_dev_init(dev);
>  }
> @@ -3352,6 +3410,7 @@ iavf_handle_hw_reset(struct rte_eth_dev *dev,
> bool vf_initiated_reset)
>  {
>  	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(dev->data-
> >dev_private);
>  	struct iavf_adapter *adapter = dev->data->dev_private;
> +	struct iavf_hw *hw = IAVF_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
>  	int ret;
>  	bool restart_device = false;
> 
> @@ -3370,6 +3429,11 @@ iavf_handle_hw_reset(struct rte_eth_dev *dev,
> bool vf_initiated_reset)
>  	vf->in_reset_recovery = true;
>  	iavf_set_no_poll(adapter, false);
> 
> +	/* For VF-initiated reset, wait for PF to start processing it */
> +	if (vf_initiated_reset)
> +		if (iavf_wait_for_reset_start(hw) != 0)

At this point of the VF initiated reset we haven't indicated to
hardware yet that we want to reset. So this wait will always time out here.
The reset is kicked off @ iavf_vf_reset where the VIRTCHNL_OP_RESET_VF
op is sent. Only after that does it make sense to check the reset status
reported by the PF.

> +			PMD_DRV_LOG(WARNING, "PF did not acknowledge
> VF reset");
> +
>  	/* Call the pre reset callback */
>  	if (vf->pre_reset_cb != NULL)
>  		vf->pre_reset_cb(dev->data->port_id, vf->pre_reset_cb_arg);
> diff --git a/drivers/net/intel/iavf/iavf_vchnl.c
> b/drivers/net/intel/iavf/iavf_vchnl.c
> index 94ccfb5d6e..0a536a15e0 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