Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: aPKRTNkzpdCjPXOrhAbVlgG3cvMN6VawQVGUHectrcUNrRhGYgBdOtMa70s/zkFpW5kKRq8nwL
 R4Su1inFZ+Kw==
IronPort-SDR: PDykufNZvLUZkR4dbJh0qRLWVqC0Bc9MqKtsqidqocsa68XJIMtlpOVCUiNBshGayiHYu7iSUf
 s5muIJf8ECag==
From:   "Wu, Hao" <hao.wu@intel.com>
To:     "matthew.gerlach@linux.intel.com" <matthew.gerlach@linux.intel.com>
CC:     "linux-fpga@vger.kernel.org" <linux-fpga@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "mdf@kernel.org" <mdf@kernel.org>,
        "trix@redhat.com" <trix@redhat.com>,
        "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
        "corbet@lwn.net" <corbet@lwn.net>
Subject: RE: [PATCH 2/2] fpga: dfl: look for vendor specific capability
Thread-Topic: [PATCH 2/2] fpga: dfl: look for vendor specific capability
Thread-Index: AQHWvIBxykoMIcZrHUW2ir3zzcjKk6nL978wgADKdoCAAFpl0A==
Date:   Wed, 18 Nov 2020 01:54:31 +0000
Message-ID: <DM6PR11MB3819B7E06DB9A4BC7FBDABD285E10@DM6PR11MB3819.namprd11.prod.outlook.com>
References: <20201117012552.262149-1-matthew.gerlach@linux.intel.com>
 <20201117012552.262149-3-matthew.gerlach@linux.intel.com>
 <DM6PR11MB381979D8AEDC1F4EA8B4C2BC85E20@DM6PR11MB3819.namprd11.prod.outlook.com>
 <alpine.DEB.2.22.394.2011171144310.296481@rhweight-WRK1>
In-Reply-To: <alpine.DEB.2.22.394.2011171144310.296481@rhweight-WRK1>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-reaction: no-action
dlp-version: 11.5.1.3
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 4f033495-38f8-4a7d-9881-08d88b64e455
X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Nov 2020 01:54:31.5411
 (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: fwhJAvjBWE83fp54d6E9d2hLBG+PQ9FMKTClS5gv1r9dC2U4SRTCL03zpZgfpe1HNAn9Wj34Y5rIvtwJ8RJh8w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB2570
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> On Tue, 17 Nov 2020, Wu, Hao wrote:

[...]

> >>  Open discussion
> >>  =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> >> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> >> index b1b157b41942..5418e8bf2496 100644
> >> --- a/drivers/fpga/dfl-pci.c
> >> +++ b/drivers/fpga/dfl-pci.c
> >> @@ -27,6 +27,13 @@
> >>  #define DRV_VERSION	"0.8"
> >>  #define DRV_NAME	"dfl-pci"
> >>
> >> +#define PCI_VNDR_ID_DFLS 0x43
> >
> > What about PCI_VSEC_ID_INTEL_DFLS?
> >
> > Is it possible a different ID chosen by different vendor?
>=20
> I think another vendor could choose their own ID.

If another vendor could choose their own ID, so should we
check vendor id as well?

[...]

> >> +	for (i =3D 0; i < dfl_cnt; i++) {
> >> +		dfl_res_off =3D voff + PCI_VNDR_DFLS_RES_OFFSET +
> >> +				      (i * sizeof(dfl_res));
> >> +		pci_read_config_dword(pcidev, dfl_res_off, &dfl_res);
> >> +
> >> +		dev_dbg(&pcidev->dev, "dfl_res 0x%x\n", dfl_res);
> >> +
> >> +		bar =3D dfl_res & PCI_VND_DFLS_RES_BAR_MASK;
> >> +
> >> +		if (bar >=3D PCI_STD_NUM_BARS) {
> >> +			dev_err(&pcidev->dev, "%s bad bar number %d\n",
> >> +				__func__, bar);
> >> +			return -EINVAL;
> >> +		}
> >> +
> >> +		len =3D pci_resource_len(pcidev, bar);
> >> +
> >
> > Remove this blank line.
> OK, v2.
>=20
> >
> >> +		if (len =3D=3D 0) {
> >> +			dev_err(&pcidev->dev, "%s unmapped bar
> >> number %d\n",
> >
> > Why "unmapped bar"?
>=20
> How about, "zero length bar"?

I think this checking can be covered by below one, right?
if (offset >=3D len)
...

>=20
> >
> >> +				__func__, bar);
> >> +			return -EINVAL;
> >> +		}
> >> +
> >> +		offset =3D dfl_res & ~PCI_VND_DFLS_RES_BAR_MASK;
> >> +
> >
> > Remove this blank line.
>=20
> OK, v2.
>=20
> >
> >> +		if (offset >=3D len) {
> >> +			dev_err(&pcidev->dev, "%s bad offset %u >=3D %llu\n",
> >> +				__func__, offset, len);
> >> +			return -EINVAL;
> >> +		}
> >> +

[....]

> >> +
> >> +		start =3D pci_resource_start(pcidev, bar) + offset;
> >> +		len -=3D offset;
> >> +
> >> +		if (!PAGE_ALIGNED(start)) {
> >
> > Is this a hard requirement? Or offset should be page aligned per VSEC
> definition?
> > Or this is just the requirement from driver point of view. Actually we =
don't
> like
> > to add rules only in driver, so it's better we have this requirement in=
 VSEC
> definition
> > with proper documentation.
>=20
> The DFL parsing code ioremaps the memory bounded by start/len.  I thought
> this would require the start to be page aligned.

If consider mmap the region to userspace, it requires page aligned, but do =
we
need to apply this rule for everyone?

Thanks
Hao