Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Wu, Hao" <hao.wu@intel.com>
To:     "Zhang, Tianfei" <tianfei.zhang@intel.com>,
        "trix@redhat.com" <trix@redhat.com>,
        "mdf@kernel.org" <mdf@kernel.org>,
        "Xu, Yilun" <yilun.xu@intel.com>,
        "linux-fpga@vger.kernel.org" <linux-fpga@vger.kernel.org>,
        "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
CC:     "corbet@lwn.net" <corbet@lwn.net>
Subject: RE: [PATCH v1] fpga: dfl: check feature type before parse irq info
Thread-Topic: [PATCH v1] fpga: dfl: check feature type before parse irq info
Thread-Index: AQHYKWCM9LLv46vm40url05b1bO9gKyo0a6AgADuMYCAABDScA==
Date:   Tue, 1 Mar 2022 02:22:52 +0000
Message-ID: <DM6PR11MB3819683BD840979FF8F6084885029@DM6PR11MB3819.namprd11.prod.outlook.com>
References: <20220224092242.127081-1-tianfei.zhang@intel.com>
 <DM6PR11MB381942D6F18C6518FE3556B585019@DM6PR11MB3819.namprd11.prod.outlook.com>
 <BN9PR11MB548388C1E086059E277F4C8BE3029@BN9PR11MB5483.namprd11.prod.outlook.com>
In-Reply-To: <BN9PR11MB548388C1E086059E277F4C8BE3029@BN9PR11MB5483.namprd11.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-reaction: no-action
dlp-version: 11.6.401.20
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?IqzfQVwi71itzb2xCCEtgHgsPYJC8bdZtwmbH+Jiz1sG8kLmptDkwhuWccoG?=
 =?us-ascii?Q?21dC5cJ1klRO3MUwYPZs+4tfjJu8q2kNN0+VceiKPRkP9iBal04xfAti5vPr?=
 =?us-ascii?Q?ailuKWyOPlr8aB9ALPc1SYRw+TturX+gCViVnXZrd8g2fwlowRQrBBjmAz8+?=
 =?us-ascii?Q?tR8mA78ynHGxOConwzB6pPQDs45vQQtAkR1AHyeurYiBjgB1qubEQpqgYAMn?=
 =?us-ascii?Q?PidY7YHYcBFhwR76nDNvS9zKxiUCXBJMkG+UKLbZo9v+ryzfd1o3+ASuego3?=
 =?us-ascii?Q?sOUg/1AtKnbU9r6Yv1hL1RC0bNsNXU1xRFMs87b5f5/Cfwhkt55P9rQ/NuKZ?=
 =?us-ascii?Q?lB0RU2BO9UD3e3cSJjJ4iZ0NTSnEUOh8bUVqZXJc2AYaFXhFEw1CeJXhYkIb?=
 =?us-ascii?Q?C90CIC9/aa2yt84HUcUyltapbhF8/TS3jyDqCXicM2OU0JSqODx+BSQiXnE5?=
 =?us-ascii?Q?Vtoi48r14HZCTDFxsdavcA7QMgnEocXzt8AgSAVWiuJW4iUUqRicTECarQkk?=
 =?us-ascii?Q?4Bmqrq+cRYv9PoDy8n3XWf6Mx7gkJ8ZDlrm6UMV2EqPO2klmEuhS9/gJ3OEP?=
 =?us-ascii?Q?nUt5TzHd4Otp7VgaTdBCGa9RLrjH1n9iTwWKyfEddCIfQfGEVzQmPEtdO4qI?=
 =?us-ascii?Q?QGf1rk7xcdz8zWEtqwDclRMQLH19IlpGt4gLrT/0qrxcqVu6BXrWhIGhpsr/?=
 =?us-ascii?Q?BXqsIC8O71jdzVOLoElULhQUTTf8PaEVW84D/45sspEOTGyr9RoydeuCMKq6?=
 =?us-ascii?Q?sQfwRowHTh2L1fHKd6XgETCfQJWaqDPMj1+7K+qzuU2xSdFHK6kXqpA9YeO2?=
 =?us-ascii?Q?/iaGdAfDg1vQbESShXICOyvy4qtky9+nD8/Yr8DEBNfH4oSg6sEuEsSodvMf?=
 =?us-ascii?Q?lNeHpL8HZEBNCCQmm2dgDCM51EmQscGMP9nPILdf5NwZpVVcs6L68FoM/azZ?=
 =?us-ascii?Q?2P0HV2LPyhr6ZsoE+20Imj/tE3w4bZL9YGBHSbhfKd9l1XfvYf8hdIt9yPrm?=
 =?us-ascii?Q?P5oy7XQ528wkh6HRmfqOn8yeziMY2f661bLs1SS6etgr5NRl35o5uUR3cy4z?=
 =?us-ascii?Q?kYgxb8D6pZXG+CgpisQfXCOS3R5LTp0T4RgyHJk5/IzUJv8N4IcbjN/tH6FK?=
 =?us-ascii?Q?82BBgzFs+a6qn5g8T0gA+kMaGPuzv6SS2hREaWMcF/VoXrF3dql+oXc/FRlH?=
 =?us-ascii?Q?4AsapJYl1QDQg0FH5G9xGSoMmZq9o0crxXw+qIlo2KSPUstqte2ImUpp6cD8?=
 =?us-ascii?Q?n6vWdSqbyP0yeWLGIv9rF27Mnv6wh+Iid3mRwdiauKa7I90Gh47iLfQY/ewk?=
 =?us-ascii?Q?9tOt/B1kWrYTLoY7XUK6+x0AyJawovd1OtlrZ8g/uJb/BHIY2OFE33U49AY2?=
 =?us-ascii?Q?4aXiTJ3WNf0niYlwVyEthyIih6gkSxHfrQ/DfeBh29z88fTfqc7pQMK+ks9B?=
 =?us-ascii?Q?oUJjQmmSHrOLy85ge1n+IqrMUgDAevQaiBIOE7UQemkMSE6Nvu5arpBaaCc/?=
 =?us-ascii?Q?EXpyu+DvHxb01MvSQbaQpTEZBD1ulpsB2PUrAWMmJFW/8uggjtYtKhV4YjZQ?=
 =?us-ascii?Q?GJCJHT8wcPBS7wrrHqQaRTVUvUxrt90V+MJalbw0I1kFj1ZYCGMYGcTG1XQc?=
 =?us-ascii?Q?VQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3819.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1a6293c4-12c7-4002-01ea-08d9fb2a6351
X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Mar 2022 02:22:52.2149
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: wSDI9o7T1oH5fGuG3zXtQhiiuIyl5jQDFAlIUAI7YpZISFQRm/ZxnISaNw/NMCxDM+moQOoeLKA94scjLGUqxg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB4867
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> > -----Original Message-----
> > From: Wu, Hao <hao.wu@intel.com>
> > Sent: Monday, February 28, 2022 7:10 PM
> > To: Zhang, Tianfei <tianfei.zhang@intel.com>; trix@redhat.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] fpga: dfl: check feature type before parse irq =
info
> >
> > > Subject: [PATCH v1] fpga: dfl: check feature type before parse irq
> > > info
> > >
> > > 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().
> > >
> > > Fixes: 8d021039cbb5 ("fpga: dfl: parse interrupt info for feature
> > > devices on
> > > enumeration")
> >
> > Actually this is not a real bug, as in original design, there is no ove=
rlap for FME
> > and Port features. This is why you see features for Port doesn't start =
from 0.
> But
> > anyway I am good with such extension.
>=20
> I think this is a bug, I add some printk debug log in DFL driver, we obse=
rved that

Per original design, you should not use overlap feature IDs for FME and Por=
t as
existing products. It extends the scope/rules of using feature IDs, for you=
r
new cards, so to me, this is not a sw bug but extension.

> it was mistaken for
> the "Port User Interrput" feature device while we are parsing the FME fea=
ture
> devices, so actually it
> read out the PMCI MMIO space for "Port User Interrput".
>=20
> Here is the log:
>=20
> [90273.482859] parse_feature: DFH_TYPE_FIU
> [90273.482861] parse_feature_fiu: id:0 fme
> [90273.482864] create_feature_instance: feature id: 0xfe
> [90273.482868] parse_feature: DFH_TYPE_PRIVATE
> [90273.482870] create_feature_instance: feature id: 0x1
> [90273.482872] parse_feature: DFH_TYPE_PRIVATE
> [90273.482874] create_feature_instance: feature id: 0x7
> ...
> [90273.482895] create_feature_instance: feature id: 0x12
> [90273.482898] parse_feature_irqs, PORT_UINT_DFH: 0x3000000200001012
> [90273.482898] parse_feature_irqs, PORT_UINT_CAP: 0xbaadbeefdeadbeef
> [90273.482899] dfl-pci 0000:b1:00.0: feature: 0x12, irq_base: 2779, nr_ir=
qs:
> 3823
> [90273.482901] dfl-pci 0000:b1:00.0: Ignoring nvalid interrupt number in =
feature
> 0x12 6602 > 7
>=20
> >
> > > Link: https://lore.kernel.org/linux-
> > >
> >
> fpga/BN9PR11MB54833D7636348D62F931526CE33A9@BN9PR11MB5483.nam
> > > prd11.prod.outlook.com/
> > >
> > > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> > > ---
> > >  Documentation/fpga/dfl.rst |  5 +++++
> > >  drivers/fpga/dfl.c         | 38 ++++++++++++++++++++++--------------=
--
> > >  2 files changed, 27 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> > > index ef9eec71f6f3..9ce418da1876 100644
> > > --- a/Documentation/fpga/dfl.rst
> > > +++ b/Documentation/fpga/dfl.rst
> > > @@ -502,6 +502,11 @@ Developer only needs to provide a sub feature
> > > driver with matched feature id.
> > >  FME Partial Reconfiguration Sub Feature driver (see
> > > drivers/fpga/dfl-fme-pr.c)  could be a reference.
> > >
> > > +Individual DFL drivers are bound DFL devices based on Feature Type
> > > +and
> > > Feature ID.
> > > +The definition of Feature Type and Feature ID can be found:
> > > +
> > > +https://github.com/OPAE/linux-dfl-feature-id/blob/master/dfl-feature=
-
> > > +ids.rst
> >
> > Thanks for tracking ID allocations. could we also add some description =
that if
> > user want to implement a new private feature, then they need to submit =
new
> ID
> > application to https://github.com/OPAE/linux-dfl-feature-id, and add so=
me
> > README file to guide people for the application process?
>=20
> That is a good point, I will add this for next version patch.
>=20
> >
> > Thanks
> > Hao
> >
> > > +
> > >  Location of DFLs on a PCI Device
> > >  =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > >  The original method for finding a DFL on a PCI device assumed the
> > > start of the diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > > index 599bb21d86af..6bff39ff21a0 100644
> > > --- a/drivers/fpga/dfl.c
> > > +++ b/drivers/fpga/dfl.c
> > > @@ -940,9 +940,12 @@ static int parse_feature_irqs(struct
> > > build_feature_devs_info *binfo,  {
> > >  	void __iomem *base =3D binfo->ioaddr + ofst;
> > >  	unsigned int i, ibase, inr =3D 0;
> > > +	enum dfl_id_type type;
> > >  	int virq;
> > >  	u64 v;
> > >
> > > +	type =3D feature_dev_id_type(binfo->feature_dev);
> > > +
> > >  	/*
> > >  	 * Ideally DFL framework should only read info from DFL header, but
> > >  	 * current version DFL only provides mmio resources information for
> > > @@ -957,22 +960,25 @@ static int parse_feature_irqs(struct
> > > build_feature_devs_info *binfo,
> > >  	 * code will be added. But in order to be compatible to old version
> > >  	 * DFL, the driver may still fall back to these quirks.
> > >  	 */
> > > -	switch (fid) {
> > > -	case PORT_FEATURE_ID_UINT:
> > > -		v =3D readq(base + PORT_UINT_CAP);
> > > -		ibase =3D FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> > > -		inr =3D FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> > > -		break;
> > > -	case PORT_FEATURE_ID_ERROR:
> > > -		v =3D readq(base + PORT_ERROR_CAP);
> > > -		ibase =3D FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> > > -		inr =3D FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> > > -		break;
> > > -	case FME_FEATURE_ID_GLOBAL_ERR:
> > > -		v =3D readq(base + FME_ERROR_CAP);
> > > -		ibase =3D FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> > > -		inr =3D FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
> > > -		break;
> > > +	if (type =3D=3D PORT_ID) {
> > > +		switch (fid) {
> > > +		case PORT_FEATURE_ID_UINT:
> > > +			v =3D readq(base + PORT_UINT_CAP);
> > > +			ibase =3D FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> > > +			inr =3D FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> > > +			break;
> > > +		case PORT_FEATURE_ID_ERROR:
> > > +			v =3D readq(base + PORT_ERROR_CAP);
> > > +			ibase =3D FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> > > +			inr =3D FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> > > +			break;
> > > +		}
> > > +	} else if (type =3D=3D FME_ID) {
> > > +		if (fid =3D=3D FME_FEATURE_ID_GLOBAL_ERR) {
> > > +			v =3D readq(base + FME_ERROR_CAP);
> > > +			ibase =3D FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> > > +			inr =3D FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
> > > +		}
> > >  	}
> > >
> > >  	if (!inr) {
> > > --
> > > 2.26.2