24-bit types: typedef and macros for accessing 3-byte arrays as integers

PATCH In review Chris Leech 6 participants 22 messages Sep 5, 2008 – Sep 11, 2008

Subsystems (4)

SCSI JFS filesystem iSCSI FCoE subsystem (libfc, libfcoe, fcoe)

lore

Share & tools

lore

Chris Leech

author

2008-09-05 16:57 UTC #0 lore raw

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

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 <[email protected]>

---

 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(-)

include/linux/byteorder.h +56 -0

index 29f002d..e41c17b 100644
@@ -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)
 {

include/linux/byteorder/generic.h +57 -0

74 lines not on this page · raw message

include/linux/types.h +2 -0

index d4a9ce6..85fcff7 100644
@@ -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;

include/scsi/iscsi_proto.h +2 -0

index f2a2c11..429c5ff 100644
@@ -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); \

Chris Leech

author

2008-09-05 16:57 UTC #1 lore raw

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

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 <[email protected]>

---

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

drivers/scsi/iscsi_tcp.c +4 -4

index 2a2f009..c9d6e96 100644
@@ -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;
 	}

drivers/scsi/libiscsi.c +3 -3

index 299e075..8432318 100644
@@ -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);

include/scsi/iscsi_proto.h +19 ~~-27~~

160 lines not on this page · raw message

Mike Christie

Acked-by on v1 2/3

2008-09-05 17:03 UTC #2 lore raw

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

Quoted 4 lines · 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 <[email protected]>

Looks fine by me. Thanks.

Acked-by Mike Christie <[email protected]>

Chris Leech

author

2008-09-05 16:57 UTC #3 lore raw

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

In reply to Chris Leech

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

Signed-off-by Chris Leech <[email protected]>

---

 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(-)

drivers/scsi/fcoe/fcoe_dev.c +3 -3

index d5a354f..d5fc479 100644
@@ -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();

drivers/scsi/libfc/fc_exch.c +12 ~~-12~~

index 11a03bd..9fd011b 100644
@@ -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)))

drivers/scsi/libfc/fc_fcp.c +1 -1

index 9402eba..6c24724 100644
@@ -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);

drivers/scsi/libfc/fc_lport.c +4 -4

index b06f519..ad5c5b1 100644
@@ -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);

drivers/scsi/libfc/fc_ns.c +7 -7

index 6204bcc..c280913 100644
@@ -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:

drivers/scsi/libfc/fc_rport.c +2 -2

index cd559dd..0ca4c12 100644
@@ -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,

include/scsi/fc/fc_els.h +9 -9

index af4bf0c..c1cfe21 100644
@@ -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 */

include/scsi/fc/fc_fcoe.h +2 -4

index b2e07ec..4c0f692 100644
@@ -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;
 }
 
 /*

include/scsi/fc/fc_fs.h +4 -4

index ba6df64..125ba15 100644
@@ -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 */

include/scsi/fc/fc_gs.h +1 -1

index ffab027..fb082bf 100644
@@ -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;

include/scsi/fc/fc_ns.h +3 -3

index 790d7b9..44c3019 100644
@@ -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 */
 };
 
 /*

include/scsi/libfc/libfc.h +0 ~~-13~~

index d8ec019..bfe91cf 100644
@@ -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;
 
 /*

Boaz Harrosh

2008-09-07 09:37 UTC #4 lore raw

In reply to Chris Leech

Quoted 10 lines · 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 <[email protected]>

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

Quoted 215 lines

---

 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(-)

include/linux/byteorder.h +56 -0

index 29f002d..e41c17b 100644
@@ -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)
 {

include/linux/byteorder/generic.h +57 -0

index 0846e6b..a57fbc3 100644
@@ -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)
 {

include/linux/types.h +2 -0

index d4a9ce6..85fcff7 100644
@@ -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;

include/scsi/iscsi_proto.h +2 -1

index f2a2c11..429c5ff 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Chris Leech

2008-09-07 15:56 UTC #5 lore raw

Quoted 10 lines · Boaz Harrosh

Quoted 9 lines · 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.

Quoted 7 lines

Quoted 1 line
Signed-off-by Chris Leech <[email protected]>
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.

Quoted 3 lines

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).

Quoted 1 line

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.

Quoted 1 line

Second - type safety.

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

Chris

Quoted 6 lines

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

Boaz Harrosh

2008-09-07 17:22 UTC #6 lore raw

Quoted 50 lines · Chris Leech

Quoted 10 lines · Boaz Harrosh
Quoted 9 lines · 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.
Quoted 6 lines
Quoted 1 line
Signed-off-by Chris Leech <[email protected]>
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.
Quoted 3 lines
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).
Quoted 1 line
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.

Quoted 13 lines

Quoted 1 line
Second - type safety.
Actually, the assignment to a locally scoped variable gives just as much type checking as an inline would :-)
Chris
Quoted 6 lines
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

Chris Leech

2008-09-07 17:52 UTC #7 lore raw

Quoted 7 lines · Boaz Harrosh

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

Chris Leech

author

2008-09-09 22:59 UTC #8 lore raw

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

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 <[email protected]>

---

 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(-)

include/linux/byteorder.h +44 -0

index 29f002d..b48b88f 100644
@@ -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)
 {

include/linux/byteorder/generic.h +45 -0

62 lines not on this page · raw message

include/linux/types.h +2 -0

index d4a9ce6..85fcff7 100644
@@ -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;

include/scsi/iscsi_proto.h +2 -0

index f2a2c11..429c5ff 100644
@@ -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); \

Boaz Harrosh

2008-09-10 12:57 UTC #9 lore raw

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

Quoted 18 lines · 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.

Quoted 9 lines

Signed-off-by Chris Leech <[email protected]>

---

 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.

Quoted 1 line

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

Boaz Harrosh

2008-09-07 09:57 UTC #10 lore raw

In reply to Chris Leech

Quoted 10 lines · 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 <[email protected]>

<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 [email protected] mailing list. Drop the members only. It is useless with it. I know because I made the same mistake with my [email protected]. Believe me the span-filter at mailman is fine and the span that will get through is not that bad.

Boaz

Christoph Hellwig

2008-09-10 14:07 UTC #11 lore raw

In reply to Chris Leech

Quoted 8 lines · 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.

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.

Dave Kleikamp

2008-09-10 15:40 UTC #12 lore raw

Quoted 2 lines · Christoph Hellwig

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 <[email protected]>

fs/jfs/endian24.h deleted +0 ~~-49~~

deleted file mode 100644
index fa92f7f..0000000
@@ -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 */

fs/jfs/jfs_types.h +10 ~~-12~~

index 649f981..6c49b93 100644
@@ -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 */

fs/jfs/jfs_xtree.h +8 -8

index 70815c8..92beff3 100644
@@ -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)

Signature · 2 lines

David Kleikamp
IBM Linux Technology Center

Boaz Harrosh

2008-09-10 16:16 UTC #13 lore raw

Quoted 87 lines · Dave Kleikamp

Quoted 2 lines · Christoph Hellwig

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 <[email protected]>

fs/jfs/endian24.h deleted +0 ~~-49~~

deleted file mode 100644
index fa92f7f..0000000
@@ -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 */

fs/jfs/jfs_types.h +0 -2

index 649f981..6c49b93 100644
@@ -30,8 +30,6 @@
 #include <linux/types.h>
 #include <linux/nls.h>
 
-#include "endian24.h"
-
 /*
  * transaction and lock id's
  *

fs/jfs/jfs_types.h struct timestruc_t { · line 66

Quoted 8 lines

fs/jfs/jfs_types.h +1 -1

@@ -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?

fs/jfs/jfs_types.h struct lxdlist { · line 91

Quoted 7 lines

fs/jfs/jfs_types.h +1 -2

@@ -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;

fs/jfs/jfs_types.h struct lxdlist { · line 94

Quoted 3 lines

fs/jfs/jfs_types.h +1 -0

+	u8 addr1;
 	__le32 addr2;
 } pxd_t;

and:
} pxd_t __packed ;

Quoted 40 lines

fs/jfs/jfs_types.h +7 -7

@@ -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 */

fs/jfs/jfs_xtree.h +1 -8

index 70815c8..92beff3 100644
@@ -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 */

fs/jfs/jfs_xtree.h line 42

Quoted 10 lines

fs/jfs/jfs_xtree.h +7 -0

+	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

Boaz Harrosh

2008-09-10 16:25 UTC #14 lore raw

Quoted 9 lines · [email protected] 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:

Quoted 8 lines

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 ?

Dave Kleikamp

2008-09-10 19:20 UTC #15 lore raw

In reply to Boaz Harrosh

Quoted 14 lines · Boaz Harrosh

fs/jfs/jfs_types.h struct timestruc_t { · line 66
Quoted 9 lines · Dave Kleikamp
fs/jfs/jfs_types.h +1 -1
@@ -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.

Quoted 9 lines

fs/jfs/jfs_types.h struct lxdlist { · line 91
Quoted 7 lines
fs/jfs/jfs_types.h +1 -2
@@ -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.

Quoted 8 lines

Do you need a:
(quoted diff) +1 -0
+	__le24 len __packed;
fs/jfs/jfs_types.h struct lxdlist { · line 94
Quoted 4 lines
fs/jfs/jfs_types.h +1 -0
+	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)?

Quoted 4 lines

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.

Quoted 11 lines

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

Signature · 2 lines

David Kleikamp
IBM Linux Technology Center

Boaz Harrosh

2008-09-11 08:37 UTC #16 lore raw

Quoted 41 lines · Dave Kleikamp

Quoted 13 lines · Boaz Harrosh
fs/jfs/jfs_types.h struct timestruc_t { · line 66
Quoted 9 lines · Dave Kleikamp
fs/jfs/jfs_types.h +1 -1
@@ -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.
Quoted 8 lines
fs/jfs/jfs_types.h struct lxdlist { · line 91
Quoted 7 lines
fs/jfs/jfs_types.h +1 -2
@@ -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.
Quoted 8 lines
Do you need a:
(quoted diff) +1 -0
+	__le24 len __packed;
fs/jfs/jfs_types.h struct lxdlist { · line 94
Quoted 4 lines
fs/jfs/jfs_types.h +1 -0
+	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)?

Quoted 8 lines

Quoted 3 lines
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"

Quoted 5 lines

Quoted 5 lines
Chris you might want to change the definitions at linux/types.h to:
typedef struct { __u8 b[3]; } __be24, __le24 __packed;

Quoted 8 lines

Quoted 6 lines
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

Chris Leech

author

2008-09-11 01:51 UTC #17 lore raw

In reply to Boaz Harrosh

Quoted 8 lines · Boaz Harrosh

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

Chris Leech

author

2008-09-10 16:25 UTC #18 lore raw

In reply to Dave Kleikamp

Quoted 10 lines · Dave Kleikamp

Quoted 2 lines · Christoph Hellwig
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.

fs/jfs/jfs_types.h struct timestruc_t { · line 65

Quoted 8 lines

fs/jfs/jfs_types.h +1 -1

@@ -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

Chris Leech

author

2008-09-10 17:45 UTC #19 lore raw

Quoted 12 lines · Chris Leech

fs/jfs/jfs_types.h struct timestruc_t { · line 65
Quoted 8 lines · Dave Kleikamp
fs/jfs/jfs_types.h +1 -1
@@ -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

Boaz Harrosh

2008-09-10 18:04 UTC #20 lore raw

Quoted 13 lines · Chris Leech

Quoted 11 lines · Chris Leech
fs/jfs/jfs_types.h struct timestruc_t { · line 65
Quoted 8 lines · Dave Kleikamp
fs/jfs/jfs_types.h +1 -1
@@ -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.

Quoted 4 lines

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=/

Dave Kleikamp

2008-09-10 18:23 UTC #21 lore raw

In reply to Chris Leech

Quoted 14 lines · Chris Leech

Quoted 12 lines · Chris Leech
fs/jfs/jfs_types.h struct timestruc_t { · line 65
Quoted 8 lines · Dave Kleikamp
fs/jfs/jfs_types.h +1 -1
@@ -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.

Quoted 2 lines

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.

Signature · 2 lines

David Kleikamp
IBM Linux Technology Center