Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Laba, SlawomirX" <slawomirx.laba@intel.com>
To:     Zhengchao Shao <shaozhengchao@huawei.com>,
        "intel-wired-lan@lists.osuosl.org" <intel-wired-lan@lists.osuosl.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "bpf@vger.kernel.org" <bpf@vger.kernel.org>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "edumazet@google.com" <edumazet@google.com>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "Brandeburg, Jesse" <jesse.brandeburg@intel.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@intel.com>,
        "ast@kernel.org" <ast@kernel.org>,
        "daniel@iogearbox.net" <daniel@iogearbox.net>,
        "hawk@kernel.org" <hawk@kernel.org>,
        "john.fastabend@gmail.com" <john.fastabend@gmail.com>
CC:     "weiyongjun1@huawei.com" <weiyongjun1@huawei.com>,
        "yuehaibing@huawei.com" <yuehaibing@huawei.com>
Subject: RE: [PATCH net-next] i40e: i40e_reset_vf should return false if reset
 vf timeout
Thread-Topic: [PATCH net-next] i40e: i40e_reset_vf should return false if
 reset vf timeout
Thread-Index: AQHYkAvOBFJBstQ/rU2/2iuMxzBiiq154qHw
Date:   Tue, 12 Jul 2022 00:03:27 +0000
Message-ID: <DM6PR11MB311352D9E711CD02F740600087869@DM6PR11MB3113.namprd11.prod.outlook.com>
References: <20220705011304.230622-1-shaozhengchao@huawei.com>
In-Reply-To: <20220705011304.230622-1-shaozhengchao@huawei.com>
Accept-Language: en-US
Content-Language: en-US
dlp-version: 11.6.500.17
dlp-product: dlpe-windows
dlp-reaction: no-action
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?/pRhIP9sW4WWtkx3xPMgMuEGnwqlfG/MJKtCmC6uucklFq6STa/1dcXtqBcw?=
 =?us-ascii?Q?K1txaZbSI28WgqXrxa5LiIN91RTZ2J6nL0F+YOIAlKVAxXtygB3/4has4Ag4?=
 =?us-ascii?Q?cPNTO9sSpQo+8sXz7tZm19umNWG8k7Ker3xavl2/OF3VQ2QJsvWw9Fk9Ouig?=
 =?us-ascii?Q?/YZBgz7FQPNXapjqVLCUSgviRd01kZHc/SEgfNkGn1n8N7Dnuk19oCLhSOBy?=
 =?us-ascii?Q?tbRoULRxIzh1kE9P6ClLW1w7hR28E+rQayeOQ4VrHVOpuWT3jEfUbVNZ5sSZ?=
 =?us-ascii?Q?clrgzAdNJDaU1e06rWOsxpcs7olVsZagEM7U36k41vP/zsuCQ/b5VQXjorPQ?=
 =?us-ascii?Q?Zi7bgniSZw8uwNureNpbHN2LTW5H9x6K/wPCO4Q97Zev6aG31zX25SEAttql?=
 =?us-ascii?Q?dytlr1vBd4cYfPqjQS9R9x/1NUig3gcbeYJzaEHrCfFDn7REm6zdKDTFVHfK?=
 =?us-ascii?Q?iD3ytKbWMHZU7C6AaKKF9e6mqlNMzOHAv6gGVSzPdQ4xofWi55Cozg2kpurQ?=
 =?us-ascii?Q?qu0ORwHK6mYP2lHEhRNiN8NxvAyJ8undkJQ5brnFisO4z2N8nGzwLaqzqEi1?=
 =?us-ascii?Q?Ik3zSq/c+ptt+tX+Q9GDr67FaEGZqMVo002xa4ZcTFJljNmO7EXQaMZ7IFj9?=
 =?us-ascii?Q?rBmdz4eEZhtz2x/JxxdNYyyO1Ec5mLORLW4N+rxD/ba0mg/bds59fh2TTO6s?=
 =?us-ascii?Q?q3HirDeG5kUJ3B0iVsEizu7o1bEX3H+lFrfr882k+LFWQyIO72Io901Skzrx?=
 =?us-ascii?Q?By93kSEyunAFd9E6IfTctzansA2nW0lW5CZKQWDgOmeNKjA+oAviot9GP8b4?=
 =?us-ascii?Q?nRZmR8C1K2qfmrCed+yIyb9IA8SHiZ6qXwHHs1NUALTuuDiINNpnA+0W62yl?=
 =?us-ascii?Q?kEn0QIsfPB19CrrtLWjUnFe87MzdGmTPr3BAbkkuwbnRwsNpN+lq70BhBPJM?=
 =?us-ascii?Q?hsgPm/XKbmmmeUAE1FMyeMW9KQjwBgAA97XFd/zuHdvrTqcsQ9stJmGotp/C?=
 =?us-ascii?Q?90vFqLE0kXbzBLGTgYh6g+lXuLheb7GnOVFqMfuXpBfeEYqCVXrhtVS77J1v?=
 =?us-ascii?Q?3U//xiEuieEDAIyYSXi5E1rGYOfU9mBMc6+NTlnidBWcMARUU+UhDbbaQvI9?=
 =?us-ascii?Q?8Qnd33WGfBXBKWR9IUHFacLWl8HbtgzI5Ec54Vns+vLxH0c9idSpCrIR4aHY?=
 =?us-ascii?Q?MxvOgTrqPmdOYtd4Cl68QRGZbvFfC/codDWxUi4t2Y2E1HMN2WxHd49yN7Ti?=
 =?us-ascii?Q?10IkOB6z9Ypu64ETOvxEzNqi4NneLaRUhd8CWzZA9nuY+M5EPKkFK7AAya6Q?=
 =?us-ascii?Q?9jtezwnvTGRs31Xxkt/DX5NUlsfzwt6chtC6TOnuhFLQhrs5By2W2RJJhWkl?=
 =?us-ascii?Q?weWY5vHPfLNm0aJqRkHMY+VEDXOIELQ0JwbhImt0X1UO4sJdletMnvgm1+Ub?=
 =?us-ascii?Q?6+BjjG8akBEumH0v2iy9F4Ma0ppEQt8md5a27pag7LdQLIdmGO6UAQZY7sZu?=
 =?us-ascii?Q?hOl7Uf4zG93Q8h5IAnX0dYKxgJiSA9AZn5bywNcsmLUnbvMnfd/cjdNx5cjA?=
 =?us-ascii?Q?qczRVnN2/F46UFJSMB1y4pk8+tBPg8V0tCXxHeM9?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3113.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d939f623-89b0-422e-1b10-08da6399f26e
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Jul 2022 00:03:27.4482
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: qp8/KuMJueg9Na/q+G7rRwbwY4OkbbiCGRMq6M9dy6rw1/H6N764j6C669pp7v9QuqSmNfKhdzRRLPA6Nceru5RUO42CZS6LNSaAWhyG02s=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB4821
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> -----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 rese=
t vf
> timeout
>=20
> when trigger vf reset, but vf status is not ready, i40e_reset_vf should n=
ot 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 provide=
d a test case.
Could you please provide reproduction steps along with the description of t=
he 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.
>=20
> 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(-)
>=20
> 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);
>=20
> -	if (!rsd)
> -		dev_err(&pf->pdev->dev, "VF reset check timeout on VF %d\n",
> -			vf->vf_id);
>  	usleep_range(10000, 20000);
>=20
> +	if (!rsd) {
> +		reg =3D 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 re=
turn.

This solution could work if you move the cleanups to the i40e_vc_reset_vf w=
hen 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 !=3D 0)
>  		i40e_vsi_stop_rings(pf->vsi[vf->lan_vsi_idx]);
> --
> 2.17.1