[PATCH 1/3] net/ark: support secondary process

[PATCH 1/3] net/ark: support secondary process

[Ed Czeck]

From: John Miller <john.miller@atomicrules.com>

disable device configuration for secondary processes

Signed-off-by: John Miller <john.miller@atomicrules.com>
---
 drivers/net/ark/ark_ethdev.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ark/ark_ethdev.c b/drivers/net/ark/ark_ethdev.c
index b2995427c8..f96722551e 100644
--- a/drivers/net/ark/ark_ethdev.c
+++ b/drivers/net/ark/ark_ethdev.c
@@ -147,6 +147,9 @@ eth_ark_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	struct rte_eth_dev *eth_dev;
 	int ret;
 
+	if (rte_eal_process_type() == RTE_PROC_SECONDARY)
+		fprintf(stderr, "ARK probed by secondary process\n");
+
 	eth_dev = rte_eth_dev_pci_allocate(pci_dev, sizeof(struct ark_adapter));
 
 	if (eth_dev == NULL)
@@ -385,9 +388,11 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
 		    0xcafef00d, ark->sysctrl.t32[4], __func__);
 
 	/* We are a single function multi-port device. */
-	ret = ark_config_device(dev);
-	if (ret)
-		return -1;
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		ret = ark_config_device(dev);
+		if (ret)
+			return -1;
+	}
 
 	dev->dev_ops = &ark_eth_dev_ops;
 	dev->rx_queue_count = eth_ark_dev_rx_queue_count;
-- 
2.34.1

[PATCH 2/3] net/ark: support for single function with multiple port

[Ed Czeck]

allows the creation of multiple ports from one ark device via
the use of ark pmd extension, though the splitting of queues
Add unique dev_private data for each port

Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
---
 drivers/net/ark/ark_ethdev.c    | 14 +++++++++++++-
 drivers/net/ark/ark_ethdev_rx.c |  6 +++---
 drivers/net/ark/ark_ethdev_tx.c |  2 +-
 drivers/net/ark/ark_global.h    |  4 ++++
 4 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ark/ark_ethdev.c b/drivers/net/ark/ark_ethdev.c
index f96722551e..2fff3cd6ba 100644
--- a/drivers/net/ark/ark_ethdev.c
+++ b/drivers/net/ark/ark_ethdev.c
@@ -432,6 +432,7 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
 			ark->user_ext.dev_get_port_count(dev,
 				 ark->user_data[dev->data->port_id]);
 	ark->num_ports = port_count;
+	ark->num_queues = ark_api_num_queues_per_port(ark->mpurx.v, port_count);
 
 	for (p = 0; p < port_count; p++) {
 		struct rte_eth_dev *eth_dev;
@@ -457,7 +458,18 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
 		}
 
 		eth_dev->device = &pci_dev->device;
-		eth_dev->data->dev_private = ark;
+		/* Device requires new dev_private data */
+		eth_dev->data->dev_private =
+			rte_zmalloc_socket(name,
+					   sizeof(struct ark_adapter),
+					   RTE_CACHE_LINE_SIZE,
+					   rte_socket_id());
+
+		memcpy(eth_dev->data->dev_private, ark,
+		       sizeof(struct ark_adapter));
+		ark = eth_dev->data->dev_private;
+		ark->qbase = p * ark->num_queues;
+
 		eth_dev->dev_ops = ark->eth_dev->dev_ops;
 		eth_dev->tx_pkt_burst = ark->eth_dev->tx_pkt_burst;
 		eth_dev->rx_pkt_burst = ark->eth_dev->rx_pkt_burst;
diff --git a/drivers/net/ark/ark_ethdev_rx.c b/drivers/net/ark/ark_ethdev_rx.c
index cbc0416bc2..38bc69dff4 100644
--- a/drivers/net/ark/ark_ethdev_rx.c
+++ b/drivers/net/ark/ark_ethdev_rx.c
@@ -68,7 +68,7 @@ struct ark_rx_queue {
 static int
 eth_ark_rx_hw_setup(struct rte_eth_dev *dev,
 		    struct ark_rx_queue *queue,
-		    uint16_t rx_queue_id __rte_unused, uint16_t rx_queue_idx)
+		    uint16_t rx_queue_idx)
 {
 	rte_iova_t queue_base;
 	rte_iova_t phys_addr_q_base;
@@ -124,7 +124,7 @@ eth_ark_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	uint32_t i;
 	int status;
 
-	int qidx = queue_idx;
+	int qidx = ark->qbase + queue_idx;
 
 	/* We may already be setup, free memory prior to re-allocation */
 	if (dev->data->rx_queues[queue_idx] != NULL) {
@@ -215,7 +215,7 @@ eth_ark_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	}
 	/* MPU Setup */
 	if (status == 0)
-		status = eth_ark_rx_hw_setup(dev, queue, qidx, queue_idx);
+		status = eth_ark_rx_hw_setup(dev, queue, queue_idx);
 
 	if (unlikely(status != 0)) {
 		struct rte_mbuf **mbuf;
diff --git a/drivers/net/ark/ark_ethdev_tx.c b/drivers/net/ark/ark_ethdev_tx.c
index 5940a592a2..4792754f19 100644
--- a/drivers/net/ark/ark_ethdev_tx.c
+++ b/drivers/net/ark/ark_ethdev_tx.c
@@ -229,7 +229,7 @@ eth_ark_tx_queue_setup(struct rte_eth_dev *dev,
 	struct ark_tx_queue *queue;
 	int status;
 
-	int qidx = queue_idx;
+	int qidx = ark->qbase + queue_idx;
 
 	if (!rte_is_power_of_2(nb_desc)) {
 		ARK_PMD_LOG(ERR,
diff --git a/drivers/net/ark/ark_global.h b/drivers/net/ark/ark_global.h
index 71d0b53e03..176fbcda17 100644
--- a/drivers/net/ark/ark_global.h
+++ b/drivers/net/ark/ark_global.h
@@ -112,7 +112,11 @@ struct ark_adapter {
 	ark_pkt_chkr_t pc;
 	ark_pkt_dir_t pd;
 
+	/* For single function, multiple ports */
 	int num_ports;
+	uint16_t qbase;
+	uint16_t num_queues;
+
 	bool isvf;
 
 	/* Packet generator/checker args */
-- 
2.34.1

[PATCH 3/3] net/ark: resize data field to match fpga access

[Ed Czeck]

all fpga accesses are multiples of 4 bytes

Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
---
 drivers/net/ark/ark_mpu.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ark/ark_mpu.h b/drivers/net/ark/ark_mpu.h
index 9d2b70d35f..10842ba484 100644
--- a/drivers/net/ark/ark_mpu.h
+++ b/drivers/net/ark/ark_mpu.h
@@ -39,8 +39,7 @@ struct ark_mpu_id_t {
 
 #define ARK_MPU_HW 0x010
 struct ark_mpu_hw_t {
-	uint16_t num_queues;
-	uint16_t reserved;
+	uint32_t num_queues;
 	uint32_t hw_depth;
 	uint32_t obj_size;
 	uint32_t obj_per_mrr;
-- 
2.34.1

[PATCH] net/ark: resize data field to match fpga access

[Ed Czeck]

all fpga accesses are multiples of 4 bytes

Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
---
 drivers/net/ark/ark_mpu.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ark/ark_mpu.h b/drivers/net/ark/ark_mpu.h
index 9d2b70d35f..10842ba484 100644
--- a/drivers/net/ark/ark_mpu.h
+++ b/drivers/net/ark/ark_mpu.h
@@ -39,8 +39,7 @@ struct ark_mpu_id_t {
 
 #define ARK_MPU_HW 0x010
 struct ark_mpu_hw_t {
-	uint16_t num_queues;
-	uint16_t reserved;
+	uint32_t num_queues;
 	uint32_t hw_depth;
 	uint32_t obj_size;
 	uint32_t obj_per_mrr;
-- 
2.34.1

Re: [PATCH] net/ark: resize data field to match fpga access

[Ferruh Yigit]

On 2/17/2023 4:00 PM, Ed Czeck wrote:
> all fpga accesses are multiples of 4 bytes
> 
> Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>

Applied to dpdk-next-net/main, thanks.

[PATCH v2 1/3] net/ark: support secondary process

[Ed Czeck]

From: John Miller <john.miller@atomicrules.com>

disable device configuration for secondary processes

Signed-off-by: John Miller <john.miller@atomicrules.com>
---
v2:
* Use standard logging
---
 drivers/net/ark/ark_ethdev.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ark/ark_ethdev.c b/drivers/net/ark/ark_ethdev.c
index b2995427c8..d237e80cf4 100644
--- a/drivers/net/ark/ark_ethdev.c
+++ b/drivers/net/ark/ark_ethdev.c
@@ -147,6 +147,9 @@ eth_ark_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	struct rte_eth_dev *eth_dev;
 	int ret;
 
+	if (rte_eal_process_type() == RTE_PROC_SECONDARY)
+		ARK_PMD_LOG(DEBUG, "ARK probed by secondary process\n");
+
 	eth_dev = rte_eth_dev_pci_allocate(pci_dev, sizeof(struct ark_adapter));
 
 	if (eth_dev == NULL)
@@ -385,9 +388,11 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
 		    0xcafef00d, ark->sysctrl.t32[4], __func__);
 
 	/* We are a single function multi-port device. */
-	ret = ark_config_device(dev);
-	if (ret)
-		return -1;
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		ret = ark_config_device(dev);
+		if (ret)
+			return -1;
+	}
 
 	dev->dev_ops = &ark_eth_dev_ops;
 	dev->rx_queue_count = eth_ark_dev_rx_queue_count;
-- 
2.34.1

[PATCH v2 2/3] net/ark: support for single function with multiple port

[Ed Czeck]

allows the creation of multiple ports from one ark device via
the use of ark pmd extension, though the splitting of queues
Add unique dev_private data for each port

Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
---
 drivers/net/ark/ark_ethdev.c    | 14 +++++++++++++-
 drivers/net/ark/ark_ethdev_rx.c |  6 +++---
 drivers/net/ark/ark_ethdev_tx.c |  2 +-
 drivers/net/ark/ark_global.h    |  4 ++++
 4 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ark/ark_ethdev.c b/drivers/net/ark/ark_ethdev.c
index d237e80cf4..96d0c2b0f0 100644
--- a/drivers/net/ark/ark_ethdev.c
+++ b/drivers/net/ark/ark_ethdev.c
@@ -432,6 +432,7 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
 			ark->user_ext.dev_get_port_count(dev,
 				 ark->user_data[dev->data->port_id]);
 	ark->num_ports = port_count;
+	ark->num_queues = ark_api_num_queues_per_port(ark->mpurx.v, port_count);
 
 	for (p = 0; p < port_count; p++) {
 		struct rte_eth_dev *eth_dev;
@@ -457,7 +458,18 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
 		}
 
 		eth_dev->device = &pci_dev->device;
-		eth_dev->data->dev_private = ark;
+		/* Device requires new dev_private data */
+		eth_dev->data->dev_private =
+			rte_zmalloc_socket(name,
+					   sizeof(struct ark_adapter),
+					   RTE_CACHE_LINE_SIZE,
+					   rte_socket_id());
+
+		memcpy(eth_dev->data->dev_private, ark,
+		       sizeof(struct ark_adapter));
+		ark = eth_dev->data->dev_private;
+		ark->qbase = p * ark->num_queues;
+
 		eth_dev->dev_ops = ark->eth_dev->dev_ops;
 		eth_dev->tx_pkt_burst = ark->eth_dev->tx_pkt_burst;
 		eth_dev->rx_pkt_burst = ark->eth_dev->rx_pkt_burst;
diff --git a/drivers/net/ark/ark_ethdev_rx.c b/drivers/net/ark/ark_ethdev_rx.c
index cbc0416bc2..38bc69dff4 100644
--- a/drivers/net/ark/ark_ethdev_rx.c
+++ b/drivers/net/ark/ark_ethdev_rx.c
@@ -68,7 +68,7 @@ struct ark_rx_queue {
 static int
 eth_ark_rx_hw_setup(struct rte_eth_dev *dev,
 		    struct ark_rx_queue *queue,
-		    uint16_t rx_queue_id __rte_unused, uint16_t rx_queue_idx)
+		    uint16_t rx_queue_idx)
 {
 	rte_iova_t queue_base;
 	rte_iova_t phys_addr_q_base;
@@ -124,7 +124,7 @@ eth_ark_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	uint32_t i;
 	int status;
 
-	int qidx = queue_idx;
+	int qidx = ark->qbase + queue_idx;
 
 	/* We may already be setup, free memory prior to re-allocation */
 	if (dev->data->rx_queues[queue_idx] != NULL) {
@@ -215,7 +215,7 @@ eth_ark_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	}
 	/* MPU Setup */
 	if (status == 0)
-		status = eth_ark_rx_hw_setup(dev, queue, qidx, queue_idx);
+		status = eth_ark_rx_hw_setup(dev, queue, queue_idx);
 
 	if (unlikely(status != 0)) {
 		struct rte_mbuf **mbuf;
diff --git a/drivers/net/ark/ark_ethdev_tx.c b/drivers/net/ark/ark_ethdev_tx.c
index 5940a592a2..4792754f19 100644
--- a/drivers/net/ark/ark_ethdev_tx.c
+++ b/drivers/net/ark/ark_ethdev_tx.c
@@ -229,7 +229,7 @@ eth_ark_tx_queue_setup(struct rte_eth_dev *dev,
 	struct ark_tx_queue *queue;
 	int status;
 
-	int qidx = queue_idx;
+	int qidx = ark->qbase + queue_idx;
 
 	if (!rte_is_power_of_2(nb_desc)) {
 		ARK_PMD_LOG(ERR,
diff --git a/drivers/net/ark/ark_global.h b/drivers/net/ark/ark_global.h
index 71d0b53e03..176fbcda17 100644
--- a/drivers/net/ark/ark_global.h
+++ b/drivers/net/ark/ark_global.h
@@ -112,7 +112,11 @@ struct ark_adapter {
 	ark_pkt_chkr_t pc;
 	ark_pkt_dir_t pd;
 
+	/* For single function, multiple ports */
 	int num_ports;
+	uint16_t qbase;
+	uint16_t num_queues;
+
 	bool isvf;
 
 	/* Packet generator/checker args */
-- 
2.34.1

[PATCH v2 3/3] net/ark: resize data field to match fpga access

[Ed Czeck]

all fpga accesses are multiples of 4 bytes

Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
---
 drivers/net/ark/ark_mpu.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ark/ark_mpu.h b/drivers/net/ark/ark_mpu.h
index 9d2b70d35f..10842ba484 100644
--- a/drivers/net/ark/ark_mpu.h
+++ b/drivers/net/ark/ark_mpu.h
@@ -39,8 +39,7 @@ struct ark_mpu_id_t {
 
 #define ARK_MPU_HW 0x010
 struct ark_mpu_hw_t {
-	uint16_t num_queues;
-	uint16_t reserved;
+	uint32_t num_queues;
 	uint32_t hw_depth;
 	uint32_t obj_size;
 	uint32_t obj_per_mrr;
-- 
2.34.1

Re: [PATCH v2 1/3] net/ark: support secondary process

[Ferruh Yigit]

On 2/17/2023 9:59 PM, Ed Czeck wrote:
> From: John Miller <john.miller@atomicrules.com>
> 
> disable device configuration for secondary processes
> 
> Signed-off-by: John Miller <john.miller@atomicrules.com>
> ---
> v2:
> * Use standard logging
> ---
>  drivers/net/ark/ark_ethdev.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ark/ark_ethdev.c b/drivers/net/ark/ark_ethdev.c
> index b2995427c8..d237e80cf4 100644
> --- a/drivers/net/ark/ark_ethdev.c
> +++ b/drivers/net/ark/ark_ethdev.c
> @@ -147,6 +147,9 @@ eth_ark_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>  	struct rte_eth_dev *eth_dev;
>  	int ret;
>  
> +	if (rte_eal_process_type() == RTE_PROC_SECONDARY)
> +		ARK_PMD_LOG(DEBUG, "ARK probed by secondary process\n");
> +
>  	eth_dev = rte_eth_dev_pci_allocate(pci_dev, sizeof(struct ark_adapter));
>  
>  	if (eth_dev == NULL)
> @@ -385,9 +388,11 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
>  		    0xcafef00d, ark->sysctrl.t32[4], __func__);
>  
>  	/* We are a single function multi-port device. */
> -	ret = ark_config_device(dev);
> -	if (ret)
> -		return -1;
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +		ret = ark_config_device(dev);
> +		if (ret)
> +			return -1;
> +	}


Hi Ed,

As far as I can see both primary and secondary process continues to run
after this point, and below there are a few places that updates
'eth_dev->data'.

'eth_dev->data' is shared between primary and secondaries, so each
secondary will be overwriting the shared data.
Better usage is shared data only updated by primary process and
secondary processes use available values.
But 'eth_dev' is process specific and all primary and shared processes
must set fields of this struct.

You may need to re-order calls in function to make secondary quit after
'eth_dev' fields updated and before 'eth_dev->data' updated, to make
sure secondaries don't update shared data.

>  
>  	dev->dev_ops = &ark_eth_dev_ops;
>  	dev->rx_queue_count = eth_ark_dev_rx_queue_count;

Re: [PATCH v2 2/3] net/ark: support for single function with multiple port

[Ferruh Yigit]

On 2/17/2023 9:59 PM, Ed Czeck wrote:
> allows the creation of multiple ports from one ark device via
> the use of ark pmd extension, though the splitting of queues

Hi Ed,

As far as I can see "single function with multiple port" support was
already there but this commit is fixing queue index usage for it, if
correct can you please update commit log accordingly? (with fixes line
etc..)
This also helps the fix to be backported to LTS versions.


btw, how this feature was working until now, when queue ids "0 to
ark_api_num_queues_per_port()" used for each port?
Was it not tested at all, or is there something changed in FW causing
this issue, if so is there any FW version dependency to this change?

>
> Add unique dev_private data for each port
> 

Please check comment below related to this one.

>
> Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
> ---
>  drivers/net/ark/ark_ethdev.c    | 14 +++++++++++++-
>  drivers/net/ark/ark_ethdev_rx.c |  6 +++---
>  drivers/net/ark/ark_ethdev_tx.c |  2 +-
>  drivers/net/ark/ark_global.h    |  4 ++++
>  4 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ark/ark_ethdev.c b/drivers/net/ark/ark_ethdev.c
> index d237e80cf4..96d0c2b0f0 100644
> --- a/drivers/net/ark/ark_ethdev.c
> +++ b/drivers/net/ark/ark_ethdev.c
> @@ -432,6 +432,7 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
>  			ark->user_ext.dev_get_port_count(dev,
>  				 ark->user_data[dev->data->port_id]);
>  	ark->num_ports = port_count;
> +	ark->num_queues = ark_api_num_queues_per_port(ark->mpurx.v, port_count);
>  
>  	for (p = 0; p < port_count; p++) {
>  		struct rte_eth_dev *eth_dev;
> @@ -457,7 +458,18 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
>  		}
>  
>  		eth_dev->device = &pci_dev->device;
> -		eth_dev->data->dev_private = ark;
> +		/* Device requires new dev_private data */
> +		eth_dev->data->dev_private =
> +			rte_zmalloc_socket(name,
> +					   sizeof(struct ark_adapter),
> +					   RTE_CACHE_LINE_SIZE,
> +					   rte_socket_id());
> +
> +		memcpy(eth_dev->data->dev_private, ark,
> +		       sizeof(struct ark_adapter))> +		ark = eth_dev->data->dev_private;
> +		ark->qbase = p * ark->num_queues;
> +

These each are a new eth_dev, so nothing wrong for each to allocate
device private data, but if the only difference in private data is
'ark->qbase', it is possible to use 'eth_dev->process_private' for it,
which is per eth_dev. It is up to you.


Btw, how there are handled in the secondary process? (previous patch)
Since this code just creates new eth_dev, shouldn't secondary process
needs some code to find and re-use them?

>  		eth_dev->dev_ops = ark->eth_dev->dev_ops;
>  		eth_dev->tx_pkt_burst = ark->eth_dev->tx_pkt_burst;
>  		eth_dev->rx_pkt_burst = ark->eth_dev->rx_pkt_burst;
> diff --git a/drivers/net/ark/ark_ethdev_rx.c b/drivers/net/ark/ark_ethdev_rx.c
> index cbc0416bc2..38bc69dff4 100644
> --- a/drivers/net/ark/ark_ethdev_rx.c
> +++ b/drivers/net/ark/ark_ethdev_rx.c
> @@ -68,7 +68,7 @@ struct ark_rx_queue {
>  static int
>  eth_ark_rx_hw_setup(struct rte_eth_dev *dev,
>  		    struct ark_rx_queue *queue,
> -		    uint16_t rx_queue_id __rte_unused, uint16_t rx_queue_idx)
> +		    uint16_t rx_queue_idx)
>  {
>  	rte_iova_t queue_base;
>  	rte_iova_t phys_addr_q_base;
> @@ -124,7 +124,7 @@ eth_ark_dev_rx_queue_setup(struct rte_eth_dev *dev,
>  	uint32_t i;
>  	int status;
>  
> -	int qidx = queue_idx;
> +	int qidx = ark->qbase + queue_idx;
>  
>  	/* We may already be setup, free memory prior to re-allocation */
>  	if (dev->data->rx_queues[queue_idx] != NULL) {
> @@ -215,7 +215,7 @@ eth_ark_dev_rx_queue_setup(struct rte_eth_dev *dev,
>  	}
>  	/* MPU Setup */
>  	if (status == 0)
> -		status = eth_ark_rx_hw_setup(dev, queue, qidx, queue_idx);
> +		status = eth_ark_rx_hw_setup(dev, queue, queue_idx);
>  
>  	if (unlikely(status != 0)) {
>  		struct rte_mbuf **mbuf;
> diff --git a/drivers/net/ark/ark_ethdev_tx.c b/drivers/net/ark/ark_ethdev_tx.c
> index 5940a592a2..4792754f19 100644
> --- a/drivers/net/ark/ark_ethdev_tx.c
> +++ b/drivers/net/ark/ark_ethdev_tx.c
> @@ -229,7 +229,7 @@ eth_ark_tx_queue_setup(struct rte_eth_dev *dev,
>  	struct ark_tx_queue *queue;
>  	int status;
>  
> -	int qidx = queue_idx;
> +	int qidx = ark->qbase + queue_idx;
>  
>  	if (!rte_is_power_of_2(nb_desc)) {
>  		ARK_PMD_LOG(ERR,
> diff --git a/drivers/net/ark/ark_global.h b/drivers/net/ark/ark_global.h
> index 71d0b53e03..176fbcda17 100644
> --- a/drivers/net/ark/ark_global.h
> +++ b/drivers/net/ark/ark_global.h
> @@ -112,7 +112,11 @@ struct ark_adapter {
>  	ark_pkt_chkr_t pc;
>  	ark_pkt_dir_t pd;
>  
> +	/* For single function, multiple ports */
>  	int num_ports;
> +	uint16_t qbase;
> +	uint16_t num_queues;


it looks like 'num_queues' only used locally and not needed to be part
of device data, unless there is some more usage planned for future.

> +
>  	bool isvf;
>  
>  	/* Packet generator/checker args */

Re: [PATCH v2 1/3] net/ark: support secondary process

[Ed Czeck]

[-- Attachment #1: Type: text/plain, Size: 2665 bytes --]

Hi ferruh,
We have limited support for secondary processes.  This patch simply avoids
corrupting the FPGA state if a secondary process attaches.
Improved support for secondary processes is on our list, but we need a
strong customer driver for this feature.
An update patch is following soon.
Thanks for the review.
Ed.

On Mon, Feb 20, 2023 at 9:17 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> On 2/17/2023 9:59 PM, Ed Czeck wrote:
> > From: John Miller <john.miller@atomicrules.com>
> >
> > disable device configuration for secondary processes
> >
> > Signed-off-by: John Miller <john.miller@atomicrules.com>
> > ---
> > v2:
> > * Use standard logging
> > ---
> >  drivers/net/ark/ark_ethdev.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ark/ark_ethdev.c b/drivers/net/ark/ark_ethdev.c
> > index b2995427c8..d237e80cf4 100644
> > --- a/drivers/net/ark/ark_ethdev.c
> > +++ b/drivers/net/ark/ark_ethdev.c
> > @@ -147,6 +147,9 @@ eth_ark_pci_probe(struct rte_pci_driver *pci_drv
> __rte_unused,
> >       struct rte_eth_dev *eth_dev;
> >       int ret;
> >
> > +     if (rte_eal_process_type() == RTE_PROC_SECONDARY)
> > +             ARK_PMD_LOG(DEBUG, "ARK probed by secondary process\n");
> > +
> >       eth_dev = rte_eth_dev_pci_allocate(pci_dev, sizeof(struct
> ark_adapter));
> >
> >       if (eth_dev == NULL)
> > @@ -385,9 +388,11 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
> >                   0xcafef00d, ark->sysctrl.t32[4], __func__);
> >
> >       /* We are a single function multi-port device. */
> > -     ret = ark_config_device(dev);
> > -     if (ret)
> > -             return -1;
> > +     if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> > +             ret = ark_config_device(dev);
> > +             if (ret)
> > +                     return -1;
> > +     }
>
>
> Hi Ed,
>
> As far as I can see both primary and secondary process continues to run
> after this point, and below there are a few places that updates
> 'eth_dev->data'.
>
> 'eth_dev->data' is shared between primary and secondaries, so each
> secondary will be overwriting the shared data.
> Better usage is shared data only updated by primary process and
> secondary processes use available values.
> But 'eth_dev' is process specific and all primary and shared processes
> must set fields of this struct.
>
> You may need to re-order calls in function to make secondary quit after
> 'eth_dev' fields updated and before 'eth_dev->data' updated, to make
> sure secondaries don't update shared data.
>
> >
> >       dev->dev_ops = &ark_eth_dev_ops;
> >       dev->rx_queue_count = eth_ark_dev_rx_queue_count;
>
>

[-- Attachment #2: Type: text/html, Size: 3658 bytes --]

Re: [PATCH v2 2/3] net/ark: support for single function with multiple port

[Ed Czeck]

[-- Attachment #1: Type: text/plain, Size: 5988 bytes --]

Hi Ferruh,
We had looked at providing this patch as an LTS backport, but there are
some ties to FPGA firmware which complicate its application.  As this is a
uncommon net/ark feature we support users on an as-needed basis.
Thanks.
Ed.

On Mon, Feb 20, 2023 at 9:54 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> On 2/17/2023 9:59 PM, Ed Czeck wrote:
> > allows the creation of multiple ports from one ark device via
> > the use of ark pmd extension, though the splitting of queues
>
> Hi Ed,
>
> As far as I can see "single function with multiple port" support was
> already there but this commit is fixing queue index usage for it, if
> correct can you please update commit log accordingly? (with fixes line
> etc..)
> This also helps the fix to be backported to LTS versions.
>
>
> btw, how this feature was working until now, when queue ids "0 to
> ark_api_num_queues_per_port()" used for each port?
> Was it not tested at all, or is there something changed in FW causing
> this issue, if so is there any FW version dependency to this change?
>
> >
> > Add unique dev_private data for each port
> >
>
> Please check comment below related to this one.
>
> >
> > Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
> > ---
> >  drivers/net/ark/ark_ethdev.c    | 14 +++++++++++++-
> >  drivers/net/ark/ark_ethdev_rx.c |  6 +++---
> >  drivers/net/ark/ark_ethdev_tx.c |  2 +-
> >  drivers/net/ark/ark_global.h    |  4 ++++
> >  4 files changed, 21 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ark/ark_ethdev.c b/drivers/net/ark/ark_ethdev.c
> > index d237e80cf4..96d0c2b0f0 100644
> > --- a/drivers/net/ark/ark_ethdev.c
> > +++ b/drivers/net/ark/ark_ethdev.c
> > @@ -432,6 +432,7 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
> >                       ark->user_ext.dev_get_port_count(dev,
> >                                ark->user_data[dev->data->port_id]);
> >       ark->num_ports = port_count;
> > +     ark->num_queues = ark_api_num_queues_per_port(ark->mpurx.v,
> port_count);
> >
> >       for (p = 0; p < port_count; p++) {
> >               struct rte_eth_dev *eth_dev;
> > @@ -457,7 +458,18 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
> >               }
> >
> >               eth_dev->device = &pci_dev->device;
> > -             eth_dev->data->dev_private = ark;
> > +             /* Device requires new dev_private data */
> > +             eth_dev->data->dev_private =
> > +                     rte_zmalloc_socket(name,
> > +                                        sizeof(struct ark_adapter),
> > +                                        RTE_CACHE_LINE_SIZE,
> > +                                        rte_socket_id());
> > +
> > +             memcpy(eth_dev->data->dev_private, ark,
> > +                    sizeof(struct ark_adapter))> +           ark =
> eth_dev->data->dev_private;
> > +             ark->qbase = p * ark->num_queues;
> > +
>
> These each are a new eth_dev, so nothing wrong for each to allocate
> device private data, but if the only difference in private data is
> 'ark->qbase', it is possible to use 'eth_dev->process_private' for it,
> which is per eth_dev. It is up to you.
>
>
> Btw, how there are handled in the secondary process? (previous patch)
> Since this code just creates new eth_dev, shouldn't secondary process
> needs some code to find and re-use them?
>
> >               eth_dev->dev_ops = ark->eth_dev->dev_ops;
> >               eth_dev->tx_pkt_burst = ark->eth_dev->tx_pkt_burst;
> >               eth_dev->rx_pkt_burst = ark->eth_dev->rx_pkt_burst;
> > diff --git a/drivers/net/ark/ark_ethdev_rx.c
> b/drivers/net/ark/ark_ethdev_rx.c
> > index cbc0416bc2..38bc69dff4 100644
> > --- a/drivers/net/ark/ark_ethdev_rx.c
> > +++ b/drivers/net/ark/ark_ethdev_rx.c
> > @@ -68,7 +68,7 @@ struct ark_rx_queue {
> >  static int
> >  eth_ark_rx_hw_setup(struct rte_eth_dev *dev,
> >                   struct ark_rx_queue *queue,
> > -                 uint16_t rx_queue_id __rte_unused, uint16_t
> rx_queue_idx)
> > +                 uint16_t rx_queue_idx)
> >  {
> >       rte_iova_t queue_base;
> >       rte_iova_t phys_addr_q_base;
> > @@ -124,7 +124,7 @@ eth_ark_dev_rx_queue_setup(struct rte_eth_dev *dev,
> >       uint32_t i;
> >       int status;
> >
> > -     int qidx = queue_idx;
> > +     int qidx = ark->qbase + queue_idx;
> >
> >       /* We may already be setup, free memory prior to re-allocation */
> >       if (dev->data->rx_queues[queue_idx] != NULL) {
> > @@ -215,7 +215,7 @@ eth_ark_dev_rx_queue_setup(struct rte_eth_dev *dev,
> >       }
> >       /* MPU Setup */
> >       if (status == 0)
> > -             status = eth_ark_rx_hw_setup(dev, queue, qidx, queue_idx);
> > +             status = eth_ark_rx_hw_setup(dev, queue, queue_idx);
> >
> >       if (unlikely(status != 0)) {
> >               struct rte_mbuf **mbuf;
> > diff --git a/drivers/net/ark/ark_ethdev_tx.c
> b/drivers/net/ark/ark_ethdev_tx.c
> > index 5940a592a2..4792754f19 100644
> > --- a/drivers/net/ark/ark_ethdev_tx.c
> > +++ b/drivers/net/ark/ark_ethdev_tx.c
> > @@ -229,7 +229,7 @@ eth_ark_tx_queue_setup(struct rte_eth_dev *dev,
> >       struct ark_tx_queue *queue;
> >       int status;
> >
> > -     int qidx = queue_idx;
> > +     int qidx = ark->qbase + queue_idx;
> >
> >       if (!rte_is_power_of_2(nb_desc)) {
> >               ARK_PMD_LOG(ERR,
> > diff --git a/drivers/net/ark/ark_global.h b/drivers/net/ark/ark_global.h
> > index 71d0b53e03..176fbcda17 100644
> > --- a/drivers/net/ark/ark_global.h
> > +++ b/drivers/net/ark/ark_global.h
> > @@ -112,7 +112,11 @@ struct ark_adapter {
> >       ark_pkt_chkr_t pc;
> >       ark_pkt_dir_t pd;
> >
> > +     /* For single function, multiple ports */
> >       int num_ports;
> > +     uint16_t qbase;
> > +     uint16_t num_queues;
>
>
> it looks like 'num_queues' only used locally and not needed to be part
> of device data, unless there is some more usage planned for future.
>
> > +
> >       bool isvf;
> >
> >       /* Packet generator/checker args */
>
>

[-- Attachment #2: Type: text/html, Size: 7708 bytes --]

[PATCH v3 1/2] net/ark: limited support for secondary processes

[Ed Czeck]

From: John Miller <john.miller@atomicrules.com>

disable fpga device configuration for secondary processes.
This is minimum support to avoid unneeded fpga resets

Signed-off-by: John Miller <john.miller@atomicrules.com>
Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
---
v2:
* Use standard logging
v3:
* clarify limits on secondary process support
---
 drivers/net/ark/ark_ethdev.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ark/ark_ethdev.c b/drivers/net/ark/ark_ethdev.c
index b2995427c8..785bb386d7 100644
--- a/drivers/net/ark/ark_ethdev.c
+++ b/drivers/net/ark/ark_ethdev.c
@@ -147,6 +147,9 @@ eth_ark_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	struct rte_eth_dev *eth_dev;
 	int ret;
 
+	if (rte_eal_process_type() == RTE_PROC_SECONDARY)
+		ARK_PMD_LOG(INFO, "ARK probed by secondary process\n");
+
 	eth_dev = rte_eth_dev_pci_allocate(pci_dev, sizeof(struct ark_adapter));
 
 	if (eth_dev == NULL)
@@ -385,9 +388,11 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
 		    0xcafef00d, ark->sysctrl.t32[4], __func__);
 
 	/* We are a single function multi-port device. */
-	ret = ark_config_device(dev);
-	if (ret)
-		return -1;
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		ret = ark_config_device(dev);
+		if (ret)
+			return -1;
+	}
 
 	dev->dev_ops = &ark_eth_dev_ops;
 	dev->rx_queue_count = eth_ark_dev_rx_queue_count;
-- 
2.34.1

[PATCH v3 2/2] net/ark: support for single function with multiple port

[Ed Czeck]

Allows the creation of multiple ports from one ark device via
the use of ark pmd extension, though the splitting of queues
Add unique dev_private data for each port.

Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
---
v3:
* Backport to LTS version is not recommended since firmware
  changes are required and limited
* remove unused num_queues field from struct ark_adapter
---
 drivers/net/ark/ark_ethdev.c    | 15 ++++++++++++++-
 drivers/net/ark/ark_ethdev_rx.c |  6 +++---
 drivers/net/ark/ark_ethdev_tx.c |  2 +-
 drivers/net/ark/ark_global.h    |  3 +++
 4 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ark/ark_ethdev.c b/drivers/net/ark/ark_ethdev.c
index 785bb386d7..7077a144de 100644
--- a/drivers/net/ark/ark_ethdev.c
+++ b/drivers/net/ark/ark_ethdev.c
@@ -303,6 +303,7 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
 	int ret;
 	int port_count = 1;
 	int p;
+	uint16_t num_queues;
 	bool rqpacing = false;
 
 	ark->eth_dev = dev;
@@ -432,6 +433,7 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
 			ark->user_ext.dev_get_port_count(dev,
 				 ark->user_data[dev->data->port_id]);
 	ark->num_ports = port_count;
+	num_queues = ark_api_num_queues_per_port(ark->mpurx.v, port_count);
 
 	for (p = 0; p < port_count; p++) {
 		struct rte_eth_dev *eth_dev;
@@ -457,7 +459,18 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
 		}
 
 		eth_dev->device = &pci_dev->device;
-		eth_dev->data->dev_private = ark;
+		/* Device requires new dev_private data */
+		eth_dev->data->dev_private =
+			rte_zmalloc_socket(name,
+					   sizeof(struct ark_adapter),
+					   RTE_CACHE_LINE_SIZE,
+					   rte_socket_id());
+
+		memcpy(eth_dev->data->dev_private, ark,
+		       sizeof(struct ark_adapter));
+		ark = eth_dev->data->dev_private;
+		ark->qbase = p * num_queues;
+
 		eth_dev->dev_ops = ark->eth_dev->dev_ops;
 		eth_dev->tx_pkt_burst = ark->eth_dev->tx_pkt_burst;
 		eth_dev->rx_pkt_burst = ark->eth_dev->rx_pkt_burst;
diff --git a/drivers/net/ark/ark_ethdev_rx.c b/drivers/net/ark/ark_ethdev_rx.c
index cbc0416bc2..38bc69dff4 100644
--- a/drivers/net/ark/ark_ethdev_rx.c
+++ b/drivers/net/ark/ark_ethdev_rx.c
@@ -68,7 +68,7 @@ struct ark_rx_queue {
 static int
 eth_ark_rx_hw_setup(struct rte_eth_dev *dev,
 		    struct ark_rx_queue *queue,
-		    uint16_t rx_queue_id __rte_unused, uint16_t rx_queue_idx)
+		    uint16_t rx_queue_idx)
 {
 	rte_iova_t queue_base;
 	rte_iova_t phys_addr_q_base;
@@ -124,7 +124,7 @@ eth_ark_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	uint32_t i;
 	int status;
 
-	int qidx = queue_idx;
+	int qidx = ark->qbase + queue_idx;
 
 	/* We may already be setup, free memory prior to re-allocation */
 	if (dev->data->rx_queues[queue_idx] != NULL) {
@@ -215,7 +215,7 @@ eth_ark_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	}
 	/* MPU Setup */
 	if (status == 0)
-		status = eth_ark_rx_hw_setup(dev, queue, qidx, queue_idx);
+		status = eth_ark_rx_hw_setup(dev, queue, queue_idx);
 
 	if (unlikely(status != 0)) {
 		struct rte_mbuf **mbuf;
diff --git a/drivers/net/ark/ark_ethdev_tx.c b/drivers/net/ark/ark_ethdev_tx.c
index 5940a592a2..4792754f19 100644
--- a/drivers/net/ark/ark_ethdev_tx.c
+++ b/drivers/net/ark/ark_ethdev_tx.c
@@ -229,7 +229,7 @@ eth_ark_tx_queue_setup(struct rte_eth_dev *dev,
 	struct ark_tx_queue *queue;
 	int status;
 
-	int qidx = queue_idx;
+	int qidx = ark->qbase + queue_idx;
 
 	if (!rte_is_power_of_2(nb_desc)) {
 		ARK_PMD_LOG(ERR,
diff --git a/drivers/net/ark/ark_global.h b/drivers/net/ark/ark_global.h
index 71d0b53e03..2f198edfe4 100644
--- a/drivers/net/ark/ark_global.h
+++ b/drivers/net/ark/ark_global.h
@@ -112,7 +112,10 @@ struct ark_adapter {
 	ark_pkt_chkr_t pc;
 	ark_pkt_dir_t pd;
 
+	/* For single function, multiple ports */
 	int num_ports;
+	uint16_t qbase;
+
 	bool isvf;
 
 	/* Packet generator/checker args */
-- 
2.34.1

Re: [PATCH v2 1/3] net/ark: support secondary process

[Ferruh Yigit]

On 2/20/2023 10:04 PM, Ed Czeck wrote:
> Hi ferruh,
> We have limited support for secondary processes.  This patch simply
> avoids corrupting the FPGA state if a secondary process attaches.
> Improved support for secondary processes is on our list, but we need a
> strong customer driver for this feature.
> An update patch is following soon.

Hi Ed,

I see new version just updates commit log, mentioning this is minimal
secondary process support, but current implementation is wrong, it is
not about minimal support or full support.

Like just below secondary process, there is following code:

``dev->data->mac_addrs = rte_zmalloc()``

so you are allocating memory and set it to exact same pointer for
primary and each secondaries. This is probably leaking memory and
secondaries overwriting existing mac config by pointing it to new memory
location.

And there are more `dev->data->xxx` modifications in the function, which
is cause unexpected result.


As mentioned in the previous review, you need to find proper location to
return from function for secondary processes.

Please try to fix the implementation, instead of trying to push it with
known errors.


> Thanks for the review.
> Ed.
> 
> On Mon, Feb 20, 2023 at 9:17 AM Ferruh Yigit <ferruh.yigit@amd.com
> <mailto:ferruh.yigit@amd.com>> wrote:
> 
>     On 2/17/2023 9:59 PM, Ed Czeck wrote:
>     > From: John Miller <john.miller@atomicrules.com
>     <mailto:john.miller@atomicrules.com>>
>     >
>     > disable device configuration for secondary processes
>     >
>     > Signed-off-by: John Miller <john.miller@atomicrules.com
>     <mailto:john.miller@atomicrules.com>>
>     > ---
>     > v2:
>     > * Use standard logging
>     > ---
>     >  drivers/net/ark/ark_ethdev.c | 11 ++++++++---
>     >  1 file changed, 8 insertions(+), 3 deletions(-)
>     >
>     > diff --git a/drivers/net/ark/ark_ethdev.c
>     b/drivers/net/ark/ark_ethdev.c
>     > index b2995427c8..d237e80cf4 100644
>     > --- a/drivers/net/ark/ark_ethdev.c
>     > +++ b/drivers/net/ark/ark_ethdev.c
>     > @@ -147,6 +147,9 @@ eth_ark_pci_probe(struct rte_pci_driver
>     *pci_drv __rte_unused,
>     >       struct rte_eth_dev *eth_dev;
>     >       int ret;
>     > 
>     > +     if (rte_eal_process_type() == RTE_PROC_SECONDARY)
>     > +             ARK_PMD_LOG(DEBUG, "ARK probed by secondary process\n");
>     > +
>     >       eth_dev = rte_eth_dev_pci_allocate(pci_dev, sizeof(struct
>     ark_adapter));
>     > 
>     >       if (eth_dev == NULL)
>     > @@ -385,9 +388,11 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
>     >                   0xcafef00d, ark->sysctrl.t32[4], __func__);
>     > 
>     >       /* We are a single function multi-port device. */
>     > -     ret = ark_config_device(dev);
>     > -     if (ret)
>     > -             return -1;
>     > +     if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>     > +             ret = ark_config_device(dev);
>     > +             if (ret)
>     > +                     return -1;
>     > +     }
> 
> 
>     Hi Ed,
> 
>     As far as I can see both primary and secondary process continues to run
>     after this point, and below there are a few places that updates
>     'eth_dev->data'.
> 
>     'eth_dev->data' is shared between primary and secondaries, so each
>     secondary will be overwriting the shared data.
>     Better usage is shared data only updated by primary process and
>     secondary processes use available values.
>     But 'eth_dev' is process specific and all primary and shared processes
>     must set fields of this struct.
> 
>     You may need to re-order calls in function to make secondary quit after
>     'eth_dev' fields updated and before 'eth_dev->data' updated, to make
>     sure secondaries don't update shared data.
> 
>     > 
>     >       dev->dev_ops = &ark_eth_dev_ops;
>     >       dev->rx_queue_count = eth_ark_dev_rx_queue_count;
> 

Re: [PATCH v2 2/3] net/ark: support for single function with multiple port

[Ferruh Yigit]

On 2/20/2023 10:09 PM, Ed Czeck wrote:
> Hi Ferruh,
> We had looked at providing this patch as an LTS backport, but there are
> some ties to FPGA firmware which complicate its application.  As this is
> a uncommon net/ark feature we support users on an as-needed basis.

Got it, you don't want this fix to be backported, that is OK.

Still, it is a fix right, instead of a new support, if so can you please
reflect this in the commit log?

Also below comments seems ignored, can you please response them?

Specially I am curious about how come this failed now, was it a testing
gap in your end or a FW change caused it, and if it is a FW change this
may break previous FW support, right?


> Thanks.
> Ed.
> 
> On Mon, Feb 20, 2023 at 9:54 AM Ferruh Yigit <ferruh.yigit@amd.com
> <mailto:ferruh.yigit@amd.com>> wrote:
> 
>     On 2/17/2023 9:59 PM, Ed Czeck wrote:
>     > allows the creation of multiple ports from one ark device via
>     > the use of ark pmd extension, though the splitting of queues
> 
>     Hi Ed,
> 
>     As far as I can see "single function with multiple port" support was
>     already there but this commit is fixing queue index usage for it, if
>     correct can you please update commit log accordingly? (with fixes line
>     etc..)
>     This also helps the fix to be backported to LTS versions.
> 
> 
>     btw, how this feature was working until now, when queue ids "0 to
>     ark_api_num_queues_per_port()" used for each port?
>     Was it not tested at all, or is there something changed in FW causing
>     this issue, if so is there any FW version dependency to this change?
> 
>     >
>     > Add unique dev_private data for each port
>     >
> 
>     Please check comment below related to this one.
> 
>     >
>     > Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com
>     <mailto:ed.czeck@atomicrules.com>>
>     > ---
>     >  drivers/net/ark/ark_ethdev.c    | 14 +++++++++++++-
>     >  drivers/net/ark/ark_ethdev_rx.c |  6 +++---
>     >  drivers/net/ark/ark_ethdev_tx.c |  2 +-
>     >  drivers/net/ark/ark_global.h    |  4 ++++
>     >  4 files changed, 21 insertions(+), 5 deletions(-)
>     >
>     > diff --git a/drivers/net/ark/ark_ethdev.c
>     b/drivers/net/ark/ark_ethdev.c
>     > index d237e80cf4..96d0c2b0f0 100644
>     > --- a/drivers/net/ark/ark_ethdev.c
>     > +++ b/drivers/net/ark/ark_ethdev.c
>     > @@ -432,6 +432,7 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
>     >                       ark->user_ext.dev_get_port_count(dev,
>     >                                ark->user_data[dev->data->port_id]);
>     >       ark->num_ports = port_count;
>     > +     ark->num_queues = ark_api_num_queues_per_port(ark->mpurx.v,
>     port_count);
>     > 
>     >       for (p = 0; p < port_count; p++) {
>     >               struct rte_eth_dev *eth_dev;
>     > @@ -457,7 +458,18 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
>     >               }
>     > 
>     >               eth_dev->device = &pci_dev->device;
>     > -             eth_dev->data->dev_private = ark;
>     > +             /* Device requires new dev_private data */
>     > +             eth_dev->data->dev_private =
>     > +                     rte_zmalloc_socket(name,
>     > +                                        sizeof(struct ark_adapter),
>     > +                                        RTE_CACHE_LINE_SIZE,
>     > +                                        rte_socket_id());
>     > +
>     > +             memcpy(eth_dev->data->dev_private, ark,
>     > +                    sizeof(struct ark_adapter))> +           ark
>     = eth_dev->data->dev_private;
>     > +             ark->qbase = p * ark->num_queues;
>     > +
> 
>     These each are a new eth_dev, so nothing wrong for each to allocate
>     device private data, but if the only difference in private data is
>     'ark->qbase', it is possible to use 'eth_dev->process_private' for it,
>     which is per eth_dev. It is up to you.
> 
> 
>     Btw, how there are handled in the secondary process? (previous patch)
>     Since this code just creates new eth_dev, shouldn't secondary process
>     needs some code to find and re-use them?
> 
>     >               eth_dev->dev_ops = ark->eth_dev->dev_ops;
>     >               eth_dev->tx_pkt_burst = ark->eth_dev->tx_pkt_burst;
>     >               eth_dev->rx_pkt_burst = ark->eth_dev->rx_pkt_burst;
>     > diff --git a/drivers/net/ark/ark_ethdev_rx.c
>     b/drivers/net/ark/ark_ethdev_rx.c
>     > index cbc0416bc2..38bc69dff4 100644
>     > --- a/drivers/net/ark/ark_ethdev_rx.c
>     > +++ b/drivers/net/ark/ark_ethdev_rx.c
>     > @@ -68,7 +68,7 @@ struct ark_rx_queue {
>     >  static int
>     >  eth_ark_rx_hw_setup(struct rte_eth_dev *dev,
>     >                   struct ark_rx_queue *queue,
>     > -                 uint16_t rx_queue_id __rte_unused, uint16_t
>     rx_queue_idx)
>     > +                 uint16_t rx_queue_idx)
>     >  {
>     >       rte_iova_t queue_base;
>     >       rte_iova_t phys_addr_q_base;
>     > @@ -124,7 +124,7 @@ eth_ark_dev_rx_queue_setup(struct rte_eth_dev
>     *dev,
>     >       uint32_t i;
>     >       int status;
>     > 
>     > -     int qidx = queue_idx;
>     > +     int qidx = ark->qbase + queue_idx;
>     > 
>     >       /* We may already be setup, free memory prior to
>     re-allocation */
>     >       if (dev->data->rx_queues[queue_idx] != NULL) {
>     > @@ -215,7 +215,7 @@ eth_ark_dev_rx_queue_setup(struct rte_eth_dev
>     *dev,
>     >       }
>     >       /* MPU Setup */
>     >       if (status == 0)
>     > -             status = eth_ark_rx_hw_setup(dev, queue, qidx,
>     queue_idx);
>     > +             status = eth_ark_rx_hw_setup(dev, queue, queue_idx);
>     > 
>     >       if (unlikely(status != 0)) {
>     >               struct rte_mbuf **mbuf;
>     > diff --git a/drivers/net/ark/ark_ethdev_tx.c
>     b/drivers/net/ark/ark_ethdev_tx.c
>     > index 5940a592a2..4792754f19 100644
>     > --- a/drivers/net/ark/ark_ethdev_tx.c
>     > +++ b/drivers/net/ark/ark_ethdev_tx.c
>     > @@ -229,7 +229,7 @@ eth_ark_tx_queue_setup(struct rte_eth_dev *dev,
>     >       struct ark_tx_queue *queue;
>     >       int status;
>     > 
>     > -     int qidx = queue_idx;
>     > +     int qidx = ark->qbase + queue_idx;
>     > 
>     >       if (!rte_is_power_of_2(nb_desc)) {
>     >               ARK_PMD_LOG(ERR,
>     > diff --git a/drivers/net/ark/ark_global.h
>     b/drivers/net/ark/ark_global.h
>     > index 71d0b53e03..176fbcda17 100644
>     > --- a/drivers/net/ark/ark_global.h
>     > +++ b/drivers/net/ark/ark_global.h
>     > @@ -112,7 +112,11 @@ struct ark_adapter {
>     >       ark_pkt_chkr_t pc;
>     >       ark_pkt_dir_t pd;
>     > 
>     > +     /* For single function, multiple ports */
>     >       int num_ports;
>     > +     uint16_t qbase;
>     > +     uint16_t num_queues;
> 
> 
>     it looks like 'num_queues' only used locally and not needed to be part
>     of device data, unless there is some more usage planned for future.
> 
>     > +
>     >       bool isvf;
>     > 
>     >       /* Packet generator/checker args */
> 

[PATCH v4] net/ark: support for single function with multiple port

[Ed Czeck]

Support the creation of multiple ports from one ark device via
the use of ark pmd extension, though the assignment of queues
to port.

Add unique dev_private data for each port.

This patch repairs a latent issue uncovered during testing.

Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
---
v3:
* Backport to LTS version is not recommended since firmware
  changes are required and limited
* remove unused num_queues field from struct ark_adapter
v4:
* additional comments
* separate patch from unrelated patch in series
---
 drivers/net/ark/ark_ethdev.c    | 15 ++++++++++++++-
 drivers/net/ark/ark_ethdev_rx.c |  6 +++---
 drivers/net/ark/ark_ethdev_tx.c |  2 +-
 drivers/net/ark/ark_global.h    |  3 +++
 4 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ark/ark_ethdev.c b/drivers/net/ark/ark_ethdev.c
index b2995427c8..24c1cb3932 100644
--- a/drivers/net/ark/ark_ethdev.c
+++ b/drivers/net/ark/ark_ethdev.c
@@ -300,6 +300,7 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
 	int ret;
 	int port_count = 1;
 	int p;
+	uint16_t num_queues;
 	bool rqpacing = false;
 
 	ark->eth_dev = dev;
@@ -427,6 +428,7 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
 			ark->user_ext.dev_get_port_count(dev,
 				 ark->user_data[dev->data->port_id]);
 	ark->num_ports = port_count;
+	num_queues = ark_api_num_queues_per_port(ark->mpurx.v, port_count);
 
 	for (p = 0; p < port_count; p++) {
 		struct rte_eth_dev *eth_dev;
@@ -452,7 +454,18 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
 		}
 
 		eth_dev->device = &pci_dev->device;
-		eth_dev->data->dev_private = ark;
+		/* Device requires new dev_private data */
+		eth_dev->data->dev_private =
+			rte_zmalloc_socket(name,
+					   sizeof(struct ark_adapter),
+					   RTE_CACHE_LINE_SIZE,
+					   rte_socket_id());
+
+		memcpy(eth_dev->data->dev_private, ark,
+		       sizeof(struct ark_adapter));
+		ark = eth_dev->data->dev_private;
+		ark->qbase = p * num_queues;
+
 		eth_dev->dev_ops = ark->eth_dev->dev_ops;
 		eth_dev->tx_pkt_burst = ark->eth_dev->tx_pkt_burst;
 		eth_dev->rx_pkt_burst = ark->eth_dev->rx_pkt_burst;
diff --git a/drivers/net/ark/ark_ethdev_rx.c b/drivers/net/ark/ark_ethdev_rx.c
index cbc0416bc2..38bc69dff4 100644
--- a/drivers/net/ark/ark_ethdev_rx.c
+++ b/drivers/net/ark/ark_ethdev_rx.c
@@ -68,7 +68,7 @@ struct ark_rx_queue {
 static int
 eth_ark_rx_hw_setup(struct rte_eth_dev *dev,
 		    struct ark_rx_queue *queue,
-		    uint16_t rx_queue_id __rte_unused, uint16_t rx_queue_idx)
+		    uint16_t rx_queue_idx)
 {
 	rte_iova_t queue_base;
 	rte_iova_t phys_addr_q_base;
@@ -124,7 +124,7 @@ eth_ark_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	uint32_t i;
 	int status;
 
-	int qidx = queue_idx;
+	int qidx = ark->qbase + queue_idx;
 
 	/* We may already be setup, free memory prior to re-allocation */
 	if (dev->data->rx_queues[queue_idx] != NULL) {
@@ -215,7 +215,7 @@ eth_ark_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	}
 	/* MPU Setup */
 	if (status == 0)
-		status = eth_ark_rx_hw_setup(dev, queue, qidx, queue_idx);
+		status = eth_ark_rx_hw_setup(dev, queue, queue_idx);
 
 	if (unlikely(status != 0)) {
 		struct rte_mbuf **mbuf;
diff --git a/drivers/net/ark/ark_ethdev_tx.c b/drivers/net/ark/ark_ethdev_tx.c
index 5940a592a2..4792754f19 100644
--- a/drivers/net/ark/ark_ethdev_tx.c
+++ b/drivers/net/ark/ark_ethdev_tx.c
@@ -229,7 +229,7 @@ eth_ark_tx_queue_setup(struct rte_eth_dev *dev,
 	struct ark_tx_queue *queue;
 	int status;
 
-	int qidx = queue_idx;
+	int qidx = ark->qbase + queue_idx;
 
 	if (!rte_is_power_of_2(nb_desc)) {
 		ARK_PMD_LOG(ERR,
diff --git a/drivers/net/ark/ark_global.h b/drivers/net/ark/ark_global.h
index 71d0b53e03..2f198edfe4 100644
--- a/drivers/net/ark/ark_global.h
+++ b/drivers/net/ark/ark_global.h
@@ -112,7 +112,10 @@ struct ark_adapter {
 	ark_pkt_chkr_t pc;
 	ark_pkt_dir_t pd;
 
+	/* For single function, multiple ports */
 	int num_ports;
+	uint16_t qbase;
+
 	bool isvf;
 
 	/* Packet generator/checker args */
-- 
2.34.1

Re: [PATCH v4] net/ark: support for single function with multiple port

[Ferruh Yigit]

On 2/21/2023 4:30 PM, Ed Czeck wrote:
> Support the creation of multiple ports from one ark device via
> the use of ark pmd extension, though the assignment of queues
> to port.
> 
> Add unique dev_private data for each port.
> 
> This patch repairs a latent issue uncovered during testing.
> 
> Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>

Can you please update patch title as fix commit, and add Fixes line?

It can be good to add information to commit log that backport is not
desired, for LTS maintainers see and prevent backporting by mistake.

Re: [PATCH v4] net/ark: support for single function with multiple port

[Ferruh Yigit]

On 2/23/2023 2:25 PM, Ferruh Yigit wrote:
> On 2/21/2023 4:30 PM, Ed Czeck wrote:
>> Support the creation of multiple ports from one ark device via
>> the use of ark pmd extension, though the assignment of queues
>> to port.
>>
>> Add unique dev_private data for each port.
>>
>> This patch repairs a latent issue uncovered during testing.
>>
>> Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
> 
> Can you please update patch title as fix commit, and add Fixes line?
> 
> It can be good to add information to commit log that backport is not
> desired, for LTS maintainers see and prevent backporting by mistake.


Hi Ed,

Will there be a new version of this patch or can I drop it in the patchwork?

Re: [PATCH 1/3] net/ark: support secondary process

[Burakov, Anatoly]

On 2/17/2023 4:00 PM, Ed Czeck wrote:
> From: John Miller <john.miller@atomicrules.com>
> 
> disable device configuration for secondary processes
> 
> Signed-off-by: John Miller <john.miller@atomicrules.com>
> ---
>   drivers/net/ark/ark_ethdev.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ark/ark_ethdev.c b/drivers/net/ark/ark_ethdev.c
> index b2995427c8..f96722551e 100644
> --- a/drivers/net/ark/ark_ethdev.c
> +++ b/drivers/net/ark/ark_ethdev.c
> @@ -147,6 +147,9 @@ eth_ark_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>   	struct rte_eth_dev *eth_dev;
>   	int ret;
>   
> +	if (rte_eal_process_type() == RTE_PROC_SECONDARY)
> +		fprintf(stderr, "ARK probed by secondary process\n");

Why is this printing directly to stderr? IMO this should be RTE_LOG(ERR, 
...)

-- 
Thanks,
Anatoly

[PATCH 2/3] net/ark: support for single function with multiple port

[Ed Czeck]

allows the creation of multiple ports from one ark device via
the use of ark pmd extension, though the splitting of queues
Add unique dev_private data for each port

Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
---
 drivers/net/ark/ark_ethdev.c    | 14 +++++++++++++-
 drivers/net/ark/ark_ethdev_rx.c |  6 +++---
 drivers/net/ark/ark_ethdev_tx.c |  2 +-
 drivers/net/ark/ark_global.h    |  4 ++++
 4 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ark/ark_ethdev.c b/drivers/net/ark/ark_ethdev.c
index f96722551e..2fff3cd6ba 100644
--- a/drivers/net/ark/ark_ethdev.c
+++ b/drivers/net/ark/ark_ethdev.c
@@ -432,6 +432,7 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
 			ark->user_ext.dev_get_port_count(dev,
 				 ark->user_data[dev->data->port_id]);
 	ark->num_ports = port_count;
+	ark->num_queues = ark_api_num_queues_per_port(ark->mpurx.v, port_count);
 
 	for (p = 0; p < port_count; p++) {
 		struct rte_eth_dev *eth_dev;
@@ -457,7 +458,18 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
 		}
 
 		eth_dev->device = &pci_dev->device;
-		eth_dev->data->dev_private = ark;
+		/* Device requires new dev_private data */
+		eth_dev->data->dev_private =
+			rte_zmalloc_socket(name,
+					   sizeof(struct ark_adapter),
+					   RTE_CACHE_LINE_SIZE,
+					   rte_socket_id());
+
+		memcpy(eth_dev->data->dev_private, ark,
+		       sizeof(struct ark_adapter));
+		ark = eth_dev->data->dev_private;
+		ark->qbase = p * ark->num_queues;
+
 		eth_dev->dev_ops = ark->eth_dev->dev_ops;
 		eth_dev->tx_pkt_burst = ark->eth_dev->tx_pkt_burst;
 		eth_dev->rx_pkt_burst = ark->eth_dev->rx_pkt_burst;
diff --git a/drivers/net/ark/ark_ethdev_rx.c b/drivers/net/ark/ark_ethdev_rx.c
index cbc0416bc2..38bc69dff4 100644
--- a/drivers/net/ark/ark_ethdev_rx.c
+++ b/drivers/net/ark/ark_ethdev_rx.c
@@ -68,7 +68,7 @@ struct ark_rx_queue {
 static int
 eth_ark_rx_hw_setup(struct rte_eth_dev *dev,
 		    struct ark_rx_queue *queue,
-		    uint16_t rx_queue_id __rte_unused, uint16_t rx_queue_idx)
+		    uint16_t rx_queue_idx)
 {
 	rte_iova_t queue_base;
 	rte_iova_t phys_addr_q_base;
@@ -124,7 +124,7 @@ eth_ark_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	uint32_t i;
 	int status;
 
-	int qidx = queue_idx;
+	int qidx = ark->qbase + queue_idx;
 
 	/* We may already be setup, free memory prior to re-allocation */
 	if (dev->data->rx_queues[queue_idx] != NULL) {
@@ -215,7 +215,7 @@ eth_ark_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	}
 	/* MPU Setup */
 	if (status == 0)
-		status = eth_ark_rx_hw_setup(dev, queue, qidx, queue_idx);
+		status = eth_ark_rx_hw_setup(dev, queue, queue_idx);
 
 	if (unlikely(status != 0)) {
 		struct rte_mbuf **mbuf;
diff --git a/drivers/net/ark/ark_ethdev_tx.c b/drivers/net/ark/ark_ethdev_tx.c
index 5940a592a2..4792754f19 100644
--- a/drivers/net/ark/ark_ethdev_tx.c
+++ b/drivers/net/ark/ark_ethdev_tx.c
@@ -229,7 +229,7 @@ eth_ark_tx_queue_setup(struct rte_eth_dev *dev,
 	struct ark_tx_queue *queue;
 	int status;
 
-	int qidx = queue_idx;
+	int qidx = ark->qbase + queue_idx;
 
 	if (!rte_is_power_of_2(nb_desc)) {
 		ARK_PMD_LOG(ERR,
diff --git a/drivers/net/ark/ark_global.h b/drivers/net/ark/ark_global.h
index 71d0b53e03..176fbcda17 100644
--- a/drivers/net/ark/ark_global.h
+++ b/drivers/net/ark/ark_global.h
@@ -112,7 +112,11 @@ struct ark_adapter {
 	ark_pkt_chkr_t pc;
 	ark_pkt_dir_t pd;
 
+	/* For single function, multiple ports */
 	int num_ports;
+	uint16_t qbase;
+	uint16_t num_queues;
+
 	bool isvf;
 
 	/* Packet generator/checker args */
-- 
2.34.1