> -----Original Message----- > From: Tom Rix <trix@xxxxxxxxxx> > Sent: Friday, February 18, 2022 10:30 PM > To: Zhang, Tianfei <tianfei.zhang@xxxxxxxxx>; Wu, Hao <hao.wu@xxxxxxxxx>; > mdf@xxxxxxxxxx; Xu, Yilun <yilun.xu@xxxxxxxxx>; linux-fpga@xxxxxxxxxxxxxxx; > linux-doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > Cc: corbet@xxxxxxx > Subject: Re: [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info > > > On 2/17/22 10:53 PM, Zhang, Tianfei wrote: > > > >> -----Original Message----- > >> From: Tom Rix <trix@xxxxxxxxxx> > >> Sent: Tuesday, February 15, 2022 10:49 PM > >> To: Zhang, Tianfei <tianfei.zhang@xxxxxxxxx>; Wu, Hao > >> <hao.wu@xxxxxxxxx>; mdf@xxxxxxxxxx; Xu, Yilun <yilun.xu@xxxxxxxxx>; > >> linux-fpga@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx; > >> linux-kernel@xxxxxxxxxxxxxxx > >> Cc: corbet@xxxxxxx > >> 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@xxxxxxxxx> > >>> > >>> 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. > > ? I agree, I will send this patch as a bug fix. > > >> > >> 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-i > > ds.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@xxxxxxxxx> > >>> --- > >>> 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);
