[PATCH for-rc 2/7] RDMA/bnxt_re: Honor a completion that races with a wait timeout

[Selvin Xavier <selvin.xavier@broadcom.com>]

If the wait helper gives up (timeout or firmware stall) and the CREQ
tasklet processes the command before the waiter takes hwq->lock, the
response is already copied into the caller's buffer and is_in_used is
clear. The waiter still returns -ETIMEDOUT and may set
FIRMWARE_STALL_DETECTED, and for CREATE_AH the handler skips
destroy_timedout_ah() because the waiter was alive, so the AH leaks in
firmware.

Under the lock, if the slot no longer holds this command's response
buffer, a completion was delivered. Use its status instead of failing.

To keep that check valid, never reuse a cookie slot that is still in
use: __send_message() returns -EAGAIN and __send_message_no_waiter()
drops the internal command.

A slot stays in use only while its completion is outstanding. If that
completion never arrives, the cause is a PCI error or firmware still
processing the command, so there is no point in posting more commands.
Continuing to return -EAGAIN until firmware completes the command in
order is intended; otherwise recovery is initiated.

An internal destroy_ah dropped for the same reason leaks the AH in
firmware; this is accepted and the comment in
bnxt_qplib_service_creq() is updated to say so.

Fixes: 691eb7c6110f ("RDMA/bnxt_re: handle command completions after driver detect a timedout")
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 31 ++++++++++++++++++----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
index 707687a8f301..167626246800 100644
--- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
+++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
@@ -223,7 +223,7 @@ static int __block_for_resp(struct bnxt_qplib_rcfw *rcfw, u16 cookie)
  * Current design of this function is -
  * user must hold the completion queue hwq->lock.
  * user must have used existing completion and free the resources.
- * this function will not check queue full condition.
+ * the command is dropped if the cookie slot is in use or the queue is full.
  * this function will explicitly set is_waiter_alive=false.
  * current use case is - send destroy_ah if create_ah is return
  * after waiter of create_ah is lost. It can be extended for other
@@ -245,9 +245,17 @@ static void __send_message_no_waiter(struct bnxt_qplib_rcfw *rcfw,
 	u8 *preq;
 
 	cookie = cmdq->seq_num & RCFW_MAX_COOKIE_VALUE;
-	__set_cmdq_base_cookie(msg->req, msg->req_sz, cpu_to_le16(cookie));
 	crsqe = &rcfw->crsqe_tbl[cookie];
 
+	if (crsqe->is_in_used ||
+	    bnxt_qplib_get_cmd_slots(msg->req) >= HWQ_FREE_SLOTS(hwq)) {
+		dev_warn_ratelimited(&rcfw->pdev->dev,
+				     "CMDQ cookie %#x busy or CMDQ full, dropping internal command\n",
+				     cookie);
+		return;
+	}
+	__set_cmdq_base_cookie(msg->req, msg->req_sz, cpu_to_le16(cookie));
+
 	/* Set cmd_size in terms of 16B slots in req. */
 	bsize = bnxt_qplib_set_cmd_slots(msg->req);
 	/* GET_CMD_SIZE would return number of slots in either case of tlv
@@ -314,6 +322,12 @@ static int __send_message(struct bnxt_qplib_rcfw *rcfw,
 		spin_unlock_bh(&hwq->lock);
 		return -EAGAIN;
 	}
+	if (crsqe->is_in_used) {
+		dev_info_ratelimited(&pdev->dev,
+				     "CMDQ cookie %#x still in use\n", cookie);
+		spin_unlock_bh(&hwq->lock);
+		return -EAGAIN;
+	}
 	if (msg->block)
 		cookie |= RCFW_CMD_IS_BLOCKING;
 	__set_cmdq_base_cookie(msg->req, msg->req_sz, cpu_to_le16(cookie));
@@ -520,6 +534,11 @@ static int __bnxt_qplib_rcfw_send_message(struct bnxt_qplib_rcfw *rcfw,
 		pending = true;
 		crsqe->is_waiter_alive = false;
 		crsqe->resp = NULL;
+	} else if (rc) {
+		/* The completion was processed after the wait gave up but
+		 * before the lock was taken. The response is valid.
+		 */
+		rc = 0;
 	}
 	if (rc == -ENODEV)
 		set_bit(FIRMWARE_STALL_DETECTED, &rcfw->cmdq.flags);
@@ -726,9 +745,11 @@ static int bnxt_qplib_process_qp_event(struct bnxt_qplib_rcfw *rcfw,
 		 * so let's destroy the same address vector since it is
 		 * no more used in stack. We don't care about completion
 		 * in __send_message_no_waiter.
-		 * If destroy_ah is failued by firmware, there will be AH
-		 * resource leak and relatively not critical +  unlikely
-		 * scenario. Current design is not to handle such case.
+		 * If destroy_ah is failed by firmware, or is dropped locally
+		 * because the cookie slot is in use or the CMDQ is full,
+		 * there will be AH resource leak and relatively not
+		 * critical + unlikely scenario. Current design is not to
+		 * handle such case.
 		 */
 		if (!is_waiter_alive && !qp_event->status &&
 		    qp_event->event == CREQ_QP_EVENT_EVENT_CREATE_AH)
-- 
2.39.3