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

[matthew . gerlach]

On Wed, 18 Nov 2020, Wu, Hao wrote:

> > 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?

Yes, the vendor id should be checked.  I will add it to v2.
> [...]
>
> > > > +	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)
> ...

I agree. I will make the change in v2.
> > > > +				__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?

I will remove this check in v2.

> Thanks
> Hao