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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20211021104650.GB26665@kernel.org>
Date:   Thu, 21 Oct 2021 12:46:50 +0200
From:   Simon Horman <horms@...nel.org>
To:     Jεan Sacren <sakiwit@...il.com>
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

On Thu, Oct 21, 2021 at 01:35:48AM -0600, Jεan Sacren wrote:
> 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.

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@...nel.org>