> -----Original Message-----
> From: Zhengchao Shao <shaozhengchao@xxxxxxxxxx>
> Sent: Tuesday, July 5, 2022 3:13 AM
> To: intel-wired-lan@xxxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; bpf@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; Brandeburg,
> Jesse <jesse.brandeburg@xxxxxxxxx>; Nguyen, Anthony L
> <anthony.l.nguyen@xxxxxxxxx>; ast@xxxxxxxxxx; daniel@xxxxxxxxxxxxx;
> hawk@xxxxxxxxxx; john.fastabend@xxxxxxxxx
> Cc: weiyongjun1@xxxxxxxxxx; yuehaibing@xxxxxxxxxx;
> shaozhengchao@xxxxxxxxxx
> 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@xxxxxxxxxx>
> ---
> 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
