[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