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
*
* 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)
+ */
+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
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.
Thanks for the feedback,
Matthew
Thanks,
Yilun
Thanks for the suggestion,
Matthew Gerlach
if (!finfo)
return -ENOMEM;
--
With Best Regards,
Andy Shevchenko
