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

[matthew . gerlach]

On Thu, 29 Dec 2022, Andy Shevchenko wrote:

> On Wed, Dec 28, 2022 at 10:16:23AM -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
>
> Single space is enough.

Yes, single space is enough. Two spaces after a period is hard habit for 
me break. I will update in v9.

> > 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.
>
> I'm wondering what DFL states for.

I will define DFL in the commit message like DFH in the next revision.

> ...
>
> > +/**
> > + * dfh_get_u64_param_vals() - get array of u64 param values for given parameter id
> > + * @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);
>
> ENODATA ?

ENODATA does seem to be more accurate than ENOMEM in this case.

> > +	for (i = 0; i < nvals; i++)
> > +		*pval++ = param[i + 1];
>
> memcpy() ?

Using memcpy() will make code cleaner.

> > +	return param;
> > +}
>
> ...
>
> > +	finfo = kzalloc(struct_size(finfo, params, dfh_psize/sizeof(u64)), GFP_KERNEL);
>
> ' / ' (mind the spaces)

Yes, I will mind the spaces.

> Also, perhaps better to use sizeof(*params) or what is the member of that
> structure. So it will be more robust against possible changes.

params is the name of the structure member that is the trailing array.

> >  	if (!finfo)
> >  		return -ENOMEM;
>
> --
> With Best Regards,
> Andy Shevchenko

Thanks for the feedback,
Matthew Gerlach

>