On Wed, Nov 02, 2022 at 11:04:45AM -0700, Elliot Berman wrote:
> > > > > +/* Resource Manager Header */
> > > > > +struct gh_rm_rpc_hdr {
> > > > > + u8 version : 4, hdr_words : 4;
> > > > > + u8 type : 2, fragments : 6;
> > > >
> > > > Ick, that's hard to read. One variable per line please?
> > >
> > > Ack.
> > >
> > > > And why the bit packed stuff? Are you sure this is the way to do this?
> > > > Why not use a bitmask instead?
> > > >
> > >
> > > I felt bit packed implementation is cleaner and easier to map to
> > > understanding what the fields are used for.
> >
> > Ah, so this isn't what is on the "wire", then don't use a bitfield like
> > this, use a real variable and that will be faster and simpler to
> > understand.
> >
>
> This is what's on the "wire". Whether I use bitfield or bit packed would be
> functionally the same and is just a cosmetic change IMO.
Ah, that wasn't obvious at all.
Usually using bitfields like this for "wire" protocols is not a good
idea (endian issues and all of that.) Please use a bitmask instead, as
that way you know exactly what is happening, and the compiler can
usually generate much better code overall.
And as this is on the wire, please specify the endian values, _AND_ use
the proper kernel types for stuff that goes between user/kernel or
kernel/hardware, as you are not doing that here.
thanks,
greg k-h
