I see couple of comments on rsvd words. They were primarily not introduced for alignment. But there are other new features that we will be adding with new set of hardware and firmware updates. I don't want to change the user-kernel interface at such stage by modifying the size of the structure. For some features its under testing stage internally. So once its ready rsvd will be replaced with actual element. This will avoid abi compatibility issues between library and driver.
I'll consider alignment macro too so that compiler related byte alignment access issue also gets resolved. Parav > -----Original Message----- > From: Roland Dreier [mailto:rol...@purestorage.com] > Sent: Wednesday, March 21, 2012 9:50 PM > To: Pandit, Parav > Cc: linux-rdma@vger.kernel.org; net...@vger.kernel.org > Subject: Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA > adapter > > On Tue, Mar 20, 2012 at 3:39 PM, <parav.pan...@emulex.com> wrote: > > From: Parav Pandit <parav.pan...@emulex.com> > > > > - Header file for userspace library and kernel driver interface. > > > +struct ocrdma_alloc_ucontext_resp { > > + u32 dev_id; > > + u32 wqe_size; > > + u32 max_inline_data; > > + u32 dpp_wqe_size; > > + u64 ah_tbl_page; > > + u32 ah_tbl_len; > > + u32 rsvd; > > + u8 fw_ver[32]; > > + u32 rqe_size; > > + u64 rsvd1; > > +} __packed; > > If I'm reading this correctly, you have the 8-byte rsvd1 member at an offset > only aligned to 4 bytes, because of the __packed directive. It would be much > better to have these structures laid out so they are naturally the same on > both 32-bit and 64-bit ABIs, and get rid of the __packed directive, which > wrecks gcc code generation in some cases. > > In this particular case, it seems you could just move rqe_size into the slot > where rsvd is, and get rid of rsvd1? > > > +/* user kernel communication data structures. */ struct > > +ocrdma_alloc_pd_ureq { > > + u64 rsvd1; > > +} __packed; > > Similar comment -- __packed is silly for a structure with one reserved > member (and which you don't seem to use anywhere)... why not just delete > this struct? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
