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.yi...@amd.com > <mailto:ferruh.yi...@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.cz...@atomicrules.com > <mailto:ed.cz...@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 */ >
