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@kernel.org>
Date: Wed, 20 Oct 2021 10:48:35 +0200

quoted

On Tue, Oct 19, 2021 at 12:26:41AM -0600, Jεan Sacren wrote:

quoted

From: Jean Sacren <redacted>

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 <redacted>
---
 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>