| 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
| ||
|
Message-ID: <3dfe11ac-4215-21d0-2242-7a4ad6157e56@gmail.com>
Date: Fri, 13 Jan 2017 18:00:36 -0800
From: Florian Fainelli <f.fainelli@...il.com>
To: Alexander Loktionov <Alexander.Loktionov@...antia.com>,
netdev@...r.kernel.org, David VomLehn <vomlehn@...as.net>
Cc: "David S . Miller" <davem@...emloft.net>,
Simon Edelhaus <Simon.Edelhaus@...antia.com>,
Dmitrii Tarakanov <Dmitrii.Tarakanov@...antia.com>,
Pavel Belous <Pavel.Belous@...antia.com>,
Dmitry Bezrukov <Dmitry.Bezrukov@...antia.com>
Subject: Re: [PATCH v5 05/13] net: ethernet: aquantia: Support for
NIC-specific code
On 01/12/2017 09:02 PM, Alexander Loktionov wrote:
> From: David VomLehn <vomlehn@...as.net>
>
> Add support for code specific to the Atlantic NIC.
>
> Signed-off-by: Alexander Loktionov <Alexander.Loktionov@...antia.com>
> Signed-off-by: Dmitrii Tarakanov <Dmitrii.Tarakanov@...antia.com>
> Signed-off-by: Pavel Belous <Pavel.Belous@...antia.com>
> Signed-off-by: Dmitry Bezrukov <Dmitry.Bezrukov@...antia.com>
> Signed-off-by: David M. VomLehn <vomlehn@...as.net>
> ---
> drivers/net/ethernet/aquantia/aq_main.c | 291 ++++++++
> drivers/net/ethernet/aquantia/aq_main.h | 17 +
> drivers/net/ethernet/aquantia/aq_nic.c | 910 ++++++++++++++++++++++++
> drivers/net/ethernet/aquantia/aq_nic.h | 108 +++
> drivers/net/ethernet/aquantia/aq_nic_internal.h | 46 ++
> 5 files changed, 1372 insertions(+)
> create mode 100644 drivers/net/ethernet/aquantia/aq_main.c
> create mode 100644 drivers/net/ethernet/aquantia/aq_main.h
> create mode 100644 drivers/net/ethernet/aquantia/aq_nic.c
> create mode 100644 drivers/net/ethernet/aquantia/aq_nic.h
> create mode 100644 drivers/net/ethernet/aquantia/aq_nic_internal.h
>
> diff --git a/drivers/net/ethernet/aquantia/aq_main.c b/drivers/net/ethernet/aquantia/aq_main.c
> new file mode 100644
> index 0000000..18a6012
> --- /dev/null
> +++ b/drivers/net/ethernet/aquantia/aq_main.c
> @@ -0,0 +1,291 @@
> +/*
> + * aQuantia Corporation Network Driver
> + * Copyright (C) 2014-2017 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_main.c: Main file for aQuantia Linux driver. */
> +
> +#include "aq_main.h"
> +#include "aq_nic.h"
> +#include "aq_pci_func.h"
> +#include "aq_ethtool.h"
> +#include "hw_atl/hw_atl_a0.h"
> +#include "hw_atl/hw_atl_b0.h"
> +
> +#include <linux/netdevice.h>
> +#include <linux/module.h>
> +
> +static const struct pci_device_id aq_pci_tbl[] = {
> + { PCI_VDEVICE(AQUANTIA, HW_ATL_DEVICE_ID_0001), },
> + { PCI_VDEVICE(AQUANTIA, HW_ATL_DEVICE_ID_D100), },
> + { PCI_VDEVICE(AQUANTIA, HW_ATL_DEVICE_ID_D107), },
> + { PCI_VDEVICE(AQUANTIA, HW_ATL_DEVICE_ID_D108), },
> + { PCI_VDEVICE(AQUANTIA, HW_ATL_DEVICE_ID_D109), },
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(pci, aq_pci_tbl);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_VERSION(AQ_CFG_DRV_VERSION);
> +MODULE_AUTHOR(AQ_CFG_DRV_AUTHOR);
> +MODULE_DESCRIPTION(AQ_CFG_DRV_DESC);
Inline the defines directly into that file, and these typically go at
the end of the file.
> +
> +static struct aq_hw_ops *aq_pci_probe_get_hw_ops_by_id(struct pci_dev *pdev)
> +{
> + struct aq_hw_ops *ops = NULL;
> + int err = 0;
> +
> + ops = hw_atl_a0_get_ops_by_id(pdev);
> + if (ops) {
> + err = 0;
> + goto err_exit;
> + }
Should not that come from the pci_device_id .driver_data structure?
> +
> + ops = hw_atl_b0_get_ops_by_id(pdev);
> + if (ops) {
> + err = 0;
> + goto err_exit;
> + }
> +
> +/* the H/W was not recognized */
> + err = -EFAULT;
> +
> +err_exit:
> + return ops;
> +}
> +
> +static int aq_ndev_open(struct net_device *ndev)
> +{
> + struct aq_nic_s *aq_nic = NULL;
> + int err = 0;
> +
> + aq_nic = aq_nic_alloc_hot(ndev);
> + if (!aq_nic) {
> + err = -ENOMEM;
> + goto err_exit;
> + }
> + err = aq_nic_init(aq_nic);
> + if (err < 0)
> + goto err_exit;
> + err = aq_nic_start(aq_nic);
> + if (err < 0)
> + goto err_exit;
> +
> +err_exit:
> + if (err < 0) {
Just make the error label be taken only if there is an error instead of
checking here again for a negative error code.
> + if (aq_nic)
> + aq_nic_deinit(aq_nic);
> + }
> + return err;
> +}
> +
> +static int aq_ndev_close(struct net_device *ndev)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
No casting needed.
> +
> + aq_nic_stop(aq_nic);
> + aq_nic_deinit(aq_nic);
> + aq_nic_free_hot_resources(aq_nic);
> +
> + return 0;
> +}
> +
> +static int aq_ndev_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> + int err = 0;
> +
> + err = aq_nic_xmit(aq_nic, skb);
> + if (err < 0)
> + goto err_exit;
> +
> +err_exit:
> + return err;
Just inline the contents of aq_nic_xmit(), this function serves nothing
useful here.
> +}
> +
> +static int aq_ndev_change_mtu(struct net_device *ndev, int new_mtu)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> + int err = 0;
> +
> + if (new_mtu == ndev->mtu) {
> + err = 0;
> + goto err_exit;
> + }
> + if (new_mtu < 68) {
> + err = -EINVAL;
> + goto err_exit;
> + }
What's so special about 68 here?
> + err = aq_nic_set_mtu(aq_nic, new_mtu + ETH_HLEN);
Why do not need to include ETH_HLEN here?
> + if (err < 0)
> + goto err_exit;
> + ndev->mtu = new_mtu;
> +
> + if (netif_running(ndev)) {
> + aq_ndev_close(ndev);
> + aq_ndev_open(ndev);
> + }
> +
> +err_exit:
> + return err;
> +}
> +
> +static int aq_ndev_set_features(struct net_device *ndev,
> + netdev_features_t features)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> + struct aq_nic_cfg_s *aq_cfg = aq_nic_get_cfg(aq_nic);
> + bool is_lro = false;
> +
> + if (aq_cfg->hw_features & NETIF_F_LRO) {
> + is_lro = features & NETIF_F_LRO;
> +
> + if (aq_cfg->is_lro != is_lro) {
> + aq_cfg->is_lro = is_lro;
> +
> + if (netif_running(ndev)) {
> + aq_ndev_close(ndev);
> + aq_ndev_open(ndev);
> + }
Can this be made less disruptive to the network interface?
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int aq_ndev_set_mac_address(struct net_device *ndev, void *addr)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> + int err = 0;
> +
> + err = eth_mac_addr(ndev, addr);
> + if (err < 0)
> + goto err_exit;
> + err = aq_nic_set_mac(aq_nic, ndev);
> + if (err < 0)
> + goto err_exit;
> +
> +err_exit:
> + return err;
> +}
> +
> +static void aq_ndev_set_multicast_settings(struct net_device *ndev)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> + int err = 0;
> +
> + err = aq_nic_set_packet_filter(aq_nic, ndev->flags);
> + if (err < 0)
> + goto err_exit;
> +
> + if (netdev_mc_count(ndev)) {
> + err = aq_nic_set_multicast_list(aq_nic, ndev);
> + if (err < 0)
> + goto err_exit;
> + }
That's not enough, you need to deal with Unicast address filtering,
promiscuous mode, and if you run out of space in your multicast address
filter.
--
Florian
Powered by blists - more mailing lists