[PATCH 1/3] 24-bit types: typedef and macros for accessing 3-byte arrays as integers

[PATCH 1/3] 24-bit types: typedef and macros for accessing 3-byte arrays as integers

[Chris Leech]

Both iSCSI and Fibre Channel make use of 24-bit big-endian values in
frame headers.  This patch defines __be24 and __le24 typedefs for a
structure wrapped around a 3-byte array, and macros to convert back and
forth to a 32-bit integer.

The undefs in iscsi_proto.h are because of the different calling
convention for the existing hton24 macro in the iSCSI code.  iSCSI will
be converted in a subsequent patch.

Signed-off-by: Chris Leech <christopher.leech-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---

 include/linux/byteorder.h         |   56 ++++++++++++++++++++++++++++++++++++
 include/linux/byteorder/generic.h |   57 +++++++++++++++++++++++++++++++++++++
 include/linux/types.h             |    2 +
 include/scsi/iscsi_proto.h        |    2 +
 4 files changed, 117 insertions(+), 0 deletions(-)


diff --git a/include/linux/byteorder.h b/include/linux/byteorder.h
index 29f002d..e41c17b 100644
--- a/include/linux/byteorder.h
+++ b/include/linux/byteorder.h
@@ -62,6 +62,54 @@
 # define __cpu_to_le64(x) ((__force __le64)__swab64(x))
 #endif
 
+/**
+ * __le24_to_cpu - read a 3-byte array as a 24-bit little-endian integer
+ * @x: __le24, a structure wrapper around a 3-byte array
+ *
+ * Evaluates to a __u32 integer type
+ */
+#define __le24_to_cpu(x) \
+({ \
+	__le24 _x = (x); \
+	(__u32) ((_x.b[2] << 16) | (_x.b[1] << 8) | (_x.b[0])); \
+})
+
+/**
+ * __cpu_to_le24 - store a value in a 3-byte array in little-endian format
+ * @x: __u32, there is no checking to ensure only the lower 24 bits are set
+ *
+ * Evaluates to an __le24 compound literal
+ */
+#define __cpu_to_le24(x) \
+({ \
+	__u32 _x = (x); \
+	(__le24) { .b = { _x & 0xff, (_x >> 8) & 0xff, (_x >> 16) & 0xff } }; \
+})
+
+/**
+ * __be24_to_cpu - read a 3-byte array as a 24-bit big-endian integer
+ * @x: __be24, a structure wrapper around a 3-byte array
+ *
+ * Evaluates to a __u32 integer type
+ */
+#define __be24_to_cpu(x) \
+({ \
+	__be24 _x = (x); \
+	(__u32) ((_x.b[0] << 16) | (_x.b[1] << 8) | (_x.b[2])); \
+})
+
+/**
+ * __cpu_to_be24 - store a value in a 3-byte array in big-endian format
+ * @x: __u32, there is no checking to ensure only the lower 24 bits are set
+ *
+ * Evaluates to an __be24 compound literal
+ */
+#define __cpu_to_be24(x) \
+({ \
+	__u32 _x = (x); \
+	(__be24) { .b = { (_x >> 16) & 0xff, (_x >> 8) & 0xff, _x & 0xff } }; \
+})
+
 /*
  * These helpers could be phased out over time as the base version
  * handles constant folding.
@@ -280,15 +328,19 @@ static inline __be64 __cpu_to_be64p(const __u64 *p)
 #ifdef __KERNEL__
 
 # define le16_to_cpu __le16_to_cpu
+# define le24_to_cpu __le24_to_cpu
 # define le32_to_cpu __le32_to_cpu
 # define le64_to_cpu __le64_to_cpu
 # define be16_to_cpu __be16_to_cpu
+# define be24_to_cpu __be24_to_cpu
 # define be32_to_cpu __be32_to_cpu
 # define be64_to_cpu __be64_to_cpu
 # define cpu_to_le16 __cpu_to_le16
+# define cpu_to_le24 __cpu_to_le24
 # define cpu_to_le32 __cpu_to_le32
 # define cpu_to_le64 __cpu_to_le64
 # define cpu_to_be16 __cpu_to_be16
+# define cpu_to_be24 __cpu_to_be24
 # define cpu_to_be32 __cpu_to_be32
 # define cpu_to_be64 __cpu_to_be64
 
@@ -332,11 +384,15 @@ static inline __be64 __cpu_to_be64p(const __u64 *p)
 # define ___htons(x) __cpu_to_be16(x)
 # define ___ntohl(x) __be32_to_cpu(x)
 # define ___ntohs(x) __be16_to_cpu(x)
+# define ___hton24(x) __cpu_to_be24(x)
+# define ___ntoh24(x) __be24_to_cpu(x)
 
 # define htonl(x) ___htonl(x)
 # define ntohl(x) ___ntohl(x)
 # define htons(x) ___htons(x)
 # define ntohs(x) ___ntohs(x)
+# define hton24(x) ___hton24(x)
+# define ntoh24(x) ___ntoh24(x)
 
 static inline void le16_add_cpu(__le16 *var, u16 val)
 {
diff --git a/include/linux/byteorder/generic.h b/include/linux/byteorder/generic.h
index 0846e6b..a57fbc3 100644
--- a/include/linux/byteorder/generic.h
+++ b/include/linux/byteorder/generic.h
@@ -119,6 +119,59 @@
 #define cpu_to_be16s __cpu_to_be16s
 #define be16_to_cpus __be16_to_cpus
 
+/**
+ * __le24_to_cpu - read a 3-byte array as a 24-bit little-endian integer
+ * @x: __le24, a structure wrapper around a 3-byte array
+ *
+ * Evaluates to a __u32 integer type
+ */
+#define __le24_to_cpu(x) \
+({ \
+	__le24 _x = (x); \
+	(__u32) ((_x.b[2] << 16) | (_x.b[1] << 8) | (_x.b[0])); \
+})
+
+/**
+ * __cpu_to_le24 - store a value in a 3-byte array in little-endian format
+ * @x: __u32, there is no checking to ensure only the lower 24 bits are set
+ *
+ * Evaluates to an __le24 compound literal
+ */
+#define __cpu_to_le24(x) \
+({ \
+	__u32 _x = (x); \
+	(__le24) { .b = { _x & 0xff, (_x >> 8) & 0xff, (_x >> 16) & 0xff } }; \
+})
+
+/**
+ * __be24_to_cpu - read a 3-byte array as a 24-bit big-endian integer
+ * @x: __be24, a structure wrapper around a 3-byte array
+ *
+ * Evaluates to a __u32 integer type
+ */
+#define __be24_to_cpu(x) \
+({ \
+	__be24 _x = (x); \
+	(__u32) ((_x.b[0] << 16) | (_x.b[1] << 8) | (_x.b[2])); \
+})
+
+/**
+ * __cpu_to_be24 - store a value in a 3-byte array in big-endian format
+ * @x: __u32, there is no checking to ensure only the lower 24 bits are set
+ *
+ * Evaluates to an __be24 compound literal
+ */
+#define __cpu_to_be24(x) \
+({ \
+	__u32 _x = (x); \
+	(__be24) { .b = { (_x >> 16) & 0xff, (_x >> 8) & 0xff, _x & 0xff } }; \
+})
+
+#define le24_to_cpu __le24_to_cpu
+#define cpu_to_le24 __cpu_to_le24
+#define be24_to_cpu __be24_to_cpu
+#define cpu_to_be24 __cpu_to_be24
+
 /*
  * They have to be macros in order to do the constant folding
  * correctly - if the argument passed into a inline function
@@ -134,11 +187,15 @@
 #define ___htons(x) __cpu_to_be16(x)
 #define ___ntohl(x) __be32_to_cpu(x)
 #define ___ntohs(x) __be16_to_cpu(x)
+#define ___hton24(x) __cpu_to_be24(x)
+#define ___ntoh24(x) __be24_to_cpu(x)
 
 #define htonl(x) ___htonl(x)
 #define ntohl(x) ___ntohl(x)
 #define htons(x) ___htons(x)
 #define ntohs(x) ___ntohs(x)
+#define hton24(x) ___hton24(x)
+#define ntoh24(x) ___ntoh24(x)
 
 static inline void le16_add_cpu(__le16 *var, u16 val)
 {
diff --git a/include/linux/types.h b/include/linux/types.h
index d4a9ce6..85fcff7 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -188,6 +188,8 @@ typedef __u64 __bitwise __be64;
 typedef __u16 __bitwise __sum16;
 typedef __u32 __bitwise __wsum;
 
+typedef struct { __u8 b[3]; } __be24, __le24;
+
 #ifdef __KERNEL__
 typedef unsigned __bitwise__ gfp_t;
 
diff --git a/include/scsi/iscsi_proto.h b/include/scsi/iscsi_proto.h
index f2a2c11..429c5ff 100644
--- a/include/scsi/iscsi_proto.h
+++ b/include/scsi/iscsi_proto.h
@@ -35,6 +35,8 @@
 /*
  * useful common(control and data pathes) macro
  */
+#undef ntoh24
+#undef hton24
 #define ntoh24(p) (((p)[0] << 16) | ((p)[1] << 8) | ((p)[2]))
 #define hton24(p, v) { \
         p[0] = (((v) >> 16) & 0xFF); \

[PATCH 2/3] 24-bit types: convert iSCSI to use the __be24 type and macros

[Chris Leech]

The calling convention of hton24 is different now, it takes a single u32
as an argument and evaluates to a __be24 lvalue.

Signed-off-by: Chris Leech <christopher.leech-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---

 drivers/scsi/iscsi_tcp.c   |    8 ++++----
 drivers/scsi/libiscsi.c    |    6 +++---
 include/scsi/iscsi_proto.h |   46 ++++++++++++++++++--------------------------
 3 files changed, 26 insertions(+), 34 deletions(-)


diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 2a2f009..c9d6e96 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -608,11 +608,11 @@ iscsi_solicit_data_init(struct iscsi_conn *conn, struct iscsi_task *task,
 	hdr->exp_statsn = r2t->exp_statsn;
 	hdr->offset = cpu_to_be32(r2t->data_offset);
 	if (r2t->data_length > conn->max_xmit_dlength) {
-		hton24(hdr->dlength, conn->max_xmit_dlength);
+		hdr->dlength = hton24(conn->max_xmit_dlength);
 		r2t->data_count = conn->max_xmit_dlength;
 		hdr->flags = 0;
 	} else {
-		hton24(hdr->dlength, r2t->data_length);
+		hdr->dlength = hton24(r2t->data_length);
 		r2t->data_count = r2t->data_length;
 		hdr->flags = ISCSI_FLAG_CMD_FINAL;
 	}
@@ -1311,10 +1311,10 @@ iscsi_solicit_data_cont(struct iscsi_conn *conn, struct iscsi_task *task,
 	new_offset = r2t->data_offset + r2t->sent;
 	hdr->offset = cpu_to_be32(new_offset);
 	if (left > conn->max_xmit_dlength) {
-		hton24(hdr->dlength, conn->max_xmit_dlength);
+		hdr->dlength = hton24(conn->max_xmit_dlength);
 		r2t->data_count = conn->max_xmit_dlength;
 	} else {
-		hton24(hdr->dlength, left);
+		hdr->dlength = hton24(left);
 		r2t->data_count = left;
 		hdr->flags = ISCSI_FLAG_CMD_FINAL;
 	}
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 299e075..8432318 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -105,12 +105,12 @@ void iscsi_prep_unsolicit_data_pdu(struct iscsi_task *task,
 	hdr->offset = cpu_to_be32(task->unsol_offset);
 
 	if (task->unsol_count > conn->max_xmit_dlength) {
-		hton24(hdr->dlength, conn->max_xmit_dlength);
+		hdr->dlength = hton24(conn->max_xmit_dlength);
 		task->data_count = conn->max_xmit_dlength;
 		task->unsol_offset += task->data_count;
 		hdr->flags = 0;
 	} else {
-		hton24(hdr->dlength, task->unsol_count);
+		hdr->dlength = hton24(task->unsol_count);
 		task->data_count = task->unsol_count;
 		hdr->flags = ISCSI_FLAG_CMD_FINAL;
 	}
@@ -269,7 +269,7 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task *task)
 			else
 				task->imm_count = min(out_len,
 							conn->max_xmit_dlength);
-			hton24(hdr->dlength, task->imm_count);
+			hdr->dlength = hton24(task->imm_count);
 		} else
 			zero_data(hdr->dlength);
 
diff --git a/include/scsi/iscsi_proto.h b/include/scsi/iscsi_proto.h
index 429c5ff..a778097 100644
--- a/include/scsi/iscsi_proto.h
+++ b/include/scsi/iscsi_proto.h
@@ -35,15 +35,7 @@
 /*
  * useful common(control and data pathes) macro
  */
-#undef ntoh24
-#undef hton24
-#define ntoh24(p) (((p)[0] << 16) | ((p)[1] << 8) | ((p)[2]))
-#define hton24(p, v) { \
-        p[0] = (((v) >> 16) & 0xFF); \
-        p[1] = (((v) >> 8) & 0xFF); \
-        p[2] = ((v) & 0xFF); \
-}
-#define zero_data(p) {p[0]=0;p[1]=0;p[2]=0;}
+#define zero_data(x) (x) = (__be24) { .b = { 0, 0, 0 } }
 
 /* initiator tags; opaque for target */
 typedef uint32_t __bitwise__ itt_t;
@@ -61,7 +53,7 @@ struct iscsi_hdr {
 	uint8_t		flags;		/* Final bit */
 	uint8_t		rsvd2[2];
 	uint8_t		hlength;	/* AHSs total length */
-	uint8_t		dlength[3];	/* Data length */
+	__be24		dlength;	/* Data length */
 	uint8_t		lun[8];
 	itt_t		itt;		/* Initiator Task Tag, opaque for target */
 	__be32		ttt;		/* Target Task Tag */
@@ -123,7 +115,7 @@ struct iscsi_cmd {
 	uint8_t flags;
 	__be16 rsvd2;
 	uint8_t hlength;
-	uint8_t dlength[3];
+	__be24 dlength;
 	uint8_t lun[8];
 	itt_t	 itt;	/* Initiator Task Tag */
 	__be32 data_length;
@@ -169,7 +161,7 @@ struct iscsi_cmd_rsp {
 	uint8_t response;
 	uint8_t cmd_status;
 	uint8_t hlength;
-	uint8_t dlength[3];
+	__be24 dlength;
 	uint8_t rsvd[8];
 	itt_t	 itt;	/* Initiator Task Tag */
 	__be32	rsvd1;
@@ -199,7 +191,7 @@ struct iscsi_async {
 	uint8_t flags;
 	uint8_t rsvd2[2];
 	uint8_t rsvd3;
-	uint8_t dlength[3];
+	__be24 dlength;
 	uint8_t lun[8];
 	uint8_t rsvd4[8];
 	__be32	statsn;
@@ -227,7 +219,7 @@ struct iscsi_nopout {
 	uint8_t flags;
 	__be16	rsvd2;
 	uint8_t rsvd3;
-	uint8_t dlength[3];
+	__be24 dlength;
 	uint8_t lun[8];
 	itt_t	 itt;	/* Initiator Task Tag */
 	__be32	ttt;	/* Target Transfer Tag */
@@ -242,7 +234,7 @@ struct iscsi_nopin {
 	uint8_t flags;
 	__be16	rsvd2;
 	uint8_t rsvd3;
-	uint8_t dlength[3];
+	__be24 dlength;
 	uint8_t lun[8];
 	itt_t	 itt;	/* Initiator Task Tag */
 	__be32	ttt;	/* Target Transfer Tag */
@@ -258,7 +250,7 @@ struct iscsi_tm {
 	uint8_t flags;
 	uint8_t rsvd1[2];
 	uint8_t hlength;
-	uint8_t dlength[3];
+	__be24 dlength;
 	uint8_t lun[8];
 	itt_t	 itt;	/* Initiator Task Tag */
 	itt_t	 rtt;	/* Reference Task Tag */
@@ -288,7 +280,7 @@ struct iscsi_tm_rsp {
 	uint8_t response;	/* see Response values below */
 	uint8_t qualifier;
 	uint8_t hlength;
-	uint8_t dlength[3];
+	__be24 dlength;
 	uint8_t rsvd2[8];
 	itt_t	 itt;	/* Initiator Task Tag */
 	itt_t	 rtt;	/* Reference Task Tag */
@@ -314,7 +306,7 @@ struct iscsi_r2t_rsp {
 	uint8_t flags;
 	uint8_t rsvd2[2];
 	uint8_t	hlength;
-	uint8_t	dlength[3];
+	__be24	dlength;
 	uint8_t lun[8];
 	itt_t	 itt;	/* Initiator Task Tag */
 	__be32	ttt;	/* Target Transfer Tag */
@@ -332,7 +324,7 @@ struct iscsi_data {
 	uint8_t flags;
 	uint8_t rsvd2[2];
 	uint8_t rsvd3;
-	uint8_t dlength[3];
+	__be24 dlength;
 	uint8_t lun[8];
 	itt_t	 itt;
 	__be32	ttt;
@@ -352,7 +344,7 @@ struct iscsi_data_rsp {
 	uint8_t rsvd2;
 	uint8_t cmd_status;
 	uint8_t hlength;
-	uint8_t dlength[3];
+	__be24 dlength;
 	uint8_t lun[8];
 	itt_t	 itt;
 	__be32	ttt;
@@ -376,7 +368,7 @@ struct iscsi_text {
 	uint8_t flags;
 	uint8_t rsvd2[2];
 	uint8_t hlength;
-	uint8_t dlength[3];
+	__be24 dlength;
 	uint8_t rsvd4[8];
 	itt_t	 itt;
 	__be32	ttt;
@@ -394,7 +386,7 @@ struct iscsi_text_rsp {
 	uint8_t flags;
 	uint8_t rsvd2[2];
 	uint8_t hlength;
-	uint8_t dlength[3];
+	__be24 dlength;
 	uint8_t rsvd4[8];
 	itt_t	 itt;
 	__be32	ttt;
@@ -412,7 +404,7 @@ struct iscsi_login {
 	uint8_t max_version;	/* Max. version supported */
 	uint8_t min_version;	/* Min. version supported */
 	uint8_t hlength;
-	uint8_t dlength[3];
+	__be24 dlength;
 	uint8_t isid[6];	/* Initiator Session ID */
 	__be16	tsih;	/* Target Session Handle */
 	itt_t	 itt;	/* Initiator Task Tag */
@@ -441,7 +433,7 @@ struct iscsi_login_rsp {
 	uint8_t max_version;	/* Max. version supported */
 	uint8_t active_version;	/* Active version */
 	uint8_t hlength;
-	uint8_t dlength[3];
+	__be24 dlength;
 	uint8_t isid[6];	/* Initiator Session ID */
 	__be16	tsih;	/* Target Session Handle */
 	itt_t	 itt;	/* Initiator Task Tag */
@@ -499,7 +491,7 @@ struct iscsi_logout {
 	uint8_t flags;
 	uint8_t rsvd1[2];
 	uint8_t hlength;
-	uint8_t dlength[3];
+	__be24 dlength;
 	uint8_t rsvd2[8];
 	itt_t	 itt;	/* Initiator Task Tag */
 	__be16	cid;
@@ -526,7 +518,7 @@ struct iscsi_logout_rsp {
 	uint8_t response;	/* see Logout response values below */
 	uint8_t rsvd2;
 	uint8_t hlength;
-	uint8_t dlength[3];
+	__be24 dlength;
 	uint8_t rsvd3[8];
 	itt_t	 itt;	/* Initiator Task Tag */
 	__be32	rsvd4;
@@ -570,7 +562,7 @@ struct iscsi_reject {
 	uint8_t reason;
 	uint8_t rsvd2;
 	uint8_t hlength;
-	uint8_t dlength[3];
+	__be24 dlength;
 	uint8_t rsvd3[8];
 	__be32  ffffffff;
 	uint8_t rsvd4[4];

[PATCH 3/3] 24-bit types: Convert Open-FCoE to use __be24 type and macros

[Chris Leech]

This converts the Open-FCoE tree to use the common 24-bit types

Signed-off-by: Chris Leech <christopher.leech-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---

 drivers/scsi/fcoe/fcoe_dev.c  |    6 +++---
 drivers/scsi/libfc/fc_exch.c  |   24 ++++++++++++------------
 drivers/scsi/libfc/fc_fcp.c   |    2 +-
 drivers/scsi/libfc/fc_lport.c |    8 ++++----
 drivers/scsi/libfc/fc_ns.c    |   14 +++++++-------
 drivers/scsi/libfc/fc_rport.c |    4 ++--
 include/scsi/fc/fc_els.h      |   18 +++++++++---------
 include/scsi/fc/fc_fcoe.h     |    6 ++----
 include/scsi/fc/fc_fs.h       |    8 ++++----
 include/scsi/fc/fc_gs.h       |    2 +-
 include/scsi/fc/fc_ns.h       |    6 +++---
 include/scsi/libfc/libfc.h    |   13 -------------
 12 files changed, 48 insertions(+), 63 deletions(-)


diff --git a/drivers/scsi/fcoe/fcoe_dev.c b/drivers/scsi/fcoe/fcoe_dev.c
index d5a354f..d5fc479 100644
--- a/drivers/scsi/fcoe/fcoe_dev.c
+++ b/drivers/scsi/fcoe/fcoe_dev.c
@@ -233,7 +233,7 @@ int fcoe_xmit(struct fc_lport *lp, struct fc_frame *fp)
 			 * MAC in this case would have been set by receving the
 			 * FLOGI.
 			 */
-			fc_fcoe_set_mac(fc->data_src_addr, fh->fh_s_id);
+			fc_fcoe_set_mac(fc->data_src_addr, &fh->fh_s_id);
 			fc->flogi_progress = 0;
 		}
 	}
@@ -311,7 +311,7 @@ int fcoe_xmit(struct fc_lport *lp, struct fc_frame *fp)
 	skb->ip_summed = CHECKSUM_NONE;
 	eh = (struct ethhdr *)skb_push(skb, hlen + sizeof(struct ethhdr));
 	if (fc->address_mode == FCOE_FCOUI_ADDR_MODE)
-		fc_fcoe_set_mac(eh->h_dest, fh->fh_d_id);
+		fc_fcoe_set_mac(eh->h_dest, &fh->fh_d_id);
 	else
 		/* insert GW address */
 		memcpy(eh->h_dest, fc->dest_addr, ETH_ALEN);
@@ -516,7 +516,7 @@ static void fcoe_recv_flogi(struct fcoe_softc *fc, struct fc_frame *fp, u8 *sa)
 		if (compare_ether_addr(fc->data_src_addr, (u8[6]) { 0 }))
 			dev_unicast_delete(fc->real_dev, fc->data_src_addr,
 					   ETH_ALEN);
-		fc_fcoe_set_mac(fc->data_src_addr, fh->fh_d_id);
+		fc_fcoe_set_mac(fc->data_src_addr, &fh->fh_d_id);
 		dev_unicast_add(fc->real_dev, fc->data_src_addr, ETH_ALEN);
 		rtnl_unlock();
 
diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
index 11a03bd..9fd011b 100644
--- a/drivers/scsi/libfc/fc_exch.c
+++ b/drivers/scsi/libfc/fc_exch.c
@@ -278,8 +278,8 @@ static void fc_seq_fill_hdr(struct fc_seq *sp, struct fc_frame *fp)
 
 	ep = fc_seq_exch(sp);
 
-	hton24(fh->fh_s_id, ep->sid);
-	hton24(fh->fh_d_id, ep->did);
+	fh->fh_s_id = hton24(ep->sid);
+	fh->fh_d_id = hton24(ep->did);
 	fh->fh_ox_id = htons(ep->oxid);
 	fh->fh_rx_id = htons(ep->rxid);
 	fh->fh_seq_id = sp->id;
@@ -892,7 +892,7 @@ int fc_seq_send(struct fc_lport *lp, struct fc_seq *sp,
 		fr_eof(fp) = FC_EOF_N;
 	}
 
-	hton24(fh->fh_f_ctl, f_ctl | fill);
+	fh->fh_f_ctl = hton24(f_ctl | fill);
 	fh->fh_seq_cnt = htons(sp->cnt++);
 
 	/*
@@ -992,7 +992,7 @@ static void fc_seq_send_ack(struct fc_seq *sp, const struct fc_frame *rx_fp)
 			FC_FC_END_SEQ | FC_FC_END_CONN | FC_FC_SEQ_INIT |
 			FC_FC_RETX_SEQ | FC_FC_UNI_TX;
 		f_ctl ^= FC_FC_EX_CTX | FC_FC_SEQ_CTX;
-		hton24(fh->fh_f_ctl, f_ctl);
+		fh->fh_f_ctl = hton24(f_ctl);
 
 		fh->fh_seq_id = rx_fh->fh_seq_id;
 		fh->fh_seq_cnt = rx_fh->fh_seq_cnt;
@@ -1039,8 +1039,8 @@ fc_exch_send_ba_rjt(struct fc_frame *rx_fp, enum fc_ba_rjt_reason reason,
 	/*
 	 * seq_id, cs_ctl, df_ctl and param/offset are zero.
 	 */
-	memcpy(fh->fh_s_id, rx_fh->fh_d_id, 3);
-	memcpy(fh->fh_d_id, rx_fh->fh_s_id, 3);
+	fh->fh_s_id = rx_fh->fh_d_id;
+	fh->fh_d_id = rx_fh->fh_s_id;
 	fh->fh_ox_id = rx_fh->fh_rx_id;
 	fh->fh_rx_id = rx_fh->fh_ox_id;
 	fh->fh_seq_cnt = rx_fh->fh_seq_cnt;
@@ -1062,7 +1062,7 @@ fc_exch_send_ba_rjt(struct fc_frame *rx_fp, enum fc_ba_rjt_reason reason,
 	f_ctl ^= FC_FC_EX_CTX | FC_FC_SEQ_CTX;
 	f_ctl |= FC_FC_LAST_SEQ | FC_FC_END_SEQ;
 	f_ctl &= ~FC_FC_FIRST_SEQ;
-	hton24(fh->fh_f_ctl, f_ctl);
+	fh->fh_f_ctl = hton24(f_ctl);
 
 	fr_sof(fp) = fc_sof_class(fr_sof(rx_fp));
 	fr_eof(fp) = FC_EOF_T;
@@ -1607,12 +1607,12 @@ static void fc_exch_els_rec(struct fc_seq *sp, struct fc_frame *rfp)
 	memset(acc, 0, sizeof(*acc));
 	acc->reca_cmd = ELS_LS_ACC;
 	acc->reca_ox_id = rp->rec_ox_id;
-	memcpy(acc->reca_ofid, rp->rec_s_id, 3);
+	acc->reca_ofid = rp->rec_s_id;
 	acc->reca_rx_id = htons(ep->rxid);
 	if (ep->sid == ep->oid)
-		hton24(acc->reca_rfid, ep->did);
+		acc->reca_rfid = hton24(ep->did);
 	else
-		hton24(acc->reca_rfid, ep->sid);
+		acc->reca_rfid = hton24(ep->sid);
 	acc->reca_fc4value = htonl(ep->seq.rec_data);
 	acc->reca_e_stat = htonl(ep->esb_stat & (ESB_ST_RESP |
 						  ESB_ST_SEQ_INIT |
@@ -1704,7 +1704,7 @@ static void fc_exch_rrq(struct fc_exch *ep)
 	rrq = fc_frame_payload_get(fp, sizeof(*rrq));
 	memset(rrq, 0, sizeof(*rrq));
 	rrq->rrq_cmd = ELS_RRQ;
-	hton24(rrq->rrq_s_id, ep->sid);
+	rrq->rrq_s_id = hton24(ep->sid);
 	rrq->rrq_ox_id = htons(ep->oxid);
 	rrq->rrq_rx_id = htons(ep->rxid);
 
@@ -1906,7 +1906,7 @@ struct fc_seq *fc_exch_seq_send(struct fc_lport *lp,
 	}
 	f_ctl |= ep->f_ctl;
 	fh = fc_frame_header_get(fp);
-	hton24(fh->fh_f_ctl, f_ctl | fill);
+	fh->fh_f_ctl = hton24(f_ctl | fill);
 	fh->fh_seq_cnt = htons(sp->cnt++);
 
 	if (unlikely(lp->tt.frame_send(lp, fp)))
diff --git a/drivers/scsi/libfc/fc_fcp.c b/drivers/scsi/libfc/fc_fcp.c
index 9402eba..6c24724 100644
--- a/drivers/scsi/libfc/fc_fcp.c
+++ b/drivers/scsi/libfc/fc_fcp.c
@@ -1332,7 +1332,7 @@ static void fc_fcp_rec(struct fc_fcp_pkt *fsp)
 	rec = fc_frame_payload_get(fp, sizeof(*rec));
 	memset(rec, 0, sizeof(*rec));
 	rec->rec_cmd = ELS_REC;
-	hton24(rec->rec_s_id, lp->fid);
+	rec->rec_s_id = hton24(lp->fid);
 	rec->rec_ox_id = htons(ox_id);
 	rec->rec_rx_id = htons(rx_id);
 
diff --git a/drivers/scsi/libfc/fc_lport.c b/drivers/scsi/libfc/fc_lport.c
index b06f519..ad5c5b1 100644
--- a/drivers/scsi/libfc/fc_lport.c
+++ b/drivers/scsi/libfc/fc_lport.c
@@ -244,8 +244,8 @@ static void fc_lport_rnid_req(struct fc_seq *sp, struct fc_frame *in_fp,
 			rp->rnid.rnid_cmd = ELS_LS_ACC;
 			rp->rnid.rnid_fmt = fmt;
 			rp->rnid.rnid_cid_len = sizeof(rp->cid);
-			rp->cid.rnid_wwpn = htonll(lp->wwpn);
-			rp->cid.rnid_wwnn = htonll(lp->wwnn);
+			rp->cid.rnid_wwpn = cpu_to_be64(lp->wwpn);
+			rp->cid.rnid_wwnn = cpu_to_be64(lp->wwnn);
 			if (fmt == ELS_RNIDF_GEN) {
 				rp->rnid.rnid_sid_len = sizeof(rp->gen);
 				memcpy(&rp->gen, &lp->rnid_gen,
@@ -747,8 +747,8 @@ static void fc_lport_enter_logo(struct fc_lport *lp)
 	logo = fc_frame_payload_get(fp, sizeof(*logo));
 	memset(logo, 0, sizeof(*logo));
 	logo->fl_cmd = ELS_LOGO;
-	hton24(logo->fl_n_port_id, lp->fid);
-	logo->fl_n_port_wwn = htonll(lp->wwpn);
+	logo->fl_n_port_id = hton24(lp->fid);
+	logo->fl_n_port_wwn = cpu_to_be64(lp->wwpn);
 
 	fc_frame_setup(fp, FC_RCTL_ELS_REQ, FC_TYPE_ELS);
 	fc_frame_set_offset(fp, 0);
diff --git a/drivers/scsi/libfc/fc_ns.c b/drivers/scsi/libfc/fc_ns.c
index 6204bcc..c280913 100644
--- a/drivers/scsi/libfc/fc_ns.c
+++ b/drivers/scsi/libfc/fc_ns.c
@@ -265,7 +265,7 @@ static void fc_ns_enter_reg_ft(struct fc_lport *lp)
 					      FC_NS_RFT_ID,
 					      sizeof(*req) -
 					      sizeof(struct fc_ct_hdr));
-			hton24(req->fid.fp_fid, lp->fid);
+			req->fid.fp_fid = hton24(lp->fid);
 			req->fts = *lps;
 			fc_frame_setup(fp, FC_RCTL_DD_UNSOL_CTL, FC_TYPE_CT);
 			if (!lp->tt.exch_seq_send(lp, fp,
@@ -774,7 +774,7 @@ static int fc_ns_gpn_ft_parse(struct fc_lport *lp, void *buf, size_t len)
 			break;
 		dp->lp = lp;
 		dp->ids.port_id = ntoh24(np->fp_fid);
-		dp->ids.port_name = ntohll(np->fp_wwpn);
+		dp->ids.port_name = be64_to_cpu(np->fp_wwpn);
 		dp->ids.node_name = -1;
 		dp->ids.roles = FC_RPORT_ROLE_UNKNOWN;
 		error = fc_ns_gnn_id_req(lp, dp);
@@ -934,7 +934,7 @@ static int fc_ns_gpn_id_req(struct fc_lport *lp, struct fc_ns_port *dp)
 
 	cp = fc_frame_payload_get(fp, sizeof(*cp));
 	fc_ns_fill_dns_hdr(lp, &cp->ct, FC_NS_GPN_ID, sizeof(cp->fid));
-	hton24(cp->fid.fp_fid, dp->ids.port_id);
+	cp->fid.fp_fid = hton24(dp->ids.port_id);
 
 	WARN_ON(!fc_lport_test_ready(lp));
 
@@ -989,7 +989,7 @@ static void fc_ns_gpn_id_resp(struct fc_seq *sp, struct fc_frame *fp,
 			       fr_len(fp));
 			break;
 		}
-		dp->ids.port_name = ntohll(cp->wwn);
+		dp->ids.port_name = be64_to_cpu(cp->wwn);
 		fc_ns_gnn_id_req(lp, dp);
 		break;
 	case FC_FS_RJT:
@@ -1137,7 +1137,7 @@ static void fc_ns_enter_reg_pn(struct fc_lport *lp)
 	req = fc_frame_payload_get(fp, sizeof(*req));
 	memset(req, 0, sizeof(*req));
 	fc_lport_fill_dns_hdr(lp, &req->ct, FC_NS_RPN_ID, sizeof(req->rn));
-	hton24(req->rn.fr_fid.fp_fid, lp->fid);
+	req->rn.fr_fid.fp_fid = hton24(lp->fid);
 	put_unaligned_be64(lp->wwpn, &req->rn.fr_wwn);
 	fc_frame_setup(fp, FC_RCTL_DD_UNSOL_CTL, FC_TYPE_CT);
 	if (!lp->tt.exch_seq_send(lp, fp,
@@ -1191,7 +1191,7 @@ static int fc_ns_gnn_id_req(struct fc_lport *lp, struct fc_ns_port *dp)
 
 	cp = fc_frame_payload_get(fp, sizeof(*cp));
 	fc_ns_fill_dns_hdr(lp, &cp->ct, FC_NS_GNN_ID, sizeof(cp->fid));
-	hton24(cp->fid.fp_fid, dp->ids.port_id);
+	cp->fid.fp_fid = hton24(dp->ids.port_id);
 
 	WARN_ON(!fc_lport_test_ready(lp));
 
@@ -1246,7 +1246,7 @@ static void fc_ns_gnn_id_resp(struct fc_seq *sp, struct fc_frame *fp,
 			       fr_len(fp));
 			break;
 		}
-		dp->ids.node_name = ntohll(cp->wwn);
+		dp->ids.node_name = be64_to_cpu(cp->wwn);
 		fc_ns_new_target(lp, NULL, &dp->ids);
 		break;
 	case FC_FS_RJT:
diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
index cd559dd..0ca4c12 100644
--- a/drivers/scsi/libfc/fc_rport.c
+++ b/drivers/scsi/libfc/fc_rport.c
@@ -857,8 +857,8 @@ static void fc_rport_enter_logo(struct fc_rport *rport)
 	logo = fc_frame_payload_get(fp, sizeof(*logo));
 	memset(logo, 0, sizeof(*logo));
 	logo->fl_cmd = ELS_LOGO;
-	hton24(logo->fl_n_port_id, lp->fid);
-	logo->fl_n_port_wwn = htonll(lp->wwpn);
+	logo->fl_n_port_id = hton24(lp->fid);
+	logo->fl_n_port_wwn = cpu_to_be64(lp->wwpn);
 
 	fc_frame_setup(fp, FC_RCTL_ELS_REQ, FC_TYPE_ELS);
 	if (!lp->tt.exch_seq_send(lp, fp,
diff --git a/include/scsi/fc/fc_els.h b/include/scsi/fc/fc_els.h
index af4bf0c..c1cfe21 100644
--- a/include/scsi/fc/fc_els.h
+++ b/include/scsi/fc/fc_els.h
@@ -357,7 +357,7 @@ struct fc_els_rrq {
 	__u8		rrq_cmd;	/* command (0x12) */
 	__u8		rrq_zero[3];	/* specified as zero - part of cmd */
 	__u8		rrq_resvd;	/* reserved */
-	__u8		rrq_s_id[3];	/* originator FID */
+	__be24		rrq_s_id;	/* originator FID */
 	__be16		rrq_ox_id;	/* originator exchange ID */
 	__be16		rrq_rx_id;	/* responders exchange ID */
 };
@@ -369,7 +369,7 @@ struct fc_els_rec {
 	__u8		rec_cmd;	/* command (0x13) */
 	__u8		rec_zero[3];	/* specified as zero - part of cmd */
 	__u8		rec_resvd;	/* reserved */
-	__u8		rec_s_id[3];	/* originator FID */
+	__be24		rec_s_id;	/* originator FID */
 	__be16		rec_ox_id;	/* originator exchange ID */
 	__be16		rec_rx_id;	/* responders exchange ID */
 };
@@ -383,9 +383,9 @@ struct fc_els_rec_acc {
 	__be16		reca_ox_id;	/* originator exchange ID */
 	__be16		reca_rx_id;	/* responders exchange ID */
 	__u8		reca_resvd1;	/* reserved */
-	__u8		reca_ofid[3];	/* originator FID */
+	__be24		reca_ofid;	/* originator FID */
 	__u8		reca_resvd2;	/* reserved */
-	__u8		reca_rfid[3];	/* responder FID */
+	__be24		reca_rfid;	/* responder FID */
 	__be32		reca_fc4value;	/* FC4 value */
 	__be32		reca_e_stat;	/* ESB (exchange status block) status */
 };
@@ -407,7 +407,7 @@ struct fc_els_logo {
 	__u8		fl_cmd;		/* command code */
 	__u8		fl_zero[3];	/* specified as zero - part of cmd */
 	__u8		fl_resvd;	/* reserved */
-	__u8		fl_n_port_id[3];/* N port ID */
+	__be24		fl_n_port_id;	/* N port ID */
 	__be64		fl_n_port_wwn;	/* port name */
 };
 
@@ -465,7 +465,7 @@ struct fc_els_rscn {
 
 struct fc_els_rscn_page {
 	__u8		rscn_page_flags; /* event and address format */
-	__u8		rscn_fid[3];	/* fabric ID */
+	__be24		rscn_fid;	/* fabric ID */
 };
 
 #define	ELS_RSCN_EV_QUAL_BIT	2	/* shift count for event qualifier */
@@ -598,7 +598,7 @@ struct fc_els_rpl {
 struct fc_els_pnb {
 	__be32		pnb_phys_pn;	/* physical port number */
 	__u8		pnb_resv;	/* reserved */
-	__u8		pnb_port_id[3];	/* port ID */
+	__be24		pnb_port_id;	/* port ID */
 	__be64		pnb_wwpn;	/* port name */
 };
 
@@ -709,7 +709,7 @@ enum fc_els_srl_flag {
 struct fc_els_rls {
 	__u8		rls_cmd;	/* command */
 	__u8		rls_resv[4];	/* reserved - must be zero */
-	__u8		rls_port_id[3];	/* port ID */
+	__be24		rls_port_id;	/* port ID */
 };
 
 /*
@@ -741,7 +741,7 @@ struct fc_els_clir {
 	__be64		clir_wwpn;	/* incident port name */
 	__be64		clir_wwnn;	/* incident port node name */
 	__u8		clir_port_type;	/* incident port type */
-	__u8		clir_port_id[3];	/* incident port ID */
+	__be24		clir_port_id;	/* incident port ID */
 
 	__be64		clir_conn_wwpn;	/* connected port name */
 	__be64		clir_conn_wwnn;	/* connected node name */
diff --git a/include/scsi/fc/fc_fcoe.h b/include/scsi/fc/fc_fcoe.h
index b2e07ec..4c0f692 100644
--- a/include/scsi/fc/fc_fcoe.h
+++ b/include/scsi/fc/fc_fcoe.h
@@ -83,14 +83,12 @@ struct fcoe_crc_eof {
 /*
  * Store OUI + DID into MAC address field.
  */
-static inline void fc_fcoe_set_mac(u8 *mac, u8 *did)
+static inline void fc_fcoe_set_mac(u8 *mac, __be24 *did)
 {
 	mac[0] = (u8) (FC_FCOE_OUI >> 16);
 	mac[1] = (u8) (FC_FCOE_OUI >> 8);
 	mac[2] = (u8) FC_FCOE_OUI;
-	mac[3] = did[0];
-	mac[4] = did[1];
-	mac[5] = did[2];
+	*(__be24 *) &mac[3] = *did;
 }
 
 /*
diff --git a/include/scsi/fc/fc_fs.h b/include/scsi/fc/fc_fs.h
index ba6df64..125ba15 100644
--- a/include/scsi/fc/fc_fs.h
+++ b/include/scsi/fc/fc_fs.h
@@ -29,14 +29,14 @@
  * Frame header
  */
 struct fc_frame_header {
-	__u8          fh_r_ctl;	/* routing control */
-	__u8          fh_d_id[3];	/* Destination ID */
+	__u8          fh_r_ctl;		/* routing control */
+	__be24        fh_d_id;		/* Destination ID */
 
 	__u8          fh_cs_ctl;	/* class of service control / pri */
-	__u8          fh_s_id[3];	/* Source ID */
+	__be24        fh_s_id;		/* Source ID */
 
 	__u8          fh_type;		/* see enum fc_fh_type below */
-	__u8          fh_f_ctl[3];	/* frame control */
+	__be24        fh_f_ctl;		/* frame control */
 
 	__u8          fh_seq_id;	/* sequence ID */
 	__u8          fh_df_ctl;	/* data field control */
diff --git a/include/scsi/fc/fc_gs.h b/include/scsi/fc/fc_gs.h
index ffab027..fb082bf 100644
--- a/include/scsi/fc/fc_gs.h
+++ b/include/scsi/fc/fc_gs.h
@@ -27,7 +27,7 @@
 
 struct fc_ct_hdr {
 	__u8		ct_rev;		/* revision */
-	__u8		ct_in_id[3];	/* N_Port ID of original requestor */
+	__be24		ct_in_id;	/* N_Port ID of original requestor */
 	__u8		ct_fs_type;	/* type of fibre channel service */
 	__u8		ct_fs_subtype;	/* subtype */
 	__u8		ct_options;
diff --git a/include/scsi/fc/fc_ns.h b/include/scsi/fc/fc_ns.h
index 790d7b9..44c3019 100644
--- a/include/scsi/fc/fc_ns.h
+++ b/include/scsi/fc/fc_ns.h
@@ -76,7 +76,7 @@ struct fc_ns_pt_obj {
  */
 struct fc_ns_fid {
 	__u8		fp_flags;	/* flags for responses only */
-	__u8		fp_fid[3];
+	__be24		fp_fid;
 };
 
 /*
@@ -119,7 +119,7 @@ struct fc_ns_gid_ft {
  */
 struct fc_gpn_ft_resp {
 	__u8		fp_flags;	/* see fp_flags definitions above */
-	__u8		fp_fid[3];	/* port ID */
+	__be24		fp_fid;		/* port ID */
 	__be32		fp_resvd;
 	__be64		fp_wwpn;	/* port name */
 };
@@ -136,7 +136,7 @@ struct fc_ns_gid_pn {
  */
 struct fc_gid_pn_resp {
 	__u8      fp_resvd;
-	__u8      fp_fid[3];     /* port ID */
+	__be24    fp_fid;     /* port ID */
 };
 
 /*
diff --git a/include/scsi/libfc/libfc.h b/include/scsi/libfc/libfc.h
index d8ec019..bfe91cf 100644
--- a/include/scsi/libfc/libfc.h
+++ b/include/scsi/libfc/libfc.h
@@ -53,19 +53,6 @@
 #define	FC_EX_TIMEOUT	1	/* Exchange timeout */
 #define	FC_EX_CLOSED	2	/* Exchange closed */
 
-/* some helpful macros */
-
-#define ntohll(x) be64_to_cpu(x)
-#define htonll(x) cpu_to_be64(x)
-
-#define ntoh24(p)	(((p)[0] << 16) | ((p)[1] << 8) | ((p)[2]))
-
-#define hton24(p, v)	do { \
-	p[0] = (((v) >> 16) & 0xFF); \
-	p[1] = (((v) >> 8) & 0xFF); \
-	p[2] = ((v) & 0xFF); \
-} while (0)
-
 struct fc_exch_mgr;
 
 /*

Re: [PATCH 2/3] 24-bit types: convert iSCSI to use the __be24 type and macros

[Mike Christie]

Chris Leech wrote:
> The calling convention of hton24 is different now, it takes a single u32
> as an argument and evaluates to a __be24 lvalue.
> 
> Signed-off-by: Chris Leech <christopher.leech-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Looks fine by me. Thanks.

Acked-by: Mike Christie <michaelc-hcNo3dDEHLuVc3sceRu5cw@public.gmane.org>

Re: [PATCH 1/3] 24-bit types: typedef and macros for accessing 3-byte arrays as integers

[Boaz Harrosh]

Chris Leech wrote:
> Both iSCSI and Fibre Channel make use of 24-bit big-endian values in
> frame headers.  This patch defines __be24 and __le24 typedefs for a
> structure wrapped around a 3-byte array, and macros to convert back and
> forth to a 32-bit integer.
> 
> The undefs in iscsi_proto.h are because of the different calling
> convention for the existing hton24 macro in the iSCSI code.  iSCSI will
> be converted in a subsequent patch.
> 
> Signed-off-by: Chris Leech <christopher.leech@intel.com>

I like what this patch wants to accomplish, but I disagree with the
implementation.

First why is the double definition, one in include/linux/byteorder.h
and one in include/linux/byteorder/generic.h ?

Second and most important, in both these files all routines are inline's
not MACROs, and rightly so. There is no place for a macro and the MACRO
works bad for these. One - the definition of a local variable in a {} scope
in the middle of anywhere. Second - type safety.

I CC: Harvey Harrison which did lots of work in these area's.

For me this patch is totally unacceptable.

Thanks for working on this
Boaz
> ---
> 
>  include/linux/byteorder.h         |   56 ++++++++++++++++++++++++++++++++++++
>  include/linux/byteorder/generic.h |   57 +++++++++++++++++++++++++++++++++++++
>  include/linux/types.h             |    2 +
>  include/scsi/iscsi_proto.h        |    2 +
>  4 files changed, 117 insertions(+), 0 deletions(-)
> 
> 
> diff --git a/include/linux/byteorder.h b/include/linux/byteorder.h
> index 29f002d..e41c17b 100644
> --- a/include/linux/byteorder.h
> +++ b/include/linux/byteorder.h
> @@ -62,6 +62,54 @@
>  # define __cpu_to_le64(x) ((__force __le64)__swab64(x))
>  #endif
>  
> +/**
> + * __le24_to_cpu - read a 3-byte array as a 24-bit little-endian integer
> + * @x: __le24, a structure wrapper around a 3-byte array
> + *
> + * Evaluates to a __u32 integer type
> + */
> +#define __le24_to_cpu(x) \
> +({ \
> +	__le24 _x = (x); \
> +	(__u32) ((_x.b[2] << 16) | (_x.b[1] << 8) | (_x.b[0])); \
> +})
> +
> +/**
> + * __cpu_to_le24 - store a value in a 3-byte array in little-endian format
> + * @x: __u32, there is no checking to ensure only the lower 24 bits are set
> + *
> + * Evaluates to an __le24 compound literal
> + */
> +#define __cpu_to_le24(x) \
> +({ \
> +	__u32 _x = (x); \
> +	(__le24) { .b = { _x & 0xff, (_x >> 8) & 0xff, (_x >> 16) & 0xff } }; \
> +})
> +
> +/**
> + * __be24_to_cpu - read a 3-byte array as a 24-bit big-endian integer
> + * @x: __be24, a structure wrapper around a 3-byte array
> + *
> + * Evaluates to a __u32 integer type
> + */
> +#define __be24_to_cpu(x) \
> +({ \
> +	__be24 _x = (x); \
> +	(__u32) ((_x.b[0] << 16) | (_x.b[1] << 8) | (_x.b[2])); \
> +})
> +
> +/**
> + * __cpu_to_be24 - store a value in a 3-byte array in big-endian format
> + * @x: __u32, there is no checking to ensure only the lower 24 bits are set
> + *
> + * Evaluates to an __be24 compound literal
> + */
> +#define __cpu_to_be24(x) \
> +({ \
> +	__u32 _x = (x); \
> +	(__be24) { .b = { (_x >> 16) & 0xff, (_x >> 8) & 0xff, _x & 0xff } }; \
> +})
> +
>  /*
>   * These helpers could be phased out over time as the base version
>   * handles constant folding.
> @@ -280,15 +328,19 @@ static inline __be64 __cpu_to_be64p(const __u64 *p)
>  #ifdef __KERNEL__
>  
>  # define le16_to_cpu __le16_to_cpu
> +# define le24_to_cpu __le24_to_cpu
>  # define le32_to_cpu __le32_to_cpu
>  # define le64_to_cpu __le64_to_cpu
>  # define be16_to_cpu __be16_to_cpu
> +# define be24_to_cpu __be24_to_cpu
>  # define be32_to_cpu __be32_to_cpu
>  # define be64_to_cpu __be64_to_cpu
>  # define cpu_to_le16 __cpu_to_le16
> +# define cpu_to_le24 __cpu_to_le24
>  # define cpu_to_le32 __cpu_to_le32
>  # define cpu_to_le64 __cpu_to_le64
>  # define cpu_to_be16 __cpu_to_be16
> +# define cpu_to_be24 __cpu_to_be24
>  # define cpu_to_be32 __cpu_to_be32
>  # define cpu_to_be64 __cpu_to_be64
>  
> @@ -332,11 +384,15 @@ static inline __be64 __cpu_to_be64p(const __u64 *p)
>  # define ___htons(x) __cpu_to_be16(x)
>  # define ___ntohl(x) __be32_to_cpu(x)
>  # define ___ntohs(x) __be16_to_cpu(x)
> +# define ___hton24(x) __cpu_to_be24(x)
> +# define ___ntoh24(x) __be24_to_cpu(x)
>  
>  # define htonl(x) ___htonl(x)
>  # define ntohl(x) ___ntohl(x)
>  # define htons(x) ___htons(x)
>  # define ntohs(x) ___ntohs(x)
> +# define hton24(x) ___hton24(x)
> +# define ntoh24(x) ___ntoh24(x)
>  
>  static inline void le16_add_cpu(__le16 *var, u16 val)
>  {
> diff --git a/include/linux/byteorder/generic.h b/include/linux/byteorder/generic.h
> index 0846e6b..a57fbc3 100644
> --- a/include/linux/byteorder/generic.h
> +++ b/include/linux/byteorder/generic.h
> @@ -119,6 +119,59 @@
>  #define cpu_to_be16s __cpu_to_be16s
>  #define be16_to_cpus __be16_to_cpus
>  
> +/**
> + * __le24_to_cpu - read a 3-byte array as a 24-bit little-endian integer
> + * @x: __le24, a structure wrapper around a 3-byte array
> + *
> + * Evaluates to a __u32 integer type
> + */
> +#define __le24_to_cpu(x) \
> +({ \
> +	__le24 _x = (x); \
> +	(__u32) ((_x.b[2] << 16) | (_x.b[1] << 8) | (_x.b[0])); \
> +})
> +
> +/**
> + * __cpu_to_le24 - store a value in a 3-byte array in little-endian format
> + * @x: __u32, there is no checking to ensure only the lower 24 bits are set
> + *
> + * Evaluates to an __le24 compound literal
> + */
> +#define __cpu_to_le24(x) \
> +({ \
> +	__u32 _x = (x); \
> +	(__le24) { .b = { _x & 0xff, (_x >> 8) & 0xff, (_x >> 16) & 0xff } }; \
> +})
> +
> +/**
> + * __be24_to_cpu - read a 3-byte array as a 24-bit big-endian integer
> + * @x: __be24, a structure wrapper around a 3-byte array
> + *
> + * Evaluates to a __u32 integer type
> + */
> +#define __be24_to_cpu(x) \
> +({ \
> +	__be24 _x = (x); \
> +	(__u32) ((_x.b[0] << 16) | (_x.b[1] << 8) | (_x.b[2])); \
> +})
> +
> +/**
> + * __cpu_to_be24 - store a value in a 3-byte array in big-endian format
> + * @x: __u32, there is no checking to ensure only the lower 24 bits are set
> + *
> + * Evaluates to an __be24 compound literal
> + */
> +#define __cpu_to_be24(x) \
> +({ \
> +	__u32 _x = (x); \
> +	(__be24) { .b = { (_x >> 16) & 0xff, (_x >> 8) & 0xff, _x & 0xff } }; \
> +})
> +
> +#define le24_to_cpu __le24_to_cpu
> +#define cpu_to_le24 __cpu_to_le24
> +#define be24_to_cpu __be24_to_cpu
> +#define cpu_to_be24 __cpu_to_be24
> +
>  /*
>   * They have to be macros in order to do the constant folding
>   * correctly - if the argument passed into a inline function
> @@ -134,11 +187,15 @@
>  #define ___htons(x) __cpu_to_be16(x)
>  #define ___ntohl(x) __be32_to_cpu(x)
>  #define ___ntohs(x) __be16_to_cpu(x)
> +#define ___hton24(x) __cpu_to_be24(x)
> +#define ___ntoh24(x) __be24_to_cpu(x)
>  
>  #define htonl(x) ___htonl(x)
>  #define ntohl(x) ___ntohl(x)
>  #define htons(x) ___htons(x)
>  #define ntohs(x) ___ntohs(x)
> +#define hton24(x) ___hton24(x)
> +#define ntoh24(x) ___ntoh24(x)
>  
>  static inline void le16_add_cpu(__le16 *var, u16 val)
>  {
> diff --git a/include/linux/types.h b/include/linux/types.h
> index d4a9ce6..85fcff7 100644
> --- a/include/linux/types.h
> +++ b/include/linux/types.h
> @@ -188,6 +188,8 @@ typedef __u64 __bitwise __be64;
>  typedef __u16 __bitwise __sum16;
>  typedef __u32 __bitwise __wsum;
>  
> +typedef struct { __u8 b[3]; } __be24, __le24;
> +
>  #ifdef __KERNEL__
>  typedef unsigned __bitwise__ gfp_t;
>  
> diff --git a/include/scsi/iscsi_proto.h b/include/scsi/iscsi_proto.h
> index f2a2c11..429c5ff 100644
> --- a/include/scsi/iscsi_proto.h
> +++ b/include/scsi/iscsi_proto.h
> @@ -35,6 +35,8 @@
>  /*
>   * useful common(control and data pathes) macro
>   */
> +#undef ntoh24
> +#undef hton24
>  #define ntoh24(p) (((p)[0] << 16) | ((p)[1] << 8) | ((p)[2]))
>  #define hton24(p, v) { \
>          p[0] = (((v) >> 16) & 0xFF); \
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] 24-bit types: typedef and macros for accessing 3-byte arrays as integers

[Boaz Harrosh]

Chris Leech wrote:
> Both iSCSI and Fibre Channel make use of 24-bit big-endian values in
> frame headers.  This patch defines __be24 and __le24 typedefs for a
> structure wrapped around a 3-byte array, and macros to convert back and
> forth to a 32-bit integer.
> 
> The undefs in iscsi_proto.h are because of the different calling
> convention for the existing hton24 macro in the iSCSI code.  iSCSI will
> be converted in a subsequent patch.
> 
> Signed-off-by: Chris Leech <christopher.leech@intel.com>
<snip>

Chris please don't cross-post to a mailing list that is members only.
I press reply-all when on linux-scsi or linux-kernel, and then I get
accused of pocking where I don't belong. So just don't do that.

And my advise to the devel@open-fcoe.org mailing list. Drop the
members only. It is useless with it. I know because I made the same
mistake with my osd-dev@open-osd.org. Believe me the span-filter
at mailman is fine and the span that will get through is not that bad.

Boaz

Re: [PATCH 1/3] 24-bit types: typedef and macros for accessing 3-byte arrays as integers

[Chris Leech]

On Sun, Sep 7, 2008 at 2:36 AM, Boaz Harrosh <bharrosh@panasas.com> wrote:
> Chris Leech wrote:
>> Both iSCSI and Fibre Channel make use of 24-bit big-endian values in
>> frame headers.  This patch defines __be24 and __le24 typedefs for a
>> structure wrapped around a 3-byte array, and macros to convert back and
>> forth to a 32-bit integer.
>>
>> The undefs in iscsi_proto.h are because of the different calling
>> convention for the existing hton24 macro in the iSCSI code.  iSCSI will
>> be converted in a subsequent patch.
>>
>> Signed-off-by: Chris Leech <christopher.leech@intel.com>
>
> I like what this patch wants to accomplish, but I disagree with the
> implementation.
>
> First why is the double definition, one in include/linux/byteorder.h
> and one in include/linux/byteorder/generic.h ?

Because there are currently two byteorder/swab implementations in the
kernel.  As you said Harvey Harrison has done a lot of work in this
area, but right now only the arm and avr32 architectures make use of
the new linux/byteorder.h.  I could put the 24-bit support in it's own
header instead.

> Second and most important, in both these files all routines are inline's
> not MACROs, and rightly so. There is no place for a macro and the MACRO
> works bad for these.

I understand the benefits of inline functions, but I disagree that a
macro is a bad choice in this case.  An inline implementation of
cpu_to_be24/hton24 would require a different interface than the rest
of the byteorder functions, looking more like the existing iSCSI
hton24 macro by taking a pointer to a memory location to store the
result in.

By using a macro that expands to a compound literal, I was able to
maintain the same calling convention of X = cpu_to_be24(Y);
Attempting to do so with an inline results in a function definition
that returns a structure by value, and even when inlined generates
much worse assembly (yes, I tried it).

> One - the definition of a local variable in a {} scope in the middle of anywhere.

That was done in order to only evaluate the macro operand only once,
making it safe to call with an expression that may have side effects.
Macros that create scoped variables are all over the place, like min,
max and container_of.  I consider it necessary in creating a good
macro implementation for this functionality, and my motivation for
using macros was given above.

> Second - type safety.

Actually, the assignment to a locally scoped variable gives just as
much type checking as an inline would :-)

Chris

> I CC: Harvey Harrison which did lots of work in these area's.
>
> For me this patch is totally unacceptable.
>
> Thanks for working on this
> Boaz

Re: [PATCH 1/3] 24-bit types: typedef and macros for accessing 3-byte arrays as integers

[Boaz Harrosh]

Chris Leech wrote:
> On Sun, Sep 7, 2008 at 2:36 AM, Boaz Harrosh <bharrosh@panasas.com> wrote:
>> Chris Leech wrote:
>>> Both iSCSI and Fibre Channel make use of 24-bit big-endian values in
>>> frame headers.  This patch defines __be24 and __le24 typedefs for a
>>> structure wrapped around a 3-byte array, and macros to convert back and
>>> forth to a 32-bit integer.
>>>
>>> The undefs in iscsi_proto.h are because of the different calling
>>> convention for the existing hton24 macro in the iSCSI code.  iSCSI will
>>> be converted in a subsequent patch.
>>>
>>> Signed-off-by: Chris Leech <christopher.leech@intel.com>
>> I like what this patch wants to accomplish, but I disagree with the
>> implementation.
>>
>> First why is the double definition, one in include/linux/byteorder.h
>> and one in include/linux/byteorder/generic.h ?
> 
> Because there are currently two byteorder/swab implementations in the
> kernel.  As you said Harvey Harrison has done a lot of work in this
> area, but right now only the arm and avr32 architectures make use of
> the new linux/byteorder.h.  I could put the 24-bit support in it's own
> header instead.
> 
>> Second and most important, in both these files all routines are inline's
>> not MACROs, and rightly so. There is no place for a macro and the MACRO
>> works bad for these.
> 
> I understand the benefits of inline functions, but I disagree that a
> macro is a bad choice in this case.  An inline implementation of
> cpu_to_be24/hton24 would require a different interface than the rest
> of the byteorder functions, looking more like the existing iSCSI
> hton24 macro by taking a pointer to a memory location to store the
> result in.
> 
> By using a macro that expands to a compound literal, I was able to
> maintain the same calling convention of X = cpu_to_be24(Y);
> Attempting to do so with an inline results in a function definition
> that returns a structure by value, and even when inlined generates
> much worse assembly (yes, I tried it).
> 
>> One - the definition of a local variable in a {} scope in the middle of anywhere.
> 
> That was done in order to only evaluate the macro operand only once,
> making it safe to call with an expression that may have side effects.
> Macros that create scoped variables are all over the place, like min,
> max and container_of.  I consider it necessary in creating a good
> macro implementation for this functionality, and my motivation for
> using macros was given above.
> 
>> Second - type safety.
> 
> Actually, the assignment to a locally scoped variable gives just as
> much type checking as an inline would :-)
> 
> Chris
> 
>> I CC: Harvey Harrison which did lots of work in these area's.
>>
>> For me this patch is totally unacceptable.
>>
>> Thanks for working on this
>> Boaz

I wanted to see what you're saying and tried this test code below:

<test.c>
#include "stdio.h"

typedef unsigned char __u8;
typedef unsigned int __u32;

typedef struct { __u8 b[3]; } __be24, __le24;

#define __be24_to_cpu(x) \
({ \
	__be24 _x = (x); \
	(__u32) ((_x.b[0] << 16) | (_x.b[1] << 8) | (_x.b[2])); \
})

static inline __u32 be24_to_cpu(__be24 _x)
{
	return (__u32) ((_x.b[0] << 16) | (_x.b[1] << 8) | (_x.b[2]));
}

#define __cpu_to_be24(x) \
({ \
	__u32 _x = (x); \
	(__be24) { .b = { (_x >> 16) & 0xff, (_x >> 8) & 0xff, _x & 0xff } }; \
})

static inline __be24 cpu_to_be24(__u32 _x)
{
	__be24 be = {
		.b = { (_x >> 16) & 0xff, (_x >> 8) & 0xff, _x & 0xff } 
	};
	return be;
}

int test1(__u32 r)
{
	union {
		__be24 be17;
		__u32 be_as_u;
	} be = {.be_as_u = 0};

	be.be17 = cpu_to_be24(r);
	__u32 cpu = be24_to_cpu(be.be17) + 1;

	printf("cpu=%x be=%x\n",cpu, be.be_as_u);
	return cpu;
}

int test2(__u32 r)
{
	union {
		__be24 be17;
		__u32 be_as_u;
	} be = {.be_as_u = 0};

	be.be17 = __cpu_to_be24(r);
	__u32 cpu = __be24_to_cpu(be.be17) + 1;

	printf("cpu=%x be=%x\n",cpu, be.be_as_u);
	return cpu;
}

int main()
{
	__u32 r = rand();
	test1(r);
	test2(r);

	return 0;
}
</test.c>

I compile it like this:
$ gcc -O1 test.c -o test
if I
$ gdb test
gdb> disass test1
gdb> disass test2

I get the exact same assembly.

What am I doing wrong ?

$ gcc --version
gcc (GCC) 4.1.2 20070925 (Red Hat 4.1.2-27)

Boaz

Re: [PATCH 1/3] 24-bit types: typedef and macros for accessing 3-byte arrays as integers

[Chris Leech]

On Sun, Sep 7, 2008 at 10:21 AM, Boaz Harrosh <bharrosh@panasas.com> wrote:
> I wanted to see what you're saying and tried this test code below:
>
> <test.c>
>
> I get the exact same assembly.
>
> What am I doing wrong ?

Interesting.  My earlier comparisons were done in the kernel and
comparing the resulting libicssi.o and iscsi_tcp.o.  Let me look into
this more.

Chris

[PATCH] 24-bit types: typedef and functions for accessing 3-byte arrays as integers

[Chris Leech]

Both iSCSI and Fibre Channel make use of 24-bit big-endian values in
frame headers.  This patch defines __be24 and __le24 typedefs for a
structure wrapped around a 3-byte array, and functions to convert back and
forth to a 32-bit integer.

The undefs in iscsi_proto.h are because of the different calling
convention for the existing hton24 macro in the iSCSI code.  iSCSI will
be converted in a subsequent patch.

Changes from last posting:

Switched from preprocessor macros to inline functions.  The generated assembly
is the same with gcc 4.1.2 as long as the function is actually inlined.  I
applied the __always_inline attribute to all of these, after seeing that with
one of my test kernel configurations they were not being inlined without it
and the generated instructions in the iSCSI code could be considered a
regression from the existing macro.

Signed-off-by: Chris Leech <christopher.leech@intel.com>
---

 include/linux/byteorder.h         |   44 ++++++++++++++++++++++++++++++++++++
 include/linux/byteorder/generic.h |   45 +++++++++++++++++++++++++++++++++++++
 include/linux/types.h             |    2 ++
 include/scsi/iscsi_proto.h        |    2 ++
 4 files changed, 93 insertions(+), 0 deletions(-)


diff --git a/include/linux/byteorder.h b/include/linux/byteorder.h
index 29f002d..b48b88f 100644
--- a/include/linux/byteorder.h
+++ b/include/linux/byteorder.h
@@ -62,6 +62,42 @@
 # define __cpu_to_le64(x) ((__force __le64)__swab64(x))
 #endif
 
+/**
+ * __le24_to_cpu - read a 3-byte array as a 24-bit little-endian integer
+ * @x: __le24, a structure wrapper around a 3-byte array
+ */
+static __always_inline __u32 __le24_to_cpu(const __le24 x)
+{
+	return (__u32) ((x.b[2] << 16) | (x.b[1] << 8) | (x.b[0]));
+}
+
+/**
+ * __cpu_to_le24 - store a value in a 3-byte array in little-endian format
+ * @x: __u32, there is no checking to ensure only the lower 24 bits are set
+ */
+static __always_inline __le24 __cpu_to_le24(const __u32 x)
+{
+	return (__le24) { { x & 0xff, (x >> 8) & 0xff, (x >> 16) & 0xff } };
+}
+
+/**
+ * __be24_to_cpu - read a 3-byte array as a 24-bit big-endian integer
+ * @x: __be24, a structure wrapper around a 3-byte array
+ */
+static __always_inline __u32 __be24_to_cpu(const __be24 x)
+{
+	return (__u32) ((x.b[0] << 16) | (x.b[1] << 8) | (x.b[2]));
+}
+
+/**
+ * __cpu_to_be24 - store a value in a 3-byte array in big-endian format
+ * @x: __u32, there is no checking to ensure only the lower 24 bits are set
+ */
+static __always_inline __be24 __cpu_to_be24(const __u32 x)
+{
+	return (__be24) { { (x >> 16) & 0xff, (x >> 8) & 0xff, x & 0xff } };
+}
+
 /*
  * These helpers could be phased out over time as the base version
  * handles constant folding.
@@ -280,15 +316,19 @@ static inline __be64 __cpu_to_be64p(const __u64 *p)
 #ifdef __KERNEL__
 
 # define le16_to_cpu __le16_to_cpu
+# define le24_to_cpu __le24_to_cpu
 # define le32_to_cpu __le32_to_cpu
 # define le64_to_cpu __le64_to_cpu
 # define be16_to_cpu __be16_to_cpu
+# define be24_to_cpu __be24_to_cpu
 # define be32_to_cpu __be32_to_cpu
 # define be64_to_cpu __be64_to_cpu
 # define cpu_to_le16 __cpu_to_le16
+# define cpu_to_le24 __cpu_to_le24
 # define cpu_to_le32 __cpu_to_le32
 # define cpu_to_le64 __cpu_to_le64
 # define cpu_to_be16 __cpu_to_be16
+# define cpu_to_be24 __cpu_to_be24
 # define cpu_to_be32 __cpu_to_be32
 # define cpu_to_be64 __cpu_to_be64
 
@@ -332,11 +372,15 @@ static inline __be64 __cpu_to_be64p(const __u64 *p)
 # define ___htons(x) __cpu_to_be16(x)
 # define ___ntohl(x) __be32_to_cpu(x)
 # define ___ntohs(x) __be16_to_cpu(x)
+# define ___hton24(x) __cpu_to_be24(x)
+# define ___ntoh24(x) __be24_to_cpu(x)
 
 # define htonl(x) ___htonl(x)
 # define ntohl(x) ___ntohl(x)
 # define htons(x) ___htons(x)
 # define ntohs(x) ___ntohs(x)
+# define hton24(x) ___hton24(x)
+# define ntoh24(x) ___ntoh24(x)
 
 static inline void le16_add_cpu(__le16 *var, u16 val)
 {
diff --git a/include/linux/byteorder/generic.h b/include/linux/byteorder/generic.h
index 0846e6b..71b97dd 100644
--- a/include/linux/byteorder/generic.h
+++ b/include/linux/byteorder/generic.h
@@ -119,6 +119,47 @@
 #define cpu_to_be16s __cpu_to_be16s
 #define be16_to_cpus __be16_to_cpus
 
+/**
+ * __le24_to_cpu - read a 3-byte array as a 24-bit little-endian integer
+ * @x: __le24, a structure wrapper around a 3-byte array
+ */
+static __always_inline __u32 __le24_to_cpu(const __le24 x)
+{
+	return (__u32) ((x.b[2] << 16) | (x.b[1] << 8) | (x.b[0]));
+}
+
+/**
+ * __cpu_to_le24 - store a value in a 3-byte array in little-endian format
+ * @x: __u32, there is no checking to ensure only the lower 24 bits are set
+ */
+static __always_inline __le24 __cpu_to_le24(const __u32 x)
+{
+	return (__le24) { { x & 0xff, (x >> 8) & 0xff, (x >> 16) & 0xff } };
+}
+
+/**
+ * __be24_to_cpu - read a 3-byte array as a 24-bit big-endian integer
+ * @x: __be24, a structure wrapper around a 3-byte array
+ */
+static __always_inline __u32 __be24_to_cpu(const __be24 x)
+{
+	return (__u32) ((x.b[0] << 16) | (x.b[1] << 8) | (x.b[2]));
+}
+
+/**
+ * __cpu_to_be24 - store a value in a 3-byte array in big-endian format
+ * @x: __u32, there is no checking to ensure only the lower 24 bits are set
+ */
+static __always_inline __be24 __cpu_to_be24(const __u32 x)
+{
+	return (__be24) { { (x >> 16) & 0xff, (x >> 8) & 0xff, x & 0xff } };
+}
+
+#define le24_to_cpu __le24_to_cpu
+#define cpu_to_le24 __cpu_to_le24
+#define be24_to_cpu __be24_to_cpu
+#define cpu_to_be24 __cpu_to_be24
+
 /*
  * They have to be macros in order to do the constant folding
  * correctly - if the argument passed into a inline function
@@ -134,11 +175,15 @@
 #define ___htons(x) __cpu_to_be16(x)
 #define ___ntohl(x) __be32_to_cpu(x)
 #define ___ntohs(x) __be16_to_cpu(x)
+#define ___hton24(x) __cpu_to_be24(x)
+#define ___ntoh24(x) __be24_to_cpu(x)
 
 #define htonl(x) ___htonl(x)
 #define ntohl(x) ___ntohl(x)
 #define htons(x) ___htons(x)
 #define ntohs(x) ___ntohs(x)
+#define hton24(x) ___hton24(x)
+#define ntoh24(x) ___ntoh24(x)
 
 static inline void le16_add_cpu(__le16 *var, u16 val)
 {
diff --git a/include/linux/types.h b/include/linux/types.h
index d4a9ce6..85fcff7 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -188,6 +188,8 @@ typedef __u64 __bitwise __be64;
 typedef __u16 __bitwise __sum16;
 typedef __u32 __bitwise __wsum;
 
+typedef struct { __u8 b[3]; } __be24, __le24;
+
 #ifdef __KERNEL__
 typedef unsigned __bitwise__ gfp_t;
 
diff --git a/include/scsi/iscsi_proto.h b/include/scsi/iscsi_proto.h
index f2a2c11..429c5ff 100644
--- a/include/scsi/iscsi_proto.h
+++ b/include/scsi/iscsi_proto.h
@@ -35,6 +35,8 @@
 /*
  * useful common(control and data pathes) macro
  */
+#undef ntoh24
+#undef hton24
 #define ntoh24(p) (((p)[0] << 16) | ((p)[1] << 8) | ((p)[2]))
 #define hton24(p, v) { \
         p[0] = (((v) >> 16) & 0xFF); \

Re: [PATCH] 24-bit types: typedef and functions for accessing 3-byte arrays as integers

[Boaz Harrosh]

Chris Leech wrote:
> Both iSCSI and Fibre Channel make use of 24-bit big-endian values in
> frame headers.  This patch defines __be24 and __le24 typedefs for a
> structure wrapped around a 3-byte array, and functions to convert back and
> forth to a 32-bit integer.
> 
> The undefs in iscsi_proto.h are because of the different calling
> convention for the existing hton24 macro in the iSCSI code.  iSCSI will
> be converted in a subsequent patch.
> 
> Changes from last posting:
> 
> Switched from preprocessor macros to inline functions.  The generated assembly
> is the same with gcc 4.1.2 as long as the function is actually inlined.  I
> applied the __always_inline attribute to all of these, after seeing that with
> one of my test kernel configurations they were not being inlined without it
> and the generated instructions in the iSCSI code could be considered a
> regression from the existing macro.
> 
> Signed-off-by: Chris Leech <christopher.leech@intel.com>
> ---
> 
>  include/linux/byteorder.h         |   44 ++++++++++++++++++++++++++++++++++++
>  include/linux/byteorder/generic.h |   45 +++++++++++++++++++++++++++++++++++++
>  include/linux/types.h             |    2 ++
>  include/scsi/iscsi_proto.h        |    2 ++
>  4 files changed, 93 insertions(+), 0 deletions(-)
> 
<snip>

Thanks! Personally I like this much better.

Sorry to be nit picking but again I hate that double implementation thing.
>From what I could see, and considering a few of the ARCHs you mentioned,
it looks to me like <linux/swab.h> is common to the two implementations,
(old and new). Perhaps put the "__xxx" implementations in swab.h and
only the xxx => __xxx switching in the double byteorder headers, which
fits better with the surrounding style of these headers. 

Thanks
Boaz

Re: [PATCH 1/3] 24-bit types: typedef and macros for accessing 3-byte arrays as integers

[Christoph Hellwig]

On Fri, Sep 05, 2008 at 09:57:32AM -0700, Chris Leech wrote:
> Both iSCSI and Fibre Channel make use of 24-bit big-endian values in
> frame headers.  This patch defines __be24 and __le24 typedefs for a
> structure wrapped around a 3-byte array, and macros to convert back and
> forth to a 32-bit integer.
> 
> The undefs in iscsi_proto.h are because of the different calling
> convention for the existing hton24 macro in the iSCSI code.  iSCSI will
> be converted in a subsequent patch.

JFS already defines an __le24, see fs/jfs/endian24.h.  Please try to
cover it, too or at least make sure you don't break it.

Re:[PATCH 1/3] 24-bit types: typedef and macros for accessing 3-byte arrays as integers

[Dave Kleikamp]

On Wed, 2008-09-10 at 10:07 -0400, Christoph Hellwig wrote:

> JFS already defines an __le24, see fs/jfs/endian24.h.  Please try to
> cover it, too or at least make sure you don't break it.

Chris,
This patch takes care of jfs.  Please add it to your patchset.

Thanks,
Shaggy

24-bit types: Convert jfs to use the common 24-bit types

This patch cleans up some of the ugliness in the jfs headers and
uses the common 24-bit types instead of its private definitions.

Signed-off-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com>

diff --git a/fs/jfs/endian24.h b/fs/jfs/endian24.h
deleted file mode 100644
index fa92f7f..0000000
--- a/fs/jfs/endian24.h
+++ /dev/null
@@ -1,49 +0,0 @@
-/*
- *   Copyright (C) International Business Machines Corp., 2001
- *
- *   This program is free software;  you can redistribute it and/or modify
- *   it under the terms of the GNU General Public License as published by
- *   the Free Software Foundation; either version 2 of the License, or
- *   (at your option) any later version.
- *
- *   This program is distributed in the hope that it will be useful,
- *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
- *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
- *   the GNU General Public License for more details.
- *
- *   You should have received a copy of the GNU General Public License
- *   along with this program;  if not, write to the Free Software
- *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
- */
-#ifndef _H_ENDIAN24
-#define	_H_ENDIAN24
-
-/*
- *	endian24.h:
- *
- * Endian conversion for 24-byte data
- *
- */
-#define __swab24(x) \
-({ \
-	__u32 __x = (x); \
-	((__u32)( \
-		((__x & (__u32)0x000000ffUL) << 16) | \
-		 (__x & (__u32)0x0000ff00UL)	    | \
-		((__x & (__u32)0x00ff0000UL) >> 16) )); \
-})
-
-#if (defined(__KERNEL__) && defined(__LITTLE_ENDIAN)) || (defined(__BYTE_ORDER) && (__BYTE_ORDER == __LITTLE_ENDIAN))
-	#define __cpu_to_le24(x) ((__u32)(x))
-	#define __le24_to_cpu(x) ((__u32)(x))
-#else
-	#define __cpu_to_le24(x) __swab24(x)
-	#define __le24_to_cpu(x) __swab24(x)
-#endif
-
-#ifdef __KERNEL__
-	#define cpu_to_le24 __cpu_to_le24
-	#define le24_to_cpu __le24_to_cpu
-#endif
-
-#endif				/* !_H_ENDIAN24 */
diff --git a/fs/jfs/jfs_types.h b/fs/jfs/jfs_types.h
index 649f981..6c49b93 100644
--- a/fs/jfs/jfs_types.h
+++ b/fs/jfs/jfs_types.h
@@ -30,8 +30,6 @@
 #include <linux/types.h>
 #include <linux/nls.h>
 
-#include "endian24.h"
-
 /*
  * transaction and lock id's
  *
@@ -62,7 +60,7 @@ struct timestruc_t {
  */
 typedef struct {
 	unsigned len:24;
-	unsigned off1:8;
+	u8 off1;
 	u32 off2;
 } lxd_t;
 
@@ -90,8 +88,8 @@ struct lxdlist {
  *	physical xd (pxd)
  */
 typedef struct {
-	unsigned len:24;
-	unsigned addr1:8;
+	__le24 len;
+	u8 addr1;
 	__le32 addr2;
 } pxd_t;
 
@@ -122,13 +120,13 @@ struct pxdlist {
  *	data extent descriptor (dxd)
  */
 typedef struct {
-	unsigned flag:8;	/* 1: flags */
-	unsigned rsrvd:24;
-	__le32 size;		/* 4: size in byte */
-	unsigned len:24;	/* 3: length in unit of fsblksize */
-	unsigned addr1:8;	/* 1: address in unit of fsblksize */
-	__le32 addr2;		/* 4: address in unit of fsblksize */
-} dxd_t;			/* - 16 - */
+	u8 flag;	/* 1: flags */
+	u8 rsrvd[3];
+	__le32 size;	/* 4: size in byte */
+	__le24 len;	/* 3: length in unit of fsblksize */
+	u8 addr1;	/* 1: address in unit of fsblksize */
+	__le32 addr2;	/* 4: address in unit of fsblksize */
+} dxd_t;		/* - 16 - */
 
 /* dxd_t flags */
 #define	DXD_INDEX	0x80	/* B+-tree index */
diff --git a/fs/jfs/jfs_xtree.h b/fs/jfs/jfs_xtree.h
index 70815c8..92beff3 100644
--- a/fs/jfs/jfs_xtree.h
+++ b/fs/jfs/jfs_xtree.h
@@ -29,14 +29,14 @@
  *	extent allocation descriptor (xad)
  */
 typedef struct xad {
-	unsigned flag:8;	/* 1: flag */
-	unsigned rsvrd:16;	/* 2: reserved */
-	unsigned off1:8;	/* 1: offset in unit of fsblksize */
-	__le32 off2;		/* 4: offset in unit of fsblksize */
-	unsigned len:24;	/* 3: length in unit of fsblksize */
-	unsigned addr1:8;	/* 1: address in unit of fsblksize */
-	__le32 addr2;		/* 4: address in unit of fsblksize */
-} xad_t;			/* (16) */
+	u8 flag;	/* 1: flag */
+	u8 rsvrd[2];	/* 2: reserved */
+	u8 off1;	/* 1: offset in unit of fsblksize */
+	__le32 off2;	/* 4: offset in unit of fsblksize */
+	__le24 len;	/* 3: length in unit of fsblksize */
+	u8 addr1;	/* 1: address in unit of fsblksize */
+	__le32 addr2;	/* 4: address in unit of fsblksize */
+} xad_t;		/* (16) */
 
 #define MAXXLEN		((1 << 24) - 1)
 

-- 
David Kleikamp
IBM Linux Technology Center

Re: [PATCH 1/3] 24-bit types: typedef and macros for accessing 3-byte arrays as integers

[Boaz Harrosh]

Dave Kleikamp wrote:
> On Wed, 2008-09-10 at 10:07 -0400, Christoph Hellwig wrote:
> 
>> JFS already defines an __le24, see fs/jfs/endian24.h.  Please try to
>> cover it, too or at least make sure you don't break it.
> 
> Chris,
> This patch takes care of jfs.  Please add it to your patchset.
> 
> Thanks,
> Shaggy
> 
> 24-bit types: Convert jfs to use the common 24-bit types
> 
> This patch cleans up some of the ugliness in the jfs headers and
> uses the common 24-bit types instead of its private definitions.
> 
> Signed-off-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
> 
> diff --git a/fs/jfs/endian24.h b/fs/jfs/endian24.h
> deleted file mode 100644
> index fa92f7f..0000000
> --- a/fs/jfs/endian24.h
> +++ /dev/null
> @@ -1,49 +0,0 @@
> -/*
> - *   Copyright (C) International Business Machines Corp., 2001
> - *
> - *   This program is free software;  you can redistribute it and/or modify
> - *   it under the terms of the GNU General Public License as published by
> - *   the Free Software Foundation; either version 2 of the License, or
> - *   (at your option) any later version.
> - *
> - *   This program is distributed in the hope that it will be useful,
> - *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
> - *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
> - *   the GNU General Public License for more details.
> - *
> - *   You should have received a copy of the GNU General Public License
> - *   along with this program;  if not, write to the Free Software
> - *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> - */
> -#ifndef _H_ENDIAN24
> -#define	_H_ENDIAN24
> -
> -/*
> - *	endian24.h:
> - *
> - * Endian conversion for 24-byte data
> - *
> - */
> -#define __swab24(x) \
> -({ \
> -	__u32 __x = (x); \
> -	((__u32)( \
> -		((__x & (__u32)0x000000ffUL) << 16) | \
> -		 (__x & (__u32)0x0000ff00UL)	    | \
> -		((__x & (__u32)0x00ff0000UL) >> 16) )); \
> -})
> -
> -#if (defined(__KERNEL__) && defined(__LITTLE_ENDIAN)) || (defined(__BYTE_ORDER) && (__BYTE_ORDER == __LITTLE_ENDIAN))
> -	#define __cpu_to_le24(x) ((__u32)(x))
> -	#define __le24_to_cpu(x) ((__u32)(x))
> -#else
> -	#define __cpu_to_le24(x) __swab24(x)
> -	#define __le24_to_cpu(x) __swab24(x)
> -#endif
> -
> -#ifdef __KERNEL__
> -	#define cpu_to_le24 __cpu_to_le24
> -	#define le24_to_cpu __le24_to_cpu
> -#endif
> -
> -#endif				/* !_H_ENDIAN24 */
> diff --git a/fs/jfs/jfs_types.h b/fs/jfs/jfs_types.h
> index 649f981..6c49b93 100644
> --- a/fs/jfs/jfs_types.h
> +++ b/fs/jfs/jfs_types.h
> @@ -30,8 +30,6 @@
>  #include <linux/types.h>
>  #include <linux/nls.h>
>  
> -#include "endian24.h"
> -
>  /*
>   * transaction and lock id's
>   *
> @@ -62,7 +60,7 @@ struct timestruc_t {
>   */
>  typedef struct {
>  	unsigned len:24;
> -	unsigned off1:8;
> +	u8 off1;
>  	u32 off2;
>  } lxd_t;
>  

Why is the difference from below definition. That is the
use/not of __le24? 

> @@ -90,8 +88,8 @@ struct lxdlist {
>   *	physical xd (pxd)
>   */
>  typedef struct {
> -	unsigned len:24;
> -	unsigned addr1:8;
> +	__le24 len;

Is this stuff on-the-wire?
Do you need a:
+	__le24 len __packed;

> +	u8 addr1;
>  	__le32 addr2;
>  } pxd_t;
and:
  } pxd_t __packed ;

>  
> @@ -122,13 +120,13 @@ struct pxdlist {
>   *	data extent descriptor (dxd)
>   */
>  typedef struct {
> -	unsigned flag:8;	/* 1: flags */
> -	unsigned rsrvd:24;
> -	__le32 size;		/* 4: size in byte */
> -	unsigned len:24;	/* 3: length in unit of fsblksize */
> -	unsigned addr1:8;	/* 1: address in unit of fsblksize */
> -	__le32 addr2;		/* 4: address in unit of fsblksize */
> -} dxd_t;			/* - 16 - */
> +	u8 flag;	/* 1: flags */
> +	u8 rsrvd[3];
> +	__le32 size;	/* 4: size in byte */
> +	__le24 len;	/* 3: length in unit of fsblksize */
> +	u8 addr1;	/* 1: address in unit of fsblksize */
> +	__le32 addr2;	/* 4: address in unit of fsblksize */
> +} dxd_t;		/* - 16 - */
>  
>  /* dxd_t flags */
>  #define	DXD_INDEX	0x80	/* B+-tree index */
> diff --git a/fs/jfs/jfs_xtree.h b/fs/jfs/jfs_xtree.h
> index 70815c8..92beff3 100644
> --- a/fs/jfs/jfs_xtree.h
> +++ b/fs/jfs/jfs_xtree.h
> @@ -29,14 +29,14 @@
>   *	extent allocation descriptor (xad)
>   */
>  typedef struct xad {
> -	unsigned flag:8;	/* 1: flag */
> -	unsigned rsvrd:16;	/* 2: reserved */
> -	unsigned off1:8;	/* 1: offset in unit of fsblksize */
> -	__le32 off2;		/* 4: offset in unit of fsblksize */
> -	unsigned len:24;	/* 3: length in unit of fsblksize */
> -	unsigned addr1:8;	/* 1: address in unit of fsblksize */
> -	__le32 addr2;		/* 4: address in unit of fsblksize */
> -} xad_t;			/* (16) */
> +	u8 flag;	/* 1: flag */
> +	u8 rsvrd[2];	/* 2: reserved */
> +	u8 off1;	/* 1: offset in unit of fsblksize */
> +	__le32 off2;	/* 4: offset in unit of fsblksize */
> +	__le24 len;	/* 3: length in unit of fsblksize */
> +	u8 addr1;	/* 1: address in unit of fsblksize */
> +	__le32 addr2;	/* 4: address in unit of fsblksize */
> +} xad_t;		/* (16) */
>  
>  #define MAXXLEN		((1 << 24) - 1)
>  
> 

Note that before the :24 bit-field was kept packed but now
with the use of struct at the __le24 definition it might
choose to pad them.

Chris you might want to change the definitions at linux/types.h
to:

typedef struct { __u8 b[3]; } __be24, __le24 __packed;

With gcc it will not help with the proceeding fields, and the
containing struct will need it's own "__packed" declaration
but it will keep it packed with previous fields.

Just my $0.017
Boaz

Re: [PATCH 1/3] 24-bit types: typedef and macros for accessing 3-byte arrays as integers

[Boaz Harrosh]

devel-bounces@open-fcoe.org wrote:
> Your mail to 'devel' with the subject
> 
>     Re: [PATCH 1/3] 24-bit types: typedef and macros for	accessing
> 3-byte arrays as integers
> 
> Is being held until the list moderator can review it for approval.
> 
> The reason it is being held:
> 
>     Post by non-member to a members-only list
> 
> Either the message will get posted to the list, or you will receive
> notification of the moderator's decision.  If you would like to cancel
> this posting, please visit the following URL:
> 
>     http://www.open-fcoe.org/mailman/confirm/devel/2402f0a8c12e310105827ba074ab7376a0dee4a0
> 


Can this be fixed please ?

Re: [PATCH 1/3] 24-bit types: typedef and macros for accessing 3-byte arrays as integers

[Chris Leech]

Dave Kleikamp wrote:
> On Wed, 2008-09-10 at 10:07 -0400, Christoph Hellwig wrote:
> 
>> JFS already defines an __le24, see fs/jfs/endian24.h.  Please try to
>> cover it, too or at least make sure you don't break it.
> 
> Chris,
> This patch takes care of jfs.  Please add it to your patchset.
> 
> Thanks,
> Shaggy

Thanks, I was just about to look into JFS.

> @@ -62,7 +60,7 @@ struct timestruc_t {
>   */
>  typedef struct {
>  	unsigned len:24;
> -	unsigned off1:8;
> +	u8 off1;
>  	u32 off2;
>  } lxd_t;

Shouldn't len here be changed to a __le24?  I think this just changed
the size of lxd_t by a byte.

- Chris

Re: [Open-FCoE] [PATCH 1/3] 24-bit types: typedef and macros for accessing 3-byte arrays as integers

[Chris Leech]

Chris Leech wrote:
> Dave Kleikamp wrote:
>> @@ -62,7 +60,7 @@ struct timestruc_t {
>>   */
>>  typedef struct {
>>  	unsigned len:24;
>> -	unsigned off1:8;
>> +	u8 off1;
>>  	u32 off2;
>>  } lxd_t;
> 
> Shouldn't len here be changed to a __le24?  I think this just changed
> the size of lxd_t by a byte.

Never mind, I see that it's a host order field.  And presently surprised
to see that gcc combines the 24-bit bitfield with the following u8.

Chris

Re: [Open-FCoE] [PATCH 1/3] 24-bit types: typedef and macros for accessing 3-byte arrays as integers

[Boaz Harrosh]

Chris Leech wrote:
> Chris Leech wrote:
>> Dave Kleikamp wrote:
>>> @@ -62,7 +60,7 @@ struct timestruc_t {
>>>   */
>>>  typedef struct {
>>>  	unsigned len:24;
>>> -	unsigned off1:8;
>>> +	u8 off1;
>>>  	u32 off2;
>>>  } lxd_t;
>> Shouldn't len here be changed to a __le24?  I think this just changed
>> the size of lxd_t by a byte.
> 
> Never mind, I see that it's a host order field.  And presently surprised
> to see that gcc combines the 24-bit bitfield with the following u8.
> 
> Chris

It does because these are all bytes. on x86. But this is not guarantied
for all ARCHs and machine-word-sizes. In any way this should be consistent
with the rest of the file.

Please see my other reply about packing of structures. If this is
on-the-wire then there are problems.

Boaz

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [Open-FCoE] [PATCH 1/3] 24-bit types: typedef and macros for accessing 3-byte arrays as integers

[Dave Kleikamp]

On Wed, 2008-09-10 at 10:45 -0700, Chris Leech wrote:
> Chris Leech wrote:
> > Dave Kleikamp wrote:
> >> @@ -62,7 +60,7 @@ struct timestruc_t {
> >>   */
> >>  typedef struct {
> >>  	unsigned len:24;
> >> -	unsigned off1:8;
> >> +	u8 off1;
> >>  	u32 off2;
> >>  } lxd_t;
> > 
> > Shouldn't len here be changed to a __le24?  I think this just changed
> > the size of lxd_t by a byte.
> 
> Never mind, I see that it's a host order field.  And presently surprised
> to see that gcc combines the 24-bit bitfield with the following u8.

Right.  I just made a note to throw away this ridiculous structure and
replace it with something sane.  It's only used in-memory and there's no
reason not to have an unsigned long long for offset, and an int for len.
This patch didn't seem to be the right place to fix that though.

-- 
David Kleikamp
IBM Linux Technology Center

Re: [PATCH 1/3] 24-bit types: typedef and macros for accessing 3-byte arrays as integers

[Dave Kleikamp]

On Wed, 2008-09-10 at 19:11 +0300, Boaz Harrosh wrote:
> Dave Kleikamp wrote:

> > @@ -62,7 +60,7 @@ struct timestruc_t {
> >   */
> >  typedef struct {
> >  	unsigned len:24;
> > -	unsigned off1:8;
> > +	u8 off1;
> >  	u32 off2;
> >  } lxd_t;
> >  
> 
> Why is the difference from below definition. That is the
> use/not of __le24? 

Answered elsewhere, but this is host-endian.  I plan to kill this
structure soon.

> > @@ -90,8 +88,8 @@ struct lxdlist {
> >   *	physical xd (pxd)
> >   */
> >  typedef struct {
> > -	unsigned len:24;
> > -	unsigned addr1:8;
> > +	__le24 len;
> 
> Is this stuff on-the-wire?

Written to disk, so basically, yeah.

> Do you need a:
> +	__le24 len __packed;
> 
> > +	u8 addr1;
> >  	__le32 addr2;
> >  } pxd_t;
> and:
>   } pxd_t __packed ;

I'm not convinced that this is needed.  Does the compiler do any padding
for alignment when it only contains char types (or structs of chars)?

> 
> Note that before the :24 bit-field was kept packed but now
> with the use of struct at the __le24 definition it might
> choose to pad them.

Maybe, but I can't get the compiler to add any padding playing around
with variants of these structures.  I've tested a simple program on both
x86 and ppc64, but I'm not sure what would happen on, say, arm.

> Chris you might want to change the definitions at linux/types.h
> to:
> 
> typedef struct { __u8 b[3]; } __be24, __le24 __packed;
> 
> With gcc it will not help with the proceeding fields, and the
> containing struct will need it's own "__packed" declaration
> but it will keep it packed with previous fields.
>
> Just my $0.017
> Boaz

Shaggy
-- 
David Kleikamp
IBM Linux Technology Center

Re: [PATCH 1/3] 24-bit types: typedef and macros for accessing 3-byte arrays as integers

[Chris Leech]

On Wed, Sep 10, 2008 at 9:11 AM, Boaz Harrosh <bharrosh@panasas.com> wrote:
> Chris you might want to change the definitions at linux/types.h
> to:
>
> typedef struct { __u8 b[3]; } __be24, __le24 __packed;
>
> With gcc it will not help with the proceeding fields, and the
> containing struct will need it's own "__packed" declaration
> but it will keep it packed with previous fields.

I haven't seen padding added simply because of a nested structure
boundary, but I'm not up on all the ABIs for the different
architectures.  Obviously a containing structure would want to have
the 24-bit type adjacent to an 8-bit type, or have it's own packed
attribute if needed.

It shouldn't hurt, in this case the members shouldn't be expected to
have more than byte alignment anyway, but I can't see how it would
help.

If there's a particular arch that might be a problem I'm happy to look
into it, but I don't want to start throwing packed attributes around
just in case.

Chris

Re: [PATCH 1/3] 24-bit types: typedef and macros for accessing 3-byte arrays as integers

[Boaz Harrosh]

Dave Kleikamp wrote:
> On Wed, 2008-09-10 at 19:11 +0300, Boaz Harrosh wrote:
>> Dave Kleikamp wrote:
> 
>>> @@ -62,7 +60,7 @@ struct timestruc_t {
>>>   */
>>>  typedef struct {
>>>  	unsigned len:24;
>>> -	unsigned off1:8;
>>> +	u8 off1;
>>>  	u32 off2;
>>>  } lxd_t;
>>>  
>> Why is the difference from below definition. That is the
>> use/not of __le24? 
> 
> Answered elsewhere, but this is host-endian.  I plan to kill this
> structure soon.
> 
>>> @@ -90,8 +88,8 @@ struct lxdlist {
>>>   *	physical xd (pxd)
>>>   */
>>>  typedef struct {
>>> -	unsigned len:24;
>>> -	unsigned addr1:8;
>>> +	__le24 len;
>> Is this stuff on-the-wire?
> 
> Written to disk, so basically, yeah.
> 
>> Do you need a:
>> +	__le24 len __packed;
>>
>>> +	u8 addr1;
>>>  	__le32 addr2;
>>>  } pxd_t;
>> and:
>>   } pxd_t __packed ;
> 
> I'm not convinced that this is needed.  Does the compiler do any padding
> for alignment when it only contains char types (or structs of chars)?
> 
>> Note that before the :24 bit-field was kept packed but now
>> with the use of struct at the __le24 definition it might
>> choose to pad them.
> 
> Maybe, but I can't get the compiler to add any padding playing around
> with variants of these structures.  I've tested a simple program on both
> x86 and ppc64, but I'm not sure what would happen on, say, arm.
> 

You have an "__le32 addr2" followed, it might want too in rare cases.
But for me this is also a a declaration issue and a readability issue.

I'm telling the compiler, don't mess with my structure, this needs
to be constant whatever the compiler is. In C the compiler is even
allowed to change fields order if it wants to. Why the guess work,
__packed and be done with it.

And it's also a readability issue. Look above lxd_t vs pxd_t. I can't
know that one is in memory and one is on disk. I have to ask questions
and make wrong remarks. But if the later had a __packed on it, then
there are no more questions.

__packed for me is a statement for both the programmer and compiler
that says: "This stuff will be seen externally of the machine. It must
be universally constant"

>> Chris you might want to change the definitions at linux/types.h
>> to:
>>
>> typedef struct { __u8 b[3]; } __be24, __le24 __packed;
>>
>> With gcc it will not help with the proceeding fields, and the
>> containing struct will need it's own "__packed" declaration
>> but it will keep it packed with previous fields.
>>
>> Just my $0.017
>> Boaz
> 
> Shaggy

Boaz