On Tue, 3 Jan 2023, Xu Yilun wrote:
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)
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?
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
