[dpdk-dev,v2,6/6] eal: Fix interface of pci_map_resource()
Commit Message
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 <mukawa@igel.co.jp>
---
lib/librte_eal/bsdapp/eal/eal_pci.c | 111 ++++++++++++++++++------------
lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 21 +++---
2 files changed, 77 insertions(+), 55 deletions(-)
Comments
> -----Original Message-----
> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> Sent: Tuesday, March 24, 2015 4:19 AM
> To: dev@dpdk.org
> Cc: Iremonger, Bernard; Richardson, Bruce; david.marchand@6wind.com; Tetsuya Mukawa
> Subject: [PATCH v2 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 <mukawa@igel.co.jp>
> ---
> lib/librte_eal/bsdapp/eal/eal_pci.c | 111 ++++++++++++++++++------------
> lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 21 +++---
> 2 files changed, 77 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..d83916b 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;
> - }
> -
> - 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 == MAP_FAILED)
> + 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,14 @@ 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:
> + for (i = 0; i < map_idx; i++)
Hi Tetsuya,
fail1: falls through to fail0:
Would it be cleaner to drop fail1: and change the for loop in fail0: to
for (i = 0; i <= map_idx; i++)
Regards,
Bernard.
> + rte_free(maps[i].path);
> + 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 0128cec..1f94d3a 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
@@ -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;
- }
-
- 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 == MAP_FAILED)
+ 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,14 @@ 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:
+ for (i = 0; i < map_idx; i++)
+ rte_free(maps[i].path);
+ rte_free(uio_res);
+ return -1;
}
/* Scan one pci sysfs entry, and fill the devices list from it. */
@@ -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