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

["Jεan Sacren" <sakiwit@gmail.com>]

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)