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

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Elliot Berman <quic_eberman@quicinc.com>
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 01:22:59 +0100	[thread overview]
Message-ID: <Y2MJ43oVYfNgBZsQ@kroah.com> (raw)
In-Reply-To: <28eaa4bd-a9ee-c415-57c4-a9a56ffeef18@quicinc.com>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel