Re: [dpdk-dev] [EXT] Re: [PATCH v6 4/5] net/enetfec: add enqueue and dequeue support

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

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, October 27, 2021 7:56 PM
> To: Apeksha Gupta <apeksha.gupta@nxp.com>; david.marchand@redhat.com;
> andrew.rybchenko@oktetlabs.ru; ferruh.yigit@intel.com
> Cc: dev@dpdk.org; Sachin Saxena <sachin.saxena@nxp.com>; Hemant Agrawal
> <hemant.agrawal@nxp.com>
> Subject: [EXT] Re: [dpdk-dev] [PATCH v6 4/5] net/enetfec: add enqueue and
> dequeue support
> 
> Caution: EXT Email
> 
> On 10/21/2021 5:46 AM, 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.
> >
> 
> In the patch title you can prefer "Rx/Tx support" instead of
> "enqueue and dequeue support", which is more common usage.
> 
> > Signed-off-by: Sachin Saxena <sachin.saxena@nxp.com>
> > Signed-off-by: Apeksha Gupta <apeksha.gupta@nxp.com>
> 
> <...>
> 
> > --- a/doc/guides/nics/features/enetfec.ini
> > +++ b/doc/guides/nics/features/enetfec.ini
> > @@ -4,6 +4,8 @@
> >   ; Refer to default.ini for the full list of available PMD features.
> >   ;
> >   [Features]
> > +Basic stats       = Y
> > +Promiscuous mode     = Y
> 
> Can you please keep the order same with default.ini file.
[Apeksha] yes, updated in v7 patch series.

> 
> <...>
> 
> > @@ -226,6 +264,110 @@ enetfec_eth_stop(__rte_unused struct rte_eth_dev
> *dev)
> >       return 0;
> >   }
> >
> > +static int
> > +enetfec_eth_close(__rte_unused struct rte_eth_dev *dev)
> > +{
> 
> 'dev' is used.
> 
> > +     enet_free_buffers(dev);
> > +     return 0;
> > +}
> > +
> > +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) {
> > +             ENETFEC_PMD_ERR("Invalid device in link_update.\n");
> 
> Duplicated '\n'.
> 
> > +             return 0;
> > +     }
> > +
> > +     memset(&link, 0, sizeof(struct rte_eth_link));
> > +
> > +     link.link_status = lstatus;
> > +     link.link_speed = ETH_SPEED_NUM_1G;
> > +
> > +     ENETFEC_PMD_INFO("Port (%d) link is %s\n", dev->data->port_id,
> > +                      "Up");
> > +
> > +     return rte_eth_linkstatus_set(dev, &link);
> > +}
> > +
> > +static int
> > +enetfec_promiscuous_enable(__rte_unused struct rte_eth_dev *dev)
> 
> 'dev' is used.
> 
> <...>
> 
> > +static int
> > +enetfec_stats_get(struct rte_eth_dev *dev,
> > +           struct rte_eth_stats *stats)
> > +{
> > +     struct enetfec_private *fep = dev->data->dev_private;
> > +     struct rte_eth_stats *eth_stats = &fep->stats;
> > +
> > +     if (stats == NULL)
> > +             return -1;
> 
> No need to check this, ethdev layer already does.
> 
> > +
> > +     memset(stats, 0, sizeof(struct rte_eth_stats));
> > +
> 
> Same here, ethdev does this.
> 
> <...>
> 
> > +
> > +     /*
> > +      * Set default mac address
> > +      */
> > +     macaddr.addr_bytes[0] = 1;
> > +     macaddr.addr_bytes[1] = 1;
> > +     macaddr.addr_bytes[2] = 1;
> > +     macaddr.addr_bytes[3] = 1;
> > +     macaddr.addr_bytes[4] = 1;
> > +     macaddr.addr_bytes[5] = 1;
> 
> if it is fixed, you can set the addr while declaring the variable:
> struct rte_ether_addr macaddr = {
>      .addr_bytes = { 0x1, 0x1, 0x1, 0x1, 0x1, 0x1 }
> };
[Apeksha] As suggested all above comments are handled in v7 patch series.
> 
> 
> <...>
> 
> > index 0000000000..445fa97e77
> > --- /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
> > +
> 
> 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.
[Apeksha] These are added for debug purpose only, we didn't want the per packet check. We are removing this functionality.
	As suggested, later we will implement and add it in proper format.
> 
> <...>
> 
> > +
> > +#if ENETFEC_LOOPBACK
> > +static volatile bool lb_quit;
> > +
> > +static void fec_signal_handler(int signum)
> > +{
> > +     if (signum == SIGINT || signum == SIGTSTP || signum == SIGTERM) {
> > +             printf("\n\n %s: Signal %d received, preparing to exit...\n",
> > +                             __func__, signum);
> > +             lb_quit = true;
> > +     }
> > +}
> 
> Not sure if handling signals in the driver is a good idea, this is more an
> application level desicion. Please remember that DPDK is library and this
> PMD is one of many PMDs in the library.
> 
> Also please don't use 'printf' directly.
> 
> > +
> > +static void
> > +enetfec_lb_rxtx(void *rxq1)
> > +{
> > +     struct rte_mempool *pool;
> > +     struct bufdesc *rx_bdp = NULL, *tx_bdp = NULL;
> > +     struct rte_mbuf *mbuf = NULL, *new_mbuf = NULL;
> > +     unsigned short status;
> > +     unsigned short pkt_len = 0;
> > +     int index_r = 0, index_t = 0;
> > +     u8 *data;
> > +     struct enetfec_priv_rx_q *rxq  = (struct enetfec_priv_rx_q *)rxq1;
> > +     struct rte_eth_stats *stats = &rxq->fep->stats;
> > +     unsigned int i;
> > +     struct enetfec_private *fep;
> > +     struct enetfec_priv_tx_q *txq;
> > +     fep = rxq->fep->dev->data->dev_private;
> > +     txq = fep->tx_queues[0];
> > +
> > +     pool = rxq->pool;
> > +     rx_bdp = rxq->bd.cur;
> > +     tx_bdp = txq->bd.cur;
> > +
> > +     signal(SIGTSTP, fec_signal_handler);
> > +     while (!lb_quit) {
> > +chk_again:
> > +             status = rte_le_to_cpu_16(rte_read16(&rx_bdp->bd_sc));
> > +             if (status & RX_BD_EMPTY) {
> > +                     if (!lb_quit)
> > +                             goto chk_again;
> > +                     rxq->bd.cur = rx_bdp;
> > +                     txq->bd.cur = tx_bdp;
> > +                     return;
> > +             }
> > +
> > +             /* Check for errors. */
> > +             status ^= RX_BD_LAST;
> > +             if (status & (RX_BD_LG | RX_BD_SH | RX_BD_NO |
> > +                     RX_BD_CR | RX_BD_OV | RX_BD_LAST |
> > +                     RX_BD_TR)) {
> > +                     stats->ierrors++;
> > +                     if (status & RX_BD_OV) {
> > +                             /* FIFO overrun */
> > +                             ENETFEC_PMD_ERR("rx_fifo_error\n");
> > +                             goto rx_processing_done;
> > +                     }
> > +                     if (status & (RX_BD_LG | RX_BD_SH
> > +                                             | RX_BD_LAST)) {
> > +                             /* Frame too long or too short. */
> > +                             ENETFEC_PMD_ERR("rx_length_error\n");
> > +                             if (status & RX_BD_LAST)
> > +                                     ENETFEC_PMD_ERR("rcv is not +last\n");
> 
> duplicated '\n', but more importantly this is datapath, are you sure to use
> debug logs in datapath?
> 
> 'ENETFEC_DP_LOG' should be the to use in the datapath, since it is optimized
> out based on the default value it has, to not impact datapath.