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
