| Message ID | 20220705011304.230622-1-shaozhengchao@huawei.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] i40e: i40e_reset_vf should return false if reset vf timeout | expand |
> -----Original Message----- > From: Zhengchao Shao <shaozhengchao@huawei.com> > Sent: Tuesday, July 5, 2022 3:13 AM > To: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux- > kernel@vger.kernel.org; bpf@vger.kernel.org; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; Brandeburg, > Jesse <jesse.brandeburg@intel.com>; Nguyen, Anthony L > <anthony.l.nguyen@intel.com>; ast@kernel.org; daniel@iogearbox.net; > hawk@kernel.org; john.fastabend@gmail.com > Cc: weiyongjun1@huawei.com; yuehaibing@huawei.com; > shaozhengchao@huawei.com > Subject: [PATCH net-next] i40e: i40e_reset_vf should return false if reset vf > timeout > > when trigger vf reset, but vf status is not ready, i40e_reset_vf should not do > other cleanup action. The current logic is always return true. But it can't cover Are you sure the queues are stopped/disabled when the VF timeouts? This could lead to DMA corruption. It is hard for me to elaborate on the code path because you haven't provided a test case. Could you please provide reproduction steps along with the description of the issue? > timeout scenary, and the looping in function i40e_vc_reset_vf is useless. > Waiting for 120ms will cover most normal scenary. And the caller function > should try again when timeout or accept that resetting vf failed. > > Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> > --- > drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c > b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c > index d01fb592778c..42262009a00c 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c > @@ -1564,11 +1564,17 @@ bool i40e_reset_vf(struct i40e_vf *vf, bool flr) > if (flr) > usleep_range(10000, 20000); > > - if (!rsd) > - dev_err(&pf->pdev->dev, "VF reset check timeout on VF %d\n", > - vf->vf_id); > usleep_range(10000, 20000); > > + if (!rsd) { > + reg = rd32(hw, I40E_VPGEN_VFRSTAT(vf->vf_id)); > + if (!(reg & I40E_VPGEN_VFRSTAT_VFRD_MASK)) { > + dev_err(&pf->pdev->dev, "VF reset check timeout on VF > %d\n", > + vf->vf_id); > + return false; I think this could introduce the bug since you are never calling: clear_bit(__I40E_VF_DISABLE, pf->state); So the next time you enter this function the bit is still set and you'll return. This solution could work if you move the cleanups to the i40e_vc_reset_vf when the timeout happens. Still I'd like to see a test case and what is not working. > + } > + } > + > /* On initial reset, we don't have any queues to disable */ > if (vf->lan_vsi_idx != 0) > i40e_vsi_stop_rings(pf->vsi[vf->lan_vsi_idx]); > -- > 2.17.1
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c index d01fb592778c..42262009a00c 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c @@ -1564,11 +1564,17 @@ bool i40e_reset_vf(struct i40e_vf *vf, bool flr) if (flr) usleep_range(10000, 20000); - if (!rsd) - dev_err(&pf->pdev->dev, "VF reset check timeout on VF %d\n", - vf->vf_id); usleep_range(10000, 20000); + if (!rsd) { + reg = rd32(hw, I40E_VPGEN_VFRSTAT(vf->vf_id)); + if (!(reg & I40E_VPGEN_VFRSTAT_VFRD_MASK)) { + dev_err(&pf->pdev->dev, "VF reset check timeout on VF %d\n", + vf->vf_id); + return false; + } + } + /* On initial reset, we don't have any queues to disable */ if (vf->lan_vsi_idx != 0) i40e_vsi_stop_rings(pf->vsi[vf->lan_vsi_idx]);
when trigger vf reset, but vf status is not ready, i40e_reset_vf should not do other cleanup action. The current logic is always return true. But it can't cover timeout scenary, and the looping in function i40e_vc_reset_vf is useless. Waiting for 120ms will cover most normal scenary. And the caller function should try again when timeout or accept that resetting vf failed. Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> --- drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)