subject:"\[PATCH 05\/12\] Support for NIC\-specific code"

Re: [PATCH 05/12] Support for NIC-specific code

2016-12-29 Thread David VomLehn


Responses inline.

On 12/27/2016 09:21 PM, Rami Rosen wrote:

Hi, David,

Several nitpicks and comments, from a brief overview:

The commented label //err_exit:  should be removed

+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -0,0 +1,993 @@
+//err_exit:
+//err_exit:

Shouldn't aq_nic_rss_init() be static? isn't it called only from
aq_nic_cfg_init_defaults()?
and it always returns 0, shouldn't it be void as well ? (+ remove
checking the return code when invoking it in
aq_nic_cfg_init_defaults())

Yes, thanks.


+int aq_nic_rss_init(struct aq_nic_s *self, unsigned int num_rss_queues)
+{
+   struct aq_nic_cfg_s *cfg = >aq_nic_cfg;
+   struct aq_receive_scale_parameters *rss_params = >aq_rss;
+   int i = 0;
+

...

+   return 0;
+}


Shouldn't aq_nic_ndev_alloc() be static ? Isn't it invoked only from
aq_nic_alloc_cold()?

Yes.



+struct net_device *aq_nic_ndev_alloc(void)
+{

...

+}




+
+static unsigned int aq_nic_map_skb_lso(struct aq_nic_s *self,
+  struct sk_buff *skb,
+  struct aq_ring_buff_s *dx)
+{
+   unsigned int ret = 0U;
+
+   dx->flags = 0U;
+   dx->len_pkt = skb->len;
+   dx->len_l2 = ETH_HLEN;
+   dx->len_l3 = ip_hdrlen(skb);
+   dx->len_l4 = tcp_hdrlen(skb);
+   dx->mss = skb_shinfo(skb)->gso_size;
+   dx->is_txc = 1U;
+   ret = 1U;
+

Why not remove this "ret" variable, and simply return 1 ? the method
always returns 1:


+   return ret;
+}
+

Yes, better.

+int aq_nic_xmit(struct aq_nic_s *self, struct sk_buff *skb)
+{
+   struct aq_ring_s *ring = NULL;
+   unsigned int frags = 0U;
+   unsigned int vec = skb->queue_mapping % self->aq_nic_cfg.vecs;
+   unsigned int tc = 0U;
+   int err = 0;
+   bool is_nic_in_bad_state;
+   bool is_locked = false;
+   bool is_busy = false;
+   struct aq_ring_buff_s buffers[AQ_CFG_SKB_FRAGS_MAX];
+
+   frags = skb_shinfo(skb)->nr_frags + 1;
+
+   ring = self->aq_ring_tx[AQ_NIC_TCVEC2RING(self, tc, vec)];
+
+   atomic_inc(>busy_count);
+   is_busy = true;
+
+   if (frags > AQ_CFG_SKB_FRAGS_MAX) {
+   dev_kfree_skb_any(skb);
+   goto err_exit;
+   }
+
+   is_nic_in_bad_state = AQ_OBJ_TST(self, AQ_NIC_FLAGS_IS_NOT_TX_READY) ||
+   (aq_ring_avail_dx(ring) < AQ_CFG_SKB_FRAGS_MAX);
+
+   if (is_nic_in_bad_state) {
+   aq_nic_ndev_queue_stop(self, ring->idx);
+   err = NETDEV_TX_BUSY;
+   goto err_exit;
+   }
+

Usage of this internal block is not common (unless it is under #ifdef,
and also not very common also in that case). I suggest move "unsigned
int trys" to the variables definitions in the beginning of the method
and remove the opening and closing brackets of the following block:

+   {
+   unsigned int trys = AQ_CFG_LOCK_TRYS;
+
+   frags = aq_nic_map_skb(self, skb, [0]);
+
+   do {
+   is_locked = spin_trylock(>lock);
+   } while (--trys && !is_locked);
+   if (!(is_locked)) {
+   err = NETDEV_TX_BUSY;
+   goto err_exit;
+   }
+

Yes, this is better.

Usually you don't let the mtu be less than 68, for example:
http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/i40e/i40e_main.c#L2246
See also RFV 791:
https://tools.ietf.org/html/rfc791



+int aq_nic_set_mtu(struct aq_nic_s *self, int new_mtu)
+{
+   int err = 0;
+
+   if (new_mtu > self->aq_hw_caps.mtu) {
+   err = 0;
+   goto err_exit;
+   }
+   self->aq_nic_cfg.mtu = new_mtu;
+
+err_exit:
+   return err;
+}

Clearly a must--thanks!

+
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.h 
b/drivers/net/ethernet/aquantia/atlantic/aq_nic.h
new file mode 100644
index 000..89958e7
--- /dev/null
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.h
@@ -0,0 +1,111 @@
+/*
+ * 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.
+ */
+
+/*

Should be, of course, aq_nic.h:


+ * File aq_nic.c: Declaration of common code for NIC.
+ */
+
Good point. Better still, including the name of the file has little 
value and makes the comment incorrect if it gets renamed. So, thanks!

Regards,
Rami Rosen



--
David VL

Re: [PATCH 05/12] Support for NIC-specific code

2016-12-27 Thread Rami Rosen

Hi, David,

Several nitpicks and comments, from a brief overview:

The commented label //err_exit:  should be removed
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> @@ -0,0 +1,993 @@
> +//err_exit:
> +//err_exit:

Shouldn't aq_nic_rss_init() be static? isn't it called only from
aq_nic_cfg_init_defaults()?
and it always returns 0, shouldn't it be void as well ? (+ remove
checking the return code when invoking it in
aq_nic_cfg_init_defaults())

> +int aq_nic_rss_init(struct aq_nic_s *self, unsigned int num_rss_queues)
> +{
> +   struct aq_nic_cfg_s *cfg = >aq_nic_cfg;
> +   struct aq_receive_scale_parameters *rss_params = >aq_rss;
> +   int i = 0;
> +
...
> +   return 0;
> +}


Shouldn't aq_nic_ndev_alloc() be static ? Isn't it invoked only from
aq_nic_alloc_cold()?

> +struct net_device *aq_nic_ndev_alloc(void)
> +{
...
> +}



> +
> +static unsigned int aq_nic_map_skb_lso(struct aq_nic_s *self,
> +  struct sk_buff *skb,
> +  struct aq_ring_buff_s *dx)
> +{
> +   unsigned int ret = 0U;
> +
> +   dx->flags = 0U;
> +   dx->len_pkt = skb->len;
> +   dx->len_l2 = ETH_HLEN;
> +   dx->len_l3 = ip_hdrlen(skb);
> +   dx->len_l4 = tcp_hdrlen(skb);
> +   dx->mss = skb_shinfo(skb)->gso_size;
> +   dx->is_txc = 1U;
> +   ret = 1U;
> +
Why not remove this "ret" variable, and simply return 1 ? the method
always returns 1:

> +   return ret;
> +}
> +

> +int aq_nic_xmit(struct aq_nic_s *self, struct sk_buff *skb)
> +{
> +   struct aq_ring_s *ring = NULL;
> +   unsigned int frags = 0U;
> +   unsigned int vec = skb->queue_mapping % self->aq_nic_cfg.vecs;
> +   unsigned int tc = 0U;
> +   int err = 0;
> +   bool is_nic_in_bad_state;
> +   bool is_locked = false;
> +   bool is_busy = false;
> +   struct aq_ring_buff_s buffers[AQ_CFG_SKB_FRAGS_MAX];
> +
> +   frags = skb_shinfo(skb)->nr_frags + 1;
> +
> +   ring = self->aq_ring_tx[AQ_NIC_TCVEC2RING(self, tc, vec)];
> +
> +   atomic_inc(>busy_count);
> +   is_busy = true;
> +
> +   if (frags > AQ_CFG_SKB_FRAGS_MAX) {
> +   dev_kfree_skb_any(skb);
> +   goto err_exit;
> +   }
> +
> +   is_nic_in_bad_state = AQ_OBJ_TST(self, AQ_NIC_FLAGS_IS_NOT_TX_READY) 
> ||
> +   (aq_ring_avail_dx(ring) < 
> AQ_CFG_SKB_FRAGS_MAX);
> +
> +   if (is_nic_in_bad_state) {
> +   aq_nic_ndev_queue_stop(self, ring->idx);
> +   err = NETDEV_TX_BUSY;
> +   goto err_exit;
> +   }
> +

Usage of this internal block is not common (unless it is under #ifdef,
and also not very common also in that case). I suggest move "unsigned
int trys" to the variables definitions in the beginning of the method
and remove the opening and closing brackets of the following block:
> +   {
> +   unsigned int trys = AQ_CFG_LOCK_TRYS;
> +
> +   frags = aq_nic_map_skb(self, skb, [0]);
> +
> +   do {
> +   is_locked = spin_trylock(>lock);
> +   } while (--trys && !is_locked);
> +   if (!(is_locked)) {
> +   err = NETDEV_TX_BUSY;
> +   goto err_exit;
> +   }
> +

Usually you don't let the mtu be less than 68, for example:
http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/i40e/i40e_main.c#L2246
See also RFV 791:
https://tools.ietf.org/html/rfc791


> +int aq_nic_set_mtu(struct aq_nic_s *self, int new_mtu)
> +{
> +   int err = 0;
> +
> +   if (new_mtu > self->aq_hw_caps.mtu) {
> +   err = 0;
> +   goto err_exit;
> +   }
> +   self->aq_nic_cfg.mtu = new_mtu;
> +
> +err_exit:
> +   return err;
> +}

> +

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.h 
> b/drivers/net/ethernet/aquantia/atlantic/aq_nic.h
> new file mode 100644
> index 000..89958e7
> --- /dev/null
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.h
> @@ -0,0 +1,111 @@
> +/*
> + * 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.
> + */
> +
> +/*

Should be, of course, aq_nic.h:

> + * File aq_nic.c: Declaration of common code for NIC.
> + */
> +

Regards,
Rami Rosen

[PATCH 05/12] Support for NIC-specific code

2016-12-27 Thread David VomLehn

Add support for code specific to the Atlantic NIC.

Signed-off-by: Dmitrii Tarakanov 
Signed-off-by: Alexander Loktionov 
Signed-off-by: David M. VomLehn 
---
 drivers/net/ethernet/aquantia/atlantic/aq_nic.c| 993 +
 drivers/net/ethernet/aquantia/atlantic/aq_nic.h| 111 +++
 .../ethernet/aquantia/atlantic/aq_nic_internal.h   |  48 +
 3 files changed, 1152 insertions(+)
 create mode 100644 drivers/net/ethernet/aquantia/atlantic/aq_nic.c
 create mode 100644 drivers/net/ethernet/aquantia/atlantic/aq_nic.h
 create mode 100644 drivers/net/ethernet/aquantia/atlantic/aq_nic_internal.h

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c 
b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
new file mode 100644
index 000..bc4c672
--- /dev/null
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -0,0 +1,993 @@
+//err_exit:
+//err_exit:
+/*
+ * 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_nic.c: Definition of common code for NIC.
+ */
+
+#include "aq_nic.h"
+#include "aq_vec.h"
+#include "aq_ring.h"
+#include "aq_hw.h"
+#include "aq_pci_func.h"
+#include "aq_rss.h"
+#include "aq_nic_internal.h"
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/*
+ * Fills aq_nic_cfg with valid defaults
+ */
+static int aq_nic_cfg_init_defaults(struct aq_nic_s *self)
+{
+   struct aq_nic_cfg_s *cfg = >aq_nic_cfg;
+   int err = 0;
+
+   cfg->aq_hw_caps = >aq_hw_caps;
+
+   cfg->vecs = AQ_CFG_VECS_DEF;
+   cfg->tcs = AQ_CFG_TCS_DEF;
+
+   cfg->rxds = AQ_CFG_RXDS_DEF;
+   cfg->txds = AQ_CFG_TXDS_DEF;
+
+   cfg->is_polling = AQ_CFG_IS_POLLING_DEF;
+
+   cfg->is_interrupt_moderation = AQ_CFG_IS_INTERRUPT_MODERATION_DEF;
+   cfg->itr = cfg->is_interrupt_moderation ?
+   AQ_CFG_INTERRUPT_MODERATION_RATE_DEF : 0U;
+
+   cfg->is_rss = AQ_CFG_IS_RSS_DEF;
+   cfg->num_rss_queues = AQ_CFG_NUM_RSS_QUEUES_DEF;
+   cfg->aq_rss.base_cpu_number = AQ_CFG_RSS_BASE_CPU_NUM_DEF;
+   cfg->flow_control = AQ_CFG_FC_MODE;
+
+   cfg->mtu = AQ_CFG_MTU_DEF;
+   cfg->link_speed_msk = AQ_CFG_SPEED_MSK;
+   cfg->is_autoneg = AQ_CFG_IS_AUTONEG_DEF;
+
+   cfg->vlan_id = 0U;
+
+   err = aq_nic_rss_init(self, cfg->num_rss_queues);
+   if (err < 0)
+   goto err_exit;
+
+err_exit:
+   return err;
+}
+
+/*
+ * Checks hw_caps and 'corrects' aq_nic_cfg in runtime
+ */
+int aq_nic_cfg_start(struct aq_nic_s *self)
+{
+   struct aq_nic_cfg_s *cfg = >aq_nic_cfg;
+
+   /*descriptors */
+   cfg->rxds = min(cfg->rxds, cfg->aq_hw_caps->rxds);
+   cfg->txds = min(cfg->txds, cfg->aq_hw_caps->txds);
+
+   /*rss rings */
+   cfg->vecs = min(cfg->vecs, cfg->aq_hw_caps->vecs);
+   cfg->vecs = min(cfg->vecs, num_online_cpus());
+
+   cfg->irq_type = aq_pci_func_get_irq_type(self->aq_pci_func);
+
+   if ((cfg->irq_type == AQ_IRQ_LEGACY) ||
+   (self->aq_hw_caps.vecs == 1U) ||
+   (cfg->vecs == 1U)) {
+   cfg->is_rss = 0U;
+   cfg->vecs = 1U;
+   }
+
+   cfg->link_speed_msk &= self->aq_hw_caps.link_speed_msk;
+
+   return 0;
+}
+
+static int aq_nic_cfg_read(struct aq_nic_s *self)
+{
+   return 0;
+}
+
+static int aq_nic_cfg_check(struct aq_nic_s *self)
+{
+   return 0;
+}
+
+int aq_nic_rss_init(struct aq_nic_s *self, unsigned int num_rss_queues)
+{
+   struct aq_nic_cfg_s *cfg = >aq_nic_cfg;
+   struct aq_receive_scale_parameters *rss_params = >aq_rss;
+   int i = 0;
+
+   static u8 rss_key[40] = {
+   0x1e, 0xad, 0x71, 0x87, 0x65, 0xfc, 0x26, 0x7d,
+   0x0d, 0x45, 0x67, 0x74, 0xcd, 0x06, 0x1a, 0x18,
+   0xb6, 0xc1, 0xf0, 0xc7, 0xbb, 0x18, 0xbe, 0xf8,
+   0x19, 0x13, 0x4b, 0xa9, 0xd0, 0x3e, 0xfe, 0x70,
+   0x25, 0x03, 0xab, 0x50, 0x6a, 0x8b, 0x82, 0x0c
+   };
+
+   rss_params->hash_secret_key_size = sizeof(rss_key);
+   memcpy(rss_params->hash_secret_key, rss_key, sizeof(rss_key));
+   rss_params->indirection_table_size = AQ_CFG_RSS_INDIRECTION_TABLE_MAX;
+
+   for (i = rss_params->indirection_table_size; i--;)
+   rss_params->indirection_table[i] = i & (num_rss_queues - 1);
+   return 0;
+}
+
+static void aq_nic_service_timer_cb(unsigned long param)
+{
+   struct aq_nic_s *self = (struct aq_nic_s *)param;
+   int err = 0;
+   bool is_busy = false;
+   struct aq_hw_link_status_s link_status;
+
+   atomic_inc(>busy_count);
+   is_busy = true;
+   if (AQ_OBJ_TST(self, AQ_NIC_FLAGS_IS_NOT_READY)) {
+

Re: [PATCH 05/12] Support for NIC-specific code

Re: [PATCH 05/12] Support for NIC-specific code

[PATCH 05/12] Support for NIC-specific code

3 matches

Site Navigation

Mail list logo

Footer information