From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ob0-f181.google.com (mail-ob0-f181.google.com [209.85.214.181]) by dpdk.org (Postfix) with ESMTP id 884EE5A52 for ; Tue, 7 Jul 2015 10:04:56 +0200 (CEST) Received: by obbop1 with SMTP id op1so123465794obb.2 for ; Tue, 07 Jul 2015 01:04:56 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=MmMWJBoO8HXOOzPkuiaKVwqDAxWsZLdoHjBJk/eGlLY=; b=DEMjxMOviLxh/tB682w6Vsk9Xj0Ek6X/eR9d5MDn6EjpDNajJju9eoTuNhWdg5G/9j gGKOLp8rkcF14FcPbDMyqFx9og0Uxk94y6HN/seZVEtXil3WcOXxY5/YCpNK06nmpIyc 6WGkZ6ZLOP2puiK2Lj46t62tEQYp0lrbHDV1FN0O0MgYkizw5TJVGYwXkyiZwUzL4UGl eO64PngDYR6IbS4qWU4QoPmKe/XpUXiiVAXKplE9beCyLO0td29Cfd08xwDBZawa+PR3 hyJAofPRTQbutQjVkFkcss40TjG8E445aOvpbebQhKsxf1e3jbhYj3NIxBU3xx82NS8/ 9fOg== X-Gm-Message-State: ALoCoQnrVoMXH5Nc+4zwyYpH10bLEpZEe65n3YbOHUqu5ZSn/Q/qfzSWtk+fhJXtRfwB5apUyr8q MIME-Version: 1.0 X-Received: by 10.202.202.129 with SMTP id a123mr2758224oig.118.1436256296035; Tue, 07 Jul 2015 01:04:56 -0700 (PDT) Received: by 10.76.84.233 with HTTP; Tue, 7 Jul 2015 01:04:55 -0700 (PDT) In-Reply-To: <1436163861-3025-7-git-send-email-mukawa@igel.co.jp> 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> Date: Tue, 7 Jul 2015 10:04:55 +0200 Message-ID: From: David Marchand To: Tetsuya Mukawa Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 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: Tue, 07 Jul 2015 08:04:57 -0000 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. > - /* 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. > /* 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. > + 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. -- David Marchand