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

[Iremonger, Bernard]

> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Tetsuya Mukawa
> Sent: Wednesday, February 18, 2015 1:55 AM
> To: Thomas Monjalon
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v8 04/14] eal/pci: Consolidate pci address comparison APIs
> 
> On 2015/02/18 10:02, Thomas Monjalon wrote:
> > 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.

Hi Tetsuya, Thomas,
I think that redesigning the EAL structures is beyond the scope of this patchset and should be undertaken as a separate task.
I suspect there may be a problem in the original code when  a device which was using a kernel driver is bound to igb_uio.  The igb_uio  driver adds /sys/bus/pci/devices/0000\:05\:00.0/max_vfs.

Regards,

Bernard.

> >
> > [...]
> >>>> -		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.
> >
> 
> Sure I will add it.
> 
> Thanks,
> Tetsuya