[PATCH v3 0/6] connect reject event helpers

[PATCH v3 0/6] connect reject event helpers

[Steve Wise]

While reviewing:

http://lists.infradead.org/pipermail/linux-nvme/2016-October/006681.html

I decided to propose transport-agnostic helper functions to better
handle connection reject event information.  Included are patches
for nvme_rdma, ib_ser, and rds_rdma to utilize the new helpers.

Changes since v2:

- reworked ibcm/iwcm_reject_msg() as per Christoph's recommendation
- use ibcm_ and iwcm_ prefix instead of ib_ and iw_ for reject_msg funcs
- change rdma_consumer_reject() to rdma_is_consumer_reject()
- add rdma_consumer_reject_data() helper function to return private
  data/len
- use new helpers in nvme_rdma, ib_iser, and rdma_rds
- in nvme_rdma, add strings for nvme_rdma_cm_status values

---

Steve Wise (6):
  rdma_cm: add rdma_reject_msg() helper function
  rdma_cm: add rdma_is_consumer_reject() helper function
  rdma_cm: add rdma_consumer_reject_data helper function
  nvme-rdma: use rdma connection reject helper functions
  ib_iser: log the connection reject message
  rds_rdma: log the connection reject message

 drivers/infiniband/core/cm.c             | 48 ++++++++++++++++++++++++++++++++
 drivers/infiniband/core/cma.c            | 43 ++++++++++++++++++++++++++++
 drivers/infiniband/core/iwcm.c           | 21 ++++++++++++++
 drivers/infiniband/ulp/iser/iser_verbs.c |  5 +++-
 drivers/nvme/host/rdma.c                 | 46 ++++++++++++++++++++++++------
 include/rdma/ib_cm.h                     |  6 ++++
 include/rdma/iw_cm.h                     |  6 ++++
 include/rdma/rdma_cm.h                   | 25 +++++++++++++++++
 net/rds/rdma_transport.c                 |  5 +++-
 9 files changed, 195 insertions(+), 10 deletions(-)

-- 
2.7.0

[PATCH v3 3/6] rdma_cm: add rdma_consumer_reject_data helper function

[Steve Wise]

rdma_consumer_reject_data() will return the private data pointer
and length if any is available.

Signed-off-by: Steve Wise <swise at opengridcomputing.com>
---
 drivers/infiniband/core/cma.c | 16 ++++++++++++++++
 include/rdma/rdma_cm.h        | 10 ++++++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 0d88c9d..2536d42 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -128,6 +128,22 @@ bool rdma_is_consumer_reject(struct rdma_cm_id *id, int reason)
 }
 EXPORT_SYMBOL(rdma_is_consumer_reject);
 
+const void *rdma_consumer_reject_data(struct rdma_cm_id *id,
+				      struct rdma_cm_event *ev, u8 *data_len)
+{
+	void *p;
+
+	if (rdma_is_consumer_reject(id, ev->status)) {
+		*data_len = ev->param.conn.private_data_len;
+		p = ev->param.conn.private_data;
+	} else {
+		*data_len = 0;
+		p = NULL;
+	}
+	return p;
+}
+EXPORT_SYMBOL(rdma_consumer_reject_data);
+
 static void cma_add_one(struct ib_device *device);
 static void cma_remove_one(struct ib_device *device, void *client_data);
 
diff --git a/include/rdma/rdma_cm.h b/include/rdma/rdma_cm.h
index 62039c2..d3968b5 100644
--- a/include/rdma/rdma_cm.h
+++ b/include/rdma/rdma_cm.h
@@ -403,4 +403,14 @@ const char *__attribute_const__ rdma_reject_msg(struct rdma_cm_id *id,
  */
 bool rdma_is_consumer_reject(struct rdma_cm_id *id, int reason);
 
+/**
+ * rdma_consumer_reject_data - return the consumer reject private data and
+ *			       length, if any.
+ * @id: Communication identifier that received the REJECT event.
+ * @ev: RDMA CM reject event.
+ * @data_len: Pointer to the resulting length of the consumer data.
+ */
+const void *rdma_consumer_reject_data(struct rdma_cm_id *id,
+				      struct rdma_cm_event *ev, u8 *data_len);
+
 #endif /* RDMA_CM_H */
-- 
2.7.0

[PATCH v3 3/6] rdma_cm: add rdma_consumer_reject_data helper function

[Sagi Grimberg]

Looks good,

Reviewed-by: Sagi Grimberg <sagi at grimbeg.me>

[PATCH v3 3/6] rdma_cm: add rdma_consumer_reject_data helper function

[Christoph Hellwig]

On Mon, Oct 24, 2016@11:59:03AM -0700, Steve Wise wrote:
> rdma_consumer_reject_data() will return the private data pointer
> and length if any is available.
> 
> Signed-off-by: Steve Wise <swise at opengridcomputing.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH v3 2/6] rdma_cm: add rdma_is_consumer_reject() helper function

[Steve Wise]

Return true if the peer consumer application rejected the
connection attempt.

Signed-off-by: Steve Wise <swise at opengridcomputing.com>
---
 drivers/infiniband/core/cma.c | 13 +++++++++++++
 include/rdma/rdma_cm.h        |  7 +++++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 831e94b..0d88c9d 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -115,6 +115,19 @@ const char *__attribute_const__ rdma_reject_msg(struct rdma_cm_id *id,
 }
 EXPORT_SYMBOL(rdma_reject_msg);
 
+bool rdma_is_consumer_reject(struct rdma_cm_id *id, int reason)
+{
+	if (rdma_ib_or_roce(id->device, id->port_num))
+		return reason == IB_CM_REJ_CONSUMER_DEFINED;
+
+	if (rdma_protocol_iwarp(id->device, id->port_num))
+		return reason == -ECONNREFUSED;
+
+	WARN_ON_ONCE(1);
+	return false;
+}
+EXPORT_SYMBOL(rdma_is_consumer_reject);
+
 static void cma_add_one(struct ib_device *device);
 static void cma_remove_one(struct ib_device *device, void *client_data);
 
diff --git a/include/rdma/rdma_cm.h b/include/rdma/rdma_cm.h
index f11a768..62039c2 100644
--- a/include/rdma/rdma_cm.h
+++ b/include/rdma/rdma_cm.h
@@ -395,5 +395,12 @@ __be64 rdma_get_service_id(struct rdma_cm_id *id, struct sockaddr *addr);
  */
 const char *__attribute_const__ rdma_reject_msg(struct rdma_cm_id *id,
 						int reason);
+/**
+ * rdma_is_consumer_reject - return true if the consumer rejected the connect
+ *                           request.
+ * @id: Communication identifier that received the REJECT event.
+ * @reason: Value returned in the REJECT event status field.
+ */
+bool rdma_is_consumer_reject(struct rdma_cm_id *id, int reason);
 
 #endif /* RDMA_CM_H */
-- 
2.7.0

[PATCH v3 2/6] rdma_cm: add rdma_is_consumer_reject() helper function

[Sagi Grimberg]

Looks good,

Reviewed-by: Sagi Grimberg <sagi at grimbeg.me>

[PATCH v3 2/6] rdma_cm: add rdma_is_consumer_reject() helper function

[Christoph Hellwig]

On Mon, Oct 24, 2016@11:59:03AM -0700, Steve Wise wrote:
> Return true if the peer consumer application rejected the
> connection attempt.
> 
> Signed-off-by: Steve Wise <swise at opengridcomputing.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH v3 1/6] rdma_cm: add rdma_reject_msg() helper function

[Steve Wise]

rdma_reject_msg() returns a pointer to a string message associated with
the transport reject reason codes.

Signed-off-by: Steve Wise <swise at opengridcomputing.com>
---
 drivers/infiniband/core/cm.c   | 48 ++++++++++++++++++++++++++++++++++++++++++
 drivers/infiniband/core/cma.c  | 14 ++++++++++++
 drivers/infiniband/core/iwcm.c | 21 ++++++++++++++++++
 include/rdma/ib_cm.h           |  6 ++++++
 include/rdma/iw_cm.h           |  6 ++++++
 include/rdma/rdma_cm.h         |  8 +++++++
 6 files changed, 103 insertions(+)

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index c995255..98bcc9c 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -57,6 +57,54 @@ MODULE_AUTHOR("Sean Hefty");
 MODULE_DESCRIPTION("InfiniBand CM");
 MODULE_LICENSE("Dual BSD/GPL");
 
+static const char * const ibcm_rej_reason_strs[] = {
+	[IB_CM_REJ_NO_QP]			= "no qp",
+	[IB_CM_REJ_NO_EEC]			= "no eec",
+	[IB_CM_REJ_NO_RESOURCES]		= "no resources",
+	[IB_CM_REJ_TIMEOUT]			= "timeout",
+	[IB_CM_REJ_UNSUPPORTED]			= "unsupported",
+	[IB_CM_REJ_INVALID_COMM_ID]		= "invalid comm id",
+	[IB_CM_REJ_INVALID_COMM_INSTANCE]	= "invalid comm instance",
+	[IB_CM_REJ_INVALID_SERVICE_ID]		= "invalid service id",
+	[IB_CM_REJ_INVALID_TRANSPORT_TYPE]	= "invalid transport type",
+	[IB_CM_REJ_STALE_CONN]			= "stale conn",
+	[IB_CM_REJ_RDC_NOT_EXIST]		= "rdc not exist",
+	[IB_CM_REJ_INVALID_GID]			= "invalid gid",
+	[IB_CM_REJ_INVALID_LID]			= "invalid lid",
+	[IB_CM_REJ_INVALID_SL]			= "invalid sl",
+	[IB_CM_REJ_INVALID_TRAFFIC_CLASS]	= "invalid traffic class",
+	[IB_CM_REJ_INVALID_HOP_LIMIT]		= "invalid hop limit",
+	[IB_CM_REJ_INVALID_PACKET_RATE]		= "invalid packet rate",
+	[IB_CM_REJ_INVALID_ALT_GID]		= "invalid alt gid",
+	[IB_CM_REJ_INVALID_ALT_LID]		= "invalid alt lid",
+	[IB_CM_REJ_INVALID_ALT_SL]		= "invalid alt sl",
+	[IB_CM_REJ_INVALID_ALT_TRAFFIC_CLASS]	= "invalid alt traffic class",
+	[IB_CM_REJ_INVALID_ALT_HOP_LIMIT]	= "invalid alt hop limit",
+	[IB_CM_REJ_INVALID_ALT_PACKET_RATE]	= "invalid alt packet rate",
+	[IB_CM_REJ_PORT_CM_REDIRECT]		= "port cm redirect",
+	[IB_CM_REJ_PORT_REDIRECT]		= "port redirect",
+	[IB_CM_REJ_INVALID_MTU]			= "invalid mtu",
+	[IB_CM_REJ_INSUFFICIENT_RESP_RESOURCES]	= "insufficient resp resources",
+	[IB_CM_REJ_CONSUMER_DEFINED]		= "consumer defined",
+	[IB_CM_REJ_INVALID_RNR_RETRY]		= "invalid rnr retry",
+	[IB_CM_REJ_DUPLICATE_LOCAL_COMM_ID]	= "duplicate local comm id",
+	[IB_CM_REJ_INVALID_CLASS_VERSION]	= "invalid class version",
+	[IB_CM_REJ_INVALID_FLOW_LABEL]		= "invalid flow label",
+	[IB_CM_REJ_INVALID_ALT_FLOW_LABEL]	= "invalid alt flow label",
+};
+
+const char *__attribute_const__ ibcm_reject_msg(int reason)
+{
+	size_t index = reason;
+
+	if (index >= ARRAY_SIZE(ibcm_rej_reason_strs) ||
+	    !ibcm_rej_reason_strs[index])
+		return "unrecognized reason";
+	else
+		return ibcm_rej_reason_strs[index];
+}
+EXPORT_SYMBOL(ibcm_reject_msg);
+
 static void cm_add_one(struct ib_device *device);
 static void cm_remove_one(struct ib_device *device, void *client_data);
 
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 5f65a78..831e94b 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -101,6 +101,20 @@ const char *__attribute_const__ rdma_event_msg(enum rdma_cm_event_type event)
 }
 EXPORT_SYMBOL(rdma_event_msg);
 
+const char *__attribute_const__ rdma_reject_msg(struct rdma_cm_id *id,
+						int reason)
+{
+	if (rdma_ib_or_roce(id->device, id->port_num))
+		return ibcm_reject_msg(reason);
+
+	if (rdma_protocol_iwarp(id->device, id->port_num))
+		return iwcm_reject_msg(reason);
+
+	WARN_ON_ONCE(1);
+	return "unrecognized reason";
+}
+EXPORT_SYMBOL(rdma_reject_msg);
+
 static void cma_add_one(struct ib_device *device);
 static void cma_remove_one(struct ib_device *device, void *client_data);
 
diff --git a/drivers/infiniband/core/iwcm.c b/drivers/infiniband/core/iwcm.c
index 357624f..8d01253 100644
--- a/drivers/infiniband/core/iwcm.c
+++ b/drivers/infiniband/core/iwcm.c
@@ -59,6 +59,27 @@ MODULE_AUTHOR("Tom Tucker");
 MODULE_DESCRIPTION("iWARP CM");
 MODULE_LICENSE("Dual BSD/GPL");
 
+static const char * const iwcm_rej_reason_strs[] = {
+	[ECONNRESET]			= "reset by remote host",
+	[ECONNREFUSED]			= "refused by remote application",
+	[ETIMEDOUT]			= "setup timeout",
+};
+
+const char *__attribute_const__ iwcm_reject_msg(int reason)
+{
+	size_t index;
+
+	/* iWARP uses negative errnos */
+	index = -reason;
+
+	if (index >= ARRAY_SIZE(iwcm_rej_reason_strs) ||
+	    !iwcm_rej_reason_strs[index])
+		return "unrecognized reason";
+	else
+		return iwcm_rej_reason_strs[index];
+}
+EXPORT_SYMBOL(iwcm_reject_msg);
+
 static struct ibnl_client_cbs iwcm_nl_cb_table[] = {
 	[RDMA_NL_IWPM_REG_PID] = {.dump = iwpm_register_pid_cb},
 	[RDMA_NL_IWPM_ADD_MAPPING] = {.dump = iwpm_add_mapping_cb},
diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h
index 92a7d85..b49258b 100644
--- a/include/rdma/ib_cm.h
+++ b/include/rdma/ib_cm.h
@@ -603,4 +603,10 @@ struct ib_cm_sidr_rep_param {
 int ib_send_cm_sidr_rep(struct ib_cm_id *cm_id,
 			struct ib_cm_sidr_rep_param *param);
 
+/**
+ * ibcm_reject_msg - return a pointer to a reject message string.
+ * @reason: Value returned in the REJECT event status field.
+ */
+const char *__attribute_const__ ibcm_reject_msg(int reason);
+
 #endif /* IB_CM_H */
diff --git a/include/rdma/iw_cm.h b/include/rdma/iw_cm.h
index 6d0065c..5cd7701 100644
--- a/include/rdma/iw_cm.h
+++ b/include/rdma/iw_cm.h
@@ -253,4 +253,10 @@ int iw_cm_disconnect(struct iw_cm_id *cm_id, int abrupt);
 int iw_cm_init_qp_attr(struct iw_cm_id *cm_id, struct ib_qp_attr *qp_attr,
 		       int *qp_attr_mask);
 
+/**
+ * iwcm_reject_msg - return a pointer to a reject message string.
+ * @reason: Value returned in the REJECT event status field.
+ */
+const char *__attribute_const__ iwcm_reject_msg(int reason);
+
 #endif /* IW_CM_H */
diff --git a/include/rdma/rdma_cm.h b/include/rdma/rdma_cm.h
index 81fb1d1..f11a768 100644
--- a/include/rdma/rdma_cm.h
+++ b/include/rdma/rdma_cm.h
@@ -388,4 +388,12 @@ int rdma_set_afonly(struct rdma_cm_id *id, int afonly);
  */
 __be64 rdma_get_service_id(struct rdma_cm_id *id, struct sockaddr *addr);
 
+/**
+ * rdma_reject_msg - return a pointer to a reject message string.
+ * @id: Communication identifier that received the REJECT event.
+ * @reason: Value returned in the REJECT event status field.
+ */
+const char *__attribute_const__ rdma_reject_msg(struct rdma_cm_id *id,
+						int reason);
+
 #endif /* RDMA_CM_H */
-- 
2.7.0

[PATCH v3 1/6] rdma_cm: add rdma_reject_msg() helper function

[Sagi Grimberg]

Looks good,

Reviewed-by: Sagi Grimberg <sagi at grimbeg.me>

[PATCH v3 1/6] rdma_cm: add rdma_reject_msg() helper function

[Christoph Hellwig]

On Mon, Oct 24, 2016@11:59:03AM -0700, Steve Wise wrote:
> rdma_reject_msg() returns a pointer to a string message associated with
> the transport reject reason codes.
> 
> Signed-off-by: Steve Wise <swise at opengridcomputing.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch at lst.de>

(and /me notes the difference in error numbers between iWarp and the IB
universe..  Stunning)

[PATCH v3 1/6] rdma_cm: add rdma_reject_msg() helper function

[Bart Van Assche]

On 10/24/2016 12:09 PM, Steve Wise wrote:
> +	[IB_CM_REJ_RDC_NOT_EXIST]		= "rdc not exist",

Please change this into "RDC does not exist".

> +	[IB_CM_REJ_INVALID_GID]			= "invalid gid",
> +	[IB_CM_REJ_INVALID_LID]			= "invalid lid",
> +	[IB_CM_REJ_INVALID_SL]			= "invalid sl",
> +	[IB_CM_REJ_INVALID_TRAFFIC_CLASS]	= "invalid traffic class",
> +	[IB_CM_REJ_INVALID_HOP_LIMIT]		= "invalid hop limit",
> +	[IB_CM_REJ_INVALID_PACKET_RATE]		= "invalid packet rate",
> +	[IB_CM_REJ_INVALID_ALT_GID]		= "invalid alt gid",
> +	[IB_CM_REJ_INVALID_ALT_LID]		= "invalid alt lid",
> +	[IB_CM_REJ_INVALID_ALT_SL]		= "invalid alt sl",
> +	[IB_CM_REJ_INVALID_ALT_TRAFFIC_CLASS]	= "invalid alt traffic class",
> +	[IB_CM_REJ_INVALID_ALT_HOP_LIMIT]	= "invalid alt hop limit",
> +	[IB_CM_REJ_INVALID_ALT_PACKET_RATE]	= "invalid alt packet rate",
> +	[IB_CM_REJ_PORT_CM_REDIRECT]		= "port cm redirect",
> +	[IB_CM_REJ_PORT_REDIRECT]		= "port redirect",
> +	[IB_CM_REJ_INVALID_MTU]			= "invalid mtu",
> +	[IB_CM_REJ_INSUFFICIENT_RESP_RESOURCES]	= "insufficient resp resources",
> +	[IB_CM_REJ_CONSUMER_DEFINED]		= "consumer defined",
> +	[IB_CM_REJ_INVALID_RNR_RETRY]		= "invalid rnr retry",
> +	[IB_CM_REJ_DUPLICATE_LOCAL_COMM_ID]	= "duplicate local comm id",
> +	[IB_CM_REJ_INVALID_CLASS_VERSION]	= "invalid class version",
> +	[IB_CM_REJ_INVALID_FLOW_LABEL]		= "invalid flow label",
> +	[IB_CM_REJ_INVALID_ALT_FLOW_LABEL]	= "invalid alt flow label",

Other error messages capitalize GID, LID, CM, ID, RNR and SL so please 
do this here too.

> +const char *__attribute_const__ ibcm_reject_msg(int reason)
> +{
> +	size_t index = reason;
> +
> +	if (index >= ARRAY_SIZE(ibcm_rej_reason_strs) ||
> +	    !ibcm_rej_reason_strs[index])
> +		return "unrecognized reason";
> +	else
> +		return ibcm_rej_reason_strs[index];
> +}

Please consider using positive logic - this means negating the 
if-condition and swapping the if- and else-parts.

> +const char *__attribute_const__ rdma_reject_msg(struct rdma_cm_id *id,
> +						int reason)
> +{
> +	if (rdma_ib_or_roce(id->device, id->port_num))
> +		return ibcm_reject_msg(reason);
> +
> +	if (rdma_protocol_iwarp(id->device, id->port_num))
> +		return iwcm_reject_msg(reason);
> +
> +	WARN_ON_ONCE(1);
> +	return "unrecognized reason";

Have you considered to return "unrecognized transport" here instead?

> +const char *__attribute_const__ iwcm_reject_msg(int reason)
> +{
> +	size_t index;
> +
> +	/* iWARP uses negative errnos */
> +	index = -reason;
> +
> +	if (index >= ARRAY_SIZE(iwcm_rej_reason_strs) ||
> +	    !iwcm_rej_reason_strs[index])
> +		return "unrecognized reason";
> +	else
> +		return iwcm_rej_reason_strs[index];
> +}

Also for this function, please consider using positive logic.

Thanks,

Bart.

[PATCH v3 1/6] rdma_cm: add rdma_reject_msg() helper function

[Steve Wise]

> -----Original Message-----
> From: Bart Van Assche [mailto:bart.vanassche at sandisk.com]
> Sent: Tuesday, October 25, 2016 12:58 PM
> To: Steve Wise; dledford at redhat.com; sean.hefty at intel.com
> Cc: linux-rdma at vger.kernel.org; linux-nvme at lists.infradead.org;
> sagi at grimberg.me; hch at lst.de; axboe at fb.com; santosh.shilimkar at oracle.com
> Subject: Re: [PATCH v3 1/6] rdma_cm: add rdma_reject_msg() helper function
> 
> On 10/24/2016 12:09 PM, Steve Wise wrote:
> > +	[IB_CM_REJ_RDC_NOT_EXIST]		= "rdc not exist",
> 
> Please change this into "RDC does not exist".
>

ok
 
> > +	[IB_CM_REJ_INVALID_GID]			= "invalid gid",
> > +	[IB_CM_REJ_INVALID_LID]			= "invalid lid",
> > +	[IB_CM_REJ_INVALID_SL]			= "invalid sl",
> > +	[IB_CM_REJ_INVALID_TRAFFIC_CLASS]	= "invalid traffic class",
> > +	[IB_CM_REJ_INVALID_HOP_LIMIT]		= "invalid hop limit",
> > +	[IB_CM_REJ_INVALID_PACKET_RATE]		= "invalid packet rate",
> > +	[IB_CM_REJ_INVALID_ALT_GID]		= "invalid alt gid",
> > +	[IB_CM_REJ_INVALID_ALT_LID]		= "invalid alt lid",
> > +	[IB_CM_REJ_INVALID_ALT_SL]		= "invalid alt sl",
> > +	[IB_CM_REJ_INVALID_ALT_TRAFFIC_CLASS]	= "invalid alt traffic class",
> > +	[IB_CM_REJ_INVALID_ALT_HOP_LIMIT]	= "invalid alt hop limit",
> > +	[IB_CM_REJ_INVALID_ALT_PACKET_RATE]	= "invalid alt packet rate",
> > +	[IB_CM_REJ_PORT_CM_REDIRECT]		= "port cm redirect",
> > +	[IB_CM_REJ_PORT_REDIRECT]		= "port redirect",
> > +	[IB_CM_REJ_INVALID_MTU]			= "invalid mtu",
> > +	[IB_CM_REJ_INSUFFICIENT_RESP_RESOURCES]	= "insufficient resp
> resources",
> > +	[IB_CM_REJ_CONSUMER_DEFINED]		= "consumer defined",
> > +	[IB_CM_REJ_INVALID_RNR_RETRY]		= "invalid rnr retry",
> > +	[IB_CM_REJ_DUPLICATE_LOCAL_COMM_ID]	= "duplicate local comm
> id",
> > +	[IB_CM_REJ_INVALID_CLASS_VERSION]	= "invalid class version",
> > +	[IB_CM_REJ_INVALID_FLOW_LABEL]		= "invalid flow label",
> > +	[IB_CM_REJ_INVALID_ALT_FLOW_LABEL]	= "invalid alt flow label",
> 
> Other error messages capitalize GID, LID, CM, ID, RNR and SL so please
> do this here too.
> 

ok

> > +const char *__attribute_const__ ibcm_reject_msg(int reason)
> > +{
> > +	size_t index = reason;
> > +
> > +	if (index >= ARRAY_SIZE(ibcm_rej_reason_strs) ||
> > +	    !ibcm_rej_reason_strs[index])
> > +		return "unrecognized reason";
> > +	else
> > +		return ibcm_rej_reason_strs[index];
> > +}
> 
> Please consider using positive logic - this means negating the
> if-condition and swapping the if- and else-parts.

yea that might be even clearer.

> 
> > +const char *__attribute_const__ rdma_reject_msg(struct rdma_cm_id *id,
> > +						int reason)
> > +{
> > +	if (rdma_ib_or_roce(id->device, id->port_num))
> > +		return ibcm_reject_msg(reason);
> > +
> > +	if (rdma_protocol_iwarp(id->device, id->port_num))
> > +		return iwcm_reject_msg(reason);
> > +
> > +	WARN_ON_ONCE(1);
> > +	return "unrecognized reason";
> 
> Have you considered to return "unrecognized transport" here instead?
> 

I can do that.

> > +const char *__attribute_const__ iwcm_reject_msg(int reason)
> > +{
> > +	size_t index;
> > +
> > +	/* iWARP uses negative errnos */
> > +	index = -reason;
> > +
> > +	if (index >= ARRAY_SIZE(iwcm_rej_reason_strs) ||
> > +	    !iwcm_rej_reason_strs[index])
> > +		return "unrecognized reason";
> > +	else
> > +		return iwcm_rej_reason_strs[index];
> > +}
> 
> Also for this function, please consider using positive logic.
> 
> Thanks,
> 
> Bart.

[PATCH v3 4/6] nvme-rdma: use rdma connection reject helper functions

[Steve Wise]

Also add nvme cm status strings and use them.

Signed-off-by: Steve Wise <swise at opengridcomputing.com>
---
 drivers/nvme/host/rdma.c | 46 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index fbdb226..b6a6b50 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -43,6 +43,28 @@
 
 #define NVME_RDMA_MAX_INLINE_SEGMENTS	1
 
+static const char *const nvme_rdma_cm_status_strs[] = {
+	[NVME_RDMA_CM_INVALID_LEN]	= "invalid length",
+	[NVME_RDMA_CM_INVALID_RECFMT]	= "invalid record format",
+	[NVME_RDMA_CM_INVALID_QID]	= "invalid queue id",
+	[NVME_RDMA_CM_INVALID_HSQSIZE]	= "invalid host sq size",
+	[NVME_RDMA_CM_INVALID_HRQSIZE]	= "invalid host rq size",
+	[NVME_RDMA_CM_NO_RSC]		= "resource not found",
+	[NVME_RDMA_CM_INVALID_IRD]	= "invalid ird",
+	[NVME_RDMA_CM_INVALID_ORD]	= "Invalid ord",
+};
+
+static const char *nvme_rdma_cm_msg(enum nvme_rdma_cm_status status)
+{
+	size_t index = status;
+
+	if (index >= ARRAY_SIZE(nvme_rdma_cm_status_strs) ||
+	    !nvme_rdma_cm_status_strs[index])
+		return "unrecognized reason";
+	else
+		return nvme_rdma_cm_status_strs[index];
+};
+
 /*
  * We handle AEN commands ourselves and don't even let the
  * block layer know about them.
@@ -1222,17 +1244,25 @@ out_destroy_queue_ib:
 static int nvme_rdma_conn_rejected(struct nvme_rdma_queue *queue,
 		struct rdma_cm_event *ev)
 {
-	if (ev->param.conn.private_data_len) {
-		struct nvme_rdma_cm_rej *rej =
-			(struct nvme_rdma_cm_rej *)ev->param.conn.private_data;
+	struct rdma_cm_id *cm_id = queue->cm_id;
+	int status = ev->status;
+	const char *rej_msg;
+	struct nvme_rdma_cm_rej *rej_data;
+	u8 rej_data_len;
+
+	rej_msg = rdma_reject_msg(cm_id, status);
+	rej_data = (struct nvme_rdma_cm_rej *)
+		   rdma_consumer_reject_data(cm_id, ev, &rej_data_len);
+
+	if (rej_data && rej_data_len >= sizeof(u16)) {
+		u16 sts = le16_to_cpu(rej_data->sts);
 
 		dev_err(queue->ctrl->ctrl.device,
-			"Connect rejected, status %d.", le16_to_cpu(rej->sts));
-		/* XXX: Think of something clever to do here... */
-	} else {
+		      "Connect rejected: status %d (%s) nvme status %d (%s).\n",
+		      status, rej_msg, sts, nvme_rdma_cm_msg(sts));
+	} else
 		dev_err(queue->ctrl->ctrl.device,
-			"Connect rejected, no private data.\n");
-	}
+			"Connect rejected: status %d (%s).\n", status, rej_msg);
 
 	return -ECONNRESET;
 }
-- 
2.7.0

[PATCH v3 4/6] nvme-rdma: use rdma connection reject helper functions

[Sagi Grimberg]

> +static const char *const nvme_rdma_cm_status_strs[] = {
> +	[NVME_RDMA_CM_INVALID_LEN]	= "invalid length",
> +	[NVME_RDMA_CM_INVALID_RECFMT]	= "invalid record format",
> +	[NVME_RDMA_CM_INVALID_QID]	= "invalid queue id",
> +	[NVME_RDMA_CM_INVALID_HSQSIZE]	= "invalid host sq size",
> +	[NVME_RDMA_CM_INVALID_HRQSIZE]	= "invalid host rq size",
> +	[NVME_RDMA_CM_NO_RSC]		= "resource not found",
> +	[NVME_RDMA_CM_INVALID_IRD]	= "invalid ird",
> +	[NVME_RDMA_CM_INVALID_ORD]	= "Invalid ord",
> +};

I think it'd be better to move them to include/linux/nvme-rdma.h
and use them for logging on the target side too.

[PATCH v3 4/6] nvme-rdma: use rdma connection reject helper functions

[Steve Wise]

> > +static const char *const nvme_rdma_cm_status_strs[] = {
> > +	[NVME_RDMA_CM_INVALID_LEN]	= "invalid length",
> > +	[NVME_RDMA_CM_INVALID_RECFMT]	= "invalid record format",
> > +	[NVME_RDMA_CM_INVALID_QID]	= "invalid queue id",
> > +	[NVME_RDMA_CM_INVALID_HSQSIZE]	= "invalid host sq size",
> > +	[NVME_RDMA_CM_INVALID_HRQSIZE]	= "invalid host rq size",
> > +	[NVME_RDMA_CM_NO_RSC]		= "resource not found",
> > +	[NVME_RDMA_CM_INVALID_IRD]	= "invalid ird",
> > +	[NVME_RDMA_CM_INVALID_ORD]	= "Invalid ord",
> > +};
> 
> I think it'd be better to move them to include/linux/nvme-rdma.h
> and use them for logging on the target side too.
> 

You want to put this in nvme-rdma.h as-is so the memory for this is allocated in
each .c file that includes it? 

[PATCH v3 4/6] nvme-rdma: use rdma connection reject helper functions

[Christoph Hellwig]

On Tue, Oct 25, 2016@07:36:25PM +0300, Sagi Grimberg wrote:
> I think it'd be better to move them to include/linux/nvme-rdma.h
> and use them for logging on the target side too.

Hmm, that would mean they get compiled into every source file.
Also on the target side we can have free-form messages for whatever
the exact rejection reason was, no need to limit us to the enum.

[PATCH v3 4/6] nvme-rdma: use rdma connection reject helper functions

[Sagi Grimberg]

> On Tue, Oct 25, 2016@07:36:25PM +0300, Sagi Grimberg wrote:
>> I think it'd be better to move them to include/linux/nvme-rdma.h
>> and use them for logging on the target side too.
>
> Hmm, that would mean they get compiled into every source file.

"every source file" is exactly two...

> Also on the target side we can have free-form messages for whatever
> the exact rejection reason was, no need to limit us to the enum.

Thought it'd be nice to use the same verbosity on both ends, just a
suggestion though...

[PATCH v3 4/6] nvme-rdma: use rdma connection reject helper functions

[Christoph Hellwig]

On Mon, Oct 24, 2016@12:07:13PM -0700, Steve Wise wrote:
> Also add nvme cm status strings and use them.
> 
> Signed-off-by: Steve Wise <swise at opengridcomputing.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH v3 4/6] nvme-rdma: use rdma connection reject helper functions

[Bart Van Assche]

On 10/24/2016 12:09 PM, Steve Wise wrote:
> +static const char *const nvme_rdma_cm_status_strs[] = {
> +	[NVME_RDMA_CM_INVALID_LEN]	= "invalid length",
> +	[NVME_RDMA_CM_INVALID_RECFMT]	= "invalid record format",
> +	[NVME_RDMA_CM_INVALID_QID]	= "invalid queue id",
> +	[NVME_RDMA_CM_INVALID_HSQSIZE]	= "invalid host sq size",
> +	[NVME_RDMA_CM_INVALID_HRQSIZE]	= "invalid host rq size",
> +	[NVME_RDMA_CM_NO_RSC]		= "resource not found",
> +	[NVME_RDMA_CM_INVALID_IRD]	= "invalid ird",
> +	[NVME_RDMA_CM_INVALID_ORD]	= "Invalid ord",
> +};

Please capitalize abbreviations (SQ, RQ, ID, IRD and ORD).

> +static const char *nvme_rdma_cm_msg(enum nvme_rdma_cm_status status)
> +{
> +	size_t index = status;
> +
> +	if (index >= ARRAY_SIZE(nvme_rdma_cm_status_strs) ||
> +	    !nvme_rdma_cm_status_strs[index])
> +		return "unrecognized reason";
> +	else
> +		return nvme_rdma_cm_status_strs[index];
> +};

Please consider rewriting this if-statement such that the recognized 
reason scenario occurs first.

> +	rej_msg = rdma_reject_msg(cm_id, status);
> +	rej_data = (struct nvme_rdma_cm_rej *)
> +		   rdma_consumer_reject_data(cm_id, ev, &rej_data_len);

Please reorder the patches in this patch series such that it doesn't 
break a bisect. rdma_consumer_reject_data() should be introduced before 
it is used.

Thanks,

Bart.

[PATCH v3 4/6] nvme-rdma: use rdma connection reject helper functions

[Steve Wise]

> 
> On 10/24/2016 12:09 PM, Steve Wise wrote:
> > +static const char *const nvme_rdma_cm_status_strs[] = {
> > +	[NVME_RDMA_CM_INVALID_LEN]	= "invalid length",
> > +	[NVME_RDMA_CM_INVALID_RECFMT]	= "invalid record format",
> > +	[NVME_RDMA_CM_INVALID_QID]	= "invalid queue id",
> > +	[NVME_RDMA_CM_INVALID_HSQSIZE]	= "invalid host sq size",
> > +	[NVME_RDMA_CM_INVALID_HRQSIZE]	= "invalid host rq size",
> > +	[NVME_RDMA_CM_NO_RSC]		= "resource not found",
> > +	[NVME_RDMA_CM_INVALID_IRD]	= "invalid ird",
> > +	[NVME_RDMA_CM_INVALID_ORD]	= "Invalid ord",
> > +};
> 
> Please capitalize abbreviations (SQ, RQ, ID, IRD and ORD).
>

will do.

 
> > +static const char *nvme_rdma_cm_msg(enum nvme_rdma_cm_status status)
> > +{
> > +	size_t index = status;
> > +
> > +	if (index >= ARRAY_SIZE(nvme_rdma_cm_status_strs) ||
> > +	    !nvme_rdma_cm_status_strs[index])
> > +		return "unrecognized reason";
> > +	else
> > +		return nvme_rdma_cm_status_strs[index];
> > +};
> 
> Please consider rewriting this if-statement such that the recognized
> reason scenario occurs first.
>

sure.
 
> > +	rej_msg = rdma_reject_msg(cm_id, status);
> > +	rej_data = (struct nvme_rdma_cm_rej *)
> > +		   rdma_consumer_reject_data(cm_id, ev, &rej_data_len);
> 
> Please reorder the patches in this patch series such that it doesn't
> break a bisect. rdma_consumer_reject_data() should be introduced before
> it is used.
> 

rdma_consumer_reject_data() is introduced in patch 3.

[PATCH v3 4/6] nvme-rdma: use rdma connection reject helper functions

[Bart Van Assche]

On Tue, 2016-10-25@13:20 -0500, Steve Wise wrote:
> > 
> > On 10/24/2016 12:09 PM, Steve Wise wrote:
> > > 
> > > +	rej_msg = rdma_reject_msg(cm_id, status);
> > > +	rej_data = (struct nvme_rdma_cm_rej *)
> > > +		???rdma_consumer_reject_data(cm_id, ev,
> > > &rej_data_len);
> > 
> > Please reorder the patches in this patch series such that it
> > doesn't
> > break a bisect. rdma_consumer_reject_data() should be introduced
> > before
> > it is used.
> 
> rdma_consumer_reject_data() is introduced in patch 3.

Hello Steve,

Thanks for the feedback. Apparently my e-mail client displayed the
patches in the wrong order ...

Bart.

[PATCH v3 5/6] ib_iser: log the connection reject message

[Steve Wise]

Signed-off-by: Steve Wise <swise at opengridcomputing.com>
---
 drivers/infiniband/ulp/iser/iser_verbs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index 1b49453..7970757 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -906,11 +906,14 @@ static int iser_cma_handler(struct rdma_cm_id *cma_id, struct rdma_cm_event *eve
 	case RDMA_CM_EVENT_ESTABLISHED:
 		iser_connected_handler(cma_id, event->param.conn.private_data);
 		break;
+	case RDMA_CM_EVENT_REJECTED:
+		iser_info("Connection rejected: %s\n",
+			 rdma_reject_msg(cma_id, event->status));
+		/*FALLTHROUGH*/
 	case RDMA_CM_EVENT_ADDR_ERROR:
 	case RDMA_CM_EVENT_ROUTE_ERROR:
 	case RDMA_CM_EVENT_CONNECT_ERROR:
 	case RDMA_CM_EVENT_UNREACHABLE:
-	case RDMA_CM_EVENT_REJECTED:
 		iser_connect_error(cma_id);
 		break;
 	case RDMA_CM_EVENT_DISCONNECTED:
-- 
2.7.0

[PATCH v3 5/6] ib_iser: log the connection reject message

[Sagi Grimberg]

> +	case RDMA_CM_EVENT_REJECTED:
> +		iser_info("Connection rejected: %s\n",
> +			 rdma_reject_msg(cma_id, event->status));
> +		/*FALLTHROUGH*/

NIT: /* FALLTHROUGH */

Otherwise:

Acked-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH v3 6/6] rds_rdma: log the connection reject message

[Steve Wise]

Signed-off-by: Steve Wise <swise at opengridcomputing.com>
---
 net/rds/rdma_transport.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
index 345f090..6bd2814 100644
--- a/net/rds/rdma_transport.c
+++ b/net/rds/rdma_transport.c
@@ -100,11 +100,14 @@ int rds_rdma_cm_event_handler(struct rdma_cm_id *cm_id,
 		trans->cm_connect_complete(conn, event);
 		break;
 
+	case RDMA_CM_EVENT_REJECTED:
+		rdsdebug("Connection rejected: %s\n",
+			 rdma_reject_msg(cm_id, event->status));
+		/*FALLTHROUGH*/
 	case RDMA_CM_EVENT_ADDR_ERROR:
 	case RDMA_CM_EVENT_ROUTE_ERROR:
 	case RDMA_CM_EVENT_CONNECT_ERROR:
 	case RDMA_CM_EVENT_UNREACHABLE:
-	case RDMA_CM_EVENT_REJECTED:
 	case RDMA_CM_EVENT_DEVICE_REMOVAL:
 	case RDMA_CM_EVENT_ADDR_CHANGE:
 		if (conn)
-- 
2.7.0

[PATCH v3 6/6] rds_rdma: log the connection reject message

[Santosh Shilimkar]

On 10/24/2016 12:07 PM, Steve Wise wrote:
> Signed-off-by: Steve Wise <swise at opengridcomputing.com>
> ---
>  net/rds/rdma_transport.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
> index 345f090..6bd2814 100644
> --- a/net/rds/rdma_transport.c
> +++ b/net/rds/rdma_transport.c
> @@ -100,11 +100,14 @@ int rds_rdma_cm_event_handler(struct rdma_cm_id *cm_id,
>  		trans->cm_connect_complete(conn, event);
>  		break;
>
> +	case RDMA_CM_EVENT_REJECTED:
> +		rdsdebug("Connection rejected: %s\n",
> +			 rdma_reject_msg(cm_id, event->status));
For current code above is fine but I will need the other helper
while adding few connection cases where ARI field carries information
for the reason for the reject.

I assume, rdma_consumer_reject_data() is meant to be used for that ?

The patch looks fine to me.
FWIW, Acked-by: Santosh Shilimkar <santosh.shilimkar at oracle.com>

[PATCH v3 6/6] rds_rdma: log the connection reject message

[Steve Wise]

> -----Original Message-----
> From: Santosh Shilimkar [mailto:santosh.shilimkar at oracle.com]
> Sent: Tuesday, October 25, 2016 10:52 AM
> To: Steve Wise; dledford at redhat.com; sean.hefty at intel.com
> Cc: linux-rdma at vger.kernel.org; bart.vanassche at sandisk.com; linux-
> nvme at lists.infradead.org; sagi at grimberg.me; hch at lst.de; axboe at fb.com
> Subject: Re: [PATCH v3 6/6] rds_rdma: log the connection reject message
> 
> On 10/24/2016 12:07 PM, Steve Wise wrote:
> > Signed-off-by: Steve Wise <swise at opengridcomputing.com>
> > ---
> >  net/rds/rdma_transport.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
> > index 345f090..6bd2814 100644
> > --- a/net/rds/rdma_transport.c
> > +++ b/net/rds/rdma_transport.c
> > @@ -100,11 +100,14 @@ int rds_rdma_cm_event_handler(struct rdma_cm_id
> *cm_id,
> >  		trans->cm_connect_complete(conn, event);
> >  		break;
> >
> > +	case RDMA_CM_EVENT_REJECTED:
> > +		rdsdebug("Connection rejected: %s\n",
> > +			 rdma_reject_msg(cm_id, event->status));
> For current code above is fine but I will need the other helper
> while adding few connection cases where ARI field carries information
> for the reason for the reject.
> 
> I assume, rdma_consumer_reject_data() is meant to be used for that ?

Yep.  You can see the nvme_rdma patch in this series for an example.

> 
> The patch looks fine to me.
> FWIW, Acked-by: Santosh Shilimkar <santosh.shilimkar at oracle.com>

[PATCH v3 6/6] rds_rdma: log the connection reject message

[Santosh Shilimkar]

On 10/25/2016 9:04 AM, Steve Wise wrote:
>
>
>> -----Original Message-----
>> From: Santosh Shilimkar [mailto:santosh.shilimkar at oracle.com]
>> Sent: Tuesday, October 25, 2016 10:52 AM
>> To: Steve Wise; dledford at redhat.com; sean.hefty at intel.com
>> Cc: linux-rdma at vger.kernel.org; bart.vanassche at sandisk.com; linux-
>> nvme at lists.infradead.org; sagi at grimberg.me; hch at lst.de; axboe at fb.com
>> Subject: Re: [PATCH v3 6/6] rds_rdma: log the connection reject message
>>
>> On 10/24/2016 12:07 PM, Steve Wise wrote:
>>> Signed-off-by: Steve Wise <swise at opengridcomputing.com>
>>> ---
>>>  net/rds/rdma_transport.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
>>> index 345f090..6bd2814 100644
>>> --- a/net/rds/rdma_transport.c
>>> +++ b/net/rds/rdma_transport.c
>>> @@ -100,11 +100,14 @@ int rds_rdma_cm_event_handler(struct rdma_cm_id
>> *cm_id,
>>>  		trans->cm_connect_complete(conn, event);
>>>  		break;
>>>
>>> +	case RDMA_CM_EVENT_REJECTED:
>>> +		rdsdebug("Connection rejected: %s\n",
>>> +			 rdma_reject_msg(cm_id, event->status));
>> For current code above is fine but I will need the other helper
>> while adding few connection cases where ARI field carries information
>> for the reason for the reject.
>>
>> I assume, rdma_consumer_reject_data() is meant to be used for that ?
>
> Yep.  You can see the nvme_rdma patch in this series for an example.
>
Cool. Thanks for info Steve.

regards,
Santosh