| Message ID | 20221220163652.499831-4-matthew.gerlach@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enhance definition of DFH and use enhancements for UART driver | expand |
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? 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? > + */ > +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. > if (!finfo) > return -ENOMEM;
On Tue, 20 Dec 2022, 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. > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > --- > v7: no change > > v6: move MSI_X parameter definitions to drivers/fpga/dfl.h > > v5: update field names > fix find_param/dfh_get_psize > clean up mmio_res assignments > use u64* instead of void* > use FIELD_GET instead of masking > > v4: s/MSIX/MSI_X > move kernel doc to implementation > use structure assignment > fix decode of absolute address > clean up comment in parse_feature_irqs > remove use of csr_res > > v3: remove unneeded blank line > use clearer variable name > pass finfo into parse_feature_irqs() > refactor code for better indentation > use switch statement for irq parsing > squash in code parsing register location > > v2: fix kernel doc > clarify use of DFH_VERSION field > --- > +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 data for the given parameter id > + * @dfl_dev: dfl device > + * @param: id of dfl parameter > + * > + * Return: pointer to parameter header on success, 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); BTW, should there be a way for the caller to ensure the parameter is long enough? All callers probably want to ensure the length of the parameter is valid so it would perhaps make sense to add a parameter for the required (minimum) length?
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. > >> + */ >> +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. Thanks for the suggestion, Matthew Gerlach > >> if (!finfo) >> return -ENOMEM; > > -- > With Best Regards, > Andy Shevchenko > > >
On Wed, 21 Dec 2022, Ilpo Järvinen wrote: > On Tue, 20 Dec 2022, 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. >> >> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> >> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> >> --- >> v7: no change >> >> v6: move MSI_X parameter definitions to drivers/fpga/dfl.h >> >> v5: update field names >> fix find_param/dfh_get_psize >> clean up mmio_res assignments >> use u64* instead of void* >> use FIELD_GET instead of masking >> >> v4: s/MSIX/MSI_X >> move kernel doc to implementation >> use structure assignment >> fix decode of absolute address >> clean up comment in parse_feature_irqs >> remove use of csr_res >> >> v3: remove unneeded blank line >> use clearer variable name >> pass finfo into parse_feature_irqs() >> refactor code for better indentation >> use switch statement for irq parsing >> squash in code parsing register location >> >> v2: fix kernel doc >> clarify use of DFH_VERSION field >> --- > >> +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 data for the given parameter id >> + * @dfl_dev: dfl device >> + * @param: id of dfl parameter >> + * >> + * Return: pointer to parameter header on success, 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); > > BTW, should there be a way for the caller to ensure the parameter is long > enough? The caller can look at the DFHv1_PARAM_HDR_NEXT_OFFSET field of the parameter header to see the size of the parameter block (header plus data). > > All callers probably want to ensure the length of the parameter is valid > so it would perhaps make sense to add a parameter for the required > (minimum) length? Yes, all callers should ensure that the length of the parameter is valid. I will add another API call that performs length checking. Thanks for the feedback, Matthew Gerlach > > > -- > i. >
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 > > > > > >
On Mon, 26 Dec 2022, Xu Yilun wrote: > 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. 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 >>> >>> >>> >
On 2022-12-31 at 12:46:28 -0800, matthew.gerlach@linux.intel.com wrote: > > > On Mon, 26 Dec 2022, Xu Yilun wrote: > > > 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. > > 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) 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) 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. > 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 > > > > > > > > > > > > > >
On Tue, 3 Jan 2023, Xu Yilun wrote: > On 2022-12-31 at 12:46:28 -0800, matthew.gerlach@linux.intel.com wrote: >> >> >> On Mon, 26 Dec 2022, Xu Yilun wrote: >> >>> 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. >> >> 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 >>>>> >>>>> >>>>> >>> >
On 2023-01-03 at 11:50:04 -0800, matthew.gerlach@linux.intel.com wrote: > > > On Tue, 3 Jan 2023, Xu Yilun wrote: > > > On 2022-12-31 at 12:46:28 -0800, matthew.gerlach@linux.intel.com wrote: > > > > > > > > > On Mon, 26 Dec 2022, Xu Yilun wrote: > > > > > > > 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. > > > > > > 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? And adding a local variable dfh_psize = dfh_pcount * sizeof(u64) solves your concern. Using pcount for struct_size is more straightforward to me. dfh_psize could be truncated if it is not aligned to u64. People need to look into the dfh_get_psize() to check the correctness. Anyway this is trivial, I'm also OK with the change in v8. Thanks, Yilun > > 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 > > > > > > > > > > > > > > > > > > > > > > > >
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c index b9aae85ba930..df63c96bcbfc 100644 --- a/drivers/fpga/dfl.c +++ b/drivers/fpga/dfl.c @@ -342,6 +342,8 @@ static void release_dfl_dev(struct device *dev) if (ddev->mmio_res.parent) release_resource(&ddev->mmio_res); + kfree(ddev->params); + ida_free(&dfl_device_ida, ddev->id); kfree(ddev->irqs); kfree(ddev); @@ -380,7 +382,16 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata, ddev->type = feature_dev_id_type(pdev); ddev->feature_id = feature->id; ddev->revision = feature->revision; + ddev->dfh_version = feature->dfh_version; ddev->cdev = pdata->dfl_cdev; + if (feature->param_size) { + ddev->params = kmemdup(feature->params, feature->param_size, GFP_KERNEL); + if (!ddev->params) { + ret = -ENOMEM; + goto put_dev; + } + ddev->param_size = feature->param_size; + } /* add mmio resource */ parent_res = &pdev->resource[feature->resource_index]; @@ -708,20 +719,27 @@ struct build_feature_devs_info { * struct dfl_feature_info - sub feature info collected during feature dev build * * @fid: id of this sub feature. + * @revision: revision of this sub feature + * @dfh_version: version of Device Feature Header (DFH) * @mmio_res: mmio resource of this sub feature. * @ioaddr: mapped base address of mmio resource. * @node: node in sub_features linked list. * @irq_base: start of irq index in this sub feature. * @nr_irqs: number of irqs of this sub feature. + * @param_size: size DFH parameters. + * @params: DFH parameter data. */ struct dfl_feature_info { u16 fid; u8 revision; + u8 dfh_version; struct resource mmio_res; void __iomem *ioaddr; struct list_head node; unsigned int irq_base; unsigned int nr_irqs; + unsigned int param_size; + u64 params[]; }; static void dfl_fpga_cdev_add_port_dev(struct dfl_fpga_cdev *cdev, @@ -797,7 +815,17 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo) feature->dev = fdev; feature->id = finfo->fid; feature->revision = finfo->revision; + feature->dfh_version = finfo->dfh_version; + if (finfo->param_size) { + feature->params = devm_kmemdup(binfo->dev, + finfo->params, finfo->param_size, + GFP_KERNEL); + if (!feature->params) + return -ENOMEM; + + feature->param_size = finfo->param_size; + } /* * the FIU header feature has some fundamental functions (sriov * set, port enable/disable) needed for the dfl bus device and @@ -934,56 +962,105 @@ static u16 feature_id(u64 value) return 0; } +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 data for the given parameter id + * @dfl_dev: dfl device + * @param: id of dfl parameter + * + * Return: pointer to parameter header on success, 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); + static int parse_feature_irqs(struct build_feature_devs_info *binfo, - resource_size_t ofst, u16 fid, - unsigned int *irq_base, unsigned int *nr_irqs) + resource_size_t ofst, struct dfl_feature_info *finfo) { void __iomem *base = binfo->ioaddr + ofst; unsigned int i, ibase, inr = 0; + void *params = finfo->params; enum dfl_id_type type; + u16 fid = finfo->fid; int virq; + u64 *p; u64 v; - type = feature_dev_id_type(binfo->feature_dev); + switch (finfo->dfh_version) { + case 0: + /* + * DFHv0 only provides MMIO resource information for each feature + * in the DFL header. There is no generic interrupt information. + * Instead, features with interrupt functionality provide + * the information in feature specific registers. + */ + type = feature_dev_id_type(binfo->feature_dev); + if (type == PORT_ID) { + switch (fid) { + case PORT_FEATURE_ID_UINT: + v = readq(base + PORT_UINT_CAP); + ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v); + inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v); + break; + case PORT_FEATURE_ID_ERROR: + v = readq(base + PORT_ERROR_CAP); + ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v); + inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v); + break; + } + } else if (type == FME_ID) { + switch (fid) { + case FME_FEATURE_ID_GLOBAL_ERR: + v = readq(base + FME_ERROR_CAP); + ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v); + inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v); + break; + } + } + break; - /* - * Ideally DFL framework should only read info from DFL header, but - * current version DFL only provides mmio resources information for - * each feature in DFL Header, no field for interrupt resources. - * Interrupt resource information is provided by specific mmio - * registers of each private feature which supports interrupt. So in - * order to parse and assign irq resources, DFL framework has to look - * into specific capability registers of these private features. - * - * Once future DFL version supports generic interrupt resource - * information in common DFL headers, the generic interrupt parsing - * code will be added. But in order to be compatible to old version - * DFL, the driver may still fall back to these quirks. - */ - if (type == PORT_ID) { - switch (fid) { - case PORT_FEATURE_ID_UINT: - v = readq(base + PORT_UINT_CAP); - ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v); - inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v); - break; - case PORT_FEATURE_ID_ERROR: - v = readq(base + PORT_ERROR_CAP); - ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v); - inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v); + case 1: + /* + * DFHv1 provides interrupt resource information in DFHv1 + * parameter blocks. + */ + p = find_param(params, finfo->param_size, DFHv1_PARAM_ID_MSI_X); + if (!p) break; - } - } else if (type == FME_ID) { - if (fid == FME_FEATURE_ID_GLOBAL_ERR) { - v = readq(base + FME_ERROR_CAP); - ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v); - inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v); - } + + p++; + ibase = FIELD_GET(DFHv1_PARAM_MSI_X_STARTV, *p); + inr = FIELD_GET(DFHv1_PARAM_MSI_X_NUMV, *p); + break; + + default: + dev_warn(binfo->dev, "unexpected DFH version %d\n", finfo->dfh_version); + break; } if (!inr) { - *irq_base = 0; - *nr_irqs = 0; + finfo->irq_base = 0; + finfo->nr_irqs = 0; return 0; } @@ -1006,12 +1083,37 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo, } } - *irq_base = ibase; - *nr_irqs = inr; + finfo->irq_base = ibase; + finfo->nr_irqs = inr; return 0; } +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max) +{ + int size = 0; + u64 v, next; + + if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, + readq(dfh_base + DFHv1_CSR_SIZE_GRP))) + return 0; + + while (size + DFHv1_PARAM_HDR < max) { + v = readq(dfh_base + DFHv1_PARAM_HDR + size); + + next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v); + if (!next) + return -EINVAL; + + size += next * sizeof(u64); + + if (FIELD_GET(DFHv1_PARAM_HDR_NEXT_EOP, v)) + return size; + } + + return -ENOENT; +} + /* * when create sub feature instances, for private features, it doesn't need * to provide resource size and feature id as they could be read from DFH @@ -1023,39 +1125,69 @@ static int create_feature_instance(struct build_feature_devs_info *binfo, resource_size_t ofst, resource_size_t size, u16 fid) { - unsigned int irq_base, nr_irqs; struct dfl_feature_info *finfo; + resource_size_t start, end; + int dfh_psize = 0; u8 revision = 0; + u64 v, addr_off; + u8 dfh_ver = 0; int ret; - u64 v; if (fid != FEATURE_ID_AFU) { v = readq(binfo->ioaddr + ofst); revision = FIELD_GET(DFH_REVISION, v); - + dfh_ver = FIELD_GET(DFH_VERSION, v); /* read feature size and id if inputs are invalid */ size = size ? size : feature_size(v); fid = fid ? fid : feature_id(v); + if (dfh_ver == 1) { + dfh_psize = dfh_get_psize(binfo->ioaddr + ofst, size); + if (dfh_psize < 0) { + dev_err(binfo->dev, + "failed to read size of DFHv1 parameters %d\n", + dfh_psize); + return dfh_psize; + } + dev_dbg(binfo->dev, "dfhv1_psize %d\n", dfh_psize); + } } if (binfo->len - ofst < size) return -EINVAL; - ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs); - if (ret) - return ret; - - finfo = kzalloc(sizeof(*finfo), GFP_KERNEL); + finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL); if (!finfo) return -ENOMEM; + memcpy_fromio(finfo->params, binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize); + finfo->param_size = dfh_psize; + finfo->fid = fid; finfo->revision = revision; - finfo->mmio_res.start = binfo->start + ofst; - finfo->mmio_res.end = finfo->mmio_res.start + size - 1; + finfo->dfh_version = dfh_ver; + if (dfh_ver == 1) { + v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR); + addr_off = FIELD_GET(DFHv1_CSR_ADDR_MASK, v); + if (FIELD_GET(DFHv1_CSR_ADDR_REL, v)) + start = addr_off << 1; + else + start = binfo->start + ofst + addr_off; + + v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP); + end = start + FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1; + } else { + start = binfo->start + ofst; + end = start + size - 1; + } finfo->mmio_res.flags = IORESOURCE_MEM; - finfo->irq_base = irq_base; - finfo->nr_irqs = nr_irqs; + finfo->mmio_res.start = start; + finfo->mmio_res.end = end; + + ret = parse_feature_irqs(binfo, ofst, finfo); + if (ret) { + kfree(finfo); + return ret; + } list_add_tail(&finfo->node, &binfo->sub_features); binfo->feature_num++; diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h index fc59f33367ee..a21065cec04e 100644 --- a/drivers/fpga/dfl.h +++ b/drivers/fpga/dfl.h @@ -111,6 +111,10 @@ #define DFHv1_PARAM_HDR_NEXT_EOP BIT_ULL(32) #define DFHv1_PARAM_DATA 0x08 /* Offset of Param data from Param header */ +#define DFHv1_PARAM_ID_MSI_X 0x1 +#define DFHv1_PARAM_MSI_X_NUMV GENMASK_ULL(63, 32) +#define DFHv1_PARAM_MSI_X_STARTV GENMASK_ULL(31, 0) + /* Next AFU Register Bitfield */ #define NEXT_AFU_NEXT_DFH_OFST GENMASK_ULL(23, 0) /* Offset to next AFU */ @@ -272,11 +276,14 @@ struct dfl_feature_irq_ctx { * @ops: ops of this sub feature. * @ddev: ptr to the dfl device of this sub feature. * @priv: priv data of this feature. + * @param_size: size of dfh parameters + * @params: point to memory copy of dfh parameters */ struct dfl_feature { struct platform_device *dev; u16 id; u8 revision; + u8 dfh_version; int resource_index; void __iomem *ioaddr; struct dfl_feature_irq_ctx *irq_ctx; @@ -284,6 +291,8 @@ struct dfl_feature { const struct dfl_feature_ops *ops; struct dfl_device *ddev; void *priv; + unsigned int param_size; + void *params; }; #define FEATURE_DEV_ID_UNUSED (-1) diff --git a/include/linux/dfl.h b/include/linux/dfl.h index 431636a0dc78..a9b7ae84e5fd 100644 --- a/include/linux/dfl.h +++ b/include/linux/dfl.h @@ -39,9 +39,12 @@ struct dfl_device { u16 type; u16 feature_id; u8 revision; + u8 dfh_version; struct resource mmio_res; int *irqs; unsigned int num_irqs; + unsigned int param_size; + void *params; struct dfl_fpga_cdev *cdev; const struct dfl_device_id *id_entry; }; @@ -84,4 +87,5 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv); module_driver(__dfl_driver, dfl_driver_register, \ dfl_driver_unregister) +u64 *dfh_find_param(struct dfl_device *dfl_dev, int param); #endif /* __LINUX_DFL_H */