[dpdk-dev] [PATCH v8 04/14] eal/pci: Consolidate pci address comparison APIs

[Thomas Monjalon]

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.