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

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 for Android: free password hash cracker in your pocket

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

Message-ID: <YXEYVC4gBC4JC7t9@mail.gmail.com>
Date:   Thu, 21 Oct 2021 01:35:48 -0600
From:   Jεan Sacren <sakiwit@...il.com>
To:     Simon Horman <horms@...nel.org>
Cc:     Ariel Elior <aelior@...vell.com>, GR-everest-linux-l2@...vell.com,
        davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/2] net: qed_ptp: fix redundant check of rc and
 against -EINVAL

From: Simon Horman <horms@...nel.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@...il.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@...il.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)