Hi David, Thank you for reviewing this.
On 9/9/2016 5:06 PM, David Marchand wrote: > Hello Jianfeng, > > On Thu, Sep 1, 2016 at 4:16 AM, Jianfeng Tan <jianfeng.tan at intel.com> > wrote: >> Previously in igb_uio, iomem is mapped, and both ioport and io mem >> are recorded into uio framework, which is duplicated and makes the >> code too complex. >> >> For iomem, DPDK user space code never opens or reads files under >> /sys/pci/bus/devices/xxxx:xx:xx.x/uio/uioY/maps/. Instead, >> /sys/pci/bus/devices/xxxx:xx:xx.x/resourceY are used to map device >> memory. >> >> For ioport, non-x86 platforms cannot read from files under >> /sys/pci/bus/devices/xxxx:xx:xx.x/uio/uioY/portio/ directly, because >> non-x86 platforms need to map port region for access in user space, >> see non-x86 version pci_uio_ioport_map(). x86 platforms can use the >> the same way as uio_pci_generic. >> >> This patch deprecates iomem and ioport mapping in igb_uio kernel >> module, and adjusts the iomem implementation in both igb_uio and >> uio_pci_generic: >> - for x86 platform, get ports info from /proc/ioports; >> - for non-x86 platform, map and get ports info by pci_uio_ioport_map(). >> >> Note: this will affect those applications who are using files under >> /sys/pci/bus/devices/xxxx:xx:xx.x/uio/uioY/maps/ and >> /sys/pci/bus/devices/xxxx:xx:xx.x/uio/uioY/portio/. >> >> Signed-off-by: Jianfeng Tan <jianfeng.tan at intel.com> >> --- >> lib/librte_eal/linuxapp/eal/eal_pci.c | 4 - >> lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 56 +------------- >> lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 119 >> ++---------------------------- >> 3 files changed, 9 insertions(+), 170 deletions(-) > [snip] > >> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c >> b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c >> index 1786b75..28d09ed 100644 >> --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c >> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c > [snip] > >> - /* FIXME only for primary process ? */ >> - if (dev->intr_handle.type == RTE_INTR_HANDLE_UNKNOWN) { >> - >> - snprintf(filename, sizeof(filename), "/dev/uio%u", uio_num); >> - dev->intr_handle.fd = open(filename, O_RDWR); >> - if (dev->intr_handle.fd < 0) { >> - RTE_LOG(ERR, EAL, "Cannot open %s: %s\n", >> - filename, strerror(errno)); >> - return -1; >> - } >> - dev->intr_handle.type = RTE_INTR_HANDLE_UIO; >> - } > The only potential issue I can see removing this is that if a device > has no iomem resource, then its interrupt fd will never be > initialised. I'm catching it completely. IMO, dev->intr_handle.fd will be bound to be initialized in pci_uio_alloc_resource() <- pci_uio_map_resource() <- rte_eal_pci_map_device() after this patch, just like what we've done with uio-pci-generic. Or I'm missing something? > I can see no problem at the moment, so let's go with this. > If such a problem arises later, we can separate this from the resource > mapping stuff (with a new api ?). > The rest looks good to me. > > As Ferruh said, this should go through deprecation notices then go in 17.02. > Yes, no problem. Thanks, Jianfeng