* [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; 6+ 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] 6+ 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; 6+ 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 related [flat|nested] 6+ 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; 6+ 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(¶ms, NULL, resource, true);
rc = qed_mcp_resc_lock(p_hwfn, p_ptt, ¶ms);
- 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 related [flat|nested] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ messages in thread
end of thread, other threads:[~2021-10-23 9:27 UTC | newest]
Thread overview: 6+ 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
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.