From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754715AbcFPUq3 (ORCPT ); Thu, 16 Jun 2016 16:46:29 -0400 Received: from violet.fr.zoreil.com ([92.243.8.30]:52751 "EHLO violet.fr.zoreil.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754094AbcFPUq2 (ORCPT ); Thu, 16 Jun 2016 16:46:28 -0400 Date: Thu, 16 Jun 2016 22:46:23 +0200 From: Francois Romieu To: Netanel Belgazal Cc: netdev@vger.kernel.org, davm@davemeloft.net, linux-kernel@vger.kernel.org, zorik@annapurnalabs.com, saeed@annapurnalabs.com, alex@annapurnalabs.com, msw@amazon.com, aligouri@amazon.com, antoine.tenart@free-electrons.com Subject: Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA) Message-ID: <20160616204623.GA7546@electric-eye.fr.zoreil.com> References: <1465807573-11293-1-git-send-email-netanel@annapurnalabs.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1465807573-11293-1-git-send-email-netanel@annapurnalabs.com> X-Organisation: Land of Sunshine Inc. User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Netanel Belgazal : [...] Very limited review below. > diff --git a/Documentation/networking/ena.txt b/Documentation/networking/ena.txt > new file mode 100644 > index 0000000..528544f > --- /dev/null > +++ b/Documentation/networking/ena.txt > @@ -0,0 +1,330 @@ > +Linux kernel driver for Elastic Network Adapter (ENA) family: > +============================================================= > + > +Overview: > +========= > +The ENA driver provides a modern Ethernet device interface optimized > +for high performance and low CPU overhead. Marketing boilerplate. How much of it will still be true in 6 months ? 6 years ? Hint: stay technical. If in doubt, make it boring. :o) [...] > +ENA devices allow high speed and low overhead Ethernet traffic > +processing by providing a dedicated Tx/Rx queue pair per host CPU, a > +dedicated MSI-X interrupt vector per Tx/Rx queue pair, adaptive > +interrupt moderation, and CPU cacheline optimized data placement. How many queues exactly ? [...] > +Memory Allocations: > +=================== > +DMA Coherent buffers are allocated for the following DMA rings: > +- Tx submission ring (For regular mode; for LLQ mode it is allocated > + using kzalloc) > +- Tx completion ring > +- Rx submission ring > +- Rx completion ring > +- Admin submission ring > +- Admin completion ring > +- AENQ ring Useless documentation (implementation detail). Nobody will maintain it when the driver changes. Please remove. [...] > +MTU: > +==== > +The driver supports an arbitrarily large MTU with a maximum that is > +negotiated with the device. The driver configures MTU using the > +SetFeature command (ENA_ADMIN_MTU property). The user can change MTU > +via ifconfig(8) and ip(8). Please avoid advertising legacy ifconfig. [...] > diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h > new file mode 100644 > index 0000000..8516e5e > --- /dev/null > +++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h [...] > +/* admin commands opcodes */ > +enum ena_admin_aq_opcode { > + /* create submission queue */ > + ENA_ADMIN_CREATE_SQ = 1, > + > + /* destroy submission queue */ > + ENA_ADMIN_DESTROY_SQ = 2, > + > + /* create completion queue */ > + ENA_ADMIN_CREATE_CQ = 3, > + > + /* destroy completion queue */ > + ENA_ADMIN_DESTROY_CQ = 4, > + > + /* get capabilities of particular feature */ > + ENA_ADMIN_GET_FEATURE = 8, > + > + /* get capabilities of particular feature */ > + ENA_ADMIN_SET_FEATURE = 9, > + > + /* get statistics */ > + ENA_ADMIN_GET_STATS = 11, > +}; The documentation above simply duplicates the member names -> useless visual payload. Please replace space with tab before "=" to line things up. [...] > +/* Basic Statistics Command. */ > +struct ena_admin_basic_stats { > + /* word 0 : */ > + u32 tx_bytes_low; > + > + /* word 1 : */ > + u32 tx_bytes_high; > + > + /* word 2 : */ > + u32 tx_pkts_low; > + > + /* word 3 : */ > + u32 tx_pkts_high; > + > + /* word 4 : */ > + u32 rx_bytes_low; > + > + /* word 5 : */ > + u32 rx_bytes_high; > + > + /* word 6 : */ > + u32 rx_pkts_low; > + > + /* word 7 : */ > + u32 rx_pkts_high; > + > + /* word 8 : */ > + u32 rx_drops_low; > + > + /* word 9 : */ > + u32 rx_drops_high; > +}; struct zorg { u32 ...; u32 ...; u32 ...; u32 ...; u32 ...; u32 ...; u32 ...; u32 ...; u32 ...; u32 ...; u32 ...; }; -> No comment needed to figure the offset. Comment removed => no need to check if offset starts at word 0 or word 1. [...] > diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c > new file mode 100644 > index 0000000..357e10f > --- /dev/null > +++ b/drivers/net/ethernet/amazon/ena/ena_com.c [...] > +static inline int ena_com_mem_addr_set(struct ena_com_dev *ena_dev, > + struct ena_common_mem_addr *ena_addr, > + dma_addr_t addr) > +{ > + if ((addr & GENMASK_ULL(ena_dev->dma_addr_bits - 1, 0)) != addr) { > + ena_trc_err("dma address has more bits that the device supports\n"); > + return -EINVAL; > + } > + > + ena_addr->mem_addr_low = (u32)addr; > + ena_addr->mem_addr_high = > + ((addr & GENMASK_ULL(ena_dev->dma_addr_bits - 1, 32)) >> 32); No need to "... & GENMASK" given the test above. > + > + return 0; > +} > + > +static int ena_com_admin_init_sq(struct ena_com_admin_queue *queue) > +{ > + queue->sq.entries = > + dma_alloc_coherent(queue->q_dmadev, > + ADMIN_SQ_SIZE(queue->q_depth), > + &queue->sq.dma_addr, > + GFP_KERNEL | __GFP_ZERO); 1. Use dma_zalloc_coherent(). 2. Style xyzab = dma_alloc_coherent(..., ..., ... ..., ...); 3. Add local variable for &queue->sq. In a different part of the code: s/uint32_t/u32/ -- Ueimor