RE: [PATCH 2/5] net/iavf: defer device start when PF VSI not ready

> Subject: [PATCH 2/5] net/iavf: defer device start when PF VSI not ready
> 
> During reset recovery iavf_dev_start() might fail (typically -EIO
> from VIRTCHNL_OP_CONFIG_VSI_QUEUES) because the PF VSI is not yet
> active, leaving the VF down and requiring manual intervention
> to recover.
> 
> Added a start_pending flag: when device start fails during recovery,
> defer it instead of erroring out and resume it from newly added
> iavf_resume_pending_start() on the next link-up event, so the
> VF comes back automatically.

It sounds like a fix, can you add a Fixes tag?

> 
> Signed-off-by: Anurag Mandal <[email protected]>
> ---
>  drivers/net/intel/iavf/iavf.h        |  2 +
>  drivers/net/intel/iavf/iavf_ethdev.c | 56 ++++++++++++++++++++++++++--
>  drivers/net/intel/iavf/iavf_vchnl.c  | 16 +++++++-
>  3 files changed, 69 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/intel/iavf/iavf.h b/drivers/net/intel/iavf/iavf.h
> index 293adaf6c9..e76c3bb410 100644
> --- a/drivers/net/intel/iavf/iavf.h
> +++ b/drivers/net/intel/iavf/iavf.h
> @@ -293,6 +293,7 @@ struct iavf_info {
>  	bool in_reset_recovery;
>  	bool reset_pending;
>  	bool pf_reset_in_progress;
> +	bool start_pending;
> 
>  	uint32_t ptp_caps;
>  	rte_spinlock_t phc_time_aq_lock;
> @@ -533,4 +534,5 @@ void iavf_handle_hw_reset(struct rte_eth_dev *dev,
> bool vf_initiated_reset);
>  void iavf_set_no_poll(struct iavf_adapter *adapter, bool link_change);
>  bool is_iavf_supported(struct rte_eth_dev *dev);
>  void iavf_hash_uninit(struct iavf_adapter *ad);
> +void iavf_resume_pending_start(struct rte_eth_dev *dev);
>  #endif /* _IAVF_ETHDEV_H_ */
> diff --git a/drivers/net/intel/iavf/iavf_ethdev.c
> b/drivers/net/intel/iavf/iavf_ethdev.c
> index e475b64971..87b826c873 100644
> --- a/drivers/net/intel/iavf/iavf_ethdev.c
> +++ b/drivers/net/intel/iavf/iavf_ethdev.c
> @@ -1093,6 +1093,9 @@ iavf_dev_start(struct rte_eth_dev *dev)
> 
>  	iavf_phc_sync_alarm_start(dev);
> 
> +	/* An explicit start supersedes any pending deferred start */
> +	vf->start_pending = false;
> +
>  	return 0;
> 
>  error:
> @@ -1131,6 +1134,9 @@ iavf_dev_stop(struct rte_eth_dev *dev)
>  	adapter->stopped = 1;
>  	dev->data->dev_started = 0;
> 
> +	/* An explicit stop cancels any pending deferred start */
> +	vf->start_pending = false;
> +
>  	return 0;
>  }
> 
> @@ -3425,6 +3431,7 @@ iavf_handle_hw_reset(struct rte_eth_dev *dev,
> bool vf_initiated_reset)
> 
>  	vf->in_reset_recovery = true;
>  	vf->pf_reset_in_progress = !vf_initiated_reset;
> +	vf->start_pending = false;
>  	iavf_set_no_poll(adapter, false);
> 
>  	/* Call the pre reset callback */
> @@ -3445,10 +3452,17 @@ iavf_handle_hw_reset(struct rte_eth_dev *dev,
> bool vf_initiated_reset)
>  	if (!vf_initiated_reset || restart_device) {
>  		/* start the device */
>  		ret = iavf_dev_start(dev);
> -		if (ret)
> -			goto error;
> -
> -		dev->data->dev_started = 1;
> +		if (ret == 0) {
> +			dev->data->dev_started = 1;
> +		} else {
> +			PMD_DRV_LOG(WARNING,
> +				    "dev_start failed during reset recovery
> (rc=%d);"
> +				    "deferring to next link-up event",
> +				    ret);
> +			vf->start_pending = true;
> +			dev->data->dev_started = 0;
> +			ret = 0;
> +		}
>  	}
> 
>  	/* Restore settings after the reset */
> @@ -3662,6 +3676,40 @@ bool is_iavf_supported(struct rte_eth_dev *dev)
>  	return !strcmp(dev->device->driver->name,
> rte_iavf_pmd.driver.name);
>  }
> 
> +void
> +iavf_resume_pending_start(struct rte_eth_dev *dev)
> +{
> +	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(dev->data-
> >dev_private);
> +	int ret;
> +
> +	if (!vf->start_pending)
> +		return;
> +	/*
> +	 * If the application has already (re)started the port itself, the
> +	 * deferred start is stale, the application's action is honoured
> +	 * and resume pending is dropped to avoid starting an
> +	 * already-running port a second time.
> +	 */
> +	if (dev->data->dev_started) {
> +		vf->start_pending = false;
> +		return;
> +	}
> +
> +	if (!vf->link_up)
> +		return;
> +
> +	PMD_DRV_LOG(DEBUG, "PF link back up; resuming deferred
> dev_start");
> +	ret = iavf_dev_start(dev);

This call to iavf_dev_start could result in another nested call to
iavf_resume_pending_start which would be problematic.
iavf_dev_start polls the ARQ eg.

iavf_dev_start -> iavf_configure_queues -> iavf_execute_vf_cmd_safe ->
iavf_wait_for_msg -> iavf_read_msg_from_pf -> iavf_clean_arq_element

If another LSC happens during this time, you could enter
iavf_handle_link_change_event again and end up calling
iavf_resume_pending_start again. It can maybe be prevented by clearing the
pending flag at the beginning of iavf_resume_pending_start and setting it
again if the start fails.

> +	if (ret == 0) {
> +		dev->data->dev_started = 1;
> +		vf->start_pending = false;
> +	} else {
> +		PMD_DRV_LOG(ERR,
> +			    "deferred dev_start failed (ret=%d); will retry on
> next link-up",
> +			    ret);
> +	}
> +}
> +
>  RTE_PMD_REGISTER_PCI(net_iavf, rte_iavf_pmd);
>  RTE_PMD_REGISTER_PCI_TABLE(net_iavf, pci_id_iavf_map);
>  RTE_PMD_REGISTER_KMOD_DEP(net_iavf, "* igb_uio | vfio-pci");
> diff --git a/drivers/net/intel/iavf/iavf_vchnl.c
> b/drivers/net/intel/iavf/iavf_vchnl.c
> index 56918ebcc1..8e102b02aa 100644
> --- a/drivers/net/intel/iavf/iavf_vchnl.c
> +++ b/drivers/net/intel/iavf/iavf_vchnl.c
> @@ -260,7 +260,7 @@ iavf_handle_link_change_event(struct rte_eth_dev
> *dev,
>  	 * (link is down or a VF reset is in progress); the watchdog drives
>  	 * auto-reset recovery, so it must remain armed in those cases.
>  	 */
> -	if (vf->link_up && !vf->vf_reset)
> +	if (vf->link_up && !vf->vf_reset && !vf->in_reset_recovery)

Is this change relevant to the rest of this patch?

>  		iavf_dev_watchdog_disable(adapter);
>  	else
>  		iavf_dev_watchdog_enable(adapter);
> @@ -271,6 +271,20 @@ iavf_handle_link_change_event(struct rte_eth_dev
> *dev,
>  			    adapter->no_poll ? "on" : "off");
>  	}
> 
> +	/*
> +	 * Resume a deferred dev_start.
> +	 * iavf_handle_hw_reset() sets vf->start_pending when
> +	 * reset recovery completed dev_init() but iavf_dev_start()
> +	 * itself failed (typically -EIO from
> VIRTCHNL_OP_CONFIG_VSI_QUEUES
> +	 * when the PF VSI was inactive).
> +	 * A link-up event implies the PF VSI is active again, so retry now.
> +	 * Run before the LSC event post so the port is ready to accept Tx
> +	 * by the time the app's link-up callback fires; no_poll has already
> +	 * been cleared above so bursts go through as soon as
> +	 * dev_start sets dev_started=1.
> +	 */

I think this comment is verbose, consider shortening it.

> +	iavf_resume_pending_start(dev);
> +
>  	iavf_dev_event_post(dev, RTE_ETH_EVENT_INTR_LSC, NULL, 0);
> 
>  	PMD_DRV_LOG(INFO, "Link status update:%s",
> --
> 2.34.1