Re: [PATCH v10 3/4] fpga: dfl: add basic support for DFHv1

From: Xu Yilun <yilun.xu@intel.com>
To: matthew.gerlach@linux.intel.com
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	hao.wu@intel.com, russell.h.weight@intel.com,
	basheer.ahmed.muddebihal@intel.com, trix@redhat.com,
	mdf@kernel.org, linux-fpga@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	tianfei.zhang@intel.com, corbet@lwn.net,
	gregkh@linuxfoundation.org, linux-serial@vger.kernel.org,
	jirislaby@kernel.org, geert+renesas@glider.be,
	niklas.soderlund+renesas@ragnatech.se, macro@orcam.me.uk,
	johan@kernel.org, lukas@wunner.de, ilpo.jarvinen@linux.intel.com,
	marpagan@redhat.com, bagasdotme@gmail.com
Subject: Re: [PATCH v10 3/4] fpga: dfl: add basic support for DFHv1
Date: Wed, 11 Jan 2023 10:13:31 +0800	[thread overview]
Message-ID: <Y74bSzUBLYH4cLDh@yilunxu-OptiPlex-7050> (raw)
In-Reply-To: <alpine.DEB.2.22.394.2301101310150.815911@rhweight-WRK1>

On 2023-01-10 at 14:07:16 -0800, matthew.gerlach@linux.intel.com wrote:
> 
> 
> On Tue, 10 Jan 2023, Andy Shevchenko wrote:
> 
> > On Mon, Jan 09, 2023 at 04:30:28PM -0800, matthew.gerlach@linux.intel.com wrote:
> > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > 
> > > Version 1 of the Device Feature Header (DFH) definition adds
> > > functionality to the Device Feature List (DFL) bus.
> > > 
> > > A DFHv1 header may have one or more parameter blocks that
> > > further describes the HW to SW. Add support to the DFL bus
> > > to parse the MSI-X parameter.
> > > 
> > > The location of a feature's register set is explicitly
> > > described in DFHv1 and can be relative to the base of the DFHv1
> > > or an absolute address. Parse the location and pass the information
> > > to DFL driver.
> > 
> > ...
> > 
> > > v10: change dfh_find_param to return size of parameter data in bytes
> > 
> > The problem that might occur with this approach is byte ordering.
> > When we have u64 items, we know that they all are placed in CPU
> > ordering by the bottom layer. What's the contract now? Can it be
> > a problematic? Please double check this (always keep in mind BE32
> > as most interesting case for u64/unsigned long representation and
> > other possible byte ordering outcomes).
> 
> A number of u64 items certainly states explicit alignment of the memory, but
> I think byte ordering is a different issue.
> 
> The bottom layer, by design, is still enforcing a number u64 items under the
> hood. So the contract has not changed. Changing units of size from u64s to
> bytes was suggested to match the general practice of size of memory being in
> bytes. I think the suggestion was made because the return type for
> dfh_find_param() changed from u64* to void* in version 9, when indirectly
> returning the size of the parameter data was introduced.  So a void * with a
> size in bytes makes sense. On the other hand, returning a u64 * is a more
> precise reflection of the data alignment. I think the API should be as

I prefer (void *) + bytes. The properties in the parameter block are not
guarateed to be u64 for each, e.g. the REG_LAYOUT, so (void *) could better
indicate it is not. It is just a block of data unknown to DFL core and to
be parsed by drivers.

And why users/drivers need to care about the alignment of the parameter
block?

Thanks,
Yilun

> follows:
> 
> /**
>  * dfh_find_param() - find parameter block for the given parameter id
>  * @dfl_dev: dfl device
>  * @param_id: id of dfl parameter
>  * @pcount: destination to store size of parameter data in u64 bit words
>  *
>  * Return: pointer to start of parameter data, PTR_ERR otherwise.
>  */
> u64 *dfh_find_param(struct dfl_device *dfl_dev, int param_id, size_t
> *pcount)
> 
> Regarding byte ordering, Documentation/fpga/dfl.rst does not currently
> mention endianness. All current HW implementations of DFL are little-endian.
> I should add a statement in Documentation/fpga/dfl.rst that fields in the
> Device Feature Header are little-endian.
> 
> Thanks for the feedback,
> Matthew Gerlach
> 
> > 
> > -- 
> > With Best Regards,
> > Andy Shevchenko
> > 
> > 
> > 