Skip to contentSkip to conversation

Small fixes for redundant checks

PATCH In review Jεan Sacren 3 participants 6 messages Oct 22, 2021 – Oct 23, 2021

Subsystems (3)
Networking Network drivers QLogic QL4xxx Ethernet driver
lore
Share & tools
lore

Jεan Sacren

author
#0 lore raw

v2 cover Small fixes for redundant checks

From: Jean Sacren <[email protected]>

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(-)
author
#1 lore raw

PATCH v2 1/2 net: qed_ptp: fix redundant check of rc and against -EINVAL

From: Jean Sacren <[email protected]>

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 <[email protected]>
Reviewed-by Simon Horman <[email protected]>

---
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(-)
drivers/net/ethernet/qlogic/qed/qed_ptp.c +7 -5
index 2c62d732e5c2..c927ff409109 100644
@@ -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)
Acked-by on v2 1/2
#2 lore raw

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

Quoted 18 lines

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

External Email

----------------------------------------------------------------------
From: Jean Sacren <[email protected]>

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

Quoted 8 lines

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 <[email protected]>
Reviewed-by Simon Horman <[email protected]>

---

Acked-by Prabhakar Kushwaha <[email protected]>
author
#3 lore raw

PATCH v2 2/2 net: qed_dev: fix redundant check of rc and against -EINVAL

In reply to Jεan Sacren

From: Jean Sacren <[email protected]>

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 <[email protected]>

---
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(-)
drivers/net/ethernet/qlogic/qed/qed_dev.c +17 -14
index 18f3bf7c4dfe..4ae9867b2535 100644
@@ -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");
maintainer
#4 lore raw

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

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

From: Jean Sacren <[email protected]>

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 <[email protected]>

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.

Quoted 55 lines

(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(-)
drivers/net/ethernet/qlogic/qed/qed_dev.c +17 -14
index 18f3bf7c4dfe..4ae9867b2535 100644
@@ -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");
author
#5 lore raw

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

From: Jakub Kicinski <[email protected]>
Date: Fri, 22 Oct 2021 14:47:20 -0700

Quoted 29 lines · On Thu, 21 Oct 2021 21:37:41 -0600 Jεan Sacren wrote:
Quoted 11 lines

From: Jean Sacren <[email protected]>

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 <[email protected]>

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
		...
Quoted 1 line

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

Agreed.