RE: [PATCH 2/2] fpga: dfl: look for vendor specific capability

[Wu, Hao]

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

[...]

> >>  Open discussion
> >>  ===============
> >> 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?
> 
> 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 = 0; i < dfl_cnt; i++) {
> >> +		dfl_res_off = 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 = dfl_res & PCI_VND_DFLS_RES_BAR_MASK;
> >> +
> >> +		if (bar >= PCI_STD_NUM_BARS) {
> >> +			dev_err(&pcidev->dev, "%s bad bar number %d\n",
> >> +				__func__, bar);
> >> +			return -EINVAL;
> >> +		}
> >> +
> >> +		len = pci_resource_len(pcidev, bar);
> >> +
> >
> > Remove this blank line.
> OK, v2.
> 
> >
> >> +		if (len == 0) {
> >> +			dev_err(&pcidev->dev, "%s unmapped bar
> >> number %d\n",
> >
> > Why "unmapped bar"?
> 
> How about, "zero length bar"?

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

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

[....]

> >> +
> >> +		start = pci_resource_start(pcidev, bar) + offset;
> >> +		len -= 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.
> 
> 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