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