Message-ID: <Y7TlvD+tULKVl8WQ@yilunxu-OptiPlex-7050>
Date:   Wed, 4 Jan 2023 10:34:36 +0800
From:   Xu Yilun <yilun.xu@...el.com>
To:     matthew.gerlach@...ux.intel.com
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        hao.wu@...el.com, russell.h.weight@...el.com,
        basheer.ahmed.muddebihal@...el.com, trix@...hat.com,
        mdf@...nel.org, linux-fpga@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        tianfei.zhang@...el.com, corbet@....net,
        gregkh@...uxfoundation.org, linux-serial@...r.kernel.org,
        jirislaby@...nel.org, geert+renesas@...der.be,
        niklas.soderlund+renesas@...natech.se, macro@...am.me.uk,
        johan@...nel.org, lukas@...ner.de, ilpo.jarvinen@...ux.intel.com,
        marpagan@...hat.com, bagasdotme@...il.com
Subject: Re: [PATCH v7 3/4] fpga: dfl: add basic support for DFHv1

On 2023-01-03 at 11:50:04 -0800, matthew.gerlach@...ux.intel.com wrote:
> 
> 
> On Tue, 3 Jan 2023, Xu Yilun wrote:
> 
> > On 2022-12-31 at 12:46:28 -0800, matthew.gerlach@...ux.intel.com wrote:
> > > 
> > > 
> > > On Mon, 26 Dec 2022, Xu Yilun wrote:
> > > 
> > > > On 2022-12-21 at 11:14:59 -0800, matthew.gerlach@...ux.intel.com wrote:
> > > > > 
> > > > > 
> > > > > On Tue, 20 Dec 2022, Andy Shevchenko wrote:
> > > > > 
> > > > > > On Tue, Dec 20, 2022 at 08:36:51AM -0800, matthew.gerlach@...ux.intel.com wrote:
> > > > > > > From: Matthew Gerlach <matthew.gerlach@...ux.intel.com>
> > > > > > > 
> > > > > > > Version 1 of the Device Feature Header (DFH) definition adds
> > > > > > > functionality to the 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.
> > > > > > 
> > > > > > ...
> > > > > > 
> > > > > > > +/**
> > > > > > > + * dfh_find_param() - find data for the given parameter id
> > > > > > > + * @dfl_dev: dfl device
> > > > > > > + * @param: id of dfl parameter
> > > > > > > + *
> > > > > > > + * Return: pointer to parameter header on success, NULL otherwise.
> > > > > > 
> > > > > > header is a bit confusing here, does it mean we give and ID and we got
> > > > > > something more than just a data as summary above suggests?
> > > > > 
> > > > > Yes, the summary is not correct.  It should say "find the parameter block
> > > > > for the given parameter id".
> > > > > 
> > > > > > 
> > > > > > In such case summary and this text should clarify what exactly we get
> > > > > > and layout of the data. Since this is a pointer, who is responsible of
> > > > > > checking out-of-boundary accesses? For instance, if the parameters are
> > > > > > variadic by length the length should be returned as well. Otherwise it
> > > > > > should be specified as a constant somewhere, right?
> > > > > 
> > > > > The parameter header has the next/size field; so the caller of
> > > > > dfh_find_param should perform boundary checking as part of interpreting the
> > > > > parameter data.  I think a function to perform this checking and data
> > > > > interpretation would help here.
> > > > 
> > > > It is better the DFL core provides the size of the parameter block, just
> > > > in this API. It provides the pointer and should be ensured the memory
> > > > for the pointer be correctly understood.
> > > 
> > > Ok, how about the following API for dfh_find_param?
> > > 
> > > /**
> > >  * dfh_find_param() - find parameter block for the given parameter id
> > >  * @dfl_dev: dfl device
> > >  * @param_id: id of dfl parameter
> > >  * @pver: destination to store parameter version
> > >  * @pcount: destination to store size of parameter data in u64 bit words
> > 
> > The size of the parameter data could just be number of bytes (size_t is
> > ok?), this is the most common way for a data block.
> > 
> > >  *
> > >  * Return: pointer to start of parameter data, PTR_ERR otherwise.
> > >  */
> > > void *dfh_find_param(struct dfl_device *dfl_dev, int param_id, unsigned
> > > *pver, unsigned *pcount)
> > 
> > For now no driver is caring about parameter version, so we could just have
> > a simplified API without version, like:
> > 
> >  void *dfh_find_param(struct dfl_device *dfl_dev, int param_id, size_t *psize)
> 
> Using size_t and the simplified API you suggest is fine with me.
> 
> > 
> > I assume this simplified API should be most commonly used by drivers,
> > changing the layout of the parameter block is not such a good idea to
> > me, try best not to do so.
> > 
> > If more property is to be added without changing the existing fields,
> > drivers could be aware of this just by the parameter size?
> > 
> > 
> > Anyway, if version is really needed in future, create another API like:
> > 
> >  void *dfh_find_param_version(struct dfl_device *dfl_dev, int param_id,
> >  			       size_t *psize, unsigned int *pver)
> 
> Sure, we can add API when it is actually used, as you point out, the
> structure of a particular paramater should not change very often.
> 
> > 
> > Thanks,
> > Yilun
> > 
> > > 
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > > + */
> > > > > > > +u64 *dfh_find_param(struct dfl_device *dfl_dev, int param_id)
> > > > > > > +{
> > > > > > > +	return find_param(dfl_dev->params, dfl_dev->param_size, param_id);
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL_GPL(dfh_find_param);
> > > > > > 
> > > > > > ...
> > > > > > 
> > > > > > > +	finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
> > > > > > 
> > > > > > It sounds like a candidate for struct_size() from overflow.h.
> > > > > > I.o.w. check that header and come up with the best what can
> > > > > > suit your case.
> > > > > 
> > > > > 	finfo = kzalloc(struct_size(finfo, params, dfh_psize/sizeof(u64)),
> > > > > GFP_KERNEL);
> > > > > 
> > > > > Does seem better.
> > > > 
> > > > How about we change the dfh_get_psize() to like dfh_get_pcount(), so we
> > > > don't have to multiply & divide back and forth.
> > > 
> > > We need the size in bytes for calls to kmemdup, devm_kmemdup, and
> > 
> > When the count of u64 is caculated, you could still convert it to size of
> > bytes when needed.
> 
> We need to use number of bytes more often than than count of u64.  How would
> calculating bytes from counts of u64 three times be better than calculating
> counts of u64 once, like it is now?

And adding a local variable
dfh_psize = dfh_pcount * sizeof(u64) solves your concern.

Using pcount for struct_size is more straightforward to me. dfh_psize
could be truncated if it is not aligned to u64. People need to look into
the dfh_get_psize() to check the correctness.

Anyway this is trivial, I'm also OK with the change in v8.

Thanks,
Yilun

> 
> Thanks,
> Matthew Gerlach
> 
> > 
> > > memcpy_fromio, but we only need to divide once here.
> > > 
> > > 
> > > > 
> > > > Or we just use size_add()?
> > > 
> > > I think using struct_size is better because the params member of struct
> > > dfl_feature_info is a trailing flexible array.
> > 
> > That's OK.
> > 
> > > 
> > > Thanks for the feedback,
> > > Matthew
> > > 
> > > 
> > > > 
> > > > Thanks,
> > > > Yilun
> > > > 
> > > > > 
> > > > > Thanks for the suggestion,
> > > > > Matthew Gerlach
> > > > > 
> > > > > 
> > > > > > 
> > > > > > >  	if (!finfo)
> > > > > > >  		return -ENOMEM;
> > > > > > 
> > > > > > --
> > > > > > With Best Regards,
> > > > > > Andy Shevchenko
> > > > > > 
> > > > > > 
> > > > > > 
> > > > 
> > 

Powered by blists - more mailing lists