On Mon, Nov 18, 2019 at 03:04:58PM +0200, Leon Romanovsky wrote:
> 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.
That 'magic' means we can just copy the spec text directly and don't
have to reprocess it by hand via different magic, which caused all the
mistakes above.
Jason
