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

[Thomas Monjalon <thomas.monjalon@6wind.com>]

Hi Bernard,

2015-02-18 10:26, Iremonger, Bernard:
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tetsuya Mukawa
> > 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 strongly disagrees this opinion. We should never workaround design problems
and add more complex/weird code.
I think that this kind of consideration is the heart of some design problems we
have to face today. Please let's stop adding some code which just works without
thinking the whole design.

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