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

[Ferruh Yigit <ferruh.yigit@amd.com>]

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 */
>