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
