diff mbox series

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

Message ID 20230710164030.2821326-2-anthony.l.nguyen@intel.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Intel Wired LAN Driver Updates 2023-07-10 (i40e) | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1341 this patch: 1341
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 1364 this patch: 1364
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1364 this patch: 1364
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 65 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Tony Nguyen July 10, 2023, 4:40 p.m. UTC 
From: Ivan Vecera <ivecera@redhat.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@redhat.com>
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 .../ethernet/intel/i40e/i40e_virtchnl_pf.c    | 47 ++++++++++++-------
 1 file changed, 31 insertions(+), 16 deletions(-)

Comments

Leon Romanovsky July 11, 2023, 12:09 p.m. UTC | #1 
On Mon, Jul 10, 2023 at 09:40:29AM -0700, Tony Nguyen wrote:
> From: Ivan Vecera <ivecera@redhat.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@redhat.com>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.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

> +
> +	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
> 
>
Jakub Kicinski July 12, 2023, 12:37 a.m. UTC | #2 
On Tue, 11 Jul 2023 15:09:04 +0300 Leon Romanovsky wrote:
> > +	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.

Right, it's a change in behavior from existing code,
old code re-checked if INIT is set after the last sleep.
Ivan Vecera July 12, 2023, 1:18 p.m. UTC | #3 
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@redhat.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@redhat.com>
>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>> Reviewed-by: Simon Horman <simon.horman@corigine.com>
>> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.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
>>
>>
>
Ivan Vecera July 12, 2023, 1:28 p.m. UTC | #4 
On 12. 07. 23 2:37, Jakub Kicinski wrote:
> On Tue, 11 Jul 2023 15:09:04 +0300 Leon Romanovsky wrote:
>>> +	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.
> 
> Right, it's a change in behavior from existing code,
> old code re-checked if INIT is set after the last sleep.

I will simplify this and add an extra check.

Thanks for the review..

I.
Jakub Kicinski July 12, 2023, 5:31 p.m. UTC | #5 
On Wed, 12 Jul 2023 15:18:29 +0200 Ivan Vecera wrote:
> > 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)...

15 or 16 is of lesser importance, the bigger irritant is that the last
sleep is no pointless. I'd move the condition check into the middle of
the loop:

for (i = 0;; i++) {
	if (test())
		return GOOD;

	if (i >= 15)
		return BAD;

	take_a_nap();
}
diff mbox series

Patch

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);
+
+	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;
 	}