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

[Xu Yilun]

On 2022-12-28 at 10:16:23 -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.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> ---
> v8: use struct_size() from overflow.h
>     add dfh_get_u64_param_vals()

Could you help check my comments?
https://lore.kernel.org/linux-fpga/alpine.DEB.2.22.394.2212211421210.570436@rhweight-WRK1/T/#md86e3836130ebacd3c088f5c512ba741aac8a4d1

[...]

> 
> +static u64 *find_param(u64 *params, resource_size_t max, int param_id)
> +{
> +	u64 *end = params + max / sizeof(u64);
> +	u64 v, next;
> +
> +	while (params < end) {
> +		v = *params;
> +		if (param_id == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
> +			return params;
> +
> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> +		params += next;
> +		if (FIELD_GET(DFHv1_PARAM_HDR_NEXT_EOP, v))
> +			break;
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * dfh_find_param() - find parameter block for the given parameter id
> + * @dfl_dev: dfl device
> + * @param: id of dfl parameter
> + *
> + * Return: pointer to start of parameter block, NULL otherwise.
> + */
> +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);

Didn't find where to use it?

> +
> +/**
> + * dfh_get_u64_param_vals() - get array of u64 param values for given parameter id

There is no rule to say one u64 for each property in the parameter block.
So I don't see the reason for DFL core to provide u64 array for the API,
And the size of the parameter block is decided by HW, why make the user
input the value?

As we discussed before, dfl core doesn't try to look into the parameter
block. So please just provide the const void *data & data_size for drivers.
This is the most common way to represent a data block.

Thanks,
Yilun

> + * @dfl_dev: dfl device
> + * @param: id of dfl parameter
> + * @pval: location of parameter data destination
> + * @nvals: number of u64 elements of parameter data
> + *
> + * Return: pointer to start of parameter block, PTR_ERR otherwise
> + */
> +u64 *dfh_get_u64_param_vals(struct dfl_device *dfl_dev, int param_id, u64 *pval, int nvals)
> +{
> +	u64 *param = find_param(dfl_dev->params, dfl_dev->param_size, param_id);
> +	u64 next;
> +	int i;
> +
> +	if (!param)
> +		return ERR_PTR(-ENOENT);
> +
> +	next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, *param);
> +
> +	if (nvals >= next)
> +		return ERR_PTR(-ENOMEM);
> +
> +	for (i = 0; i < nvals; i++)
> +		*pval++ = param[i + 1];
> +
> +	return param;
> +}
> +EXPORT_SYMBOL_GPL(dfh_get_u64_param_vals);
> +