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

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

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.

> +		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?

> +
> +	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?

> +
>   	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'.

> +#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?