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

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
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAKoUArn+zfeU16SgJbPxh1f0ud+M8ERi2j6c34cqeO=+LGjuCw@mail.gmail.com>
Date:   Wed, 28 Dec 2016 07:21:56 +0200
From:   Rami Rosen <roszenrami@...il.com>
To:     David VomLehn <vomlehn@...as.net>
Cc:     Netdev <netdev@...r.kernel.org>,
        Simon Edelhaus <Simon.Edelhaus@...antia.com>,
        Dmitrii Tarakanov <Dmitrii.Tarakanov@...antia.com>,
        Alexander Loktionov <Alexander.Loktionov@...antia.com>
Subject: Re: [PATCH 05/12] Support for NIC-specific code

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 = &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()?

> +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(&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;
> +               }
> +

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 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.
> + */
> +

Regards,
Rami Rosen