From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 9976D58C5 for ; Fri, 9 Sep 2016 11:31:40 +0200 (CEST) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga102.fm.intel.com with ESMTP; 09 Sep 2016 02:31:39 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,304,1470726000"; d="scan'208";a="6764322" Received: from shwdeisgchi083.ccr.corp.intel.com (HELO [10.239.67.193]) ([10.239.67.193]) by orsmga004.jf.intel.com with ESMTP; 09 Sep 2016 02:31:38 -0700 To: David Marchand References: <1472696197-37614-1-git-send-email-jianfeng.tan@intel.com> Cc: "dev@dpdk.org" , Ferruh Yigit , Stephen Hemminger From: "Tan, Jianfeng" Message-ID: Date: Fri, 9 Sep 2016 17:31:37 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [RFC] igb_uio: deprecate iomem and ioport mapping X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 09 Sep 2016 09:31:41 -0000 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 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 >> --- >> 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