All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/15] Various ib_srpt patches
@ 2016-01-05 14:19 Bart Van Assche
       [not found] ` <568BD0FC.70207-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  0 siblings, 1 reply; 57+ messages in thread
From: Bart Van Assche @ 2016-01-05 14:19 UTC (permalink / raw)
  To: Doug Ledford; +Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

The following series of patches is what I came up with while testing the 
most recent version of my SCSI target patch series (see also 
http://thread.gmane.org/gmane.linux.scsi.target.devel/10905):

0001-IB-srpt-Add-parentheses-around-sizeof-argument.patch
0002-IB-srpt-Inline-srpt_sdev_name.patch
0003-IB-srpt-Inline-srpt_get_ch_state.patch
0004-IB-srpt-Introduce-target_reverse_dma_direction.patch
0005-IB-srpt-Use-scsilun_to_int.patch
0006-IB-srpt-Simplify-srpt_handle_tsk_mgmt.patch
0007-IB-srpt-Simplify-channel-state-management.patch
0008-IB-srpt-Simplify-srpt_shutdown_session.patch
0009-IB-srpt-Fix-srpt_close_session.patch
0010-IB-srpt-Fix-srpt_handle_cmd-error-paths.patch
0011-IB-srpt-Fix-how-aborted-commands-are-processed.patch
0012-IB-srpt-Eliminate-srpt_find_channel.patch
0013-IB-srpt-Detect-session-shutdown-reliably.patch
0014-IB-srpt-Fix-srpt_write_pending.patch
0015-IB-srpt-Fix-a-rare-crash-in-srpt_close_session.patch
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH 01/15] IB/srpt: Add parentheses around sizeof argument
       [not found] ` <568BD0FC.70207-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
@ 2016-01-05 14:20   ` Bart Van Assche
       [not found]     ` <568BD142.3070900-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-05 14:21   ` [PATCH 02/15] IB/srpt: Inline srpt_sdev_name() Bart Van Assche
                     ` (13 subsequent siblings)
  14 siblings, 1 reply; 57+ messages in thread
From: Bart Van Assche @ 2016-01-05 14:20 UTC (permalink / raw)
  To: Doug Ledford; +Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

Although sizeof is an operator and hence in many cases parentheses can
be left out, the recommended kernel coding style is to surround the
sizeof argument with parentheses. This patch does not change any
functionality. This patch has been generated by running the following
shell command:

sed -i 's/sizeof \([^ );,]*\)/sizeof(\1)/g' drivers/infiniband/ulp/srpt/*.[ch]

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 42 +++++++++++++++++------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 0c3ed7c..fe9474f 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -281,7 +281,7 @@ static void srpt_get_class_port_info(struct ib_dm_mad *mad)
 	struct ib_class_port_info *cif;
 
 	cif = (struct ib_class_port_info *)mad->data;
-	memset(cif, 0, sizeof *cif);
+	memset(cif, 0, sizeof(*cif));
 	cif->base_version = 1;
 	cif->class_version = 1;
 	cif->resp_time_value = 20;
@@ -340,7 +340,7 @@ static void srpt_get_ioc(struct srpt_port *sport, u32 slot,
 		return;
 	}
 
-	memset(iocp, 0, sizeof *iocp);
+	memset(iocp, 0, sizeof(*iocp));
 	strcpy(iocp->id_string, SRPT_ID_STRING);
 	iocp->guid = cpu_to_be64(srpt_service_guid);
 	iocp->vendor_id = cpu_to_be32(sdev->dev_attr.vendor_id);
@@ -390,7 +390,7 @@ static void srpt_get_svc_entries(u64 ioc_guid,
 	}
 
 	svc_entries = (struct ib_dm_svc_entries *)mad->data;
-	memset(svc_entries, 0, sizeof *svc_entries);
+	memset(svc_entries, 0, sizeof(*svc_entries));
 	svc_entries->service_entries[0].id = cpu_to_be64(ioc_guid);
 	snprintf(svc_entries->service_entries[0].name,
 		 sizeof(svc_entries->service_entries[0].name),
@@ -483,7 +483,7 @@ static void srpt_mad_recv_handler(struct ib_mad_agent *mad_agent,
 	rsp->ah = ah;
 
 	dm_mad = rsp->mad;
-	memcpy(dm_mad, mad_wc->recv_buf.mad, sizeof *dm_mad);
+	memcpy(dm_mad, mad_wc->recv_buf.mad, sizeof(*dm_mad));
 	dm_mad->mad_hdr.method = IB_MGMT_METHOD_GET_RESP;
 	dm_mad->mad_hdr.status = 0;
 
@@ -531,7 +531,7 @@ static int srpt_refresh_port(struct srpt_port *sport)
 	struct ib_port_attr port_attr;
 	int ret;
 
-	memset(&port_modify, 0, sizeof port_modify);
+	memset(&port_modify, 0, sizeof(port_modify));
 	port_modify.set_port_cap_mask = IB_PORT_DEVICE_MGMT_SUP;
 	port_modify.clr_port_cap_mask = 0;
 
@@ -552,7 +552,7 @@ static int srpt_refresh_port(struct srpt_port *sport)
 		goto err_query_port;
 
 	if (!sport->mad_agent) {
-		memset(&reg_req, 0, sizeof reg_req);
+		memset(&reg_req, 0, sizeof(reg_req));
 		reg_req.mgmt_class = IB_MGMT_CLASS_DEVICE_MGMT;
 		reg_req.mgmt_class_version = IB_MGMT_BASE_VERSION;
 		set_bit(IB_MGMT_METHOD_GET, reg_req.method_mask);
@@ -902,14 +902,14 @@ static int srpt_get_desc_tbl(struct srpt_send_ioctx *ioctx,
 
 		db = (struct srp_direct_buf *)(srp_cmd->add_data
 					       + add_cdb_offset);
-		memcpy(ioctx->rbufs, db, sizeof *db);
+		memcpy(ioctx->rbufs, db, sizeof(*db));
 		*data_len = be32_to_cpu(db->len);
 	} else if (((srp_cmd->buf_fmt & 0xf) == SRP_DATA_DESC_INDIRECT) ||
 		   ((srp_cmd->buf_fmt >> 4) == SRP_DATA_DESC_INDIRECT)) {
 		idb = (struct srp_indirect_buf *)(srp_cmd->add_data
 						  + add_cdb_offset);
 
-		ioctx->n_rbuf = be32_to_cpu(idb->table_desc.len) / sizeof *db;
+		ioctx->n_rbuf = be32_to_cpu(idb->table_desc.len) / sizeof(*db);
 
 		if (ioctx->n_rbuf >
 		    (srp_cmd->data_out_desc_cnt + srp_cmd->data_in_desc_cnt)) {
@@ -928,7 +928,7 @@ static int srpt_get_desc_tbl(struct srpt_send_ioctx *ioctx,
 			ioctx->rbufs = &ioctx->single_rbuf;
 		else {
 			ioctx->rbufs =
-				kmalloc(ioctx->n_rbuf * sizeof *db, GFP_ATOMIC);
+				kmalloc(ioctx->n_rbuf * sizeof(*db), GFP_ATOMIC);
 			if (!ioctx->rbufs) {
 				ioctx->n_rbuf = 0;
 				ret = -ENOMEM;
@@ -937,7 +937,7 @@ static int srpt_get_desc_tbl(struct srpt_send_ioctx *ioctx,
 		}
 
 		db = idb->desc_list;
-		memcpy(ioctx->rbufs, db, ioctx->n_rbuf * sizeof *db);
+		memcpy(ioctx->rbufs, db, ioctx->n_rbuf * sizeof(*db));
 		*data_len = be32_to_cpu(idb->len);
 	}
 out:
@@ -955,7 +955,7 @@ static int srpt_init_ch_qp(struct srpt_rdma_ch *ch, struct ib_qp *qp)
 	struct ib_qp_attr *attr;
 	int ret;
 
-	attr = kzalloc(sizeof *attr, GFP_KERNEL);
+	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
 	if (!attr)
 		return -ENOMEM;
 
@@ -1458,7 +1458,7 @@ static int srpt_build_cmd_rsp(struct srpt_rdma_ch *ch,
 	sense_data_len = ioctx->cmd.scsi_sense_length;
 	WARN_ON(sense_data_len > sizeof(ioctx->sense_data));
 
-	memset(srp_rsp, 0, sizeof *srp_rsp);
+	memset(srp_rsp, 0, sizeof(*srp_rsp));
 	srp_rsp->opcode = SRP_RSP;
 	srp_rsp->req_lim_delta =
 		cpu_to_be32(1 + atomic_xchg(&ch->req_lim_delta, 0));
@@ -1508,7 +1508,7 @@ static int srpt_build_tskmgmt_rsp(struct srpt_rdma_ch *ch,
 
 	srp_rsp = ioctx->ioctx.buf;
 	BUG_ON(!srp_rsp);
-	memset(srp_rsp, 0, sizeof *srp_rsp);
+	memset(srp_rsp, 0, sizeof(*srp_rsp));
 
 	srp_rsp->opcode = SRP_RSP;
 	srp_rsp->req_lim_delta =
@@ -1944,7 +1944,7 @@ static int srpt_create_ch_ib(struct srpt_rdma_ch *ch)
 	WARN_ON(ch->rq_size < 1);
 
 	ret = -ENOMEM;
-	qp_init = kzalloc(sizeof *qp_init, GFP_KERNEL);
+	qp_init = kzalloc(sizeof(*qp_init), GFP_KERNEL);
 	if (!qp_init)
 		goto out;
 
@@ -2285,9 +2285,9 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 		be64_to_cpu(*(__be64 *)&sdev->port[param->port - 1].gid.raw[0]),
 		be64_to_cpu(*(__be64 *)&sdev->port[param->port - 1].gid.raw[8]));
 
-	rsp = kzalloc(sizeof *rsp, GFP_KERNEL);
-	rej = kzalloc(sizeof *rej, GFP_KERNEL);
-	rep_param = kzalloc(sizeof *rep_param, GFP_KERNEL);
+	rsp = kzalloc(sizeof(*rsp), GFP_KERNEL);
+	rej = kzalloc(sizeof(*rej), GFP_KERNEL);
+	rep_param = kzalloc(sizeof(*rep_param), GFP_KERNEL);
 
 	if (!rsp || !rej || !rep_param) {
 		ret = -ENOMEM;
@@ -2359,7 +2359,7 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 		goto reject;
 	}
 
-	ch = kzalloc(sizeof *ch, GFP_KERNEL);
+	ch = kzalloc(sizeof(*ch), GFP_KERNEL);
 	if (!ch) {
 		rej->reason = cpu_to_be32(
 			      SRP_LOGIN_REJ_INSUFFICIENT_RESOURCES);
@@ -2461,7 +2461,7 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 	/* create cm reply */
 	rep_param->qp_num = ch->qp->qp_num;
 	rep_param->private_data = (void *)rsp;
-	rep_param->private_data_len = sizeof *rsp;
+	rep_param->private_data_len = sizeof(*rsp);
 	rep_param->rnr_retry_count = 7;
 	rep_param->flow_control = 1;
 	rep_param->failover_accepted = 0;
@@ -2507,7 +2507,7 @@ reject:
 				   | SRP_BUF_FORMAT_INDIRECT);
 
 	ib_send_cm_rej(cm_id, IB_CM_REJ_CONSUMER_DEFINED, NULL, 0,
-			     (void *)rej, sizeof *rej);
+			     (void *)rej, sizeof(*rej));
 
 out:
 	kfree(rep_param);
@@ -3005,7 +3005,7 @@ static void srpt_add_one(struct ib_device *device)
 	pr_debug("device = %p, device->dma_ops = %p\n", device,
 		 device->dma_ops);
 
-	sdev = kzalloc(sizeof *sdev, GFP_KERNEL);
+	sdev = kzalloc(sizeof(*sdev), GFP_KERNEL);
 	if (!sdev)
 		goto err;
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH 02/15] IB/srpt: Inline srpt_sdev_name()
       [not found] ` <568BD0FC.70207-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-05 14:20   ` [PATCH 01/15] IB/srpt: Add parentheses around sizeof argument Bart Van Assche
@ 2016-01-05 14:21   ` Bart Van Assche
       [not found]     ` <568BD15F.6010909-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-05 14:21   ` [PATCH 03/15] IB/srpt: Inline srpt_get_ch_state() Bart Van Assche
                     ` (12 subsequent siblings)
  14 siblings, 1 reply; 57+ messages in thread
From: Bart Van Assche @ 2016-01-05 14:21 UTC (permalink / raw)
  To: Doug Ledford; +Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

srpt_sdev_name() is too trivial to keep it as a separate function.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index fe9474f..7abad26 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -109,16 +109,6 @@ enum dma_data_direction opposite_dma_dir(enum dma_data_direction dir)
 	}
 }
 
-/**
- * srpt_sdev_name() - Return the name associated with the HCA.
- *
- * Examples are ib0, ib1, ...
- */
-static inline const char *srpt_sdev_name(struct srpt_device *sdev)
-{
-	return sdev->device->name;
-}
-
 static enum rdma_ch_state srpt_get_ch_state(struct srpt_rdma_ch *ch)
 {
 	unsigned long flags;
@@ -182,7 +172,7 @@ static void srpt_event_handler(struct ib_event_handler *handler,
 		return;
 
 	pr_debug("ASYNC event= %d on device= %s\n", event->event,
-		 srpt_sdev_name(sdev));
+		 sdev->device->name);
 
 	switch (event->event) {
 	case IB_EVENT_PORT_ERR:
@@ -3089,7 +3079,7 @@ static void srpt_add_one(struct ib_device *device)
 
 		if (srpt_refresh_port(sport)) {
 			pr_err("MAD registration failed for %s-%d.\n",
-			       srpt_sdev_name(sdev), i);
+			       sdev->device->name, i);
 			goto err_ring;
 		}
 		snprintf(sport->port_guid, sizeof(sport->port_guid),
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH 03/15] IB/srpt: Inline srpt_get_ch_state()
       [not found] ` <568BD0FC.70207-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-05 14:20   ` [PATCH 01/15] IB/srpt: Add parentheses around sizeof argument Bart Van Assche
  2016-01-05 14:21   ` [PATCH 02/15] IB/srpt: Inline srpt_sdev_name() Bart Van Assche
@ 2016-01-05 14:21   ` Bart Van Assche
       [not found]     ` <568BD181.5040009-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-05 14:22   ` [PATCH 04/15] IB/srpt: Introduce target_reverse_dma_direction() Bart Van Assche
                     ` (11 subsequent siblings)
  14 siblings, 1 reply; 57+ messages in thread
From: Bart Van Assche @ 2016-01-05 14:21 UTC (permalink / raw)
  To: Doug Ledford; +Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

The callers of srpt_get_ch_state() can access ch->state safely without
using locking. Hence inline this function.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 42 ++++++++++-------------------------
 1 file changed, 12 insertions(+), 30 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 7abad26..cbcc73e 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -109,17 +109,6 @@ enum dma_data_direction opposite_dma_dir(enum dma_data_direction dir)
 	}
 }
 
-static enum rdma_ch_state srpt_get_ch_state(struct srpt_rdma_ch *ch)
-{
-	unsigned long flags;
-	enum rdma_ch_state state;
-
-	spin_lock_irqsave(&ch->spinlock, flags);
-	state = ch->state;
-	spin_unlock_irqrestore(&ch->spinlock, flags);
-	return state;
-}
-
 static enum rdma_ch_state
 srpt_set_ch_state(struct srpt_rdma_ch *ch, enum rdma_ch_state new_state)
 {
@@ -216,7 +205,7 @@ static void srpt_srq_event(struct ib_event *event, void *ctx)
 static void srpt_qp_event(struct ib_event *event, struct srpt_rdma_ch *ch)
 {
 	pr_debug("QP event %d on cm_id=%p sess_name=%s state=%d\n",
-		 event->event, ch->cm_id, ch->sess_name, srpt_get_ch_state(ch));
+		 event->event, ch->cm_id, ch->sess_name, ch->state);
 
 	switch (event->event) {
 	case IB_EVENT_COMM_EST:
@@ -228,7 +217,7 @@ static void srpt_qp_event(struct ib_event *event, struct srpt_rdma_ch *ch)
 			srpt_release_channel(ch);
 		else
 			pr_debug("%s: state %d - ignored LAST_WQE.\n",
-				 ch->sess_name, srpt_get_ch_state(ch));
+				 ch->sess_name, ch->state);
 		break;
 	default:
 		pr_err("received unrecognized IB QP event %d\n", event->event);
@@ -1784,7 +1773,6 @@ static void srpt_handle_new_iu(struct srpt_rdma_ch *ch,
 			       struct srpt_send_ioctx *send_ioctx)
 {
 	struct srp_cmd *srp_cmd;
-	enum rdma_ch_state ch_state;
 
 	BUG_ON(!ch);
 	BUG_ON(!recv_ioctx);
@@ -1793,13 +1781,12 @@ static void srpt_handle_new_iu(struct srpt_rdma_ch *ch,
 				   recv_ioctx->ioctx.dma, srp_max_req_size,
 				   DMA_FROM_DEVICE);
 
-	ch_state = srpt_get_ch_state(ch);
-	if (unlikely(ch_state == CH_CONNECTING)) {
+	if (unlikely(ch->state == CH_CONNECTING)) {
 		list_add_tail(&recv_ioctx->wait_list, &ch->cmd_wait_list);
 		goto out;
 	}
 
-	if (unlikely(ch_state != CH_LIVE))
+	if (unlikely(ch->state != CH_LIVE))
 		goto out;
 
 	srp_cmd = recv_ioctx->ioctx.buf;
@@ -1908,7 +1895,7 @@ static void srpt_send_done(struct ib_cq *cq, struct ib_wc *wc)
 
 out:
 	while (!list_empty(&ch->cmd_wait_list) &&
-	       srpt_get_ch_state(ch) == CH_LIVE &&
+	       ch->state == CH_LIVE &&
 	       (ioctx = srpt_get_send_ioctx(ch)) != NULL) {
 		struct srpt_recv_ioctx *recv_ioctx;
 
@@ -2314,17 +2301,14 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 			    && param->port == ch->sport->port
 			    && param->listen_id == ch->sport->sdev->cm_id
 			    && ch->cm_id) {
-				enum rdma_ch_state ch_state;
-
-				ch_state = srpt_get_ch_state(ch);
-				if (ch_state != CH_CONNECTING
-				    && ch_state != CH_LIVE)
+				if (ch->state != CH_CONNECTING
+				    && ch->state != CH_LIVE)
 					continue;
 
 				/* found an existing channel */
 				pr_debug("Found existing channel %s"
 					 " cm_id= %p state= %d\n",
-					 ch->sess_name, ch->cm_id, ch_state);
+					 ch->sess_name, ch->cm_id, ch->state);
 
 				__srpt_close_ch(ch);
 
@@ -2566,7 +2550,7 @@ static void srpt_cm_dreq_recv(struct ib_cm_id *cm_id)
 	ch = srpt_find_channel(cm_id->context, cm_id);
 	BUG_ON(!ch);
 
-	pr_debug("cm_id= %p ch->state= %d\n", cm_id, srpt_get_ch_state(ch));
+	pr_debug("cm_id= %p ch->state= %d\n", cm_id, ch->state);
 
 	spin_lock_irqsave(&ch->spinlock, flags);
 	switch (ch->state) {
@@ -2750,7 +2734,6 @@ static int srpt_write_pending(struct se_cmd *se_cmd)
 	struct srpt_rdma_ch *ch;
 	struct srpt_send_ioctx *ioctx;
 	enum srpt_command_state new_state;
-	enum rdma_ch_state ch_state;
 	int ret;
 
 	ioctx = container_of(se_cmd, struct srpt_send_ioctx, cmd);
@@ -2761,10 +2744,9 @@ static int srpt_write_pending(struct se_cmd *se_cmd)
 	ch = ioctx->ch;
 	BUG_ON(!ch);
 
-	ch_state = srpt_get_ch_state(ch);
-	switch (ch_state) {
+	switch (ch->state) {
 	case CH_CONNECTING:
-		WARN(true, "unexpected channel state %d\n", ch_state);
+		WARN(true, "unexpected channel state %d\n", ch->state);
 		ret = -EINVAL;
 		goto out;
 	case CH_LIVE:
@@ -3235,7 +3217,7 @@ static void srpt_close_session(struct se_session *se_sess)
 	ch = se_sess->fabric_sess_ptr;
 	WARN_ON(ch->sess != se_sess);
 
-	pr_debug("ch %p state %d\n", ch, srpt_get_ch_state(ch));
+	pr_debug("ch %p state %d\n", ch, ch->state);
 
 	sdev = ch->sport->sdev;
 	spin_lock_irq(&sdev->spinlock);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH 04/15] IB/srpt: Introduce target_reverse_dma_direction()
       [not found] ` <568BD0FC.70207-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
                     ` (2 preceding siblings ...)
  2016-01-05 14:21   ` [PATCH 03/15] IB/srpt: Inline srpt_get_ch_state() Bart Van Assche
@ 2016-01-05 14:22   ` Bart Van Assche
       [not found]     ` <568BD19C.6060404-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-05 14:22   ` [PATCH 05/15] IB/srpt: Use scsilun_to_int() Bart Van Assche
                     ` (10 subsequent siblings)
  14 siblings, 1 reply; 57+ messages in thread
From: Bart Van Assche @ 2016-01-05 14:22 UTC (permalink / raw)
  To: Doug Ledford; +Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

Use the function target_reverse_dma_direction() instead of
reimplementing it.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index cbcc73e..fd94780 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -96,19 +96,6 @@ static int srpt_queue_status(struct se_cmd *cmd);
 static void srpt_recv_done(struct ib_cq *cq, struct ib_wc *wc);
 static void srpt_send_done(struct ib_cq *cq, struct ib_wc *wc);
 
-/**
- * opposite_dma_dir() - Swap DMA_TO_DEVICE and DMA_FROM_DEVICE.
- */
-static inline
-enum dma_data_direction opposite_dma_dir(enum dma_data_direction dir)
-{
-	switch (dir) {
-	case DMA_TO_DEVICE:	return DMA_FROM_DEVICE;
-	case DMA_FROM_DEVICE:	return DMA_TO_DEVICE;
-	default:		return dir;
-	}
-}
-
 static enum rdma_ch_state
 srpt_set_ch_state(struct srpt_rdma_ch *ch, enum rdma_ch_state new_state)
 {
@@ -1048,7 +1035,7 @@ static void srpt_unmap_sg_to_ib_sge(struct srpt_rdma_ch *ch,
 		dir = ioctx->cmd.data_direction;
 		BUG_ON(dir == DMA_NONE);
 		ib_dma_unmap_sg(ch->sport->sdev->device, sg, ioctx->sg_cnt,
-				opposite_dma_dir(dir));
+				target_reverse_dma_direction(&ioctx->cmd));
 		ioctx->mapped_sg_count = 0;
 	}
 }
@@ -1085,7 +1072,7 @@ static int srpt_map_sg_to_ib_sge(struct srpt_rdma_ch *ch,
 	ioctx->sg_cnt = sg_cnt = cmd->t_data_nents;
 
 	count = ib_dma_map_sg(ch->sport->sdev->device, sg, sg_cnt,
-			      opposite_dma_dir(dir));
+			      target_reverse_dma_direction(cmd));
 	if (unlikely(!count))
 		return -EAGAIN;
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH 05/15] IB/srpt: Use scsilun_to_int()
       [not found] ` <568BD0FC.70207-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
                     ` (3 preceding siblings ...)
  2016-01-05 14:22   ` [PATCH 04/15] IB/srpt: Introduce target_reverse_dma_direction() Bart Van Assche
@ 2016-01-05 14:22   ` Bart Van Assche
       [not found]     ` <568BD1B6.4090308-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-05 14:23   ` [PATCH 06/15] IB/srpt: Simplify srpt_handle_tsk_mgmt() Bart Van Assche
                     ` (9 subsequent siblings)
  14 siblings, 1 reply; 57+ messages in thread
From: Bart Van Assche @ 2016-01-05 14:22 UTC (permalink / raw)
  To: Doug Ledford; +Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

Just like other target drivers, use scsilun_to_int() to unpack SCSI
LUN numbers. This patch only changes the behavior of ib_srpt for LUN
numbers >= 16384.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 92 +++--------------------------------
 1 file changed, 6 insertions(+), 86 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index fd94780..fc19203 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1488,80 +1488,6 @@ static int srpt_build_tskmgmt_rsp(struct srpt_rdma_ch *ch,
 	return resp_len;
 }
 
-#define NO_SUCH_LUN ((uint64_t)-1LL)
-
-/*
- * SCSI LUN addressing method. See also SAM-2 and the section about
- * eight byte LUNs.
- */
-enum scsi_lun_addr_method {
-	SCSI_LUN_ADDR_METHOD_PERIPHERAL   = 0,
-	SCSI_LUN_ADDR_METHOD_FLAT         = 1,
-	SCSI_LUN_ADDR_METHOD_LUN          = 2,
-	SCSI_LUN_ADDR_METHOD_EXTENDED_LUN = 3,
-};
-
-/*
- * srpt_unpack_lun() - Convert from network LUN to linear LUN.
- *
- * Convert an 2-byte, 4-byte, 6-byte or 8-byte LUN structure in network byte
- * order (big endian) to a linear LUN. Supports three LUN addressing methods:
- * peripheral, flat and logical unit. See also SAM-2, section 4.9.4 (page 40).
- */
-static uint64_t srpt_unpack_lun(const uint8_t *lun, int len)
-{
-	uint64_t res = NO_SUCH_LUN;
-	int addressing_method;
-
-	if (unlikely(len < 2)) {
-		pr_err("Illegal LUN length %d, expected 2 bytes or more\n",
-		       len);
-		goto out;
-	}
-
-	switch (len) {
-	case 8:
-		if ((*((__be64 *)lun) &
-		     cpu_to_be64(0x0000FFFFFFFFFFFFLL)) != 0)
-			goto out_err;
-		break;
-	case 4:
-		if (*((__be16 *)&lun[2]) != 0)
-			goto out_err;
-		break;
-	case 6:
-		if (*((__be32 *)&lun[2]) != 0)
-			goto out_err;
-		break;
-	case 2:
-		break;
-	default:
-		goto out_err;
-	}
-
-	addressing_method = (*lun) >> 6; /* highest two bits of byte 0 */
-	switch (addressing_method) {
-	case SCSI_LUN_ADDR_METHOD_PERIPHERAL:
-	case SCSI_LUN_ADDR_METHOD_FLAT:
-	case SCSI_LUN_ADDR_METHOD_LUN:
-		res = *(lun + 1) | (((*lun) & 0x3f) << 8);
-		break;
-
-	case SCSI_LUN_ADDR_METHOD_EXTENDED_LUN:
-	default:
-		pr_err("Unimplemented LUN addressing method %u\n",
-		       addressing_method);
-		break;
-	}
-
-out:
-	return res;
-
-out_err:
-	pr_err("Support for multi-level LUNs has not yet been implemented\n");
-	goto out;
-}
-
 static int srpt_check_stop_free(struct se_cmd *cmd)
 {
 	struct srpt_send_ioctx *ioctx = container_of(cmd,
@@ -1579,7 +1505,6 @@ static int srpt_handle_cmd(struct srpt_rdma_ch *ch,
 {
 	struct se_cmd *cmd;
 	struct srp_cmd *srp_cmd;
-	uint64_t unpacked_lun;
 	u64 data_len;
 	enum dma_data_direction dir;
 	sense_reason_t ret;
@@ -1614,11 +1539,10 @@ static int srpt_handle_cmd(struct srpt_rdma_ch *ch,
 		goto send_sense;
 	}
 
-	unpacked_lun = srpt_unpack_lun((uint8_t *)&srp_cmd->lun,
-				       sizeof(srp_cmd->lun));
 	rc = target_submit_cmd(cmd, ch->sess, srp_cmd->cdb,
-			&send_ioctx->sense_data[0], unpacked_lun, data_len,
-			TCM_SIMPLE_TAG, dir, TARGET_SCF_ACK_KREF);
+			       &send_ioctx->sense_data[0],
+			       scsilun_to_int(&srp_cmd->lun), data_len,
+			       TCM_SIMPLE_TAG, dir, TARGET_SCF_ACK_KREF);
 	if (rc != 0) {
 		ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 		goto send_sense;
@@ -1704,7 +1628,6 @@ static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,
 	struct srp_tsk_mgmt *srp_tsk;
 	struct se_cmd *cmd;
 	struct se_session *sess = ch->sess;
-	uint64_t unpacked_lun;
 	uint32_t tag = 0;
 	int tcm_tmr;
 	int rc;
@@ -1726,9 +1649,6 @@ static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,
 			TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED;
 		goto fail;
 	}
-	unpacked_lun = srpt_unpack_lun((uint8_t *)&srp_tsk->lun,
-				       sizeof(srp_tsk->lun));
-
 	if (srp_tsk->tsk_mgmt_func == SRP_TSK_ABORT_TASK) {
 		rc = srpt_rx_mgmt_fn_tag(send_ioctx, srp_tsk->task_tag);
 		if (rc < 0) {
@@ -1738,9 +1658,9 @@ static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,
 		}
 		tag = srp_tsk->task_tag;
 	}
-	rc = target_submit_tmr(&send_ioctx->cmd, sess, NULL, unpacked_lun,
-				srp_tsk, tcm_tmr, GFP_KERNEL, tag,
-				TARGET_SCF_ACK_KREF);
+	rc = target_submit_tmr(&send_ioctx->cmd, sess, NULL,
+			       scsilun_to_int(&srp_tsk->lun), srp_tsk, tcm_tmr,
+			       GFP_KERNEL, tag, TARGET_SCF_ACK_KREF);
 	if (rc != 0) {
 		send_ioctx->cmd.se_tmr_req->response = TMR_FUNCTION_REJECTED;
 		goto fail;
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH 06/15] IB/srpt: Simplify srpt_handle_tsk_mgmt()
       [not found] ` <568BD0FC.70207-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
                     ` (4 preceding siblings ...)
  2016-01-05 14:22   ` [PATCH 05/15] IB/srpt: Use scsilun_to_int() Bart Van Assche
@ 2016-01-05 14:23   ` Bart Van Assche
       [not found]     ` <568BD1D2.1030609-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-05 14:23   ` [PATCH 07/15] IB/srpt: Simplify channel state management Bart Van Assche
                     ` (8 subsequent siblings)
  14 siblings, 1 reply; 57+ messages in thread
From: Bart Van Assche @ 2016-01-05 14:23 UTC (permalink / raw)
  To: Doug Ledford; +Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

Let the target core check task existence instead of the SRP target
driver.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 54 ++---------------------------------
 1 file changed, 2 insertions(+), 52 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index fc19203..9cb1a14 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1554,47 +1554,6 @@ send_sense:
 	return -1;
 }
 
-/**
- * srpt_rx_mgmt_fn_tag() - Process a task management function by tag.
- * @ch: RDMA channel of the task management request.
- * @fn: Task management function to perform.
- * @req_tag: Tag of the SRP task management request.
- * @mgmt_ioctx: I/O context of the task management request.
- *
- * Returns zero if the target core will process the task management
- * request asynchronously.
- *
- * Note: It is assumed that the initiator serializes tag-based task management
- * requests.
- */
-static int srpt_rx_mgmt_fn_tag(struct srpt_send_ioctx *ioctx, u64 tag)
-{
-	struct srpt_device *sdev;
-	struct srpt_rdma_ch *ch;
-	struct srpt_send_ioctx *target;
-	int ret, i;
-
-	ret = -EINVAL;
-	ch = ioctx->ch;
-	BUG_ON(!ch);
-	BUG_ON(!ch->sport);
-	sdev = ch->sport->sdev;
-	BUG_ON(!sdev);
-	spin_lock_irq(&sdev->spinlock);
-	for (i = 0; i < ch->rq_size; ++i) {
-		target = ch->ioctx_ring[i];
-		if (target->cmd.se_lun == ioctx->cmd.se_lun &&
-		    target->cmd.tag == tag &&
-		    srpt_get_cmd_state(target) != SRPT_STATE_DONE) {
-			ret = 0;
-			/* now let the target core abort &target->cmd; */
-			break;
-		}
-	}
-	spin_unlock_irq(&sdev->spinlock);
-	return ret;
-}
-
 static int srp_tmr_to_tcm(int fn)
 {
 	switch (fn) {
@@ -1628,7 +1587,6 @@ static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,
 	struct srp_tsk_mgmt *srp_tsk;
 	struct se_cmd *cmd;
 	struct se_session *sess = ch->sess;
-	uint32_t tag = 0;
 	int tcm_tmr;
 	int rc;
 
@@ -1649,18 +1607,10 @@ static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,
 			TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED;
 		goto fail;
 	}
-	if (srp_tsk->tsk_mgmt_func == SRP_TSK_ABORT_TASK) {
-		rc = srpt_rx_mgmt_fn_tag(send_ioctx, srp_tsk->task_tag);
-		if (rc < 0) {
-			send_ioctx->cmd.se_tmr_req->response =
-					TMR_TASK_DOES_NOT_EXIST;
-			goto fail;
-		}
-		tag = srp_tsk->task_tag;
-	}
 	rc = target_submit_tmr(&send_ioctx->cmd, sess, NULL,
 			       scsilun_to_int(&srp_tsk->lun), srp_tsk, tcm_tmr,
-			       GFP_KERNEL, tag, TARGET_SCF_ACK_KREF);
+			       GFP_KERNEL, srp_tsk->task_tag,
+			       TARGET_SCF_ACK_KREF);
 	if (rc != 0) {
 		send_ioctx->cmd.se_tmr_req->response = TMR_FUNCTION_REJECTED;
 		goto fail;
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH 07/15] IB/srpt: Simplify channel state management
       [not found] ` <568BD0FC.70207-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
                     ` (5 preceding siblings ...)
  2016-01-05 14:23   ` [PATCH 06/15] IB/srpt: Simplify srpt_handle_tsk_mgmt() Bart Van Assche
@ 2016-01-05 14:23   ` Bart Van Assche
       [not found]     ` <568BD1F1.5050107-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-05 14:24   ` [PATCH 08/15] IB/srpt: Simplify srpt_shutdown_session() Bart Van Assche
                     ` (7 subsequent siblings)
  14 siblings, 1 reply; 57+ messages in thread
From: Bart Van Assche @ 2016-01-05 14:23 UTC (permalink / raw)
  To: Doug Ledford; +Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

The only allowed channel state changes are those that change
the channel state into a state with a higher numerical value.
This allows to merge the functions srpt_set_ch_state() and
srpt_test_and_set_ch_state() into a single function.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 42 +++++++++--------------------------
 1 file changed, 10 insertions(+), 32 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 9cb1a14..a27414d 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -96,37 +96,21 @@ static int srpt_queue_status(struct se_cmd *cmd);
 static void srpt_recv_done(struct ib_cq *cq, struct ib_wc *wc);
 static void srpt_send_done(struct ib_cq *cq, struct ib_wc *wc);
 
-static enum rdma_ch_state
-srpt_set_ch_state(struct srpt_rdma_ch *ch, enum rdma_ch_state new_state)
+static bool srpt_set_ch_state(struct srpt_rdma_ch *ch, enum rdma_ch_state new)
 {
 	unsigned long flags;
 	enum rdma_ch_state prev;
+	bool changed = false;
 
 	spin_lock_irqsave(&ch->spinlock, flags);
 	prev = ch->state;
-	ch->state = new_state;
-	spin_unlock_irqrestore(&ch->spinlock, flags);
-	return prev;
-}
-
-/**
- * srpt_test_and_set_ch_state() - Test and set the channel state.
- *
- * Returns true if and only if the channel state has been set to the new state.
- */
-static bool
-srpt_test_and_set_ch_state(struct srpt_rdma_ch *ch, enum rdma_ch_state old,
-			   enum rdma_ch_state new)
-{
-	unsigned long flags;
-	enum rdma_ch_state prev;
-
-	spin_lock_irqsave(&ch->spinlock, flags);
-	prev = ch->state;
-	if (prev == old)
+	if (new > prev) {
 		ch->state = new;
+		changed = true;
+	}
 	spin_unlock_irqrestore(&ch->spinlock, flags);
-	return prev == old;
+
+	return changed;
 }
 
 /**
@@ -199,8 +183,7 @@ static void srpt_qp_event(struct ib_event *event, struct srpt_rdma_ch *ch)
 		ib_cm_notify(ch->cm_id, event->event);
 		break;
 	case IB_EVENT_QP_LAST_WQE_REACHED:
-		if (srpt_test_and_set_ch_state(ch, CH_DRAINING,
-					       CH_RELEASING))
+		if (srpt_set_ch_state(ch, CH_RELEASING))
 			srpt_release_channel(ch);
 		else
 			pr_debug("%s: state %d - ignored LAST_WQE.\n",
@@ -1946,12 +1929,7 @@ static void srpt_drain_channel(struct ib_cm_id *cm_id)
 	spin_lock_irq(&sdev->spinlock);
 	list_for_each_entry(ch, &sdev->rch_list, list) {
 		if (ch->cm_id == cm_id) {
-			do_reset = srpt_test_and_set_ch_state(ch,
-					CH_CONNECTING, CH_DRAINING) ||
-				   srpt_test_and_set_ch_state(ch,
-					CH_LIVE, CH_DRAINING) ||
-				   srpt_test_and_set_ch_state(ch,
-					CH_DISCONNECTING, CH_DRAINING);
+			do_reset = srpt_set_ch_state(ch, CH_DRAINING);
 			break;
 		}
 	}
@@ -2368,7 +2346,7 @@ static void srpt_cm_rtu_recv(struct ib_cm_id *cm_id)
 	ch = srpt_find_channel(cm_id->context, cm_id);
 	BUG_ON(!ch);
 
-	if (srpt_test_and_set_ch_state(ch, CH_CONNECTING, CH_LIVE)) {
+	if (srpt_set_ch_state(ch, CH_LIVE)) {
 		struct srpt_recv_ioctx *ioctx, *ioctx_tmp;
 
 		ret = srpt_ch_qp_rts(ch, ch->qp);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH 08/15] IB/srpt: Simplify srpt_shutdown_session()
       [not found] ` <568BD0FC.70207-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
                     ` (6 preceding siblings ...)
  2016-01-05 14:23   ` [PATCH 07/15] IB/srpt: Simplify channel state management Bart Van Assche
@ 2016-01-05 14:24   ` Bart Van Assche
       [not found]     ` <568BD20F.7020101-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-05 14:24   ` [PATCH 09/15] IB/srpt: Fix srpt_close_session() Bart Van Assche
                     ` (6 subsequent siblings)
  14 siblings, 1 reply; 57+ messages in thread
From: Bart Van Assche @ 2016-01-05 14:24 UTC (permalink / raw)
  To: Doug Ledford; +Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

The target core guarantees that shutdown_session() is only invoked
once per session. This means that the ib_srpt target driver doesn't
have to track whether or not shutdown_session() has been called.
Additionally, ensure that target_sess_cmd_list_set_waiting() is
called before target_wait_for_sess_cmds() by moving it into
srpt_release_channel_work().

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 14 +-------------
 drivers/infiniband/ulp/srpt/ib_srpt.h |  1 -
 2 files changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index a27414d..0ff4ed6 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1887,19 +1887,6 @@ static void srpt_close_ch(struct srpt_rdma_ch *ch)
  */
 static int srpt_shutdown_session(struct se_session *se_sess)
 {
-	struct srpt_rdma_ch *ch = se_sess->fabric_sess_ptr;
-	unsigned long flags;
-
-	spin_lock_irqsave(&ch->spinlock, flags);
-	if (ch->in_shutdown) {
-		spin_unlock_irqrestore(&ch->spinlock, flags);
-		return true;
-	}
-
-	ch->in_shutdown = true;
-	target_sess_cmd_list_set_waiting(se_sess);
-	spin_unlock_irqrestore(&ch->spinlock, flags);
-
 	return true;
 }
 
@@ -2003,6 +1990,7 @@ static void srpt_release_channel_work(struct work_struct *w)
 	se_sess = ch->sess;
 	BUG_ON(!se_sess);
 
+	target_sess_cmd_list_set_waiting(se_sess);
 	target_wait_for_sess_cmds(se_sess);
 
 	transport_deregister_session_configfs(se_sess);
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h
index a98b86b..d0de468 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.h
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.h
@@ -288,7 +288,6 @@ struct srpt_rdma_ch {
 	u8			sess_name[36];
 	struct work_struct	release_work;
 	struct completion	*release_done;
-	bool			in_shutdown;
 };
 
 /**
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH 09/15] IB/srpt: Fix srpt_close_session()
       [not found] ` <568BD0FC.70207-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
                     ` (7 preceding siblings ...)
  2016-01-05 14:24   ` [PATCH 08/15] IB/srpt: Simplify srpt_shutdown_session() Bart Van Assche
@ 2016-01-05 14:24   ` Bart Van Assche
       [not found]     ` <568BD231.10205-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-05 14:25   ` [PATCH 10/15] IB/srpt: Fix srpt_handle_cmd() error paths Bart Van Assche
                     ` (5 subsequent siblings)
  14 siblings, 1 reply; 57+ messages in thread
From: Bart Van Assche @ 2016-01-05 14:24 UTC (permalink / raw)
  To: Doug Ledford; +Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

Avoid that srpt_close_session() waits if it doesn't have to wait.
Additionally, increase the time during which srpt_close_session()
waits until closing a session has finished. This makes it easier
to detect session shutdown bugs.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 0ff4ed6..1857d17 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1981,8 +1981,8 @@ static void srpt_release_channel_work(struct work_struct *w)
 	struct se_session *se_sess;
 
 	ch = container_of(w, struct srpt_rdma_ch, release_work);
-	pr_debug("ch = %p; ch->sess = %p; release_done = %p\n", ch, ch->sess,
-		 ch->release_done);
+	pr_debug("%s: %s-%d; release_done = %p\n", __func__, ch->sess_name,
+		 ch->qp->qp_num, ch->release_done);
 
 	sdev = ch->sport->sdev;
 	BUG_ON(!sdev);
@@ -2006,11 +2006,10 @@ static void srpt_release_channel_work(struct work_struct *w)
 			     ch->rsp_size, DMA_TO_DEVICE);
 
 	spin_lock_irq(&sdev->spinlock);
-	list_del(&ch->list);
-	spin_unlock_irq(&sdev->spinlock);
-
+	list_del_init(&ch->list);
 	if (ch->release_done)
 		complete(ch->release_done);
+	spin_unlock_irq(&sdev->spinlock);
 
 	wake_up(&sdev->ch_releaseQ);
 
@@ -3033,24 +3032,26 @@ static void srpt_release_cmd(struct se_cmd *se_cmd)
 static void srpt_close_session(struct se_session *se_sess)
 {
 	DECLARE_COMPLETION_ONSTACK(release_done);
-	struct srpt_rdma_ch *ch;
-	struct srpt_device *sdev;
-	unsigned long res;
-
-	ch = se_sess->fabric_sess_ptr;
-	WARN_ON(ch->sess != se_sess);
+	struct srpt_rdma_ch *ch = se_sess->fabric_sess_ptr;
+	struct srpt_device *sdev = ch->sport->sdev;
+	bool wait;
 
-	pr_debug("ch %p state %d\n", ch, ch->state);
+	pr_debug("ch %s-%d state %d\n", ch->sess_name, ch->qp->qp_num,
+		 ch->state);
 
-	sdev = ch->sport->sdev;
 	spin_lock_irq(&sdev->spinlock);
 	BUG_ON(ch->release_done);
 	ch->release_done = &release_done;
+	wait = !list_empty(&ch->list);
 	__srpt_close_ch(ch);
 	spin_unlock_irq(&sdev->spinlock);
 
-	res = wait_for_completion_timeout(&release_done, 60 * HZ);
-	WARN_ON(res == 0);
+	if (!wait)
+		return;
+
+	while (wait_for_completion_timeout(&release_done, 180 * HZ) == 0)
+		pr_info("%s(%s-%d state %d): still waiting ...\n", __func__,
+			ch->sess_name, ch->qp->qp_num, ch->state);
 }
 
 /**
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH 10/15] IB/srpt: Fix srpt_handle_cmd() error paths
       [not found] ` <568BD0FC.70207-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
                     ` (8 preceding siblings ...)
  2016-01-05 14:24   ` [PATCH 09/15] IB/srpt: Fix srpt_close_session() Bart Van Assche
@ 2016-01-05 14:25   ` Bart Van Assche
       [not found]     ` <568BD249.1000100-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-05 14:25   ` [PATCH 11/15] IB/srpt: Fix how aborted commands are processed Bart Van Assche
                     ` (4 subsequent siblings)
  14 siblings, 1 reply; 57+ messages in thread
From: Bart Van Assche @ 2016-01-05 14:25 UTC (permalink / raw)
  To: Doug Ledford; +Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

The target core function that should be called if target_submit_cmd()
fails is target_put_sess_cmd(). Additionally, change the return type
of srpt_handle_cmd() from int into void.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 1857d17..395bc1b 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1482,15 +1482,14 @@ static int srpt_check_stop_free(struct se_cmd *cmd)
 /**
  * srpt_handle_cmd() - Process SRP_CMD.
  */
-static int srpt_handle_cmd(struct srpt_rdma_ch *ch,
-			   struct srpt_recv_ioctx *recv_ioctx,
-			   struct srpt_send_ioctx *send_ioctx)
+static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
+			    struct srpt_recv_ioctx *recv_ioctx,
+			    struct srpt_send_ioctx *send_ioctx)
 {
 	struct se_cmd *cmd;
 	struct srp_cmd *srp_cmd;
 	u64 data_len;
 	enum dma_data_direction dir;
-	sense_reason_t ret;
 	int rc;
 
 	BUG_ON(!send_ioctx);
@@ -1518,8 +1517,7 @@ static int srpt_handle_cmd(struct srpt_rdma_ch *ch,
 	if (srpt_get_desc_tbl(send_ioctx, srp_cmd, &dir, &data_len)) {
 		pr_err("0x%llx: parsing SRP descriptor table failed.\n",
 		       srp_cmd->tag);
-		ret = TCM_INVALID_CDB_FIELD;
-		goto send_sense;
+		goto put;
 	}
 
 	rc = target_submit_cmd(cmd, ch->sess, srp_cmd->cdb,
@@ -1527,14 +1525,16 @@ static int srpt_handle_cmd(struct srpt_rdma_ch *ch,
 			       scsilun_to_int(&srp_cmd->lun), data_len,
 			       TCM_SIMPLE_TAG, dir, TARGET_SCF_ACK_KREF);
 	if (rc != 0) {
-		ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
-		goto send_sense;
+		pr_debug("target_submit_cmd() returned %d for tag %#llx\n", rc,
+			 srp_cmd->tag);
+		goto put;
 	}
-	return 0;
+	return;
 
-send_sense:
-	transport_send_check_condition_and_sense(cmd, ret, 0);
-	return -1;
+put:
+	send_ioctx->state = SRPT_STATE_DONE;
+	target_put_sess_cmd(cmd);
+	return;
 }
 
 static int srp_tmr_to_tcm(int fn)
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH 11/15] IB/srpt: Fix how aborted commands are processed
       [not found] ` <568BD0FC.70207-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
                     ` (9 preceding siblings ...)
  2016-01-05 14:25   ` [PATCH 10/15] IB/srpt: Fix srpt_handle_cmd() error paths Bart Van Assche
@ 2016-01-05 14:25   ` Bart Van Assche
       [not found]     ` <568BD26D.9080003-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-05 14:26   ` [PATCH 12/15] IB/srpt: Eliminate srpt_find_channel() Bart Van Assche
                     ` (3 subsequent siblings)
  14 siblings, 1 reply; 57+ messages in thread
From: Bart Van Assche @ 2016-01-05 14:25 UTC (permalink / raw)
  To: Doug Ledford; +Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

srpt_abort_cmd() must not be called in state SRPT_STATE_DATA_IN. Issue
a warning if this occurs.

srpt_abort_cmd() must not invoke target_put_sess_cmd() for commands
in state SRPT_STATE_DONE because the srpt_abort_cmd() callers already
do this when necessary. Hence remove this call.

If an RDMA read fails the corresponding SCSI command must fail. Hence
add a transport_generic_request_failure() call.

Remove an incorrect srpt_abort_cmd() call from srpt_rdma_write_done().

Avoid that srpt_send_done() calls srpt_abort_cmd() for finished SCSI
commands.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 39 ++++++++++++++---------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 395bc1b..2ae3c1b 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1273,25 +1273,17 @@ static int srpt_abort_cmd(struct srpt_send_ioctx *ioctx)
 	case SRPT_STATE_NEED_DATA:
 		ioctx->state = SRPT_STATE_DATA_IN;
 		break;
-	case SRPT_STATE_DATA_IN:
 	case SRPT_STATE_CMD_RSP_SENT:
 	case SRPT_STATE_MGMT_RSP_SENT:
 		ioctx->state = SRPT_STATE_DONE;
 		break;
 	default:
+		WARN_ONCE(true, "%s: unexpected I/O context state %d\n",
+			  __func__, state);
 		break;
 	}
 	spin_unlock_irqrestore(&ioctx->spinlock, flags);
 
-	if (state == SRPT_STATE_DONE) {
-		struct srpt_rdma_ch *ch = ioctx->ch;
-
-		BUG_ON(ch->sess == NULL);
-
-		target_put_sess_cmd(&ioctx->cmd);
-		goto out;
-	}
-
 	pr_debug("Aborting cmd with state %d and tag %lld\n", state,
 		 ioctx->cmd.tag);
 
@@ -1299,14 +1291,16 @@ static int srpt_abort_cmd(struct srpt_send_ioctx *ioctx)
 	case SRPT_STATE_NEW:
 	case SRPT_STATE_DATA_IN:
 	case SRPT_STATE_MGMT:
+	case SRPT_STATE_DONE:
 		/*
 		 * Do nothing - defer abort processing until
 		 * srpt_queue_response() is invoked.
 		 */
-		WARN_ON(!(ioctx->cmd.transport_state & CMD_T_ABORTED));
 		break;
 	case SRPT_STATE_NEED_DATA:
-		/* DMA_TO_DEVICE (write) - RDMA read error. */
+		pr_debug("tag %#llx: RDMA read error\n", ioctx->cmd.tag);
+		transport_generic_request_failure(&ioctx->cmd,
+					TCM_CHECK_CONDITION_ABORT_CMD);
 		break;
 	case SRPT_STATE_CMD_RSP_SENT:
 		/*
@@ -1314,18 +1308,16 @@ static int srpt_abort_cmd(struct srpt_send_ioctx *ioctx)
 		 * not been received in time.
 		 */
 		srpt_unmap_sg_to_ib_sge(ioctx->ch, ioctx);
-		target_put_sess_cmd(&ioctx->cmd);
+		transport_generic_free_cmd(&ioctx->cmd, 0);
 		break;
 	case SRPT_STATE_MGMT_RSP_SENT:
-		srpt_set_cmd_state(ioctx, SRPT_STATE_DONE);
-		target_put_sess_cmd(&ioctx->cmd);
+		transport_generic_free_cmd(&ioctx->cmd, 0);
 		break;
 	default:
 		WARN(1, "Unexpected command state (%d)", state);
 		break;
 	}
 
-out:
 	return state;
 }
 
@@ -1365,9 +1357,14 @@ static void srpt_rdma_write_done(struct ib_cq *cq, struct ib_wc *wc)
 		container_of(wc->wr_cqe, struct srpt_send_ioctx, rdma_cqe);
 
 	if (unlikely(wc->status != IB_WC_SUCCESS)) {
+		/*
+		 * Note: if an RDMA write error completion is received that
+		 * means that a SEND also has been posted. Defer further
+		 * processing of the associated command until the send error
+		 * completion has been received.
+		 */
 		pr_info("RDMA_WRITE for ioctx 0x%p failed with status %d\n",
 			ioctx, wc->status);
-		srpt_abort_cmd(ioctx);
 	}
 }
 
@@ -1716,15 +1713,10 @@ static void srpt_send_done(struct ib_cq *cq, struct ib_wc *wc)
 
 	atomic_inc(&ch->sq_wr_avail);
 
-	if (wc->status != IB_WC_SUCCESS) {
+	if (wc->status != IB_WC_SUCCESS)
 		pr_info("sending response for ioctx 0x%p failed"
 			" with status %d\n", ioctx, wc->status);
 
-		atomic_dec(&ch->req_lim);
-		srpt_abort_cmd(ioctx);
-		goto out;
-	}
-
 	if (state != SRPT_STATE_DONE) {
 		srpt_unmap_sg_to_ib_sge(ch, ioctx);
 		transport_generic_free_cmd(&ioctx->cmd, 0);
@@ -1733,7 +1725,6 @@ static void srpt_send_done(struct ib_cq *cq, struct ib_wc *wc)
 		       " wr_id = %u.\n", ioctx->ioctx.index);
 	}
 
-out:
 	while (!list_empty(&ch->cmd_wait_list) &&
 	       ch->state == CH_LIVE &&
 	       (ioctx = srpt_get_send_ioctx(ch)) != NULL) {
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH 12/15] IB/srpt: Eliminate srpt_find_channel()
       [not found] ` <568BD0FC.70207-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
                     ` (10 preceding siblings ...)
  2016-01-05 14:25   ` [PATCH 11/15] IB/srpt: Fix how aborted commands are processed Bart Van Assche
@ 2016-01-05 14:26   ` Bart Van Assche
       [not found]     ` <568BD28E.40302-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-05 14:26   ` [PATCH 13/15] IB/srpt: Detect session shutdown reliably Bart Van Assche
                     ` (2 subsequent siblings)
  14 siblings, 1 reply; 57+ messages in thread
From: Bart Van Assche @ 2016-01-05 14:26 UTC (permalink / raw)
  To: Doug Ledford; +Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

In the CM REQ message handler, store the channel pointer in
cm_id->context such that the function srpt_find_channel() is no
longer needed. Additionally, make the CM event messages more
informative.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 113 +++++++++++++---------------------
 1 file changed, 43 insertions(+), 70 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 2ae3c1b..6ec130d 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1893,25 +1893,14 @@ static int srpt_shutdown_session(struct se_session *se_sess)
  * ib_destroy_cm_id(), which locks the cm_id spinlock and hence waits until
  * this function has finished).
  */
-static void srpt_drain_channel(struct ib_cm_id *cm_id)
+static void srpt_drain_channel(struct srpt_rdma_ch *ch)
 {
-	struct srpt_device *sdev;
-	struct srpt_rdma_ch *ch;
 	int ret;
 	bool do_reset = false;
 
 	WARN_ON_ONCE(irqs_disabled());
 
-	sdev = cm_id->context;
-	BUG_ON(!sdev);
-	spin_lock_irq(&sdev->spinlock);
-	list_for_each_entry(ch, &sdev->rch_list, list) {
-		if (ch->cm_id == cm_id) {
-			do_reset = srpt_set_ch_state(ch, CH_DRAINING);
-			break;
-		}
-	}
-	spin_unlock_irq(&sdev->spinlock);
+	do_reset = srpt_set_ch_state(ch, CH_DRAINING);
 
 	if (do_reset) {
 		if (ch->sess)
@@ -1925,34 +1914,6 @@ static void srpt_drain_channel(struct ib_cm_id *cm_id)
 }
 
 /**
- * srpt_find_channel() - Look up an RDMA channel.
- * @cm_id: Pointer to the CM ID of the channel to be looked up.
- *
- * Return NULL if no matching RDMA channel has been found.
- */
-static struct srpt_rdma_ch *srpt_find_channel(struct srpt_device *sdev,
-					      struct ib_cm_id *cm_id)
-{
-	struct srpt_rdma_ch *ch;
-	bool found;
-
-	WARN_ON_ONCE(irqs_disabled());
-	BUG_ON(!sdev);
-
-	found = false;
-	spin_lock_irq(&sdev->spinlock);
-	list_for_each_entry(ch, &sdev->rch_list, list) {
-		if (ch->cm_id == cm_id) {
-			found = true;
-			break;
-		}
-	}
-	spin_unlock_irq(&sdev->spinlock);
-
-	return found ? ch : NULL;
-}
-
-/**
  * srpt_release_channel() - Release channel resources.
  *
  * Schedules the actual release because:
@@ -2160,6 +2121,7 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 	memcpy(ch->t_port_id, req->target_port_id, 16);
 	ch->sport = &sdev->port[param->port - 1];
 	ch->cm_id = cm_id;
+	cm_id->context = ch;
 	/*
 	 * Avoid QUEUE_FULL conditions by limiting the number of buffers used
 	 * for the SRP protocol to the command queue size.
@@ -2304,10 +2266,23 @@ out:
 	return ret;
 }
 
-static void srpt_cm_rej_recv(struct ib_cm_id *cm_id)
+static void srpt_cm_rej_recv(struct srpt_rdma_ch *ch,
+			     enum ib_cm_rej_reason reason,
+			     const u8 *private_data,
+			     u8 private_data_len)
 {
-	pr_info("Received IB REJ for cm_id %p.\n", cm_id);
-	srpt_drain_channel(cm_id);
+	char *priv = kmalloc(private_data_len * 3 + 1, GFP_KERNEL);
+	int i;
+
+	if (priv) {
+		priv[0] = '\0';
+		for (i = 0; i < private_data_len; i++)
+			sprintf(priv + 3 * i, "%02x ", private_data[i]);
+	}
+	pr_info("Received CM REJ for ch %s-%d; reason %d; private data %s.\n",
+		ch->sess_name, ch->qp->qp_num, reason, priv ? : "(?)");
+	kfree(priv);
+	srpt_drain_channel(ch);
 }
 
 /**
@@ -2316,14 +2291,10 @@ static void srpt_cm_rej_recv(struct ib_cm_id *cm_id)
  * An IB_CM_RTU_RECEIVED message indicates that the connection is established
  * and that the recipient may begin transmitting (RTU = ready to use).
  */
-static void srpt_cm_rtu_recv(struct ib_cm_id *cm_id)
+static void srpt_cm_rtu_recv(struct srpt_rdma_ch *ch)
 {
-	struct srpt_rdma_ch *ch;
 	int ret;
 
-	ch = srpt_find_channel(cm_id->context, cm_id);
-	BUG_ON(!ch);
-
 	if (srpt_set_ch_state(ch, CH_LIVE)) {
 		struct srpt_recv_ioctx *ioctx, *ioctx_tmp;
 
@@ -2339,31 +2310,30 @@ static void srpt_cm_rtu_recv(struct ib_cm_id *cm_id)
 	}
 }
 
-static void srpt_cm_timewait_exit(struct ib_cm_id *cm_id)
+static void srpt_cm_timewait_exit(struct srpt_rdma_ch *ch)
 {
-	pr_info("Received IB TimeWait exit for cm_id %p.\n", cm_id);
-	srpt_drain_channel(cm_id);
+	pr_info("Received CM TimeWait exit for ch %s-%d.\n", ch->sess_name,
+		ch->qp->qp_num);
+	srpt_drain_channel(ch);
 }
 
-static void srpt_cm_rep_error(struct ib_cm_id *cm_id)
+static void srpt_cm_rep_error(struct srpt_rdma_ch *ch)
 {
-	pr_info("Received IB REP error for cm_id %p.\n", cm_id);
-	srpt_drain_channel(cm_id);
+	pr_info("Received CM REP error for ch %s-%d.\n", ch->sess_name,
+		ch->qp->qp_num);
+	srpt_drain_channel(ch);
 }
 
 /**
  * srpt_cm_dreq_recv() - Process reception of a DREQ message.
  */
-static void srpt_cm_dreq_recv(struct ib_cm_id *cm_id)
+static void srpt_cm_dreq_recv(struct srpt_rdma_ch *ch)
 {
-	struct srpt_rdma_ch *ch;
 	unsigned long flags;
 	bool send_drep = false;
 
-	ch = srpt_find_channel(cm_id->context, cm_id);
-	BUG_ON(!ch);
-
-	pr_debug("cm_id= %p ch->state= %d\n", cm_id, ch->state);
+	pr_debug("ch %s-%d state %d\n", ch->sess_name, ch->qp->qp_num,
+		 ch->state);
 
 	spin_lock_irqsave(&ch->spinlock, flags);
 	switch (ch->state) {
@@ -2391,10 +2361,10 @@ static void srpt_cm_dreq_recv(struct ib_cm_id *cm_id)
 /**
  * srpt_cm_drep_recv() - Process reception of a DREP message.
  */
-static void srpt_cm_drep_recv(struct ib_cm_id *cm_id)
+static void srpt_cm_drep_recv(struct srpt_rdma_ch *ch)
 {
-	pr_info("Received InfiniBand DREP message for cm_id %p.\n", cm_id);
-	srpt_drain_channel(cm_id);
+	pr_info("Received InfiniBand DREP message for cm_id %p.\n", ch->cm_id);
+	srpt_drain_channel(ch);
 }
 
 /**
@@ -2409,6 +2379,7 @@ static void srpt_cm_drep_recv(struct ib_cm_id *cm_id)
  */
 static int srpt_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event)
 {
+	struct srpt_rdma_ch *ch = cm_id->context;
 	int ret;
 
 	ret = 0;
@@ -2418,23 +2389,25 @@ static int srpt_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event)
 				       event->private_data);
 		break;
 	case IB_CM_REJ_RECEIVED:
-		srpt_cm_rej_recv(cm_id);
+		srpt_cm_rej_recv(ch, event->param.rej_rcvd.reason,
+				 event->private_data,
+				 IB_CM_REJ_PRIVATE_DATA_SIZE);
 		break;
 	case IB_CM_RTU_RECEIVED:
 	case IB_CM_USER_ESTABLISHED:
-		srpt_cm_rtu_recv(cm_id);
+		srpt_cm_rtu_recv(ch);
 		break;
 	case IB_CM_DREQ_RECEIVED:
-		srpt_cm_dreq_recv(cm_id);
+		srpt_cm_dreq_recv(ch);
 		break;
 	case IB_CM_DREP_RECEIVED:
-		srpt_cm_drep_recv(cm_id);
+		srpt_cm_drep_recv(ch);
 		break;
 	case IB_CM_TIMEWAIT_EXIT:
-		srpt_cm_timewait_exit(cm_id);
+		srpt_cm_timewait_exit(ch);
 		break;
 	case IB_CM_REP_ERROR:
-		srpt_cm_rep_error(cm_id);
+		srpt_cm_rep_error(ch);
 		break;
 	case IB_CM_DREQ_ERROR:
 		pr_info("Received IB DREQ ERROR event.\n");
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH 13/15] IB/srpt: Detect session shutdown reliably
       [not found] ` <568BD0FC.70207-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
                     ` (11 preceding siblings ...)
  2016-01-05 14:26   ` [PATCH 12/15] IB/srpt: Eliminate srpt_find_channel() Bart Van Assche
@ 2016-01-05 14:26   ` Bart Van Assche
       [not found]     ` <568BD2A9.6070600-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-05 14:27   ` [PATCH 14/15] IB/srpt: Fix srpt_write_pending() Bart Van Assche
  2016-01-05 14:27   ` [PATCH 15/15] IB/srpt: Fix a rare crash in srpt_close_session() Bart Van Assche
  14 siblings, 1 reply; 57+ messages in thread
From: Bart Van Assche @ 2016-01-05 14:26 UTC (permalink / raw)
  To: Doug Ledford; +Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

The Last WQE Reached event is only generated after one or more work
requests have been queued on the QP associated with a session. Since
session shutdown can start before any work requests have been queued,
use a zero-length RDMA write to wait until a QP has been drained.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 274 +++++++++++++++++-----------------
 drivers/infiniband/ulp/srpt/ib_srpt.h |  18 ++-
 2 files changed, 150 insertions(+), 142 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 6ec130d..cacb697 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -91,10 +91,11 @@ MODULE_PARM_DESC(srpt_service_guid,
 		 " instead of using the node_guid of the first HCA.");
 
 static struct ib_client srpt_client;
-static void srpt_release_channel(struct srpt_rdma_ch *ch);
+static void srpt_free_ch(struct kref *kref);
 static int srpt_queue_status(struct se_cmd *cmd);
 static void srpt_recv_done(struct ib_cq *cq, struct ib_wc *wc);
 static void srpt_send_done(struct ib_cq *cq, struct ib_wc *wc);
+static void srpt_zerolength_write_done(struct ib_cq *cq, struct ib_wc *wc);
 
 static bool srpt_set_ch_state(struct srpt_rdma_ch *ch, enum rdma_ch_state new)
 {
@@ -170,6 +171,23 @@ static void srpt_srq_event(struct ib_event *event, void *ctx)
 	pr_info("SRQ event %d\n", event->event);
 }
 
+static const char *get_ch_state_name(enum rdma_ch_state s)
+{
+	switch (s) {
+	case CH_CONNECTING:
+		return "connecting";
+	case CH_LIVE:
+		return "live";
+	case CH_DISCONNECTING:
+		return "disconnecting";
+	case CH_DRAINING:
+		return "draining";
+	case CH_DISCONNECTED:
+		return "disconnected";
+	}
+	return "???";
+}
+
 /**
  * srpt_qp_event() - QP event callback function.
  */
@@ -183,11 +201,9 @@ static void srpt_qp_event(struct ib_event *event, struct srpt_rdma_ch *ch)
 		ib_cm_notify(ch->cm_id, event->event);
 		break;
 	case IB_EVENT_QP_LAST_WQE_REACHED:
-		if (srpt_set_ch_state(ch, CH_RELEASING))
-			srpt_release_channel(ch);
-		else
-			pr_debug("%s: state %d - ignored LAST_WQE.\n",
-				 ch->sess_name, ch->state);
+		pr_debug("%s-%d, state %s: received Last WQE event.\n",
+			 ch->sess_name, ch->qp->qp_num,
+			 get_ch_state_name(ch->state));
 		break;
 	default:
 		pr_err("received unrecognized IB QP event %d\n", event->event);
@@ -789,6 +805,37 @@ out:
 }
 
 /**
+ * srpt_zerolength_write() - Perform a zero-length RDMA write.
+ *
+ * A quote from the InfiniBand specification: C9-88: For an HCA responder
+ * using Reliable Connection service, for each zero-length RDMA READ or WRITE
+ * request, the R_Key shall not be validated, even if the request includes
+ * Immediate data.
+ */
+static int srpt_zerolength_write(struct srpt_rdma_ch *ch)
+{
+	struct ib_send_wr wr, *bad_wr;
+
+	memset(&wr, 0, sizeof(wr));
+	wr.opcode = IB_WR_RDMA_WRITE;
+	wr.wr_cqe = &ch->zw_cqe;
+	wr.send_flags = IB_SEND_SIGNALED;
+	return ib_post_send(ch->qp, &wr, &bad_wr);
+}
+
+static void srpt_zerolength_write_done(struct ib_cq *cq, struct ib_wc *wc)
+{
+	struct srpt_rdma_ch *ch = cq->cq_context;
+
+	WARN(wc->status == IB_WC_SUCCESS, "%s-%d: QP not in error state\n",
+	     ch->sess_name, ch->qp->qp_num);
+	if (srpt_set_ch_state(ch, CH_DISCONNECTED))
+		schedule_work(&ch->release_work);
+	else
+		WARN_ONCE("%s-%d\n", ch->sess_name, ch->qp->qp_num);
+}
+
+/**
  * srpt_get_desc_tbl() - Parse the data descriptors of an SRP_CMD request.
  * @ioctx: Pointer to the I/O context associated with the request.
  * @srp_cmd: Pointer to the SRP_CMD request data.
@@ -1819,111 +1866,102 @@ static void srpt_destroy_ch_ib(struct srpt_rdma_ch *ch)
 }
 
 /**
- * __srpt_close_ch() - Close an RDMA channel by setting the QP error state.
+ * srpt_close_ch() - Close an RDMA channel.
  *
- * Reset the QP and make sure all resources associated with the channel will
- * be deallocated at an appropriate time.
+ * Make sure all resources associated with the channel will be deallocated at
+ * an appropriate time.
  *
- * Note: The caller must hold ch->sport->sdev->spinlock.
+ * Returns true if and only if the channel state has been modified into
+ * CH_DRAINING.
  */
-static void __srpt_close_ch(struct srpt_rdma_ch *ch)
+static bool srpt_close_ch(struct srpt_rdma_ch *ch)
 {
-	enum rdma_ch_state prev_state;
-	unsigned long flags;
+	int ret;
 
-	spin_lock_irqsave(&ch->spinlock, flags);
-	prev_state = ch->state;
-	switch (prev_state) {
-	case CH_CONNECTING:
-	case CH_LIVE:
-		ch->state = CH_DISCONNECTING;
-		break;
-	default:
-		break;
+	if (!srpt_set_ch_state(ch, CH_DRAINING)) {
+		pr_debug("%s-%d: already closed\n", ch->sess_name,
+			 ch->qp->qp_num);
+		return false;
 	}
-	spin_unlock_irqrestore(&ch->spinlock, flags);
 
-	switch (prev_state) {
-	case CH_CONNECTING:
-		ib_send_cm_rej(ch->cm_id, IB_CM_REJ_NO_RESOURCES, NULL, 0,
-			       NULL, 0);
-		/* fall through */
-	case CH_LIVE:
-		if (ib_send_cm_dreq(ch->cm_id, NULL, 0) < 0)
-			pr_err("sending CM DREQ failed.\n");
-		break;
-	case CH_DISCONNECTING:
-		break;
-	case CH_DRAINING:
-	case CH_RELEASING:
-		break;
-	}
-}
+	kref_get(&ch->kref);
 
-/**
- * srpt_close_ch() - Close an RDMA channel.
- */
-static void srpt_close_ch(struct srpt_rdma_ch *ch)
-{
-	struct srpt_device *sdev;
+	ret = srpt_ch_qp_err(ch);
+	if (ret < 0)
+		pr_err("%s-%d: changing queue pair into error state failed: %d\n",
+		       ch->sess_name, ch->qp->qp_num, ret);
 
-	sdev = ch->sport->sdev;
-	spin_lock_irq(&sdev->spinlock);
-	__srpt_close_ch(ch);
-	spin_unlock_irq(&sdev->spinlock);
-}
+	pr_debug("%s-%d: queued zerolength write\n", ch->sess_name,
+		 ch->qp->qp_num);
+	ret = srpt_zerolength_write(ch);
+	if (ret < 0) {
+		pr_err("%s-%d: queuing zero-length write failed: %d\n",
+		       ch->sess_name, ch->qp->qp_num, ret);
+		if (srpt_set_ch_state(ch, CH_DISCONNECTED))
+			schedule_work(&ch->release_work);
+		else
+			WARN_ON_ONCE(true);
+	}
+
+	kref_put(&ch->kref, srpt_free_ch);
 
-/**
- * srpt_shutdown_session() - Whether or not a session may be shut down.
- */
-static int srpt_shutdown_session(struct se_session *se_sess)
-{
 	return true;
 }
 
-/**
- * srpt_drain_channel() - Drain a channel by resetting the IB queue pair.
- * @cm_id: Pointer to the CM ID of the channel to be drained.
- *
- * Note: Must be called from inside srpt_cm_handler to avoid a race between
- * accessing sdev->spinlock and the call to kfree(sdev) in srpt_remove_one()
- * (the caller of srpt_cm_handler holds the cm_id spinlock; srpt_remove_one()
- * waits until all target sessions for the associated IB device have been
- * unregistered and target session registration involves a call to
- * ib_destroy_cm_id(), which locks the cm_id spinlock and hence waits until
- * this function has finished).
+/*
+ * Change the channel state into CH_DISCONNECTING. If a channel has not yet
+ * reached the connected state, close it. If a channel is in the connected
+ * state, send a DREQ. If a DREQ has been received, send a DREP. Note: it is
+ * the responsibility of the caller to ensure that this function is not
+ * invoked concurrently with the code that accepts a connection. This means
+ * that this function must either be invoked from inside a CM callback
+ * function or that it must be invoked with the srpt_port.mutex held.
  */
-static void srpt_drain_channel(struct srpt_rdma_ch *ch)
+static int srpt_disconnect_ch(struct srpt_rdma_ch *ch)
 {
 	int ret;
-	bool do_reset = false;
 
-	WARN_ON_ONCE(irqs_disabled());
+	if (!srpt_set_ch_state(ch, CH_DISCONNECTING))
+		return -ENOTCONN;
 
-	do_reset = srpt_set_ch_state(ch, CH_DRAINING);
+	ret = ib_send_cm_dreq(ch->cm_id, NULL, 0);
+	if (ret < 0)
+		ret = ib_send_cm_drep(ch->cm_id, NULL, 0);
 
-	if (do_reset) {
-		if (ch->sess)
-			srpt_shutdown_session(ch->sess);
+	if (ret < 0 && srpt_close_ch(ch))
+		ret = 0;
 
-		ret = srpt_ch_qp_err(ch);
-		if (ret < 0)
-			pr_err("Setting queue pair in error state"
-			       " failed: %d\n", ret);
+	return ret;
+}
+
+static void __srpt_close_all_ch(struct srpt_device *sdev)
+{
+	struct srpt_rdma_ch *ch;
+
+	lockdep_assert_held(&sdev->spinlock);
+
+	list_for_each_entry(ch, &sdev->rch_list, list) {
+		if (srpt_disconnect_ch(ch) >= 0)
+			pr_info("Closing channel %s-%d because target %s has been disabled\n",
+				ch->sess_name, ch->qp->qp_num,
+				sdev->device->name);
+		srpt_close_ch(ch);
 	}
 }
 
 /**
- * srpt_release_channel() - Release channel resources.
- *
- * Schedules the actual release because:
- * - Calling the ib_destroy_cm_id() call from inside an IB CM callback would
- *   trigger a deadlock.
- * - It is not safe to call TCM transport_* functions from interrupt context.
+ * srpt_shutdown_session() - Whether or not a session may be shut down.
  */
-static void srpt_release_channel(struct srpt_rdma_ch *ch)
+static int srpt_shutdown_session(struct se_session *se_sess)
+{
+	return true;
+}
+
+static void srpt_free_ch(struct kref *kref)
 {
-	schedule_work(&ch->release_work);
+	struct srpt_rdma_ch *ch = container_of(kref, struct srpt_rdma_ch, kref);
+
+	kfree(ch);
 }
 
 static void srpt_release_channel_work(struct work_struct *w)
@@ -1965,7 +2003,7 @@ static void srpt_release_channel_work(struct work_struct *w)
 
 	wake_up(&sdev->ch_releaseQ);
 
-	kfree(ch);
+	kref_put(&ch->kref, srpt_free_ch);
 }
 
 static struct srpt_node_acl *__srpt_lookup_acl(struct srpt_port *sport,
@@ -2075,17 +2113,10 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 			    && param->port == ch->sport->port
 			    && param->listen_id == ch->sport->sdev->cm_id
 			    && ch->cm_id) {
-				if (ch->state != CH_CONNECTING
-				    && ch->state != CH_LIVE)
+				if (srpt_disconnect_ch(ch) < 0)
 					continue;
-
-				/* found an existing channel */
-				pr_debug("Found existing channel %s"
-					 " cm_id= %p state= %d\n",
-					 ch->sess_name, ch->cm_id, ch->state);
-
-				__srpt_close_ch(ch);
-
+				pr_info("Relogin - closed existing channel %s\n",
+					ch->sess_name);
 				rsp->rsp_flags =
 					SRP_LOGIN_RSP_MULTICHAN_TERMINATED;
 			}
@@ -2116,6 +2147,8 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 		goto reject;
 	}
 
+	kref_init(&ch->kref);
+	ch->zw_cqe.done = srpt_zerolength_write_done;
 	INIT_WORK(&ch->release_work, srpt_release_channel_work);
 	memcpy(ch->i_port_id, req->initiator_port_id, 16);
 	memcpy(ch->t_port_id, req->target_port_id, 16);
@@ -2232,7 +2265,7 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 	goto out;
 
 release_channel:
-	srpt_set_ch_state(ch, CH_RELEASING);
+	srpt_disconnect_ch(ch);
 	transport_deregister_session_configfs(ch->sess);
 
 deregister_session:
@@ -2282,7 +2315,6 @@ static void srpt_cm_rej_recv(struct srpt_rdma_ch *ch,
 	pr_info("Received CM REJ for ch %s-%d; reason %d; private data %s.\n",
 		ch->sess_name, ch->qp->qp_num, reason, priv ? : "(?)");
 	kfree(priv);
-	srpt_drain_channel(ch);
 }
 
 /**
@@ -2314,14 +2346,13 @@ static void srpt_cm_timewait_exit(struct srpt_rdma_ch *ch)
 {
 	pr_info("Received CM TimeWait exit for ch %s-%d.\n", ch->sess_name,
 		ch->qp->qp_num);
-	srpt_drain_channel(ch);
+	srpt_close_ch(ch);
 }
 
 static void srpt_cm_rep_error(struct srpt_rdma_ch *ch)
 {
 	pr_info("Received CM REP error for ch %s-%d.\n", ch->sess_name,
 		ch->qp->qp_num);
-	srpt_drain_channel(ch);
 }
 
 /**
@@ -2329,33 +2360,7 @@ static void srpt_cm_rep_error(struct srpt_rdma_ch *ch)
  */
 static void srpt_cm_dreq_recv(struct srpt_rdma_ch *ch)
 {
-	unsigned long flags;
-	bool send_drep = false;
-
-	pr_debug("ch %s-%d state %d\n", ch->sess_name, ch->qp->qp_num,
-		 ch->state);
-
-	spin_lock_irqsave(&ch->spinlock, flags);
-	switch (ch->state) {
-	case CH_CONNECTING:
-	case CH_LIVE:
-		send_drep = true;
-		ch->state = CH_DISCONNECTING;
-		break;
-	case CH_DISCONNECTING:
-	case CH_DRAINING:
-	case CH_RELEASING:
-		WARN(true, "unexpected channel state %d\n", ch->state);
-		break;
-	}
-	spin_unlock_irqrestore(&ch->spinlock, flags);
-
-	if (send_drep) {
-		if (ib_send_cm_drep(ch->cm_id, NULL, 0) < 0)
-			pr_err("Sending IB DREP failed.\n");
-		pr_info("Received DREQ and sent DREP for session %s.\n",
-			ch->sess_name);
-	}
+	srpt_disconnect_ch(ch);
 }
 
 /**
@@ -2364,7 +2369,7 @@ static void srpt_cm_dreq_recv(struct srpt_rdma_ch *ch)
 static void srpt_cm_drep_recv(struct srpt_rdma_ch *ch)
 {
 	pr_info("Received InfiniBand DREP message for cm_id %p.\n", ch->cm_id);
-	srpt_drain_channel(ch);
+	srpt_close_ch(ch);
 }
 
 /**
@@ -2539,7 +2544,7 @@ static int srpt_write_pending(struct se_cmd *se_cmd)
 		break;
 	case CH_DISCONNECTING:
 	case CH_DRAINING:
-	case CH_RELEASING:
+	case CH_DISCONNECTED:
 		pr_debug("cmd with tag %lld: channel disconnecting\n",
 			 ioctx->cmd.tag);
 		srpt_set_cmd_state(ioctx, SRPT_STATE_DATA_IN);
@@ -2696,16 +2701,16 @@ static int srpt_ch_list_empty(struct srpt_device *sdev)
  */
 static int srpt_release_sdev(struct srpt_device *sdev)
 {
-	struct srpt_rdma_ch *ch, *tmp_ch;
-	int res;
+	int i, res;
 
 	WARN_ON_ONCE(irqs_disabled());
 
 	BUG_ON(!sdev);
 
 	spin_lock_irq(&sdev->spinlock);
-	list_for_each_entry_safe(ch, tmp_ch, &sdev->rch_list, list)
-		__srpt_close_ch(ch);
+	for (i = 0; i < ARRAY_SIZE(sdev->port); i++)
+		sdev->port[i].enabled = false;
+	__srpt_close_all_ch(sdev);
 	spin_unlock_irq(&sdev->spinlock);
 
 	res = wait_event_interruptible(sdev->ch_releaseQ,
@@ -3007,9 +3012,10 @@ static void srpt_close_session(struct se_session *se_sess)
 	BUG_ON(ch->release_done);
 	ch->release_done = &release_done;
 	wait = !list_empty(&ch->list);
-	__srpt_close_ch(ch);
 	spin_unlock_irq(&sdev->spinlock);
 
+	srpt_disconnect_ch(ch);
+
 	if (!wait)
 		return;
 
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h
index d0de468..17058ef 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.h
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.h
@@ -218,20 +218,20 @@ struct srpt_send_ioctx {
 
 /**
  * enum rdma_ch_state - SRP channel state.
- * @CH_CONNECTING:	 QP is in RTR state; waiting for RTU.
- * @CH_LIVE:		 QP is in RTS state.
- * @CH_DISCONNECTING:    DREQ has been received; waiting for DREP
- *                       or DREQ has been send and waiting for DREP
- *                       or .
- * @CH_DRAINING:	 QP is in ERR state; waiting for last WQE event.
- * @CH_RELEASING:	 Last WQE event has been received; releasing resources.
+ * @CH_CONNECTING:    QP is in RTR state; waiting for RTU.
+ * @CH_LIVE:	      QP is in RTS state.
+ * @CH_DISCONNECTING: DREQ has been sent and waiting for DREP or DREQ has
+ *                    been received.
+ * @CH_DRAINING:      DREP has been received or waiting for DREP timed out
+ *                    and last work request has been queued.
+ * @CH_DISCONNECTED:  Last completion has been received.
  */
 enum rdma_ch_state {
 	CH_CONNECTING,
 	CH_LIVE,
 	CH_DISCONNECTING,
 	CH_DRAINING,
-	CH_RELEASING
+	CH_DISCONNECTED,
 };
 
 /**
@@ -268,6 +268,8 @@ struct srpt_rdma_ch {
 	struct ib_cm_id		*cm_id;
 	struct ib_qp		*qp;
 	struct ib_cq		*cq;
+	struct ib_cqe		zw_cqe;
+	struct kref		kref;
 	int			rq_size;
 	u32			rsp_size;
 	atomic_t		sq_wr_avail;
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH 14/15] IB/srpt: Fix srpt_write_pending()
       [not found] ` <568BD0FC.70207-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
                     ` (12 preceding siblings ...)
  2016-01-05 14:26   ` [PATCH 13/15] IB/srpt: Detect session shutdown reliably Bart Van Assche
@ 2016-01-05 14:27   ` Bart Van Assche
       [not found]     ` <568BD2C2.6090808-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-05 14:27   ` [PATCH 15/15] IB/srpt: Fix a rare crash in srpt_close_session() Bart Van Assche
  14 siblings, 1 reply; 57+ messages in thread
From: Bart Van Assche @ 2016-01-05 14:27 UTC (permalink / raw)
  To: Doug Ledford; +Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

The only allowed return values for the write_pending() callback
function are 0, -EAGAIN and -ENOMEM. Since attempting to perform
RDMA over a disconnecting channel will result in an IB error
completion anyway, remove the code that checks the channel state
from srpt_write_pending().

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 33 ++++-----------------------------
 1 file changed, 4 insertions(+), 29 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index cacb697..669ae5c 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2522,39 +2522,14 @@ out_unmap:
  */
 static int srpt_write_pending(struct se_cmd *se_cmd)
 {
-	struct srpt_rdma_ch *ch;
-	struct srpt_send_ioctx *ioctx;
+	struct srpt_send_ioctx *ioctx =
+		container_of(se_cmd, struct srpt_send_ioctx, cmd);
+	struct srpt_rdma_ch *ch = ioctx->ch;
 	enum srpt_command_state new_state;
-	int ret;
-
-	ioctx = container_of(se_cmd, struct srpt_send_ioctx, cmd);
 
 	new_state = srpt_set_cmd_state(ioctx, SRPT_STATE_NEED_DATA);
 	WARN_ON(new_state == SRPT_STATE_DONE);
-
-	ch = ioctx->ch;
-	BUG_ON(!ch);
-
-	switch (ch->state) {
-	case CH_CONNECTING:
-		WARN(true, "unexpected channel state %d\n", ch->state);
-		ret = -EINVAL;
-		goto out;
-	case CH_LIVE:
-		break;
-	case CH_DISCONNECTING:
-	case CH_DRAINING:
-	case CH_DISCONNECTED:
-		pr_debug("cmd with tag %lld: channel disconnecting\n",
-			 ioctx->cmd.tag);
-		srpt_set_cmd_state(ioctx, SRPT_STATE_DATA_IN);
-		ret = -EINVAL;
-		goto out;
-	}
-	ret = srpt_xfer_data(ch, ioctx);
-
-out:
-	return ret;
+	return srpt_xfer_data(ch, ioctx);
 }
 
 static u8 tcm_to_srp_tsk_mgmt_status(const int tcm_mgmt_status)
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH 15/15] IB/srpt: Fix a rare crash in srpt_close_session()
       [not found] ` <568BD0FC.70207-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
                     ` (13 preceding siblings ...)
  2016-01-05 14:27   ` [PATCH 14/15] IB/srpt: Fix srpt_write_pending() Bart Van Assche
@ 2016-01-05 14:27   ` Bart Van Assche
       [not found]     ` <568BD2E1.1060701-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  14 siblings, 1 reply; 57+ messages in thread
From: Bart Van Assche @ 2016-01-05 14:27 UTC (permalink / raw)
  To: Doug Ledford; +Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

Keep the ib_srpt session as long as srpt_close_session() may
access it.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 669ae5c..7548bd5 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2228,6 +2228,8 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 	pr_debug("Establish connection sess=%p name=%s cm_id=%p\n", ch->sess,
 		 ch->sess_name, ch->cm_id);
 
+	kref_get(&ch->kref);
+
 	/* create srp_login_response */
 	rsp->opcode = SRP_LOGIN_RSP;
 	rsp->tag = req->tag;
@@ -2991,6 +2993,8 @@ static void srpt_close_session(struct se_session *se_sess)
 
 	srpt_disconnect_ch(ch);
 
+	kref_put(&ch->kref, srpt_free_ch);
+
 	if (!wait)
 		return;
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 57+ messages in thread

* Re: [PATCH 01/15] IB/srpt: Add parentheses around sizeof argument
       [not found]     ` <568BD142.3070900-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
@ 2016-01-06  4:58       ` Christoph Hellwig
  2016-01-06 13:58       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Christoph Hellwig @ 2016-01-06  4:58 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Doug Ledford, Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On Tue, Jan 05, 2016 at 03:20:50PM +0100, Bart Van Assche wrote:
> Although sizeof is an operator and hence in many cases parentheses can
> be left out, the recommended kernel coding style is to surround the
> sizeof argument with parentheses. This patch does not change any
> functionality. This patch has been generated by running the following
> shell command:

I don't really care about this formatting, but the patch looks fine:

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 02/15] IB/srpt: Inline srpt_sdev_name()
       [not found]     ` <568BD15F.6010909-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
@ 2016-01-06  4:58       ` Christoph Hellwig
  2016-01-06 13:58       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Christoph Hellwig @ 2016-01-06  4:58 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Doug Ledford, Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On Tue, Jan 05, 2016 at 03:21:19PM +0100, Bart Van Assche wrote:
> srpt_sdev_name() is too trivial to keep it as a separate function.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>

Looks good,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 03/15] IB/srpt: Inline srpt_get_ch_state()
       [not found]     ` <568BD181.5040009-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
@ 2016-01-06  4:58       ` Christoph Hellwig
  2016-01-06 13:59       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Christoph Hellwig @ 2016-01-06  4:58 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Doug Ledford, Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On Tue, Jan 05, 2016 at 03:21:53PM +0100, Bart Van Assche wrote:
> The callers of srpt_get_ch_state() can access ch->state safely without
> using locking. Hence inline this function.

Looks good,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 04/15] IB/srpt: Introduce target_reverse_dma_direction()
       [not found]     ` <568BD19C.6060404-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
@ 2016-01-06  4:59       ` Christoph Hellwig
  2016-01-06 14:00       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Christoph Hellwig @ 2016-01-06  4:59 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Doug Ledford, Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

Looks good,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 05/15] IB/srpt: Use scsilun_to_int()
       [not found]     ` <568BD1B6.4090308-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
@ 2016-01-06  4:59       ` Christoph Hellwig
  2016-01-06 14:01       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Christoph Hellwig @ 2016-01-06  4:59 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Doug Ledford, Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On Tue, Jan 05, 2016 at 03:22:46PM +0100, Bart Van Assche wrote:
> Just like other target drivers, use scsilun_to_int() to unpack SCSI
> LUN numbers. This patch only changes the behavior of ib_srpt for LUN
> numbers >= 16384.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>

Looks good,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 06/15] IB/srpt: Simplify srpt_handle_tsk_mgmt()
       [not found]     ` <568BD1D2.1030609-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
@ 2016-01-06  5:00       ` Christoph Hellwig
  2016-01-06 14:02       ` Sagi Grimberg
  2016-01-26 16:57       ` Estrin, Alex
  2 siblings, 0 replies; 57+ messages in thread
From: Christoph Hellwig @ 2016-01-06  5:00 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Doug Ledford, Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On Tue, Jan 05, 2016 at 03:23:14PM +0100, Bart Van Assche wrote:
> Let the target core check task existence instead of the SRP target
> driver.

Looks good,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 07/15] IB/srpt: Simplify channel state management
       [not found]     ` <568BD1F1.5050107-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
@ 2016-01-06  5:01       ` Christoph Hellwig
  2016-01-06 14:08       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Christoph Hellwig @ 2016-01-06  5:01 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Doug Ledford, Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On Tue, Jan 05, 2016 at 03:23:45PM +0100, Bart Van Assche wrote:
> The only allowed channel state changes are those that change
> the channel state into a state with a higher numerical value.
> This allows to merge the functions srpt_set_ch_state() and
> srpt_test_and_set_ch_state() into a single function.

It would be great having a little comment in srpt_set_ch_state explaining
why only changing to the numerical greater state is fine.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 08/15] IB/srpt: Simplify srpt_shutdown_session()
       [not found]     ` <568BD20F.7020101-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
@ 2016-01-06  5:07       ` Christoph Hellwig
  2016-01-06 14:13       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Christoph Hellwig @ 2016-01-06  5:07 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Doug Ledford, Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On Tue, Jan 05, 2016 at 03:24:15PM +0100, Bart Van Assche wrote:
> The target core guarantees that shutdown_session() is only invoked
> once per session. This means that the ib_srpt target driver doesn't
> have to track whether or not shutdown_session() has been called.
> Additionally, ensure that target_sess_cmd_list_set_waiting() is
> called before target_wait_for_sess_cmds() by moving it into
> srpt_release_channel_work().

Looks good,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>

Mote that now most drivers return either always return 0 or 1 from
shutdown_session, so it might be worth to investigate if we can get
rid of this method in the future.

Minor nitpick below:

>  static int srpt_shutdown_session(struct se_session *se_sess)
>  {
>  	return true;
>  }


Given that the function returns in this really should be 1 in instead of
true, but it's not really worth respinning the patch just for this.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 09/15] IB/srpt: Fix srpt_close_session()
       [not found]     ` <568BD231.10205-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
@ 2016-01-06  5:08       ` Christoph Hellwig
  2016-01-06 14:21       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Christoph Hellwig @ 2016-01-06  5:08 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Doug Ledford, Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On Tue, Jan 05, 2016 at 03:24:49PM +0100, Bart Van Assche wrote:
> Avoid that srpt_close_session() waits if it doesn't have to wait.
> Additionally, increase the time during which srpt_close_session()
> waits until closing a session has finished. This makes it easier
> to detect session shutdown bugs.

Looks good,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 10/15] IB/srpt: Fix srpt_handle_cmd() error paths
       [not found]     ` <568BD249.1000100-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
@ 2016-01-06  5:09       ` Christoph Hellwig
  2016-01-06 14:31       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Christoph Hellwig @ 2016-01-06  5:09 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Doug Ledford, Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On Tue, Jan 05, 2016 at 03:25:13PM +0100, Bart Van Assche wrote:
> The target core function that should be called if target_submit_cmd()
> fails is target_put_sess_cmd(). Additionally, change the return type
> of srpt_handle_cmd() from int into void.

I actually ran into this bug a long time ago with a modified srpt driver
and forgot to send a similar fix..

Looks good:

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>

Minor nitpick below:

> +	send_ioctx->state = SRPT_STATE_DONE;
> +	target_put_sess_cmd(cmd);
> +	return;
>  }

no need for that return statement.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 11/15] IB/srpt: Fix how aborted commands are processed
       [not found]     ` <568BD26D.9080003-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
@ 2016-01-06  5:13       ` Christoph Hellwig
       [not found]         ` <20160106051305.GK15574-jcswGhMUV9g@public.gmane.org>
  0 siblings, 1 reply; 57+ messages in thread
From: Christoph Hellwig @ 2016-01-06  5:13 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Doug Ledford, Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

>  	pr_debug("Aborting cmd with state %d and tag %lld\n", state,
>  		 ioctx->cmd.tag);
>  
> @@ -1299,14 +1291,16 @@ static int srpt_abort_cmd(struct srpt_send_ioctx *ioctx)
>  	case SRPT_STATE_NEW:
>  	case SRPT_STATE_DATA_IN:
>  	case SRPT_STATE_MGMT:
> +	case SRPT_STATE_DONE:
>  		/*
>  		 * Do nothing - defer abort processing until
>  		 * srpt_queue_response() is invoked.
>  		 */
> -		WARN_ON(!(ioctx->cmd.transport_state & CMD_T_ABORTED));

Seems like this depends on your target core changes?  Maybe it would be better
to respin the series to got just on top of Doug's RDMA tree, as I think
we're more likely to get this series merged for 4.5 than the target core
changes..

Otherwise these changes look fine to me.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 12/15] IB/srpt: Eliminate srpt_find_channel()
       [not found]     ` <568BD28E.40302-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
@ 2016-01-06  5:15       ` Christoph Hellwig
  2016-01-06 14:33       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Christoph Hellwig @ 2016-01-06  5:15 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Doug Ledford, Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On Tue, Jan 05, 2016 at 03:26:22PM +0100, Bart Van Assche wrote:
> In the CM REQ message handler, store the channel pointer in
> cm_id->context such that the function srpt_find_channel() is no
> longer needed. Additionally, make the CM event messages more
> informative.

Looks fine,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 13/15] IB/srpt: Detect session shutdown reliably
       [not found]     ` <568BD2A9.6070600-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
@ 2016-01-06  5:21       ` Christoph Hellwig
       [not found]         ` <20160106052138.GM15574-jcswGhMUV9g@public.gmane.org>
  2016-01-06 14:39       ` Sagi Grimberg
  1 sibling, 1 reply; 57+ messages in thread
From: Christoph Hellwig @ 2016-01-06  5:21 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Doug Ledford, Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On Tue, Jan 05, 2016 at 03:26:49PM +0100, Bart Van Assche wrote:
> The Last WQE Reached event is only generated after one or more work
> requests have been queued on the QP associated with a session. Since
> session shutdown can start before any work requests have been queued,
> use a zero-length RDMA write to wait until a QP has been drained.

We actually ran into the same issue with a SRPT-derived work in progress
driver recently..

> @@ -2314,14 +2346,13 @@ static void srpt_cm_timewait_exit(struct srpt_rdma_ch *ch)
>  {
>  	pr_info("Received CM TimeWait exit for ch %s-%d.\n", ch->sess_name,
>  		ch->qp->qp_num);
> +	srpt_close_ch(ch);
>  }
>  
>  static void srpt_cm_rep_error(struct srpt_rdma_ch *ch)
>  {
>  	pr_info("Received CM REP error for ch %s-%d.\n", ch->sess_name,
>  		ch->qp->qp_num);
>  }
>  
>  /**
> @@ -2329,33 +2360,7 @@ static void srpt_cm_rep_error(struct srpt_rdma_ch *ch)
>   */
>  static void srpt_cm_dreq_recv(struct srpt_rdma_ch *ch)
>  {
> +	srpt_disconnect_ch(ch);
>  }
>  
>  /**
> @@ -2364,7 +2369,7 @@ static void srpt_cm_dreq_recv(struct srpt_rdma_ch *ch)
>  static void srpt_cm_drep_recv(struct srpt_rdma_ch *ch)
>  {
>  	pr_info("Received InfiniBand DREP message for cm_id %p.\n", ch->cm_id);
> +	srpt_close_ch(ch);
>  }


Is there any good reson to keep these one-liner helpers around?

Otherwise looks good,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 14/15] IB/srpt: Fix srpt_write_pending()
       [not found]     ` <568BD2C2.6090808-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
@ 2016-01-06  5:22       ` Christoph Hellwig
  2016-01-06 14:41       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Christoph Hellwig @ 2016-01-06  5:22 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Doug Ledford, Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

Looks good,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 15/15] IB/srpt: Fix a rare crash in srpt_close_session()
       [not found]     ` <568BD2E1.1060701-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
@ 2016-01-06  5:23       ` Christoph Hellwig
  2016-01-06 14:47       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Christoph Hellwig @ 2016-01-06  5:23 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Doug Ledford, Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

>  	srpt_disconnect_ch(ch);
>  
> +	kref_put(&ch->kref, srpt_free_ch);

At some point it might be a good idea to have a srpt_put_ch helper to wrap
this pattern.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 11/15] IB/srpt: Fix how aborted commands are processed
       [not found]         ` <20160106051305.GK15574-jcswGhMUV9g@public.gmane.org>
@ 2016-01-06 13:30           ` Bart Van Assche
  0 siblings, 0 replies; 57+ messages in thread
From: Bart Van Assche @ 2016-01-06 13:30 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On 01/06/2016 06:13 AM, Christoph Hellwig wrote:
>>   	pr_debug("Aborting cmd with state %d and tag %lld\n", state,
>>   		 ioctx->cmd.tag);
>>
>> @@ -1299,14 +1291,16 @@ static int srpt_abort_cmd(struct srpt_send_ioctx *ioctx)
>>   	case SRPT_STATE_NEW:
>>   	case SRPT_STATE_DATA_IN:
>>   	case SRPT_STATE_MGMT:
>> +	case SRPT_STATE_DONE:
>>   		/*
>>   		 * Do nothing - defer abort processing until
>>   		 * srpt_queue_response() is invoked.
>>   		 */
>> -		WARN_ON(!(ioctx->cmd.transport_state & CMD_T_ABORTED));
>
> Seems like this depends on your target core changes?  Maybe it would be better
> to respin the series to got just on top of Doug's RDMA tree, as I think
> we're more likely to get this series merged for 4.5 than the target core
> changes..

This series indeed depends on my recently posted target core patch 
series. I will rebase, retest and repost this patch series.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 13/15] IB/srpt: Detect session shutdown reliably
       [not found]         ` <20160106052138.GM15574-jcswGhMUV9g@public.gmane.org>
@ 2016-01-06 13:34           ` Bart Van Assche
  0 siblings, 0 replies; 57+ messages in thread
From: Bart Van Assche @ 2016-01-06 13:34 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On 01/06/2016 06:21 AM, Christoph Hellwig wrote:
> On Tue, Jan 05, 2016 at 03:26:49PM +0100, Bart Van Assche wrote:
>> The Last WQE Reached event is only generated after one or more work
>> requests have been queued on the QP associated with a session. Since
>> session shutdown can start before any work requests have been queued,
>> use a zero-length RDMA write to wait until a QP has been drained.
>
> We actually ran into the same issue with a SRPT-derived work in progress
> driver recently..
>
>> @@ -2314,14 +2346,13 @@ static void srpt_cm_timewait_exit(struct srpt_rdma_ch *ch)
>>   {
>>   	pr_info("Received CM TimeWait exit for ch %s-%d.\n", ch->sess_name,
>>   		ch->qp->qp_num);
>> +	srpt_close_ch(ch);
>>   }
>>
>>   static void srpt_cm_rep_error(struct srpt_rdma_ch *ch)
>>   {
>>   	pr_info("Received CM REP error for ch %s-%d.\n", ch->sess_name,
>>   		ch->qp->qp_num);
>>   }
>>
>>   /**
>> @@ -2329,33 +2360,7 @@ static void srpt_cm_rep_error(struct srpt_rdma_ch *ch)
>>    */
>>   static void srpt_cm_dreq_recv(struct srpt_rdma_ch *ch)
>>   {
>> +	srpt_disconnect_ch(ch);
>>   }
>>
>>   /**
>> @@ -2364,7 +2369,7 @@ static void srpt_cm_dreq_recv(struct srpt_rdma_ch *ch)
>>   static void srpt_cm_drep_recv(struct srpt_rdma_ch *ch)
>>   {
>>   	pr_info("Received InfiniBand DREP message for cm_id %p.\n", ch->cm_id);
>> +	srpt_close_ch(ch);
>>   }
>
>
> Is there any good reason to keep these one-liner helpers around?

Hello Christoph,

Not really. I will inline these functions.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 01/15] IB/srpt: Add parentheses around sizeof argument
       [not found]     ` <568BD142.3070900-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-06  4:58       ` Christoph Hellwig
@ 2016-01-06 13:58       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Sagi Grimberg @ 2016-01-06 13:58 UTC (permalink / raw)
  To: Bart Van Assche, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 02/15] IB/srpt: Inline srpt_sdev_name()
       [not found]     ` <568BD15F.6010909-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-06  4:58       ` Christoph Hellwig
@ 2016-01-06 13:58       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Sagi Grimberg @ 2016-01-06 13:58 UTC (permalink / raw)
  To: Bart Van Assche, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 03/15] IB/srpt: Inline srpt_get_ch_state()
       [not found]     ` <568BD181.5040009-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-06  4:58       ` Christoph Hellwig
@ 2016-01-06 13:59       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Sagi Grimberg @ 2016-01-06 13:59 UTC (permalink / raw)
  To: Bart Van Assche, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 04/15] IB/srpt: Introduce target_reverse_dma_direction()
       [not found]     ` <568BD19C.6060404-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-06  4:59       ` Christoph Hellwig
@ 2016-01-06 14:00       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Sagi Grimberg @ 2016-01-06 14:00 UTC (permalink / raw)
  To: Bart Van Assche, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 05/15] IB/srpt: Use scsilun_to_int()
       [not found]     ` <568BD1B6.4090308-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-06  4:59       ` Christoph Hellwig
@ 2016-01-06 14:01       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Sagi Grimberg @ 2016-01-06 14:01 UTC (permalink / raw)
  To: Bart Van Assche, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 06/15] IB/srpt: Simplify srpt_handle_tsk_mgmt()
       [not found]     ` <568BD1D2.1030609-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-06  5:00       ` Christoph Hellwig
@ 2016-01-06 14:02       ` Sagi Grimberg
  2016-01-26 16:57       ` Estrin, Alex
  2 siblings, 0 replies; 57+ messages in thread
From: Sagi Grimberg @ 2016-01-06 14:02 UTC (permalink / raw)
  To: Bart Van Assche, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 07/15] IB/srpt: Simplify channel state management
       [not found]     ` <568BD1F1.5050107-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-06  5:01       ` Christoph Hellwig
@ 2016-01-06 14:08       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Sagi Grimberg @ 2016-01-06 14:08 UTC (permalink / raw)
  To: Bart Van Assche, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA


> The only allowed channel state changes are those that change
> the channel state into a state with a higher numerical value.
> This allows to merge the functions srpt_set_ch_state() and
> srpt_test_and_set_ch_state() into a single function.
>
> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>

The patch looks correct to me, but IMO it would be better to
open-code state transitions instead of relying on the assumption of
a numerical relationship between states. This can save a potential
future bug if someone will add/change a state in the future, and it
is also more readable IMO.

Just a suggestion though, otherwise

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 08/15] IB/srpt: Simplify srpt_shutdown_session()
       [not found]     ` <568BD20F.7020101-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-06  5:07       ` Christoph Hellwig
@ 2016-01-06 14:13       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Sagi Grimberg @ 2016-01-06 14:13 UTC (permalink / raw)
  To: Bart Van Assche, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 09/15] IB/srpt: Fix srpt_close_session()
       [not found]     ` <568BD231.10205-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-06  5:08       ` Christoph Hellwig
@ 2016-01-06 14:21       ` Sagi Grimberg
       [not found]         ` <568D22F6.4050803-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
  1 sibling, 1 reply; 57+ messages in thread
From: Sagi Grimberg @ 2016-01-06 14:21 UTC (permalink / raw)
  To: Bart Van Assche, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA


> Avoid that srpt_close_session() waits if it doesn't have to wait.

Can you explain when it doesn't have to wait? is it possible that
srpt_release_channel_work() was already triggered? isn't that a problem?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 10/15] IB/srpt: Fix srpt_handle_cmd() error paths
       [not found]     ` <568BD249.1000100-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-06  5:09       ` Christoph Hellwig
@ 2016-01-06 14:31       ` Sagi Grimberg
       [not found]         ` <568D2553.8050302-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
  1 sibling, 1 reply; 57+ messages in thread
From: Sagi Grimberg @ 2016-01-06 14:31 UTC (permalink / raw)
  To: Bart Van Assche, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA



On 05/01/2016 16:25, Bart Van Assche wrote:
> The target core function that should be called if target_submit_cmd()
> fails is target_put_sess_cmd(). Additionally, change the return type
> of srpt_handle_cmd() from int into void.
>
> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
> ---
>   drivers/infiniband/ulp/srpt/ib_srpt.c | 24 ++++++++++++------------
>   1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index 1857d17..395bc1b 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -1482,15 +1482,14 @@ static int srpt_check_stop_free(struct se_cmd *cmd)
>   /**
>    * srpt_handle_cmd() - Process SRP_CMD.
>    */
> -static int srpt_handle_cmd(struct srpt_rdma_ch *ch,
> -			   struct srpt_recv_ioctx *recv_ioctx,
> -			   struct srpt_send_ioctx *send_ioctx)
> +static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
> +			    struct srpt_recv_ioctx *recv_ioctx,
> +			    struct srpt_send_ioctx *send_ioctx)
>   {
>   	struct se_cmd *cmd;
>   	struct srp_cmd *srp_cmd;
>   	u64 data_len;
>   	enum dma_data_direction dir;
> -	sense_reason_t ret;
>   	int rc;
>
>   	BUG_ON(!send_ioctx);
> @@ -1518,8 +1517,7 @@ static int srpt_handle_cmd(struct srpt_rdma_ch *ch,
>   	if (srpt_get_desc_tbl(send_ioctx, srp_cmd, &dir, &data_len)) {
>   		pr_err("0x%llx: parsing SRP descriptor table failed.\n",
>   		       srp_cmd->tag);
> -		ret = TCM_INVALID_CDB_FIELD;
> -		goto send_sense;
> +		goto put;

Do you still need to call target_put_sess_cmd() in this failure?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 12/15] IB/srpt: Eliminate srpt_find_channel()
       [not found]     ` <568BD28E.40302-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-06  5:15       ` Christoph Hellwig
@ 2016-01-06 14:33       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Sagi Grimberg @ 2016-01-06 14:33 UTC (permalink / raw)
  To: Bart Van Assche, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

Looks good,

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 09/15] IB/srpt: Fix srpt_close_session()
       [not found]         ` <568D22F6.4050803-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
@ 2016-01-06 14:34           ` Bart Van Assche
       [not found]             ` <568D25D9.3050809-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  0 siblings, 1 reply; 57+ messages in thread
From: Bart Van Assche @ 2016-01-06 14:34 UTC (permalink / raw)
  To: Sagi Grimberg, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On 01/06/2016 03:21 PM, Sagi Grimberg wrote:
>> Avoid that srpt_close_session() waits if it doesn't have to wait.
>
> Can you explain when it doesn't have to wait? is it possible that
> srpt_release_channel_work() was already triggered? isn't that a problem?

Hello Sagi,

The target core can decide to shut down an RDMA channel or a channel 
shutdown can be the result of the reception of a DREQ message. It is in 
the latter case that srpt_release_channel_work() can have finished 
before srpt_close_session() is called.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 10/15] IB/srpt: Fix srpt_handle_cmd() error paths
       [not found]         ` <568D2553.8050302-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
@ 2016-01-06 14:36           ` Bart Van Assche
  0 siblings, 0 replies; 57+ messages in thread
From: Bart Van Assche @ 2016-01-06 14:36 UTC (permalink / raw)
  To: Sagi Grimberg, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On 01/06/2016 03:31 PM, Sagi Grimberg wrote:
> On 05/01/2016 16:25, Bart Van Assche wrote:
>> @@ -1518,8 +1517,7 @@ static int srpt_handle_cmd(struct srpt_rdma_ch *ch,
>>       if (srpt_get_desc_tbl(send_ioctx, srp_cmd, &dir, &data_len)) {
>>           pr_err("0x%llx: parsing SRP descriptor table failed.\n",
>>                  srp_cmd->tag);
>> -        ret = TCM_INVALID_CDB_FIELD;
>> -        goto send_sense;
>> +        goto put;
>
> Do you still need to call target_put_sess_cmd() in this failure?

Good catch. I will update this patch before I repost this patch series.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 13/15] IB/srpt: Detect session shutdown reliably
       [not found]     ` <568BD2A9.6070600-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-06  5:21       ` Christoph Hellwig
@ 2016-01-06 14:39       ` Sagi Grimberg
       [not found]         ` <568D2707.9020309-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
  1 sibling, 1 reply; 57+ messages in thread
From: Sagi Grimberg @ 2016-01-06 14:39 UTC (permalink / raw)
  To: Bart Van Assche, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA



On 05/01/2016 16:26, Bart Van Assche wrote:
> The Last WQE Reached event is only generated after one or more work
> requests have been queued on the QP associated with a session. Since
> session shutdown can start before any work requests have been queued,
> use a zero-length RDMA write to wait until a QP has been drained.

Is it just me or is there (a lot) more going on here other than
this patch description?

Would it be possible to either:
- break the patch to smaller pieces (preferable), or
- document all the changes in this patch

It's hard to understand the reason for each change.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 14/15] IB/srpt: Fix srpt_write_pending()
       [not found]     ` <568BD2C2.6090808-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-06  5:22       ` Christoph Hellwig
@ 2016-01-06 14:41       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Sagi Grimberg @ 2016-01-06 14:41 UTC (permalink / raw)
  To: Bart Van Assche, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

Looks good,

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 09/15] IB/srpt: Fix srpt_close_session()
       [not found]             ` <568D25D9.3050809-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
@ 2016-01-06 14:46               ` Sagi Grimberg
  0 siblings, 0 replies; 57+ messages in thread
From: Sagi Grimberg @ 2016-01-06 14:46 UTC (permalink / raw)
  To: Bart Van Assche, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA



On 06/01/2016 16:34, Bart Van Assche wrote:
> On 01/06/2016 03:21 PM, Sagi Grimberg wrote:
>>> Avoid that srpt_close_session() waits if it doesn't have to wait.
>>
>> Can you explain when it doesn't have to wait? is it possible that
>> srpt_release_channel_work() was already triggered? isn't that a problem?
>
> Hello Sagi,
>
> The target core can decide to shut down an RDMA channel or a channel
> shutdown can be the result of the reception of a DREQ message. It is in
> the latter case that srpt_release_channel_work() can have finished
> before srpt_close_session() is called.

I understood that, and the reason I asked was because of the fact that
you dereference the ch while the channel release is ongoing...

Further reading tells me this is handled in patch 15 "IB/srpt: Fix a
rare crash in srpt_close_session()" correct?

If so, should it come before this patch?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 13/15] IB/srpt: Detect session shutdown reliably
       [not found]         ` <568D2707.9020309-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
@ 2016-01-06 14:46           ` Bart Van Assche
       [not found]             ` <568D28CA.8010804-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  0 siblings, 1 reply; 57+ messages in thread
From: Bart Van Assche @ 2016-01-06 14:46 UTC (permalink / raw)
  To: Sagi Grimberg, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On 01/06/2016 03:39 PM, Sagi Grimberg wrote:
> On 05/01/2016 16:26, Bart Van Assche wrote:
>> The Last WQE Reached event is only generated after one or more work
>> requests have been queued on the QP associated with a session. Since
>> session shutdown can start before any work requests have been queued,
>> use a zero-length RDMA write to wait until a QP has been drained.
>
> Is it just me or is there (a lot) more going on here other than
> this patch description?

Hello Sagi,

I will make the patch description more detailed. Sorry if some of this 
code is hard to follow but that's because of the high level of 
concurrency in the SRP target driver. Some time ago I documented how 
session management in the SCST ib_srpt driver works. This driver follows 
the same model. These notes can be found here: 
http://sourceforge.net/p/scst/svn/HEAD/tree/trunk/srpt/session-management.txt.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 15/15] IB/srpt: Fix a rare crash in srpt_close_session()
       [not found]     ` <568BD2E1.1060701-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-06  5:23       ` Christoph Hellwig
@ 2016-01-06 14:47       ` Sagi Grimberg
  1 sibling, 0 replies; 57+ messages in thread
From: Sagi Grimberg @ 2016-01-06 14:47 UTC (permalink / raw)
  To: Bart Van Assche, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA


> Keep the ib_srpt session as long as srpt_close_session() may
> access it.

Makes sense,

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 13/15] IB/srpt: Detect session shutdown reliably
       [not found]             ` <568D28CA.8010804-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
@ 2016-01-06 17:04               ` Christoph Hellwig
  0 siblings, 0 replies; 57+ messages in thread
From: Christoph Hellwig @ 2016-01-06 17:04 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Sagi Grimberg, Doug Ledford, Christoph Hellwig,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA

On Wed, Jan 06, 2016 at 03:46:34PM +0100, Bart Van Assche wrote:
> I will make the patch description more detailed. Sorry if some of this code 
> is hard to follow but that's because of the high level of concurrency in 
> the SRP target driver. Some time ago I documented how session management in 
> the SCST ib_srpt driver works. This driver follows the same model. These 
> notes can be found here: 
> http://sourceforge.net/p/scst/svn/HEAD/tree/trunk/srpt/session-management.txt.

It might be useful to eventually add a version of that to the Linux
kernel tree as well.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* RE: [PATCH 06/15] IB/srpt: Simplify srpt_handle_tsk_mgmt()
       [not found]     ` <568BD1D2.1030609-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-01-06  5:00       ` Christoph Hellwig
  2016-01-06 14:02       ` Sagi Grimberg
@ 2016-01-26 16:57       ` Estrin, Alex
       [not found]         ` <F3529576D8E232409F431C309E29399328F6B06A-8k97q/ur5Z1cIJlls4ac1rfspsVTdybXVpNB7YpNyf8@public.gmane.org>
  2 siblings, 1 reply; 57+ messages in thread
From: Estrin, Alex @ 2016-01-26 16:57 UTC (permalink / raw)
  To: Bart Van Assche, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

Hi Doug, Bart,

Is it possible to expedite this patch as it actually fixes a real bug in ib_srpt?

If srp target receives ABORT TASK, it will crash the kernel while trying to respond.
We were able to reproduce it under quite heavy load:

[78395.442496] BUG: unable to handle kernel NULL pointer dereference at
0000000000000001
[78395.442534] IP: [<ffffffffa0565f37>] srpt_handle_new_iu+0x6d7/0x790
[ib_srpt]
[78395.442564] PGD 0
[78395.442574] Oops: 0002 [#1] SMP
....
[78395.443443] Call Trace:
[78395.443457]  [<ffffffffa05660ce>] srpt_process_completion+0xde/0x570
[ib_srpt]
[78395.443484]  [<ffffffffa056669f>] srpt_compl_thread+0x13f/0x160 [ib_srpt]
[78395.444406]  [<ffffffff81098230>] ? wake_up_bit+0x30/0x30
[78395.445307]  [<ffffffffa0566560>] ? srpt_process_completion+0x570/0x570
[ib_srpt]
[78395.446218]  [<ffffffff8109726f>] kthread+0xcf/0xe0
[78395.447125]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
[78395.448033]  [<ffffffff81613cfc>] ret_from_fork+0x7c/0xb0
[78395.448927]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
[78395.449814] Code: 1a 01 00 00 01 74 a3 48 8b 7d a0 89 55 b8 48 89 4d c0 e8
fd 20 00 00 8b 55 b8 48 8b 4d c0 eb 8a 0f 1f 40 00 49 8b 85 f8 00 00 00 <c6> 40
01 01 e9 44 fd ff ff 49 8b b
4 24 b8 04 00 00 49 8d 94 24
[78395.451693] RIP  [<ffffffffa0565f37>] srpt_handle_new_iu+0x6d7/0x790
[ib_srpt]
[78395.452603]  RSP <ffff88083e80fd70>
[78395.453498] CR2: 0000000000000001

Trace was obtained on RHEL7.1 distro, but could happened on any kernel,
so I believe this patch should go to stable as well.
Please see below the hit scenario(fixed by this patch).

Fixes: 3e4f574857ee ("ib_srpt: Convert TMR path to target_submit_tmr")
Tested-by: Alex Estrin <alex.estrin@intel.com>


> -----Original Message-----
> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-owner@vger.kernel.org] On
> Behalf Of Bart Van Assche
> Sent: Tuesday, January 05, 2016 9:23 AM
> To: Doug Ledford
> Cc: Christoph Hellwig; linux-rdma@vger.kernel.org
> Subject: [PATCH 06/15] IB/srpt: Simplify srpt_handle_tsk_mgmt()
> 
> Let the target core check task existence instead of the SRP target
> driver.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/infiniband/ulp/srpt/ib_srpt.c | 54 ++---------------------------------
>  1 file changed, 2 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c
> b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index fc19203..9cb1a14 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -1554,47 +1554,6 @@ send_sense:
>  	return -1;
>  }
> 
> -/**
> - * srpt_rx_mgmt_fn_tag() - Process a task management function by tag.
> - * @ch: RDMA channel of the task management request.
> - * @fn: Task management function to perform.
> - * @req_tag: Tag of the SRP task management request.
> - * @mgmt_ioctx: I/O context of the task management request.
> - *
> - * Returns zero if the target core will process the task management
> - * request asynchronously.
> - *
> - * Note: It is assumed that the initiator serializes tag-based task management
> - * requests.
> - */
> -static int srpt_rx_mgmt_fn_tag(struct srpt_send_ioctx *ioctx, u64 tag)
> -{
> -	struct srpt_device *sdev;
> -	struct srpt_rdma_ch *ch;
> -	struct srpt_send_ioctx *target;
> -	int ret, i;
> -
> -	ret = -EINVAL;
> -	ch = ioctx->ch;
> -	BUG_ON(!ch);
> -	BUG_ON(!ch->sport);
> -	sdev = ch->sport->sdev;
> -	BUG_ON(!sdev);
> -	spin_lock_irq(&sdev->spinlock);
> -	for (i = 0; i < ch->rq_size; ++i) {
> -		target = ch->ioctx_ring[i];
> -		if (target->cmd.se_lun == ioctx->cmd.se_lun &&
> -		    target->cmd.tag == tag &&
> -		    srpt_get_cmd_state(target) != SRPT_STATE_DONE) {
> -			ret = 0;
> -			/* now let the target core abort &target->cmd; */
> -			break;
> -		}
> -	}
> -	spin_unlock_irq(&sdev->spinlock);
> -	return ret;
> -}
> -
>  static int srp_tmr_to_tcm(int fn)
>  {
>  	switch (fn) {
> @@ -1628,7 +1587,6 @@ static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,
>  	struct srp_tsk_mgmt *srp_tsk;
>  	struct se_cmd *cmd;
>  	struct se_session *sess = ch->sess;
> -	uint32_t tag = 0;
>  	int tcm_tmr;
>  	int rc;
> 
> @@ -1649,18 +1607,10 @@ static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,
>  			TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED;
>  		goto fail;
>  	}
> -	if (srp_tsk->tsk_mgmt_func == SRP_TSK_ABORT_TASK) {
> -		rc = srpt_rx_mgmt_fn_tag(send_ioctx, srp_tsk->task_tag);
> -		if (rc < 0) {
> -			send_ioctx->cmd.se_tmr_req->response =
                                    ^^^^^^^^^^^^^^^^^^^^   !!!
se_tmr_req pointer was cleared a few lines of code earlier:

srpt_handle_new_iu() {
	.....
	send_ioctx = srpt_get_send_ioctx(ch) {
				.....
				memset(&ioctx->cmd, 0 ... !!! re-init se_cmd structure for response.
			}
	...
	srpt_handle_tsk_mgmt(ch, recv_ioctx, send_ioctx) {
	...
	And here we got it !!!
		send_ioctx->cmd.se_tmr_req->response =   
					TMR_TASK_DOES_NOT_EXIST;

> -			goto fail;
> -		}
> -		tag = srp_tsk->task_tag;
> -	}
>  	rc = target_submit_tmr(&send_ioctx->cmd, sess, NULL,
>  			       scsilun_to_int(&srp_tsk->lun), srp_tsk, tcm_tmr,
> -			       GFP_KERNEL, tag, TARGET_SCF_ACK_KREF);
> +			       GFP_KERNEL, srp_tsk->task_tag,
> +			       TARGET_SCF_ACK_KREF);
>  	if (rc != 0) {
>  		send_ioctx->cmd.se_tmr_req->response = TMR_FUNCTION_REJECTED;
>  		goto fail;
> --
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 06/15] IB/srpt: Simplify srpt_handle_tsk_mgmt()
       [not found]         ` <F3529576D8E232409F431C309E29399328F6B06A-8k97q/ur5Z1cIJlls4ac1rfspsVTdybXVpNB7YpNyf8@public.gmane.org>
@ 2016-01-26 17:32           ` Bart Van Assche
       [not found]             ` <56A7ADA3.5080507-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  0 siblings, 1 reply; 57+ messages in thread
From: Bart Van Assche @ 2016-01-26 17:32 UTC (permalink / raw)
  To: Estrin, Alex, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On 01/26/16 08:57, Estrin, Alex wrote:
> Is it possible to expedite this patch as it actually fixes a real bug in ib_srpt?
>
> If srp target receives ABORT TASK, it will crash the kernel while trying to respond.
> We were able to reproduce it under quite heavy load [ ... ]

Hello Alex,

Thank you for the feedback. Since the kernel 4.5 merge has been closed I 
will retest and resend the whole patch series. I would like to see all 
of these patches to be integrated in the upstream kernel instead of only 
this single patch. I will Cc stable and I will also add your Tested-by tag.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 57+ messages in thread

* RE: [PATCH 06/15] IB/srpt: Simplify srpt_handle_tsk_mgmt()
       [not found]             ` <56A7ADA3.5080507-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
@ 2016-01-26 18:35               ` Estrin, Alex
       [not found]                 ` <F3529576D8E232409F431C309E29399328F6B0C3-8k97q/ur5Z1cIJlls4ac1rfspsVTdybXVpNB7YpNyf8@public.gmane.org>
  0 siblings, 1 reply; 57+ messages in thread
From: Estrin, Alex @ 2016-01-26 18:35 UTC (permalink / raw)
  To: Bart Van Assche, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

> 
> Thank you for the feedback. Since the kernel 4.5 merge has been closed I
> will retest and resend the whole patch series. 

Bart,

In this case I would suggest to extend the patch by removing
this unlikely check, since it is also taken care of in target:

@@ -1843,25 +1801,10 @@ static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,
        srpt_set_cmd_state(send_ioctx, SRPT_STATE_MGMT);
        send_ioctx->tag = srp_tsk->tag;
        tcm_tmr = srp_tmr_to_tcm(srp_tsk->tsk_mgmt_func);
-       if (tcm_tmr < 0) {
-               send_ioctx->cmd.se_tmr_req->response =
-                       TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED;
-               goto fail;
-       }

Thanks,
Alex.

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 06/15] IB/srpt: Simplify srpt_handle_tsk_mgmt()
       [not found]                 ` <F3529576D8E232409F431C309E29399328F6B0C3-8k97q/ur5Z1cIJlls4ac1rfspsVTdybXVpNB7YpNyf8@public.gmane.org>
@ 2016-01-26 18:46                   ` Bart Van Assche
       [not found]                     ` <56A7BF23.1060306-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  0 siblings, 1 reply; 57+ messages in thread
From: Bart Van Assche @ 2016-01-26 18:46 UTC (permalink / raw)
  To: Estrin, Alex, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On 01/26/16 10:35, Estrin, Alex wrote:
>> Thank you for the feedback. Since the kernel 4.5 merge has been closed I
>> will retest and resend the whole patch series.
> 
> Bart,
> 
> In this case I would suggest to extend the patch by removing
> this unlikely check, since it is also taken care of in target:
> 
> @@ -1843,25 +1801,10 @@ static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,
>          srpt_set_cmd_state(send_ioctx, SRPT_STATE_MGMT);
>          send_ioctx->tag = srp_tsk->tag;
>          tcm_tmr = srp_tmr_to_tcm(srp_tsk->tsk_mgmt_func);
> -       if (tcm_tmr < 0) {
> -               send_ioctx->cmd.se_tmr_req->response =
> -                       TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED;
> -               goto fail;
> -       }

Hello Alex,

How about introducing a symbolic constant for unrecognized task management
functions, e.g. as in the below variant of your patch ?

---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 7 +------
 include/target/target_core_base.h     | 1 +
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 21cdb7e..352dd87 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1595,7 +1595,7 @@ static int srp_tmr_to_tcm(int fn)
 	case SRP_TSK_CLEAR_ACA:
 		return TMR_CLEAR_ACA;
 	default:
-		return -1;
+		return UNKNOWN_TMR;
 	}
 }
 
@@ -1629,11 +1629,6 @@ static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,
 	srpt_set_cmd_state(send_ioctx, SRPT_STATE_MGMT);
 	send_ioctx->cmd.tag = srp_tsk->tag;
 	tcm_tmr = srp_tmr_to_tcm(srp_tsk->tsk_mgmt_func);
-	if (tcm_tmr < 0) {
-		send_ioctx->cmd.se_tmr_req->response =
-			TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED;
-		goto fail;
-	}
 	rc = target_submit_tmr(&send_ioctx->cmd, sess, NULL,
 			       scsilun_to_int(&srp_tsk->lun), srp_tsk, tcm_tmr,
 			       GFP_KERNEL, srp_tsk->task_tag,
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index ec2ddbf..2fb005a 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -192,6 +192,7 @@ enum target_sc_flags_table {
 
 /* fabric independent task management function values */
 enum tcm_tmreq_table {
+	UNKNOWN_TMR		= -1,
 	TMR_ABORT_TASK		= 1,
 	TMR_ABORT_TASK_SET	= 2,
 	TMR_CLEAR_ACA		= 3,
-- 
2.7.0


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 57+ messages in thread

* RE: [PATCH 06/15] IB/srpt: Simplify srpt_handle_tsk_mgmt()
       [not found]                     ` <56A7BF23.1060306-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
@ 2016-01-26 18:52                       ` Estrin, Alex
  0 siblings, 0 replies; 57+ messages in thread
From: Estrin, Alex @ 2016-01-26 18:52 UTC (permalink / raw)
  To: Bart Van Assche, Doug Ledford
  Cc: Christoph Hellwig, linux-rdma-u79uwXL29TY76Z2rM5mHXA

> 
> On 01/26/16 10:35, Estrin, Alex wrote:
> >> Thank you for the feedback. Since the kernel 4.5 merge has been closed I
> >> will retest and resend the whole patch series.
> >
> > Bart,
> >
> > In this case I would suggest to extend the patch by removing
> > this unlikely check, since it is also taken care of in target:
> >
> > @@ -1843,25 +1801,10 @@ static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,
> >          srpt_set_cmd_state(send_ioctx, SRPT_STATE_MGMT);
> >          send_ioctx->tag = srp_tsk->tag;
> >          tcm_tmr = srp_tmr_to_tcm(srp_tsk->tsk_mgmt_func);
> > -       if (tcm_tmr < 0) {
> > -               send_ioctx->cmd.se_tmr_req->response =
> > -                       TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED;
> > -               goto fail;
> > -       }
> 
> Hello Alex,
> 
> How about introducing a symbolic constant for unrecognized task management
> functions, e.g. as in the below variant of your patch ?
> 
> ---
>  drivers/infiniband/ulp/srpt/ib_srpt.c | 7 +------
>  include/target/target_core_base.h     | 1 +
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c
> b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index 21cdb7e..352dd87 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -1595,7 +1595,7 @@ static int srp_tmr_to_tcm(int fn)
>  	case SRP_TSK_CLEAR_ACA:
>  		return TMR_CLEAR_ACA;
>  	default:
> -		return -1;
> +		return UNKNOWN_TMR;
>  	}
>  }
> 
> @@ -1629,11 +1629,6 @@ static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,
>  	srpt_set_cmd_state(send_ioctx, SRPT_STATE_MGMT);
>  	send_ioctx->cmd.tag = srp_tsk->tag;
>  	tcm_tmr = srp_tmr_to_tcm(srp_tsk->tsk_mgmt_func);
> -	if (tcm_tmr < 0) {
> -		send_ioctx->cmd.se_tmr_req->response =
> -			TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED;
> -		goto fail;
> -	}
>  	rc = target_submit_tmr(&send_ioctx->cmd, sess, NULL,
>  			       scsilun_to_int(&srp_tsk->lun), srp_tsk, tcm_tmr,
>  			       GFP_KERNEL, srp_tsk->task_tag,
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index ec2ddbf..2fb005a 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -192,6 +192,7 @@ enum target_sc_flags_table {
> 
>  /* fabric independent task management function values */
>  enum tcm_tmreq_table {
> +	UNKNOWN_TMR		= -1,
>  	TMR_ABORT_TASK		= 1,
>  	TMR_ABORT_TASK_SET	= 2,
>  	TMR_CLEAR_ACA		= 3,
> --
> 2.7.0
> 

Good idea.

Thanks,
Alex.

^ permalink raw reply	[flat|nested] 57+ messages in thread

end of thread, other threads:[~2016-01-26 18:52 UTC | newest]

Thread overview: 57+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-05 14:19 [PATCH 00/15] Various ib_srpt patches Bart Van Assche
     [not found] ` <568BD0FC.70207-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
2016-01-05 14:20   ` [PATCH 01/15] IB/srpt: Add parentheses around sizeof argument Bart Van Assche
     [not found]     ` <568BD142.3070900-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
2016-01-06  4:58       ` Christoph Hellwig
2016-01-06 13:58       ` Sagi Grimberg
2016-01-05 14:21   ` [PATCH 02/15] IB/srpt: Inline srpt_sdev_name() Bart Van Assche
     [not found]     ` <568BD15F.6010909-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
2016-01-06  4:58       ` Christoph Hellwig
2016-01-06 13:58       ` Sagi Grimberg
2016-01-05 14:21   ` [PATCH 03/15] IB/srpt: Inline srpt_get_ch_state() Bart Van Assche
     [not found]     ` <568BD181.5040009-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
2016-01-06  4:58       ` Christoph Hellwig
2016-01-06 13:59       ` Sagi Grimberg
2016-01-05 14:22   ` [PATCH 04/15] IB/srpt: Introduce target_reverse_dma_direction() Bart Van Assche
     [not found]     ` <568BD19C.6060404-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
2016-01-06  4:59       ` Christoph Hellwig
2016-01-06 14:00       ` Sagi Grimberg
2016-01-05 14:22   ` [PATCH 05/15] IB/srpt: Use scsilun_to_int() Bart Van Assche
     [not found]     ` <568BD1B6.4090308-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
2016-01-06  4:59       ` Christoph Hellwig
2016-01-06 14:01       ` Sagi Grimberg
2016-01-05 14:23   ` [PATCH 06/15] IB/srpt: Simplify srpt_handle_tsk_mgmt() Bart Van Assche
     [not found]     ` <568BD1D2.1030609-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
2016-01-06  5:00       ` Christoph Hellwig
2016-01-06 14:02       ` Sagi Grimberg
2016-01-26 16:57       ` Estrin, Alex
     [not found]         ` <F3529576D8E232409F431C309E29399328F6B06A-8k97q/ur5Z1cIJlls4ac1rfspsVTdybXVpNB7YpNyf8@public.gmane.org>
2016-01-26 17:32           ` Bart Van Assche
     [not found]             ` <56A7ADA3.5080507-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
2016-01-26 18:35               ` Estrin, Alex
     [not found]                 ` <F3529576D8E232409F431C309E29399328F6B0C3-8k97q/ur5Z1cIJlls4ac1rfspsVTdybXVpNB7YpNyf8@public.gmane.org>
2016-01-26 18:46                   ` Bart Van Assche
     [not found]                     ` <56A7BF23.1060306-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
2016-01-26 18:52                       ` Estrin, Alex
2016-01-05 14:23   ` [PATCH 07/15] IB/srpt: Simplify channel state management Bart Van Assche
     [not found]     ` <568BD1F1.5050107-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
2016-01-06  5:01       ` Christoph Hellwig
2016-01-06 14:08       ` Sagi Grimberg
2016-01-05 14:24   ` [PATCH 08/15] IB/srpt: Simplify srpt_shutdown_session() Bart Van Assche
     [not found]     ` <568BD20F.7020101-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
2016-01-06  5:07       ` Christoph Hellwig
2016-01-06 14:13       ` Sagi Grimberg
2016-01-05 14:24   ` [PATCH 09/15] IB/srpt: Fix srpt_close_session() Bart Van Assche
     [not found]     ` <568BD231.10205-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
2016-01-06  5:08       ` Christoph Hellwig
2016-01-06 14:21       ` Sagi Grimberg
     [not found]         ` <568D22F6.4050803-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
2016-01-06 14:34           ` Bart Van Assche
     [not found]             ` <568D25D9.3050809-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
2016-01-06 14:46               ` Sagi Grimberg
2016-01-05 14:25   ` [PATCH 10/15] IB/srpt: Fix srpt_handle_cmd() error paths Bart Van Assche
     [not found]     ` <568BD249.1000100-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
2016-01-06  5:09       ` Christoph Hellwig
2016-01-06 14:31       ` Sagi Grimberg
     [not found]         ` <568D2553.8050302-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
2016-01-06 14:36           ` Bart Van Assche
2016-01-05 14:25   ` [PATCH 11/15] IB/srpt: Fix how aborted commands are processed Bart Van Assche
     [not found]     ` <568BD26D.9080003-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
2016-01-06  5:13       ` Christoph Hellwig
     [not found]         ` <20160106051305.GK15574-jcswGhMUV9g@public.gmane.org>
2016-01-06 13:30           ` Bart Van Assche
2016-01-05 14:26   ` [PATCH 12/15] IB/srpt: Eliminate srpt_find_channel() Bart Van Assche
     [not found]     ` <568BD28E.40302-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
2016-01-06  5:15       ` Christoph Hellwig
2016-01-06 14:33       ` Sagi Grimberg
2016-01-05 14:26   ` [PATCH 13/15] IB/srpt: Detect session shutdown reliably Bart Van Assche
     [not found]     ` <568BD2A9.6070600-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
2016-01-06  5:21       ` Christoph Hellwig
     [not found]         ` <20160106052138.GM15574-jcswGhMUV9g@public.gmane.org>
2016-01-06 13:34           ` Bart Van Assche
2016-01-06 14:39       ` Sagi Grimberg
     [not found]         ` <568D2707.9020309-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
2016-01-06 14:46           ` Bart Van Assche
     [not found]             ` <568D28CA.8010804-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
2016-01-06 17:04               ` Christoph Hellwig
2016-01-05 14:27   ` [PATCH 14/15] IB/srpt: Fix srpt_write_pending() Bart Van Assche
     [not found]     ` <568BD2C2.6090808-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
2016-01-06  5:22       ` Christoph Hellwig
2016-01-06 14:41       ` Sagi Grimberg
2016-01-05 14:27   ` [PATCH 15/15] IB/srpt: Fix a rare crash in srpt_close_session() Bart Van Assche
     [not found]     ` <568BD2E1.1060701-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
2016-01-06  5:23       ` Christoph Hellwig
2016-01-06 14:47       ` Sagi Grimberg

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.