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 = &self->aq_nic_cfg;
+ struct aq_receive_scale_parameters *rss_params = &cfg->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(&self->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, &buffers[0]);
+
+ do {
+ is_locked = spin_trylock(&ring->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 0000000..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
