diff mbox series

[net-next,2/2] net: qed_dev: fix redundant check of rc and against -EINVAL

Message ID b187bc8a2a12e20dd54bce71f7de0f8e7c45f249.1634621525.git.sakiwit@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Small fixes for redundant checks | expand

Checks

Context Check Description
netdev/cover_letter success Series has a cover letter
netdev/fixes_present success Fixes tag not required for -next series
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success No Fixes tag
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 50 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success No static functions without inline keyword in header files

Commit Message

Jεan Sacren Oct. 19, 2021, 6:26 a.m. UTC 
From: Jean Sacren <sakiwit@gmail.com>

We should first check rc alone and then check it against -EINVAL to
avoid repeating the same operation multiple times.

Signed-off-by: Jean Sacren <sakiwit@gmail.com>
---
 drivers/net/ethernet/qlogic/qed/qed_dev.c | 35 +++++++++++++----------
 1 file changed, 20 insertions(+), 15 deletions(-)

Comments

Simon Horman Oct. 20, 2021, 8:47 a.m. UTC | #1 
On Tue, Oct 19, 2021 at 12:26:42AM -0600, Jεan Sacren wrote:
> From: Jean Sacren <sakiwit@gmail.com>
> 
> We should first check rc alone and then check it against -EINVAL to
> avoid repeating the same operation multiple times.
> 
> Signed-off-by: Jean Sacren <sakiwit@gmail.com>
> ---
>  drivers/net/ethernet/qlogic/qed/qed_dev.c | 35 +++++++++++++----------
>  1 file changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c b/drivers/net/ethernet/qlogic/qed/qed_dev.c
> index 18f3bf7c4dfe..fe8bdb4523b5 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_dev.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c
> @@ -3987,30 +3987,35 @@ static int qed_hw_get_resc(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt)
>  				       QED_RESC_LOCK_RESC_ALLOC, false);
>  
>  	rc = qed_mcp_resc_lock(p_hwfn, p_ptt, &resc_lock_params);
> -	if (rc && rc != -EINVAL) {
> -		return rc;
> -	} else if (rc == -EINVAL) {
> +	if (rc) {
> +		if (rc != -EINVAL)
> +			return rc;
> +
>  		DP_INFO(p_hwfn,
>  			"Skip the max values setting of the soft resources since the resource lock is not supported by the MFW\n");
> -	} else if (!rc && !resc_lock_params.b_granted) {
> +	}
> +
> +	if (!resc_lock_params.b_granted) {

Can it be the case where the condition above is met and !rc is false?
If so your patch seems to have changed the logic of this function.

>  		DP_NOTICE(p_hwfn,
>  			  "Failed to acquire the resource lock for the resource allocation commands\n");
>  		return -EBUSY;
> -	} else {
> -		rc = qed_hw_set_soft_resc_size(p_hwfn, p_ptt);
> -		if (rc && rc != -EINVAL) {
> +	}
> +
> +	rc = qed_hw_set_soft_resc_size(p_hwfn, p_ptt);
> +	if (rc) {
> +		if (rc != -EINVAL) {
>  			DP_NOTICE(p_hwfn,
>  				  "Failed to set the max values of the soft resources\n");
>  			goto unlock_and_exit;
> -		} else if (rc == -EINVAL) {
> -			DP_INFO(p_hwfn,
> -				"Skip the max values setting of the soft resources since it is not supported by the MFW\n");
> -			rc = qed_mcp_resc_unlock(p_hwfn, p_ptt,
> -						 &resc_unlock_params);

nit: it looks like the two lines above would now fit on one.

> -			if (rc)
> -				DP_INFO(p_hwfn,
> -					"Failed to release the resource lock for the resource allocation commands\n");
>  		}
> +
> +		DP_INFO(p_hwfn,
> +			"Skip the max values setting of the soft resources since it is not supported by the MFW\n");
> +		rc = qed_mcp_resc_unlock(p_hwfn, p_ptt,
> +					 &resc_unlock_params);
> +		if (rc)
> +			DP_INFO(p_hwfn,
> +				"Failed to release the resource lock for the resource allocation commands\n");
>  	}
>  
>  	rc = qed_hw_set_resc_info(p_hwfn);
>
Jεan Sacren Oct. 21, 2021, 7:36 a.m. UTC | #2 
From: Simon Horman <horms@kernel.org>
Date: Wed, 20 Oct 2021 10:47:17 +0200
>
> On Tue, Oct 19, 2021 at 12:26:42AM -0600, Jεan Sacren wrote:
> > From: Jean Sacren <sakiwit@gmail.com>
> > 
> > We should first check rc alone and then check it against -EINVAL to
> > avoid repeating the same operation multiple times.
> > 
> > Signed-off-by: Jean Sacren <sakiwit@gmail.com>
> > ---
> >  drivers/net/ethernet/qlogic/qed/qed_dev.c | 35 +++++++++++++----------
> >  1 file changed, 20 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c b/drivers/net/ethernet/qlogic/qed/qed_dev.c
> > index 18f3bf7c4dfe..fe8bdb4523b5 100644
> > --- a/drivers/net/ethernet/qlogic/qed/qed_dev.c
> > +++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c
> > @@ -3987,30 +3987,35 @@ static int qed_hw_get_resc(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt)
> >  				       QED_RESC_LOCK_RESC_ALLOC, false);
> >  
> >  	rc = qed_mcp_resc_lock(p_hwfn, p_ptt, &resc_lock_params);
> > -	if (rc && rc != -EINVAL) {
> > -		return rc;
> > -	} else if (rc == -EINVAL) {
> > +	if (rc) {
> > +		if (rc != -EINVAL)
> > +			return rc;
> > +
> >  		DP_INFO(p_hwfn,
> >  			"Skip the max values setting of the soft resources since the resource lock is not supported by the MFW\n");
> > -	} else if (!rc && !resc_lock_params.b_granted) {
> > +	}
> > +
> > +	if (!resc_lock_params.b_granted) {
> 
> Can it be the case where the condition above is met and !rc is false?
> If so your patch seems to have changed the logic of this function.

Mr. Horman,

I'm so much appreciative to you for the review.  I'm so sorry this patch
is wrong.  I redid the patch.  Could you please help me review it?

I'll add the following text in the changelog to curb the confusion I
incur.  What do you think?

We should also remove the check of !rc in this expression since it is
always true:

        (!rc && !resc_lock_params.b_granted)

[...]
> > -			rc = qed_mcp_resc_unlock(p_hwfn, p_ptt,
> > -						 &resc_unlock_params);
> 
> nit: it looks like the two lines above would now fit on one.

Absolutely.  I'll put these two lines together as one.

I'd be very much grateful if you could help me review both patches.
I'll respin the whole series and resubmit as v2 upon your review.

Thank you!

// diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c b/drivers/net/ethernet/qlogic/qed/qed_dev.c
// index 18f3bf7c4dfe..44b0d4b42bc3 100644
// --- a/drivers/net/ethernet/qlogic/qed/qed_dev.c
// +++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c
// @@ -3987,29 +3987,33 @@ static int qed_hw_get_resc(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt)
//  				       QED_RESC_LOCK_RESC_ALLOC, false);
//  
//  	rc = qed_mcp_resc_lock(p_hwfn, p_ptt, &resc_lock_params);
// -	if (rc && rc != -EINVAL) {
// -		return rc;
// -	} else if (rc == -EINVAL) {
// -		DP_INFO(p_hwfn,
// -			"Skip the max values setting of the soft resources since the resource lock is not supported by the MFW\n");
// -	} else if (!rc && !resc_lock_params.b_granted) {
// -		DP_NOTICE(p_hwfn,
// -			  "Failed to acquire the resource lock for the resource allocation commands\n");
// -		return -EBUSY;
// +	if (rc) {
// +		if (rc == -EINVAL)
// +			DP_INFO(p_hwfn,
// +				"Skip the max values setting of the soft resources since the resource lock is not supported by the MFW\n");
// +		else
// +			return rc;
//  	} else {
// -		rc = qed_hw_set_soft_resc_size(p_hwfn, p_ptt);
// -		if (rc && rc != -EINVAL) {
// +		if (!resc_lock_params.b_granted) {
//  			DP_NOTICE(p_hwfn,
// -				  "Failed to set the max values of the soft resources\n");
// -			goto unlock_and_exit;
// -		} else if (rc == -EINVAL) {
// -			DP_INFO(p_hwfn,
// -				"Skip the max values setting of the soft resources since it is not supported by the MFW\n");
// -			rc = qed_mcp_resc_unlock(p_hwfn, p_ptt,
// -						 &resc_unlock_params);
// -			if (rc)
// +				  "Failed to acquire the resource lock for the resource allocation commands\n");
// +			return -EBUSY;
// +		}
// +
// +		rc = qed_hw_set_soft_resc_size(p_hwfn, p_ptt);
// +		if (rc) {
// +			if (rc == -EINVAL) {
//  				DP_INFO(p_hwfn,
// -					"Failed to release the resource lock for the resource allocation commands\n");
// +					"Skip the max values setting of the soft resources since it is not supported by the MFW\n");
// +				rc = qed_mcp_resc_unlock(p_hwfn, p_ptt, &resc_unlock_params);
// +				if (rc)
// +					DP_INFO(p_hwfn,
// +						"Failed to release the resource lock for the resource allocation commands\n");
// +			} else {
// +				DP_NOTICE(p_hwfn,
// +					  "Failed to set the max values of the soft resources\n");
// +				goto unlock_and_exit;
// +			}
//  		}
//  	}
//
diff mbox series

Patch

diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c b/drivers/net/ethernet/qlogic/qed/qed_dev.c
index 18f3bf7c4dfe..fe8bdb4523b5 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_dev.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c
@@ -3987,30 +3987,35 @@  static int qed_hw_get_resc(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt)
 				       QED_RESC_LOCK_RESC_ALLOC, false);
 
 	rc = qed_mcp_resc_lock(p_hwfn, p_ptt, &resc_lock_params);
-	if (rc && rc != -EINVAL) {
-		return rc;
-	} else if (rc == -EINVAL) {
+	if (rc) {
+		if (rc != -EINVAL)
+			return rc;
+
 		DP_INFO(p_hwfn,
 			"Skip the max values setting of the soft resources since the resource lock is not supported by the MFW\n");
-	} else if (!rc && !resc_lock_params.b_granted) {
+	}
+
+	if (!resc_lock_params.b_granted) {
 		DP_NOTICE(p_hwfn,
 			  "Failed to acquire the resource lock for the resource allocation commands\n");
 		return -EBUSY;
-	} else {
-		rc = qed_hw_set_soft_resc_size(p_hwfn, p_ptt);
-		if (rc && rc != -EINVAL) {
+	}
+
+	rc = qed_hw_set_soft_resc_size(p_hwfn, p_ptt);
+	if (rc) {
+		if (rc != -EINVAL) {
 			DP_NOTICE(p_hwfn,
 				  "Failed to set the max values of the soft resources\n");
 			goto unlock_and_exit;
-		} else if (rc == -EINVAL) {
-			DP_INFO(p_hwfn,
-				"Skip the max values setting of the soft resources since it is not supported by the MFW\n");
-			rc = qed_mcp_resc_unlock(p_hwfn, p_ptt,
-						 &resc_unlock_params);
-			if (rc)
-				DP_INFO(p_hwfn,
-					"Failed to release the resource lock for the resource allocation commands\n");
 		}
+
+		DP_INFO(p_hwfn,
+			"Skip the max values setting of the soft resources since it is not supported by the MFW\n");
+		rc = qed_mcp_resc_unlock(p_hwfn, p_ptt,
+					 &resc_unlock_params);
+		if (rc)
+			DP_INFO(p_hwfn,
+				"Failed to release the resource lock for the resource allocation commands\n");
 	}
 
 	rc = qed_hw_set_resc_info(p_hwfn);