DPDK patches and discussions
 help / color / mirror / Atom feed
From: Apeksha Gupta <apeksha.gupta@nxp.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>,
	"david.marchand@redhat.com" <david.marchand@redhat.com>,
	"andrew.rybchenko@oktetlabs.ru" <andrew.rybchenko@oktetlabs.ru>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	Sachin Saxena <sachin.saxena@nxp.com>,
	Hemant Agrawal <hemant.agrawal@nxp.com>
Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v7 4/5] net/enetfec: add Rx/Tx support
Date: Tue, 9 Nov 2021 16:20:46 +0000	[thread overview]
Message-ID: <PAXPR04MB944544D7496CDAC72DDAD04EEF929@PAXPR04MB9445.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <319deb68-dd42-d7fc-d398-d8e1b4904cf6@intel.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.

  reply	other threads:[~2021-11-09 16:20 UTC|newest]

Thread overview: 91+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-01 11:42 [dpdk-dev] [PATCH v4 0/5] drivers/net: add NXP ENETFEC driver Apeksha Gupta
2021-10-01 11:42 ` [dpdk-dev] [PATCH v4 1/5] net/enetfec: introduce " Apeksha Gupta
2021-10-19 18:39   ` [dpdk-dev] [PATCH v5 0/5] drivers/net: add " Apeksha Gupta
2021-10-19 18:39     ` [dpdk-dev] [PATCH v5 1/5] net/enetfec: introduce " Apeksha Gupta
2021-10-21  4:46       ` [dpdk-dev] [PATCH v6 0/5] drivers/net: add " Apeksha Gupta
2021-10-21  4:46         ` [dpdk-dev] [PATCH v6 1/5] net/enetfec: introduce " Apeksha Gupta
2021-10-21  5:24           ` Hemant Agrawal
2021-10-27 14:18           ` Ferruh Yigit
2021-11-08 18:42             ` [dpdk-dev] [EXT] " Apeksha Gupta
2021-11-03 19:20           ` [dpdk-dev] [PATCH v7 0/5] drivers/net: add " Apeksha Gupta
2021-11-03 19:20             ` [dpdk-dev] [PATCH v7 1/5] net/enetfec: introduce " Apeksha Gupta
2021-11-03 23:27               ` Ferruh Yigit
2021-11-04 18:24               ` Ferruh Yigit
2021-11-08 19:13                 ` [dpdk-dev] [EXT] " Apeksha Gupta
2021-11-09 11:34               ` [dpdk-dev] [PATCH v8 0/5] drivers/net: add " Apeksha Gupta
2021-11-09 11:34                 ` [dpdk-dev] [PATCH v8 1/5] net/enetfec: introduce " Apeksha Gupta
2021-11-09 11:34                 ` [dpdk-dev] [PATCH v8 2/5] net/enetfec: add UIO support Apeksha Gupta
2021-11-09 11:34                 ` [dpdk-dev] [PATCH v8 3/5] net/enetfec: support queue configuration Apeksha Gupta
2021-11-09 11:34                 ` [dpdk-dev] [PATCH v8 4/5] net/enetfec: add Rx/Tx support Apeksha Gupta
2021-11-09 11:34                 ` [dpdk-dev] [PATCH v8 5/5] net/enetfec: add features Apeksha Gupta
2021-11-10  7:48                   ` [dpdk-dev] [PATCH v9 0/5] drivers/net: add NXP ENETFEC driver Apeksha Gupta
2021-11-10  7:48                     ` [dpdk-dev] [PATCH v9 1/5] net/enetfec: introduce " Apeksha Gupta
2021-11-10 13:53                       ` Ferruh Yigit
2021-11-13  4:31                       ` [PATCH v10 0/5] drivers/net: add " Apeksha Gupta
2021-11-13  4:31                         ` [PATCH v10 1/5] net/enetfec: introduce " Apeksha Gupta
2021-11-15  7:19                           ` [PATCH v11 0/5] drivers/net: add " Apeksha Gupta
2021-11-15  7:19                             ` [PATCH v11 1/5] net/enetfec: introduce " Apeksha Gupta
2021-11-15 10:07                               ` Ferruh Yigit
2023-03-21 18:03                               ` Ferruh Yigit
2023-03-23  6:00                                 ` Sachin Saxena (OSS)
2023-03-23 11:07                                   ` Ferruh Yigit
2023-03-23 11:09                                     ` Sachin Saxena (OSS)
2021-11-15  7:19                             ` [PATCH v11 2/5] net/enetfec: add UIO support Apeksha Gupta
2021-11-15  7:19                             ` [PATCH v11 3/5] net/enetfec: support queue configuration Apeksha Gupta
2021-11-15 10:11                               ` Ferruh Yigit
2021-11-15 10:24                                 ` Ferruh Yigit
2021-11-15 11:15                                   ` Ferruh Yigit
2021-11-15  7:19                             ` [PATCH v11 4/5] net/enetfec: add Rx/Tx support Apeksha Gupta
2021-11-15  7:19                             ` [PATCH v11 5/5] net/enetfec: add features Apeksha Gupta
2021-11-15  9:44                             ` [PATCH v11 0/5] drivers/net: add NXP ENETFEC driver Ferruh Yigit
2021-11-15 15:05                             ` Ferruh Yigit
2021-11-25 16:52                               ` Ferruh Yigit
2021-11-13  4:31                         ` [PATCH v10 2/5] net/enetfec: add UIO support Apeksha Gupta
2021-11-13  4:31                         ` [PATCH v10 3/5] net/enetfec: support queue configuration Apeksha Gupta
2021-11-13 17:11                           ` Stephen Hemminger
2021-11-13  4:31                         ` [PATCH v10 4/5] net/enetfec: add Rx/Tx support Apeksha Gupta
2021-11-13 17:10                           ` Stephen Hemminger
2021-11-13  4:31                         ` [PATCH v10 5/5] net/enetfec: add features Apeksha Gupta
2021-11-10  7:48                     ` [dpdk-dev] [PATCH v9 2/5] net/enetfec: add UIO support Apeksha Gupta
2021-11-10  7:48                     ` [dpdk-dev] [PATCH v9 3/5] net/enetfec: support queue configuration Apeksha Gupta
2021-11-10 13:54                       ` Ferruh Yigit
2021-11-13  5:00                         ` [EXT] " Apeksha Gupta
2021-11-15 10:06                         ` Ferruh Yigit
2021-11-15 10:23                           ` Ferruh Yigit
2021-11-15 10:29                             ` Ferruh Yigit
2021-11-10  7:48                     ` [dpdk-dev] [PATCH v9 4/5] net/enetfec: add Rx/Tx support Apeksha Gupta
2021-11-10 13:56                       ` Ferruh Yigit
2021-11-10  7:48                     ` [dpdk-dev] [PATCH v9 5/5] net/enetfec: add features Apeksha Gupta
2021-11-10 13:57                       ` Ferruh Yigit
2021-11-03 19:20             ` [dpdk-dev] [PATCH v7 2/5] net/enetfec: add UIO support Apeksha Gupta
2021-11-04 18:25               ` Ferruh Yigit
2021-11-08 20:24                 ` [dpdk-dev] [EXT] " Apeksha Gupta
2021-11-08 21:51                   ` Ferruh Yigit
2021-11-03 19:20             ` [dpdk-dev] [PATCH v7 3/5] net/enetfec: support queue configuration Apeksha Gupta
2021-11-04 18:26               ` Ferruh Yigit
2021-11-03 19:20             ` [dpdk-dev] [PATCH v7 4/5] net/enetfec: add Rx/Tx support Apeksha Gupta
2021-11-04 18:28               ` Ferruh Yigit
2021-11-09 16:20                 ` Apeksha Gupta [this message]
2021-11-03 19:20             ` [dpdk-dev] [PATCH v7 5/5] net/enetfec: add features Apeksha Gupta
2021-11-04 18:31             ` [dpdk-dev] [PATCH v7 0/5] drivers/net: add NXP ENETFEC driver Ferruh Yigit
2021-10-21  4:46         ` [dpdk-dev] [PATCH v6 2/5] net/enetfec: add UIO support Apeksha Gupta
2021-10-27 14:21           ` Ferruh Yigit
2021-11-08 18:44             ` [dpdk-dev] [EXT] " Apeksha Gupta
2021-10-21  4:46         ` [dpdk-dev] [PATCH v6 3/5] net/enetfec: support queue configuration Apeksha Gupta
2021-10-27 14:23           ` Ferruh Yigit
2021-11-08 18:45             ` [dpdk-dev] [EXT] " Apeksha Gupta
2021-10-21  4:46         ` [dpdk-dev] [PATCH v6 4/5] net/enetfec: add enqueue and dequeue support Apeksha Gupta
2021-10-27 14:25           ` Ferruh Yigit
2021-11-08 18:47             ` [dpdk-dev] [EXT] " Apeksha Gupta
2021-10-21  4:47         ` [dpdk-dev] [PATCH v6 5/5] net/enetfec: add features Apeksha Gupta
2021-10-27 14:26           ` Ferruh Yigit
2021-10-27 14:15         ` [dpdk-dev] [PATCH v6 0/5] drivers/net: add NXP ENETFEC driver Ferruh Yigit
2021-10-19 18:40     ` [dpdk-dev] [PATCH v5 2/5] net/enetfec: add UIO support Apeksha Gupta
2021-10-19 18:40     ` [dpdk-dev] [PATCH v5 3/5] net/enetfec: support queue configuration Apeksha Gupta
2021-10-19 18:40     ` [dpdk-dev] [PATCH v5 4/5] net/enetfec: add enqueue and dequeue support Apeksha Gupta
2021-10-19 18:40     ` [dpdk-dev] [PATCH v5 5/5] net/enetfec: add features Apeksha Gupta
2021-10-01 11:42 ` [dpdk-dev] [PATCH v4 2/5] net/enetfec: add UIO support Apeksha Gupta
2021-10-01 11:42 ` [dpdk-dev] [PATCH v4 3/5] net/enetfec: support queue configuration Apeksha Gupta
2021-10-01 11:42 ` [dpdk-dev] [PATCH v4 4/5] net/enetfec: add enqueue and dequeue support Apeksha Gupta
2021-10-01 11:42 ` [dpdk-dev] [PATCH v4 5/5] net/enetfec: add features Apeksha Gupta
2021-10-17 10:49 ` [dpdk-dev] [PATCH v4 0/5] drivers/net: add NXP ENETFEC driver Apeksha Gupta

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=PAXPR04MB944544D7496CDAC72DDAD04EEF929@PAXPR04MB9445.eurprd04.prod.outlook.com \
    --to=apeksha.gupta@nxp.com \
    --cc=andrew.rybchenko@oktetlabs.ru \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=hemant.agrawal@nxp.com \
    --cc=sachin.saxena@nxp.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).