[dpdk-dev] [PATCH v8 04/14] eal/pci: Consolidate pci address comparison APIs
Thomas Monjalon
thomas.monjalon at 6wind.com
Wed Feb 18 02:02:23 CET 2015
2015-02-17 15:14, Tetsuya Mukawa:
> On 2015/02/17 9:44, Thomas Monjalon wrote:
> > 2015-02-16 13:14, Tetsuya Mukawa:
> >> @@ -356,13 +342,24 @@ pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
> >> }
> >> else {
> >> struct rte_pci_device *dev2 = NULL;
> >> + int ret;
> >>
> >> TAILQ_FOREACH(dev2, &pci_device_list, next) {
> >> - if (pci_addr_comparison(&dev->addr, &dev2->addr))
> >> + ret = eal_compare_pci_addr(&dev->addr, &dev2->addr);
> >> + if (ret > 0)
> >> continue;
> >> - else {
> >> + else if (ret < 0) {
> >> TAILQ_INSERT_BEFORE(dev2, dev, next);
> >> return 0;
> >> + } else { /* already registered */
> >> + /* update pt_driver */
> >> + dev2->pt_driver = dev->pt_driver;
> >> + dev2->max_vfs = dev->max_vfs;
> >> + memmove(dev2->mem_resource,
> >> + dev->mem_resource,
> >> + sizeof(dev->mem_resource));
> >> + free(dev);
> >> + return 0;
> > Could you comment this "else part" please?
>
> PCI device list is allocated when rte_eal_init() is called. At the time,
> to fill pci device information, sysfs value is used.
> If sysfs values written by kernel device driver will not be changed by
> igb_uio, vfio or pci_uio_genereic, above code isn't needed.
> But actually above values are changed or added by them.
>
> Here is a step to cause issue.
> 1. Boot linux.
> 2. Start DPDK application without any physical NIC ports.
> - Here, some sysfs values are read, and store to pci device list.
> 3. igb_uio starts managing a port.
> - Here, some sysfs values are changed.
> 4. Add a NIC port to DPDK application using hotplug functions.
> - Here, we need to replace some values.
I think that you are showing that something is wrongly designed in these
EAL structures. I suggest to try cleaning this mess instead of workarounding.
[...]
> >> - if (memcmp(&uio_res->pci_addr, &dev->addr, sizeof(dev->addr)))
> >> + if (eal_compare_pci_addr(&uio_res->pci_addr, &dev->addr))
> > Why memcmp is not sufficient to compare PCI addresses?
> > The only exception I see is endianness for natural sorting.
>
> Here is the definition.
>
> struct rte_pci_addr {
> uint16_t domain; /**< Device domain */
> uint8_t bus; /**< Device bus */
> uint8_t devid; /**< Device ID */
> uint8_t function; /**< Device function. */
> };
>
> But, sizeof(struct rte_pci_addr) will be 6.
> If rte_pci_addr is allocated in a function without bzero, last 1 byte
> may have some value.
> As a result, memcmp may not work. To avoid such a case, I checked like
> above.
OK thanks. That's the kind of information which is valuable in a commit log.
More information about the dev
mailing list