netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next v2 0/2] Small fixes for redundant checks
@ 2021-10-22  3:37 Jεan Sacren
  2021-10-22  3:37 ` [PATCH net-next v2 2/2] net: qed_dev: fix redundant check of rc and against -EINVAL Jεan Sacren
  2021-10-22  3:37 ` [PATCH net-next v2 1/2] net: qed_ptp: " Jεan Sacren
  0 siblings, 2 replies; 7+ messages in thread
From: Jεan Sacren @ 2021-10-22  3:37 UTC (permalink / raw)
  To: Ariel Elior; +Cc: GR-everest-linux-l2, davem, kuba, netdev

From: Jean Sacren <sakiwit@gmail.com>

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(-)


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH net-next v2 2/2] net: qed_dev: fix redundant check of rc and against -EINVAL
  2021-10-22  3:37 [PATCH net-next v2 0/2] Small fixes for redundant checks Jεan Sacren
@ 2021-10-22  3:37 ` Jεan Sacren
  2021-10-22 21:47   ` Jakub Kicinski
  2021-10-22  3:37 ` [PATCH net-next v2 1/2] net: qed_ptp: " Jεan Sacren
  1 sibling, 1 reply; 7+ messages in thread
From: Jεan Sacren @ 2021-10-22  3:37 UTC (permalink / raw)
  To: Ariel Elior; +Cc: GR-everest-linux-l2, davem, kuba, netdev

From: Jean Sacren <sakiwit@gmail.com>

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 <sakiwit@gmail.com>
---
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");

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH net-next v2 1/2] net: qed_ptp: fix redundant check of rc and against -EINVAL
  2021-10-22  3:37 [PATCH net-next v2 0/2] Small fixes for redundant checks Jεan Sacren
  2021-10-22  3:37 ` [PATCH net-next v2 2/2] net: qed_dev: fix redundant check of rc and against -EINVAL Jεan Sacren
@ 2021-10-22  3:37 ` Jεan Sacren
  2021-10-22  5:27   ` [EXT] " Prabhakar Kushwaha
  1 sibling, 1 reply; 7+ messages in thread
From: Jεan Sacren @ 2021-10-22  3:37 UTC (permalink / raw)
  To: Ariel Elior; +Cc: GR-everest-linux-l2, davem, kuba, netdev

From: Jean Sacren <sakiwit@gmail.com>

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 <sakiwit@gmail.com>
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)

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [EXT] [PATCH net-next v2 1/2] net: qed_ptp: fix redundant check of rc and against -EINVAL
  2021-10-22  3:37 ` [PATCH net-next v2 1/2] net: qed_ptp: " Jεan Sacren
@ 2021-10-22  5:27   ` Prabhakar Kushwaha
  0 siblings, 0 replies; 7+ messages in thread
From: Prabhakar Kushwaha @ 2021-10-22  5:27 UTC (permalink / raw)
  To: Jεan Sacren, Ariel Elior; +Cc: GR-everest-linux-l2, davem, kuba, netdev


> -----Original Message-----
> From: Jεan Sacren <sakiwit@gmail.com>
> Sent: Friday, October 22, 2021 9:08 AM
> To: Ariel Elior <aelior@marvell.com>
> Cc: GR-everest-linux-l2 <GR-everest-linux-l2@marvell.com>;
> 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 <sakiwit@gmail.com>
> 
> 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 <sakiwit@gmail.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> ---

Acked-by: Prabhakar Kushwaha <pkushwaha@marvell.com>


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next v2 2/2] net: qed_dev: fix redundant check of rc and against -EINVAL
  2021-10-22  3:37 ` [PATCH net-next v2 2/2] net: qed_dev: fix redundant check of rc and against -EINVAL Jεan Sacren
@ 2021-10-22 21:47   ` Jakub Kicinski
  2021-10-23  9:26     ` Jεan Sacren
  0 siblings, 1 reply; 7+ messages in thread
From: Jakub Kicinski @ 2021-10-22 21:47 UTC (permalink / raw)
  To: Jεan Sacren; +Cc: Ariel Elior, GR-everest-linux-l2, davem, netdev

On Thu, 21 Oct 2021 21:37:41 -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 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 <sakiwit@gmail.com>

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.

> (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");


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next v2 2/2] net: qed_dev: fix redundant check of rc and against -EINVAL
  2021-10-22 21:47   ` Jakub Kicinski
@ 2021-10-23  9:26     ` Jεan Sacren
  0 siblings, 0 replies; 7+ messages in thread
From: Jεan Sacren @ 2021-10-23  9:26 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: Ariel Elior, GR-everest-linux-l2, davem, netdev

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:
> > From: Jean Sacren <sakiwit@gmail.com>
> > 
> > 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 <sakiwit@gmail.com>
> 
> 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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH net-next v2 2/2] net: qed_dev: fix redundant check of rc and against -EINVAL
@ 2021-10-22  5:28 Prabhakar Kushwaha
  0 siblings, 0 replies; 7+ messages in thread
From: Prabhakar Kushwaha @ 2021-10-22  5:28 UTC (permalink / raw)
  To: Jεan Sacren, Ariel Elior; +Cc: GR-everest-linux-l2, davem, kuba, netdev

> From: Jean Sacren <sakiwit@gmail.com>
> 
> 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 <sakiwit@gmail.com>
> ---
> 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 +++++++++++++----------

Acked-by: Prabhakar Kushwaha <pkushwaha@marvell.com>

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-10-23  9:27 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-22  3:37 [PATCH net-next v2 0/2] Small fixes for redundant checks Jεan Sacren
2021-10-22  3:37 ` [PATCH net-next v2 2/2] net: qed_dev: fix redundant check of rc and against -EINVAL Jεan Sacren
2021-10-22 21:47   ` Jakub Kicinski
2021-10-23  9:26     ` Jεan Sacren
2021-10-22  3:37 ` [PATCH net-next v2 1/2] net: qed_ptp: " Jεan Sacren
2021-10-22  5:27   ` [EXT] " Prabhakar Kushwaha
2021-10-22  5:28 [PATCH net-next v2 2/2] net: qed_dev: " Prabhakar Kushwaha

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).