Messages in this thread | | | Date | Wed, 18 Nov 2020 09:49:55 -0800 (PST) | From | matthew.gerlach@linux ... | Subject | RE: [PATCH 2/2] fpga: dfl: look for vendor specific capability |
| |
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 > >
| |