[openib-general] Re: [PATCH] [SRP] Fix CM redirection in SRP

[Roland Dreier]

I rewrote your patch to fix a few things:

 - I believe we always need to copy the redirect GID, even if the
   redirect LID is non-zero; otherwise we will end up putting the GID
   of the original port in our CM REQ.  From my reading of the
   description of ClassPortInfo in the IB spec, I think the redirect
   GID must always be valid if redirection is being done.

 - I moved the connect handling into a new function
   srp_connect_target(), so that both the initial connect and future
   reconnects get handling for port and DLID redirection.

 - I got rid of the change for CM REJ reason 25 -- that reject code
   does not carry a ClassPortInfo in the ARI, but rather just the
   GID at offset 0.

By the way, it's not that critical for SRP, since it's not in the
upstream kernel yet, but I don't think we should use obfuscated email
addresses in Signed-off-by lines:

    > Signed-off-by: John Kingman <kingman <at> storagegear.com>

Just put the '@' in there -- I think there are much bigger spam
sources to worry about.

Anyway, here's the updated patch.  Does this look OK to you?

 - R.

Index: infiniband/ulp/srp/ib_srp.c
===================================================================
--- infiniband/ulp/srp/ib_srp.c	(revision 3372)
+++ infiniband/ulp/srp/ib_srp.c	(working copy)
@@ -386,11 +386,48 @@ static void srp_remove_work(void *target
 	scsi_host_put(target->scsi_host);
 }
 
+static int srp_connect_target(struct srp_target_port *target)
+{
+	int ret;
+
+	while (1) {
+		init_completion(&target->done);
+		ret = srp_send_req(target);
+		if (ret)
+			return ret;
+		wait_for_completion(&target->done);
+
+		/*
+		 * The CM event handling code will set status to
+		 * SRP_PORT_REDIRECT if we get a port redirect REJ
+		 * back, or SRP_DLID_REDIRECT if we get a lid/qp
+		 * redirect REJ back.
+		 */
+		switch (target->status) {
+		case 0:
+			return 0;
+
+		case SRP_PORT_REDIRECT:
+			ret = srp_lookup_path(target);
+			if (ret)
+				return ret;
+			break;
+
+		case SRP_DLID_REDIRECT:
+			break;
+
+		default:
+			return target->status;
+		}
+	}
+}
+
 static int srp_reconnect_target(struct srp_target_port *target)
 {
 	struct ib_qp_attr qp_attr;
 	struct srp_request *req;
 	struct ib_wc wc;
+	u32 remote_cm_qpn;
 	int ret;
 	int i;
 
@@ -402,6 +439,8 @@ static int srp_reconnect_target(struct s
 	target->state = SRP_TARGET_CONNECTING;
 	spin_unlock_irq(target->scsi_host->host_lock);
 
+	remote_cm_qpn = target->cm_id->remote_cm_qpn;
+
 	srp_disconnect_target(target);
 
 	target->cm_id = ib_create_cm_id(srp_cm_handler, target);
@@ -411,6 +450,8 @@ static int srp_reconnect_target(struct s
 		goto err;
 	}
 
+	target->cm_id->remote_cm_qpn = remote_cm_qpn;
+
 	qp_attr.qp_state = IB_QPS_RESET;
 	ret = ib_modify_qp(target->qp, &qp_attr, IB_QP_STATE);
 	if (ret)
@@ -438,24 +479,9 @@ static int srp_reconnect_target(struct s
 	target->req_ring[SRP_SQ_SIZE - 1].next = -1;
 	INIT_LIST_HEAD(&target->req_queue);
 
-retry_connect:
-	init_completion(&target->done);
-	ret = srp_send_req(target);
+	ret = srp_connect_target(target);
 	if (ret)
 		goto err;
-	wait_for_completion(&target->done);
-
-	/*
-	 * The CM event handling code will set status to
-	 * SRP_PORT_REDIRECT if we get a port redirect REJ back.
-	 */
-	if (target->status == SRP_PORT_REDIRECT) {
-		ret = srp_lookup_path(target);
-		if (ret)
-			goto err;
-		goto retry_connect;
-	} else if (target->status < 0)
-		goto err;
 
 	spin_lock_irq(target->scsi_host->host_lock);
 	if (target->state == SRP_TARGET_CONNECTING) {
@@ -1031,8 +1057,13 @@ static int srp_cm_handler(struct ib_cm_i
 
 		if (event->param.rej_rcvd.reason == IB_CM_REJ_PORT_CM_REDIRECT) {
 			cpi = event->param.rej_rcvd.ari;
+			target->path.dlid = cpi->redirect_lid;
+			target->path.pkey = cpi->redirect_pkey;
+			cm_id->remote_cm_qpn = be32_to_cpu(cpi->redirect_qp) & 0x00ffffff;
 			memcpy(target->path.dgid.raw, cpi->redirect_gid, 16);
-			target->status = SRP_PORT_REDIRECT;
+
+			target->status = target->path.dlid ?
+				SRP_DLID_REDIRECT : SRP_PORT_REDIRECT;
 		} else if (topspin_workarounds &&
 			   !memcmp(&target->ioc_guid, topspin_oui, 3) &&
 			   event->param.rej_rcvd.reason == IB_CM_REJ_PORT_REDIRECT) {
@@ -1042,7 +1073,7 @@ static int srp_cm_handler(struct ib_cm_i
 			 * (port redirect).
 			 */
 			memcpy(target->path.dgid.raw,
-			       event->param.rej_rcvd.ari + 0, 16);
+			       event->param.rej_rcvd.ari, 16);
 
 			printk(KERN_DEBUG PFX "Topspin/Cisco redirect to target port GID %016llx%016llx\n",
 			       (unsigned long long) be64_to_cpu(target->path.dgid.global.subnet_prefix),
@@ -1386,28 +1417,15 @@ static ssize_t srp_create_target(struct 
 	       (int) be16_to_cpu(*(__be16 *) &target->path.dgid.raw[12]),
 	       (int) be16_to_cpu(*(__be16 *) &target->path.dgid.raw[14]));
 
-retry_path:
 	ret = srp_lookup_path(target);
 	if (ret) {
 		ib_destroy_cm_id(target->cm_id);
 		goto err;
 	}
 
-	init_completion(&target->done);
-	ret = srp_send_req(target);
-	if (ret)
-		goto err;
-	wait_for_completion(&target->done);
-
-	/*
-	 * The CM event handling code will set status to
-	 * SRP_PORT_REDIRECT if we get a port redirect REJ back.
-	 */
-	if (target->status == SRP_PORT_REDIRECT)
-		goto retry_path;
-	else if (target->status < 0) {
+	ret = srp_connect_target(target);
+	if (ret) {
 		printk(KERN_ERR PFX "Connection failed\n");
-		ret = target->status;
 		goto err;
 	}
 
Index: infiniband/ulp/srp/ib_srp.h
===================================================================
--- infiniband/ulp/srp/ib_srp.h	(revision 3372)
+++ infiniband/ulp/srp/ib_srp.h	(working copy)
@@ -52,6 +52,7 @@ enum {
 	SRP_ABORT_TIMEOUT_MS	= 5000,
 
 	SRP_PORT_REDIRECT	= 1,
+	SRP_DLID_REDIRECT	= 2,
 
 	SRP_MAX_IU_LEN		= 256,