Messages in this thread | | | Subject | Re: [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info | From | Tom Rix <> | Date | Fri, 18 Feb 2022 06:29:47 -0800 |
| |
On 2/17/22 10:53 PM, Zhang, Tianfei wrote: > >> -----Original Message----- >> From: Tom Rix <trix@redhat.com> >> Sent: Tuesday, February 15, 2022 10:49 PM >> To: Zhang, Tianfei <tianfei.zhang@intel.com>; Wu, Hao <hao.wu@intel.com>; >> mdf@kernel.org; Xu, Yilun <yilun.xu@intel.com>; linux-fpga@vger.kernel.org; >> linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org >> Cc: corbet@lwn.net >> Subject: Re: [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info >> >> >> On 2/14/22 3:26 AM, Tianfei zhang wrote: >>> From: Tianfei Zhang <tianfei.zhang@intel.com> >>> >>> The feature ID of "Port User Interrupt" and the "PMCI Subsystem" are >>> identical, 0x12, but one is for FME, other is for Port. It should >>> check the feature type While parsing the irq info in >>> parse_feature_irqs(). >> This seems like a bug fix and not part of iofs feature. >> >> Split this out of the patchset.
?
>> >> This is a workaround a hardware problem, there should be some comments to >> the effect that you can't trust _this_ or _that_ feature id and some special >> handling earlier. >> >> The ambiguity of feature id is a problem, and this sort of bug will happen again. >> >> What can be done to prevent this in the future ? > This patch is not workaround, this is a bug fix for DFL driver. > The root cause is that DLF driver miss check the feature type while parsing the interrupt information, > because some Feature IDs are identical between FME and Port, like PMCI in FME and "Port User Interrupt" > in Port. > The definition of Feature ID is here: > https://github.com/OPAE/linux-dfl-feature-id/blob/master/dfl-feature-ids.rst Helpful but hidden. At least a link to this should be added to Documentation/fpga/dfl.rst. >>> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com> >>> --- >>> drivers/fpga/dfl.c | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c index >>> 599bb21d86af..26f8cf890700 100644 >>> --- a/drivers/fpga/dfl.c >>> +++ b/drivers/fpga/dfl.c >>> @@ -940,9 +940,14 @@ static int parse_feature_irqs(struct >> build_feature_devs_info *binfo, >>> { >>> void __iomem *base = binfo->ioaddr + ofst; >>> unsigned int i, ibase, inr = 0; >>> + enum dfl_id_type type; >>> int virq; >>> u64 v; >>> >>> + type = feature_dev_id_type(binfo->feature_dev); >>> + if (type >= DFL_ID_MAX) >>> + return -EINVAL; >>> + >>> /* >>> * Ideally DFL framework should only read info from DFL header, but >>> * current version DFL only provides mmio resources information for >>> @@ -959,16 +964,22 @@ static int parse_feature_irqs(struct >> build_feature_devs_info *binfo, >>> */ >>> switch (fid) { >>> case PORT_FEATURE_ID_UINT: >>> + if (type != PORT_ID) >>> + break; >> Instead of embedding a break in the switch, break the switch into fme switch >> and port switch >> >> if (type == PORT_ID) >> >> port-switch >> >> else if (type == FME_ID >> >> fme-switch > Your suggestion is looks good for me, I will change on next version. > >> Tom >> >>> 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: >>> + if (type != PORT_ID) >>> + break; >>> 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; >>> case FME_FEATURE_ID_GLOBAL_ERR: >>> + if (type != FME_ID) >>> + break; >>> v = readq(base + FME_ERROR_CAP); >>> ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v); >>> inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
| |