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

From: Elliot Berman <quic_eberman@quicinc.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jassi Brar <jassisinghbrar@gmail.com>,
	Bjorn Andersson <quic_bjorande@quicinc.com>,
	Murali Nalajala <quic_mnalajal@quicinc.com>,
	Trilok Soni <quic_tsoni@quicinc.com>,
	Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>,
	Carl van Schaik <quic_cvanscha@quicinc.com>,
	Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>,
	Andy Gross <agross@kernel.org>,
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	<linux-arm-kernel@lists.infradead.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Sudeep Holla <sudeep.holla@arm.com>,
	Marc Zyngier <maz@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Jonathan Corbet <corbet@lwn.net>, "Will Deacon" <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	"Arnd Bergmann" <arnd@arndb.de>,
	Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
	Amol Maheshwari <amahesh@qti.qualcomm.com>,
	Kalle Valo <kvalo@kernel.org>, <devicetree@vger.kernel.org>,
	<linux-doc@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v6 10/21] gunyah: rsc_mgr: Add resource manager RPC core
Date: Thu, 3 Nov 2022 15:07:44 -0700	[thread overview]
Message-ID: <032eeabb-28a5-9568-6aee-5631acb72a7b@quicinc.com> (raw)
In-Reply-To: <Y2MJ43oVYfNgBZsQ@kroah.com>


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