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

[Simon Horman <horms@kernel.org>]

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>