Re: [PATCH v2 03/12] net: ethernet: aquantia: Add ring support code
From: Lino Sanfilippo <LinoSanfilippo@gmx.de>
Date: 2017-01-07 01:05:42
On 06.01.2017 09:06, Alexander Loktionov wrote:
quoted hunk ↗ jump to hunk
From: David VomLehn <redacted> Add code to support the transmit and receive ring buffers. Signed-off-by: Alexander Loktionov <redacted> Signed-off-by: Dmitrii Tarakanov <redacted> Signed-off-by: Pavel Belous <redacted> Signed-off-by: David M. VomLehn <redacted> --- drivers/net/ethernet/aquantia/aq_ring.c | 380 ++++++++++++++++++++++++++++++++ drivers/net/ethernet/aquantia/aq_ring.h | 147 ++++++++++++ 2 files changed, 527 insertions(+) create mode 100644 drivers/net/ethernet/aquantia/aq_ring.c create mode 100644 drivers/net/ethernet/aquantia/aq_ring.hdiff --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 @@ +/* + * 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 aq_pci_ring.c: Definition of functions for Rx/Tx rings. */ + +#include "aq_ring.h" +#include "aq_nic.h" +#include "aq_hw.h" + +#include <linux/netdevice.h> +#include <linux/etherdevice.h> + +static struct aq_ring_s *aq_ring_alloc(struct aq_ring_s *self, + struct aq_nic_s *aq_nic, + struct aq_nic_cfg_s *aq_nic_cfg) +{ + int err = 0; + + if (!self) { + err = -ENOMEM; + goto err_exit; + }
Why do you check this parameter but not the others? Beside this since the function is static the only caller is your own code, so why is a check needed at all? Can self ever be NULL? Same pattern occurs in several other functions.
+
+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)
+
+struct aq_ring_s *aq_ring_rx_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;
+
+void aq_ring_free(struct aq_ring_s *self)
+{
+ if (!self)
+ 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:;
+}This does not make sense. Just return immediately if "self" is NULL (and if you really have to make this check).
+
+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);netdev_alloc_skb_ip_align() will do the proper alignment for you (BTW. AQ_CFG_IP_ALIGN is defined as 0 in aq_cfg.h. Is this on purpose?)
+ 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;This is unnecessary. The following call of eth_type_trans() will do this already. Regards, Lino