> -----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
