[openib-general] [PATCH] mthca: correct IB_QP_ACCESS_FLAGS handling

[Michael S. Tsirkin]

This patch corrects some corner cases in managing the RAE/RRE bits in the
mthca qp context.  These bits need to be zero if the user requests
max_dest_rd_atomic of zero.  The bits need to be
restored to the value implied by the qp access flags attribute in
a previous (or the current) modify-qp command if the dest_rd_atomic variable
is changed to non-zero.

In the current implementation, the following scenario will not work:
RESET-to-INIT 	set QP access flags to all disabled (zeroes)
INIT-to-RTR     set max_dest_rd_atomic=10, AND 
		set qp_access_flags = IB_ACCESS_REMOTE_READ | IB_ACCESS_REMOTE_ATOMIC

The current code will incorrectly take the access-flags value set in the
RESET-to-INIT transition.

---

Simplify, and correct, IB_QP_ACCESS_FLAGS handling:
it is always safe to set qp access flags in hardware command
if either of IB_QP_MAX_DEST_RD_ATOMIC or IB_QP_ACCESS_FLAGS is set,
so lets just set it to the correct value, always.

Signed-off-by: Jack Morgenstein <jackm at mellanox.co.il>
Signed-off-by: Michael S. Tsirkin <mst at mellanox.co.il>

Index: linux-kernel/drivers/infiniband/hw/mthca/mthca_qp.c
===================================================================
--- linux-kernel.orig/drivers/infiniband/hw/mthca/mthca_qp.c
+++ linux-kernel/drivers/infiniband/hw/mthca/mthca_qp.c
@@ -520,6 +520,36 @@ static void init_port(struct mthca_dev *
 		mthca_warn(dev, "INIT_IB returned status %02x.\n", status);
 }
 
+static u32 get_hw_access_flags(struct mthca_qp *qp, struct ib_qp_attr *attr,
+			       int attr_mask)
+{
+	u8 dest_rd_atomic;
+	u32 access_flags;
+	u32 hw_access_flags;
+
+	if (attr_mask & IB_QP_MAX_DEST_RD_ATOMIC)
+		dest_rd_atomic = attr->max_dest_rd_atomic;
+	else
+		dest_rd_atomic = qp->resp_depth;
+
+	if (attr_mask & IB_QP_ACCESS_FLAGS)
+		access_flags = (u32)attr->qp_access_flags;
+	else
+		access_flags = qp->atomic_rd_en;
+
+	if (!dest_rd_atomic)
+		access_flags &= IB_ACCESS_REMOTE_WRITE;
+
+	hw_access_flags = access_flags & IB_ACCESS_REMOTE_READ ?
+			  MTHCA_QP_BIT_RRE : 0;
+	hw_access_flags |= access_flags & IB_ACCESS_REMOTE_ATOMIC ?
+			   MTHCA_QP_BIT_RAE : 0;
+	hw_access_flags |= access_flags & IB_ACCESS_REMOTE_WRITE ?
+			   MTHCA_QP_BIT_RWE : 0;
+
+	return cpu_to_be32(hw_access_flags);
+}
+
 int mthca_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr, int attr_mask)
 {
 	struct mthca_dev *dev = to_mdev(ibqp->device);
@@ -741,57 +776,7 @@ int mthca_modify_qp(struct ib_qp *ibqp, 
 		qp_context->snd_db_index   = cpu_to_be32(qp->sq.db_index);
 	}
 
-	if (attr_mask & IB_QP_ACCESS_FLAGS) {
-		qp_context->params2 |=
-			cpu_to_be32(attr->qp_access_flags & IB_ACCESS_REMOTE_WRITE ?
-				    MTHCA_QP_BIT_RWE : 0);
-
-		/*
-		 * Only enable RDMA reads and atomics if we have
-		 * responder resources set to a non-zero value.
-		 */
-		if (qp->resp_depth) {
-			qp_context->params2 |=
-				cpu_to_be32(attr->qp_access_flags & IB_ACCESS_REMOTE_READ ?
-					    MTHCA_QP_BIT_RRE : 0);
-			qp_context->params2 |=
-				cpu_to_be32(attr->qp_access_flags & IB_ACCESS_REMOTE_ATOMIC ?
-					    MTHCA_QP_BIT_RAE : 0);
-		}
-
-		qp_param->opt_param_mask |= cpu_to_be32(MTHCA_QP_OPTPAR_RWE |
-							MTHCA_QP_OPTPAR_RRE |
-							MTHCA_QP_OPTPAR_RAE);
-	}
-
 	if (attr_mask & IB_QP_MAX_DEST_RD_ATOMIC) {
-		if (qp->resp_depth && !attr->max_dest_rd_atomic) {
-			/*
-			 * Lowering our responder resources to zero.
-			 * Turn off reads RDMA and atomics as responder.
-			 * (RRE/RAE in params2 already zero)
-			 */
-			qp_param->opt_param_mask |= cpu_to_be32(MTHCA_QP_OPTPAR_RRE |
-								MTHCA_QP_OPTPAR_RAE);
-		}
-
-		if (!qp->resp_depth && attr->max_dest_rd_atomic) {
-			/*
-			 * Increasing our responder resources from
-			 * zero.  Turn on RDMA reads and atomics as
-			 * appropriate.
-			 */
-			qp_context->params2 |=
-				cpu_to_be32(qp->atomic_rd_en & IB_ACCESS_REMOTE_READ ?
-					    MTHCA_QP_BIT_RRE : 0);
-			qp_context->params2 |=
-				cpu_to_be32(qp->atomic_rd_en & IB_ACCESS_REMOTE_ATOMIC ?
-					    MTHCA_QP_BIT_RAE : 0);
-
-			qp_param->opt_param_mask |= cpu_to_be32(MTHCA_QP_OPTPAR_RRE |
-								MTHCA_QP_OPTPAR_RAE);
-		}
-
 		if (attr->max_dest_rd_atomic)
 			qp_context->params2 |=
 				cpu_to_be32(fls(attr->max_dest_rd_atomic - 1) << 21);
@@ -799,6 +784,13 @@ int mthca_modify_qp(struct ib_qp *ibqp, 
 		qp_param->opt_param_mask |= cpu_to_be32(MTHCA_QP_OPTPAR_RRA_MAX);
 	}
 
+	if (attr_mask & (IB_QP_ACCESS_FLAGS | IB_QP_MAX_DEST_RD_ATOMIC)) {
+		qp_context->params2 |= get_hw_access_flags(qp, attr, attr_mask);
+		qp_param->opt_param_mask |= cpu_to_be32(MTHCA_QP_OPTPAR_RWE |
+							MTHCA_QP_OPTPAR_RRE |
+							MTHCA_QP_OPTPAR_RAE);
+	}
+
 	qp_context->params2 |= cpu_to_be32(MTHCA_QP_BIT_RSC);
 
 	if (ibqp->srq)

-- 
MST