| Message ID | 20210512032752.16611-4-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SRP kernel patches for kernel v5.14 | expand |
On Tue, May 11, 2021 at 08:27:50PM -0700, Bart Van Assche wrote: > @@ -107,10 +107,10 @@ struct srp_direct_buf { > * having the 20-byte structure padded to 24 bytes on 64-bit architectures. > */ > struct srp_indirect_buf { > - struct srp_direct_buf table_desc; > + struct srp_direct_buf table_desc __packed; > __be32 len; > - struct srp_direct_buf desc_list[]; > -} __attribute__((packed)); > + struct srp_direct_buf desc_list[] __packed; > +}; > > /* Immediate data buffer descriptor as defined in SRP2. */ > struct srp_imm_buf { > @@ -175,13 +175,13 @@ struct srp_login_rsp { > u8 opcode; > u8 reserved1[3]; > __be32 req_lim_delta; > - u64 tag; > + u64 tag __packed; What you really want is just something like this: typedef u64 __attribute__((aligned(4))) compat_u64; And then use that every place you have the u64 and forget about packed entirely. Except for a couple exceptions IBA mads are always aligned to 4 bytes, only the 64 bit quantities are unaligned. But really this whole thing should be replaced with the IBA_FIELD macros like include/rdma/ibta_vol1_c12.h demos. Then it would be sparse safe and obviously endian correct as well. I suppose you are respinning this due to the other comments? Jason
On 5/20/21 7:48 AM, Jason Gunthorpe wrote: > On Tue, May 11, 2021 at 08:27:50PM -0700, Bart Van Assche wrote: >> @@ -107,10 +107,10 @@ struct srp_direct_buf { >> * having the 20-byte structure padded to 24 bytes on 64-bit architectures. >> */ >> struct srp_indirect_buf { >> - struct srp_direct_buf table_desc; >> + struct srp_direct_buf table_desc __packed; >> __be32 len; >> - struct srp_direct_buf desc_list[]; >> -} __attribute__((packed)); >> + struct srp_direct_buf desc_list[] __packed; >> +}; >> >> /* Immediate data buffer descriptor as defined in SRP2. */ >> struct srp_imm_buf { >> @@ -175,13 +175,13 @@ struct srp_login_rsp { >> u8 opcode; >> u8 reserved1[3]; >> __be32 req_lim_delta; >> - u64 tag; >> + u64 tag __packed; > > What you really want is just something like this: > > typedef u64 __attribute__((aligned(4))) compat_u64; > > And then use that every place you have the u64 and forget about packed > entirely. Really? My understanding is that the aligned attribute can be used to increase alignment of a structure member but not to decrease it. From https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#Common-Variable-Attributes: "When used on a struct, or struct member, the aligned attribute can only increase the alignment; in order to decrease it, the packed attribute must be specified as well." Additionally, there is a recommendation in Documentation/process/coding-style.rst not to introduce new typedefs. > Except for a couple exceptions IBA mads are always aligned to 4 bytes, > only the 64 bit quantities are unaligned. > > But really this whole thing should be replaced with the IBA_FIELD > macros like include/rdma/ibta_vol1_c12.h demos. > > Then it would be sparse safe and obviously endian correct as well. I prefer a structure over the IBA_FIELD macros so I will change __packed into __packed __aligned(4). Thanks, Bart.
On Sun, May 23, 2021 at 08:41:25PM -0700, Bart Van Assche wrote: > On 5/20/21 7:48 AM, Jason Gunthorpe wrote: > > On Tue, May 11, 2021 at 08:27:50PM -0700, Bart Van Assche wrote: > >> @@ -107,10 +107,10 @@ struct srp_direct_buf { > >> * having the 20-byte structure padded to 24 bytes on 64-bit architectures. > >> */ > >> struct srp_indirect_buf { > >> - struct srp_direct_buf table_desc; > >> + struct srp_direct_buf table_desc __packed; > >> __be32 len; > >> - struct srp_direct_buf desc_list[]; > >> -} __attribute__((packed)); > >> + struct srp_direct_buf desc_list[] __packed; > >> +}; > >> > >> /* Immediate data buffer descriptor as defined in SRP2. */ > >> struct srp_imm_buf { > >> @@ -175,13 +175,13 @@ struct srp_login_rsp { > >> u8 opcode; > >> u8 reserved1[3]; > >> __be32 req_lim_delta; > >> - u64 tag; > >> + u64 tag __packed; > > > > What you really want is just something like this: > > > > typedef u64 __attribute__((aligned(4))) compat_u64; > > > > And then use that every place you have the u64 and forget about packed > > entirely. > > Really? My understanding is that the aligned attribute can be used to > increase alignment of a structure member but not to decrease it. I didn't test it, but that is pre-existing code in Linux.. Maybe it doesn't work and/or needs packed too > Additionally, there is a recommendation in > Documentation/process/coding-style.rst not to introduce new typedefs. That we tend to selective ignore <shrug> > > Except for a couple exceptions IBA mads are always aligned to 4 bytes, > > only the 64 bit quantities are unaligned. > > > > But really this whole thing should be replaced with the IBA_FIELD > > macros like include/rdma/ibta_vol1_c12.h demos. > > > > Then it would be sparse safe and obviously endian correct as well. > > I prefer a structure over the IBA_FIELD macros so I will change __packed > into __packed __aligned(4). IMHO the struct is alot worse due to lack of endian safety and complexity in establishing the layout. Jason
diff --git a/include/scsi/srp.h b/include/scsi/srp.h index 177d8026e96f..84d2b5fc1409 100644 --- a/include/scsi/srp.h +++ b/include/scsi/srp.h @@ -107,10 +107,10 @@ struct srp_direct_buf { * having the 20-byte structure padded to 24 bytes on 64-bit architectures. */ struct srp_indirect_buf { - struct srp_direct_buf table_desc; + struct srp_direct_buf table_desc __packed; __be32 len; - struct srp_direct_buf desc_list[]; -} __attribute__((packed)); + struct srp_direct_buf desc_list[] __packed; +}; /* Immediate data buffer descriptor as defined in SRP2. */ struct srp_imm_buf { @@ -175,13 +175,13 @@ struct srp_login_rsp { u8 opcode; u8 reserved1[3]; __be32 req_lim_delta; - u64 tag; + u64 tag __packed; __be32 max_it_iu_len; __be32 max_ti_iu_len; __be16 buf_fmt; u8 rsp_flags; u8 reserved2[25]; -} __attribute__((packed)); +}; struct srp_login_rej { u8 opcode; @@ -207,10 +207,6 @@ struct srp_t_logout { u64 tag; }; -/* - * We need the packed attribute because the SRP spec only aligns the - * 8-byte LUN field to 4 bytes. - */ struct srp_tsk_mgmt { u8 opcode; u8 sol_not; @@ -225,10 +221,6 @@ struct srp_tsk_mgmt { u8 reserved5[8]; }; -/* - * We need the packed attribute because the SRP spec only aligns the - * 8-byte LUN field to 4 bytes. - */ struct srp_cmd { u8 opcode; u8 sol_not; @@ -266,7 +258,7 @@ struct srp_rsp { u8 sol_not; u8 reserved1[2]; __be32 req_lim_delta; - u64 tag; + u64 tag __packed; u8 reserved2[2]; u8 flags; u8 status; @@ -275,7 +267,7 @@ struct srp_rsp { __be32 sense_data_len; __be32 resp_data_len; u8 data[]; -} __attribute__((packed)); +}; struct srp_cred_req { u8 opcode; @@ -301,13 +293,13 @@ struct srp_aer_req { u8 sol_not; u8 reserved[2]; __be32 req_lim_delta; - u64 tag; + u64 tag __packed; u32 reserved2; struct scsi_lun lun; __be32 sense_data_len; u32 reserved3; u8 sense_data[]; -} __attribute__((packed)); +}; struct srp_aer_rsp { u8 opcode;
Applying the __packed attribute to an entire data structure results in suboptimal code on architectures that do not support unaligned accesses. Hence apply the __packed attribute only to those data members that are not naturally aligned. Cc: Nicolas Morey-Chaisemartin <nmoreychaisemartin@suse.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- include/scsi/srp.h | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-)