Re: [PATCH v6 10/21] gunyah: rsc_mgr: Add resource manager RPC core

[Elliot Berman]

On 11/2/2022 5:22 PM, Greg Kroah-Hartman wrote:
> 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.

Ack