diff mbox series

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

Message ID 492df79e1ae204ec455973e22002ca2c62c41d1e.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, 28 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.

With this change, we could also use constant 0 for return.

Signed-off-by: Jean Sacren <sakiwit@gmail.com>
---
 drivers/net/ethernet/qlogic/qed/qed_ptp.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

Simon Horman Oct. 20, 2021, 8:48 a.m. UTC | #1 
On Tue, Oct 19, 2021 at 12:26:41AM -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.
> 
> With this change, we could also use constant 0 for return.
> 
> Signed-off-by: Jean Sacren <sakiwit@gmail.com>
> ---
>  drivers/net/ethernet/qlogic/qed/qed_ptp.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_ptp.c b/drivers/net/ethernet/qlogic/qed/qed_ptp.c
> index 2c62d732e5c2..c927ff409109 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_ptp.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_ptp.c
> @@ -52,9 +52,9 @@ static int qed_ptp_res_lock(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt)
>  	qed_mcp_resc_lock_default_init(&params, NULL, resource, true);
>  
>  	rc = qed_mcp_resc_lock(p_hwfn, p_ptt, &params);
> -	if (rc && rc != -EINVAL) {
> -		return rc;
> -	} else if (rc == -EINVAL) {
> +	if (rc) {
> +		if (rc != -EINVAL)
> +			return rc;
>  		/* MFW doesn't support resource locking, first PF on the port
>  		 * has lock ownership.
>  		 */
> @@ -63,12 +63,14 @@ static int qed_ptp_res_lock(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt)
>  
>  		DP_INFO(p_hwfn, "PF doesn't have lock ownership\n");
>  		return -EBUSY;
> -	} else if (!rc && !params.b_granted) {
> +	}
> +
> +	if (!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_INFO(p_hwfn, "Failed to acquire ptp resource lock\n");
>  		return -EBUSY;
>  	}
>  
> -	return rc;
> +	return 0;
>  }
>  
>  static int qed_ptp_res_unlock(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt)
>
Jεan Sacren Oct. 21, 2021, 7:35 a.m. UTC | #2 
From: Simon Horman <horms@kernel.org>
Date: Wed, 20 Oct 2021 10:48:35 +0200
>
> On Tue, Oct 19, 2021 at 12:26:41AM -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.
> > 
> > With this change, we could also use constant 0 for return.
> > 
> > Signed-off-by: Jean Sacren <sakiwit@gmail.com>
> > ---
> >  drivers/net/ethernet/qlogic/qed/qed_ptp.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/qlogic/qed/qed_ptp.c b/drivers/net/ethernet/qlogic/qed/qed_ptp.c
> > index 2c62d732e5c2..c927ff409109 100644
> > --- a/drivers/net/ethernet/qlogic/qed/qed_ptp.c
> > +++ b/drivers/net/ethernet/qlogic/qed/qed_ptp.c
> > @@ -52,9 +52,9 @@ static int qed_ptp_res_lock(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt)
> >  	qed_mcp_resc_lock_default_init(&params, NULL, resource, true);
> >  
> >  	rc = qed_mcp_resc_lock(p_hwfn, p_ptt, &params);
> > -	if (rc && rc != -EINVAL) {
> > -		return rc;
> > -	} else if (rc == -EINVAL) {
> > +	if (rc) {
> > +		if (rc != -EINVAL)
> > +			return rc;
> >  		/* MFW doesn't support resource locking, first PF on the port
> >  		 * has lock ownership.
> >  		 */
> > @@ -63,12 +63,14 @@ static int qed_ptp_res_lock(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt)
> >  
> >  		DP_INFO(p_hwfn, "PF doesn't have lock ownership\n");
> >  		return -EBUSY;
> > -	} else if (!rc && !params.b_granted) {
> > +	}
> > +
> > +	if (!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 did verify at the point where we check (!params.b_granted), !rc is
always true.  Earlier when we check rc alone, it has to be 0 to let it
reach the point where we check (!params.b_granted).  If it is not 0, it
will hit one of the returns in the branch.

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 (!rc && !params.b_granted)
since it is always true.

// diff --git a/drivers/net/ethernet/qlogic/qed/qed_ptp.c b/drivers/net/ethernet/qlogic/qed/qed_ptp.c
// index 2c62d732e5c2..4e1b741ebb46 100644
// --- a/drivers/net/ethernet/qlogic/qed/qed_ptp.c
// +++ b/drivers/net/ethernet/qlogic/qed/qed_ptp.c
// @@ -52,23 +52,27 @@ static int qed_ptp_res_lock(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt)
//  	qed_mcp_resc_lock_default_init(&params, NULL, resource, true);
//  
//  	rc = qed_mcp_resc_lock(p_hwfn, p_ptt, &params);
// -	if (rc && rc != -EINVAL) {
// +	if (rc) {
// +		if (rc == -EINVAL) {
// +			/* MFW doesn't support resource locking, first PF on the port
// +			 * has lock ownership.
// +			 */
// +			if (p_hwfn->abs_pf_id < p_hwfn->cdev->num_ports_in_engine)
// +				return 0;
// +
// +			DP_INFO(p_hwfn, "PF doesn't have lock ownership\n");
// +			return -EBUSY;
// +		}
// +
//  		return rc;
// -	} else if (rc == -EINVAL) {
// -		/* MFW doesn't support resource locking, first PF on the port
// -		 * has lock ownership.
// -		 */
// -		if (p_hwfn->abs_pf_id < p_hwfn->cdev->num_ports_in_engine)
// -			return 0;
// +	}
//  
// -		DP_INFO(p_hwfn, "PF doesn't have lock ownership\n");
// -		return -EBUSY;
// -	} else if (!rc && !params.b_granted) {
// +	if (!params.b_granted) {
//  		DP_INFO(p_hwfn, "Failed to acquire ptp resource lock\n");
//  		return -EBUSY;
//  	}
//  
// -	return rc;
// +	return 0;
//  }
//  
//  static int qed_ptp_res_unlock(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt)
Simon Horman Oct. 21, 2021, 10:46 a.m. UTC | #3 
On Thu, Oct 21, 2021 at 01:35:48AM -0600, Jεan Sacren wrote:
> From: Simon Horman <horms@kernel.org>
> Date: Wed, 20 Oct 2021 10:48:35 +0200
> >
> > On Tue, Oct 19, 2021 at 12:26:41AM -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.
> > > 
> > > With this change, we could also use constant 0 for return.
> > > 
> > > Signed-off-by: Jean Sacren <sakiwit@gmail.com>
> > > ---
> > >  drivers/net/ethernet/qlogic/qed/qed_ptp.c | 12 +++++++-----
> > >  1 file changed, 7 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/qlogic/qed/qed_ptp.c b/drivers/net/ethernet/qlogic/qed/qed_ptp.c
> > > index 2c62d732e5c2..c927ff409109 100644
> > > --- a/drivers/net/ethernet/qlogic/qed/qed_ptp.c
> > > +++ b/drivers/net/ethernet/qlogic/qed/qed_ptp.c
> > > @@ -52,9 +52,9 @@ static int qed_ptp_res_lock(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt)
> > >  	qed_mcp_resc_lock_default_init(&params, NULL, resource, true);
> > >  
> > >  	rc = qed_mcp_resc_lock(p_hwfn, p_ptt, &params);
> > > -	if (rc && rc != -EINVAL) {
> > > -		return rc;
> > > -	} else if (rc == -EINVAL) {
> > > +	if (rc) {
> > > +		if (rc != -EINVAL)
> > > +			return rc;
> > >  		/* MFW doesn't support resource locking, first PF on the port
> > >  		 * has lock ownership.
> > >  		 */
> > > @@ -63,12 +63,14 @@ static int qed_ptp_res_lock(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt)
> > >  
> > >  		DP_INFO(p_hwfn, "PF doesn't have lock ownership\n");
> > >  		return -EBUSY;
> > > -	} else if (!rc && !params.b_granted) {
> > > +	}
> > > +
> > > +	if (!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 did verify at the point where we check (!params.b_granted), !rc is
> always true.  Earlier when we check rc alone, it has to be 0 to let it
> reach the point where we check (!params.b_granted).  If it is not 0, it
> will hit one of the returns in the branch.
> 
> 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 (!rc && !params.b_granted)
> since it is always true.

Thanks I see that now, and I agree that your patch doesn't change the logic
of the code (as far as I can tell).

Reviewed-by: Simon Horman <horms@kernel.org>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/qlogic/qed/qed_ptp.c b/drivers/net/ethernet/qlogic/qed/qed_ptp.c
index 2c62d732e5c2..c927ff409109 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_ptp.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_ptp.c
@@ -52,9 +52,9 @@  static int qed_ptp_res_lock(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt)
 	qed_mcp_resc_lock_default_init(&params, NULL, resource, true);
 
 	rc = qed_mcp_resc_lock(p_hwfn, p_ptt, &params);
-	if (rc && rc != -EINVAL) {
-		return rc;
-	} else if (rc == -EINVAL) {
+	if (rc) {
+		if (rc != -EINVAL)
+			return rc;
 		/* MFW doesn't support resource locking, first PF on the port
 		 * has lock ownership.
 		 */
@@ -63,12 +63,14 @@  static int qed_ptp_res_lock(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt)
 
 		DP_INFO(p_hwfn, "PF doesn't have lock ownership\n");
 		return -EBUSY;
-	} else if (!rc && !params.b_granted) {
+	}
+
+	if (!params.b_granted) {
 		DP_INFO(p_hwfn, "Failed to acquire ptp resource lock\n");
 		return -EBUSY;
 	}
 
-	return rc;
+	return 0;
 }
 
 static int qed_ptp_res_unlock(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt)