Re: [PATCH net-next v2 2/2] net: qed_dev: fix redundant check of rc and against -EINVAL
From: Jakub Kicinski <kuba@kernel.org>
Date: 2021-10-22 21:47:24
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.
quoted hunk ↗ jump to hunk
(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");