[PATCH for-next 1/2] RDMA/bnxt_re: Remove incorrect return check from slow path

[PATCH for-next 1/2] RDMA/bnxt_re: Remove incorrect return check from slow path

[Kashyap Desai]

[-- Attachment #1: Type: text/plain, Size: 1388 bytes --]

The patch 691eb7c6110f: "RDMA/bnxt_re: handle command
completions after driver detect a timedout" introduced code resulting in
below warning issued by the smatch static checker.

        drivers/infiniband/hw/bnxt_re/qplib_rcfw.c:513 __bnxt_qplib_rcfw_send_message()
        warn: duplicate check 'rc' (previous on line 506)

Fix the warning by removing incorrect code block.

Fixes: 691eb7c6110f ("RDMA/bnxt_re: handle command completions after driver detect a timedout")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Kashyap Desai <kashyap.desai@broadcom.com>
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
index bb5aebafe162..30c6e865d691 100644
--- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
+++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
@@ -503,12 +503,6 @@ static int __bnxt_qplib_rcfw_send_message(struct bnxt_qplib_rcfw *rcfw,
 		rc = __wait_for_resp(rcfw, cookie);
 	else
 		rc = __poll_for_resp(rcfw, cookie);
-	if (rc) {
-		/* timed out */
-		dev_err(&rcfw->pdev->dev, "cmdq[%#x]=%#x timedout (%d)msec\n",
-			cookie, opcode, RCFW_CMD_WAIT_TIME_MS);
-		return rc;
-	}
 
 	if (rc) {
 		spin_lock_irqsave(&rcfw->cmdq.hwq.lock, flags);
-- 
2.27.0


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

[PATCH for-next 2/2] RDMA/bnxt_re: refactor code around bnxt_qplib_map_rc

[Kashyap Desai]

[-- Attachment #1: Type: text/plain, Size: 2660 bytes --]

Updated function comment of bnxt_qplib_map_rc
Removed intermediate return value ENXIO and directly called bnxt_qplib_map_rc
from __send_message_basic_sanity.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Kashyap Desai <kashyap.desai@broadcom.com>
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 23 ++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
index 30c6e865d691..a8323054cfee 100644
--- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
+++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
@@ -57,13 +57,20 @@ static void bnxt_qplib_service_creq(struct tasklet_struct *t);
  * bnxt_qplib_map_rc  -  map return type based on opcode
  * @opcode    -  roce slow path opcode
  *
- * In some cases like firmware halt is detected, the driver is supposed to
- * remap the error code of the timed out command.
+ * case #1
+ * Firmware initiated error recovery is a safe state machine and
+ * driver can consider all the underlying rdma resources are free.
+ * In this state, it is safe to return success for opcodes related to
+ * destroying rdma resources (like destroy qp, destroy cq etc.).
  *
- * It is not safe to assume hardware is really inactive so certain opcodes
- * like destroy qp etc are not safe to be returned success, but this function
- * will be called when FW already reports a timeout. This would be possible
- * only when FW crashes and resets. This will clear all the HW resources.
+ * case #2
+ * If driver detect potential firmware stall, it is not safe state machine
+ * and the driver can not consider all the underlying rdma resources are
+ * freed.
+ * In this state, it is not safe to return success for opcodes related to
+ * destroying rdma resources (like destroy qp, destroy cq etc.).
+ *
+ * Scope of this helper function is only for case #1.
  *
  * Returns:
  * 0 to communicate success to caller.
@@ -418,7 +425,7 @@ static int __send_message_basic_sanity(struct bnxt_qplib_rcfw *rcfw,
 
 	/* Prevent posting if f/w is not in a state to process */
 	if (test_bit(ERR_DEVICE_DETACHED, &rcfw->cmdq.flags))
-		return -ENXIO;
+		return bnxt_qplib_map_rc(opcode);
 	if (test_bit(FIRMWARE_STALL_DETECTED, &cmdq->flags))
 		return -ETIMEDOUT;
 
@@ -488,7 +495,7 @@ static int __bnxt_qplib_rcfw_send_message(struct bnxt_qplib_rcfw *rcfw,
 
 	rc = __send_message_basic_sanity(rcfw, msg, opcode);
 	if (rc)
-		return rc == -ENXIO ? bnxt_qplib_map_rc(opcode) : rc;
+		return rc;
 
 	rc = __send_message(rcfw, msg);
 	if (rc)
-- 
2.27.0


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH for-next 2/2] RDMA/bnxt_re: refactor code around bnxt_qplib_map_rc

[Dan Carpenter]

On Fri, Jun 16, 2023 at 11:47:00AM +0530, Kashyap Desai wrote:
> Updated function comment of bnxt_qplib_map_rc
> Removed intermediate return value ENXIO and directly called bnxt_qplib_map_rc
> from __send_message_basic_sanity.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Signed-off-by: Kashyap Desai <kashyap.desai@broadcom.com>
> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> ---

Probably Reported-by is giving me to much credit but I do like this
change.  It's more clear now.  Thanks, a lot!

regards,
dan carpenter

Re: [PATCH for-next 1/2] RDMA/bnxt_re: Remove incorrect return check from slow path

[Jason Gunthorpe]

On Fri, Jun 16, 2023 at 11:46:59AM +0530, Kashyap Desai wrote:
> The patch 691eb7c6110f: "RDMA/bnxt_re: handle command
> completions after driver detect a timedout" introduced code resulting in
> below warning issued by the smatch static checker.
> 
>         drivers/infiniband/hw/bnxt_re/qplib_rcfw.c:513 __bnxt_qplib_rcfw_send_message()
>         warn: duplicate check 'rc' (previous on line 506)
> 
> Fix the warning by removing incorrect code block.
> 
> Fixes: 691eb7c6110f ("RDMA/bnxt_re: handle command completions after driver detect a timedout")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Signed-off-by: Kashyap Desai <kashyap.desai@broadcom.com>
> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> ---
>  drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 6 ------
>  1 file changed, 6 deletions(-)

Both applied to for-next, thanks

Jason