From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
Subject: Re: [PATCH v2 01/16] Separate igb_uio mapping into a
	separate file
Date: Wed, 21 May 2014 14:42:23 +0200
Message-ID: <5094274.XRo5rd3sBr@xps13>
References: <C6ECDF3AB251BE4894318F4E45123697592AA9BE@IRSMSX101.ger.corp.intel.com>
 <1400514709-24087-2-git-send-email-anatoly.burakov@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Cc: dev-VfR2kkLFssw@public.gmane.org
To: Anatoly Burakov <anatoly.burakov-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Return-path: <dev-bounces-VfR2kkLFssw@public.gmane.org>
In-Reply-To: <1400514709-24087-2-git-send-email-anatoly.burakov-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev-VfR2kkLFssw@public.gmane.org>
List-Help: <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=subscribe>
Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org
Sender: "dev" <dev-bounces-VfR2kkLFssw@public.gmane.org>

Hi Anatoly,

2014-05-19 16:51, Anatoly Burakov:
> In order to make the code a bit more clean while using multiple
> drivers, IGB_UIO mapping has been separated into its own file.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

[...]
>  /* map a particular resource from a file */
> -static void *
> -pci_map_resource(void *requested_addr, const char *devname, off_t offset,
> +void *
> +pci_map_resource(void *requested_addr, int fd, off_t offset,
>  		 size_t size)
>  {
> -	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,
> +		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;
[...]
> -static int
> -pci_uio_map_resource(struct rte_pci_device *dev)
[...]
> -		/* if matching map is found, then use it */
> -		if (j != nb_maps) {
> -			offset = j * pagesz;
> -			if (maps[j].addr != NULL ||
> -			    (mapaddr = pci_map_resource(NULL, devname,
> -							(off_t)offset,
> -							(size_t)maps[j].size)
> -			    ) == NULL) {
> -				rte_free(uio_res);
> -				return (-1);
> -			}
> -
> -			maps[j].addr = mapaddr;
> -			maps[j].offset = offset;
> -			dev->mem_resource[i].addr = mapaddr;
> -		}
[...]
> +		/* if matching map is found, then use it */
> +		if (j != nb_maps) {
> +			offset = j * pagesz;
> +
> +			/*
> +			 * open devname, to mmap it
> +			 */
> +			fd = open(uio_res->path, O_RDWR);
> +			if (fd < 0) {
> +				RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
> +					uio_res->path, strerror(errno));
> +				rte_free(uio_res);
> +				return -1;
> +			}
> +
> +			if (maps[j].addr != NULL
> +					|| (mapaddr = pci_map_resource(NULL, fd,
> +							(off_t) offset, (size_t) maps[j].size)) == NULL) 
{
> +				rte_free(uio_res);
> +				close(fd);
> +				return (-1);
> +			}
> +			close(fd);
> +
> +			maps[j].addr = mapaddr;
> +			maps[j].offset = offset;
> +			dev->mem_resource[i].addr = mapaddr;
> +		}

Looking at pci_map_resource(), it seems you are not only moving functions in 
another file.

Please split this patch in a way we can clearly see what is changing.

Thanks
-- 
Thomas