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

[Xu Yilun <yilun.xu@intel.com>]

On 2022-12-21 at 11:14:59 -0800, matthew.gerlach@linux.intel.com wrote:
> 
> 
> On Tue, 20 Dec 2022, Andy Shevchenko wrote:
> 
> > On Tue, Dec 20, 2022 at 08:36:51AM -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 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.

> 
> > 
> > > + */
> > > +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.

Or we just use size_add()?

Thanks,
Yilun

> 
> Thanks for the suggestion,
> Matthew Gerlach
> 
> 
> > 
> > >  	if (!finfo)
> > >  		return -ENOMEM;
> > 
> > -- 
> > With Best Regards,
> > Andy Shevchenko
> > 
> > 
> >