> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@intel.com>
> Sent: Thursday, November 4, 2021 11:58 PM
> To: Apeksha Gupta <apeksha.gu...@nxp.com>; david.march...@redhat.com;
> andrew.rybche...@oktetlabs.ru
> Cc: dev@dpdk.org; Sachin Saxena <sachin.sax...@nxp.com>; Hemant Agrawal
> <hemant.agra...@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.sax...@nxp.com>
> > Signed-off-by: Apeksha Gupta <apeksha.gu...@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.
