diff mbox

[dpdk-dev,6/6] eal: Fix interface of pci_map_resource()

Message ID 1426584645-28828-7-git-send-email-mukawa@igel.co.jp (mailing list archive)
State Changes Requested, archived
Headers

Commit Message

Tetsuya Mukawa March 17, 2015, 9:30 a.m. UTC 
  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       | 109 ++++++++++++++++++------------
 lib/librte_eal/linuxapp/eal/eal_pci_uio.c |  21 +++---
 2 files changed, 75 insertions(+), 55 deletions(-)

Comments

Iremonger, Bernard March 20, 2015, 9:53 a.m. UTC | #1 
> -----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 <mukawa@igel.co.jp>
> ---
>  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.


> -	}
> -
> -	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
Tetsuya Mukawa March 24, 2015, 4:18 a.m. UTC | #2 
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 <mukawa@igel.co.jp>
>> ---
>>  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
Tetsuya Mukawa March 24, 2015, 4:18 a.m. UTC | #3 
This patch set cleans up pci uio implementation. These clean up are
for consolidating pci uio implementation of linuxapp and bsdapp, and
moving consolidated functions in eal common.
Because of above, this patch set tries to implement linuxapp and bsdapp
almost same.
Actual consolidations will be done later patch set.

PATCH v2 changes:
 - Move 'if-condition' to later patch series.
 - Fix memory leaks of path.
 - Fix typos.
   (Thanks to David Marchand)
 - Fix commit title and body.
 - Fix pci_map_resource() to handle MAP_FAILED.
   (Thanks to Iremonger, Bernard)

Changes:
 - This patch set is derived from below.
   "[PATCH v2] eal: Port Hotplug support for BSD"
 - Set cfg_fd as -1, when cfg_fd is closed.
   (Thanks to Iremonger, Bernard)
 - Remove needless coding style fixings.
 - Fix coding style of if-else condition.
   (Thanks to Richardson, Bruce)


Tetsuya Mukawa (6):
  eal: Fix coding style of eal_pci.c and eal_pci_uio.c
  eal: Close file descriptor of uio configuration
  eal: Fix memory leaks and needless increment of pci_map_addr
  eal/bsdapp: Change names of pci related data structure
  eal: Use map_idx in pci_uio_map_resource() of bsdapp to work same as
    linuxapp
  eal: Fix interface of pci_map_resource()

 lib/librte_eal/bsdapp/eal/eal_pci.c       | 162 ++++++++++++++++++------------
 lib/librte_eal/linuxapp/eal/eal_pci_uio.c |  72 +++++++------
 2 files changed, 136 insertions(+), 98 deletions(-)
Tetsuya Mukawa March 27, 2015, 8:39 a.m. UTC | #4 
This patch set cleans up pci uio implementation. These clean up are
for consolidating pci uio implementation of linuxapp and bsdapp, and
moving consolidated functions in eal common.
Because of above, this patch set tries to implement linuxapp and bsdapp
almost same.
Actual consolidations will be done later patch set.

PATCH v3 changes:
 - Squash patches related with pci_map_resource().
 - Free maps[].path to easy to understand.
   (Thanks to Iremonger, Bernard)
 - Close fds opened in this function.
 - Remove unused path variable from mapped_pci_resource structure.

PATCH v2 changes:
 - Move 'if-condition' to later patch series.
 - Fix memory leaks of path.
 - Fix typos.
   (Thanks to David Marchand)
 - Fix commit title and body.
 - Fix pci_map_resource() to handle MAP_FAILED.
   (Thanks to Iremonger, Bernard)

Changes:
 - This patch set is derived from below.
   "[PATCH v2] eal: Port Hotplug support for BSD"
 - Set cfg_fd as -1, when cfg_fd is closed.
   (Thanks to Iremonger, Bernard)
 - Remove needless coding style fixings.
 - Fix coding style of if-else condition.
   (Thanks to Richardson, Bruce)



Tetsuya Mukawa (5):
  eal: Fix coding style of eal_pci.c and eal_pci_uio.c
  eal: Close file descriptor of uio configuration
  eal: Fix memory leaks and needless increment of pci_map_addr
  eal/bsdapp: Change names of pci related data structure
  eal: Fix uio mapping differences between linuxapp and bsdapp

 lib/librte_eal/bsdapp/eal/eal_pci.c        | 166 +++++++++++++++++------------
 lib/librte_eal/linuxapp/eal/eal_pci_init.h |   1 -
 lib/librte_eal/linuxapp/eal/eal_pci_uio.c  |  91 +++++++++-------
 3 files changed, 154 insertions(+), 104 deletions(-)
Iremonger, Bernard April 21, 2015, 1:15 p.m. UTC | #5 
> -----Original Message-----
> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> Sent: Friday, March 27, 2015 8:39 AM
> To: dev@dpdk.org
> Cc: Iremonger, Bernard; Richardson, Bruce; david.marchand@6wind.com; Tetsuya Mukawa
> Subject: [PATCH v3 0/5] Clean up pci uio implementations
> 
> This patch set cleans up pci uio implementation. These clean up are for consolidating pci uio
> implementation of linuxapp and bsdapp, and moving consolidated functions in eal common.
> Because of above, this patch set tries to implement linuxapp and bsdapp almost same.
> Actual consolidations will be done later patch set.
> 
> PATCH v3 changes:
>  - Squash patches related with pci_map_resource().
>  - Free maps[].path to easy to understand.
>    (Thanks to Iremonger, Bernard)
>  - Close fds opened in this function.
>  - Remove unused path variable from mapped_pci_resource structure.
> 
> PATCH v2 changes:
>  - Move 'if-condition' to later patch series.
>  - Fix memory leaks of path.
>  - Fix typos.
>    (Thanks to David Marchand)
>  - Fix commit title and body.
>  - Fix pci_map_resource() to handle MAP_FAILED.
>    (Thanks to Iremonger, Bernard)
> 
> Changes:
>  - This patch set is derived from below.
>    "[PATCH v2] eal: Port Hotplug support for BSD"
>  - Set cfg_fd as -1, when cfg_fd is closed.
>    (Thanks to Iremonger, Bernard)
>  - Remove needless coding style fixings.
>  - Fix coding style of if-else condition.
>    (Thanks to Richardson, Bruce)
> 
> 
> 
> Tetsuya Mukawa (5):
>   eal: Fix coding style of eal_pci.c and eal_pci_uio.c
>   eal: Close file descriptor of uio configuration
>   eal: Fix memory leaks and needless increment of pci_map_addr
>   eal/bsdapp: Change names of pci related data structure
>   eal: Fix uio mapping differences between linuxapp and bsdapp
> 
>  lib/librte_eal/bsdapp/eal/eal_pci.c        | 166 +++++++++++++++++------------
>  lib/librte_eal/linuxapp/eal/eal_pci_init.h |   1 -
>  lib/librte_eal/linuxapp/eal/eal_pci_uio.c  |  91 +++++++++-------
>  3 files changed, 154 insertions(+), 104 deletions(-)
> 
> --
> 1.9.1
Series
Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
Tetsuya Mukawa April 23, 2015, 4:23 a.m. UTC | #6 
On 2015/04/21 22:15, Iremonger, Bernard wrote:
>> -----Original Message-----
>> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
>> Sent: Friday, March 27, 2015 8:39 AM
>> To: dev@dpdk.org
>> Cc: Iremonger, Bernard; Richardson, Bruce; david.marchand@6wind.com; Tetsuya Mukawa
>> Subject: [PATCH v3 0/5] Clean up pci uio implementations
>>
>> This patch set cleans up pci uio implementation. These clean up are for consolidating pci uio
>> implementation of linuxapp and bsdapp, and moving consolidated functions in eal common.
>> Because of above, this patch set tries to implement linuxapp and bsdapp almost same.
>> Actual consolidations will be done later patch set.
>>
>> PATCH v3 changes:
>>  - Squash patches related with pci_map_resource().
>>  - Free maps[].path to easy to understand.
>>    (Thanks to Iremonger, Bernard)
>>  - Close fds opened in this function.
>>  - Remove unused path variable from mapped_pci_resource structure.
>>
>> PATCH v2 changes:
>>  - Move 'if-condition' to later patch series.
>>  - Fix memory leaks of path.
>>  - Fix typos.
>>    (Thanks to David Marchand)
>>  - Fix commit title and body.
>>  - Fix pci_map_resource() to handle MAP_FAILED.
>>    (Thanks to Iremonger, Bernard)
>>
>> Changes:
>>  - This patch set is derived from below.
>>    "[PATCH v2] eal: Port Hotplug support for BSD"
>>  - Set cfg_fd as -1, when cfg_fd is closed.
>>    (Thanks to Iremonger, Bernard)
>>  - Remove needless coding style fixings.
>>  - Fix coding style of if-else condition.
>>    (Thanks to Richardson, Bruce)
>>
>>
>>
>> Tetsuya Mukawa (5):
>>   eal: Fix coding style of eal_pci.c and eal_pci_uio.c
>>   eal: Close file descriptor of uio configuration
>>   eal: Fix memory leaks and needless increment of pci_map_addr
>>   eal/bsdapp: Change names of pci related data structure
>>   eal: Fix uio mapping differences between linuxapp and bsdapp
>>
>>  lib/librte_eal/bsdapp/eal/eal_pci.c        | 166 +++++++++++++++++------------
>>  lib/librte_eal/linuxapp/eal/eal_pci_init.h |   1 -
>>  lib/librte_eal/linuxapp/eal/eal_pci_uio.c  |  91 +++++++++-------
>>  3 files changed, 154 insertions(+), 104 deletions(-)
>>
>> --
>> 1.9.1
> Series
> Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
Hi Bernard,

Thanks for your checking.
It seems I need to rebase this patches for latest matser branch.
So I will update and submit it soon.

Regards,
Tetsuya
Iremonger, Bernard April 23, 2015, 8:12 a.m. UTC | #7 
> -----Original Message-----
> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> Sent: Thursday, April 23, 2015 5:24 AM
> To: Iremonger, Bernard; dev@dpdk.org
> Cc: Richardson, Bruce; david.marchand@6wind.com
> Subject: Re: [PATCH v3 0/5] Clean up pci uio implementations
> 
> On 2015/04/21 22:15, Iremonger, Bernard wrote:
> >> -----Original Message-----
> >> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> >> Sent: Friday, March 27, 2015 8:39 AM
> >> To: dev@dpdk.org
> >> Cc: Iremonger, Bernard; Richardson, Bruce; david.marchand@6wind.com;
> >> Tetsuya Mukawa
> >> Subject: [PATCH v3 0/5] Clean up pci uio implementations
> >>
> >> This patch set cleans up pci uio implementation. These clean up are
> >> for consolidating pci uio implementation of linuxapp and bsdapp, and moving consolidated
> functions in eal common.
> >> Because of above, this patch set tries to implement linuxapp and bsdapp almost same.
> >> Actual consolidations will be done later patch set.
> >>
> >> PATCH v3 changes:
> >>  - Squash patches related with pci_map_resource().
> >>  - Free maps[].path to easy to understand.
> >>    (Thanks to Iremonger, Bernard)
> >>  - Close fds opened in this function.
> >>  - Remove unused path variable from mapped_pci_resource structure.
> >>
> >> PATCH v2 changes:
> >>  - Move 'if-condition' to later patch series.
> >>  - Fix memory leaks of path.
> >>  - Fix typos.
> >>    (Thanks to David Marchand)
> >>  - Fix commit title and body.
> >>  - Fix pci_map_resource() to handle MAP_FAILED.
> >>    (Thanks to Iremonger, Bernard)
> >>
> >> Changes:
> >>  - This patch set is derived from below.
> >>    "[PATCH v2] eal: Port Hotplug support for BSD"
> >>  - Set cfg_fd as -1, when cfg_fd is closed.
> >>    (Thanks to Iremonger, Bernard)
> >>  - Remove needless coding style fixings.
> >>  - Fix coding style of if-else condition.
> >>    (Thanks to Richardson, Bruce)
> >>
> >>
> >>
> >> Tetsuya Mukawa (5):
> >>   eal: Fix coding style of eal_pci.c and eal_pci_uio.c
> >>   eal: Close file descriptor of uio configuration
> >>   eal: Fix memory leaks and needless increment of pci_map_addr
> >>   eal/bsdapp: Change names of pci related data structure
> >>   eal: Fix uio mapping differences between linuxapp and bsdapp
> >>
> >>  lib/librte_eal/bsdapp/eal/eal_pci.c        | 166 +++++++++++++++++------------
> >>  lib/librte_eal/linuxapp/eal/eal_pci_init.h |   1 -
> >>  lib/librte_eal/linuxapp/eal/eal_pci_uio.c  |  91 +++++++++-------
> >>  3 files changed, 154 insertions(+), 104 deletions(-)
> >>
> >> --
> >> 1.9.1
> > Series
> > Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
> Hi Bernard,
> 
> Thanks for your checking.
> It seems I need to rebase this patches for latest matser branch.
> So I will update and submit it soon.
> 
> Regards,
> Tetsuya

Hi Tetsuya,
I applied the v3 patches to the latest master branch on Monday last and there were no issues.
The patches built cleanly on Linux and Free BSD.

Regards,

Bernard.
Tetsuya Mukawa April 23, 2015, 9:48 a.m. UTC | #8 
On 2015/04/23 17:12, Iremonger, Bernard wrote:
>
>> -----Original Message-----
>> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
>> Sent: Thursday, April 23, 2015 5:24 AM
>> To: Iremonger, Bernard; dev@dpdk.org
>> Cc: Richardson, Bruce; david.marchand@6wind.com
>> Subject: Re: [PATCH v3 0/5] Clean up pci uio implementations
>>
>> On 2015/04/21 22:15, Iremonger, Bernard wrote:
>>>> -----Original Message-----
>>>> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
>>>> Sent: Friday, March 27, 2015 8:39 AM
>>>> To: dev@dpdk.org
>>>> Cc: Iremonger, Bernard; Richardson, Bruce; david.marchand@6wind.com;
>>>> Tetsuya Mukawa
>>>> Subject: [PATCH v3 0/5] Clean up pci uio implementations
>>>>
>>>> This patch set cleans up pci uio implementation. These clean up are
>>>> for consolidating pci uio implementation of linuxapp and bsdapp, and moving consolidated
>> functions in eal common.
>>>> Because of above, this patch set tries to implement linuxapp and bsdapp almost same.
>>>> Actual consolidations will be done later patch set.
>>>>
>>>> PATCH v3 changes:
>>>>  - Squash patches related with pci_map_resource().
>>>>  - Free maps[].path to easy to understand.
>>>>    (Thanks to Iremonger, Bernard)
>>>>  - Close fds opened in this function.
>>>>  - Remove unused path variable from mapped_pci_resource structure.
>>>>
>>>> PATCH v2 changes:
>>>>  - Move 'if-condition' to later patch series.
>>>>  - Fix memory leaks of path.
>>>>  - Fix typos.
>>>>    (Thanks to David Marchand)
>>>>  - Fix commit title and body.
>>>>  - Fix pci_map_resource() to handle MAP_FAILED.
>>>>    (Thanks to Iremonger, Bernard)
>>>>
>>>> Changes:
>>>>  - This patch set is derived from below.
>>>>    "[PATCH v2] eal: Port Hotplug support for BSD"
>>>>  - Set cfg_fd as -1, when cfg_fd is closed.
>>>>    (Thanks to Iremonger, Bernard)
>>>>  - Remove needless coding style fixings.
>>>>  - Fix coding style of if-else condition.
>>>>    (Thanks to Richardson, Bruce)
>>>>
>>>>
>>>>
>>>> Tetsuya Mukawa (5):
>>>>   eal: Fix coding style of eal_pci.c and eal_pci_uio.c
>>>>   eal: Close file descriptor of uio configuration
>>>>   eal: Fix memory leaks and needless increment of pci_map_addr
>>>>   eal/bsdapp: Change names of pci related data structure
>>>>   eal: Fix uio mapping differences between linuxapp and bsdapp
>>>>
>>>>  lib/librte_eal/bsdapp/eal/eal_pci.c        | 166 +++++++++++++++++------------
>>>>  lib/librte_eal/linuxapp/eal/eal_pci_init.h |   1 -
>>>>  lib/librte_eal/linuxapp/eal/eal_pci_uio.c  |  91 +++++++++-------
>>>>  3 files changed, 154 insertions(+), 104 deletions(-)
>>>>
>>>> --
>>>> 1.9.1
>>> Series
>>> Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
>> Hi Bernard,
>>
>> Thanks for your checking.
>> It seems I need to rebase this patches for latest matser branch.
>> So I will update and submit it soon.
>>
>> Regards,
>> Tetsuya
> Hi Tetsuya,
> I applied the v3 patches to the latest master branch on Monday last and there were no issues.
> The patches built cleanly on Linux and Free BSD.
>
> Regards,
>
> Bernard.
>  

Hi Bernard,

Thanks. I made sure I could apply my patches to latest.
I will prepare bottom half of port hotplug patches for BSD.

Regards,
Tetsuya
diff mbox

Patch

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;
-	}
-
-	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)
+			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