DPDK patches and discussions
 help / color / mirror / Atom feed
From: Stephen Hemminger <stephen@networkplumber.org>
To: Jan Medala <jan@semihalf.com>
Cc: dev@dpdk.org, matua@amazon.com
Subject: Re: [dpdk-dev] [PATCH v3 0/4] DPDK polling-mode driver for Amazon Elastic Network Adapters (ENA)
Date: Mon, 22 Feb 2016 13:07:47 -0800	[thread overview]
Message-ID: <20160222130747.4843fc2f@xeon-e3> (raw)
In-Reply-To: <1456169211-18867-1-git-send-email-jan@semihalf.com>

On Mon, 22 Feb 2016 20:26:47 +0100
Jan Medala <jan@semihalf.com> wrote:

> This drop includes additional features for Amazon ENA:
> * Low Latenycy Queue (LLQ) for Tx
> * RSS
> 
> All previous comments are resolved.
> 
> Jan Medala (4):
>   ena: Amazon ENA documentation
>   ena: Amazon ENA communication layer
>   ena: Amazon ENA communication layer for DPDK platform
>   ena: DPDK polling-mode driver for Amazon Elastic Network Adapters
>     (ENA)
> 
>  config/common_linuxapp                             |   11 +
>  doc/guides/nics/ena.rst                            |  238 ++
>  drivers/net/Makefile                               |    1 +
>  drivers/net/ena/Makefile                           |   62 +
>  drivers/net/ena/base/ena_com.c                     | 2750 ++++++++++++++++++++
>  drivers/net/ena/base/ena_com.h                     | 1038 ++++++++
>  drivers/net/ena/base/ena_defs/ena_admin_defs.h     | 1714 ++++++++++++
>  .../net/ena/base/ena_defs/ena_admin_defs_custom.h  |   40 +
>  drivers/net/ena/base/ena_defs/ena_common_defs.h    |   54 +
>  drivers/net/ena/base/ena_defs/ena_eth_io_defs.h    | 1143 ++++++++
>  drivers/net/ena/base/ena_defs/ena_gen_info.h       |   35 +
>  drivers/net/ena/base/ena_defs/ena_includes.h       |   39 +
>  drivers/net/ena/base/ena_defs/ena_regs_defs.h      |  326 +++
>  drivers/net/ena/base/ena_eth_com.c                 |  506 ++++
>  drivers/net/ena/base/ena_eth_com.h                 |  154 ++
>  drivers/net/ena/base/ena_plat.h                    |   51 +
>  drivers/net/ena/base/ena_plat_dpdk.h               |  212 ++
>  drivers/net/ena/ena_ethdev.c                       | 1327 ++++++++++
>  drivers/net/ena/ena_ethdev.h                       |  155 ++
>  drivers/net/ena/ena_logs.h                         |   76 +
>  drivers/net/ena/ena_platform.h                     |   58 +
>  mk/rte.app.mk                                      |    1 +
>  22 files changed, 9991 insertions(+)
>  create mode 100644 doc/guides/nics/ena.rst
>  create mode 100755 drivers/net/ena/Makefile
>  create mode 100644 drivers/net/ena/base/ena_com.c
>  create mode 100644 drivers/net/ena/base/ena_com.h
>  create mode 100644 drivers/net/ena/base/ena_defs/ena_admin_defs.h
>  create mode 100644 drivers/net/ena/base/ena_defs/ena_admin_defs_custom.h
>  create mode 100644 drivers/net/ena/base/ena_defs/ena_common_defs.h
>  create mode 100644 drivers/net/ena/base/ena_defs/ena_eth_io_defs.h
>  create mode 100644 drivers/net/ena/base/ena_defs/ena_gen_info.h
>  create mode 100644 drivers/net/ena/base/ena_defs/ena_includes.h
>  create mode 100644 drivers/net/ena/base/ena_defs/ena_regs_defs.h
>  create mode 100644 drivers/net/ena/base/ena_eth_com.c
>  create mode 100644 drivers/net/ena/base/ena_eth_com.h
>  create mode 100644 drivers/net/ena/base/ena_plat.h
>  create mode 100644 drivers/net/ena/base/ena_plat_dpdk.h
>  create mode 100644 drivers/net/ena/ena_ethdev.c
>  create mode 100755 drivers/net/ena/ena_ethdev.h
>  create mode 100644 drivers/net/ena/ena_logs.h
>  create mode 100644 drivers/net/ena/ena_platform.h
> 

I still see lots of style issues reported by running checkpatch (from Linux kernel)
with some of the standard DPDKism's suppressed.

CHECK: spaces preferred around that '/' (ctx:VxV)
#54: FILE: drivers/net/ena/ena_ethdev.c:54:
+#define ENA_IO_RXQ_IDX_REV(q)	((q - 1)/2) /*reverse version of ENA_IO_RXQ_IDX*/
                              	        ^

CHECK: 'desciptors' may be misspelled - perhaps 'descriptors'?
#59: FILE: drivers/net/ena/ena_ethdev.c:59:
+/* While processing submitted and completed desciptors (rx and tx path

ERROR: space prohibited before that close parenthesis ')'
#67: FILE: drivers/net/ena/ena_ethdev.c:67:
+#define __MERGE_64B_H_L(h, l) (((long)h << 32) | l )

WARNING: space prohibited before semicolon
#115: FILE: drivers/net/ena/ena_ethdev.c:115:
+static int ena_queue_restart(struct ena_ring *ring) ;

WARNING: braces {} are not necessary for any arm of this statement
#149: FILE: drivers/net/ena/ena_ethdev.c:149:
+	if (ena_rx_ctx->l4_proto == ENA_ETH_IO_L4_PROTO_TCP) {
[...]
+	} else if (ena_rx_ctx->l4_proto == ENA_ETH_IO_L4_PROTO_UDP) {
[...]

WARNING: braces {} are not necessary for any arm of this statement
#155: FILE: drivers/net/ena/ena_ethdev.c:155:
+	if (ena_rx_ctx->l3_proto == ENA_ETH_IO_L3_PROTO_IPV4) {
[...]
+	} else if (ena_rx_ctx->l3_proto == ENA_ETH_IO_L3_PROTO_IPV6) {
[...]

WARNING: void function return statements are not generally useful
#169: FILE: drivers/net/ena/ena_ethdev.c:169:
+	return;
+}

CHECK: Alignment should match open parenthesis
#177: FILE: drivers/net/ena/ena_ethdev.c:177:
+	if (mbuf->ol_flags & (PKT_TX_L4_MASK || PKT_TX_IP_CKSUM ||
+			PKT_TX_TCP_SEG)) {

CHECK: Blank lines aren't necessary after an open brace '{'
#178: FILE: drivers/net/ena/ena_ethdev.c:178:
+			PKT_TX_TCP_SEG)) {
+

WARNING: line over 100 characters
#184: FILE: drivers/net/ena/ena_ethdev.c:184:
+									 mbuf->l3_len + mbuf->l2_len)->data_off) >> 4);

CHECK: Alignment should match open parenthesis
#198: FILE: drivers/net/ena/ena_ethdev.c:198:
+			if (mbuf->packet_type & (RTE_PTYPE_L4_NONFRAG ||
+				RTE_PTYPE_INNER_L4_NONFRAG))

WARNING: void function return statements are not generally useful
#231: FILE: drivers/net/ena/ena_ethdev.c:231:
+	return;
+}

ERROR: "(foo*)" should be "(foo *)"
#235: FILE: drivers/net/ena/ena_ethdev.c:235:
+	struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);

WARNING: void function return statements are not generally useful
#243: FILE: drivers/net/ena/ena_ethdev.c:243:
+	return;
+}

ERROR: "(foo*)" should be "(foo *)"
#249: FILE: drivers/net/ena/ena_ethdev.c:249:
+	struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);

WARNING: braces {} are not necessary for single statement blocks
#256: FILE: drivers/net/ena/ena_ethdev.c:256:
+	if ((reta_size == 0) || (reta_conf == NULL)) {
+		return -EINVAL;
+	}

WARNING: line over 100 characters
#261: FILE: drivers/net/ena/ena_ethdev.c:261:
+		RTE_LOG(WARNING, PMD, "indirection table supplied (%d) is bigger than supported (%d)\n",

CHECK: Alignment should match open parenthesis
#262: FILE: drivers/net/ena/ena_ethdev.c:262:
+		RTE_LOG(WARNING, PMD, "indirection table supplied (%d) is bigger than supported (%d)\n",
+		reta_size, ENA_RX_RSS_TABLE_SIZE);

ERROR: "(foo*)" should be "(foo *)"
#300: FILE: drivers/net/ena/ena_ethdev.c:300:
+	struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);

CHECK: Alignment should match open parenthesis
#309: FILE: drivers/net/ena/ena_ethdev.c:309:
+	if (reta_size == 0 || reta_conf == NULL ||
+		(reta_size > RTE_RETA_GROUP_SIZE && ((reta_conf + 1) == NULL))) {

WARNING: braces {} are not necessary for single statement blocks
#323: FILE: drivers/net/ena/ena_ethdev.c:323:
+		if (TEST_BIT(reta_conf[reta_conf_idx].mask, reta_idx)) {
+			reta_conf[reta_conf_idx].reta[reta_idx] = ENA_IO_RXQ_IDX_REV(indirect_table[i]);
+		}

WARNING: line over 100 characters
#324: FILE: drivers/net/ena/ena_ethdev.c:324:
+			reta_conf[reta_conf_idx].reta[reta_idx] = ENA_IO_RXQ_IDX_REV(indirect_table[i]);

CHECK: Alignment should match open parenthesis
#347: FILE: drivers/net/ena/ena_ethdev.c:347:
+		rc = ena_com_indirect_table_fill_entry(ena_dev, i,
+				ENA_IO_RXQ_IDX(val));

ERROR: "(foo**)" should be "(foo **)"
#386: FILE: drivers/net/ena/ena_ethdev.c:386:
+	struct ena_ring **queues = (struct ena_ring**)dev->data->rx_queues;

WARNING: void function return statements are not generally useful
#394: FILE: drivers/net/ena/ena_ethdev.c:394:
+	return;
+}

ERROR: "(foo**)" should be "(foo **)"
#398: FILE: drivers/net/ena/ena_ethdev.c:398:
+	struct ena_ring **queues = (struct ena_ring**)dev->data->tx_queues;

WARNING: void function return statements are not generally useful
#406: FILE: drivers/net/ena/ena_ethdev.c:406:
+	return;
+}

ERROR: "(foo*)" should be "(foo *)"
#410: FILE: drivers/net/ena/ena_ethdev.c:410:
+	struct ena_ring *ring = (struct ena_ring*)queue;

WARNING: quoted string split across lines
#415: FILE: drivers/net/ena/ena_ethdev.c:415:
+	ena_assert_msg(ring->configured, "API violation. "
+		       "Trying to release not configured queue");

CHECK: 'RUNING' may be misspelled - perhaps 'RUNNING'?
#416: FILE: drivers/net/ena/ena_ethdev.c:416:
+	ena_assert_msg(ring->adapter->state != ENA_ADAPTER_STATE_RUNING,

WARNING: unnecessary whitespace before a quoted newline
#433: FILE: drivers/net/ena/ena_ethdev.c:433:
+	RTE_LOG(NOTICE, PMD, "RX Queue %d:%d released \n", ring->port_id, ring->id);

WARNING: void function return statements are not generally useful
#435: FILE: drivers/net/ena/ena_ethdev.c:435:
+	return;
+}

CHECK: No space is necessary after a cast
#439: FILE: drivers/net/ena/ena_ethdev.c:439:
+	struct ena_ring *ring = (struct ena_ring*) queue;

ERROR: "(foo*)" should be "(foo *)"
#439: FILE: drivers/net/ena/ena_ethdev.c:439:
+	struct ena_ring *ring = (struct ena_ring*) queue;

WARNING: quoted string split across lines
#444: FILE: drivers/net/ena/ena_ethdev.c:444:
+	ena_assert_msg(ring->configured, "API violation. Trying to release "
+		       "not configured queue");

CHECK: 'RUNING' may be misspelled - perhaps 'RUNNING'?
#445: FILE: drivers/net/ena/ena_ethdev.c:445:
+	ena_assert_msg(ring->adapter->state != ENA_ADAPTER_STATE_RUNING,

WARNING: unnecessary whitespace before a quoted newline
#467: FILE: drivers/net/ena/ena_ethdev.c:467:
+	RTE_LOG(NOTICE, PMD, "TX Queue %d:%d released \n", ring->port_id, ring->id);

WARNING: void function return statements are not generally useful
#469: FILE: drivers/net/ena/ena_ethdev.c:469:
+	return;
+}

WARNING: void function return statements are not generally useful
#485: FILE: drivers/net/ena/ena_ethdev.c:485:
+	return;
+}

ERROR: "foo* bar" should be "foo *bar"
#492: FILE: drivers/net/ena/ena_ethdev.c:492:
+		struct ena_tx_buffer* tx_buf =

WARNING: void function return statements are not generally useful
#502: FILE: drivers/net/ena/ena_ethdev.c:502:
+	return;
+}

ERROR: "(foo*)" should be "(foo *)"
#518: FILE: drivers/net/ena/ena_ethdev.c:518:
+	struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);

CHECK: Blank lines aren't necessary after an open brace '{'
#550: FILE: drivers/net/ena/ena_ethdev.c:550:
+{
+

WARNING: Missing a blank line after declarations
#552: FILE: drivers/net/ena/ena_ethdev.c:552:
+	uint32_t max_frame_len = adapter->max_mtu;
+	if (adapter->rte_eth_dev_data->dev_conf.rxmode.jumbo_frame == 1)

CHECK: Blank lines aren't necessary before a close brace '}'
#569: FILE: drivers/net/ena/ena_ethdev.c:569:
+
+}

ERROR: "(foo*)" should be "(foo *)"
#573: FILE: drivers/net/ena/ena_ethdev.c:573:
+	struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);

WARNING: Missing a blank line after declarations
#574: FILE: drivers/net/ena/ena_ethdev.c:574:
+	struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);
+	rte_atomic64_init(&adapter->drv_stats->ierrors);

ERROR: "(foo*)" should be "(foo *)"
#583: FILE: drivers/net/ena/ena_ethdev.c:583:
+	struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);

ERROR: "(foo*)" should be "(foo *)"
#619: FILE: drivers/net/ena/ena_ethdev.c:619:
+	adapter = (struct ena_adapter*)(dev->data->dev_private);

ERROR: "(foo*)" should be "(foo *)"
#643: FILE: drivers/net/ena/ena_ethdev.c:643:
+	struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);

ERROR: space required before the open parenthesis '('
#646: FILE: drivers/net/ena/ena_ethdev.c:646:
+	if(!(adapter->state == ENA_ADAPTER_STATE_CONFIG ||

CHECK: 'RUNING' may be misspelled - perhaps 'RUNNING'?
#672: FILE: drivers/net/ena/ena_ethdev.c:672:
+	adapter->state = ENA_ADAPTER_STATE_RUNING;

ERROR: "(foo*)" should be "(foo *)"
#706: FILE: drivers/net/ena/ena_ethdev.c:706:
+	struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);

WARNING: quoted string split across lines
#728: FILE: drivers/net/ena/ena_ethdev.c:728:
+		RTE_LOG(ERR, PMD, "failed to create io TX queue #%d (ena_qid: %d)"
+			"rc: %d\n", queue_idx, ena_qid, rc);

ERROR: "(foo*)" should be "(foo *)"
#770: FILE: drivers/net/ena/ena_ethdev.c:770:
+	struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);

ERROR: "(foo*)" should be "(foo *)"
#802: FILE: drivers/net/ena/ena_ethdev.c:802:
+					  sizeof(struct rte_mbuf*) * nb_desc, RTE_CACHE_LINE_SIZE);

ERROR: "foo ** bar" should be "foo **bar"
#822: FILE: drivers/net/ena/ena_ethdev.c:822:
+	struct rte_mbuf ** mbufs = &rxq->rx_buffer_info[0];

ERROR: trailing statements should be on next line
#824: FILE: drivers/net/ena/ena_ethdev.c:824:
+	if (unlikely(!count)) return 0;

ERROR: space required after that ',' (ctx:VxV)
#836: FILE: drivers/net/ena/ena_ethdev.c:836:
+		PMD_RX_LOG(DEBUG,"there are no enough free buffers");
 		                ^

WARNING: Block comments use a trailing */ on a separate line
#902: FILE: drivers/net/ena/ena_ethdev.c:902:
+	 * information */

ERROR: "(foo*)" should be "(foo *)"
#926: FILE: drivers/net/ena/ena_ethdev.c:926:
+	struct ena_adapter *adapter = (struct ena_adapter*)(eth_dev->data->dev_private);

ERROR: do not initialise statics to 0
#931: FILE: drivers/net/ena/ena_ethdev.c:931:
+	static int adapters_found = 0;

WARNING: braces {} are not necessary for single statement blocks
#942: FILE: drivers/net/ena/ena_ethdev.c:942:
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+		return 0;
+	}

CHECK: Alignment should match open parenthesis
#980: FILE: drivers/net/ena/ena_ethdev.c:980:
+			PMD_INIT_LOG(ERR,
+				"Trying to use LLQ but llq_num is 0. Fall back into regular queues\n");

ERROR: "(foo*)" should be "(foo *)"
#997: FILE: drivers/net/ena/ena_ethdev.c:997:
+	eth_dev->data->mac_addrs = (struct ether_addr*)adapter->mac_addr;

CHECK: Blank lines aren't necessary after an open brace '{'
#1017: FILE: drivers/net/ena/ena_ethdev.c:1017:
+{
+

ERROR: "(foo*)" should be "(foo *)"
#1018: FILE: drivers/net/ena/ena_ethdev.c:1018:
+	struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);

WARNING: Missing a blank line after declarations
#1046: FILE: drivers/net/ena/ena_ethdev.c:1046:
+		struct ena_ring *ring = &adapter->tx_ring[i];
+		ring->configured = 0;

CHECK: Blank lines aren't necessary before a close brace '}'
#1052: FILE: drivers/net/ena/ena_ethdev.c:1052:
+
+	}

WARNING: Missing a blank line after declarations
#1056: FILE: drivers/net/ena/ena_ethdev.c:1056:
+		struct ena_ring *ring = &adapter->rx_ring[i];
+		ring->configured = 0;

ERROR: "(foo*)" should be "(foo *)"
#1074: FILE: drivers/net/ena/ena_ethdev.c:1074:
+	adapter = (struct ena_adapter*)(dev->data->dev_private);

ERROR: "(foo*)" should be "(foo *)"
#1114: FILE: drivers/net/ena/ena_ethdev.c:1114:
+	struct ena_ring *rx_ring = (struct ena_ring*)(rx_queue);

CHECK: 'RUNING' may be misspelled - perhaps 'RUNNING'?
#1130: FILE: drivers/net/ena/ena_ethdev.c:1130:
+	if (unlikely(rx_ring->adapter->state != ENA_ADAPTER_STATE_RUNING)) {

ERROR: trailing statements should be on next line
#1137: FILE: drivers/net/ena/ena_ethdev.c:1137:
+	if (unlikely(nb_pkts > desc_in_use)) nb_pkts = desc_in_use;

WARNING: unnecessary whitespace before a quoted newline
#1147: FILE: drivers/net/ena/ena_ethdev.c:1147:
+			RTE_LOG(ERR, PMD, "ena_com_rx_pkt error %d \n", rc);

WARNING: Missing a blank line after declarations
#1155: FILE: drivers/net/ena/ena_ethdev.c:1155:
+		int segments = 0;
+		while (segments < ena_rx_ctx.descs) {

ERROR: "(foo*)" should be "(foo *)"
#1198: FILE: drivers/net/ena/ena_ethdev.c:1198:
+	struct ena_ring *tx_ring = (struct ena_ring*)(tx_queue);

CHECK: 'RUNING' may be misspelled - perhaps 'RUNNING'?
#1211: FILE: drivers/net/ena/ena_ethdev.c:1211:
+	if (unlikely(tx_ring->adapter->state != ENA_ADAPTER_STATE_RUNING)) {

CHECK: Blank lines aren't necessary after an open brace '{'
#1218: FILE: drivers/net/ena/ena_ethdev.c:1218:
+	for (sent_idx = 0; sent_idx < nb_pkts; sent_idx++) {
+

ERROR: "(foo*)" should be "(foo *)"
#1235: FILE: drivers/net/ena/ena_ethdev.c:1235:
+			ena_tx_ctx.push_header = (void *)((char*)(mbuf->buf_addr) + mbuf->data_off);

CHECK: spaces preferred around that '|' (ctx:VxV)
#1241: FILE: drivers/net/ena/ena_ethdev.c:1241:
+		if (unlikely(mbuf->ol_flags & (PKT_RX_L4_CKSUM_BAD|PKT_RX_IP_CKSUM_BAD))) {
 		                                                  ^

WARNING: braces {} are not necessary for single statement blocks
#1241: FILE: drivers/net/ena/ena_ethdev.c:1241:
+		if (unlikely(mbuf->ol_flags & (PKT_RX_L4_CKSUM_BAD|PKT_RX_IP_CKSUM_BAD))) {
+			rte_atomic64_inc(&tx_ring->adapter->drv_stats->ierrors);
+		}

ERROR: space required before the open parenthesis '('
#1255: FILE: drivers/net/ena/ena_ethdev.c:1255:
+		while((mbuf = mbuf->next) != NULL) {

total: 31 errors, 31 warnings, 21 checks, 1327 lines checked

drivers/net/ena/ena_ethdev.c has style problems, please review.

NOTE: Ignored message types: BIT_MACRO COMPARISON_TO_NULL PREFER_KERNEL_TYPES

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

      parent reply	other threads:[~2016-02-22 21:08 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-02-22 19:26 Jan Medala
2016-02-22 19:26 ` [dpdk-dev] [PATCH v3 1/4] ena: Amazon ENA documentation Jan Medala
2016-02-23 10:19   ` Mcnamara, John
2016-02-22 19:26 ` [dpdk-dev] [PATCH v3 2/4] ena: Amazon ENA communication layer Jan Medala
2016-02-22 19:26 ` [dpdk-dev] [PATCH v3 3/4] ena: Amazon ENA communication layer for DPDK platform Jan Medala
2016-02-22 19:26 ` [dpdk-dev] [PATCH v3 4/4] ena: DPDK polling-mode driver for Amazon Elastic Network Adapters (ENA) Jan Medala
2016-02-22 21:07 ` Stephen Hemminger [this message]

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=20160222130747.4843fc2f@xeon-e3 \
    --to=stephen@networkplumber.org \
    --cc=dev@dpdk.org \
    --cc=jan@semihalf.com \
    --cc=matua@amazon.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).