RE: [PATCH net-next] i40e: i40e_reset_vf should return false if reset vf timeout

["Laba, SlawomirX" <slawomirx.laba@intel.com>]

> -----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