[PATCH net-next v2 0/2] Small fixes for redundant checks

From: Jean Sacren <redacted>

This series fixes some redundant checks of certain variables and
expressions.

Jean Sacren (2):
  net: qed_ptp: fix redundant check of rc and against -EINVAL
  net: qed_dev: fix redundant check of rc and against -EINVAL

 drivers/net/ethernet/qlogic/qed/qed_dev.c | 31 +++++++++++++----------
 drivers/net/ethernet/qlogic/qed/qed_ptp.c | 12 +++++----
 2 files changed, 24 insertions(+), 19 deletions(-)

From: Jean Sacren <redacted>

We should first check rc alone and then check it against -EINVAL to
avoid repeating the same operation multiple times.

We should also remove the check of !rc in this expression since it is
always true:

	(!rc && !resc_lock_params.b_granted)

Signed-off-by: Jean Sacren <redacted>
---
v2:
(1) Fix missing else branch. I'm very sorry.
(2) Add text for !rc removal in the changelog.
(3) Put two lines of qed_mcp_resc_unlock() call into one.
    Thank you, Mr. Horman!
 drivers/net/ethernet/qlogic/qed/qed_dev.c | 31 +++++++++++++----------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c b/drivers/net/ethernet/qlogic/qed/qed_dev.c
index 18f3bf7c4dfe..4ae9867b2535 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_dev.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c

@@ -3987,26 +3987,29 @@ static int qed_hw_get_resc(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt)
 				       QED_RESC_LOCK_RESC_ALLOC, false);
 
 	rc = qed_mcp_resc_lock(p_hwfn, p_ptt, &resc_lock_params);
-	if (rc && rc != -EINVAL) {
-		return rc;
-	} else if (rc == -EINVAL) {
+	if (rc) {
+		if (rc != -EINVAL)
+			return rc;
 		DP_INFO(p_hwfn,
 			"Skip the max values setting of the soft resources since the resource lock is not supported by the MFW\n");
-	} else if (!rc && !resc_lock_params.b_granted) {
-		DP_NOTICE(p_hwfn,
-			  "Failed to acquire the resource lock for the resource allocation commands\n");
-		return -EBUSY;
 	} else {
-		rc = qed_hw_set_soft_resc_size(p_hwfn, p_ptt);
-		if (rc && rc != -EINVAL) {
+		if (!resc_lock_params.b_granted) {
 			DP_NOTICE(p_hwfn,
-				  "Failed to set the max values of the soft resources\n");
-			goto unlock_and_exit;
-		} else if (rc == -EINVAL) {
+				  "Failed to acquire the resource lock for the resource allocation commands\n");
+			return -EBUSY;
+		}
+
+		rc = qed_hw_set_soft_resc_size(p_hwfn, p_ptt);
+		if (rc) {
+			if (rc != -EINVAL) {
+				DP_NOTICE(p_hwfn,
+					  "Failed to set the max values of the soft resources\n");
+				goto unlock_and_exit;
+			}
+
 			DP_INFO(p_hwfn,
 				"Skip the max values setting of the soft resources since it is not supported by the MFW\n");
-			rc = qed_mcp_resc_unlock(p_hwfn, p_ptt,
-						 &resc_unlock_params);
+			rc = qed_mcp_resc_unlock(p_hwfn, p_ptt, &resc_unlock_params);
 			if (rc)
 				DP_INFO(p_hwfn,
 					"Failed to release the resource lock for the resource allocation commands\n");

From: Jean Sacren <redacted>

We should first check rc alone and then check it against -EINVAL to
avoid repeating the same operation.

We should also remove the check of !rc in (!rc && !params.b_granted)
since it is always true.

With this change, we could also use constant 0 for return.

Signed-off-by: Jean Sacren <redacted>
Reviewed-by: Simon Horman <horms@kernel.org>
---
v2:
(1) Add text for !rc removal in the changelog.
(2) Add Reviewed-by tag of Mr. Simon Horman.
 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) {
 		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)

-----Original Message-----
From: Jεan Sacren <redacted>
Sent: Friday, October 22, 2021 9:08 AM
To: Ariel Elior <redacted>
Cc: GR-everest-linux-l2 <redacted>;
davem@davemloft.net; kuba@kernel.org; netdev@vger.kernel.org
Subject: [EXT] [PATCH net-next v2 1/2] net: qed_ptp: fix redundant check of rc
and against -EINVAL

External Email

----------------------------------------------------------------------
From: Jean Sacren <redacted>

We should first check rc alone and then check it against -EINVAL to
avoid repeating the same operation.

We should also remove the check of !rc in (!rc && !params.b_granted)
since it is always true.

With this change, we could also use constant 0 for return.

Signed-off-by: Jean Sacren <redacted>
Reviewed-by: Simon Horman <horms@kernel.org>
---

Acked-by: Prabhakar Kushwaha <redacted>

On Thu, 21 Oct 2021 21:37:41 -0600 Jεan Sacren wrote:

From: Jean Sacren <redacted>

We should first check rc alone and then check it against -EINVAL to
avoid repeating the same operation multiple times.

We should also remove the check of !rc in this expression since it is
always true:

	(!rc && !resc_lock_params.b_granted)

Signed-off-by: Jean Sacren <redacted>

The code seems to be written like this on purpose. You're adding
indentation levels, and making the structure less readable IMO.

If you want to avoid checking rc / !rc multiple times you can just
code it as:

	if (rc == -EINVAL)
		...
	else if (rc)
		...
	else if (!granted)
		...
	else
		...

I'm not sure I see the point of the re-factoring.

From: Jakub Kicinski <kuba@kernel.org>
Date: Fri, 22 Oct 2021 14:47:20 -0700

On Thu, 21 Oct 2021 21:37:41 -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 multiple times.

We should also remove the check of !rc in this expression since it is
always true:

	(!rc && !resc_lock_params.b_granted)

Signed-off-by: Jean Sacren <redacted>

The code seems to be written like this on purpose. You're adding
indentation levels, and making the structure less readable IMO.

If you want to avoid checking rc / !rc multiple times you can just
code it as:

	if (rc == -EINVAL)
		...
	else if (rc)
		...
	else if (!granted)
		...
	else
		...

I'm not sure I see the point of the re-factoring.

Agreed.