From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pd0-f173.google.com (mail-pd0-f173.google.com [209.85.192.173]) by dpdk.org (Postfix) with ESMTP id 33586A6A for ; Tue, 24 Mar 2015 05:18:35 +0100 (CET) Received: by pdnc3 with SMTP id c3so208765304pdn.0 for ; Mon, 23 Mar 2015 21:18:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :cc:subject:references:in-reply-to:content-type :content-transfer-encoding; bh=MRTGhfgDHw5t0d5E+uVSNM51cCsh9x7KwCSwX3t/QmQ=; b=QFlZiK6Dz9/x3ghj52tJKNLBqdfzpxFESbtLDyjKsnAuLiRNGehv0VmczHLYRam8aj tDGlhrFkVYr/ef2i91HOSpxTq9JjyiEcHqJGsRZHisrOjl9Sj7VygoneFnjzmROCi8xw a92CIoLs2oogTd6OyA1AOeKtcJBAuKB6ESC91RINOmi4/nxlb7XbYZ7oVTIARA0OYEdk MZ/gl8g3iNbvIPYz22RatAyilHPx20WbR5Wj5p1D7EYtKpxfxEA8JSYgdwL8bF+9ZpTy mfujx0TYbYMNXKyzX4jtDVVphPdQKIw5DLmgFgCdvIpRtAQP4dH8/7LGv0NNteE3AQWO S6vA== X-Gm-Message-State: ALoCoQkZeLHf7AYOb497Mr0t1VuTkFQMeRezGpyn1yXmsjY0Ow3F1cgTkw9e3BffGksTImdL260O X-Received: by 10.68.97.65 with SMTP id dy1mr4408484pbb.10.1427170714354; Mon, 23 Mar 2015 21:18:34 -0700 (PDT) Received: from [10.16.129.101] (napt.igel.co.jp. [219.106.231.132]) by mx.google.com with ESMTPSA id fv12sm2571025pdb.34.2015.03.23.21.18.32 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 23 Mar 2015 21:18:33 -0700 (PDT) Message-ID: <5510E597.8020607@igel.co.jp> Date: Tue, 24 Mar 2015 13:18:31 +0900 From: Tetsuya Mukawa User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: "Iremonger, Bernard" , "dev@dpdk.org" References: <1426155474-1596-4-git-send-email-mukawa@igel.co.jp> <1426584645-28828-1-git-send-email-mukawa@igel.co.jp> <1426584645-28828-7-git-send-email-mukawa@igel.co.jp> <8CEF83825BEC744B83065625E567D7C2049F3E04@IRSMSX108.ger.corp.intel.com> In-Reply-To: <8CEF83825BEC744B83065625E567D7C2049F3E04@IRSMSX108.ger.corp.intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 6/6] eal: Fix interface of pci_map_resource() 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: Tue, 24 Mar 2015 04:18:35 -0000 On 2015/03/20 18:53, Iremonger, Bernard wrote: >> -----Original Message----- >> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp] >> Sent: Tuesday, March 17, 2015 9:31 AM >> To: dev@dpdk.org >> Cc: Iremonger, Bernard; Richardson, Bruce; Tetsuya Mukawa >> Subject: [PATCH 6/6] eal: Fix interface of pci_map_resource() >> >> The function is implemented in both linuxapp and bsdapp, but interface is different. The patch fixes >> the function of bsdapp to do same as linuxapp. After applying it, file descriptor should be opened and >> closed out of pci_map_resource(). >> Also, remove redundant error messages from linuxapp. >> >> Signed-off-by: Tetsuya Mukawa >> --- >> lib/librte_eal/bsdapp/eal/eal_pci.c | 109 ++++++++++++++++++------------ >> lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 21 +++--- >> 2 files changed, 75 insertions(+), 55 deletions(-) >> >> diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c b/lib/librte_eal/bsdapp/eal/eal_pci.c >> index 08b91b4..21a3d79 100644 >> --- a/lib/librte_eal/bsdapp/eal/eal_pci.c >> +++ b/lib/librte_eal/bsdapp/eal/eal_pci.c >> @@ -100,7 +100,7 @@ struct mapped_pci_resource { >> >> struct rte_pci_addr pci_addr; >> char path[PATH_MAX]; >> - size_t nb_maps; >> + int nb_maps; >> struct pci_map maps[PCI_MAX_RESOURCE]; }; >> >> @@ -122,47 +122,30 @@ pci_unbind_kernel_driver(struct rte_pci_device *dev __rte_unused) >> >> /* map a particular resource from a file */ static void * -pci_map_resource(void *requested_addr, >> const char *devname, off_t offset, >> - size_t size) >> +pci_map_resource(void *requested_addr, int fd, off_t offset, size_t size, >> + int additional_flags) >> { >> - int fd; >> void *mapaddr; >> >> - /* >> - * open devname, to mmap it >> - */ >> - fd = open(devname, O_RDWR); >> - if (fd < 0) { >> - RTE_LOG(ERR, EAL, "Cannot open %s: %s\n", >> - devname, strerror(errno)); >> - goto fail; >> - } >> - >> /* Map the PCI memory resource of device */ >> mapaddr = mmap(requested_addr, size, PROT_READ | PROT_WRITE, >> - MAP_SHARED, fd, offset); >> - close(fd); >> - if (mapaddr == MAP_FAILED || >> - (requested_addr != NULL && mapaddr != requested_addr)) { >> - RTE_LOG(ERR, EAL, "%s(): cannot mmap(%s(%d), %p, 0x%lx, 0x%lx):" >> - " %s (%p)\n", __func__, devname, fd, requested_addr, >> + MAP_SHARED | additional_flags, fd, offset); >> + if (mapaddr == MAP_FAILED) { >> + RTE_LOG(ERR, EAL, >> + "%s(): cannot mmap(%d, %p, 0x%lx, 0x%lx): %s (%p)\n", >> + __func__, fd, requested_addr, >> (unsigned long)size, (unsigned long)offset, >> strerror(errno), mapaddr); >> - goto fail; > Hi Tetsuya, > > Previously pci_map_resource() returned NULL when MAP_FAILED occurred. > It now returns MAP_FAILED. > Where pci_map_resource() is called it should now check for MAP_FAILED instead of NULL, or else the return value of NULL should be restored. > There is another comment below. Hi Bernard, I appreciate for your comment. Yes it should be. I will fix it. Regards, Tetsuya > > >> - } >> - >> - RTE_LOG(DEBUG, EAL, " PCI memory mapped at %p\n", mapaddr); >> + } else >> + RTE_LOG(DEBUG, EAL, " PCI memory mapped at %p\n", mapaddr); >> >> return mapaddr; >> - >> -fail: >> - return NULL; >> } >> >> static int >> pci_uio_map_secondary(struct rte_pci_device *dev) { >> - size_t i; >> + int i, fd; >> struct mapped_pci_resource *uio_res; >> struct mapped_pci_res_list *uio_res_list = >> RTE_TAILQ_CAST(rte_uio_tailq.head, mapped_pci_res_list); @@ -170,19 >> +153,34 @@ pci_uio_map_secondary(struct rte_pci_device *dev) >> TAILQ_FOREACH(uio_res, uio_res_list, next) { >> >> /* skip this element if it doesn't match our PCI address */ >> - if (memcmp(&uio_res->pci_addr, &dev->addr, sizeof(dev->addr))) >> + if (rte_eal_compare_pci_addr(&uio_res->pci_addr, &dev->addr)) >> continue; >> >> for (i = 0; i != uio_res->nb_maps; i++) { >> - if (pci_map_resource(uio_res->maps[i].addr, >> - uio_res->path, >> - (off_t)uio_res->maps[i].offset, >> - (size_t)uio_res->maps[i].size) >> - != uio_res->maps[i].addr) { >> + /* >> + * open devname, to mmap it >> + */ >> + fd = open(uio_res->maps[i].path, O_RDWR); >> + if (fd < 0) { >> + RTE_LOG(ERR, EAL, "Cannot open %s: %s\n", >> + uio_res->maps[i].path, strerror(errno)); >> + return -1; >> + } >> + >> + void *mapaddr = pci_map_resource(uio_res->maps[i].addr, >> + fd, (off_t)uio_res->maps[i].offset, >> + (size_t)uio_res->maps[i].size, 0); >> + if (mapaddr != uio_res->maps[i].addr) { >> RTE_LOG(ERR, EAL, >> - "Cannot mmap device resource\n"); >> + "Cannot mmap device resource " >> + "file %s to address: %p\n", >> + uio_res->maps[i].path, >> + uio_res->maps[i].addr); >> + close(fd); >> return -1; >> } >> + /* fd is not needed in slave process, close it */ >> + close(fd); >> } >> return 0; >> } >> @@ -248,24 +246,43 @@ pci_uio_map_resource(struct rte_pci_device *dev) >> >> maps = uio_res->maps; >> for (i = 0, map_idx = 0; i != PCI_MAX_RESOURCE; i++) { >> + int fd; >> >> /* skip empty BAR */ >> if ((phaddr = dev->mem_resource[i].phys_addr) == 0) >> continue; >> >> + /* allocate memory to keep path */ >> + maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0); >> + if (maps[map_idx].path == NULL) { >> + RTE_LOG(ERR, EAL, "Cannot allocate memory for " >> + "path: %s\n", strerror(errno)); >> + goto fail0; >> + } >> + >> + /* >> + * open resource file, to mmap it >> + */ >> + fd = open(devname, O_RDWR); >> + if (fd < 0) { >> + RTE_LOG(ERR, EAL, "Cannot open %s: %s\n", >> + devname, strerror(errno)); >> + goto fail1; >> + } >> + >> /* if matching map is found, then use it */ >> offset = i * pagesz; >> - maps[map_idx].offset = offset; >> + mapaddr = pci_map_resource(NULL, fd, (off_t)offset, >> + (size_t)dev->mem_resource[i].len, 0); >> + close(fd); >> + if (mapaddr == NULL) > The check here should be for MAP_FAILED instead of NULL. > > Regards, > Bernard. > > >> + goto fail1; >> + >> maps[map_idx].phaddr = dev->mem_resource[i].phys_addr; >> maps[map_idx].size = dev->mem_resource[i].len; >> - mapaddr = pci_map_resource(NULL, devname, (off_t)offset, >> - (size_t)maps[map_idx].size); >> - if ((maps[map_idx].addr != NULL) || (mapaddr == NULL)) { >> - rte_free(uio_res); >> - return -1; >> - } >> - >> maps[map_idx].addr = mapaddr; >> + maps[map_idx].offset = offset; >> + strcpy(maps[map_idx].path, devname); >> map_idx++; >> dev->mem_resource[i].addr = mapaddr; >> } >> @@ -275,6 +292,12 @@ pci_uio_map_resource(struct rte_pci_device *dev) >> TAILQ_INSERT_TAIL(uio_res_list, uio_res, next); >> >> return 0; >> + >> +fail1: >> + rte_free(maps[map_idx].path); >> +fail0: >> + rte_free(uio_res); >> + return -1; >> } >> >> /* Scan one pci sysfs entry, and fill the devices list from it. */ diff --git >> a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c >> index 5044884..9eeaebb 100644 >> --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c >> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c >> @@ -116,17 +116,11 @@ pci_uio_map_secondary(struct rte_pci_device *dev) >> fd, (off_t)uio_res->maps[i].offset, >> (size_t)uio_res->maps[i].size, 0); >> if (mapaddr != uio_res->maps[i].addr) { >> - if (mapaddr == MAP_FAILED) >> - RTE_LOG(ERR, EAL, >> - "Cannot mmap device resource file %s: %s\n", >> - uio_res->maps[i].path, >> - strerror(errno)); >> - else >> - RTE_LOG(ERR, EAL, >> - "Cannot mmap device resource file %s to >> address: %p\n", >> - uio_res->maps[i].path, >> - uio_res->maps[i].addr); >> - >> + RTE_LOG(ERR, EAL, >> + "Cannot mmap device resource " >> + "file %s to address: %p\n", >> + uio_res->maps[i].path, >> + uio_res->maps[i].addr); >> close(fd); >> return -1; >> } >> @@ -348,8 +342,11 @@ pci_uio_map_resource(struct rte_pci_device *dev) >> >> /* allocate memory to keep path */ >> maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0); >> - if (maps[map_idx].path == NULL) >> + if (maps[map_idx].path == NULL) { >> + RTE_LOG(ERR, EAL, "Cannot allocate memory for " >> + "path: %s\n", strerror(errno)); >> goto fail0; >> + } >> >> /* >> * open resource file, to mmap it >> -- >> 1.9.1