Re: [PATCH net-next 2/2] net: qed_dev: fix redundant check of rc and against -EINVAL
From: Simon Horman <horms@kernel.org>
Date: 2021-10-20 08:47:23
On Tue, Oct 19, 2021 at 12:26:42AM -0600, Jεan Sacren wrote:
quoted hunk ↗ jump to hunk
From: Jean Sacren <redacted> We should first check rc alone and then check it against -EINVAL to avoid repeating the same operation multiple times. Signed-off-by: Jean Sacren <redacted> --- drivers/net/ethernet/qlogic/qed/qed_dev.c | 35 +++++++++++++---------- 1 file changed, 20 insertions(+), 15 deletions(-)diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c b/drivers/net/ethernet/qlogic/qed/qed_dev.c index 18f3bf7c4dfe..fe8bdb4523b5 100644 --- a/drivers/net/ethernet/qlogic/qed/qed_dev.c +++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c@@ -3987,30 +3987,35 @@ 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) { + } + + if (!resc_lock_params.b_granted) {
Can it be the case where the condition above is met and !rc is false? If so your patch seems to have changed the logic of this function.
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) {
+ }
+
+ 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;
- } else if (rc == -EINVAL) {
- 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);nit: it looks like the two lines above would now fit on one.
- if (rc) - DP_INFO(p_hwfn, - "Failed to release the resource lock for the resource allocation commands\n"); } + + 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); + if (rc) + DP_INFO(p_hwfn, + "Failed to release the resource lock for the resource allocation commands\n"); } rc = qed_hw_set_resc_info(p_hwfn);