Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
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 v3] fpga: dfl: check feature type before parse irq info
Thread-Topic: [PATCH v3] fpga: dfl: check feature type before parse irq info
Thread-Index: AQHYNO3MXtNRF3jnJE2Dc78P5iqqW6zCxhFA
Date:   Thu, 17 Mar 2022 01:47:45 +0000
Message-ID: <BYAPR11MB3816A2AD9C59AC3A1B6494C485129@BYAPR11MB3816.namprd11.prod.outlook.com>
References: <20220311021121.1504442-1-tianfei.zhang@intel.com>
In-Reply-To: <20220311021121.1504442-1-tianfei.zhang@intel.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?yyc7+rByeDKhuUgIrC82ahu/7FScB4c5z1n2uqW4yU9m+F+FOyPRcQHhhYzL?=
 =?us-ascii?Q?M2R6gfqEKbaNXmPfbI15kGnrD16cV4UHObH+W3Li0AYqipRiFGsF3GskytqJ?=
 =?us-ascii?Q?2F2TWn9grT+XZxzoVVE85o05r+ADbvgMqvCLKNBwL4MvFjscf6qjI3WGo0br?=
 =?us-ascii?Q?klOU7MBhqcI28w94+U0nYfs9i7be8fW+yyeFnR9x+WkiGPrY40GmeP20QmVv?=
 =?us-ascii?Q?1XGINZd/bP2WQ90yxA3/PK6lcX5XjpjSi90xoa6MECs97NbbW2zGGnReRa/d?=
 =?us-ascii?Q?V0ovXWu/44BWKl9ey5mp6I1cvz1dnZdLY1DTM2qZO7vNvMcuQ99ZcapkNfsT?=
 =?us-ascii?Q?Ik/8nH7HIGOIHa9D5TvJnT6jadOdzWWwb+wP8R2UVkvxOj9sxvViPx/Vxq8V?=
 =?us-ascii?Q?haddM18MszY1KugKprJcwRrONnF5A4nCvciQ+yjBWkiQXqovYWv8H8ptcDfm?=
 =?us-ascii?Q?Qf/HGvi6RIhAjYfpt5Fh/vzuipirGU1Hx/sA1HurOSjgekRm8eYeeMiZ1a8N?=
 =?us-ascii?Q?DURmeUbh9BqwApUWPPm+vK5ve6unOO0jVFOyLDsxnacWbvVWVMGVYleaxg+W?=
 =?us-ascii?Q?jXDelrJ7ds+Zm/Ics4eYvrrTYdQXPXx1dCbmhAOdhe4TEBimeeUt4zJAll0q?=
 =?us-ascii?Q?E06ia8a2lAlcfFaEG5c64ld6bPvxwTkgu8mQeL7r0dZvxbCrDpldKEnscEVQ?=
 =?us-ascii?Q?ZuvikmIMUTCViDb93Te3V/ArI0vTdu3nqt++2uJiUWEv/4YeI10VIQYVK9Yp?=
 =?us-ascii?Q?oAbKIkwB2Cm/Ta+GeEbXo2rF8VBjSezwJaIaYB5InWH8l/8V2hy7V8RU2mfc?=
 =?us-ascii?Q?IMvdpJufVCm5Eu9nCAChUaNOdvoai4twjOKd33ZZ+T7PEiywKuJVKArx96W3?=
 =?us-ascii?Q?U8j4I8Mm+YGmtVWMW6rDU7G5Mtkec6sO8GcgvB649rs8uUDgSjy+PZan8YNq?=
 =?us-ascii?Q?P6sBPFkC5CuTdWtPmugtrICc4bLpJRS470AkQZTriaUKQhIbJa3ku5ksii91?=
 =?us-ascii?Q?WEJQCT7ZJxXmo2rO90r0OixMDs3iocnDUHHwYv4U2dRsx+h1xi3uIjMmR9QJ?=
 =?us-ascii?Q?XkO50s336sKVnI9/eLXdhRoIb05Qk8Z+gY2hUyJV1aw4G1hCiO0hjDXl4Jut?=
 =?us-ascii?Q?K4Lp2ZKzOkTxqmLBQrvrJJDqRSYcuiZ9gQ7EwJvrWCuOP1LGJI96Q27duTy4?=
 =?us-ascii?Q?3wdlhj+xVpXETizrPfeXUexrhXaXTut9arUon0E9yd5kMJ8xdUC3ucPhikET?=
 =?us-ascii?Q?wS1m9lnctfilkRkB7wVj+MpKyr/OsTh+Vcic96H2t5l8inBXRWKNzgoWs6Kn?=
 =?us-ascii?Q?4o5mmQvM+fAdNG1t3yl97E9Io/iAE682zHynkcXQCDSzR0mElk8nwHTZv/cs?=
 =?us-ascii?Q?o7IZ9GLVN3QqE/o4U8NKHGg8ehoJsojpRYzo6iBxkHQT+yG+jbZn9ofUhDVu?=
 =?us-ascii?Q?E7KDO8FVIlw=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: BYAPR11MB3816.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 95760b3d-706e-45bf-1b84-08da07b821fa
X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Mar 2022 01:47:45.0981
 (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: Wwhf5mH/i+ftcyDsfb/IlDiCP8Xqrou6yUfe2obyziTkzIq3DIEnsMxzZ+5mkSmvONfM2ojHBJOgVk9OEMMXEA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB2586
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-3.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,RDNS_NONE,SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable 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: Zhang, Tianfei <tianfei.zhang@intel.com>
> Sent: Friday, March 11, 2022 10:11 AM
> To: Wu, Hao <hao.wu@intel.com>; trix@redhat.com; mdf@kernel.org; Xu, Yilu=
n
> <yilun.xu@intel.com>; linux-fpga@vger.kernel.org; linux-doc@vger.kernel.o=
rg;
> linux-kernel@vger.kernel.org
> Cc: corbet@lwn.net; Zhang, Tianfei <tianfei.zhang@intel.com>
> Subject: [PATCH v3] fpga: dfl: check feature type before parse irq info
>=20
> 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().
>=20

What about this
"Previously the feature IDs defined are unique, no matter which
feature type. But currently we want to extend its usage to have
a per-type feature ID space, so this patch adds feature type
checking as well just before look into feature ID for different
features which have irq info."

> ---
> v3: Remove "Fixes" in commit log with Hao's comment, this is a
>     extension not a bug fix.
>=20
> v2: add DFL Feature ID Registry in documentation.
>=20
> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>

Move up your signed off line before these changelogs.=20

> ---
>  Documentation/fpga/dfl.rst | 11 +++++++++++
>  drivers/fpga/dfl.c         | 38 ++++++++++++++++++++++----------------
>  2 files changed, 33 insertions(+), 16 deletions(-)
>=20
> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> index ef9eec71f6f3..14f342bb85e4 100644
> --- a/Documentation/fpga/dfl.rst
> +++ b/Documentation/fpga/dfl.rst
> @@ -502,6 +502,17 @@ 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.
>=20
> +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
> +
> +If you want to add a new feature ID for FPGA DFL feature device, we
> recommend that

"recommend" or "must"

> +use a pull request to reserve a feature ID for DFL. Here is the DFL Feat=
ure ID
> +Registry:
> +
> +https://github.com/OPAE/linux-dfl-feature-id
> +

Actually we don't have to put all details in kernel doc, and feature ID is =
used
inside the device, it could be used in linux or windows or other OS.
What about putting a README file in your repo to describe the detailed proc=
ess,
not in kernel doc. And we can just put a link to that README here.
Then split this one into another patch? How do you think?

>  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;
>=20
> +	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);
> +		}
>  	}
>=20
>  	if (!inr) {
> --
> 2.26.2