From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f46.google.com (mail-pa0-f46.google.com [209.85.220.46]) by dpdk.org (Postfix) with ESMTP id 7B9125A72 for ; Wed, 8 Jul 2015 04:42:44 +0200 (CEST) Received: by pacws9 with SMTP id ws9so124489709pac.0 for ; Tue, 07 Jul 2015 19:42:43 -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=BK9q0XGDlVTBhoJJvgUov++wput0MzzxkYJWt+VWZp8=; b=O845WavABPs65pugdSp/AvGYF7xSmn49CqXIRpG6ghhzPoYs0gEHWXYwo2/VqJ+vNN BdzulQABQkqAHmNoEVFDtmeV4RNsjW0qSgMXBubi0tKVa6vqNqFwsuYa0qDVJNjHV6Fs HFSWPJoejM6ha3nQ4VfhWHN0gpagcEJ1a57AzL1DWOVA0aVunDDFG9raksFoNc1eo6/T 3ghDt3rWPNl+b07Ms5Uswum6ATNHVrV3teQGC010jH3518FaeNTHsD6p4K8IaAfaIEyp vNcDj40xomtDMYK2uPF/SOQqyW5qiLDspuk9RtUbGEg+tXCO3nYp+bPcG/KquY6x5ykc JuTw== X-Gm-Message-State: ALoCoQlWtUibJtgeJ07u+TUxDVlKamlgkA71SR81Ae+0g8yTB1/Rg3mZhEsHxyOyjv27DVIx6LSG X-Received: by 10.70.131.4 with SMTP id oi4mr15186179pdb.95.1436323363549; Tue, 07 Jul 2015 19:42:43 -0700 (PDT) Received: from [10.16.129.101] (napt.igel.co.jp. [219.106.231.132]) by smtp.googlemail.com with ESMTPSA id x3sm192060pdk.61.2015.07.07.19.42.41 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 07 Jul 2015 19:42:42 -0700 (PDT) Message-ID: <559C8E20.1010401@igel.co.jp> Date: Wed, 08 Jul 2015 11:42:40 +0900 From: Tetsuya Mukawa User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: David Marchand References: <1435652668-3380-12-git-send-email-mukawa@igel.co.jp> <1436163861-3025-1-git-send-email-mukawa@igel.co.jp> <1436163861-3025-7-git-send-email-mukawa@igel.co.jp> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v8 06/12] eal: Add pci_uio_alloc_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: Wed, 08 Jul 2015 02:42:45 -0000 On 2015/07/07 17:04, David Marchand wrote: > On Mon, Jul 6, 2015 at 8:24 AM, Tetsuya Mukawa > wrote: > > From: "Tetsuya.Mukawa" > > > This patch adds a new function called pci_uio_alloc_resource(). > The function hides how to prepare uio resource in linuxapp and bsdapp. > With the function, pci_uio_map_resource() will be more abstracted. > > Signed-off-by: Tetsuya Mukawa > > --- > lib/librte_eal/bsdapp/eal/eal_pci.c | 84 > +++++++++++++++++--------- > lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 98 > +++++++++++++++++++++---------- > 2 files changed, 123 insertions(+), 59 deletions(-) > > diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c > b/lib/librte_eal/bsdapp/eal/eal_pci.c > index 92d9886..ce0ca07 100644 > --- a/lib/librte_eal/bsdapp/eal/eal_pci.c > +++ b/lib/librte_eal/bsdapp/eal/eal_pci.c > @@ -189,28 +189,17 @@ pci_uio_map_secondary(struct rte_pci_device > *dev) > return 1; > } > > -/* map the PCI resource of a PCI device in virtual memory */ > static int > -pci_uio_map_resource(struct rte_pci_device *dev) > +pci_uio_alloc_resource(struct rte_pci_device *dev, > + struct mapped_pci_resource **uio_res) > { > - int i, map_idx = 0; > char devname[PATH_MAX]; /* contains the /dev/uioX */ > - void *mapaddr; > - uint64_t phaddr; > - uint64_t offset; > - uint64_t pagesz; > - struct rte_pci_addr *loc = &dev->addr; > - struct mapped_pci_resource *uio_res = NULL; > - struct mapped_pci_res_list *uio_res_list = > - RTE_TAILQ_CAST(rte_uio_tailq.head, > mapped_pci_res_list); > - struct pci_map *maps; > + struct rte_pci_addr *loc; > > - dev->intr_handle.fd = -1; > - dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN; > + if ((dev == NULL) || (uio_res == NULL)) > + return -1; > > > This is an "internal" function, dev cannot be null, neither uio_res. I will fix it. > > > - /* secondary processes - use already recorded details */ > - if (rte_eal_process_type() != RTE_PROC_PRIMARY) > - return pci_uio_map_secondary(dev); > + loc = &dev->addr; > > snprintf(devname, sizeof(devname), "/dev/uio@pci:%u:%u:%u", > dev->addr.bus, dev->addr.devid, > dev->addr.function); > @@ -231,18 +220,57 @@ pci_uio_map_resource(struct rte_pci_device *dev) > dev->intr_handle.type = RTE_INTR_HANDLE_UIO; > > /* allocate the mapping details for secondary processes*/ > - if ((uio_res = rte_zmalloc("UIO_RES", sizeof (*uio_res), > 0)) == NULL) { > + *uio_res = rte_zmalloc("UIO_RES", sizeof(**uio_res), 0); > + if (*uio_res == NULL) { > RTE_LOG(ERR, EAL, > "%s(): cannot store uio mmap details\n", > __func__); > goto error; > } > > - snprintf(uio_res->path, sizeof(uio_res->path), "%s", devname); > - memcpy(&uio_res->pci_addr, &dev->addr, > sizeof(uio_res->pci_addr)); > + snprintf((*uio_res)->path, sizeof((*uio_res)->path), "%s", > devname); > + memcpy(&(*uio_res)->pci_addr, &dev->addr, > sizeof((*uio_res)->pci_addr)); > > + return 0; > + > +error: > + if (dev->intr_handle.fd) { > + close(dev->intr_handle.fd); > + dev->intr_handle.fd = -1; > + dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN; > + } > + return -1; > +} > + > +/* map the PCI resource of a PCI device in virtual memory */ > +static int > +pci_uio_map_resource(struct rte_pci_device *dev) > +{ > + int i, map_idx = 0, ret; > + char *devname; > + void *mapaddr; > + uint64_t phaddr; > + uint64_t offset; > + uint64_t pagesz; > + struct mapped_pci_resource *uio_res = NULL; > + struct mapped_pci_res_list *uio_res_list = > + RTE_TAILQ_CAST(rte_uio_tailq.head, > mapped_pci_res_list); > + struct pci_map *maps; > + > + dev->intr_handle.fd = -1; > + dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN; > + > + /* secondary processes - use already recorded details */ > + if (rte_eal_process_type() != RTE_PROC_PRIMARY) > + return pci_uio_map_secondary(dev); > + > + /* allocate uio resource */ > + ret = pci_uio_alloc_resource(dev, &uio_res); > + if ((ret != 0) || (uio_res == NULL)) > + return ret; > > > If ret != 0, uio_res cannot be NULL. Also I will fix it. > > > > /* Map all BARs */ > pagesz = sysconf(_SC_PAGESIZE); > + devname = uio_res->path; > > maps = uio_res->maps; > for (i = 0; i != PCI_MAX_RESOURCE; i++) { > @@ -298,13 +326,15 @@ pci_uio_map_resource(struct rte_pci_device *dev) > error: > for (i = 0; i < map_idx; i++) > rte_free(maps[i].path); > - if (uio_res) > - rte_free(uio_res); > - if (dev->intr_handle.fd >= 0) { > - close(dev->intr_handle.fd); > - dev->intr_handle.fd = -1; > - dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN; > - } > + > + /* 'uio_res' has valid value here */ > > > Not sure what this comment means, do you mean != NULL ? It does not > matter. I thought rte_free() fails when NULL is passed. Because of above, I wanted to mention here we don't need to check NULL because uio_res will have non-NULL value. But, as you mentioned in other patch, we don't need to check it. Anyway, I will remove the comment. > > > + rte_free(uio_res); > + > + /* 'fd' has valid value here */ > + close(dev->intr_handle.fd); > + dev->intr_handle.fd = -1; > + dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN; > + > return -1; > } > > > In the end, if you add this "alloc" function, why not introduce a > "free" function that does this cleanup ? > > > Same comments apply to linux modifications. > Sure, I will introduce it. Tetsuya