Re: [dpdk-dev] [EXT] Re: [PATCH v7 4/5] net/enetfec: add Rx/Tx support

[Apeksha Gupta <apeksha.gupta@nxp.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, November 4, 2021 11:58 PM
> To: Apeksha Gupta <apeksha.gupta@nxp.com>; david.marchand@redhat.com;
> andrew.rybchenko@oktetlabs.ru
> Cc: dev@dpdk.org; Sachin Saxena <sachin.saxena@nxp.com>; Hemant Agrawal
> <hemant.agrawal@nxp.com>
> Subject: [EXT] Re: [PATCH v7 4/5] net/enetfec: add Rx/Tx support
> 
> Caution: EXT Email
> 
> On 11/3/2021 7:20 PM, Apeksha Gupta wrote:
> > This patch adds burst enqueue and dequeue operations to the enetfec
> > PMD. Loopback mode is also added, compile time flag 'ENETFEC_LOOPBACK'
> is
> > used to enable this feature. By default loopback mode is disabled.
> > Basic features added like promiscuous enable, basic stats.
> >
> > Signed-off-by: Sachin Saxena <sachin.saxena@nxp.com>
> > Signed-off-by: Apeksha Gupta <apeksha.gupta@nxp.com>
> 
> <...>
> 
> > +static int
> > +enetfec_eth_link_update(struct rte_eth_dev *dev,
> > +                     int wait_to_complete __rte_unused)
> > +{
> > +     struct rte_eth_link link;
> > +     unsigned int lstatus = 1;
> > +
> > +     if (dev == NULL) {
> 
> 'dev' can't be null for a dev_ops,
> unless it is called internally in PMD which seems not the case here.
[Apeksha] okay.

> 
> > +             ENETFEC_PMD_ERR("Invalid device in link_update.");
> > +             return 0;
> > +     }
> > +
> > +     memset(&link, 0, sizeof(struct rte_eth_link));
> > +
> > +     link.link_status = lstatus;
> > +     link.link_speed = ETH_SPEED_NUM_1G;
> 
> Isn't there an actual way to get real link status from device?
[Apeksha] 'Get link status' feature support is yet to be implemented.

> 
> > +
> > +     ENETFEC_PMD_INFO("Port (%d) link is %s\n", dev->data->port_id,
> > +                      "Up");
> > +
> > +     return rte_eth_linkstatus_set(dev, &link);
> > +}
> > +
> 
> <...>
> 
> > @@ -501,6 +658,21 @@ pmd_enetfec_probe(struct rte_vdev_device *vdev)
> >               fep->bd_addr_p = fep->bd_addr_p + bdsize;
> >       }
> >
> > +     /* Copy the station address into the dev structure, */
> > +     dev->data->mac_addrs = rte_zmalloc("mac_addr", ETHER_ADDR_LEN, 0);
> > +     if (dev->data->mac_addrs == NULL) {
> > +             ENETFEC_PMD_ERR("Failed to allocate mem %d to store MAC
> addresses",
> > +                     ETHER_ADDR_LEN);
> > +             rc = -ENOMEM;
> > +             goto err;
> > +     }
> > +
> > +     /*
> > +      * Set default mac address
> > +      */
> > +     enetfec_set_mac_address(dev, &macaddr);
> 
> In each device start, a different MAC address is set by 'enetfec_restart()',
> I also put some comment there, but there seems two different MAC address set
> in two different parts of the driver.
> 
> > +
> > +     fep->bufdesc_ex = ENETFEC_EXTENDED_BD;
> >       rc = enetfec_eth_init(dev);
> >       if (rc)
> >               goto failed_init;
> > @@ -509,6 +681,8 @@ pmd_enetfec_probe(struct rte_vdev_device *vdev)
> >
> >   failed_init:
> >       ENETFEC_PMD_ERR("Failed to init");
> > +err:
> > +     rte_eth_dev_release_port(dev);
> >       return rc;
> >   }
> >
> > @@ -516,6 +690,8 @@ static int
> >   pmd_enetfec_remove(struct rte_vdev_device *vdev)
> >   {
> >       struct rte_eth_dev *eth_dev = NULL;
> > +     struct enetfec_private *fep;
> > +     struct enetfec_priv_rx_q *rxq;
> >       int ret;
> >
> >       /* find the ethdev entry */
> > @@ -523,11 +699,22 @@ pmd_enetfec_remove(struct rte_vdev_device *vdev)
> >       if (eth_dev == NULL)
> >               return -ENODEV;
> >
> > +     fep = eth_dev->data->dev_private;
> > +     /* Free descriptor base of first RX queue as it was configured
> > +      * first in enetfec_eth_init().
> > +      */
> > +     rxq = fep->rx_queues[0];
> > +     rte_free(rxq->bd.base);
> > +     enet_free_queue(eth_dev);
> > +     enetfec_eth_stop(eth_dev);
> > +
> >       ret = rte_eth_dev_release_port(eth_dev);
> >       if (ret != 0)
> >               return -EINVAL;
> >
> >       ENETFEC_PMD_INFO("Release enetfec sw device");
> > +     munmap(fep->hw_baseaddr_v, fep->cbus_size);
> 
> instead of unmap directly here, what about having a function in 'enet_uio.c',
> and call that cleanup function from here?
[Apeksha]  Yes, this can be done. Updated in v8 series.

> 
> > +
> >       return 0;
> >   }
> >
> > diff --git a/drivers/net/enetfec/enet_ethdev.h
> b/drivers/net/enetfec/enet_ethdev.h
> > index 36202ba6c7..e48f958ad9 100644
> > --- a/drivers/net/enetfec/enet_ethdev.h
> > +++ b/drivers/net/enetfec/enet_ethdev.h
> > @@ -7,6 +7,11 @@
> >
> >   #include <rte_ethdev.h>
> >
> > +#define ETHER_ADDR_LEN               6
> 
> Below defines 'ETH_ALEN', seems for same reason.
> 
> And in DPDK we already have 'RTE_ETHER_ADDR_LEN'.
> 
> Tor prevent all these redundancy, why not drop 'ETH_ALEN' &
> 'ETHER_ADDR_LEN',
> and just use 'RTE_ETHER_ADDR_LEN'.
[Apeksha]  Okay, we will remove these redundancy and use 'RTE_ETHER_ADDR_LEN'.

> 
> > +#define BD_LEN                       49152
> > +#define ENETFEC_TX_FR_SIZE   2048
> > +#define ETH_HLEN             RTE_ETHER_HDR_LEN
> > +
> >   /* full duplex or half duplex */
> >   #define HALF_DUPLEX             0x00
> >   #define FULL_DUPLEX             0x01
> > @@ -19,6 +24,20 @@
> >   #define ETH_ALEN            RTE_ETHER_ADDR_LEN
> >
> >   #define __iomem
> > +#if defined(RTE_ARCH_ARM)
> > +#if defined(RTE_ARCH_64)
> > +#define dcbf(p) { asm volatile("dc cvac, %0" : : "r"(p) : "memory"); }
> > +#define dcbf_64(p) dcbf(p)
> > +
> > +#else /* RTE_ARCH_32 */
> > +#define dcbf(p) RTE_SET_USED(p)
> > +#define dcbf_64(p) dcbf(p)
> > +#endif
> > +
> > +#else
> > +#define dcbf(p) RTE_SET_USED(p)
> > +#define dcbf_64(p) dcbf(p)
> > +#endif
> >
> >   /*
> >    * ENETFEC with AVB IP can support maximum 3 rx and tx queues.
> > @@ -142,4 +161,9 @@ enet_get_bd_index(struct bufdesc *bdp, struct
> bufdesc_prop *bd)
> >       return ((const char *)bdp - (const char *)bd->base) >> bd->d_size_log2;
> >   }
> >
> > +uint16_t enetfec_recv_pkts(void *rxq1, __rte_unused struct rte_mbuf
> **rx_pkts,
> > +             uint16_t nb_pkts);
> > +uint16_t enetfec_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
> > +             uint16_t nb_pkts);
> > +
> >   #endif /*__ENETFEC_ETHDEV_H__*/
> > diff --git a/drivers/net/enetfec/enet_rxtx.c b/drivers/net/enetfec/enet_rxtx.c
> > new file mode 100644
> > index 0000000000..6ac4624553
> > --- /dev/null
> > +++ b/drivers/net/enetfec/enet_rxtx.c
> > @@ -0,0 +1,445 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright 2021 NXP
> > + */
> > +
> > +#include <signal.h>
> > +#include <rte_mbuf.h>
> > +#include <rte_io.h>
> > +#include "enet_regs.h"
> > +#include "enet_ethdev.h"
> > +#include "enet_pmd_logs.h"
> > +
> > +#define ENETFEC_LOOPBACK     0
> > +#define ENETFEC_DUMP         0
> 
> There was a request to convert them into devargs, again seems
> silently ignored, copy/paste from previous version:
> 
> Instead of compile time flags, why not convert them to devargs so
> they can be updated without recompile?
> This also make sure all code is enabled and prevent possible dead
> code by time.
> 
> > +
> > +#if ENETFEC_DUMP
> > +static void
> > +enet_dump(struct enetfec_priv_tx_q *txq)
> > +{
> > +     struct bufdesc *bdp;
> > +     int index = 0;
> > +
> > +     ENETFEC_PMD_DEBUG("TX ring dump\n");
> > +     ENETFEC_PMD_DEBUG("Nr     SC     addr       len  MBUF\n");
> > +
> > +     bdp = txq->bd.base;
> > +     do {
> > +             ENETFEC_PMD_DEBUG("%3u %c%c 0x%04x 0x%08x %4u %p\n",
> > +                     index,
> > +                     bdp == txq->bd.cur ? 'S' : ' ',
> > +                     bdp == txq->dirty_tx ? 'H' : ' ',
> > +                     rte_read16(rte_le_to_cpu_16(&bdp->bd_sc)),
> > +                     rte_read32(rte_le_to_cpu_32(&bdp->bd_bufaddr)),
> > +                     rte_read16(rte_le_to_cpu_16(&bdp->bd_datlen)),
> > +                     txq->tx_mbuf[index]);
> > +             bdp = enet_get_nextdesc(bdp, &txq->bd);
> > +             index++;
> > +     } while (bdp != txq->bd.base);
> > +}
> > +
> > +static void
> > +enet_dump_rx(struct enetfec_priv_rx_q *rxq)
> > +{
> > +     struct bufdesc *bdp;
> > +     int index = 0;
> > +
> > +     ENETFEC_PMD_DEBUG("RX ring dump\n");
> > +     ENETFEC_PMD_DEBUG("Nr     SC     addr       len  MBUF\n");
> > +
> > +     bdp = rxq->bd.base;
> > +     do {
> > +             ENETFEC_PMD_DEBUG("%3u %c 0x%04x 0x%08x %4u %p\n",
> > +                     index,
> > +                     bdp == rxq->bd.cur ? 'S' : ' ',
> > +                     rte_read16(rte_le_to_cpu_16(&bdp->bd_sc)),
> > +                     rte_read32(rte_le_to_cpu_32(&bdp->bd_bufaddr)),
> > +                     rte_read16(rte_le_to_cpu_16(&bdp->bd_datlen)),
> > +                     rxq->rx_mbuf[index]);
> > +             rte_pktmbuf_dump(stdout, rxq->rx_mbuf[index],
> > +                             rxq->rx_mbuf[index]->pkt_len);
> > +             bdp = enet_get_nextdesc(bdp, &rxq->bd);
> > +             index++;
> > +     } while (bdp != rxq->bd.base);
> > +}
> > +#endif
> > +
> > +#if ENETFEC_LOOPBACK
> > +static volatile bool lb_quit;
> > +
> > +static void fec_signal_handler(int signum)
> > +{
> > +     if (signum == SIGINT || signum == SIGTSTP || signum == SIGTERM) {
> > +             ENETFEC_PMD_INFO("\n\n %s: Signal %d received, preparing to
> exit...\n",
> > +                             __func__, signum);
> > +             lb_quit = true;
> > +     }
> > +}
> > +
> 
> Again another comment ignored from previos version, we shouldn't have
> signals handled by driver, that is application task.
> 
> I am stopping reviewing here, there are too many things from previous
> version just ignored, can you please check/answer comments on the
> previous version of the set?

All above comments are handled in v8 patch series.