[PATCH 3/7 v2] nvme-fabrics: Add FC transport FC-NVME definitions

[PATCH 3/7 v2] nvme-fabrics: Add FC transport FC-NVME definitions

[James Smart]

nvme-fabrics: Add FC transport FC-NVME definitions:
- Formats for Cmd, Data, Rsp IUs
- Formats FC-4 LS definitions
- cmd_iu changed in T11. The 2 reserved words, previous at words
  2 and 3, were moved to after the sqe.
- Modifications from prior reviews:
-  FC-transport-specific error codes moved to include/linux/nvme.h
-  Removed 0x28 type declaration to include/uapi/scsi/fc/fc_fs.h
-  Removed Reject reasons and explanations. ELS values are to be used.

Signed-off-by: James Smart <james.smart at broadcom.com>
---
 include/linux/nvme-fc.h | 255 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 255 insertions(+)
 create mode 100644 include/linux/nvme-fc.h

diff --git a/include/linux/nvme-fc.h b/include/linux/nvme-fc.h
new file mode 100644
index 0000000..358d155
--- /dev/null
+++ b/include/linux/nvme-fc.h
@@ -0,0 +1,255 @@
+/*
+ * Copyright (c) 2016 Avago Technologies.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful.
+ * ALL EXPRESS OR IMPLIED CONDITIONS, REPRESENTATIONS AND WARRANTIES,
+ * INCLUDING ANY IMPLIED WARRANTY OF MERCHANTABILITY, FITNESS FOR A
+ * PARTICULAR PURPOSE, OR NON-INFRINGEMENT, ARE DISCLAIMED, EXCEPT TO
+ * THE EXTENT THAT SUCH DISCLAIMERS ARE HELD TO BE LEGALLY INVALID.
+ * See the GNU General Public License for more details, a copy of which
+ * can be found in the file COPYING included with this package
+ *
+ */
+
+/*
+ * This file contains definitions relative to FC-NVME r1.09
+ */
+
+#ifndef _NVME_FC_H
+#define _NVME_FC_H 1
+
+
+#define NVME_CMD_SCSI_ID		0xFD
+#define NVME_CMD_FC_ID			FC_TYPE_NVME
+
+/* FC-NVME Cmd IU Flags */
+#define FCNVME_CMD_FLAGS_DIRMASK	0x03
+#define FCNVME_CMD_FLAGS_WRITE		0x01
+#define FCNVME_CMD_FLAGS_READ		0x02
+
+struct nvme_fc_cmd_iu {
+	__u8			scsi_id;
+	__u8			fc_id;
+	__be16			iu_len;
+	__u8			rsvd4[3];
+	__u8			flags;
+	__be64			connection_id;
+	__be32			csn;
+	__be32			data_len;
+	struct nvme_command	sqe;
+	__be32			rsvd88[2];
+};
+
+#define NVME_FC_SIZEOF_ZEROS_RSP	12
+
+struct nvme_fc_ersp_iu {
+	__u8			rsvd0[2];
+	__be16			iu_len;
+	__be32			rsn;
+	__be32			rsvd8[2];
+	struct nvme_completion	cqe;
+	/* for now - no additional payload */
+};
+
+
+/* FC-NVME r1.03/16-119v0 NVME Link Services */
+enum {
+	FCNVME_LS_RSVD			= 0,
+	FCNVME_LS_RJT			= 1,
+	FCNVME_LS_ACC			= 2,
+	FCNVME_LS_CREATE_ASSOCIATION	= 3,
+	FCNVME_LS_CREATE_CONNECTION	= 4,
+	FCNVME_LS_DISCONNECT		= 5,
+};
+
+/* FC-NVME r1.03/16-119v0 NVME Link Service Descriptors */
+enum {
+	FCNVME_LSDESC_RSVD		= 0x0,
+	FCNVME_LSDESC_RQST		= 0x1,
+	FCNVME_LSDESC_RJT		= 0x2,
+	FCNVME_LSDESC_CREATE_ASSOC_CMD	= 0x3,
+	FCNVME_LSDESC_CREATE_CONN_CMD	= 0x4,
+	FCNVME_LSDESC_DISCONN_CMD	= 0x5,
+	FCNVME_LSDESC_CONN_ID		= 0x6,
+	FCNVME_LSDESC_ASSOC_ID		= 0x7,
+};
+
+
+/* ********** start of Link Service Descriptors ********** */
+
+
+/* fills in length of a descriptor. Struture minus descriptor header */
+#define FCNVME_LSDESC_LEN(lsdesc) \
+		cpu_to_be32(sizeof(lsdesc) - (2*(sizeof(u32))))
+
+struct fcnvme_ls_rqst_w0 {
+	u8	ls_cmd;			/* FCNVME_LS_xxx */
+	u8	zeros[3];
+};
+
+/* FCNVME_LSDESC_RQST */
+struct fcnvme_lsdesc_rqst {
+	__be32	desc_tag;		/* FCNVME_LSDESC_xxx */
+	__be32	desc_len;
+	struct fcnvme_ls_rqst_w0	w0;
+	__be32	rsvd12;
+};
+
+
+
+
+/* FCNVME_LSDESC_RJT */
+struct fcnvme_lsdesc_rjt {
+	__be32	desc_tag;		/* FCNVME_LSDESC_xxx */
+	__be32	desc_len;
+	u8	rsvd8;
+
+	/*
+	 * Reject reason and explanaction codes are generic
+	 * to ELs's from LS-3.
+	 */
+	u8	reason_code;
+	u8	reason_explanation;
+
+	u8	vendor;
+	__be32	rsvd12;
+};
+
+
+#define FCNVME_ASSOC_HOSTID_LEN		64
+#define FCNVME_ASSOC_HOSTNQN_LEN	256
+#define FCNVME_ASSOC_SUBNQN_LEN		256
+
+/* FCNVME_LSDESC_CREATE_ASSOC_CMD */
+struct fcnvme_lsdesc_cr_assoc_cmd {
+	__be32	desc_tag;		/* FCNVME_LSDESC_xxx */
+	__be32	desc_len;
+	__be16	ersp_ratio;
+	__be16	rsvd10;
+	__be32	rsvd12[9];
+	__be16	cntlid;
+	__be16	sqsize;
+	__be32	rsvd52;
+	u8	hostid[FCNVME_ASSOC_HOSTID_LEN];
+	u8	hostnqn[FCNVME_ASSOC_HOSTNQN_LEN];
+	u8	subnqn[FCNVME_ASSOC_SUBNQN_LEN];
+	u8	rsvd632[384];
+};
+
+/* FCNVME_LSDESC_CREATE_CONN_CMD */
+struct fcnvme_lsdesc_cr_conn_cmd {
+	__be32	desc_tag;		/* FCNVME_LSDESC_xxx */
+	__be32	desc_len;
+	__be16	ersp_ratio;
+	__be16	rsvd10;
+	__be32	rsvd12[9];
+	__be16	qid;
+	__be16	sqsize;
+	__be32  rsvd52;
+};
+
+/* Disconnect Scope Values */
+enum {
+	FCNVME_DISCONN_ASSOCIATION	= 0,
+	FCNVME_DISCONN_CONNECTION	= 1,
+};
+
+/* FCNVME_LSDESC_DISCONN_CMD */
+struct fcnvme_lsdesc_disconn_cmd {
+	__be32	desc_tag;		/* FCNVME_LSDESC_xxx */
+	__be32	desc_len;
+	u8	rsvd8[3];
+	/* note: scope is really a 1 bit field */
+	u8	scope;			/* FCNVME_DISCONN_xxx */
+	__be32	rsvd12;
+	__be64	id;
+};
+
+/* FCNVME_LSDESC_CONN_ID */
+struct fcnvme_lsdesc_conn_id {
+	__be32	desc_tag;		/* FCNVME_LSDESC_xxx */
+	__be32	desc_len;
+	__be64	connection_id;
+};
+
+/* FCNVME_LSDESC_ASSOC_ID */
+struct fcnvme_lsdesc_assoc_id {
+	__be32	desc_tag;		/* FCNVME_LSDESC_xxx */
+	__be32	desc_len;
+	__be64	association_id;
+};
+
+
+/* ********** start of Link Services ********** */
+
+
+/* FCNVME_LS_RJT */
+struct fcnvme_ls_rjt {
+	struct fcnvme_ls_rqst_w0		w0;
+	__be32					desc_list_len;
+	struct fcnvme_lsdesc_rqst		rqst;
+	struct fcnvme_lsdesc_rjt		rjt;
+};
+
+/* FCNVME_LS_ACC */
+struct fcnvme_ls_acc_hdr {
+	struct fcnvme_ls_rqst_w0		w0;
+	__be32					desc_list_len;
+	struct fcnvme_lsdesc_rqst		rqst;
+	/* Followed by cmd-specific ACC descriptors, see next definitions */
+};
+
+/* FCNVME_LS_CREATE_ASSOCIATION */
+struct fcnvme_ls_cr_assoc_rqst {
+	struct fcnvme_ls_rqst_w0		w0;
+	__be32					desc_list_len;
+	struct fcnvme_lsdesc_cr_assoc_cmd	assoc_cmd;
+};
+
+struct fcnvme_ls_cr_assoc_acc {
+	struct fcnvme_ls_acc_hdr		hdr;
+	struct fcnvme_lsdesc_assoc_id		associd;
+	struct fcnvme_lsdesc_conn_id		connectid;
+};
+
+
+/* FCNVME_LS_CREATE_CONNECTION */
+struct fcnvme_ls_cr_conn_rqst {
+	struct fcnvme_ls_rqst_w0		w0;
+	__be32					desc_list_len;
+	struct fcnvme_lsdesc_assoc_id		associd;
+	struct fcnvme_lsdesc_cr_conn_cmd	connect_cmd;
+};
+
+struct fcnvme_ls_cr_conn_acc {
+	struct fcnvme_ls_acc_hdr		hdr;
+	struct fcnvme_lsdesc_conn_id		connectid;
+};
+
+/* FCNVME_LS_DISCONNECT */
+struct fcnvme_ls_disconnect_rqst {
+	struct fcnvme_ls_rqst_w0		w0;
+	__be32					desc_list_len;
+	struct fcnvme_lsdesc_assoc_id		associd;
+	struct fcnvme_lsdesc_disconn_cmd	discon_cmd;
+};
+
+struct fcnvme_ls_disconnect_acc {
+	struct fcnvme_ls_acc_hdr		hdr;
+};
+
+
+/*
+ * Yet to be defined in FC-NVME:
+ */
+#define NVME_FC_CONNECT_TIMEOUT_SEC	2		/* 2 seconds */
+#define NVME_FC_LS_TIMEOUT_SEC		2		/* 2 seconds */
+#define NVME_FC_TGTOP_TIMEOUT_SEC	2		/* 2 seconds */
+
+
+#endif /* _NVME_FC_H */
+
-- 
2.5.0

[PATCH 3/7 v2] nvme-fabrics: Add FC transport FC-NVME definitions

[Christoph Hellwig]

On Fri, Oct 07, 2016@04:09:04PM -0700, James Smart wrote:
> 
> nvme-fabrics: Add FC transport FC-NVME definitions:

FYI, all the patches seem to have this duplicate of the subject in
the mail body.

> - cmd_iu changed in T11. The 2 reserved words, previous at words
>   2 and 3, were moved to after the sqe.
> - Modifications from prior reviews:
> -  FC-transport-specific error codes moved to include/linux/nvme.h
> -  Removed 0x28 type declaration to include/uapi/scsi/fc/fc_fs.h
> -  Removed Reject reasons and explanations. ELS values are to be used.

This should go either into the cover letter, or below the "---" line
so that it doesn't show up in the commit message.

> +/* fills in length of a descriptor. Struture minus descriptor header */
> +#define FCNVME_LSDESC_LEN(lsdesc) \
> +		cpu_to_be32(sizeof(lsdesc) - (2*(sizeof(u32))))

Make this an inline function and use spaces between the operators,
please.


Except for these nitpicks the patch looks fine to me:

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

[PATCH 3/7 v2] nvme-fabrics: Add FC transport FC-NVME definitions

[James Smart]

On 10/12/2016 1:58 AM, Christoph Hellwig wrote:
> On Fri, Oct 07, 2016@04:09:04PM -0700, James Smart wrote:
>> nvme-fabrics: Add FC transport FC-NVME definitions:
> FYI, all the patches seem to have this duplicate of the subject in
> the mail body.

I'm surprised. I'll look at it.

>
>> - cmd_iu changed in T11. The 2 reserved words, previous at words
>>    2 and 3, were moved to after the sqe.
>> - Modifications from prior reviews:
>> -  FC-transport-specific error codes moved to include/linux/nvme.h
>> -  Removed 0x28 type declaration to include/uapi/scsi/fc/fc_fs.h
>> -  Removed Reject reasons and explanations. ELS values are to be used.
> This should go either into the cover letter, or below the "---" line
> so that it doesn't show up in the commit message.

Ok - help my style. I assumed deltas to original patch made sense to 
itemize. As they are specific to this one file I thought it made sense 
to be in the commit message for this patch. I have no problem moving it 
to the cover letter. Just not sure what the criteria is for one place vs 
another.

>
>> +/* fills in length of a descriptor. Struture minus descriptor header */
>> +#define FCNVME_LSDESC_LEN(lsdesc) \
>> +		cpu_to_be32(sizeof(lsdesc) - (2*(sizeof(u32))))
> Make this an inline function and use spaces between the operators,
> please.

Np.

-- james

[PATCH 3/7 v2] nvme-fabrics: Add FC transport FC-NVME definitions

[Christoph Hellwig]

On Wed, Oct 12, 2016@11:53:29AM -0700, James Smart wrote:
> Ok - help my style. I assumed deltas to original patch made sense to
> itemize. As they are specific to this one file I thought it made sense to be
> in the commit message for this patch. I have no problem moving it to the
> cover letter. Just not sure what the criteria is for one place vs another.

If you want to keep them in this patch that's not as usual but okay as
well.  But in that case move it under the '---' marker, so that git-am
won't pick it up.

[PATCH 3/7 v2] nvme-fabrics: Add FC transport FC-NVME definitions

[James Smart]

On 10/12/2016 1:58 AM, Christoph Hellwig wrote:
>
>> +/* fills in length of a descriptor. Struture minus descriptor header */
>> +#define FCNVME_LSDESC_LEN(lsdesc) \
>> +		cpu_to_be32(sizeof(lsdesc) - (2*(sizeof(u32))))
> Make this an inline function and use spaces between the operators,
> please.
>

Christoph,

This can't be an inline - the "lsdesc" field is a type declaration 
("struct x") and varies. That's the purpose of the define: so it can be 
applied to different structures- e.g. FCNVME_LSDESC_LEN(struct foo) and 
FCNVME_LSDESC_LEN(struct bar)

I'll adjust for the spacing, but will leave as a define.

-- james

[PATCH 3/7 v2] nvme-fabrics: Add FC transport FC-NVME definitions

[Jon Derrick]

Hi James,

Fair warning, I don't really deal with the fabrics part of our driver,
and I don't see where your macro is actually used.

On Tue, Oct 18, 2016@10:59:43AM -0700, James Smart wrote:
> 
> 
> On 10/12/2016 1:58 AM, Christoph Hellwig wrote:
> >
> >>+/* fills in length of a descriptor. Struture minus descriptor header */
> >>+#define FCNVME_LSDESC_LEN(lsdesc) \
> >>+		cpu_to_be32(sizeof(lsdesc) - (2*(sizeof(u32))))
> >Make this an inline function and use spaces between the operators,
> >please.
> >
> 
> Christoph,
> 
> This can't be an inline - the "lsdesc" field is a type declaration ("struct
> x") and varies. That's the purpose of the define: so it can be applied to
> different structures- e.g. FCNVME_LSDESC_LEN(struct foo) and
> FCNVME_LSDESC_LEN(struct bar)

How about:
static inline __be32 fcnvme_lsdesc_len(size_t lsdesc_sz)
{
	return cpu_to_be32(lsdesc_sz - 2 * sizeof(u32));
}

foo = fcnvme_lsdesc_len(sizeof(struct bar));

[PATCH 3/7 v2] nvme-fabrics: Add FC transport FC-NVME definitions

[James Smart]

On 10/18/2016 12:49 PM, Jon Derrick wrote:
> Hi James,
>
> Fair warning, I don't really deal with the fabrics part of our driver,
> and I don't see where your macro is actually used.

Its in the new FC transport files (host and target) that are added in 
subsequent patches. Wouldn't be used elsewhere.

>
> On Tue, Oct 18, 2016@10:59:43AM -0700, James Smart wrote:
> How about:
> static inline __be32 fcnvme_lsdesc_len(size_t lsdesc_sz)
> {
> 	return cpu_to_be32(lsdesc_sz - 2 * sizeof(u32));
> }
>
> foo = fcnvme_lsdesc_len(sizeof(struct bar));

Yes - that would work.

What is the rule for when it should be a macro vs an inline ?   It makes 
sense to me to be an inline if you want to strongly check the type of an 
argument, but something simple like this, it's not so clear.  If I 
didn't have the "cpu_to_be32" part, I'd abandon the inline and the macro 
and code it directly.

-- james

[PATCH 3/7 v2] nvme-fabrics: Add FC transport FC-NVME definitions

[Jon Derrick]

> >On Tue, Oct 18, 2016@10:59:43AM -0700, James Smart wrote:
> >How about:
> >static inline __be32 fcnvme_lsdesc_len(size_t lsdesc_sz)
> >{
> >	return cpu_to_be32(lsdesc_sz - 2 * sizeof(u32));
> >}
> >
> >foo = fcnvme_lsdesc_len(sizeof(struct bar));
> 
> Yes - that would work.
> 
> What is the rule for when it should be a macro vs an inline ?   It makes
> sense to me to be an inline if you want to strongly check the type of an
> argument, but something simple like this, it's not so clear.  If I didn't
> have the "cpu_to_be32" part, I'd abandon the inline and the macro and code
> it directly.
In this case they will both provide the same error checking, due to
cpu_to_be32 returning type __be32. The only difference is that one adds
a macro and the other adds a function, and if we ever need to add some
caveats to the logic it will be simpler with the function. That and the
coding style dictates that all things being equal, we should still
prefer inline. Those are my only rationales in this case.

[PATCH 3/7 v2] nvme-fabrics: Add FC transport FC-NVME definitions

[Christoph Hellwig]

On Tue, Oct 18, 2016@05:32:24PM -0600, Jon Derrick wrote:
> > What is the rule for when it should be a macro vs an inline ?   It makes
> > sense to me to be an inline if you want to strongly check the type of an
> > argument, but something simple like this, it's not so clear.  If I didn't
> > have the "cpu_to_be32" part, I'd abandon the inline and the macro and code
> > it directly.
> In this case they will both provide the same error checking, due to
> cpu_to_be32 returning type __be32. The only difference is that one adds
> a macro and the other adds a function, and if we ever need to add some
> caveats to the logic it will be simpler with the function. That and the
> coding style dictates that all things being equal, we should still
> prefer inline. Those are my only rationales in this case.

I generally prefer to avoid macros that make grepping hard.  But
I don't want to bikeshed too much over this this - I'd prefer Jon's
variant, but I'm not going to NAK the series over this helper.