Re: [dpdk-dev] [PATCH v2 3/3] net/enetc: enable Rx and Tx

[Shreyansh Jain <shreyansh.jain@nxp.com>]

On Thursday 13 September 2018 03:12 PM, Gagandeep Singh wrote:
> Add RX and TX queue setup, datapath functions
> and enable the packet parsing
> 
> Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
> ---
>   MAINTAINERS                      |   1 +
>   drivers/net/enetc/Makefile       |   3 +-
>   drivers/net/enetc/enetc_ethdev.c |   6 +-
>   drivers/net/enetc/enetc_rxtx.c   | 447 +++++++++++++++++++++++++++++++
>   drivers/net/enetc/meson.build    |   3 +-
>   5 files changed, 456 insertions(+), 4 deletions(-)
>   create mode 100644 drivers/net/enetc/enetc_rxtx.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fc70ac049..b67f2afa4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -645,6 +645,7 @@ F: doc/guides/nics/features/dpaa2.ini
>   
>   NXP enetc
>   M: Gagandeep Singh <g.singh@nxp.com>
> +M: Pankaj Chauhan <pankaj.chauhan@nxp.com>
>   F: drivers/net/enetc/
>   F: doc/guides/nics/enetc.rst
>   F: doc/guides/nics/features/enetc.ini
> diff --git a/drivers/net/enetc/Makefile b/drivers/net/enetc/Makefile
> index 3f4ba97da..1f886831a 100644
> --- a/drivers/net/enetc/Makefile
> +++ b/drivers/net/enetc/Makefile
> @@ -16,8 +16,9 @@ LIBABIVER := 1
>   # all source are stored in SRCS-y
>   #
>   SRCS-$(CONFIG_RTE_LIBRTE_ENETC_PMD) += enetc_ethdev.c
> +SRCS-$(CONFIG_RTE_LIBRTE_ENETC_PMD) += enetc_rxtx.c
>   
> -LDLIBS += -lrte_eal
> +LDLIBS += -lrte_eal -lrte_mempool
>   LDLIBS += -lrte_ethdev
>   LDLIBS += -lrte_bus_pci
>   
> diff --git a/drivers/net/enetc/enetc_ethdev.c b/drivers/net/enetc/enetc_ethdev.c
> index 06438835d..67106593f 100644
> --- a/drivers/net/enetc/enetc_ethdev.c
> +++ b/drivers/net/enetc/enetc_ethdev.c
> @@ -37,6 +37,8 @@ static const struct eth_dev_ops enetc_ops = {
>   	.dev_close            = enetc_dev_close,
>   	.link_update          = enetc_link_update,
>   	.dev_infos_get        = enetc_dev_infos_get,
> +	.rx_queue_setup       = enetc_rx_queue_setup,
> +	.tx_queue_setup       = enetc_tx_queue_setup,
>   };
>   
>   /**
> @@ -61,8 +63,8 @@ enetc_dev_init(struct rte_eth_dev *eth_dev)
>   
>   	PMD_INIT_FUNC_TRACE();
>   	eth_dev->dev_ops = &enetc_ops;
> -	eth_dev->rx_pkt_burst = NULL;
> -	eth_dev->tx_pkt_burst = NULL;
> +	eth_dev->rx_pkt_burst = &enetc_recv_pkts;
> +	eth_dev->tx_pkt_burst = &enetc_xmit_pkts;
>   
>   	rte_eth_copy_pci_info(eth_dev, pci_dev);
>   
> diff --git a/drivers/net/enetc/enetc_rxtx.c b/drivers/net/enetc/enetc_rxtx.c
> new file mode 100644
> index 000000000..b01f64b0c
> --- /dev/null
> +++ b/drivers/net/enetc/enetc_rxtx.c
> @@ -0,0 +1,447 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2018 NXP
> + */
> +
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include <unistd.h>
> +
> +#include "rte_ethdev.h"
> +#include "rte_malloc.h"
> +#include "rte_memzone.h"
> +
> +#include "base/enetc_hw.h"
> +#include "enetc.h"
> +#include "enetc_logs.h"
> +
> +#define ENETC_RXBD_BUNDLE 8 /* Number of BDs to update at once */
> +
> +static inline int enetc_bd_unused(struct enetc_bdr *bdr)

Ideally, the function definition should be like:
<static/inline etc> <return type>
<Function name>(<Arguments>,
                 <Arguments on new line>,
                 <More arguments>);

This helps in searching for the tags against function names.
Declarations are on a single line.
This is valid across other patches as well in this series.

> +{
> +	if (bdr->next_to_clean > bdr->next_to_use)
> +		return bdr->next_to_clean - bdr->next_to_use - 1;
> +
> +	return bdr->bd_count + bdr->next_to_clean - bdr->next_to_use - 1;
> +}
> +
> +static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring)
> +{
> +	int tx_frm_cnt = 0;
> +	struct enetc_swbd *tx_swbd;
> +	int i;
> +
> +	i = tx_ring->next_to_clean;
> +	tx_swbd = &tx_ring->q_swbd[i];
> +	while ((int)(enetc_rd_reg(tx_ring->tcisr) & ENETC_TBCISR_IDX_MASK) != i) {
> +		rte_pktmbuf_free(tx_swbd->buffer_addr);
> +		tx_swbd->buffer_addr = NULL;
> +		tx_swbd++;
> +		i++;
> +		if (unlikely(i == tx_ring->bd_count)) {
> +			i = 0;
> +			tx_swbd = &tx_ring->q_swbd[0];
> +		}
> +
> +		tx_frm_cnt++;
> +	}
> +	tx_ring->next_to_clean = i;
> +	return tx_frm_cnt++;
> +}
> +
> +uint16_t enetc_xmit_pkts(void *tx_queue,
> +			struct rte_mbuf **tx_pkts,
> +				uint16_t nb_pkts)

Formatting of second line arguments is wrong.
Also, maintain uniformity of the declaration syntax. Either all on 
single line with their returns or all with returns on one line and names 
on another.

> +{
> +	struct enetc_swbd *tx_swbd;
> +	int i, start;
> +	struct enetc_tx_bd *txbd;
> +	struct enetc_bdr *tx_ring = (struct enetc_bdr *)tx_queue;
> +
> +	i = tx_ring->next_to_use;
> +	start = 0;
> +	while (nb_pkts--) {
> +		enetc_clean_tx_ring(tx_ring);
> +
> +		tx_ring->q_swbd[i].buffer_addr = tx_pkts[start];
> +
> +		txbd = ENETC_TXBD(*tx_ring, i);
> +		tx_swbd = &tx_ring->q_swbd[i];
> +		txbd->frm_len = tx_pkts[start]->pkt_len;
> +		txbd->buf_len = txbd->frm_len;
> +		txbd->flags = rte_cpu_to_le_16(ENETC_TXBD_FLAGS_F);
> +		txbd->addr =
> +		(uint64_t)rte_cpu_to_le_64(tx_swbd->buffer_addr->buf_addr +
> +				tx_swbd->buffer_addr->data_off);
> +		i++;
> +		start++;
> +		if (unlikely(i == tx_ring->bd_count))
> +			i = 0;
> +	}
> +	tx_ring->next_to_use = i;
> +	enetc_wr_reg(tx_ring->tcir, i);
> +	return start;
> +}
> +
> +static int enetc_refill_rx_ring(struct enetc_bdr *rx_ring, const int buff_cnt)
> +{
> +	struct enetc_swbd *rx_swbd;
> +	union enetc_rx_bd *rxbd;
> +	int i, j;
> +
> +	i = rx_ring->next_to_use;
> +	rx_swbd = &rx_ring->q_swbd[i];
> +	rxbd = ENETC_RXBD(*rx_ring, i);
> +
> +	for (j = 0; j < buff_cnt; j++) {
> +		rx_swbd->buffer_addr =
> +			rte_cpu_to_le_64(rte_mbuf_raw_alloc(rx_ring->mb_pool));
> +		rxbd->w.addr = (uint64_t)rx_swbd->buffer_addr->buf_addr +
> +						rx_swbd->buffer_addr->data_off;
> +		/* clear 'R" as well */
> +		rxbd->r.lstatus = 0;
> +		rx_swbd++;
> +		rxbd++;
> +		i++;
> +
> +		if (unlikely(i == rx_ring->bd_count)) {
> +			i = 0;
> +			rxbd = ENETC_RXBD(*rx_ring, 0);
> +			rx_swbd = &rx_ring->q_swbd[i];
> +		}
> +	}
> +	if (likely(j)) {
> +		rx_ring->next_to_alloc = i;
> +		rx_ring->next_to_use = i;
> +		enetc_wr_reg(rx_ring->rcir, i);
> +	}

It is nice to have a new line after a logical block (like the for and if 
above). It helps in reading.
But, no formal rule exists to enforce this.

> +	return j;
> +}
> +
> +
> +static inline void __attribute__((hot))
> +enetc_dev_rx_parse(struct rte_mbuf *m, uint16_t parse_results)
> +{
> +	ENETC_PMD_DP_DEBUG("parse summary = 0x%x   ", parse_results);
> +
> +	m->packet_type = RTE_PTYPE_UNKNOWN;
> +	switch (parse_results) {
> +	case ENETC_PKT_TYPE_ETHER:
> +		m->packet_type = RTE_PTYPE_L2_ETHER;
> +		break;
> +	case ENETC_PKT_TYPE_IPV4:
> +		m->packet_type = RTE_PTYPE_L2_ETHER |
> +					RTE_PTYPE_L3_IPV4;
> +		break;
> +	case ENETC_PKT_TYPE_IPV6:
> +		m->packet_type = RTE_PTYPE_L2_ETHER |
> +					RTE_PTYPE_L3_IPV6;
> +		break;
> +	case ENETC_PKT_TYPE_IPV4_TCP:
> +		m->packet_type = RTE_PTYPE_L2_ETHER |
> +			RTE_PTYPE_L3_IPV4 | RTE_PTYPE_L4_TCP;
> +		break;
> +	case ENETC_PKT_TYPE_IPV6_TCP:
> +		m->packet_type = RTE_PTYPE_L2_ETHER |
> +			RTE_PTYPE_L3_IPV6 | RTE_PTYPE_L4_TCP;
> +		break;
> +	case ENETC_PKT_TYPE_IPV4_UDP:
> +		m->packet_type = RTE_PTYPE_L2_ETHER |
> +			RTE_PTYPE_L3_IPV4 | RTE_PTYPE_L4_UDP;
> +		break;
> +	case ENETC_PKT_TYPE_IPV6_UDP:
> +		m->packet_type = RTE_PTYPE_L2_ETHER |
> +			RTE_PTYPE_L3_IPV6 | RTE_PTYPE_L4_UDP;
> +		break;
> +	case ENETC_PKT_TYPE_IPV4_SCTP:
> +		m->packet_type = RTE_PTYPE_L2_ETHER |
> +			RTE_PTYPE_L3_IPV4 | RTE_PTYPE_L4_SCTP;
> +		break;
> +	case ENETC_PKT_TYPE_IPV6_SCTP:
> +		m->packet_type = RTE_PTYPE_L2_ETHER |
> +			RTE_PTYPE_L3_IPV6 | RTE_PTYPE_L4_SCTP;
> +		break;
> +	case ENETC_PKT_TYPE_IPV4_ICMP:
> +		m->packet_type = RTE_PTYPE_L2_ETHER |
> +			RTE_PTYPE_L3_IPV4 | RTE_PTYPE_L4_ICMP;
> +		break;
> +	case ENETC_PKT_TYPE_IPV6_ICMP:
> +		m->packet_type = RTE_PTYPE_L2_ETHER |
> +			RTE_PTYPE_L3_IPV6 | RTE_PTYPE_L4_ICMP;

If you order your packet_type values, they would be easier to read:

		m->packet_type = RTE_PTYPE_L2_ETHER |
				 RTE_PTYPE_L3_IPV6 |
				 RTE_PTYPE_L4_ICMP;

Again, just like above, no strict formatting rule exists.
You can choose to ignore.

> +		break;
> +	/* More switch cases can be added */
> +	default:
> +		m->packet_type = RTE_PTYPE_UNKNOWN;
> +	}
> +}
> +
> +static int
> +enetc_clean_rx_ring(struct enetc_bdr *rx_ring, struct rte_mbuf **rx_pkts,
> +								int work_limit)

I think the above line would exceed 79 char - even if not, formatting is 
incorrect.

> +{
> +	int rx_frm_cnt = 0;
> +	int cleaned_cnt, i;
> +	struct enetc_swbd *rx_swbd;
> +
> +	cleaned_cnt = enetc_bd_unused(rx_ring);
> +
> +	/* next descriptor to process */
> +	i = rx_ring->next_to_clean;
> +	rx_swbd = &rx_ring->q_swbd[i];
> +
> +	while (likely(rx_frm_cnt < work_limit)) {
> +		union enetc_rx_bd *rxbd;
> +		uint32_t bd_status;
> +
> +		if (cleaned_cnt >= ENETC_RXBD_BUNDLE) {
> +			int count = enetc_refill_rx_ring(rx_ring, cleaned_cnt);
> +
> +			cleaned_cnt -= count;
> +		}
> +		rxbd = ENETC_RXBD(*rx_ring, i);
> +		bd_status = rte_le_to_cpu_32(rxbd->r.lstatus);
> +		if (!bd_status)
> +			break;
> +
> +		rx_swbd->buffer_addr->pkt_len = rxbd->r.buf_len;
> +		rx_swbd->buffer_addr->data_len = rxbd->r.buf_len;
> +		rx_swbd->buffer_addr->hash.rss = rxbd->r.rss_hash;
> +		rx_swbd->buffer_addr->ol_flags = 0;
> +		enetc_dev_rx_parse(rx_swbd->buffer_addr, rxbd->r.parse_summary);
> +
> +		rx_pkts[rx_frm_cnt] = rx_swbd->buffer_addr;
> +
> +		cleaned_cnt++;
> +		rx_swbd++;
> +		i++;
> +		if (unlikely(i == rx_ring->bd_count)) {
> +			i = 0;
> +			rx_swbd = &rx_ring->q_swbd[i];
> +		}
> +		rx_ring->next_to_clean = i;
> +		rx_frm_cnt++;
> +	}
> +
> +	return rx_frm_cnt;
> +}
> +

[...]

> +static void enetc_setup_tx_bdrs(struct rte_eth_dev *dev)
> +{
> +	int i;
> +	struct enetc_eth_adapter *priv =
> +			ENETC_DEV_PRIVATE(dev->data->dev_private);
> +
> +	for (i = 0; i < priv->num_tx_rings; i++) {
> +		enetc_setup_txbdr(&priv->hw.hw, priv->tx_ring[i]);
> +		dev->data->tx_queues[i] = priv->tx_ring[i];
> +	}
> +}
> +
> +int enetc_tx_queue_setup(struct rte_eth_dev *dev,
> +				uint16_t queue_idx,
> +				uint16_t nb_desc,
> +				unsigned int socket_id,
> +				const struct rte_eth_txconf *tx_conf)

Can you please fix the format of the arguments on new line.

> +{
> +	struct enetc_eth_adapter *adapter =
> +			ENETC_DEV_PRIVATE(dev->data->dev_private);
> +	int err = 0;
> +
> +	err = enetc_alloc_tx_resources(adapter);
> +	if (err)
> +		goto err_alloc_tx;
> +
> +	enetc_setup_tx_bdrs(dev);
> +
> +err_alloc_tx:
> +	return err;
> +}
> +
> +static int enetc_alloc_rxbdr(struct enetc_bdr *rxr)
> +{
> +	int size;
> +
> +	size = rxr->bd_count * sizeof(struct enetc_swbd);
> +	rxr->q_swbd = rte_malloc(NULL, size, RTE_CACHE_LINE_SIZE);
> +	if (rxr->q_swbd == NULL)
> +		return -ENOMEM;
> +
> +	size = rxr->bd_count * sizeof(union enetc_rx_bd);
> +	rxr->bd_base = rte_malloc(NULL, size, RTE_CACHE_LINE_SIZE);
> +

New line is not required here - rxr->bd_base allocation is being 
checked, so essentially a single logical block.

> +	if (rxr->bd_base == NULL) {
> +		rte_free(rxr->q_swbd);
> +		rxr->q_swbd = NULL;
> +		return -ENOMEM;
> +	}
> +
> +	rxr->next_to_clean = 0;
> +	rxr->next_to_use = 0;
> +	rxr->next_to_alloc = 0;
> +
> +	return 0;
> +}
> +

[...]