| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20160617010857.GA24893@u54ee753d2d1854bda401.ant.amazon.com>
Date: Thu, 16 Jun 2016 18:09:03 -0700
From: Matt Wilson <msw@...n.com>
To: Francois Romieu <romieu@...zoreil.com>
Cc: Netanel Belgazal <netanel@...apurnalabs.com>,
netdev@...r.kernel.org, davem@...emloft.net,
linux-kernel@...r.kernel.org, zorik@...apurnalabs.com,
saeed@...apurnalabs.com, alex@...apurnalabs.com,
aliguori@...zon.com, antoine.tenart@...e-electrons.com
Subject: Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic
Network Adapters (ENA)
On Thu, Jun 16, 2016 at 10:46:23PM +0200, Francois Romieu wrote:
> Netanel Belgazal <netanel@...apurnalabs.com> :
> [...]
>
> Very limited review below.
I'll comment on the documentation (since I edited it heavily) but will
leave some the other parts for Netanel to answer.
> > 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)
Hopefully it will still be true for quite a while. But you make a good
point, the overview here should stick with the technical details.
> [...]
> > +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 ?
It's negotiated with the adapter via the admin queue, and it will vary
based on the capabilities of a particular adapter. See:
rc = ena_com_get_feature(ena_dev, &get_resp,
ENA_ADMIN_MAX_QUEUES_NUM);
in ena_com.c
> [...]
> > +Memory Allocations:
> > +===================
> > +DMA Coherent buffers are allocated for the f`ollowing 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.
Ack.
> [...]
> > +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.
Ack.
> [...]
> > 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.
I'll leave this to Netanel. A good amount of this comes from a unified
documentation / struct / register access generation script, and it
might need some massaging.
> [...]
> > +/* 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.
Ack.
> > +
> > + 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.
Ack.
> In a different part of the code: s/uint32_t/u32/
Argh! I thought I caught all of those.
Thanks for the review!
--msw
Powered by blists - more mailing lists