| 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
| ||
|
Date: Mon, 9 Jan 2017 01:18:00 +0200
From: Rami Rosen <roszenrami@...il.com>
To: Alexander Loktionov <Alexander.Loktionov@...antia.com>
Cc: Netdev <netdev@...r.kernel.org>, David VomLehn <vomlehn@...as.net>,
Simon Edelhaus <Simon.Edelhaus@...antia.com>,
Dmitrii Tarakanov <Dmitrii.Tarakanov@...antia.com>,
Pavel Belous <Pavel.Belous@...antia.com>
Subject: Re: [PATCH v2 03/12] net: ethernet: aquantia: Add ring support code
Hi, Alexander,
After a brief review, I have the following minor comments:
...
...
> diff --git a/drivers/net/ethernet/aquantia/aq_ring.c b/drivers/net/ethernet/aquantia/aq_ring.c
> new file mode 100644
> index 0000000..a7ef6aa
> --- /dev/null
> +++ b/drivers/net/ethernet/aquantia/aq_ring.c
> @@ -0,0 +1,380 @@
Should be aq_ring.c and not aq_pci_ring.c
> +
> +/* File aq_pci_ring.c: Definition of functions for Rx/Tx rings. */
> +
The aq_nic_cfg parameter is not used, it should be removed:
> +struct aq_ring_s *aq_ring_tx_alloc(struct aq_ring_s *self,
> + struct aq_nic_s *aq_nic,
> + unsigned int idx,
> + struct aq_nic_cfg_s *aq_nic_cfg)
> +{
> + int err = 0;
> +
> + if (!self) {
> + err = -ENOMEM;
> + goto err_exit;
> + }
> + self->aq_nic = aq_nic;
> + self->idx = idx;
> + self->size = aq_nic_cfg->txds;
> + self->dx_size = aq_nic_cfg->aq_hw_caps->txd_size;
> +
> + self = aq_ring_alloc(self, aq_nic, aq_nic_cfg);
> + if (!self) {
> + err = -ENOMEM;
> + goto err_exit;
> + }
> +
> +err_exit:
> + if (err < 0) {
> + aq_ring_free(self);
> + self = NULL;
> + }
> + return self;
> +}
> +
Shouldn't the return type be void for next 2 methods?
> +int aq_ring_init(struct aq_ring_s *self)
> +{
> + self->hw_head = 0;
> + self->sw_head = 0;
> + self->sw_tail = 0;
> + return 0;
> +}
> +
> +int aq_ring_deinit(struct aq_ring_s *self)
> +{
> + return 0;
> +}
> +
> +void aq_ring_free(struct aq_ring_s *self)
> +{
> + if (!self)
I would prefer here simply "return" and remove altogether the err_exit
label, but it is up to you:
> + goto err_exit;
> +
> + kfree(self->buff_ring);
> +
> + if (self->dx_ring)
> + dma_free_coherent(aq_nic_get_dev(self->aq_nic),
> + self->size * self->dx_size, self->dx_ring,
> + self->dx_ring_pa);
> +
> +err_exit:;
> +}
> +
Shouldn't the following method return type be void ?
> +
> +int aq_ring_tx_clean(struct aq_ring_s *self)
> +{
> + struct device *dev = aq_nic_get_dev(self->aq_nic);
> + struct net_device *ndev = aq_nic_get_ndev(self->aq_nic);
> +
> + for (; self->sw_head != self->hw_head;
> + self->sw_head = aq_ring_next_dx(self, self->sw_head)) {
> + struct aq_ring_buff_s *buff = &self->buff_ring[self->sw_head];
> +
> + ++self->stats.tx_packets;
> + ++ndev->stats.tx_packets;
> + ndev->stats.tx_bytes += buff->len;
> +
> + if (likely(buff->is_mapped)) {
> + if (unlikely(buff->is_sop))
> + dma_unmap_single(dev, buff->pa, buff->len,
> + DMA_TO_DEVICE);
> + else
> + dma_unmap_page(dev, buff->pa, buff->len,
> + DMA_TO_DEVICE);
> + }
> +
> + if (unlikely(buff->is_eop))
> + dev_kfree_skb_any(buff->skb);
> + }
> +
> + if (aq_ring_avail_dx(self) > AQ_CFG_SKB_FRAGS_MAX)
> + aq_nic_ndev_queue_start(self->aq_nic, self->idx);
> +
> + return 0;
> +}
> +
The "err" variable in aq_ring_rx_clean() is meaningless and according to
current implementation of this method it should be removed. You set it
at the beginning to
0, then later on you also assign 0 to it under certain conditions, and
that's it, no other assignment. Maybe the second assignment should
have been to some other value than 0, but as it is it, the "err"
variable has no meaning.
> +int aq_ring_rx_clean(struct aq_ring_s *self, int *work_done, int budget)
> +{
> + struct net_device *ndev = aq_nic_get_ndev(self->aq_nic);
> + int err = 0;
> + bool is_rsc_completed = true;
> +
> + for (; (self->sw_head != self->hw_head) && budget;
> + self->sw_head = aq_ring_next_dx(self, self->sw_head),
> + --budget, ++(*work_done)) {
> + struct aq_ring_buff_s *buff = &self->buff_ring[self->sw_head];
> + struct sk_buff *skb = NULL;
> + unsigned int next_ = 0U;
> + unsigned int i = 0U;
> + struct aq_ring_buff_s *buff_ = NULL;
> +
> + if (buff->is_error) {
> + __free_pages(buff->page, 0);
> + continue;
> + }
> +
> + if (buff->is_cleaned)
> + continue;
> +
> + ++self->stats.rx_packets;
> + ++ndev->stats.rx_packets;
> + ndev->stats.rx_bytes += buff->len;
> +
> + if (!buff->is_eop) {
> + for (next_ = buff->next,
> + buff_ = &self->buff_ring[next_]; true;
> + next_ = buff_->next,
> + buff_ = &self->buff_ring[next_]) {
> + is_rsc_completed =
> + aq_ring_dx_in_range(self->sw_head,
> + next_,
> + self->hw_head);
> +
> + if (unlikely(!is_rsc_completed)) {
> + is_rsc_completed = false;
> + break;
> + }
> +
> + if (buff_->is_eop)
> + break;
> + }
> +
> + if (!is_rsc_completed) {
> + err = 0;
> + goto err_exit;
> + }
> + }
> +
> + skb = netdev_alloc_skb(ndev, ETH_HLEN + AQ_CFG_IP_ALIGN);
> +
> + skb_reserve(skb, AQ_CFG_IP_ALIGN);
> + skb_put(skb, ETH_HLEN);
> + memcpy(skb->data, page_address(buff->page), ETH_HLEN);
> +
> + skb_add_rx_frag(skb, 0, buff->page, ETH_HLEN,
> + buff->len - ETH_HLEN,
> + SKB_TRUESIZE(buff->len - ETH_HLEN));
> + if (!buff->is_eop) {
> + for (i = 1U, next_ = buff->next,
> + buff_ = &self->buff_ring[next_]; true;
> + next_ = buff_->next,
> + buff_ = &self->buff_ring[next_], ++i) {
> + skb_add_rx_frag(skb, i, buff_->page, 0,
> + buff_->len,
> + SKB_TRUESIZE(buff->len -
> + ETH_HLEN));
> + buff_->is_cleaned = 1;
> +
> + if (buff_->is_eop)
> + break;
> + }
> + }
> +
> + skb->dev = ndev;
> +
> + skb->protocol = eth_type_trans(skb, ndev);
> + if (unlikely(buff->is_cso_err)) {
> + ++self->stats.rx_errors;
> + __skb_mark_checksum_bad(skb);
> + } else {
> + if (buff->is_ip_cso) {
> + __skb_incr_checksum_unnecessary(skb);
> + if (buff->is_udp_cso || buff->is_tcp_cso)
> + __skb_incr_checksum_unnecessary(skb);
> + } else {
> + skb->ip_summed = CHECKSUM_NONE;
> + }
> + }
> +
> + skb_set_hash(skb, buff->rss_hash,
> + buff->is_hash_l4 ? PKT_HASH_TYPE_L4 :
> + PKT_HASH_TYPE_NONE);
> +
> + skb_record_rx_queue(skb, self->idx);
> +
> + netif_receive_skb(skb);
> + }
> +
> +err_exit:
> + return err;
> +}
Should the following two methods be void ?
> +
> +int aq_ring_tx_drop(struct aq_ring_s *self)
> +{
> + for (; self->sw_head != self->sw_tail;
> + self->sw_head = aq_ring_next_dx(self, self->sw_head)) {
> + struct aq_ring_buff_s *buff = &self->buff_ring[self->sw_head];
> + struct device *ndev = aq_nic_get_dev(self->aq_nic);
> +
> + if (likely(buff->is_mapped)) {
> + if (unlikely(buff->is_sop))
> + dma_unmap_single(ndev, buff->pa, buff->len,
> + DMA_TO_DEVICE);
> + else
> + dma_unmap_page(ndev, buff->pa, buff->len,
> + DMA_TO_DEVICE);
> + }
> +
> + if (unlikely(buff->is_eop))
> + dev_kfree_skb_any(buff->skb);
> + }
> +
> + return 0;
> +}
> +
> +int aq_ring_rx_drop(struct aq_ring_s *self)
> +{
> + for (; self->sw_head != self->sw_tail;
> + self->sw_head = aq_ring_next_dx(self, self->sw_head)) {
> + struct aq_ring_buff_s *buff = &self->buff_ring[self->sw_head];
> +
> + dma_unmap_page(aq_nic_get_dev(self->aq_nic), buff->pa,
> + AQ_CFG_RX_FRAME_MAX, DMA_FROM_DEVICE);
> +
> + __free_pages(buff->page, 0);
> + }
> +
> + return 0;
> +}
> +
> diff --git a/drivers/net/ethernet/aquantia/aq_ring.h b/drivers/net/ethernet/aquantia/aq_ring.h
> new file mode 100644
> index 0000000..8f7e16e
> --- /dev/null
> +++ b/drivers/net/ethernet/aquantia/aq_ring.h
> @@ -0,0 +1,147 @@
> +/*
> + * aQuantia Corporation Network Driver
> + * Copyright (C) 2014-2016 aQuantia Corporation. All rights reserved
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + */
> +
File name should be aq_ring.h:
> +/* File aq_pci_ring.h: Declaration of functions for Rx/Tx rings. */
> +
> +#ifndef AQ_RING_H
> +#define AQ_RING_H
> +
> +#include "aq_common.h"
> +
> +struct page;
> +
> +/* TxC SOP DX EOP
> + * +----------+----------+----------+-----------
> + * 8bytes|len l3,l4 | pa | pa | pa
> + * +----------+----------+----------+-----------
> + * 4/8bytes|len pkt |len pkt | | skb
> + * +----------+----------+----------+-----------
> + * 4/8bytes|is_txc |len,flags |len |len,is_eop
> + * +----------+----------+----------+-----------
> + *
> + * This aq_ring_buff_s doesn't have endianness dependency.
Typo: chache->cache
> + * It is __packed for chache line optimisations.
Regards,
Rami Rosen
Powered by blists - more mailing lists