DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH] bus/pci: fix secondary process PCI uio resource map problem
@ 2024-01-24 10:45 Chaoyong He
  2024-01-29  9:22 ` [PATCH v2 0/2] fix secondary process PCI UIO resource problem Chaoyong He
  0 siblings, 1 reply; 26+ messages in thread
From: Chaoyong He @ 2024-01-24 10:45 UTC (permalink / raw)
  To: dev
  Cc: oss-drivers, Zerun Fu, mukawa, stable, Chaoyong He, Long Wu, Peng Zhang

From: Zerun Fu <zerun.fu@corigine.com>

For the primary process, the logic loops all BARs and will skip
the map of BAR with an invalid physical address (0), also will
assign 'uio_res->nb_maps' with the real mapped BARs number. But
for the secondary process, instead of loops all BARs, the logic
using the 'uio_res->nb_map' as index. If the device uses continuous
BARs there will be no problem, whereas if it uses discrete BARs,
it will lead to mapping errors.

Fix this problem by also loops all BARs and skip the map of BAR
with an invalid physical address in secondary process.

Fixes: 9b957f378abf ("pci: merge uio functions for linux and bsd")
Cc: mukawa@igel.co.jp
Cc: stable@dpdk.org

Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Long Wu <long.wu@corigine.com>
Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
---
 drivers/bus/pci/pci_common_uio.c | 94 ++++++++++++++++++++------------
 1 file changed, 60 insertions(+), 34 deletions(-)

diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c
index 76c661f054..fcd8a49daf 100644
--- a/drivers/bus/pci/pci_common_uio.c
+++ b/drivers/bus/pci/pci_common_uio.c
@@ -23,10 +23,57 @@ static struct rte_tailq_elem rte_uio_tailq = {
 };
 EAL_REGISTER_TAILQ(rte_uio_tailq)
 
+static int
+pci_uio_map_secondary_resource_by_index(struct rte_pci_device *dev,
+		int res_idx, struct mapped_pci_resource *uio_res, int map_idx)
+{
+	int fd, i;
+
+	if (map_idx >= uio_res->nb_maps)
+		return -1;
+
+	/*
+	 * open devname, to mmap it
+	 */
+	fd = open(uio_res->maps[map_idx].path, O_RDWR);
+	if (fd < 0) {
+		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
+			uio_res->maps[map_idx].path, strerror(errno));
+		return -1;
+	}
+
+	void *mapaddr = pci_map_resource(uio_res->maps[map_idx].addr,
+			fd, (off_t)uio_res->maps[map_idx].offset,
+			(size_t)uio_res->maps[map_idx].size, 0);
+
+	/* fd is not needed in secondary process, close it */
+	close(fd);
+	if (mapaddr != uio_res->maps[map_idx].addr) {
+		RTE_LOG(ERR, EAL,
+			"Cannot mmap device resource file %s to address: %p\n",
+			uio_res->maps[map_idx].path,
+			uio_res->maps[map_idx].addr);
+		if (mapaddr != NULL) {
+			/* unmap addrs correctly mapped */
+			for (i = 0; i < map_idx; i++)
+				pci_unmap_resource(
+					uio_res->maps[i].addr,
+					(size_t)uio_res->maps[i].size);
+			/* unmap addr wrongly mapped */
+			pci_unmap_resource(mapaddr,
+				(size_t)uio_res->maps[map_idx].size);
+		}
+		return -1;
+	}
+	dev->mem_resource[res_idx].addr = mapaddr;
+
+	return 0;
+}
+
 static int
 pci_uio_map_secondary(struct rte_pci_device *dev)
 {
-	int fd, i, j;
+	int map_idx = 0, res_idx, ret;
 	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);
@@ -37,41 +84,20 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
 		if (rte_pci_addr_cmp(&uio_res->pci_addr, &dev->addr))
 			continue;
 
-		for (i = 0; i != uio_res->nb_maps; i++) {
-			/*
-			 * 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;
+		/* Map all BARs */
+		for (res_idx = 0; res_idx != PCI_MAX_RESOURCE; res_idx++) {
+			 /* skip empty BAR */
+			if (dev->mem_resource[res_idx].phys_addr == 0)
+				continue;
+
+			ret = pci_uio_map_secondary_resource_by_index(dev, res_idx,
+					uio_res, map_idx);
+			if (ret < 0) {
+				RTE_LOG(ERR, EAL, "Failed to map resources\n");
+				return ret;
 			}
 
-			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);
-
-			/* fd is not needed in secondary process, close it */
-			close(fd);
-			if (mapaddr != 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);
-				if (mapaddr != NULL) {
-					/* unmap addrs correctly mapped */
-					for (j = 0; j < i; j++)
-						pci_unmap_resource(
-							uio_res->maps[j].addr,
-							(size_t)uio_res->maps[j].size);
-					/* unmap addr wrongly mapped */
-					pci_unmap_resource(mapaddr,
-						(size_t)uio_res->maps[i].size);
-				}
-				return -1;
-			}
-			dev->mem_resource[i].addr = mapaddr;
+			map_idx++;
 		}
 		return 0;
 	}
-- 
2.39.1


^ permalink raw reply	[flat|nested] 26+ messages in thread
* [PATCH v2 0/2] fix secondary process PCI UIO resource problem
  2024-01-24 10:45 [PATCH] bus/pci: fix secondary process PCI uio resource map problem Chaoyong He
@ 2024-01-29  9:22 ` Chaoyong He
  2024-01-29  9:22   ` [PATCH v2 1/2] bus/pci: fix secondary process PCI uio resource map problem Chaoyong He
                     ` (3 more replies)
  0 siblings, 4 replies; 26+ messages in thread
From: Chaoyong He @ 2024-01-29  9:22 UTC (permalink / raw)
  To: dev; +Cc: oss-drivers, Chaoyong He

This patch series aims to fix some problems in secondary process PCI UIO
resource map logic.

Zerun Fu (2):
  bus/pci: fix secondary process PCI uio resource map problem
  bus/pci: fix secondary process save 'FD' problem

 drivers/bus/pci/linux/pci_uio.c  |   5 +-
 drivers/bus/pci/pci_common_uio.c | 102 +++++++++++++++++++------------
 2 files changed, 68 insertions(+), 39 deletions(-)

-- 
2.39.1


^ permalink raw reply	[flat|nested] 26+ messages in thread
* [PATCH v2 1/2] bus/pci: fix secondary process PCI uio resource map problem
  2024-01-29  9:22 ` [PATCH v2 0/2] fix secondary process PCI UIO resource problem Chaoyong He
@ 2024-01-29  9:22   ` Chaoyong He
  2024-01-30  4:00     ` fengchengwen
  2024-03-14 10:55     ` Burakov, Anatoly
  2024-01-29  9:22   ` [PATCH v2 2/2] bus/pci: fix secondary process save 'FD' problem Chaoyong He
                     ` (2 subsequent siblings)
  3 siblings, 2 replies; 26+ messages in thread
From: Chaoyong He @ 2024-01-29  9:22 UTC (permalink / raw)
  To: dev
  Cc: oss-drivers, Zerun Fu, mukawa, stable, Chaoyong He, Long Wu, Peng Zhang

From: Zerun Fu <zerun.fu@corigine.com>

For the primary process, the logic loops all BARs and will skip
the map of BAR with an invalid physical address (0), also will
assign 'uio_res->nb_maps' with the real mapped BARs number. But
for the secondary process, instead of loops all BARs, the logic
using the 'uio_res->nb_map' as index. If the device uses continuous
BARs there will be no problem, whereas if it uses discrete BARs,
it will lead to mapping errors.

Fix this problem by also loops all BARs and skip the map of BAR
with an invalid physical address in secondary process.

Fixes: 9b957f378abf ("pci: merge uio functions for linux and bsd")
Cc: mukawa@igel.co.jp
Cc: stable@dpdk.org

Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Long Wu <long.wu@corigine.com>
Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
---
 drivers/bus/pci/pci_common_uio.c | 94 ++++++++++++++++++++------------
 1 file changed, 60 insertions(+), 34 deletions(-)

diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c
index 76c661f054..fcd8a49daf 100644
--- a/drivers/bus/pci/pci_common_uio.c
+++ b/drivers/bus/pci/pci_common_uio.c
@@ -23,10 +23,57 @@ static struct rte_tailq_elem rte_uio_tailq = {
 };
 EAL_REGISTER_TAILQ(rte_uio_tailq)
 
+static int
+pci_uio_map_secondary_resource_by_index(struct rte_pci_device *dev,
+		int res_idx, struct mapped_pci_resource *uio_res, int map_idx)
+{
+	int fd, i;
+
+	if (map_idx >= uio_res->nb_maps)
+		return -1;
+
+	/*
+	 * open devname, to mmap it
+	 */
+	fd = open(uio_res->maps[map_idx].path, O_RDWR);
+	if (fd < 0) {
+		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
+			uio_res->maps[map_idx].path, strerror(errno));
+		return -1;
+	}
+
+	void *mapaddr = pci_map_resource(uio_res->maps[map_idx].addr,
+			fd, (off_t)uio_res->maps[map_idx].offset,
+			(size_t)uio_res->maps[map_idx].size, 0);
+
+	/* fd is not needed in secondary process, close it */
+	close(fd);
+	if (mapaddr != uio_res->maps[map_idx].addr) {
+		RTE_LOG(ERR, EAL,
+			"Cannot mmap device resource file %s to address: %p\n",
+			uio_res->maps[map_idx].path,
+			uio_res->maps[map_idx].addr);
+		if (mapaddr != NULL) {
+			/* unmap addrs correctly mapped */
+			for (i = 0; i < map_idx; i++)
+				pci_unmap_resource(
+					uio_res->maps[i].addr,
+					(size_t)uio_res->maps[i].size);
+			/* unmap addr wrongly mapped */
+			pci_unmap_resource(mapaddr,
+				(size_t)uio_res->maps[map_idx].size);
+		}
+		return -1;
+	}
+	dev->mem_resource[res_idx].addr = mapaddr;
+
+	return 0;
+}
+
 static int
 pci_uio_map_secondary(struct rte_pci_device *dev)
 {
-	int fd, i, j;
+	int map_idx = 0, res_idx, ret;
 	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);
@@ -37,41 +84,20 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
 		if (rte_pci_addr_cmp(&uio_res->pci_addr, &dev->addr))
 			continue;
 
-		for (i = 0; i != uio_res->nb_maps; i++) {
-			/*
-			 * 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;
+		/* Map all BARs */
+		for (res_idx = 0; res_idx != PCI_MAX_RESOURCE; res_idx++) {
+			 /* skip empty BAR */
+			if (dev->mem_resource[res_idx].phys_addr == 0)
+				continue;
+
+			ret = pci_uio_map_secondary_resource_by_index(dev, res_idx,
+					uio_res, map_idx);
+			if (ret < 0) {
+				RTE_LOG(ERR, EAL, "Failed to map resources\n");
+				return ret;
 			}
 
-			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);
-
-			/* fd is not needed in secondary process, close it */
-			close(fd);
-			if (mapaddr != 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);
-				if (mapaddr != NULL) {
-					/* unmap addrs correctly mapped */
-					for (j = 0; j < i; j++)
-						pci_unmap_resource(
-							uio_res->maps[j].addr,
-							(size_t)uio_res->maps[j].size);
-					/* unmap addr wrongly mapped */
-					pci_unmap_resource(mapaddr,
-						(size_t)uio_res->maps[i].size);
-				}
-				return -1;
-			}
-			dev->mem_resource[i].addr = mapaddr;
+			map_idx++;
 		}
 		return 0;
 	}
-- 
2.39.1


^ permalink raw reply	[flat|nested] 26+ messages in thread
* [PATCH v2 2/2] bus/pci: fix secondary process save 'FD' problem
  2024-01-29  9:22 ` [PATCH v2 0/2] fix secondary process PCI UIO resource problem Chaoyong He
  2024-01-29  9:22   ` [PATCH v2 1/2] bus/pci: fix secondary process PCI uio resource map problem Chaoyong He
@ 2024-01-29  9:22   ` Chaoyong He
  2024-01-30  6:16     ` fengchengwen
  2024-03-14 11:01     ` Burakov, Anatoly
  2024-03-11  6:42   ` [PATCH v2 0/2] fix secondary process PCI UIO resource problem Chaoyong He
  2024-04-19  3:26   ` Chaoyong He
  3 siblings, 2 replies; 26+ messages in thread
From: Chaoyong He @ 2024-01-29  9:22 UTC (permalink / raw)
  To: dev
  Cc: oss-drivers, Zerun Fu, mukawa, stable, Chaoyong He, Long Wu, Peng Zhang

From: Zerun Fu <zerun.fu@corigine.com>

In the previous logic the 'fd' was only saved in the primary process,
but for some devices this value is also used in the secondary logic.

For example, the call of 'rte_pci_find_ext_capability()' will fail in
the secondary process.

Fix this problem by getting and saving the value of 'fd' also in the
secondary process logic.

Fixes: 9b957f378abf ("pci: merge uio functions for linux and bsd")
Cc: mukawa@igel.co.jp
Cc: stable@dpdk.org

Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Long Wu <long.wu@corigine.com>
Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
---
 drivers/bus/pci/linux/pci_uio.c  | 5 ++++-
 drivers/bus/pci/pci_common_uio.c | 8 ++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index 97d740dfe5..6680e42efb 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -237,7 +237,7 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
 	}
 	snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
 
-	/* save fd if in primary process */
+	/* save fd */
 	fd = open(devname, O_RDWR);
 	if (fd < 0) {
 		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
@@ -261,6 +261,9 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
 	if (rte_intr_dev_fd_set(dev->intr_handle, uio_cfg_fd))
 		goto error;
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return 0;
+
 	if (dev->kdrv == RTE_PCI_KDRV_IGB_UIO) {
 		if (rte_intr_type_set(dev->intr_handle, RTE_INTR_HANDLE_UIO))
 			goto error;
diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c
index fcd8a49daf..b6f79b067d 100644
--- a/drivers/bus/pci/pci_common_uio.c
+++ b/drivers/bus/pci/pci_common_uio.c
@@ -122,15 +122,15 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 	if (rte_intr_dev_fd_set(dev->intr_handle, -1))
 		return -1;
 
-	/* 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)
 		return ret;
 
+	/* secondary processes - use already recorded details */
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return pci_uio_map_secondary(dev);
+
 	/* Map all BARs */
 	for (i = 0; i != PCI_MAX_RESOURCE; i++) {
 		/* skip empty BAR */
-- 
2.39.1


^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/2] bus/pci: fix secondary process PCI uio resource map problem
  2024-01-29  9:22   ` [PATCH v2 1/2] bus/pci: fix secondary process PCI uio resource map problem Chaoyong He
@ 2024-01-30  4:00     ` fengchengwen
  2024-03-14 10:55     ` Burakov, Anatoly
  1 sibling, 0 replies; 26+ messages in thread
From: fengchengwen @ 2024-01-30  4:00 UTC (permalink / raw)
  To: Chaoyong He, dev
  Cc: oss-drivers, Zerun Fu, mukawa, stable, Long Wu, Peng Zhang

Hi,

Nice to see this fix.
ps: our team also founds this problem, and the bugfix is still being reviewed internally.

As for this patch, I prefer not extract subfunction, because the father is simple and just
need do some minor adjustment.

		map_idx = 0;
		for (i = 0; i != PCI_MAX_RESOURCE; i++) {
			/* skip empty BAR */
			if (dev->mem_resource[i].phys_addr == 0)
				continue;
			... // use map_idx instead i
			map_idx++;
			dev->mem_resource[i].addr = mapaddr;
		}

Also suggest use pci_uio_find_resource() to refactor pci_uio_map_secondary() function.

Thanks

On 2024/1/29 17:22, Chaoyong He wrote:
> From: Zerun Fu <zerun.fu@corigine.com>
> 
> For the primary process, the logic loops all BARs and will skip
> the map of BAR with an invalid physical address (0), also will
> assign 'uio_res->nb_maps' with the real mapped BARs number. But
> for the secondary process, instead of loops all BARs, the logic
> using the 'uio_res->nb_map' as index. If the device uses continuous
> BARs there will be no problem, whereas if it uses discrete BARs,
> it will lead to mapping errors.
> 
> Fix this problem by also loops all BARs and skip the map of BAR
> with an invalid physical address in secondary process.
> 
> Fixes: 9b957f378abf ("pci: merge uio functions for linux and bsd")
> Cc: mukawa@igel.co.jp
> Cc: stable@dpdk.org
> 
> Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Long Wu <long.wu@corigine.com>
> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
> ---
>  drivers/bus/pci/pci_common_uio.c | 94 ++++++++++++++++++++------------
>  1 file changed, 60 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c
> index 76c661f054..fcd8a49daf 100644
> --- a/drivers/bus/pci/pci_common_uio.c
> +++ b/drivers/bus/pci/pci_common_uio.c
> @@ -23,10 +23,57 @@ static struct rte_tailq_elem rte_uio_tailq = {
>  };
>  EAL_REGISTER_TAILQ(rte_uio_tailq)
>  
> +static int
> +pci_uio_map_secondary_resource_by_index(struct rte_pci_device *dev,
> +		int res_idx, struct mapped_pci_resource *uio_res, int map_idx)
> +{
> +	int fd, i;
> +
> +	if (map_idx >= uio_res->nb_maps)
> +		return -1;
> +
> +	/*
> +	 * open devname, to mmap it
> +	 */
> +	fd = open(uio_res->maps[map_idx].path, O_RDWR);
> +	if (fd < 0) {
> +		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
> +			uio_res->maps[map_idx].path, strerror(errno));
> +		return -1;
> +	}
> +
> +	void *mapaddr = pci_map_resource(uio_res->maps[map_idx].addr,
> +			fd, (off_t)uio_res->maps[map_idx].offset,
> +			(size_t)uio_res->maps[map_idx].size, 0);
> +
> +	/* fd is not needed in secondary process, close it */
> +	close(fd);
> +	if (mapaddr != uio_res->maps[map_idx].addr) {
> +		RTE_LOG(ERR, EAL,
> +			"Cannot mmap device resource file %s to address: %p\n",
> +			uio_res->maps[map_idx].path,
> +			uio_res->maps[map_idx].addr);
> +		if (mapaddr != NULL) {
> +			/* unmap addrs correctly mapped */
> +			for (i = 0; i < map_idx; i++)
> +				pci_unmap_resource(
> +					uio_res->maps[i].addr,
> +					(size_t)uio_res->maps[i].size);
> +			/* unmap addr wrongly mapped */
> +			pci_unmap_resource(mapaddr,
> +				(size_t)uio_res->maps[map_idx].size);
> +		}
> +		return -1;
> +	}
> +	dev->mem_resource[res_idx].addr = mapaddr;
> +
> +	return 0;
> +}
> +
>  static int
>  pci_uio_map_secondary(struct rte_pci_device *dev)
>  {
> -	int fd, i, j;
> +	int map_idx = 0, res_idx, ret;
>  	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);
> @@ -37,41 +84,20 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
>  		if (rte_pci_addr_cmp(&uio_res->pci_addr, &dev->addr))
>  			continue;
>  
> -		for (i = 0; i != uio_res->nb_maps; i++) {
> -			/*
> -			 * 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;
> +		/* Map all BARs */
> +		for (res_idx = 0; res_idx != PCI_MAX_RESOURCE; res_idx++) {
> +			 /* skip empty BAR */
> +			if (dev->mem_resource[res_idx].phys_addr == 0)
> +				continue;
> +
> +			ret = pci_uio_map_secondary_resource_by_index(dev, res_idx,
> +					uio_res, map_idx);
> +			if (ret < 0) {
> +				RTE_LOG(ERR, EAL, "Failed to map resources\n");
> +				return ret;
>  			}
>  
> -			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);
> -
> -			/* fd is not needed in secondary process, close it */
> -			close(fd);
> -			if (mapaddr != 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);
> -				if (mapaddr != NULL) {
> -					/* unmap addrs correctly mapped */
> -					for (j = 0; j < i; j++)
> -						pci_unmap_resource(
> -							uio_res->maps[j].addr,
> -							(size_t)uio_res->maps[j].size);
> -					/* unmap addr wrongly mapped */
> -					pci_unmap_resource(mapaddr,
> -						(size_t)uio_res->maps[i].size);
> -				}
> -				return -1;
> -			}
> -			dev->mem_resource[i].addr = mapaddr;
> +			map_idx++;
>  		}
>  		return 0;
>  	}
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/2] bus/pci: fix secondary process save 'FD' problem
  2024-01-29  9:22   ` [PATCH v2 2/2] bus/pci: fix secondary process save 'FD' problem Chaoyong He
@ 2024-01-30  6:16     ` fengchengwen
  2024-03-14 11:01     ` Burakov, Anatoly
  1 sibling, 0 replies; 26+ messages in thread
From: fengchengwen @ 2024-01-30  6:16 UTC (permalink / raw)
  To: Chaoyong He, dev
  Cc: oss-drivers, Zerun Fu, mukawa, stable, Long Wu, Peng Zhang

Hi,

The pci_uio_alloc_resource in drivers/bus/pci/bsd should also modified.

How about add subfunction: pci_uio_init_intr() which includes access/rte_intr_fd_set/rte_intr_fd_get/rte_intr_type_set?

Thanks

On 2024/1/29 17:22, Chaoyong He wrote:
> From: Zerun Fu <zerun.fu@corigine.com>
> 
> In the previous logic the 'fd' was only saved in the primary process,
> but for some devices this value is also used in the secondary logic.
> 
> For example, the call of 'rte_pci_find_ext_capability()' will fail in
> the secondary process.
> 
> Fix this problem by getting and saving the value of 'fd' also in the
> secondary process logic.
> 
> Fixes: 9b957f378abf ("pci: merge uio functions for linux and bsd")
> Cc: mukawa@igel.co.jp
> Cc: stable@dpdk.org
> 
> Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Long Wu <long.wu@corigine.com>
> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
> ---
>  drivers/bus/pci/linux/pci_uio.c  | 5 ++++-
>  drivers/bus/pci/pci_common_uio.c | 8 ++++----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
> index 97d740dfe5..6680e42efb 100644
> --- a/drivers/bus/pci/linux/pci_uio.c
> +++ b/drivers/bus/pci/linux/pci_uio.c
> @@ -237,7 +237,7 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
>  	}
>  	snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
>  
> -	/* save fd if in primary process */
> +	/* save fd */
>  	fd = open(devname, O_RDWR);
>  	if (fd < 0) {
>  		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
> @@ -261,6 +261,9 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
>  	if (rte_intr_dev_fd_set(dev->intr_handle, uio_cfg_fd))
>  		goto error;
>  
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return 0;
> +
>  	if (dev->kdrv == RTE_PCI_KDRV_IGB_UIO) {
>  		if (rte_intr_type_set(dev->intr_handle, RTE_INTR_HANDLE_UIO))
>  			goto error;
> diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c
> index fcd8a49daf..b6f79b067d 100644
> --- a/drivers/bus/pci/pci_common_uio.c
> +++ b/drivers/bus/pci/pci_common_uio.c
> @@ -122,15 +122,15 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>  	if (rte_intr_dev_fd_set(dev->intr_handle, -1))
>  		return -1;
>  
> -	/* 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)
>  		return ret;
>  
> +	/* secondary processes - use already recorded details */
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return pci_uio_map_secondary(dev);
> +
>  	/* Map all BARs */
>  	for (i = 0; i != PCI_MAX_RESOURCE; i++) {
>  		/* skip empty BAR */
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread
* RE: [PATCH v2 0/2] fix secondary process PCI UIO resource problem
  2024-01-29  9:22 ` [PATCH v2 0/2] fix secondary process PCI UIO resource problem Chaoyong He
  2024-01-29  9:22   ` [PATCH v2 1/2] bus/pci: fix secondary process PCI uio resource map problem Chaoyong He
  2024-01-29  9:22   ` [PATCH v2 2/2] bus/pci: fix secondary process save 'FD' problem Chaoyong He
@ 2024-03-11  6:42   ` Chaoyong He
  2024-04-19  3:26   ` Chaoyong He
  3 siblings, 0 replies; 26+ messages in thread
From: Chaoyong He @ 2024-03-11  6:42 UTC (permalink / raw)
  To: dev; +Cc: oss-drivers

A gentle ping~

> -----Original Message-----
> From: Chaoyong He
> Sent: Monday, January 29, 2024 5:23 PM
> To: dev@dpdk.org
> Cc: oss-drivers <oss-drivers@corigine.com>; Chaoyong He
> <chaoyong.he@corigine.com>
> Subject: [PATCH v2 0/2] fix secondary process PCI UIO resource problem
> 
> This patch series aims to fix some problems in secondary process PCI UIO
> resource map logic.
> 
> Zerun Fu (2):
>   bus/pci: fix secondary process PCI uio resource map problem
>   bus/pci: fix secondary process save 'FD' problem
> 
>  drivers/bus/pci/linux/pci_uio.c  |   5 +-
>  drivers/bus/pci/pci_common_uio.c | 102 +++++++++++++++++++------------
>  2 files changed, 68 insertions(+), 39 deletions(-)
> 
> --
> 2.39.1


^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/2] bus/pci: fix secondary process PCI uio resource map problem
  2024-01-29  9:22   ` [PATCH v2 1/2] bus/pci: fix secondary process PCI uio resource map problem Chaoyong He
  2024-01-30  4:00     ` fengchengwen
@ 2024-03-14 10:55     ` Burakov, Anatoly
  1 sibling, 0 replies; 26+ messages in thread
From: Burakov, Anatoly @ 2024-03-14 10:55 UTC (permalink / raw)
  To: Chaoyong He, dev
  Cc: oss-drivers, Zerun Fu, mukawa, stable, Long Wu, Peng Zhang

On 1/29/2024 10:22 AM, Chaoyong He wrote:
> From: Zerun Fu <zerun.fu@corigine.com>
> 
> For the primary process, the logic loops all BARs and will skip
> the map of BAR with an invalid physical address (0), also will
> assign 'uio_res->nb_maps' with the real mapped BARs number. But
> for the secondary process, instead of loops all BARs, the logic
> using the 'uio_res->nb_map' as index. If the device uses continuous
> BARs there will be no problem, whereas if it uses discrete BARs,
> it will lead to mapping errors.
> 
> Fix this problem by also loops all BARs and skip the map of BAR
> with an invalid physical address in secondary process.
> 
> Fixes: 9b957f378abf ("pci: merge uio functions for linux and bsd")
> Cc: mukawa@igel.co.jp
> Cc: stable@dpdk.org
> 
> Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Long Wu <long.wu@corigine.com>
> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
> ---
>   drivers/bus/pci/pci_common_uio.c | 94 ++++++++++++++++++++------------
>   1 file changed, 60 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c
> index 76c661f054..fcd8a49daf 100644
> --- a/drivers/bus/pci/pci_common_uio.c
> +++ b/drivers/bus/pci/pci_common_uio.c
> @@ -23,10 +23,57 @@ static struct rte_tailq_elem rte_uio_tailq = {
>   };
>   EAL_REGISTER_TAILQ(rte_uio_tailq)
>   
> +static int
> +pci_uio_map_secondary_resource_by_index(struct rte_pci_device *dev,
> +		int res_idx, struct mapped_pci_resource *uio_res, int map_idx)
> +{
> +	int fd, i;
> +
> +	if (map_idx >= uio_res->nb_maps)
> +		return -1;
> +
> +	/*
> +	 * open devname, to mmap it
> +	 */
> +	fd = open(uio_res->maps[map_idx].path, O_RDWR);
> +	if (fd < 0) {
> +		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
> +			uio_res->maps[map_idx].path, strerror(errno));
> +		return -1;
> +	}
> +
> +	void *mapaddr = pci_map_resource(uio_res->maps[map_idx].addr,
> +			fd, (off_t)uio_res->maps[map_idx].offset,
> +			(size_t)uio_res->maps[map_idx].size, 0);
> +
> +	/* fd is not needed in secondary process, close it */
> +	close(fd);
> +	if (mapaddr != uio_res->maps[map_idx].addr) {
> +		RTE_LOG(ERR, EAL,
> +			"Cannot mmap device resource file %s to address: %p\n",
> +			uio_res->maps[map_idx].path,
> +			uio_res->maps[map_idx].addr);
> +		if (mapaddr != NULL) {
> +			/* unmap addrs correctly mapped */
> +			for (i = 0; i < map_idx; i++)
> +				pci_unmap_resource(
> +					uio_res->maps[i].addr,
> +					(size_t)uio_res->maps[i].size);
> +			/* unmap addr wrongly mapped */
> +			pci_unmap_resource(mapaddr,
> +				(size_t)uio_res->maps[map_idx].size);
> +		}
> +		return -1;
> +	}
> +	dev->mem_resource[res_idx].addr = mapaddr;
> +
> +	return 0;
> +}
> +
>   static int
>   pci_uio_map_secondary(struct rte_pci_device *dev)
>   {
> -	int fd, i, j;
> +	int map_idx = 0, res_idx, ret;
>   	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);
> @@ -37,41 +84,20 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
>   		if (rte_pci_addr_cmp(&uio_res->pci_addr, &dev->addr))
>   			continue;
>   
> -		for (i = 0; i != uio_res->nb_maps; i++) {
> -			/*
> -			 * 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;
> +		/* Map all BARs */
> +		for (res_idx = 0; res_idx != PCI_MAX_RESOURCE; res_idx++) {
> +			 /* skip empty BAR */
> +			if (dev->mem_resource[res_idx].phys_addr == 0)
> +				continue;
> +
> +			ret = pci_uio_map_secondary_resource_by_index(dev, res_idx,
> +					uio_res, map_idx);
> +			if (ret < 0) {
> +				RTE_LOG(ERR, EAL, "Failed to map resources\n");
> +				return ret;
>   			}
>   
> -			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);
> -
> -			/* fd is not needed in secondary process, close it */
> -			close(fd);
> -			if (mapaddr != 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);
> -				if (mapaddr != NULL) {
> -					/* unmap addrs correctly mapped */
> -					for (j = 0; j < i; j++)
> -						pci_unmap_resource(
> -							uio_res->maps[j].addr,
> -							(size_t)uio_res->maps[j].size);
> -					/* unmap addr wrongly mapped */
> -					pci_unmap_resource(mapaddr,
> -						(size_t)uio_res->maps[i].size);

Nitpicking, but I feel like this would've been better done from the 
caller, not here. This is how it's done in primary process.

> -				}
> -				return -1;
> -			}
> -			dev->mem_resource[i].addr = mapaddr;
> +			map_idx++;
>   		}
>   		return 0;
>   	}

With fix above,

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly


^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/2] bus/pci: fix secondary process save 'FD' problem
  2024-01-29  9:22   ` [PATCH v2 2/2] bus/pci: fix secondary process save 'FD' problem Chaoyong He
  2024-01-30  6:16     ` fengchengwen
@ 2024-03-14 11:01     ` Burakov, Anatoly
  1 sibling, 0 replies; 26+ messages in thread
From: Burakov, Anatoly @ 2024-03-14 11:01 UTC (permalink / raw)
  To: Chaoyong He, dev
  Cc: oss-drivers, Zerun Fu, mukawa, stable, Long Wu, Peng Zhang

On 1/29/2024 10:22 AM, Chaoyong He wrote:
> From: Zerun Fu <zerun.fu@corigine.com>
> 
> In the previous logic the 'fd' was only saved in the primary process,
> but for some devices this value is also used in the secondary logic.
> 
> For example, the call of 'rte_pci_find_ext_capability()' will fail in
> the secondary process.
> 
> Fix this problem by getting and saving the value of 'fd' also in the
> secondary process logic.
> 
> Fixes: 9b957f378abf ("pci: merge uio functions for linux and bsd")
> Cc: mukawa@igel.co.jp
> Cc: stable@dpdk.org
> 
> Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Long Wu <long.wu@corigine.com>
> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
> ---
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly


^ permalink raw reply	[flat|nested] 26+ messages in thread
* [PATCH v2 0/2] fix secondary process PCI UIO resource problem
  2024-01-29  9:22 ` [PATCH v2 0/2] fix secondary process PCI UIO resource problem Chaoyong He
                     ` (2 preceding siblings ...)
  2024-03-11  6:42   ` [PATCH v2 0/2] fix secondary process PCI UIO resource problem Chaoyong He
@ 2024-04-19  3:26   ` Chaoyong He
  2024-04-19  3:26     ` [PATCH v3 1/2] bus/pci: fix secondary process PCI uio resource map problem Chaoyong He
                       ` (2 more replies)
  3 siblings, 3 replies; 26+ messages in thread
From: Chaoyong He @ 2024-04-19  3:26 UTC (permalink / raw)
  To: dev; +Cc: oss-drivers, Chaoyong He

This patch series aims to fix some problems in secondary process PCI UIO
resource map logic.

---
v3:
* Modify logic as the comments of reviewers.
---

Zerun Fu (2):
  bus/pci: fix secondary process PCI uio resource map problem
  bus/pci: fix secondary process save 'FD' problem

 drivers/bus/pci/linux/pci_uio.c  |  5 +++-
 drivers/bus/pci/pci_common_uio.c | 48 +++++++++++++++++++-------------
 2 files changed, 33 insertions(+), 20 deletions(-)

-- 
2.39.1


^ permalink raw reply	[flat|nested] 26+ messages in thread
* [PATCH v3 1/2] bus/pci: fix secondary process PCI uio resource map problem
  2024-04-19  3:26   ` Chaoyong He
@ 2024-04-19  3:26     ` Chaoyong He
  2024-06-27 14:00       ` Thomas Monjalon
  2024-04-19  3:26     ` [PATCH v3 2/2] bus/pci: fix secondary process save 'FD' problem Chaoyong He
  2024-06-28  7:36     ` [PATCH v4 0/3] fix secondary process PCI UIO resource problem Chaoyong He
  2 siblings, 1 reply; 26+ messages in thread
From: Chaoyong He @ 2024-04-19  3:26 UTC (permalink / raw)
  To: dev
  Cc: oss-drivers, Zerun Fu, mukawa, stable, Chaoyong He, Long Wu, Peng Zhang

From: Zerun Fu <zerun.fu@corigine.com>

For the primary process, the logic loops all BARs and will skip
the map of BAR with an invalid physical address (0), also will
assign 'uio_res->nb_maps' with the real mapped BARs number. But
for the secondary process, instead of loops all BARs, the logic
using the 'uio_res->nb_map' as index. If the device uses continuous
BARs there will be no problem, whereas if it uses discrete BARs,
it will lead to mapping errors.

Fix this problem by also loops all BARs and skip the map of BAR
with an invalid physical address in secondary process.

Fixes: 9b957f378abf ("pci: merge uio functions for linux and bsd")
Cc: mukawa@igel.co.jp
Cc: stable@dpdk.org

Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Long Wu <long.wu@corigine.com>
Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
---
 drivers/bus/pci/pci_common_uio.c | 40 ++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c
index 76c661f054..432d68b12c 100644
--- a/drivers/bus/pci/pci_common_uio.c
+++ b/drivers/bus/pci/pci_common_uio.c
@@ -26,7 +26,7 @@ EAL_REGISTER_TAILQ(rte_uio_tailq)
 static int
 pci_uio_map_secondary(struct rte_pci_device *dev)
 {
-	int fd, i, j;
+	int fd, i, map_idx = 0, res_idx;
 	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);
@@ -37,41 +37,51 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
 		if (rte_pci_addr_cmp(&uio_res->pci_addr, &dev->addr))
 			continue;
 
-		for (i = 0; i != uio_res->nb_maps; i++) {
+		/* Map all BARs */
+		for (res_idx = 0; res_idx != PCI_MAX_RESOURCE; res_idx++) {
+			 /* skip empty BAR */
+			if (dev->mem_resource[res_idx].phys_addr == 0)
+				continue;
+
+			if (map_idx >= uio_res->nb_maps)
+				return -1;
+
 			/*
 			 * open devname, to mmap it
 			 */
-			fd = open(uio_res->maps[i].path, O_RDWR);
+			fd = open(uio_res->maps[map_idx].path, O_RDWR);
 			if (fd < 0) {
 				RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
-					uio_res->maps[i].path, strerror(errno));
+					uio_res->maps[map_idx].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);
+			void *mapaddr = pci_map_resource(uio_res->maps[map_idx].addr,
+					fd, (off_t)uio_res->maps[map_idx].offset,
+					(size_t)uio_res->maps[map_idx].size, 0);
 
 			/* fd is not needed in secondary process, close it */
 			close(fd);
-			if (mapaddr != uio_res->maps[i].addr) {
+			if (mapaddr != uio_res->maps[map_idx].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);
+					uio_res->maps[map_idx].path,
+					uio_res->maps[map_idx].addr);
 				if (mapaddr != NULL) {
 					/* unmap addrs correctly mapped */
-					for (j = 0; j < i; j++)
+					for (i = 0; i < map_idx; i++)
 						pci_unmap_resource(
-							uio_res->maps[j].addr,
-							(size_t)uio_res->maps[j].size);
+							uio_res->maps[i].addr,
+							(size_t)uio_res->maps[i].size);
 					/* unmap addr wrongly mapped */
 					pci_unmap_resource(mapaddr,
-						(size_t)uio_res->maps[i].size);
+						(size_t)uio_res->maps[map_idx].size);
 				}
 				return -1;
 			}
-			dev->mem_resource[i].addr = mapaddr;
+			dev->mem_resource[res_idx].addr = mapaddr;
+
+			map_idx++;
 		}
 		return 0;
 	}
-- 
2.39.1


^ permalink raw reply	[flat|nested] 26+ messages in thread
* [PATCH v3 2/2] bus/pci: fix secondary process save 'FD' problem
  2024-04-19  3:26   ` Chaoyong He
  2024-04-19  3:26     ` [PATCH v3 1/2] bus/pci: fix secondary process PCI uio resource map problem Chaoyong He
@ 2024-04-19  3:26     ` Chaoyong He
  2024-06-28  7:36     ` [PATCH v4 0/3] fix secondary process PCI UIO resource problem Chaoyong He
  2 siblings, 0 replies; 26+ messages in thread
From: Chaoyong He @ 2024-04-19  3:26 UTC (permalink / raw)
  To: dev
  Cc: oss-drivers, Zerun Fu, mukawa, stable, Chaoyong He, Long Wu, Peng Zhang

From: Zerun Fu <zerun.fu@corigine.com>

In the previous logic the 'fd' was only saved in the primary process,
but for some devices this value is also used in the secondary logic.

For example, the call of 'rte_pci_find_ext_capability()' will fail in
the secondary process.

Fix this problem by getting and saving the value of 'fd' also in the
secondary process logic.

Fixes: 9b957f378abf ("pci: merge uio functions for linux and bsd")
Cc: mukawa@igel.co.jp
Cc: stable@dpdk.org

Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Long Wu <long.wu@corigine.com>
Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
---
 drivers/bus/pci/linux/pci_uio.c  | 5 ++++-
 drivers/bus/pci/pci_common_uio.c | 8 ++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index 97d740dfe5..6680e42efb 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -237,7 +237,7 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
 	}
 	snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
 
-	/* save fd if in primary process */
+	/* save fd */
 	fd = open(devname, O_RDWR);
 	if (fd < 0) {
 		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
@@ -261,6 +261,9 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
 	if (rte_intr_dev_fd_set(dev->intr_handle, uio_cfg_fd))
 		goto error;
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return 0;
+
 	if (dev->kdrv == RTE_PCI_KDRV_IGB_UIO) {
 		if (rte_intr_type_set(dev->intr_handle, RTE_INTR_HANDLE_UIO))
 			goto error;
diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c
index 432d68b12c..8688862738 100644
--- a/drivers/bus/pci/pci_common_uio.c
+++ b/drivers/bus/pci/pci_common_uio.c
@@ -106,15 +106,15 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 	if (rte_intr_dev_fd_set(dev->intr_handle, -1))
 		return -1;
 
-	/* 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)
 		return ret;
 
+	/* secondary processes - use already recorded details */
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return pci_uio_map_secondary(dev);
+
 	/* Map all BARs */
 	for (i = 0; i != PCI_MAX_RESOURCE; i++) {
 		/* skip empty BAR */
-- 
2.39.1


^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/2] bus/pci: fix secondary process PCI uio resource map problem
  2024-04-19  3:26     ` [PATCH v3 1/2] bus/pci: fix secondary process PCI uio resource map problem Chaoyong He
@ 2024-06-27 14:00       ` Thomas Monjalon
  2024-06-28  1:03         ` Chaoyong He
  0 siblings, 1 reply; 26+ messages in thread
From: Thomas Monjalon @ 2024-06-27 14:00 UTC (permalink / raw)
  To: Chaoyong He
  Cc: dev, stable, oss-drivers, Zerun Fu, mukawa, stable, Chaoyong He,
	Long Wu, Peng Zhang

19/04/2024 05:26, Chaoyong He:
> From: Zerun Fu <zerun.fu@corigine.com>
> 
> For the primary process, the logic loops all BARs and will skip
> the map of BAR with an invalid physical address (0), also will
> assign 'uio_res->nb_maps' with the real mapped BARs number. But
> for the secondary process, instead of loops all BARs, the logic
> using the 'uio_res->nb_map' as index. If the device uses continuous
> BARs there will be no problem, whereas if it uses discrete BARs,
> it will lead to mapping errors.
> 
> Fix this problem by also loops all BARs and skip the map of BAR
> with an invalid physical address in secondary process.
> 
> Fixes: 9b957f378abf ("pci: merge uio functions for linux and bsd")
> Cc: mukawa@igel.co.jp
> Cc: stable@dpdk.org
> 
> Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Long Wu <long.wu@corigine.com>
> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>

You should have kept the ack from Anatoly here.

>  drivers/bus/pci/pci_common_uio.c | 40 ++++++++++++++++++++------------
>  1 file changed, 25 insertions(+), 15 deletions(-)

There are too many changes in this sensitive code.
Please could you introduce a first patch for the renaming of the variable "i"?
It should make this patch simpler to read.

Thank you



^ permalink raw reply	[flat|nested] 26+ messages in thread
* RE: [PATCH v3 1/2] bus/pci: fix secondary process PCI uio resource map problem
  2024-06-27 14:00       ` Thomas Monjalon
@ 2024-06-28  1:03         ` Chaoyong He
  0 siblings, 0 replies; 26+ messages in thread
From: Chaoyong He @ 2024-06-28  1:03 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: dev, stable, oss-drivers, Zerun Fu, mukawa, stable, Long Wu, Nole Zhang

> 19/04/2024 05:26, Chaoyong He:
> > From: Zerun Fu <zerun.fu@corigine.com>
> >
> > For the primary process, the logic loops all BARs and will skip the
> > map of BAR with an invalid physical address (0), also will assign
> > 'uio_res->nb_maps' with the real mapped BARs number. But for the
> > secondary process, instead of loops all BARs, the logic using the
> > 'uio_res->nb_map' as index. If the device uses continuous BARs there
> > will be no problem, whereas if it uses discrete BARs, it will lead to
> > mapping errors.
> >
> > Fix this problem by also loops all BARs and skip the map of BAR with
> > an invalid physical address in secondary process.
> >
> > Fixes: 9b957f378abf ("pci: merge uio functions for linux and bsd")
> > Cc: mukawa@igel.co.jp
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
> > Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> > Reviewed-by: Long Wu <long.wu@corigine.com>
> > Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
> 
> You should have kept the ack from Anatoly here.
> 
> >  drivers/bus/pci/pci_common_uio.c | 40
> > ++++++++++++++++++++------------
> >  1 file changed, 25 insertions(+), 15 deletions(-)
> 
> There are too many changes in this sensitive code.
> Please could you introduce a first patch for the renaming of the variable "i"?
> It should make this patch simpler to read.
> 
> Thank you
> 

Okay, will do it in the next version patch.

Thanks for your review.



^ permalink raw reply	[flat|nested] 26+ messages in thread
* [PATCH v4 0/3] fix secondary process PCI UIO resource problem
  2024-04-19  3:26   ` Chaoyong He
  2024-04-19  3:26     ` [PATCH v3 1/2] bus/pci: fix secondary process PCI uio resource map problem Chaoyong He
  2024-04-19  3:26     ` [PATCH v3 2/2] bus/pci: fix secondary process save 'FD' problem Chaoyong He
@ 2024-06-28  7:36     ` Chaoyong He
  2024-06-28  7:36       ` [PATCH v4 1/3] bus/pci: rename the variable in UIO secondary map logic Chaoyong He
                         ` (4 more replies)
  2 siblings, 5 replies; 26+ messages in thread
From: Chaoyong He @ 2024-06-28  7:36 UTC (permalink / raw)
  To: dev; +Cc: oss-drivers, Chaoyong He

This patch series aims to fix some problems in secondary process PCI UIO
resource map logic.

---
v2:
* Modify as the advice from reviewer.
v3:
* Modify logic as the comments of reviewers.
v4:
* Split the commits to make it easier to understand.
---

Zerun Fu (3):
  bus/pci: rename the variable in UIO secondary map logic
  bus/pci: fix secondary process PCI uio resource map problem
  bus/pci: fix secondary process save 'FD' problem

 drivers/bus/pci/linux/pci_uio.c  |  5 +++-
 drivers/bus/pci/pci_common_uio.c | 48 +++++++++++++++++++-------------
 2 files changed, 33 insertions(+), 20 deletions(-)

-- 
2.39.1


^ permalink raw reply	[flat|nested] 26+ messages in thread
* [PATCH v4 1/3] bus/pci: rename the variable in UIO secondary map logic
  2024-06-28  7:36     ` [PATCH v4 0/3] fix secondary process PCI UIO resource problem Chaoyong He
@ 2024-06-28  7:36       ` Chaoyong He
  2024-06-28  7:36       ` [PATCH v4 2/3] bus/pci: fix secondary process PCI uio resource map problem Chaoyong He
                         ` (3 subsequent siblings)
  4 siblings, 0 replies; 26+ messages in thread
From: Chaoyong He @ 2024-06-28  7:36 UTC (permalink / raw)
  To: dev; +Cc: oss-drivers, Zerun Fu, Chaoyong He, Long Wu, Peng Zhang

From: Zerun Fu <zerun.fu@corigine.com>

To makes the logic clearer, rename the variable 'i' into 'map_idx'
and the variable 'j' into 'i'.

Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Long Wu <long.wu@corigine.com>
Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
---
 drivers/bus/pci/pci_common_uio.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c
index 76c661f054..caacbb69d9 100644
--- a/drivers/bus/pci/pci_common_uio.c
+++ b/drivers/bus/pci/pci_common_uio.c
@@ -26,7 +26,7 @@ EAL_REGISTER_TAILQ(rte_uio_tailq)
 static int
 pci_uio_map_secondary(struct rte_pci_device *dev)
 {
-	int fd, i, j;
+	int fd, i, map_idx;
 	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);
@@ -37,41 +37,41 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
 		if (rte_pci_addr_cmp(&uio_res->pci_addr, &dev->addr))
 			continue;
 
-		for (i = 0; i != uio_res->nb_maps; i++) {
+		for (map_idx = 0; map_idx != uio_res->nb_maps; map_idx++) {
 			/*
 			 * open devname, to mmap it
 			 */
-			fd = open(uio_res->maps[i].path, O_RDWR);
+			fd = open(uio_res->maps[map_idx].path, O_RDWR);
 			if (fd < 0) {
 				RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
-					uio_res->maps[i].path, strerror(errno));
+					uio_res->maps[map_idx].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);
+			void *mapaddr = pci_map_resource(uio_res->maps[map_idx].addr,
+					fd, (off_t)uio_res->maps[map_idx].offset,
+					(size_t)uio_res->maps[map_idx].size, 0);
 
 			/* fd is not needed in secondary process, close it */
 			close(fd);
-			if (mapaddr != uio_res->maps[i].addr) {
+			if (mapaddr != uio_res->maps[map_idx].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);
+					uio_res->maps[map_idx].path,
+					uio_res->maps[map_idx].addr);
 				if (mapaddr != NULL) {
 					/* unmap addrs correctly mapped */
-					for (j = 0; j < i; j++)
+					for (i = 0; i < map_idx; i++)
 						pci_unmap_resource(
-							uio_res->maps[j].addr,
-							(size_t)uio_res->maps[j].size);
+							uio_res->maps[i].addr,
+							(size_t)uio_res->maps[i].size);
 					/* unmap addr wrongly mapped */
 					pci_unmap_resource(mapaddr,
-						(size_t)uio_res->maps[i].size);
+						(size_t)uio_res->maps[map_idx].size);
 				}
 				return -1;
 			}
-			dev->mem_resource[i].addr = mapaddr;
+			dev->mem_resource[map_idx].addr = mapaddr;
 		}
 		return 0;
 	}
-- 
2.39.1


^ permalink raw reply	[flat|nested] 26+ messages in thread
* [PATCH v4 2/3] bus/pci: fix secondary process PCI uio resource map problem
  2024-06-28  7:36     ` [PATCH v4 0/3] fix secondary process PCI UIO resource problem Chaoyong He
  2024-06-28  7:36       ` [PATCH v4 1/3] bus/pci: rename the variable in UIO secondary map logic Chaoyong He
@ 2024-06-28  7:36       ` Chaoyong He
  2024-06-28  7:36       ` [PATCH v4 3/3] bus/pci: fix secondary process save 'FD' problem Chaoyong He
                         ` (2 subsequent siblings)
  4 siblings, 0 replies; 26+ messages in thread
From: Chaoyong He @ 2024-06-28  7:36 UTC (permalink / raw)
  To: dev
  Cc: oss-drivers, Zerun Fu, mukawa, stable, Chaoyong He, Long Wu, Peng Zhang

From: Zerun Fu <zerun.fu@corigine.com>

For the primary process, the logic loops all BARs and will skip
the map of BAR with an invalid physical address (0), also will
assign 'uio_res->nb_maps' with the real mapped BARs number. But
for the secondary process, instead of loops all BARs, the logic
using the 'uio_res->nb_map' as index. If the device uses continuous
BARs there will be no problem, whereas if it uses discrete BARs,
it will lead to mapping errors.

Fix this problem by also loops all BARs and skip the map of BAR
with an invalid physical address in secondary process.

Fixes: 9b957f378abf ("pci: merge uio functions for linux and bsd")
Cc: mukawa@igel.co.jp
Cc: stable@dpdk.org

Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Long Wu <long.wu@corigine.com>
Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
---
 drivers/bus/pci/pci_common_uio.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c
index caacbb69d9..432d68b12c 100644
--- a/drivers/bus/pci/pci_common_uio.c
+++ b/drivers/bus/pci/pci_common_uio.c
@@ -26,7 +26,7 @@ EAL_REGISTER_TAILQ(rte_uio_tailq)
 static int
 pci_uio_map_secondary(struct rte_pci_device *dev)
 {
-	int fd, i, map_idx;
+	int fd, i, map_idx = 0, res_idx;
 	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);
@@ -37,7 +37,15 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
 		if (rte_pci_addr_cmp(&uio_res->pci_addr, &dev->addr))
 			continue;
 
-		for (map_idx = 0; map_idx != uio_res->nb_maps; map_idx++) {
+		/* Map all BARs */
+		for (res_idx = 0; res_idx != PCI_MAX_RESOURCE; res_idx++) {
+			 /* skip empty BAR */
+			if (dev->mem_resource[res_idx].phys_addr == 0)
+				continue;
+
+			if (map_idx >= uio_res->nb_maps)
+				return -1;
+
 			/*
 			 * open devname, to mmap it
 			 */
@@ -71,7 +79,9 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
 				}
 				return -1;
 			}
-			dev->mem_resource[map_idx].addr = mapaddr;
+			dev->mem_resource[res_idx].addr = mapaddr;
+
+			map_idx++;
 		}
 		return 0;
 	}
-- 
2.39.1


^ permalink raw reply	[flat|nested] 26+ messages in thread
* [PATCH v4 3/3] bus/pci: fix secondary process save 'FD' problem
  2024-06-28  7:36     ` [PATCH v4 0/3] fix secondary process PCI UIO resource problem Chaoyong He
  2024-06-28  7:36       ` [PATCH v4 1/3] bus/pci: rename the variable in UIO secondary map logic Chaoyong He
  2024-06-28  7:36       ` [PATCH v4 2/3] bus/pci: fix secondary process PCI uio resource map problem Chaoyong He
@ 2024-06-28  7:36       ` Chaoyong He
  2024-07-01 14:14         ` David Marchand
  2024-07-01 14:12       ` [PATCH v4 0/3] fix secondary process PCI UIO resource problem David Marchand
  2024-07-02  2:19       ` [PATCH v5 " Chaoyong He
  4 siblings, 1 reply; 26+ messages in thread
From: Chaoyong He @ 2024-06-28  7:36 UTC (permalink / raw)
  To: dev
  Cc: oss-drivers, Zerun Fu, mukawa, stable, Chaoyong He, Long Wu, Peng Zhang

From: Zerun Fu <zerun.fu@corigine.com>

In the previous logic the 'fd' was only saved in the primary process,
but for some devices this value is also used in the secondary logic.

For example, the call of 'rte_pci_find_ext_capability()' will fail in
the secondary process.

Fix this problem by getting and saving the value of 'fd' also in the
secondary process logic.

Fixes: 9b957f378abf ("pci: merge uio functions for linux and bsd")
Cc: mukawa@igel.co.jp
Cc: stable@dpdk.org

Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Long Wu <long.wu@corigine.com>
Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
---
 drivers/bus/pci/linux/pci_uio.c  | 5 ++++-
 drivers/bus/pci/pci_common_uio.c | 8 ++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index 97d740dfe5..6680e42efb 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -237,7 +237,7 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
 	}
 	snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
 
-	/* save fd if in primary process */
+	/* save fd */
 	fd = open(devname, O_RDWR);
 	if (fd < 0) {
 		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
@@ -261,6 +261,9 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
 	if (rte_intr_dev_fd_set(dev->intr_handle, uio_cfg_fd))
 		goto error;
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return 0;
+
 	if (dev->kdrv == RTE_PCI_KDRV_IGB_UIO) {
 		if (rte_intr_type_set(dev->intr_handle, RTE_INTR_HANDLE_UIO))
 			goto error;
diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c
index 432d68b12c..8688862738 100644
--- a/drivers/bus/pci/pci_common_uio.c
+++ b/drivers/bus/pci/pci_common_uio.c
@@ -106,15 +106,15 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 	if (rte_intr_dev_fd_set(dev->intr_handle, -1))
 		return -1;
 
-	/* 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)
 		return ret;
 
+	/* secondary processes - use already recorded details */
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return pci_uio_map_secondary(dev);
+
 	/* Map all BARs */
 	for (i = 0; i != PCI_MAX_RESOURCE; i++) {
 		/* skip empty BAR */
-- 
2.39.1


^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/3] fix secondary process PCI UIO resource problem
  2024-06-28  7:36     ` [PATCH v4 0/3] fix secondary process PCI UIO resource problem Chaoyong He
                         ` (2 preceding siblings ...)
  2024-06-28  7:36       ` [PATCH v4 3/3] bus/pci: fix secondary process save 'FD' problem Chaoyong He
@ 2024-07-01 14:12       ` David Marchand
  2024-07-02  2:19       ` [PATCH v5 " Chaoyong He
  4 siblings, 0 replies; 26+ messages in thread
From: David Marchand @ 2024-07-01 14:12 UTC (permalink / raw)
  To: Chaoyong He; +Cc: dev, oss-drivers, Chenbo Xia, Nipun Gupta

On Fri, Jun 28, 2024 at 9:37 AM Chaoyong He <chaoyong.he@corigine.com> wrote:
>
> This patch series aims to fix some problems in secondary process PCI UIO
> resource map logic.
>
> ---
> v2:
> * Modify as the advice from reviewer.
> v3:
> * Modify logic as the comments of reviewers.
> v4:
> * Split the commits to make it easier to understand.
> ---
>
> Zerun Fu (3):
>   bus/pci: rename the variable in UIO secondary map logic
>   bus/pci: fix secondary process PCI uio resource map problem
>   bus/pci: fix secondary process save 'FD' problem
>
>  drivers/bus/pci/linux/pci_uio.c  |  5 +++-
>  drivers/bus/pci/pci_common_uio.c | 48 +++++++++++++++++++-------------
>  2 files changed, 33 insertions(+), 20 deletions(-)

Cc: maintainers.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH v4 3/3] bus/pci: fix secondary process save 'FD' problem
  2024-06-28  7:36       ` [PATCH v4 3/3] bus/pci: fix secondary process save 'FD' problem Chaoyong He
@ 2024-07-01 14:14         ` David Marchand
  2024-07-02  1:53           ` Chaoyong He
  0 siblings, 1 reply; 26+ messages in thread
From: David Marchand @ 2024-07-01 14:14 UTC (permalink / raw)
  To: Chaoyong He
  Cc: dev, oss-drivers, Zerun Fu, mukawa, stable, Long Wu, Peng Zhang

On Fri, Jun 28, 2024 at 9:37 AM Chaoyong He <chaoyong.he@corigine.com> wrote:
>
> From: Zerun Fu <zerun.fu@corigine.com>
>
> In the previous logic the 'fd' was only saved in the primary process,
> but for some devices this value is also used in the secondary logic.
>
> For example, the call of 'rte_pci_find_ext_capability()' will fail in
> the secondary process.
>
> Fix this problem by getting and saving the value of 'fd' also in the
> secondary process logic.
>
> Fixes: 9b957f378abf ("pci: merge uio functions for linux and bsd")
> Cc: mukawa@igel.co.jp
> Cc: stable@dpdk.org
>
> Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Long Wu <long.wu@corigine.com>
> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>

I did not enter in depth, but I wonder if this change leaks fd in a
secondary process when unmapping UIO resources.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 26+ messages in thread
* RE: [PATCH v4 3/3] bus/pci: fix secondary process save 'FD' problem
  2024-07-01 14:14         ` David Marchand
@ 2024-07-02  1:53           ` Chaoyong He
  0 siblings, 0 replies; 26+ messages in thread
From: Chaoyong He @ 2024-07-02  1:53 UTC (permalink / raw)
  To: David Marchand
  Cc: dev, oss-drivers, Zerun Fu, mukawa, stable, Long Wu, Nole Zhang

> On Fri, Jun 28, 2024 at 9:37 AM Chaoyong He <chaoyong.he@corigine.com>
> wrote:
> >
> > From: Zerun Fu <zerun.fu@corigine.com>
> >
> > In the previous logic the 'fd' was only saved in the primary process,
> > but for some devices this value is also used in the secondary logic.
> >
> > For example, the call of 'rte_pci_find_ext_capability()' will fail in
> > the secondary process.
> >
> > Fix this problem by getting and saving the value of 'fd' also in the
> > secondary process logic.
> >
> > Fixes: 9b957f378abf ("pci: merge uio functions for linux and bsd")
> > Cc: mukawa@igel.co.jp
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
> > Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> > Reviewed-by: Long Wu <long.wu@corigine.com>
> > Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
> 
> I did not enter in depth, but I wonder if this change leaks fd in a secondary
> process when unmapping UIO resources.
> 

Yeah, it does have problem when unmapping UIO resources, and we will send a new version patch to fix it soon.
Many thanks for your carefully review.

> 
> --
> David Marchand


^ permalink raw reply	[flat|nested] 26+ messages in thread
* [PATCH v5 0/3] fix secondary process PCI UIO resource problem
  2024-06-28  7:36     ` [PATCH v4 0/3] fix secondary process PCI UIO resource problem Chaoyong He
                         ` (3 preceding siblings ...)
  2024-07-01 14:12       ` [PATCH v4 0/3] fix secondary process PCI UIO resource problem David Marchand
@ 2024-07-02  2:19       ` Chaoyong He
  2024-07-02  2:19         ` [PATCH v5 1/3] bus/pci: rename the variable in UIO secondary map logic Chaoyong He
                           ` (2 more replies)
  4 siblings, 3 replies; 26+ messages in thread
From: Chaoyong He @ 2024-07-02  2:19 UTC (permalink / raw)
  To: dev; +Cc: oss-drivers, Chaoyong He

This patch series aims to fix some problems in secondary process PCI UIO
resource map logic.

---
v2:
* Modify as the advice from reviewer.
v3:
* Modify logic as the comments of reviewers.
v4:
* Split the commits to make it easier to understand.
v5:
* Fix the unmapping UIO resources probelm found by the reviewer.
---

Zerun Fu (3):
  bus/pci: rename the variable in UIO secondary map logic
  bus/pci: fix secondary process PCI uio resource map problem
  bus/pci: fix secondary process save 'FD' problem

 drivers/bus/pci/linux/pci_uio.c  |  5 ++-
 drivers/bus/pci/pci_common_uio.c | 72 ++++++++++++++++++--------------
 2 files changed, 45 insertions(+), 32 deletions(-)

-- 
2.39.1


^ permalink raw reply	[flat|nested] 26+ messages in thread
* [PATCH v5 1/3] bus/pci: rename the variable in UIO secondary map logic
  2024-07-02  2:19       ` [PATCH v5 " Chaoyong He
@ 2024-07-02  2:19         ` Chaoyong He
  2024-07-02  2:47           ` Stephen Hemminger
  2024-07-02  2:19         ` [PATCH v5 2/3] bus/pci: fix secondary process PCI uio resource map problem Chaoyong He
  2024-07-02  2:19         ` [PATCH v5 3/3] bus/pci: fix secondary process save 'FD' problem Chaoyong He
  2 siblings, 1 reply; 26+ messages in thread
From: Chaoyong He @ 2024-07-02  2:19 UTC (permalink / raw)
  To: dev; +Cc: oss-drivers, Zerun Fu, Chaoyong He, Long Wu, Peng Zhang

From: Zerun Fu <zerun.fu@corigine.com>

To makes the logic clearer, rename the variable 'i' into 'map_idx'
and the variable 'j' into 'i'.

Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Long Wu <long.wu@corigine.com>
Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
---
 drivers/bus/pci/pci_common_uio.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c
index 76c661f054..caacbb69d9 100644
--- a/drivers/bus/pci/pci_common_uio.c
+++ b/drivers/bus/pci/pci_common_uio.c
@@ -26,7 +26,7 @@ EAL_REGISTER_TAILQ(rte_uio_tailq)
 static int
 pci_uio_map_secondary(struct rte_pci_device *dev)
 {
-	int fd, i, j;
+	int fd, i, map_idx;
 	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);
@@ -37,41 +37,41 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
 		if (rte_pci_addr_cmp(&uio_res->pci_addr, &dev->addr))
 			continue;
 
-		for (i = 0; i != uio_res->nb_maps; i++) {
+		for (map_idx = 0; map_idx != uio_res->nb_maps; map_idx++) {
 			/*
 			 * open devname, to mmap it
 			 */
-			fd = open(uio_res->maps[i].path, O_RDWR);
+			fd = open(uio_res->maps[map_idx].path, O_RDWR);
 			if (fd < 0) {
 				RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
-					uio_res->maps[i].path, strerror(errno));
+					uio_res->maps[map_idx].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);
+			void *mapaddr = pci_map_resource(uio_res->maps[map_idx].addr,
+					fd, (off_t)uio_res->maps[map_idx].offset,
+					(size_t)uio_res->maps[map_idx].size, 0);
 
 			/* fd is not needed in secondary process, close it */
 			close(fd);
-			if (mapaddr != uio_res->maps[i].addr) {
+			if (mapaddr != uio_res->maps[map_idx].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);
+					uio_res->maps[map_idx].path,
+					uio_res->maps[map_idx].addr);
 				if (mapaddr != NULL) {
 					/* unmap addrs correctly mapped */
-					for (j = 0; j < i; j++)
+					for (i = 0; i < map_idx; i++)
 						pci_unmap_resource(
-							uio_res->maps[j].addr,
-							(size_t)uio_res->maps[j].size);
+							uio_res->maps[i].addr,
+							(size_t)uio_res->maps[i].size);
 					/* unmap addr wrongly mapped */
 					pci_unmap_resource(mapaddr,
-						(size_t)uio_res->maps[i].size);
+						(size_t)uio_res->maps[map_idx].size);
 				}
 				return -1;
 			}
-			dev->mem_resource[i].addr = mapaddr;
+			dev->mem_resource[map_idx].addr = mapaddr;
 		}
 		return 0;
 	}
-- 
2.39.1


^ permalink raw reply	[flat|nested] 26+ messages in thread
* [PATCH v5 2/3] bus/pci: fix secondary process PCI uio resource map problem
  2024-07-02  2:19       ` [PATCH v5 " Chaoyong He
  2024-07-02  2:19         ` [PATCH v5 1/3] bus/pci: rename the variable in UIO secondary map logic Chaoyong He
@ 2024-07-02  2:19         ` Chaoyong He
  2024-07-02  2:19         ` [PATCH v5 3/3] bus/pci: fix secondary process save 'FD' problem Chaoyong He
  2 siblings, 0 replies; 26+ messages in thread
From: Chaoyong He @ 2024-07-02  2:19 UTC (permalink / raw)
  To: dev
  Cc: oss-drivers, Zerun Fu, mukawa, stable, Chaoyong He, Long Wu, Peng Zhang

From: Zerun Fu <zerun.fu@corigine.com>

For the primary process, the logic loops all BARs and will skip
the map of BAR with an invalid physical address (0), also will
assign 'uio_res->nb_maps' with the real mapped BARs number. But
for the secondary process, instead of loops all BARs, the logic
using the 'uio_res->nb_map' as index. If the device uses continuous
BARs there will be no problem, whereas if it uses discrete BARs,
it will lead to mapping errors.

Fix this problem by also loops all BARs and skip the map of BAR
with an invalid physical address in secondary process.

Fixes: 9b957f378abf ("pci: merge uio functions for linux and bsd")
Cc: mukawa@igel.co.jp
Cc: stable@dpdk.org

Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Long Wu <long.wu@corigine.com>
Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
---
 drivers/bus/pci/pci_common_uio.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c
index caacbb69d9..432d68b12c 100644
--- a/drivers/bus/pci/pci_common_uio.c
+++ b/drivers/bus/pci/pci_common_uio.c
@@ -26,7 +26,7 @@ EAL_REGISTER_TAILQ(rte_uio_tailq)
 static int
 pci_uio_map_secondary(struct rte_pci_device *dev)
 {
-	int fd, i, map_idx;
+	int fd, i, map_idx = 0, res_idx;
 	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);
@@ -37,7 +37,15 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
 		if (rte_pci_addr_cmp(&uio_res->pci_addr, &dev->addr))
 			continue;
 
-		for (map_idx = 0; map_idx != uio_res->nb_maps; map_idx++) {
+		/* Map all BARs */
+		for (res_idx = 0; res_idx != PCI_MAX_RESOURCE; res_idx++) {
+			 /* skip empty BAR */
+			if (dev->mem_resource[res_idx].phys_addr == 0)
+				continue;
+
+			if (map_idx >= uio_res->nb_maps)
+				return -1;
+
 			/*
 			 * open devname, to mmap it
 			 */
@@ -71,7 +79,9 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
 				}
 				return -1;
 			}
-			dev->mem_resource[map_idx].addr = mapaddr;
+			dev->mem_resource[res_idx].addr = mapaddr;
+
+			map_idx++;
 		}
 		return 0;
 	}
-- 
2.39.1


^ permalink raw reply	[flat|nested] 26+ messages in thread
* [PATCH v5 3/3] bus/pci: fix secondary process save 'FD' problem
  2024-07-02  2:19       ` [PATCH v5 " Chaoyong He
  2024-07-02  2:19         ` [PATCH v5 1/3] bus/pci: rename the variable in UIO secondary map logic Chaoyong He
  2024-07-02  2:19         ` [PATCH v5 2/3] bus/pci: fix secondary process PCI uio resource map problem Chaoyong He
@ 2024-07-02  2:19         ` Chaoyong He
  2 siblings, 0 replies; 26+ messages in thread
From: Chaoyong He @ 2024-07-02  2:19 UTC (permalink / raw)
  To: dev
  Cc: oss-drivers, Zerun Fu, mukawa, stable, Chaoyong He, Long Wu, Peng Zhang

From: Zerun Fu <zerun.fu@corigine.com>

In the previous logic the 'fd' was only saved in the primary process,
but for some devices this value is also used in the secondary logic.

For example, the call of 'rte_pci_find_ext_capability()' will fail in
the secondary process.

Fix this problem by getting and saving the value of 'fd' also in the
secondary process logic.

Fixes: 9b957f378abf ("pci: merge uio functions for linux and bsd")
Cc: mukawa@igel.co.jp
Cc: stable@dpdk.org

Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Long Wu <long.wu@corigine.com>
Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
---
 drivers/bus/pci/linux/pci_uio.c  |  5 ++++-
 drivers/bus/pci/pci_common_uio.c | 32 ++++++++++++++++----------------
 2 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index 97d740dfe5..4afda97858 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -237,7 +237,7 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
 	}
 	snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
 
-	/* save fd if in primary process */
+	/* save fd */
 	fd = open(devname, O_RDWR);
 	if (fd < 0) {
 		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
@@ -275,6 +275,9 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
 		}
 	}
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return 0;
+
 	/* allocate the mapping details for secondary processes*/
 	*uio_res = rte_zmalloc("UIO_RES", sizeof(**uio_res), 0);
 	if (*uio_res == NULL) {
diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c
index 432d68b12c..09f30bee6c 100644
--- a/drivers/bus/pci/pci_common_uio.c
+++ b/drivers/bus/pci/pci_common_uio.c
@@ -106,15 +106,15 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 	if (rte_intr_dev_fd_set(dev->intr_handle, -1))
 		return -1;
 
-	/* 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)
 		return ret;
 
+	/* secondary processes - use already recorded details */
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return pci_uio_map_secondary(dev);
+
 	/* Map all BARs */
 	for (i = 0; i != PCI_MAX_RESOURCE; i++) {
 		/* skip empty BAR */
@@ -230,6 +230,18 @@ pci_uio_unmap_resource(struct rte_pci_device *dev)
 	if (uio_res == NULL)
 		return;
 
+	/* close fd */
+	if (rte_intr_fd_get(dev->intr_handle) >= 0)
+		close(rte_intr_fd_get(dev->intr_handle));
+	uio_cfg_fd = rte_intr_dev_fd_get(dev->intr_handle);
+	if (uio_cfg_fd >= 0) {
+		close(uio_cfg_fd);
+		rte_intr_dev_fd_set(dev->intr_handle, -1);
+	}
+
+	rte_intr_fd_set(dev->intr_handle, -1);
+	rte_intr_type_set(dev->intr_handle, RTE_INTR_HANDLE_UNKNOWN);
+
 	/* secondary processes - just free maps */
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return pci_uio_unmap(uio_res);
@@ -241,16 +253,4 @@ pci_uio_unmap_resource(struct rte_pci_device *dev)
 
 	/* free uio resource */
 	rte_free(uio_res);
-
-	/* close fd if in primary process */
-	if (rte_intr_fd_get(dev->intr_handle) >= 0)
-		close(rte_intr_fd_get(dev->intr_handle));
-	uio_cfg_fd = rte_intr_dev_fd_get(dev->intr_handle);
-	if (uio_cfg_fd >= 0) {
-		close(uio_cfg_fd);
-		rte_intr_dev_fd_set(dev->intr_handle, -1);
-	}
-
-	rte_intr_fd_set(dev->intr_handle, -1);
-	rte_intr_type_set(dev->intr_handle, RTE_INTR_HANDLE_UNKNOWN);
 }
-- 
2.39.1


^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH v5 1/3] bus/pci: rename the variable in UIO secondary map logic
  2024-07-02  2:19         ` [PATCH v5 1/3] bus/pci: rename the variable in UIO secondary map logic Chaoyong He
@ 2024-07-02  2:47           ` Stephen Hemminger
  0 siblings, 0 replies; 26+ messages in thread
From: Stephen Hemminger @ 2024-07-02  2:47 UTC (permalink / raw)
  To: Chaoyong He; +Cc: dev, oss-drivers, Zerun Fu, Long Wu, Peng Zhang

On Tue,  2 Jul 2024 10:19:44 +0800
Chaoyong He <chaoyong.he@corigine.com> wrote:

> -	int fd, i, j;
> +	int fd, i, map_idx;
>  	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);
> @@ -37,41 +37,41 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
>  		if (rte_pci_addr_cmp(&uio_res->pci_addr, &dev->addr))
>  			continue;
>  
> -		for (i = 0; i != uio_res->nb_maps; i++) {
> +		for (map_idx = 0; map_idx != uio_res->nb_maps; map_idx++) {
>  			/*
>  			 * open devname, to mmap it

Renaming i to map_idx and j to i does not change anything and makes the change
harder to read.

^ permalink raw reply	[flat|nested] 26+ messages in thread
end of thread, other threads:[~2024-07-02  2:48 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-24 10:45 [PATCH] bus/pci: fix secondary process PCI uio resource map problem Chaoyong He
2024-01-29  9:22 ` [PATCH v2 0/2] fix secondary process PCI UIO resource problem Chaoyong He
2024-01-29  9:22   ` [PATCH v2 1/2] bus/pci: fix secondary process PCI uio resource map problem Chaoyong He
2024-01-30  4:00     ` fengchengwen
2024-03-14 10:55     ` Burakov, Anatoly
2024-01-29  9:22   ` [PATCH v2 2/2] bus/pci: fix secondary process save 'FD' problem Chaoyong He
2024-01-30  6:16     ` fengchengwen
2024-03-14 11:01     ` Burakov, Anatoly
2024-03-11  6:42   ` [PATCH v2 0/2] fix secondary process PCI UIO resource problem Chaoyong He
2024-04-19  3:26   ` Chaoyong He
2024-04-19  3:26     ` [PATCH v3 1/2] bus/pci: fix secondary process PCI uio resource map problem Chaoyong He
2024-06-27 14:00       ` Thomas Monjalon
2024-06-28  1:03         ` Chaoyong He
2024-04-19  3:26     ` [PATCH v3 2/2] bus/pci: fix secondary process save 'FD' problem Chaoyong He
2024-06-28  7:36     ` [PATCH v4 0/3] fix secondary process PCI UIO resource problem Chaoyong He
2024-06-28  7:36       ` [PATCH v4 1/3] bus/pci: rename the variable in UIO secondary map logic Chaoyong He
2024-06-28  7:36       ` [PATCH v4 2/3] bus/pci: fix secondary process PCI uio resource map problem Chaoyong He
2024-06-28  7:36       ` [PATCH v4 3/3] bus/pci: fix secondary process save 'FD' problem Chaoyong He
2024-07-01 14:14         ` David Marchand
2024-07-02  1:53           ` Chaoyong He
2024-07-01 14:12       ` [PATCH v4 0/3] fix secondary process PCI UIO resource problem David Marchand
2024-07-02  2:19       ` [PATCH v5 " Chaoyong He
2024-07-02  2:19         ` [PATCH v5 1/3] bus/pci: rename the variable in UIO secondary map logic Chaoyong He
2024-07-02  2:47           ` Stephen Hemminger
2024-07-02  2:19         ` [PATCH v5 2/3] bus/pci: fix secondary process PCI uio resource map problem Chaoyong He
2024-07-02  2:19         ` [PATCH v5 3/3] bus/pci: fix secondary process save 'FD' problem Chaoyong He

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).