* Re: [PATCH v5 01/13] net: ethernet: aquantia: Make and configuration files.
From: Florian Fainelli @ 2017-01-14 2:01 UTC (permalink / raw)
To: Alexander Loktionov, netdev, David VomLehn
Cc: David S . Miller, Simon Edelhaus, Dmitrii Tarakanov, Pavel Belous,
Dmitry Bezrukov
In-Reply-To: <fe1664e502d28bcb6a5689787b8000e6c9288307.1484283610.git.vomlehn@texas.net>
On 01/12/2017 09:02 PM, Alexander Loktionov wrote:
> From: David VomLehn <vomlehn@texas.net>
>
> Patches to create the make and configuration files.
>
> Signed-off-by: Alexander Loktionov <Alexander.Loktionov@aquantia.com>
> Signed-off-by: Dmitrii Tarakanov <Dmitrii.Tarakanov@aquantia.com>
> Signed-off-by: Pavel Belous <Pavel.Belous@aquantia.com>
> Signed-off-by: Dmitry Bezrukov <Dmitry.Bezrukov@aquantia.com>
> Signed-off-by: David M. VomLehn <vomlehn@texas.net>
> ---
> drivers/net/ethernet/aquantia/Kconfig | 24 +++++++++++++++++++
> drivers/net/ethernet/aquantia/Makefile | 42 ++++++++++++++++++++++++++++++++++
> drivers/net/ethernet/aquantia/ver.h | 18 +++++++++++++++
> 3 files changed, 84 insertions(+)
> create mode 100644 drivers/net/ethernet/aquantia/Kconfig
> create mode 100644 drivers/net/ethernet/aquantia/Makefile
> create mode 100644 drivers/net/ethernet/aquantia/ver.h
>
> diff --git a/drivers/net/ethernet/aquantia/Kconfig b/drivers/net/ethernet/aquantia/Kconfig
> new file mode 100644
> index 0000000..a74a4c0
> --- /dev/null
> +++ b/drivers/net/ethernet/aquantia/Kconfig
> @@ -0,0 +1,24 @@
> +#
> +# aQuantia device configuration
> +#
> +
> +config NET_VENDOR_AQUANTIA
> + bool "aQuantia devices"
> + default y
> + ---help---
> + Set this to y if you have an Ethernet network cards that uses the aQuantia
> + chipset.
Could be interesting specify which specific commercial products available.
> +
> + This option does not build any drivers; it casues the aQuantia
> + drivers that can be built to appear in the list of Ethernet drivers.
> +
> +
> +if NET_VENDOR_AQUANTIA
> +
> +config AQTION
> + tristate "aQuantia AQtion Support"
> + depends on PCI
> + ---help---
> + This enables the support for the aQuantia AQtion Ethernet card.
> +
> +endif # NET_VENDOR_AQUANTIA
> diff --git a/drivers/net/ethernet/aquantia/Makefile b/drivers/net/ethernet/aquantia/Makefile
> new file mode 100644
> index 0000000..e4ae696
> --- /dev/null
> +++ b/drivers/net/ethernet/aquantia/Makefile
> @@ -0,0 +1,42 @@
> +################################################################################
> +#
> +# aQuantia Ethernet Controller AQtion Linux Driver
> +# Copyright(c) 2014-2017 aQuantia Corporation.
> +#
> +# 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.
> +#
> +# This program is distributed in the hope it will be useful, but WITHOUT
> +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> +# more details.
> +#
> +# You should have received a copy of the GNU General Public License along
> +# with this program. If not, see <http://www.gnu.org/licenses/>.
> +#
> +# The full GNU General Public License is included in this distribution in
> +# the file called "COPYING".
> +#
> +# Contact Information: <rdc-drv@aquantia.com>
> +# aQuantia Corporation, 105 E. Tasman Dr. San Jose, CA 95134, USA
> +#
> +################################################################################
> +
> +#
> +# Makefile for the AQtion(tm) Ethernet driver
> +#
> +
> +obj-$(CONFIG_AQTION) += atlantic.o
> +
> +atlantic-objs := aq_main.o \
> + aq_nic.o \
> + aq_pci_func.o \
> + aq_vec.o \
> + aq_ring.o \
> + aq_hw_utils.o \
> + aq_ethtool.o \
> + hw_atl/hw_atl_a0.o \
> + hw_atl/hw_atl_b0.o \
> + hw_atl/hw_atl_utils.o \
> + hw_atl/hw_atl_llh.o
> diff --git a/drivers/net/ethernet/aquantia/ver.h b/drivers/net/ethernet/aquantia/ver.h
> new file mode 100644
> index 0000000..636d646
> --- /dev/null
> +++ b/drivers/net/ethernet/aquantia/ver.h
> @@ -0,0 +1,18 @@
> +/*
> + * 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.
> + */
> +
> +#ifndef VER_H
> +#define VER_H
> +
> +#define NIC_MAJOR_DRIVER_VERSION 1
> +#define NIC_MINOR_DRIVER_VERSION 5
> +#define NIC_BUILD_DRIVER_VERSION 339
> +#define NIC_REVISION_DRIVER_VERSION 0
That probably means nothing since it's the first time for the upstream
kernel that such a driver exists. Defining versions of software is such
an interesting subject anyway!
--
Florian
^ permalink raw reply
* Re: [PATCH v5 05/13] net: ethernet: aquantia: Support for NIC-specific code
From: Florian Fainelli @ 2017-01-14 2:00 UTC (permalink / raw)
To: Alexander Loktionov, netdev, David VomLehn
Cc: David S . Miller, Simon Edelhaus, Dmitrii Tarakanov, Pavel Belous,
Dmitry Bezrukov
In-Reply-To: <5e87af3b0effb96698d0c085366b1ba9320a6305.1484283610.git.vomlehn@texas.net>
On 01/12/2017 09:02 PM, Alexander Loktionov wrote:
> From: David VomLehn <vomlehn@texas.net>
>
> Add support for code specific to the Atlantic NIC.
>
> Signed-off-by: Alexander Loktionov <Alexander.Loktionov@aquantia.com>
> Signed-off-by: Dmitrii Tarakanov <Dmitrii.Tarakanov@aquantia.com>
> Signed-off-by: Pavel Belous <Pavel.Belous@aquantia.com>
> Signed-off-by: Dmitry Bezrukov <Dmitry.Bezrukov@aquantia.com>
> Signed-off-by: David M. VomLehn <vomlehn@texas.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
^ permalink raw reply
* Re: [PATCH v5 08/13] net: ethernet: aquantia: PCI operations
From: Florian Fainelli @ 2017-01-14 1:48 UTC (permalink / raw)
To: Alexander Loktionov, netdev, David VomLehn
Cc: David S . Miller, Simon Edelhaus, Dmitrii Tarakanov, Pavel Belous,
Dmitry Bezrukov
In-Reply-To: <2889e8e87b8b89945f34e57a7f8ca253f204de47.1484283610.git.vomlehn@texas.net>
On 01/12/2017 09:02 PM, Alexander Loktionov wrote:
> From: David VomLehn <vomlehn@texas.net>
>
> Add functions that handle the PCI bus interface.
>
> Signed-off-by: Alexander Loktionov <Alexander.Loktionov@aquantia.com>
> Signed-off-by: Dmitrii Tarakanov <Dmitrii.Tarakanov@aquantia.com>
> Signed-off-by: Pavel Belous <Pavel.Belous@aquantia.com>
> Signed-off-by: Dmitry Bezrukov <Dmitry.Bezrukov@aquantia.com>
> Signed-off-by: David M. VomLehn <vomlehn@texas.net>
> ---
> + /*enable interrupts */
> +#if AQ_CFG_FORCE_LEGACY_INT
> + self->irq_type = AQ_IRQ_LEGACY;
> +#else
> + err = pci_enable_msix(self->pdev, self->msix_entry,
> + self->aq_hw_caps.msix_irqs);
You should really try MSI(x) first and fallback to leagcy PCI INT# if
you cannot succeed, asking for a driver rebuild to switch between these
behaviors may not be an option for most people (a module parameter may
be an option but is usually not such a great idea).
--
Florian
^ permalink raw reply
* Re: [PATCH v5 11/13] net: ethernet: aquantia: Ethtool support
From: Florian Fainelli @ 2017-01-14 1:47 UTC (permalink / raw)
To: Alexander Loktionov, netdev, David VomLehn
Cc: David S . Miller, Simon Edelhaus, Dmitrii Tarakanov, Pavel Belous,
Dmitry Bezrukov
In-Reply-To: <b2236a13fcf9ac65f15cfc084de670948517e003.1484283610.git.vomlehn@texas.net>
On 01/12/2017 09:02 PM, Alexander Loktionov wrote:
> From: David VomLehn <vomlehn@texas.net>
>
> Add the driver interfaces required for support by the ethtool utility.
>
> Signed-off-by: Alexander Loktionov <Alexander.Loktionov@aquantia.com>
> Signed-off-by: Dmitrii Tarakanov <Dmitrii.Tarakanov@aquantia.com>
> Signed-off-by: Pavel Belous <Pavel.Belous@aquantia.com>
> Signed-off-by: Dmitry Bezrukov <Dmitry.Bezrukov@aquantia.com>
> Signed-off-by: David M. VomLehn <vomlehn@texas.net>
> ---
> drivers/net/ethernet/aquantia/aq_ethtool.c | 250 +++++++++++++++++++++++++++++
> drivers/net/ethernet/aquantia/aq_ethtool.h | 19 +++
> 2 files changed, 269 insertions(+)
> create mode 100644 drivers/net/ethernet/aquantia/aq_ethtool.c
> create mode 100644 drivers/net/ethernet/aquantia/aq_ethtool.h
>
> diff --git a/drivers/net/ethernet/aquantia/aq_ethtool.c b/drivers/net/ethernet/aquantia/aq_ethtool.c
> new file mode 100644
> index 0000000..f11bdb1
> --- /dev/null
> +++ b/drivers/net/ethernet/aquantia/aq_ethtool.c
> @@ -0,0 +1,250 @@
> +/*
> + * 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_ethtool.c: Definition of ethertool related functions. */
> +
> +#include "aq_ethtool.h"
> +#include "aq_nic.h"
> +
> +static void aq_ethtool_get_regs(struct net_device *ndev,
> + struct ethtool_regs *regs, void *p)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
netdev_priv() returns a void * which requires no casting, please fix
this through the entire 13 patches.
> + u32 regs_count = aq_nic_get_regs_count(aq_nic);
> +
> + memset(p, 0, regs_count * sizeof(u32));
> + aq_nic_get_regs(aq_nic, regs, p);
> +}
> +
> +static int aq_ethtool_get_regs_len(struct net_device *ndev)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> + u32 regs_count = aq_nic_get_regs_count(aq_nic);
> +
> + return regs_count * sizeof(u32);
> +}
> +
> +static u32 aq_ethtool_get_link(struct net_device *ndev)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> +
> + return aq_nic_get_link_speed(aq_nic) ? 1U : 0U;
Can you either use PHYLIB to interface to your PHY (which does all the
nice state machine management) or at the very least ethtool_op_get_link()?
> +}
> +
> +static int aq_ethtool_get_settings(struct net_device *ndev,
> + struct ethtool_cmd *cmd)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> +
> + cmd->port = PORT_TP;
> + cmd->transceiver = XCVR_EXTERNAL;
> +
> + ethtool_cmd_speed_set(cmd, netif_carrier_ok(ndev) ?
> + aq_nic_get_link_speed(aq_nic) : 0U);
> +
> + cmd->duplex = DUPLEX_FULL;
> + aq_nic_get_link_settings(aq_nic, cmd);
> + return 0;
Consider switching to the new ksettings API and filling in a bit more
information like cmd->autoneg?
> +}
> +
> +static int aq_ethtool_set_settings(struct net_device *ndev,
> + struct ethtool_cmd *cmd)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> +
> + return aq_nic_set_link_settings(aq_nic, cmd);
> +}
> +
> +static const char aq_ethtool_stat_names[][ETH_GSTRING_LEN] = {
> + "InPackets",
> + "InUCast",
> + "InMCast",
> + "InBCast",
> + "InErrors",
> + "OutPackets",
> + "OutUCast",
> + "OutMCast",
> + "OutBCast",
> + "InUCastOctects",
> + "OutUCastOctects",
> + "InMCastOctects",
> + "OutMCastOctects",
> + "InBCastOctects",
> + "OutBCastOctects",
> + "InOctects",
> + "OutOctects",
> + "InPacketsDma",
> + "OutPacketsDma",
> + "InOctetsDma",
> + "OutOctetsDma",
> + "InDroppedDma",
> + "Queue[0] InPackets",
> + "Queue[0] OutPackets",
> + "Queue[0] InJumboPackets",
> + "Queue[0] InLroPackets",
> + "Queue[0] InErrors",
> +#if 1 < AQ_CFG_VECS_DEF
Yoda notations are usually frowned upon. Instead of making this decision
here, can you push that down to the actual function reading the statistics?
> +static void aq_ethtool_get_strings(struct net_device *ndev,
> + u32 stringset, u8 *data)
> +{
You need to check that stringset == ETH_SS_STATS here since that's the
only thing you support. ethtool usually does not do it if the ioctl()
returning the data does not exist, but other bogus applications might.
> + memcpy(data, *aq_ethtool_stat_names, sizeof(aq_ethtool_stat_names));
> +}
> +
> +static int aq_ethtool_get_sset_count(struct net_device *ndev, int stringset)
> +{
Same here.
> + return ARRAY_SIZE(aq_ethtool_stat_names);
> +#ifndef AQ_ETHTOOL_H
> +#define AQ_ETHTOOL_H
> +
> +#include "aq_common.h"
> +
> +extern const struct ethtool_ops aq_ethtool_ops;
The extern does not appear necessary here. Alternatively you may define
all the ethtool function pointers in this header file, but leave the
struct ethtool_ops in the main driver file which registers them at the
time of the netdev creation, up to you.
> +
> +#endif /* AQ_ETHTOOL_H */
>
--
Florian
^ permalink raw reply
* Re: [PATCH v5 00/13] net: ethernet: aquantia: Add AQtion 2.5/5 GB NIC driver
From: Florian Fainelli @ 2017-01-14 1:38 UTC (permalink / raw)
To: Alexander Loktionov, netdev, David VomLehn
Cc: David S . Miller, Simon Edelhaus, Dmitrii Tarakanov, Pavel Belous
In-Reply-To: <cover.1484283610.git.vomlehn@texas.net>
On 01/12/2017 09:02 PM, Alexander Loktionov wrote:
> From: David VomLehn <vomlehn@texas.net>
>
> v1: Initial version
> v2: o Make necessary drivers/net/ethernet changes to integrate software
> o Drop intermediate atlantic directory
> o Remove Makefile things only appropriate to out of tree module
> building
> v3: o Move changes to drivers/net/ethernet/{Kconfig,Makefile} to the last
> patch to ensure clean bisection.
> o Removed inline attribute aq_hw_write_req() as it was defined in
> only one .c file.
> o #included pci.h in aq_common.h to get struct pci definition.
> o Modified code to unlock based execution flow rather than using a
> flag.
> o Made a number of functions that were only used in a single file
> static.
> o Cleaned up error and return code handling in various places.
> o Remove AQ_CFG_IP_ALIGN definition.
> o Other minor code clean up.
> v4: o Using do_div for 64 bit division.
> o Modified NIC statistics code.
> o Using build_skb instead netdev_alloc_skb for single fragment
> packets.
> o Removed extra aq_nic.o from Makefile
> v5: o Removed extra newline at the end of the files.
> o Wrapped cover letter lines.
Have not looked at the driver yet, but the threading of your emails is
weird, each patch is in reply to the previous one. It would be more
natural to have all numbered patches be in reply to the cover letter,
which according to the version of git you seem to have used (2.7.4)
should already be the default. In graphical terms what we see right now is:
[PATCH 00/13]
[PATCH 01/13]
[PATCH 02/13]
....
While we should see:
[PATCH 00/13]
[PATCH 01/13]
[PATCH 02/13]
....
Can you fix that for future submissions, this may sound like a cosmetic
thing, but it really helps with threading/reading etc.
Thanks!
--
Florian
^ permalink raw reply
* Re: [PATCH v5 02/13] net: ethernet: aquantia: Common functions and definitions
From: David Miller @ 2017-01-14 1:32 UTC (permalink / raw)
To: Alexander.Loktionov
Cc: netdev, vomlehn, Simon.Edelhaus, Dmitrii.Tarakanov, Pavel.Belous,
Dmitry.Bezrukov
In-Reply-To: <2c056fd544958594f6356a17892c251ea26ea8c8.1484283610.git.vomlehn@texas.net>
From: Alexander Loktionov <Alexander.Loktionov@aquantia.com>
Date: Thu, 12 Jan 2017 21:02:18 -0800
> +#define AQ_OBJ_HEADER spinlock_t lock; atomic_t flags; atomic_t busy_count
> +
> +struct aq_obj_s {
> + AQ_OBJ_HEADER;
> +};
Please don't hide multiple declarations and types inside of a macro,
that makes the code harder to understand.
Use a sub-structure or similar, and pass that sub-structure to the
handlers.
> +#define AQ_OBJ_TST(_OBJ_, _FLAG_) ((_FLAG_) & atomic_read(&(_OBJ_)->flags))
> +
> +#define AQ_OBJ_SET(_OBJ_, _F_) \
...
> +#define AQ_OBJ_CLR(_OBJ_, _F_) \
Please don't reinvent the wheel.
Use test_bit, set_bit, clear_bit, test_and_set_bit, and
test_and_clear_bit. Using an atomic_t for flag bits is completely
inappropriate, that type is primarily meant for atomic counters.
The appropriate type for *_bit() operations is "unsigned long".
^ permalink raw reply
* Re: [PATCH] i40e: Invoke softirqs after napi_reschedule
From: David Miller @ 2017-01-14 1:27 UTC (permalink / raw)
To: bpoirier; +Cc: jeffrey.t.kirsher, intel-wired-lan, netdev
In-Reply-To: <20170113010414.22708-1-bpoirier@suse.com>
From: Benjamin Poirier <bpoirier@suse.com>
Date: Thu, 12 Jan 2017 17:04:14 -0800
> The following message is logged from time to time when using i40e:
> NOHZ: local_softirq_pending 08
>
> i40e may schedule napi from a workqueue. Afterwards, softirqs are not run
> in a deterministic time frame. The problem is the same as what was
> described in commit ec13ee80145c ("virtio_net: invoke softirqs after
> __napi_schedule") and this patch applies the same fix to i40e.
>
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
I hope to see this from one of Jeff's pull requests to me in the
near future.
^ permalink raw reply
* Re: [PATCH net-next] liquidio: use fallback for selecting txq
From: David Miller @ 2017-01-14 1:18 UTC (permalink / raw)
To: felix.manlunas; +Cc: netdev, raghu.vatsavayi, derek.chickles, satananda.burla
In-Reply-To: <20170113001822.GA10813@felix.cavium.com>
From: Felix Manlunas <felix.manlunas@cavium.com>
Date: Thu, 12 Jan 2017 16:18:22 -0800
> From: Satanand Burla <satananda.burla@cavium.com>
>
> Remove assignment to ndo_select_queue so that fallback is used for
> selecting txq. Also remove the now-useless function that used to be
> assigned to ndo_select_queue.
>
> Signed-off-by: Satanand Burla <satananda.burla@cavium.com>
> Signed-off-by: Felix Manlunas <felix.manlunas@cavium.com>
> Signed-off-by: Derek Chickles <derek.chickles@cavium.com>
Applied, thank you.
^ permalink raw reply
* Re: [PATCH net-next] net: dsa: mv88e6xxx: add EEPROM support to 6390
From: David Miller @ 2017-01-14 1:18 UTC (permalink / raw)
To: vivien.didelot
Cc: netdev, linux-kernel, kernel, f.fainelli, andrew, cphealy,
nikita.yoush
In-Reply-To: <20170112230716.2083-1-vivien.didelot@savoirfairelinux.com>
From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Thu, 12 Jan 2017 18:07:16 -0500
> The Marvell 6352 chip has a 8-bit address/16-bit data EEPROM access.
> The Marvell 6390 chip has a 16-bit address/8-bit data EEPROM access.
>
> This patch implements the 8-bit data EEPROM access in the mv88e6xxx
> driver and adds its support to chips of the 6390 family.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Applied.
^ permalink raw reply
* [PATCH] cxgb4: Remove redundant memset before memcpy
From: Shyam Saini @ 2017-01-14 1:17 UTC (permalink / raw)
To: hariprasad; +Cc: tklauser, netdev, linux-kernel, Shyam Saini
The region set by the call to memset, immediately overwritten by
the subsequent call to memcpy and thus makes the memset redundant.
Also remove the memset((&info, 0, sizeof(info)) on line 398 because
info is memcpy()'ed to before being used in the loop and it isn't
used outside of the loop.
Signed-off-by: Shyam Saini <mayhs11saini@gmail.com>
---
drivers/net/ethernet/chelsio/cxgb4/sched.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/net/ethernet/chelsio/cxgb4/sched.c b/drivers/net/ethernet/chelsio/cxgb4/sched.c
index cbd68a8..c902635 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/sched.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/sched.c
@@ -397,9 +397,6 @@ static struct sched_class *t4_sched_class_lookup(struct port_info *pi,
struct ch_sched_params info;
struct ch_sched_params tp;
- memset(&info, 0, sizeof(info));
- memset(&tp, 0, sizeof(tp));
-
memcpy(&tp, p, sizeof(tp));
/* Don't try to match class parameter */
tp.u.params.class = SCHED_CLS_NONE;
@@ -409,7 +406,6 @@ static struct sched_class *t4_sched_class_lookup(struct port_info *pi,
if (e->state == SCHED_STATE_UNUSED)
continue;
- memset(&info, 0, sizeof(info));
memcpy(&info, &e->info, sizeof(info));
/* Don't try to match class parameter */
info.u.params.class = SCHED_CLS_NONE;
@@ -458,7 +454,6 @@ static struct sched_class *t4_sched_class_alloc(struct port_info *pi,
if (!e)
goto out;
- memset(&np, 0, sizeof(np));
memcpy(&np, p, sizeof(np));
np.u.params.class = e->idx;
--
2.7.4
^ permalink raw reply related
* Re: [net PATCH v3 5/5] virtio_net: XDP support for adjust_head
From: John Fastabend @ 2017-01-14 0:45 UTC (permalink / raw)
To: Jason Wang, mst; +Cc: john.r.fastabend, netdev, alexei.starovoitov, daniel
In-Reply-To: <587933C2.8090003@gmail.com>
On 17-01-13 12:08 PM, John Fastabend wrote:
> On 17-01-12 11:41 PM, Jason Wang wrote:
>>
>>
>> On 2017年01月13日 10:52, John Fastabend wrote:
>>> Add support for XDP adjust head by allocating a 256B header region
>>> that XDP programs can grow into. This is only enabled when a XDP
>>> program is loaded.
>>>
>>> In order to ensure that we do not have to unwind queue headroom push
>>> queue setup below bpf_prog_add. It reads better to do a prog ref
>>> unwind vs another queue setup call.
>>>
>>> At the moment this code must do a full reset to ensure old buffers
>>> without headroom on program add or with headroom on program removal
>>> are not used incorrectly in the datapath. Ideally we would only
>>> have to disable/enable the RX queues being updated but there is no
>>> API to do this at the moment in virtio so use the big hammer. In
>>> practice it is likely not that big of a problem as this will only
>>> happen when XDP is enabled/disabled changing programs does not
>>> require the reset. There is some risk that the driver may either
>>> have an allocation failure or for some reason fail to correctly
>>> negotiate with the underlying backend in this case the driver will
>>> be left uninitialized. I have not seen this ever happen on my test
>>> systems and for what its worth this same failure case can occur
>>> from probe and other contexts in virtio framework.
>>>
>>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>>> ---
>>> drivers/net/virtio_net.c | 155 ++++++++++++++++++++++++++++++++++++++++------
>>> drivers/virtio/virtio.c | 9 ++-
>>> include/linux/virtio.h | 3 +
>>> 3 files changed, 144 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 6041828..8b897e7 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -28,6 +28,7 @@
>>> #include <linux/slab.h>
>>> #include <linux/cpu.h>
>>> #include <linux/average.h>
>>> +#include <linux/pci.h>
>>> #include <net/busy_poll.h>
>>> static int napi_weight = NAPI_POLL_WEIGHT;
>>> @@ -159,6 +160,9 @@ struct virtnet_info {
>>> /* Ethtool settings */
>>> u8 duplex;
>>> u32 speed;
>>> +
>>> + /* Headroom allocated in RX Queue */
>>> + unsigned int headroom;
>>
>> If this could not be changed in anyway, better use a macro instead of a filed
>> here. And there's even no need to add an extra parameter to
>> add_recvbuf_mergeable().
>
> OK originally I thought this might be dynamic but I agree no need
> for it here.
>
Well there is a bit of an order of operation issue that means we need at
least some bit here to tell us an enablement is pending.
The problem is when we do the reset we need to know that headroom for XDP
is needed. But we can't use the xdp_prog values because xdp_prog can not
be added on an device that is up without headroom otherwise the program
could fail. Plus reset via freeze/restore tears these structures down and
rebuilds them.
How about a boolean bit here instead of an unsigned int,
'bool xdp_headroom_needed'
seems better than an int.
Thanks,
John
^ permalink raw reply
* Re: [Patch net] atm: remove an unnecessary loop
From: Cong Wang @ 2017-01-14 0:41 UTC (permalink / raw)
To: Francois Romieu
Cc: Linux Kernel Network Developers, Michal Hocko, Chas Williams,
Andrey Konovalov
In-Reply-To: <20170114001519.GB6874@electric-eye.fr.zoreil.com>
On Fri, Jan 13, 2017 at 4:15 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> Cong Wang <xiyou.wangcong@gmail.com> :
>> On Fri, Jan 13, 2017 at 5:23 AM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> [...]
>> > alloc_skb() does not need to be in the "while" loop.
>>
>> This is exactly what I describe in my changelog, don't know
>> why you want to repeat it...
>
> Because it is still hidden in a while loop.
>
> You turned the alloc from a two level deep "while" loop to a one level
> one. I want it at zero level. alloc_skb(..., GFP_KERNEL) fails ?
> So let it be done (see patch in other message).
>
Why I didn't remove all the loops is already stated in the later patch,
you said you read it? I doubt. ;)
> [...]
>> Please don't expect me to fix many things in one patch, let's
>> fix each of them separately, agreed?
>
> I am not convinced that several patches are needed to get the whole
> picture right.
>
My guideline for stable fixes is one patch fixes one problem, maybe
not suitable to you I think. Let's agree to disagree. ;)
^ permalink raw reply
* Re: [Patch net] atm: remove an unnecessary loop
From: Cong Wang @ 2017-01-14 0:36 UTC (permalink / raw)
To: Francois Romieu
Cc: David Miller, Linux Kernel Network Developers, Michal Hocko,
Chas Williams, Andrey Konovalov
In-Reply-To: <20170114001422.GA6874@electric-eye.fr.zoreil.com>
On Fri, Jan 13, 2017 at 4:14 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> Cong Wang <xiyou.wangcong@gmail.com> :
> [...]
>> If you can justify API is not broken by doing that, I am more than happy
>> to do it, as I already stated in the latter patch:
>>
>> "Of course, the logic itself is suspicious, other sendmsg()
>> could handle skb allocation failure very well, not sure
>> why ATM has to wait for a successful one here. But probably
>> it is too late to change since the errno and behavior is
>> visible to user-space. So just leave the logic as it is."
>>
>> For some reason, no one reads that patch. :-/
>
> Believe it or not but I actually read it.
>
> It changes the logic : the original code would have been unable to
> escape the while loop on memory failure. Fine, I don't mind the change.
> Actually I believe that these two patches are too shy (and backport
> unefficient). Instead of trying to reformulate why, here's what I have
> in mind. Uncompiled, caveat emptor, etc.
I just don't want to break things, that is it. If you can convince me your
change will not break any user-space application, again I am more
than just happy about it. My ATM knowledge is close to zero. ;)
^ permalink raw reply
* Re: [Patch net] atm: remove an unnecessary loop
From: Cong Wang @ 2017-01-14 0:30 UTC (permalink / raw)
To: Chas Williams
Cc: David Miller, Francois Romieu, Linux Kernel Network Developers,
Michal Hocko, Andrey Konovalov
In-Reply-To: <1484351674.1643.16.camel@gmail.com>
On Fri, Jan 13, 2017 at 3:54 PM, Chas Williams <3chas3@gmail.com> wrote:
> On Fri, 2017-01-13 at 10:20 -0800, Cong Wang wrote:
>> On Fri, Jan 13, 2017 at 9:10 AM, David Miller <davem@davemloft.net> wrote:
>> > From: Francois Romieu <romieu@fr.zoreil.com>
>> > Date: Fri, 13 Jan 2017 01:07:00 +0100
>> >
>> >> Were alloc_skb moved one level up in the call stack, there would be
>> >> no need to use the new wait api in the subsequent page, thus easing
>> >> pre 3.19 longterm kernel maintenance (at least those on korg page).
>> >>
>> >> But it tastes a tad bit too masochistic.
>> >
>> > Lack of error handling of allocation failure is always a huge red
>> > flag. We even long ago tried to do something like this for TCP FIN
>> > handling.
>> >
>> > It's dumb, it doesn't work.
>> >
>> > Therefore I agree that the correct fix is to move the SKB allocation
>> > up one level to vcc_sendmsg() and make it handle errors properly.
>>
>> If you can justify API is not broken by doing that, I am more than happy
>> to do it, as I already stated in the latter patch:
>
> The man page for sendmsg() allows for ENOMEM. See below.
>
Errno is just one part, you miss the behavior behind the logic.
>>
>> "Of course, the logic itself is suspicious, other sendmsg()
>> could handle skb allocation failure very well, not sure
>> why ATM has to wait for a successful one here. But probably
>> it is too late to change since the errno and behavior is
>> visible to user-space. So just leave the logic as it is."
>>
>> For some reason, no one reads that patch. :-/
>
> I read it and I agree. I think it should be moved up/conflated with
> vcc_sendmsg(). vcc_sendmsg() can already return an errno for other
> conditions so if so has written something where they are explicitly
> not expecting a ENOMEM, we really can't help them.
Nope, the reason is never ENOMEM is expected or not. The current
_behavior_ behind this logic might be relied on by user-space.
The behavior here is, when allocation fails, kernel will retry under
certain circumstances, for example, if any fatal signal pending,
returns ERESTARTSYS, etc.. This is what I worry, not just ENOMEM
or not, which is too obvious.
Of course, I could be too conservative, I'd rather not to break things
for -stable at least.
Thanks.
^ permalink raw reply
* Re: [Patch net] atm: remove an unnecessary loop
From: Francois Romieu @ 2017-01-14 0:24 UTC (permalink / raw)
To: Cong Wang
Cc: David Miller, Linux Kernel Network Developers, Michal Hocko,
Chas Williams, Andrey Konovalov
In-Reply-To: <20170114001422.GA6874@electric-eye.fr.zoreil.com>
Francois Romieu <romieu@fr.zoreil.com> :
[...]
Now with a proper error code. Have a nice night.
diff --git a/net/atm/common.c b/net/atm/common.c
index a3ca922..e20d040 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -62,21 +62,16 @@ static void vcc_remove_socket(struct sock *sk)
write_unlock_irq(&vcc_sklist_lock);
}
-static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size)
+static bool vcc_tx_ready(struct atm_vcc *vcc, unsigned int size)
{
- struct sk_buff *skb;
struct sock *sk = sk_atm(vcc);
if (sk_wmem_alloc_get(sk) && !atm_may_send(vcc, size)) {
pr_debug("Sorry: wmem_alloc = %d, size = %d, sndbuf = %d\n",
sk_wmem_alloc_get(sk), size, sk->sk_sndbuf);
- return NULL;
+ return false;
}
- while (!(skb = alloc_skb(size, GFP_KERNEL)))
- schedule();
- pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
- atomic_add(skb->truesize, &sk->sk_wmem_alloc);
- return skb;
+ return true;
}
static void vcc_sock_destruct(struct sock *sk)
@@ -606,7 +601,7 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
eff = (size+3) & ~3; /* align to word boundary */
prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
error = 0;
- while (!(skb = alloc_tx(vcc, eff))) {
+ while (!vcc_tx_ready(vcc, eff)) {
if (m->msg_flags & MSG_DONTWAIT) {
error = -EAGAIN;
break;
@@ -628,6 +623,15 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
finish_wait(sk_sleep(sk), &wait);
if (error)
goto out;
+
+ skb = alloc_skb(eff, GFP_KERNEL);
+ if (!skb) {
+ error = -ENOMEM;
+ goto out;
+ }
+ pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
+ atomic_add(skb->truesize, &sk->sk_wmem_alloc);
+
skb->dev = NULL; /* for paths shared with net_device interfaces */
ATM_SKB(skb)->atm_options = vcc->atm_options;
if (!copy_from_iter_full(skb_put(skb, size), size, &m->msg_iter)) {
^ permalink raw reply related
* Re: [Patch net] atm: remove an unnecessary loop
From: Francois Romieu @ 2017-01-14 0:15 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, Michal Hocko, Chas Williams,
Andrey Konovalov
In-Reply-To: <CAM_iQpUCVh9y8uhZ8Hj0mcjgfMGUQDvaEvSu5Wur1oVn+EZNGQ@mail.gmail.com>
Cong Wang <xiyou.wangcong@gmail.com> :
> On Fri, Jan 13, 2017 at 5:23 AM, Francois Romieu <romieu@fr.zoreil.com> wrote:
[...]
> > alloc_skb() does not need to be in the "while" loop.
>
> This is exactly what I describe in my changelog, don't know
> why you want to repeat it...
Because it is still hidden in a while loop.
You turned the alloc from a two level deep "while" loop to a one level
one. I want it at zero level. alloc_skb(..., GFP_KERNEL) fails ?
So let it be done (see patch in other message).
[...]
> Please don't expect me to fix many things in one patch, let's
> fix each of them separately, agreed?
I am not convinced that several patches are needed to get the whole
picture right.
--
Ueimor
^ permalink raw reply
* Re: [Patch net] atm: remove an unnecessary loop
From: Francois Romieu @ 2017-01-14 0:14 UTC (permalink / raw)
To: Cong Wang
Cc: David Miller, Linux Kernel Network Developers, Michal Hocko,
Chas Williams, Andrey Konovalov
In-Reply-To: <CAM_iQpXd8qTTRT_+CkHnpMNC=CSS9vfB6zKtSjmLV_yya5BL8Q@mail.gmail.com>
Cong Wang <xiyou.wangcong@gmail.com> :
[...]
> If you can justify API is not broken by doing that, I am more than happy
> to do it, as I already stated in the latter patch:
>
> "Of course, the logic itself is suspicious, other sendmsg()
> could handle skb allocation failure very well, not sure
> why ATM has to wait for a successful one here. But probably
> it is too late to change since the errno and behavior is
> visible to user-space. So just leave the logic as it is."
>
> For some reason, no one reads that patch. :-/
Believe it or not but I actually read it.
It changes the logic : the original code would have been unable to
escape the while loop on memory failure. Fine, I don't mind the change.
Actually I believe that these two patches are too shy (and backport
unefficient). Instead of trying to reformulate why, here's what I have
in mind. Uncompiled, caveat emptor, etc.
I'll do a (slow) build and test on saturday's night with a pair of
iphase 5575.
diff --git a/net/atm/common.c b/net/atm/common.c
index a3ca922..67f76f3 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -62,21 +62,16 @@ static void vcc_remove_socket(struct sock *sk)
write_unlock_irq(&vcc_sklist_lock);
}
-static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size)
+static bool vcc_tx_ready(struct atm_vcc *vcc, unsigned int size)
{
- struct sk_buff *skb;
struct sock *sk = sk_atm(vcc);
if (sk_wmem_alloc_get(sk) && !atm_may_send(vcc, size)) {
pr_debug("Sorry: wmem_alloc = %d, size = %d, sndbuf = %d\n",
sk_wmem_alloc_get(sk), size, sk->sk_sndbuf);
- return NULL;
+ return false;
}
- while (!(skb = alloc_skb(size, GFP_KERNEL)))
- schedule();
- pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
- atomic_add(skb->truesize, &sk->sk_wmem_alloc);
- return skb;
+ return true;
}
static void vcc_sock_destruct(struct sock *sk)
@@ -606,7 +601,7 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
eff = (size+3) & ~3; /* align to word boundary */
prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
error = 0;
- while (!(skb = alloc_tx(vcc, eff))) {
+ while (!vcc_tx_ready(vcc, eff)) {
if (m->msg_flags & MSG_DONTWAIT) {
error = -EAGAIN;
break;
@@ -628,6 +623,13 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
finish_wait(sk_sleep(sk), &wait);
if (error)
goto out;
+
+ skb = alloc_skb(eff, GFP_KERNEL);
+ if (!skb)
+ goto out;
+ pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
+ atomic_add(skb->truesize, &sk->sk_wmem_alloc);
+
skb->dev = NULL; /* for paths shared with net_device interfaces */
ATM_SKB(skb)->atm_options = vcc->atm_options;
if (!copy_from_iter_full(skb_put(skb, size), size, &m->msg_iter)) {
--
Ueimor
^ permalink raw reply related
* Re: [PATCH v2 net-next] Introduce a sysctl that modifies the value of PROT_SOCK.
From: Krister Johansen @ 2017-01-14 0:13 UTC (permalink / raw)
To: David Miller; +Cc: kjlx, stephen, netdev
In-Reply-To: <20170112.092213.864894939381841760.davem@davemloft.net>
On Thu, Jan 12, 2017 at 09:22:13AM -0500, David Miller wrote:
> From: Krister Johansen <kjlx@templeofstupid.com>
> > The use case for this change is to allow containerized processes to bind
> > to priviliged ports, but prevent them from ever being allowed to modify
> > their container's network configuration. The latter is accomplished by
> > ensuring that the network namespace is not a child of the user
> > namespace. This modification was needed to allow the container manager
> > to disable a namespace's priviliged port restrictions without exposing
> > control of the network namespace to processes in the user namespace.
>
> This is what CAP_NET_BIND_SERVICE is for, and why it is a separate
> network privilege, please use it.
It sounds like I may have done an inadequate job of explaining why I
took this approach instead of going the CAP_NET_BIND_SERVICE route.
In this scenario, the network namespace is created and configured first.
Then the containerized processed get placed into a separate user
namespace. This is so that the processes in the container, even if they
somehow manage to obtain extra privilege in the userns, can never modify
the network namespace.
The check in ns_capable() is looking at the priviliges of the user
namespace that created the netns and its parents. Even if I were to
grant a process in the container CAP_NET_BIND_SERVICE, ns_capable()
wouldn't recognize that as being a valid privilige for the netns.
If I were to invert the order of operations and create the userns before
the netns, then the capability would be recognized. However, that also
allows any potential privilege escalation in the userns to bring with it
the potential that an attacker can modify the container's network
configuration.
I'd much rather run the containers without privs, and without the userns
having rights to the netns, to mitigate the risk of an attacker being
able to alter the container's networking configuration.
-K
^ permalink raw reply
* Re: [PATCH v2 net-next] Introduce a sysctl that modifies the value of PROT_SOCK.
From: Krister Johansen @ 2017-01-14 0:11 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Krister Johansen, Stephen Hemminger, David S. Miller, netdev
In-Reply-To: <1484231997.15816.36.camel@edumazet-glaptop3.roam.corp.google.com>
On Thu, Jan 12, 2017 at 06:39:57AM -0800, Eric Dumazet wrote:
> On Wed, 2017-01-11 at 22:52 -0800, Krister Johansen wrote:
> > Add net.ipv4.ip_unprotected_port_start, which is a per namespace sysctl
> > that denotes the first unprotected inet port in the namespace. To
> > disable all protected ports set this to zero. It also checks for
> > overlap with the local port range. The protected and local range may
> > not overlap.
> >
> > The use case for this change is to allow containerized processes to bind
> > to priviliged ports, but prevent them from ever being allowed to modify
> > their container's network configuration. The latter is accomplished by
> > ensuring that the network namespace is not a child of the user
> > namespace. This modification was needed to allow the container manager
> > to disable a namespace's priviliged port restrictions without exposing
> > control of the network namespace to processes in the user namespace.
> >
> > Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
> > ---
> > include/net/ip.h | 10 +++++++++
> > include/net/netns/ipv4.h | 1 +
> > net/ipv4/af_inet.c | 5 ++++-
> > net/ipv4/sysctl_net_ipv4.c | 50 +++++++++++++++++++++++++++++++++++++++++-
> > net/ipv6/af_inet6.c | 3 ++-
> > net/netfilter/ipvs/ip_vs_ctl.c | 7 +++---
> > net/sctp/socket.c | 10 +++++----
> > security/selinux/hooks.c | 3 ++-
>
> Adding a new sysctl without documentation is generally not accepted.
>
> Please take a look at Documentation/networking/ip-sysctl.txt
Thanks for catching this. I'll add an entry to the documentation.
> BTW, sticking to 'unprivileged' ports might be better than 'unprotected'
> which is vague.
I don't have a strong preference about the naming. I'd be happy to
change it to 'unprivileged' instead.
-K
^ permalink raw reply
* Re: [PATCH net] bpf: rework prog_digest into prog_tag
From: Daniel Borkmann @ 2017-01-13 23:59 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David S. Miller, Alexei Starovoitov, Andrew Lutomirski,
Network Development
In-Reply-To: <CALCETrWD7FuRLH0dC46bMf85mXosC3r54=TAXdar5EsZvbvyzg@mail.gmail.com>
On 01/14/2017 12:49 AM, Andy Lutomirski wrote:
> On Fri, Jan 13, 2017 at 3:41 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 01/14/2017 12:16 AM, Andy Lutomirski wrote:
>>> On Fri, Jan 13, 2017 at 2:38 PM, Daniel Borkmann <daniel@iogearbox.net>
>>> wrote:
>>>>
>>>> Commit 7bd509e311f4 ("bpf: add prog_digest and expose it via
>>>> fdinfo/netlink") was recently discussed, partially due to
>>>> admittedly suboptimal name of "prog_digest" in combination
>>>> with sha1 hash usage, thus inevitably and rightfully concerns
>>>> about its security in terms of collision resistance were
>>>> raised with regards to use-cases.
>>>
>>> Seems reasonable. My only question is whether you'd still want to
>>> switch to SHA-256 just from a code cleanliness perspective. With
>>> SHA-256 you can use the easy streaming API I wrote, but with SHA-1
>>> you're still stuck with the crappy API in lib/, and I'm not
>>> volunteering to fix up the SHA-1 API.
>>
>> We'd need to truncate that in kernel anyway to not get a too long
>> tag, so given that I'm actually fine with it as-is. I was planning
>> to submit the code for testing to bpf selftests for net-next once
>> it's merged back, too.
>
> Unless you want to kill off that vmalloc()+vfree() pair...
That is really just in slow-path, and should that become a bottleneck
compared to the rest of the verification steps or allocs we do there,
then we can always clean it up in net-next.
Thanks,
Daniel
^ permalink raw reply
* Re: [net PATCH v3 2/5] net: virtio: wrap rtnl_lock in test for calling with lock already held
From: John Fastabend @ 2017-01-13 23:56 UTC (permalink / raw)
To: Stephen Hemminger
Cc: jasowang, mst, john.r.fastabend, netdev, alexei.starovoitov,
daniel
In-Reply-To: <58790F02.8090409@gmail.com>
On 17-01-13 09:31 AM, John Fastabend wrote:
> On 17-01-13 08:34 AM, Stephen Hemminger wrote:
>> On Thu, 12 Jan 2017 18:51:00 -0800
>> John Fastabend <john.fastabend@gmail.com> wrote:
>>
>>>
>>> -static void free_receive_bufs(struct virtnet_info *vi)
>>> +static void free_receive_bufs(struct virtnet_info *vi, bool need_lock)
>>> {
>>> struct bpf_prog *old_prog;
>>> int i;
>>>
>>> - rtnl_lock();
>>> + if (need_lock)
>>> + rtnl_lock();
>>> for (i = 0; i < vi->max_queue_pairs; i++) {
>>> while (vi->rq[i].pages)
>>> __free_pages(get_a_page(&vi->rq[i], GFP_KERNEL), 0);
>>> @@ -1879,7 +1880,8 @@ static void free_receive_bufs(struct virtnet_info *vi)
>>> if (old_prog)
>>> bpf_prog_put(old_prog);
>>> }
>>> - rtnl_unlock();
>>> + if (need_lock)
>>> + rtnl_unlock();
>>> }
>>
>> Conditional locking is bad idea; sparse complains about it and is later source
>> of bugs. The more typical way of doing this in kernel is:
>
> OK I'll use the normal form.
>
>>
>> void _foo(some args)
>> {
>> ASSERT_RTNL();
>>
>> ...
>> }
>>
>> void foo(some args)
>> {
>> rtnl_lock();
>> _foo(some args)
>> rtnl_unlock();
>> }
>>
>>
>
Actually doing this without a rtnl_try_lock() is going to create two more
callbacks in virtio core just for virtio_net. All the other users do not
appear to have locking restrictions. How about the following it at least
helps in that there is no argument passing and if/else on the locks itself
but does use the if around rtnl_try_lock().
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1864,12 +1864,11 @@ static void virtnet_free_queues(struct virtnet_info *vi)
kfree(vi->sq);
}
-static void free_receive_bufs(struct virtnet_info *vi)
+static void _free_receive_bufs(struct virtnet_info *vi)
{
struct bpf_prog *old_prog;
int i;
- rtnl_lock();
for (i = 0; i < vi->max_queue_pairs; i++) {
while (vi->rq[i].pages)
__free_pages(get_a_page(&vi->rq[i], GFP_KERNEL), 0);
@@ -1879,6 +1878,12 @@ static void free_receive_bufs(struct virtnet_info *vi)
if (old_prog)
bpf_prog_put(old_prog);
}
+}
+
+static void free_receive_bufs(struct virtnet_info *vi)
+{
+ rtnl_lock();
+ _free_receive_bufs(vi);
rtnl_unlock();
}
@@ -2358,7 +2363,10 @@ static void remove_vq_common(struct virtnet_info *vi)
/* Free unused buffers in both send and recv, if any. */
free_unused_bufs(vi);
- free_receive_bufs(vi);
+ if (rtnl_is_locked());
+ _free_receive_bufs(vi);
+ else
+ free_receive_bufs(vi);
free_receive_page_frags(vi);
^ permalink raw reply
* Re: [Patch net] atm: remove an unnecessary loop
From: Chas Williams @ 2017-01-13 23:54 UTC (permalink / raw)
To: Cong Wang, David Miller
Cc: Francois Romieu, Linux Kernel Network Developers, Michal Hocko,
Andrey Konovalov
In-Reply-To: <CAM_iQpXd8qTTRT_+CkHnpMNC=CSS9vfB6zKtSjmLV_yya5BL8Q@mail.gmail.com>
On Fri, 2017-01-13 at 10:20 -0800, Cong Wang wrote:
> On Fri, Jan 13, 2017 at 9:10 AM, David Miller <davem@davemloft.net> wrote:
> > From: Francois Romieu <romieu@fr.zoreil.com>
> > Date: Fri, 13 Jan 2017 01:07:00 +0100
> >
> >> Were alloc_skb moved one level up in the call stack, there would be
> >> no need to use the new wait api in the subsequent page, thus easing
> >> pre 3.19 longterm kernel maintenance (at least those on korg page).
> >>
> >> But it tastes a tad bit too masochistic.
> >
> > Lack of error handling of allocation failure is always a huge red
> > flag. We even long ago tried to do something like this for TCP FIN
> > handling.
> >
> > It's dumb, it doesn't work.
> >
> > Therefore I agree that the correct fix is to move the SKB allocation
> > up one level to vcc_sendmsg() and make it handle errors properly.
>
> If you can justify API is not broken by doing that, I am more than happy
> to do it, as I already stated in the latter patch:
The man page for sendmsg() allows for ENOMEM. See below.
>
> "Of course, the logic itself is suspicious, other sendmsg()
> could handle skb allocation failure very well, not sure
> why ATM has to wait for a successful one here. But probably
> it is too late to change since the errno and behavior is
> visible to user-space. So just leave the logic as it is."
>
> For some reason, no one reads that patch. :-/
I read it and I agree. I think it should be moved up/conflated with
vcc_sendmsg(). vcc_sendmsg() can already return an errno for other
conditions so if so has written something where they are explicitly
not expecting a ENOMEM, we really can't help them.
I would certainly prefer to not have to resort to an atomic allocation.
That's just going to make matters worse as far as similarity to the
existing API.
So, as Francois has suggested, just wait for the atm socket to
drain, and then do the allocation after the wait is finished.
^ permalink raw reply
* [PATCH net-next] net: ipv6: Allow shorthand delete of all nexthops in multipath route
From: David Ahern @ 2017-01-13 23:54 UTC (permalink / raw)
To: netdev; +Cc: ddutt, David Ahern
IPv4 allows multipath routes to be deleted using just the prefix and
length. For example:
$ ip ro ls vrf red
unreachable default metric 8192
1.1.1.0/24
nexthop via 10.100.1.254 dev eth1 weight 1
nexthop via 10.11.200.2 dev eth11.200 weight 1
10.11.200.0/24 dev eth11.200 proto kernel scope link src 10.11.200.3
10.100.1.0/24 dev eth1 proto kernel scope link src 10.100.1.3
$ ip ro del 1.1.1.0/24 vrf red
$ ip ro ls vrf red
unreachable default metric 8192
10.11.200.0/24 dev eth11.200 proto kernel scope link src 10.11.200.3
10.100.1.0/24 dev eth1 proto kernel scope link src 10.100.1.3
The same notation does not work with IPv6 because of how multipath routes
are implemented for IPv6. For IPv6 only the first nexthop of a multipath
route is deleted if the request contains only a prefix and length. This
leads to unnecessary complexity in userspace dealing with IPv6 multipath
routes.
This patch allows all nexthops to be deleted without specifying each one
in the delete request by passing a new flag, RTM_F_ALL_NEXTHOPS, in
rtm_flags.
With this patch (and an updated iproute2 command):
$ ip -6 ro ls vrf red
1111::/120 via 2100:1::62 dev eth1 metric 1024 pref medium
1111::/120 via 2100:1::61 dev eth1 metric 1024 pref medium
1111::/120 via 2100:1::60 dev eth1 metric 1024 pref medium
2100:1::/120 dev eth1 proto kernel metric 256 pref medium
2100:1::/64 dev eth1 proto kernel metric 256 expires 86386sec pref medium
...
$ ip -6 ro del vrf red 1111::1/120
$ ip -6 ro ls vrf red
2100:1::/120 dev eth1 proto kernel metric 256 pref medium
2100:1::/64 dev eth1 proto kernel metric 256 expires 86382sec pref medium
...
The flag is added to fib6_config by converting fc_type to a u16 (as
noted fc_type only uses 8 bits). The new u16 hole is a bitmap with
fc_delete_all_nexthop as the first bit.
Suggested-by: Dinesh Dutt <ddutt@cumulusnetworks.com>
Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
include/net/ip6_fib.h | 4 +++-
include/uapi/linux/rtnetlink.h | 1 +
net/ipv6/route.c | 10 +++++++++-
3 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index a74e2aa40ef4..11ab99e87c5f 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -37,7 +37,9 @@ struct fib6_config {
int fc_ifindex;
u32 fc_flags;
u32 fc_protocol;
- u32 fc_type; /* only 8 bits are used */
+ u16 fc_type; /* only 8 bits are used */
+ u16 fc_delete_all_nexthop : 1,
+ __unused : 15;
struct in6_addr fc_dst;
struct in6_addr fc_src;
diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 8c93ad1ef9ab..7fb206bc42f9 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -276,6 +276,7 @@ enum rt_scope_t {
#define RTM_F_EQUALIZE 0x400 /* Multipath equalizer: NI */
#define RTM_F_PREFIX 0x800 /* Prefix addresses */
#define RTM_F_LOOKUP_TABLE 0x1000 /* set rtm_table to FIB lookup result */
+#define RTM_F_ALL_NEXTHOPS 0x2000 /* delete all nexthops (IPv6) */
/* Reserved table identifiers */
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ce5aaf448c54..8bb5f6a35ba8 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2154,6 +2154,7 @@ static int ip6_route_del(struct fib6_config *cfg)
if (!table)
return err;
+again:
read_lock_bh(&table->tb6_lock);
fn = fib6_locate(&table->tb6_root,
@@ -2179,7 +2180,11 @@ static int ip6_route_del(struct fib6_config *cfg)
dst_hold(&rt->dst);
read_unlock_bh(&table->tb6_lock);
- return __ip6_del_rt(rt, &cfg->fc_nlinfo);
+ err = __ip6_del_rt(rt, &cfg->fc_nlinfo);
+ if (err || !cfg->fc_delete_all_nexthop)
+ return err;
+
+ goto again;
}
}
read_unlock_bh(&table->tb6_lock);
@@ -2849,6 +2854,9 @@ static int rtm_to_fib6_config(struct sk_buff *skb, struct nlmsghdr *nlh,
if (rtm->rtm_flags & RTM_F_CLONED)
cfg->fc_flags |= RTF_CACHE;
+ if (rtm->rtm_flags & RTM_F_ALL_NEXTHOPS)
+ cfg->fc_delete_all_nexthop = 1;
+
cfg->fc_nlinfo.portid = NETLINK_CB(skb).portid;
cfg->fc_nlinfo.nlh = nlh;
cfg->fc_nlinfo.nl_net = sock_net(skb->sk);
--
2.1.4
^ permalink raw reply related
* Re: [PATCH net] bpf: rework prog_digest into prog_tag
From: Andy Lutomirski @ 2017-01-13 23:49 UTC (permalink / raw)
To: Daniel Borkmann
Cc: David S. Miller, Alexei Starovoitov, Andrew Lutomirski,
Network Development
In-Reply-To: <58796595.3030904@iogearbox.net>
On Fri, Jan 13, 2017 at 3:41 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 01/14/2017 12:16 AM, Andy Lutomirski wrote:
>>
>> On Fri, Jan 13, 2017 at 2:38 PM, Daniel Borkmann <daniel@iogearbox.net>
>> wrote:
>>>
>>> Commit 7bd509e311f4 ("bpf: add prog_digest and expose it via
>>> fdinfo/netlink") was recently discussed, partially due to
>>> admittedly suboptimal name of "prog_digest" in combination
>>> with sha1 hash usage, thus inevitably and rightfully concerns
>>> about its security in terms of collision resistance were
>>> raised with regards to use-cases.
>>
>>
>> Seems reasonable. My only question is whether you'd still want to
>> switch to SHA-256 just from a code cleanliness perspective. With
>> SHA-256 you can use the easy streaming API I wrote, but with SHA-1
>> you're still stuck with the crappy API in lib/, and I'm not
>> volunteering to fix up the SHA-1 API.
>
>
> We'd need to truncate that in kernel anyway to not get a too long
> tag, so given that I'm actually fine with it as-is. I was planning
> to submit the code for testing to bpf selftests for net-next once
> it's merged back, too.
Unless you want to kill off that vmalloc()+vfree() pair...
--Andy
^ permalink raw reply
* [PATCH next] ipvlan: fix dev_id creation corner case.
From: Mahesh Bandewar @ 2017-01-13 23:48 UTC (permalink / raw)
To: David Miller
Cc: netdev, Mahesh Bandewar, Mahesh Bandewar, David Miller,
Eric Dumazet
From: Mahesh Bandewar <maheshb@google.com>
In the last patch da36e13cf65 ("ipvlan: improvise dev_id generation
logic in IPvlan") I missed some part of Dave's suggestion and because
of that the dev_id creation could fail in a corner case scenario. This
would happen when more or less 64k devices have been already created and
several have been deleted. If the devices that are still sticking around
are the last n bits from the bitmap. So in this scenario even if lower
bits are available, the dev_id search is so narrow that it always fails.
Fixes: da36e13cf65 ("ipvlan: improvise dev_id generation logic in IPvlan")
CC: David Miller <davem@davemloft.org>
CC: Eric Dumazet <edumazet@google.com>
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
drivers/net/ipvlan/ipvlan_main.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index 92b221a03350..b5c390f0f2b3 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -550,6 +550,9 @@ static int ipvlan_link_new(struct net *src_net, struct net_device *dev,
err = ida_simple_get(&port->ida, port->dev_id_start, 0xFFFE,
GFP_KERNEL);
if (err < 0)
+ err = ida_simple_get(&port->ida, 0x1, port->dev_id_start,
+ GFP_KERNEL);
+ if (err < 0)
goto destroy_ipvlan_port;
dev->dev_id = err;
/* Increment id-base to the next slot for the future assignment */
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox