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

[Xu Yilun]

On 2022-12-31 at 12:46:28 -0800, matthew.gerlach@xxxxxxxxxxxxxxx wrote:
> 
> 
> On Mon, 26 Dec 2022, Xu Yilun wrote:
> 
> > On 2022-12-21 at 11:14:59 -0800, matthew.gerlach@xxxxxxxxxxxxxxx wrote:
> > > 
> > > 
> > > On Tue, 20 Dec 2022, Andy Shevchenko wrote:
> > > 
> > > > On Tue, Dec 20, 2022 at 08:36:51AM -0800, matthew.gerlach@xxxxxxxxxxxxxxx wrote:
> > > > > From: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx>
> > > > > 
> > > > > 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)

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)

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.

> 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
> > > > 
> > > > 
> > > > 
> >