netdev - Re: [PATCH net-next 1/2] i40e: Add helper for VF inited state check with timeout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cd7a39b2-c73e-6919-7ae5-5a2cea5a3ed9@redhat.com>
Date: Wed, 12 Jul 2023 15:18:29 +0200
From: Ivan Vecera <ivecera@...hat.com>
To: Leon Romanovsky <leon@...nel.org>,
 Tony Nguyen <anthony.l.nguyen@...el.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
 edumazet@...gle.com, netdev@...r.kernel.org, Ma Yuying <yuma@...hat.com>,
 Simon Horman <simon.horman@...igine.com>,
 Rafal Romanowski <rafal.romanowski@...el.com>
Subject: Re: [PATCH net-next 1/2] i40e: Add helper for VF inited state check
 with timeout

On 11. 07. 23 14:09, Leon Romanovsky wrote:
> On Mon, Jul 10, 2023 at 09:40:29AM -0700, Tony Nguyen wrote:
>> From: Ivan Vecera <ivecera@...hat.com>
>>
>> Move the check for VF inited state (with optional up-to 300ms
>> timeout to separate helper i40e_check_vf_init_timeout() that
>> will be used in the following commit.
>>
>> Tested-by: Ma Yuying <yuma@...hat.com>
>> Signed-off-by: Ivan Vecera <ivecera@...hat.com>
>> Reviewed-by: Simon Horman <simon.horman@...igine.com>
>> Tested-by: Rafal Romanowski <rafal.romanowski@...el.com>
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
>> ---
>>   .../ethernet/intel/i40e/i40e_virtchnl_pf.c    | 47 ++++++++++++-------
>>   1 file changed, 31 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
>> index be59ba3774e1..b84b6b675fa7 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
>> @@ -4304,6 +4304,36 @@ static int i40e_validate_vf(struct i40e_pf *pf, int vf_id)
>>   	return ret;
>>   }
>>   
>> +/**
>> + * i40e_check_vf_init_timeout
>> + * @vf: the virtual function
>> + *
>> + * Check that the VF's initialization was successfully done and if not
>> + * wait up to 300ms for its finish.
>> + *
>> + * Returns true when VF is initialized, false on timeout
>> + **/
>> +static bool i40e_check_vf_init_timeout(struct i40e_vf *vf)
>> +{
>> +	int i;
>> +
>> +	/* When the VF is resetting wait until it is done.
>> +	 * It can take up to 200 milliseconds, but wait for
>> +	 * up to 300 milliseconds to be safe.
>> +	 */
>> +	for (i = 0; i < 15; i++) {
>> +		if (test_bit(I40E_VF_STATE_INIT, &vf->vf_states))
>> +			return true;
>> +
>> +		msleep(20);
>> +	}
>> +
>> +	dev_err(&vf->pf->pdev->dev, "VF %d still in reset. Try again.\n",
>> +		vf->vf_id);
> 
> This error is not accurate in the edge case, when VF state changed to
> be INIT during msleep() while i was 14.
> 
> Thanks

Would you like to add an extra check after the cycle or just increase 
limit from 15 to 16 (there will be an extra msleep)...

Ivan
> 
>> +
>> +	return false;
>> +}
>> +
>>   /**
>>    * i40e_ndo_set_vf_mac
>>    * @netdev: network interface device structure
>> @@ -4322,7 +4352,6 @@ int i40e_ndo_set_vf_mac(struct net_device *netdev, int vf_id, u8 *mac)
>>   	int ret = 0;
>>   	struct hlist_node *h;
>>   	int bkt;
>> -	u8 i;
>>   
>>   	if (test_and_set_bit(__I40E_VIRTCHNL_OP_PENDING, pf->state)) {
>>   		dev_warn(&pf->pdev->dev, "Unable to configure VFs, other operation is pending.\n");
>> @@ -4335,21 +4364,7 @@ int i40e_ndo_set_vf_mac(struct net_device *netdev, int vf_id, u8 *mac)
>>   		goto error_param;
>>   
>>   	vf = &pf->vf[vf_id];
>> -
>> -	/* When the VF is resetting wait until it is done.
>> -	 * It can take up to 200 milliseconds,
>> -	 * but wait for up to 300 milliseconds to be safe.
>> -	 * Acquire the VSI pointer only after the VF has been
>> -	 * properly initialized.
>> -	 */
>> -	for (i = 0; i < 15; i++) {
>> -		if (test_bit(I40E_VF_STATE_INIT, &vf->vf_states))
>> -			break;
>> -		msleep(20);
>> -	}
>> -	if (!test_bit(I40E_VF_STATE_INIT, &vf->vf_states)) {
>> -		dev_err(&pf->pdev->dev, "VF %d still in reset. Try again.\n",
>> -			vf_id);
>> +	if (!i40e_check_vf_init_timeout(vf)) {
>>   		ret = -EAGAIN;
>>   		goto error_param;
>>   	}
>> -- 
>> 2.38.1
>>
>>
>