> 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
