lkml.org 
[lkml]   [2022]   [Jul]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH net-next] i40e: i40e_reset_vf should return false if reset vf timeout
Date
> -----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

\
 
 \ /
  Last update: 2022-07-12 02:04    [W:0.071 / U:0.240 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site