| Message ID | 20210909192709.26269-1-brett.creeley@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net] ice: Fix not stopping Tx queues for VFs | expand |
On Thu, 2021-09-09 at 12:27 -0700, Brett Creeley wrote: > When a VF is removed and/or reset its Tx queues need to be > stopped from the PF. This is done by calling the ice_dis_vf_qs() > function, which calls ice_vsi_stop_lan_tx_rings(). Currently > ice_dis_vf_qs() is protected by the VF state bit ICE_VF_STATE_QS_ENA. > Unfortunately, this is causing the Tx queues to not be disabled in > some > cases and when the VF tries to re-enable/reconfigure its Tx queues > over > virtchnl the op is failing. This is because a VF can be reset and/or > removed before the ICE_VF_STATE_QS_ENA bit is set, but the Tx queues > were already configured via ice_vsi_cfg_single_txq() in the > VIRTCHNL_OP_CONFIG_VSI_QUEUES op. However, the ICE_VF_STATE_QS_ENA > bit > is set on a successful VIRTCHNL_OP_ENABLE_QUEUES, which will always > happen after the VIRTCHNL_OP_CONFIG_VSI_QUEUES op. > > This was causing the following error message when loading the ice > driver, creating VFs, and modifying VF trust in an endless loop: > > [35274.192484] ice 0000:88:00.0: Failed to set LAN Tx queue context, > error: ICE_ERR_PARAM > [35274.193074] ice 0000:88:00.0: VF 0 failed opcode 6, retval: -5 > [35274.193640] iavf 0000:88:01.0: PF returned error -5 > (IAVF_ERR_PARAM) to our request 6 > > Fix this by always calling ice_dis_vf_qs() and silencing the error > message in ice_vsi_stop_tx_ring() since the calling code ignores the > return anyway. Also, all other places that call > ice_vsi_stop_tx_ring() > catch the error, so this doesn't affect those flows since there was > no > change to the values the function returns. > > Other solutions were considered (i.e. tracking which VF queues had > been > "started/configured" in VIRTCHNL_OP_CONFIG_VSI_QUEUES, but it seemed > more complicated than it was worth. This solution also brings in the > chance for other unexpected conditions due to invalid state bit > checks. > So, the proposed solution seemed like the best option since there is > no > harm in failing to stop Tx queues that were never started. > > This issue can be seen using the following commands: > > for i in {0..50}; do > rmmod ice > modprobe ice > > sleep 1 > > echo 1 > /sys/class/net/ens785f0/device/sriov_numvfs > echo 1 > /sys/class/net/ens785f1/device/sriov_numvfs > > ip link set ens785f1 vf 0 trust on > ip link set ens785f0 vf 0 trust on > > sleep 2 > > echo 0 > /sys/class/net/ens785f0/device/sriov_numvfs > echo 0 > /sys/class/net/ens785f1/device/sriov_numvfs > sleep 1 > echo 1 > /sys/class/net/ens785f0/device/sriov_numvfs > echo 1 > /sys/class/net/ens785f1/device/sriov_numvfs > > ip link set ens785f1 vf 0 trust on > ip link set ens785f0 vf 0 trust on > done > > Fixes: 77ca27c ("ice: add support for > virtchnl_queue_select.[tx|rx]_queues bitmap") The Fixes tag needs a little work: Fixes tag: Fixes: 77ca27c ("ice: add support for virtchnl_queue_select.[tx|rx]_queues bitmap") Has these problem(s): - SHA1 should be at least 12 digits long Can be fixed by setting core.abbrev to 12 (or more) or (for git v2.11 or later) just making sure it is not set (or set to "auto"). > Signed-off-by: Brett Creeley <brett.creeley@intel.com>
diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c index c36057efc7ae..f74610442bda 100644 --- a/drivers/net/ethernet/intel/ice/ice_base.c +++ b/drivers/net/ethernet/intel/ice/ice_base.c @@ -909,7 +909,7 @@ ice_vsi_stop_tx_ring(struct ice_vsi *vsi, enum ice_disq_rst_src rst_src, } else if (status == ICE_ERR_DOES_NOT_EXIST) { dev_dbg(ice_pf_to_dev(vsi->back), "LAN Tx queues do not exist, nothing to disable\n"); } else if (status) { - dev_err(ice_pf_to_dev(vsi->back), "Failed to disable LAN Tx queues, error: %s\n", + dev_dbg(ice_pf_to_dev(vsi->back), "Failed to disable LAN Tx queues, error: %s\n", ice_stat_str(status)); return -ENODEV; } diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c index 28b6013de425..e74ca81409b1 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c @@ -634,8 +634,7 @@ void ice_free_vfs(struct ice_pf *pf) /* Avoid wait time by stopping all VFs at the same time */ ice_for_each_vf(pf, i) - if (test_bit(ICE_VF_STATE_QS_ENA, pf->vf[i].vf_states)) - ice_dis_vf_qs(&pf->vf[i]); + ice_dis_vf_qs(&pf->vf[i]); tmp = pf->num_alloc_vfs; pf->num_qps_per_vf = 0; @@ -1646,8 +1645,7 @@ bool ice_reset_vf(struct ice_vf *vf, bool is_vflr) vsi = ice_get_vf_vsi(vf); - if (test_bit(ICE_VF_STATE_QS_ENA, vf->vf_states)) - ice_dis_vf_qs(vf); + ice_dis_vf_qs(vf); /* Call Disable LAN Tx queue AQ whether or not queues are * enabled. This is needed for successful completion of VFR.
When a VF is removed and/or reset its Tx queues need to be stopped from the PF. This is done by calling the ice_dis_vf_qs() function, which calls ice_vsi_stop_lan_tx_rings(). Currently ice_dis_vf_qs() is protected by the VF state bit ICE_VF_STATE_QS_ENA. Unfortunately, this is causing the Tx queues to not be disabled in some cases and when the VF tries to re-enable/reconfigure its Tx queues over virtchnl the op is failing. This is because a VF can be reset and/or removed before the ICE_VF_STATE_QS_ENA bit is set, but the Tx queues were already configured via ice_vsi_cfg_single_txq() in the VIRTCHNL_OP_CONFIG_VSI_QUEUES op. However, the ICE_VF_STATE_QS_ENA bit is set on a successful VIRTCHNL_OP_ENABLE_QUEUES, which will always happen after the VIRTCHNL_OP_CONFIG_VSI_QUEUES op. This was causing the following error message when loading the ice driver, creating VFs, and modifying VF trust in an endless loop: [35274.192484] ice 0000:88:00.0: Failed to set LAN Tx queue context, error: ICE_ERR_PARAM [35274.193074] ice 0000:88:00.0: VF 0 failed opcode 6, retval: -5 [35274.193640] iavf 0000:88:01.0: PF returned error -5 (IAVF_ERR_PARAM) to our request 6 Fix this by always calling ice_dis_vf_qs() and silencing the error message in ice_vsi_stop_tx_ring() since the calling code ignores the return anyway. Also, all other places that call ice_vsi_stop_tx_ring() catch the error, so this doesn't affect those flows since there was no change to the values the function returns. Other solutions were considered (i.e. tracking which VF queues had been "started/configured" in VIRTCHNL_OP_CONFIG_VSI_QUEUES, but it seemed more complicated than it was worth. This solution also brings in the chance for other unexpected conditions due to invalid state bit checks. So, the proposed solution seemed like the best option since there is no harm in failing to stop Tx queues that were never started. This issue can be seen using the following commands: for i in {0..50}; do rmmod ice modprobe ice sleep 1 echo 1 > /sys/class/net/ens785f0/device/sriov_numvfs echo 1 > /sys/class/net/ens785f1/device/sriov_numvfs ip link set ens785f1 vf 0 trust on ip link set ens785f0 vf 0 trust on sleep 2 echo 0 > /sys/class/net/ens785f0/device/sriov_numvfs echo 0 > /sys/class/net/ens785f1/device/sriov_numvfs sleep 1 echo 1 > /sys/class/net/ens785f0/device/sriov_numvfs echo 1 > /sys/class/net/ens785f1/device/sriov_numvfs ip link set ens785f1 vf 0 trust on ip link set ens785f0 vf 0 trust on done Fixes: 77ca27c ("ice: add support for virtchnl_queue_select.[tx|rx]_queues bitmap") Signed-off-by: Brett Creeley <brett.creeley@intel.com> --- drivers/net/ethernet/intel/ice/ice_base.c | 2 +- drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-)