On Fri, Nov 15, 2019 at 04:45:58PM -0400, Jason Gunthorpe wrote:
> On Sun, Oct 27, 2019 at 09:05:39AM +0200, Leon Romanovsky wrote:
>
> > #define IB_CM_CLASS_VERSION 2 /* IB specification 1.2 */
> > +#define _CM_SET(p, offset, mask, value) \
> > + ({ \
> > + void *field = (u8 *)p + sizeof(struct ib_mad_hdr) + offset; \
> > + u8 bytes = \
> > + DIV_ROUND_UP(__builtin_popcount(mask), BITS_PER_BYTE); \
> > + switch (bytes) { \
> > + case 1: { \
> > + *(u8 *)field &= ~mask; \
> > + *(u8 *)field |= FIELD_PREP(mask, value); \
> > + } break; \
> > + case 2: { \
> > + u16 val = ntohs(*(__be16 *)field) & ~mask; \
> > + val |= FIELD_PREP(mask, value); \
> > + *(__be16 *)field = htons(val); \
> > + } break; \
> > + case 3: { \
> > + u32 val = ntohl(*(__be32 *)field) & ~(mask << 8); \
> > + val |= FIELD_PREP(mask, value) << 8; \
> > + *(__be32 *)field = htonl(val); \
>
> This doesn't work for flow label which has a 20 byte field, the <<8 is
> just a hack to fix the 24 byte case.
>
> This is also some typo's:
>
> > + #define CM_REQ_LOCAL_EECN_OFFSET 36
> > + #define CM_REQ_LOCAL_EECN_MASK GENMASK(24, 0)
>
> Should be 23, 0
>
> > + #define CM_REQ_PRIMARY_PACKET_RATE_OFFSET 91
> > + #define CM_REQ_PRIMARY_PACKET_RATE_MASK GENMASK(3, 2)
>
> Packet rate is a 6 bit field, not a 2 bit field
>
> I only looked at REQ. I assume all the others have a similar error
> rate.
>
> Overall, I don't like this approach. The macros are messy/buggy and
> there isn't a clear mapping of the data in the tables to the C code.
>
> How about this instead:
I very liked type safety in your solution, but I think that IBA_FIELD*_LOC()
macros add too much magic into such simple thing like spec declarations.
I'll update, recheck and resend.
Thanks
