* Re: [PATCH net 1/2] i40e: Fix comment about locking for __i40e_read_nvm_word()
From: Jeff Kirsher @ 2017-09-07 1:16 UTC (permalink / raw)
To: Stefano Brivio, netdev, intel-wired-lan
Cc: David S . Miller, Anjali Singhai Jain
In-Reply-To: <4d27bbb33031ac1489fe6d969edd74d44155136c.1504684488.git.sbrivio@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 663 bytes --]
On Wed, 2017-09-06 at 10:11 +0200, Stefano Brivio wrote:
> Caller needs to acquire the lock. Called functions will not.
>
> Fixes: 09f79fd49d94 ("i40e: avoid NVM acquire deadlock during NVM
> update")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> drivers/net/ethernet/intel/i40e/i40e_nvm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Yes, this fixes the function header comment, not sure if it requires
the "Fixes:" tag. If that were the case, wonder why all the other code
comment changes do not have this. :-) I do agree it reads better with
this change, so I do not have an issue queue this up for Dave's net
tree.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH net 2/2] i40e: Avoid some useless variables and initializers in nvm functions
From: Jeff Kirsher @ 2017-09-07 1:13 UTC (permalink / raw)
To: Stefano Brivio, netdev, intel-wired-lan
Cc: David S . Miller, Anjali Singhai Jain
In-Reply-To: <d0881adf53a3f0a546b3e7940b92b4ce938e7e50.1504684488.git.sbrivio@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 571 bytes --]
On Wed, 2017-09-06 at 10:11 +0200, Stefano Brivio wrote:
> Fixes: 09f79fd49d94 ("i40e: avoid NVM acquire deadlock during NVM
> update")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> drivers/net/ethernet/intel/i40e/i40e_nvm.c | 20 +++++++-------------
> 1 file changed, 7 insertions(+), 13 deletions(-)
This is NOT a fix, it is a coding style preference whether or not you
like multiple returns or a single return in functions. I do not mind
picking this up for net-next (4.15 kernel), but this does not qualify
for a fix for Dave's net tree.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Andrew Lunn @ 2017-09-07 0:47 UTC (permalink / raw)
To: Florian Fainelli
Cc: Roopa Prabhu, netdev, Vivien Didelot, Woojung.Huh, jbe, sean.wang,
john
In-Reply-To: <44233399-2cb9-35af-2b93-b0024aac8dc4@gmail.com>
> At the very least we should probably move the skb->offload_fwd_mark
> setting down into the individual taggers since they should be in a
> better position to set it or not based on the switch device they are
> driving, this should address, on a per-switch basis whether 2) or 3)
> applies to a given switch.
Yes, that could be done.
> That being said, I have a feeling that the Marvell switches behave a
> tiny bit differently than others in that they do not flood broadcast by
> default in a given L2 domain.
It seems like the switch can be configured to flood broadcast. Just at
the moment, it is not.
> On b53/bcm_sf2 there is the ability to disable the reception of
> broadcast frames on the management/CPU port, and while there is the
> ability to configure which ports should be flooded in case of
> unicast/multicast lookup failures, I don't see anything for Broadcast,
> so I am assuming this will get forwarded by default. Will test with your
> patch set later on time permitting.
Please let me know the results of the tests.
If Marvell is being different, it also means that all other switches
today are duplicating broadcasts. We should fix that. In fact, i think
we need to fix this first, before these patches for IGMP snooping on
br0 goes in.
Andrew
^ permalink raw reply
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Andrew Lunn @ 2017-09-07 0:43 UTC (permalink / raw)
To: Woojung.Huh
Cc: f.fainelli, roopa, netdev, vivien.didelot, jbe, sean.wang, john
In-Reply-To: <9235D6609DB808459E95D78E17F2E43D40B19584@CHN-SV-EXMX02.mchp-main.com>
On Wed, Sep 06, 2017 at 11:44:47PM +0000, Woojung.Huh@microchip.com wrote:
> > That being said, I have a feeling that the Marvell switches behave a
> > tiny bit differently than others in that they do not flood broadcast by
> > default in a given L2 domain.
> Florian,
>
> Because some DSA switches from Marvell & Microchip can do IGMP snooping,
> can we propose switch layer another flag what to do when HW support it?
Hi Woojung
I expect all the current DSA devices should be able to do IGMP
snooping, with some modifications.
Two things are required:
1) The .port_mdb_prepare, .port_mdb_add and .port_mdb_del ops, so that
mdb entries can be added. As you said, only Marvell and Microchip
support these, but i expect the other switch can do this, it just
needs implementing.
2) The switch needs to identify and forward IGMP packets to the host,
even when they would normally be blocked.
And for the implementation, i don't think it actually matters. For
switches which don't implement the port_mdb operations, IGMP packets
will get forwarded to the software bridge. It will attempt to put in
an mdb, but the request will come back with EOPNOTSUPP. The switch
should continue to flood multicast out all ports. No harm done.
Andrew
^ permalink raw reply
* [PATCH RFC v1 3/3] vethtap: veth based tap driver
From: sainath.grandhi @ 2017-09-07 0:34 UTC (permalink / raw)
To: netdev; +Cc: davem, Sainath Grandhi
In-Reply-To: <1504744467-79590-1-git-send-email-sainath.grandhi@intel.com>
From: Sainath Grandhi <sainath.grandhi@intel.com>
This patch adds a tap character device driver that is based on the
veth network interface, called vethtap. This patchset allows vethtap device
to be created ONLY as a peer interface to a veth network interface. It can
be created in the following way,
ip link add veth1 type veth peer name veth2 type vethtap
With this packets on veth2 can be accessed using tap user space interface.
Signed-off-by: Sainath Grandhi <sainath.grandhi@intel.com>
---
drivers/net/Kconfig | 1 +
drivers/net/Makefile | 2 +
drivers/net/{veth.c => veth_main.c} | 33 +++++-
drivers/net/vethtap.c | 216 ++++++++++++++++++++++++++++++++++++
include/linux/if_veth.h | 4 +
5 files changed, 255 insertions(+), 1 deletion(-)
rename drivers/net/{veth.c => veth_main.c} (94%)
create mode 100644 drivers/net/vethtap.c
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index aba0d65..265853e 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -323,6 +323,7 @@ config TUN_VNET_CROSS_LE
config VETH
tristate "Virtual ethernet pair device"
+ select TAP
---help---
This device is a local ethernet tunnel. Devices are created in pairs.
When one end receives the packet it appears on its pair and vice
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index 8dff900..7c63e69 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -32,6 +32,8 @@ obj-$(CONFIG_NLMON) += nlmon.o
obj-$(CONFIG_NET_VRF) += vrf.o
obj-$(CONFIG_VSOCKMON) += vsockmon.o
+veth-objs := veth_main.o vethtap.o
+
#
# Networking Drivers
#
diff --git a/drivers/net/veth.c b/drivers/net/veth_main.c
similarity index 94%
rename from drivers/net/veth.c
rename to drivers/net/veth_main.c
index a1b370d..fc91dd7 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth_main.c
@@ -359,6 +359,9 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
unsigned char name_assign_type;
struct ifinfomsg *ifmp;
struct net *net;
+ struct nlattr *linkinfo[IFLA_INFO_MAX + 1];
+ char peer_type[8];
+ struct rtnl_link_ops *link_ops;
/*
* create and register peer first
@@ -393,17 +396,38 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
name_assign_type = NET_NAME_ENUM;
}
+ link_ops = &veth_link_ops;
+ if (tbp[IFLA_LINKINFO]) {
+ err = rtnl_nla_parse_ifla_info(linkinfo,
+ nla_data(tbp[IFLA_LINKINFO]),
+ nla_len(tbp[IFLA_LINKINFO]),
+ NULL);
+
+ if (err < 0)
+ return err;
+
+ if (linkinfo[IFLA_INFO_KIND]) {
+ nla_strlcpy(peer_type, linkinfo[IFLA_INFO_KIND],
+ sizeof(peer_type));
+ if (!strncmp(peer_type, "vethtap", sizeof(peer_type)))
+ link_ops = &vethtap_link_ops;
+ }
+ }
+
net = rtnl_link_get_net(src_net, tbp);
if (IS_ERR(net))
return PTR_ERR(net);
peer = rtnl_create_link(net, ifname, name_assign_type,
- &veth_link_ops, tbp);
+ link_ops, tbp);
if (IS_ERR(peer)) {
put_net(net);
return PTR_ERR(peer);
}
+ if (!strncmp(peer_type, "vethtap", sizeof(peer_type)))
+ link_ops->newlink(net, peer, tbp, NULL, NULL);
+
if (!ifmp || !tbp[IFLA_ADDRESS])
eth_hw_addr_random(peer);
@@ -536,12 +560,19 @@ static __init int veth_init(void)
err = veth_link_register(&veth_link_ops);
+ if (err)
+ goto out1;
+
+ err = vethtap_init();
+
+out1:
return err;
}
static __exit void veth_exit(void)
{
rtnl_link_unregister(&veth_link_ops);
+ vethtap_exit();
}
module_init(veth_init);
diff --git a/drivers/net/vethtap.c b/drivers/net/vethtap.c
new file mode 100644
index 0000000..922b3ea
--- /dev/null
+++ b/drivers/net/vethtap.c
@@ -0,0 +1,216 @@
+#include <linux/etherdevice.h>
+#include <linux/if_tap.h>
+#include <linux/if_vlan.h>
+#include <linux/if_veth.h>
+#include <linux/interrupt.h>
+#include <linux/nsproxy.h>
+#include <linux/compat.h>
+#include <linux/if_tun.h>
+#include <linux/module.h>
+#include <linux/skbuff.h>
+#include <linux/cache.h>
+#include <linux/sched/signal.h>
+#include <linux/types.h>
+#include <linux/slab.h>
+#include <linux/wait.h>
+#include <linux/cdev.h>
+#include <linux/idr.h>
+#include <linux/fs.h>
+#include <linux/uio.h>
+
+#include <net/net_namespace.h>
+#include <net/rtnetlink.h>
+#include <net/sock.h>
+#include <linux/virtio_net.h>
+#include <linux/skb_array.h>
+
+struct vethtap_dev {
+ struct veth_priv veth;
+ struct tap_dev tap;
+};
+
+/* Variables for dealing with vethtaps device numbers.
+ */
+static dev_t vethtap_major;
+
+static const void *vethtap_net_namespace(struct device *d)
+{
+ struct net_device *dev = to_net_dev(d->parent);
+
+ return dev_net(dev);
+}
+
+static struct class vethtap_class = {
+ .name = "vethtap",
+ .owner = THIS_MODULE,
+ .ns_type = &net_ns_type_operations,
+ .namespace = vethtap_net_namespace,
+};
+
+static struct cdev vethtap_cdev;
+
+#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
+ NETIF_F_TSO6)
+
+static void vethtap_count_tx_dropped(struct tap_dev *tap)
+{
+ struct vethtap_dev *vethtap = container_of(tap, struct vethtap_dev,
+ tap);
+ struct veth_priv *veth = &vethtap->veth;
+
+ atomic64_inc(&veth->dropped);
+}
+
+static int vethtap_newlink(struct net *src_net, struct net_device *dev,
+ struct nlattr *tb[], struct nlattr *data[],
+ struct netlink_ext_ack *extack)
+{
+ struct vethtap_dev *vethtap = netdev_priv(dev);
+ int err;
+
+ INIT_LIST_HEAD(&vethtap->tap.queue_list);
+
+ /* Since macvlan supports all offloads by default, make
+ * tap support all offloads also.
+ */
+ vethtap->tap.tap_features = TUN_OFFLOADS;
+
+ /* Register callbacks for rx/tx drops accounting and updating
+ * net_device features
+ */
+ vethtap->tap.count_tx_dropped = vethtap_count_tx_dropped;
+ vethtap->tap.count_rx_dropped = NULL;
+ vethtap->tap.update_features = NULL;
+
+ err = netdev_rx_handler_register(dev, tap_handle_frame, &vethtap->tap);
+ if (err)
+ return err;
+
+ vethtap->tap.dev = dev;
+
+ return 0;
+}
+
+static void vethtap_dellink(struct net_device *dev,
+ struct list_head *head)
+{
+ struct vethtap_dev *vethtap = netdev_priv(dev);
+
+ netdev_rx_handler_unregister(dev);
+ tap_del_queues(&vethtap->tap);
+ veth_dellink(dev, head);
+}
+
+static void vethtap_setup(struct net_device *dev)
+{
+ veth_common_setup(dev);
+ dev->tx_queue_len = TUN_READQ_SIZE;
+}
+
+struct rtnl_link_ops vethtap_link_ops __read_mostly = {
+ .kind = "vethtap",
+ .setup = vethtap_setup,
+ .newlink = vethtap_newlink,
+ .dellink = vethtap_dellink,
+ .priv_size = sizeof(struct vethtap_dev),
+};
+
+static int vethtap_device_event(struct notifier_block *unused,
+ unsigned long event, void *ptr)
+{
+ struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+ struct vethtap_dev *vethtap;
+ struct device *classdev;
+ dev_t devt;
+ int err;
+ char tap_name[IFNAMSIZ];
+
+ if (dev->rtnl_link_ops != &vethtap_link_ops)
+ return NOTIFY_DONE;
+
+ snprintf(tap_name, IFNAMSIZ, "tap%d", dev->ifindex);
+ vethtap = netdev_priv(dev);
+
+ switch (event) {
+ case NETDEV_REGISTER:
+ /* Create the device node here after the network device has
+ * been registered but before register_netdevice has
+ * finished running.
+ */
+ err = tap_get_minor(vethtap_major, &vethtap->tap);
+ if (err)
+ return notifier_from_errno(err);
+
+ devt = MKDEV(MAJOR(vethtap_major), vethtap->tap.minor);
+ classdev = device_create(&vethtap_class, &dev->dev, devt,
+ dev, tap_name);
+ if (IS_ERR(classdev)) {
+ tap_free_minor(vethtap_major, &vethtap->tap);
+ return notifier_from_errno(PTR_ERR(classdev));
+ }
+ err = sysfs_create_link(&dev->dev.kobj, &classdev->kobj,
+ tap_name);
+ if (err)
+ return notifier_from_errno(err);
+ break;
+ case NETDEV_UNREGISTER:
+ /* vlan->minor == 0 if NETDEV_REGISTER above failed */
+ if (vethtap->tap.minor == 0)
+ break;
+ sysfs_remove_link(&dev->dev.kobj, tap_name);
+ devt = MKDEV(MAJOR(vethtap_major), vethtap->tap.minor);
+ device_destroy(&vethtap_class, devt);
+ tap_free_minor(vethtap_major, &vethtap->tap);
+ break;
+ case NETDEV_CHANGE_TX_QUEUE_LEN:
+ if (tap_queue_resize(&vethtap->tap))
+ return NOTIFY_BAD;
+ break;
+ }
+
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block vethtap_notifier_block __read_mostly = {
+ .notifier_call = vethtap_device_event,
+};
+
+int vethtap_init(void)
+{
+ int err;
+
+ err = tap_create_cdev(&vethtap_cdev, &vethtap_major, "vethtap");
+
+ if (err)
+ goto out1;
+
+ err = class_register(&vethtap_class);
+ if (err)
+ goto out2;
+
+ err = register_netdevice_notifier(&vethtap_notifier_block);
+ if (err)
+ goto out3;
+
+ veth_link_ops_init(&vethtap_link_ops);
+ if (err)
+ goto out4;
+
+ return 0;
+
+out4:
+ unregister_netdevice_notifier(&vethtap_notifier_block);
+out3:
+ class_unregister(&vethtap_class);
+out2:
+ tap_destroy_cdev(vethtap_major, &vethtap_cdev);
+out1:
+ return err;
+}
+
+void vethtap_exit(void)
+{
+ unregister_netdevice_notifier(&vethtap_notifier_block);
+ class_unregister(&vethtap_class);
+ tap_destroy_cdev(vethtap_major, &vethtap_cdev);
+}
diff --git a/include/linux/if_veth.h b/include/linux/if_veth.h
index b007891..dbc06c4 100644
--- a/include/linux/if_veth.h
+++ b/include/linux/if_veth.h
@@ -4,6 +4,10 @@ struct veth_priv {
unsigned int requested_headroom;
};
+extern struct rtnl_link_ops vethtap_link_ops;
+
void veth_common_setup(struct net_device *dev);
void veth_dellink(struct net_device *dev, struct list_head *head);
void veth_link_ops_init(struct rtnl_link_ops *ops);
+int vethtap_init(void);
+void vethtap_exit(void);
--
2.7.4
^ permalink raw reply related
* [PATCH RFC v1 2/3] net: Abstracting out common routines from veth for use by vethtap
From: sainath.grandhi @ 2017-09-07 0:34 UTC (permalink / raw)
To: netdev; +Cc: davem, Sainath Grandhi
In-Reply-To: <1504744467-79590-1-git-send-email-sainath.grandhi@intel.com>
From: Sainath Grandhi <sainath.grandhi@intel.com>
Abstracting out common routines for link operations in veth implementation
for use by vethtap interfaces
Signed-off-by: Sainath Grandhi <sainath.grandhi@intel.com>
---
drivers/net/veth.c | 47 +++++++++++++++++++++++++++++++++--------------
include/linux/if_veth.h | 9 +++++++++
2 files changed, 42 insertions(+), 14 deletions(-)
create mode 100644 include/linux/if_veth.h
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index f5438d0..a1b370d 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -18,6 +18,7 @@
#include <net/dst.h>
#include <net/xfrm.h>
#include <linux/veth.h>
+#include <linux/if_veth.h>
#include <linux/module.h>
#define DRV_NAME "veth"
@@ -29,12 +30,6 @@ struct pcpu_vstats {
struct u64_stats_sync syncp;
};
-struct veth_priv {
- struct net_device __rcu *peer;
- atomic64_t dropped;
- unsigned requested_headroom;
-};
-
/*
* ethtool interface
*/
@@ -298,13 +293,12 @@ static const struct net_device_ops veth_netdev_ops = {
NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX | \
NETIF_F_HW_VLAN_STAG_TX | NETIF_F_HW_VLAN_STAG_RX )
-static void veth_setup(struct net_device *dev)
+void veth_common_setup(struct net_device *dev)
{
ether_setup(dev);
dev->priv_flags &= ~IFF_TX_SKB_SHARING;
dev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
- dev->priv_flags |= IFF_NO_QUEUE;
dev->priv_flags |= IFF_PHONY_HEADROOM;
dev->netdev_ops = &veth_netdev_ops;
@@ -325,6 +319,12 @@ static void veth_setup(struct net_device *dev)
dev->mpls_features = NETIF_F_HW_CSUM | NETIF_F_GSO_SOFTWARE;
}
+static void veth_setup(struct net_device *dev)
+{
+ veth_common_setup(dev);
+ dev->priv_flags |= IFF_NO_QUEUE;
+}
+
/*
* netlink interface
*/
@@ -465,7 +465,7 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
return err;
}
-static void veth_dellink(struct net_device *dev, struct list_head *head)
+void veth_dellink(struct net_device *dev, struct list_head *head)
{
struct veth_priv *priv;
struct net_device *peer;
@@ -503,21 +503,40 @@ static struct rtnl_link_ops veth_link_ops = {
.kind = DRV_NAME,
.priv_size = sizeof(struct veth_priv),
.setup = veth_setup,
- .validate = veth_validate,
.newlink = veth_newlink,
.dellink = veth_dellink,
- .policy = veth_policy,
- .maxtype = VETH_INFO_MAX,
- .get_link_net = veth_get_link_net,
};
+int veth_link_register(struct rtnl_link_ops *ops)
+{
+ /* common fields */
+ ops->validate = veth_validate;
+ ops->policy = veth_policy;
+ ops->maxtype = VETH_INFO_MAX;
+ ops->get_link_net = veth_get_link_net;
+
+ return rtnl_link_register(ops);
+}
+
+void veth_link_ops_init(struct rtnl_link_ops *ops)
+{
+ /*common fields*/
+ ops->validate = veth_validate;
+ ops->policy = veth_policy;
+ ops->maxtype = VETH_INFO_MAX;
+ ops->get_link_net = veth_get_link_net;
+}
/*
* init/fini
*/
static __init int veth_init(void)
{
- return rtnl_link_register(&veth_link_ops);
+ int err;
+
+ err = veth_link_register(&veth_link_ops);
+
+ return err;
}
static __exit void veth_exit(void)
diff --git a/include/linux/if_veth.h b/include/linux/if_veth.h
new file mode 100644
index 0000000..b007891
--- /dev/null
+++ b/include/linux/if_veth.h
@@ -0,0 +1,9 @@
+struct veth_priv {
+ struct net_device __rcu *peer;
+ atomic64_t dropped;
+ unsigned int requested_headroom;
+};
+
+void veth_common_setup(struct net_device *dev);
+void veth_dellink(struct net_device *dev, struct list_head *head);
+void veth_link_ops_init(struct rtnl_link_ops *ops);
--
2.7.4
^ permalink raw reply related
* [PATCH RFC v1 1/3] net: Adding API to parse IFLA_LINKINFO attribute
From: sainath.grandhi @ 2017-09-07 0:34 UTC (permalink / raw)
To: netdev; +Cc: davem, Sainath Grandhi
In-Reply-To: <1504744467-79590-1-git-send-email-sainath.grandhi@intel.com>
From: Sainath Grandhi <sainath.grandhi@intel.com>
Adding rtnl_nla_parse_ifla_info as an exported symbol in rtnetlink.c helps
other modules to parse IFLA_LINKINFO attribute
Signed-off-by: Sainath Grandhi <sainath.grandhi@intel.com>
---
include/net/rtnetlink.h | 3 +++
net/core/rtnetlink.c | 8 ++++++++
2 files changed, 11 insertions(+)
diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index 21837ca..cb15ddb 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -170,6 +170,9 @@ int rtnl_configure_link(struct net_device *dev, const struct ifinfomsg *ifm);
int rtnl_nla_parse_ifla(struct nlattr **tb, const struct nlattr *head, int len,
struct netlink_ext_ack *exterr);
+int rtnl_nla_parse_ifla_info(struct nlattr **tb, const struct nlattr *head,
+ int len, struct netlink_ext_ack *exterr);
+
#define MODULE_ALIAS_RTNL_LINK(kind) MODULE_ALIAS("rtnl-link-" kind)
#endif
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a78fd61..0784b7d 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1688,6 +1688,14 @@ int rtnl_nla_parse_ifla(struct nlattr **tb, const struct nlattr *head, int len,
}
EXPORT_SYMBOL(rtnl_nla_parse_ifla);
+int rtnl_nla_parse_ifla_info(struct nlattr **tb, const struct nlattr *head,
+ int len, struct netlink_ext_ack *exterr)
+{
+ return nla_parse(tb, IFLA_INFO_MAX, head, len, ifla_info_policy,
+ exterr);
+}
+EXPORT_SYMBOL(rtnl_nla_parse_ifla_info);
+
struct net *rtnl_link_get_net(struct net *src_net, struct nlattr *tb[])
{
struct net *net;
--
2.7.4
^ permalink raw reply related
* [PATCH RFC v1 0/3] Support for tap user-space access with veth interfaces
From: sainath.grandhi @ 2017-09-07 0:34 UTC (permalink / raw)
To: netdev; +Cc: davem, Sainath Grandhi
From: Sainath Grandhi <sainath.grandhi@intel.com>
This patchset adds a tap device driver for veth virtual network interface.
With this implementation, tap character interface can be added only to the
peer veth interface. Adding tap interface to veth is for usecases that forwards
packets between host and VMs. This eliminates the need for an additional
software bridge. This can be extended to create both the peer interfaces as
tap interfaces. These patches are a step in that direction.
Sainath Grandhi (3):
net: Adding API to parse IFLA_LINKINFO attribute
net: Abstracting out common routines from veth for use by vethtap
vethtap: veth based tap driver
drivers/net/Kconfig | 1 +
drivers/net/Makefile | 2 +
drivers/net/{veth.c => veth_main.c} | 80 ++++++++++---
drivers/net/vethtap.c | 216 ++++++++++++++++++++++++++++++++++++
include/linux/if_veth.h | 13 +++
include/net/rtnetlink.h | 3 +
net/core/rtnetlink.c | 8 ++
7 files changed, 308 insertions(+), 15 deletions(-)
rename drivers/net/{veth.c => veth_main.c} (89%)
create mode 100644 drivers/net/vethtap.c
create mode 100644 include/linux/if_veth.h
--
2.7.4
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: David Daney @ 2017-09-07 0:10 UTC (permalink / raw)
To: Florian Fainelli, David Daney, Mason
Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
Andrew Lunn, Mans Rullgard
In-Reply-To: <a4b70bf9-ec48-314d-b63d-e44f7cbb4bab@gmail.com>
On 09/06/2017 04:14 PM, Florian Fainelli wrote:
> On 09/06/2017 03:51 PM, David Daney wrote:
[...]
>>
>> Consider instead the case of a Marvell phy with no interrupts connected
>> on a v4.9.43 kernel, single CPU:
>>
>>
>> 0) | phy_disconnect() {
>> 0) | phy_stop_machine() {
>> 0) | cancel_delayed_work_sync() {
>> 0) + 23.986 us | } /* cancel_delayed_work_sync */
>> 0) | phy_state_machine() {
>> 0) | phy_start_aneg_priv() {
>
> Thanks for providing the trace, I think I have an idea of what's going
> on, see below.
>
>> 0) | marvell_config_aneg() {
>> 0) ! 240.538 us | } /* marvell_config_aneg */
>> 0) ! 244.971 us | } /* phy_start_aneg_priv */
>> 0) | queue_delayed_work_on() {
>> 0) + 18.016 us | } /* queue_delayed_work_on */
>> 0) ! 268.184 us | } /* phy_state_machine */
>> 0) ! 297.394 us | } /* phy_stop_machine */
>> 0) | phy_detach() {
>> 0) | phy_suspend() {
>> 0) | phy_ethtool_get_wol() {
>> 0) 0.677 us | } /* phy_ethtool_get_wol */
>> 0) | genphy_suspend() {
>> 0) + 71.250 us | } /* genphy_suspend */
>> 0) + 74.197 us | } /* phy_suspend */
>> 0) + 80.302 us | } /* phy_detach */
>> 0) ! 380.072 us | } /* phy_disconnect */
>> .
>> .
>> .
>> 0) | process_one_work() {
>> 0) | find_worker_executing_work() {
>> 0) 0.688 us | } /* find_worker_executing_work */
>> 0) | set_work_pool_and_clear_pending() {
>> 0) 0.734 us | } /* set_work_pool_and_clear_pending */
>> 0) | phy_state_machine() {
>> 0) | genphy_read_status() {
>> 0) ! 205.721 us | } /* genphy_read_status */
>> 0) | netif_carrier_off() {
>> 0) | do_page_fault() {
>>
>>
>> The do_page_fault() at the end indicates the NULL pointer dereference.
>>
>> That added call to phy_state_machine() turns the polling back on
>> unconditionally for a phy that should be disconnected. How is that
>> correct?
>
> It is not fundamentally correct and I don't think there was any
> objection to that to begin with. In fact there is a bug/inefficiency
> here in that if we have entered the PHY state machine with PHY_HALTED we
> should not re-schedule it period, only applicable to PHY_POLL cases
> *and* properly calling phy_stop() followed by phy_disconnect().
>
> What I now think is happening in your case is the following:
>
> phy_stop() was not called, so nothing does set phydev->state to
> PHY_HALTED in the first place so we have:
>
> phy_disconnect()
> -> phy_stop_machine()
> -> cancel_delayed_work_sync() OK
> phydev->state is probably RUNNING so we have:
> -> phydev->state = PHY_UP
> phy_state_machine() is called synchronously
> -> PHY_UP -> needs_aneg = true
> -> phy_restart_aneg()
> -> queue_delayed_work_sync()
> -> phydev->adjust_link = NULL
> -> phy_deatch() -> boom
>
> Can you confirm whether the driver you are using does call phy_stop()
> prior to phy_disconnect()?
There is no call to phy_stop().
I can add this to the ethernet drivers, but I wonder if it should be
called by the code code when doing phy_disconnect(), if it was not
already stopped.
> If that is the case then this whole theory
> falls apart, if not, then this needs fixing in both the driver and PHYLIB.
>
> Thanks
>
^ permalink raw reply
* RE: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Woojung.Huh @ 2017-09-06 23:44 UTC (permalink / raw)
To: f.fainelli, andrew; +Cc: roopa, netdev, vivien.didelot, jbe, sean.wang, john
In-Reply-To: <44233399-2cb9-35af-2b93-b0024aac8dc4@gmail.com>
> That being said, I have a feeling that the Marvell switches behave a
> tiny bit differently than others in that they do not flood broadcast by
> default in a given L2 domain.
Florian,
Because some DSA switches from Marvell & Microchip can do IGMP snooping,
can we propose switch layer another flag what to do when HW support it?
- Woojung
^ permalink raw reply
* Re: [patch net] net: sched: fix memleak for chain zero
From: Cong Wang @ 2017-09-06 23:37 UTC (permalink / raw)
To: Jiri Pirko
Cc: Linux Kernel Network Developers, David Miller, Jamal Hadi Salim,
Jakub Kicinski, mlxsw
In-Reply-To: <20170906203323.GA16570@nanopsycho>
On Wed, Sep 6, 2017 at 1:33 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Wed, Sep 06, 2017 at 07:40:02PM CEST, xiyou.wangcong@gmail.com wrote:
>>On Wed, Sep 6, 2017 at 4:14 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> From: Jiri Pirko <jiri@mellanox.com>
>>>
>>> There's a memleak happening for chain 0. The thing is, chain 0 needs to
>>> be always present, not created on demand. Therefore tcf_block_get upon
>>> creation of block calls the tcf_chain_create function directly. The
>>> chain is created with refcnt == 1, which is not correct in this case and
>>> causes the memleak. So move the refcnt increment into tcf_chain_get
>>> function even for the case when chain needs to be created.
>>>
>>
>>Your approach could work but you just make the code even
>>uglier than it is now:
>>
>>1. The current code is already ugly for special-casing chain 0:
>>
>> if (--chain->refcnt == 0 && !chain->filter_chain && chain->index != 0)
>> tcf_chain_destroy(chain);
>>
>>2. With your patch, chain 0 has a different _initial_ refcnt with others.
>
> No. Initial refcnt is the same. ! for every action that holds the chain.
> So actually, it returns it back where it should be.
Not all all.
tcf_block_get() calls tcf_chain_create(, 0), after your patch
chain 0 has refcnt==0 initially.
Non-0 chain? They are created via tcf_chain_get(), aka,
refcnt==0 initially.
>
>
>>
>>3. Allowing an object (chain 0) exists with refcnt==0
>
> So? That is for every chain that does not have goto_chain action
> pointing at. Please read the code.
So you are pretending to be GC but you are apparently not.
You create all the troubles by setting yourself to believe chain 0
is special and refcnt==0 is okay. Both are wrong.
Actually the !list_empty() check is totally unnecessary too,
it is yet another place you get it wrong, you hide the race
condition in commit 744a4cf63e52 which makes it harder
to expose.
I understand you don't trust me. Look at DaveM's reaction
to your refcnt==0 madness.
Remember, refcnt can be very simple, you just want to
make it harder by abusing it or attempting to invent a GC.
I am going to update my patch (to remove all your madness)
since this is horribly wrong to me. Sorry.
^ permalink raw reply
* Re: [GIT] Networking
From: Linus Torvalds @ 2017-09-06 23:37 UTC (permalink / raw)
To: David Miller, Johannes Berg, Emmanuel Grumbach, Luca Coelho,
Kalle Valo
Cc: Andrew Morton, Network Development, Linux Kernel Mailing List,
Intel Linux Wireless, Linux Wireless List
In-Reply-To: <CA+55aFw0KNTKFrRHk5hthDGTGgL9BGt9G=x_m9Tz7m_-pN+NoA@mail.gmail.com>
On Wed, Sep 6, 2017 at 4:27 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The firmware is iwlwifi-8000C-28.ucode from
> iwl7260-firmware-25.30.13.0-75.fc26.noarch, and the kernel reports
>
> iwlwifi 0000:3a:00.0: loaded firmware version 27.455470.0 op_mode iwlmvm
And when I said "iwlwifi-8000C-28.ucode" I obviously meant
"iwlwifi-8000C-27.ucode".
At least it was _hopefully_ obvious from that "27" in the actual
version number it reports.
Linus
^ permalink raw reply
* Re: [GIT] Networking
From: David Miller @ 2017-09-06 23:31 UTC (permalink / raw)
To: torvalds
Cc: johannes.berg, emmanuel.grumbach, luciano.coelho, kvalo, akpm,
netdev, linux-kernel, linuxwifi, linux-wireless
In-Reply-To: <CA+55aFw0KNTKFrRHk5hthDGTGgL9BGt9G=x_m9Tz7m_-pN+NoA@mail.gmail.com>
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed, 6 Sep 2017 16:27:15 -0700
> This pull request completely breaks Intel wireless for me.
>
> This is my trusty old XPS 13 (9350), using Intel Wireless 8260 (rev 3a).
>
> That remains a very standard Intel machine with absolutely zero odd
> things going on.
>
> The firmware is iwlwifi-8000C-28.ucode from
> iwl7260-firmware-25.30.13.0-75.fc26.noarch, and the kernel reports
...
Johannes and other Intel folks please look into this.
^ permalink raw reply
* Re: [GIT] Networking
From: Linus Torvalds @ 2017-09-06 23:27 UTC (permalink / raw)
To: David Miller, Johannes Berg, Emmanuel Grumbach, Luca Coelho,
Kalle Valo
Cc: Andrew Morton, Network Development, Linux Kernel Mailing List,
Intel Linux Wireless, Linux Wireless List
In-Reply-To: <20170905.214143.826912481689443792.davem@davemloft.net>
This pull request completely breaks Intel wireless for me.
This is my trusty old XPS 13 (9350), using Intel Wireless 8260 (rev 3a).
That remains a very standard Intel machine with absolutely zero odd
things going on.
The firmware is iwlwifi-8000C-28.ucode from
iwl7260-firmware-25.30.13.0-75.fc26.noarch, and the kernel reports
iwlwifi 0000:3a:00.0: loaded firmware version 27.455470.0 op_mode iwlmvm
the thing starts acting badly with this:
iwlwifi 0000:3a:00.0: FW Error notification: type 0x00000000 cmd_id 0x04
iwlwifi 0000:3a:00.0: FW Error notification: seq 0x0000 service 0x00000004
iwlwifi 0000:3a:00.0: FW Error notification: timestamp 0x 5D84
iwlwifi 0000:3a:00.0: Microcode SW error detected. Restarting 0x2000000.
iwlwifi 0000:3a:00.0: Start IWL Error Log Dump:
iwlwifi 0000:3a:00.0: Status: 0x00000100, count: 6
iwlwifi 0000:3a:00.0: Loaded firmware version: 27.455470.0
iwlwifi 0000:3a:00.0: 0x00000038 | BAD_COMMAND
iwlwifi 0000:3a:00.0: 0x00A002F0 | trm_hw_status0
...
iwlwifi 0000:3a:00.0: 0x00000000 | isr status reg
ieee80211 phy0: Hardware restart was requested
iwlwifi 0000:3a:00.0: FW error in SYNC CMD MAC_CONTEXT_CMD
CPU: 2 PID: 993 Comm: NetworkManager Not tainted 4.13.0-06466-g80cee03bf1d6 #4
Hardware name: Dell Inc. XPS 13 9350/09JHRY, BIOS 1.4.17 05/10/2017
Call Trace:
dump_stack+0x4d/0x70
iwl_trans_pcie_send_hcmd+0x4e7/0x530 [iwlwifi]
? wait_woken+0x80/0x80
iwl_trans_send_cmd+0x5c/0xc0 [iwlwifi]
iwl_mvm_send_cmd+0x32/0x90 [iwlmvm]
iwl_mvm_send_cmd_pdu+0x58/0x80 [iwlmvm]
iwl_mvm_mac_ctxt_send_cmd+0x2a/0x60 [iwlmvm]
? iwl_mvm_mac_ctxt_send_cmd+0x2a/0x60 [iwlmvm]
iwl_mvm_mac_ctxt_cmd_sta+0x140/0x1e0 [iwlmvm]
iwl_mvm_mac_ctx_send+0x2d/0x60 [iwlmvm]
iwl_mvm_mac_ctxt_add+0x43/0xc0 [iwlmvm]
iwl_mvm_mac_add_interface+0x139/0x2b0 [iwlmvm]
? iwl_led_brightness_set+0x1f/0x30 [iwlmvm]
drv_add_interface+0x4a/0x120 [mac80211]
ieee80211_do_open+0x33d/0x820 [mac80211]
ieee80211_open+0x52/0x60 [mac80211]
__dev_open+0xae/0x120
__dev_change_flags+0x17b/0x1c0
dev_change_flags+0x29/0x60
do_setlink+0x2f7/0xe60
? __nla_put+0x20/0x30
? _raw_read_unlock_bh+0x20/0x30
? inet6_fill_ifla6_attrs+0x4be/0x4e0
? __kmalloc_node_track_caller+0x35/0x2b0
? nla_parse+0x35/0x100
rtnl_newlink+0x5d2/0x8f0
? __netlink_sendskb+0x3b/0x60
? security_capset+0x40/0x80
? ns_capable_common+0x68/0x80
? ns_capable+0x13/0x20
rtnetlink_rcv_msg+0x1f9/0x280
? rtnl_calcit.isra.26+0x110/0x110
netlink_rcv_skb+0x8e/0x130
rtnetlink_rcv+0x15/0x20
netlink_unicast+0x18b/0x220
netlink_sendmsg+0x2ad/0x3a0
sock_sendmsg+0x38/0x50
___sys_sendmsg+0x269/0x2c0
? addrconf_sysctl_forward+0x114/0x280
? dev_forward_change+0x140/0x140
? sysctl_head_finish.part.22+0x32/0x40
? lockref_put_or_lock+0x5e/0x80
? dput.part.22+0x13e/0x1c0
? mntput+0x24/0x40
__sys_sendmsg+0x54/0x90
? __sys_sendmsg+0x54/0x90
SyS_sendmsg+0x12/0x20
entry_SYSCALL_64_fastpath+0x13/0x94
RIP: 0033:0x7ff1f9933134
RSP: 002b:00007ffe7419b460 EFLAGS: 00000293 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 000055604b6d82b9 RCX: 00007ff1f9933134
RDX: 0000000000000000 RSI: 00007ffe7419b4b0 RDI: 0000000000000007
RBP: 00007ffe7419b940 R08: 0000000000000000 R09: 000055604d16b400
R10: 00007ff1f7cf8b38 R11: 0000000000000293 R12: 0000000000000001
R13: 0000000000000001 R14: 00007ffe7419b670 R15: 000055604b9515a0
iwlwifi 0000:3a:00.0: Failed to send MAC context (action:1): -5
and it doesn't get any better from there. The next error seems to be
Timeout waiting for hardware access (CSR_GP_CNTRL 0x08000008)
------------[ cut here ]------------
WARNING: CPU: 3 PID: 1075 at
drivers/net/wireless/intel/iwlwifi/pcie/trans.c:1874
iwl_trans_pcie_grab_nic_access+0xdf/0xf0 [iwlwifi]
and it will continue with those microcode failure errors and various
other warnigns about how nothing is working.
And no, nothing works. A lot of log output, no actual network access..
Linus
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Florian Fainelli @ 2017-09-06 23:14 UTC (permalink / raw)
To: David Daney, David Daney, Mason
Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
Andrew Lunn, Mans Rullgard
In-Reply-To: <4a65e53c-f13b-9cc3-bffa-f2f2aae423b9@gmail.com>
On 09/06/2017 03:51 PM, David Daney wrote:
> On 09/06/2017 01:49 PM, David Daney wrote:
>> On 09/06/2017 11:59 AM, Florian Fainelli wrote:
>>> On 09/06/2017 11:00 AM, David Daney wrote:
>>>> On 08/31/2017 11:29 AM, Florian Fainelli wrote:
>>>>> On 08/31/2017 11:12 AM, Mason wrote:
>>>>>> On 31/08/2017 19:53, Florian Fainelli wrote:
>>>>>>> On 08/31/2017 10:49 AM, Mason wrote:
>>>>>>>> On 31/08/2017 18:57, Florian Fainelli wrote:
>>>>>>>>> And the race is between phy_detach() setting phydev->attached_dev
>>>>>>>>> = NULL
>>>>>>>>> and phy_state_machine() running in PHY_HALTED state and calling
>>>>>>>>> netif_carrier_off().
>>>>>>>>
>>>>>>>> I must be missing something.
>>>>>>>> (Since a thread cannot race against itself.)
>>>>>>>>
>>>>>>>> phy_disconnect calls phy_stop_machine which
>>>>>>>> 1) stops the work queue from running in a separate thread
>>>>>>>> 2) calls phy_state_machine *synchronously*
>>>>>>>> which runs the PHY_HALTED case with everything well-defined
>>>>>>>> end of phy_stop_machine
>>>>>>>>
>>>>>>>> phy_disconnect only then calls phy_detach()
>>>>>>>> which makes future calls of phy_state_machine perilous.
>>>>>>>>
>>>>>>>> This all happens in the same thread, so I'm not yet
>>>>>>>> seeing where the race happens?
>>>>>>>
>>>>>>> The race is as described in David's earlier email, so let's recap:
>>>>>>>
>>>>>>> Thread 1 Thread 2
>>>>>>> phy_disconnect()
>>>>>>> phy_stop_interrupts()
>>>>>>> phy_stop_machine()
>>>>>>> phy_state_machine()
>>>>>>> -> queue_delayed_work()
>>>>>>> phy_detach()
>>>>>>> phy_state_machine()
>>>>>>> -> netif_carrier_off()
>>>>>>>
>>>>>>> If phy_detach() finishes earlier than the workqueue had a chance
>>>>>>> to be
>>>>>>> scheduled and process PHY_HALTED again, then we trigger the NULL
>>>>>>> pointer
>>>>>>> de-reference.
>>>>>>>
>>>>>>> workqueues are not tasklets, the CPU scheduling them gets no
>>>>>>> guarantee
>>>>>>> they will run on the same CPU.
>>>>>>
>>>>>> Something does not add up.
>>>>>>
>>>>>> The synchronous call to phy_state_machine() does:
>>>>>>
>>>>>> case PHY_HALTED:
>>>>>> if (phydev->link) {
>>>>>> phydev->link = 0;
>>>>>> netif_carrier_off(phydev->attached_dev);
>>>>>> phy_adjust_link(phydev);
>>>>>> do_suspend = true;
>>>>>> }
>>>>>>
>>>>>> then sets phydev->link = 0; therefore subsequent calls to
>>>>>> phy_state_machin() will be no-op.
>>>>>
>>>>> Actually you are right, once phydev->link is set to 0 these would
>>>>> become
>>>>> no-ops. Still scratching my head as to what happens for David then...
>>>>>
>>>>>>
>>>>>> Also, queue_delayed_work() is only called in polling mode.
>>>>>> David stated that he's using interrupt mode.
>>>>
>>>> Did you see what I wrote?
>>>
>>> Still not following, see below.
>>>
>>>>
>>>> phy_disconnect() calls phy_stop_interrupts() which puts it into polling
>>>> mode. So the polling work gets queued unconditionally.
>>>
>>> What part of phy_stop_interrupts() is responsible for changing
>>> phydev->irq to PHY_POLL? free_irq() cannot touch phydev->irq otherwise
>>> subsequent request_irq() calls won't work anymore.
>>> phy_disable_interrupts() only calls back into the PHY driver to
>>> acknowledge and clear interrupts.
>>>
>>> If we were using a PHY with PHY_POLL, as Marc said, the first
>>> synchronous call to phy_state_machine() would have acted on PHY_HALTED
>>> and even if we incorrectly keep re-scheduling the state machine from
>>> PHY_HALTED to PHY_HALTED the second time around nothing can happen.
>>>
>>> What are we missing here?
>>>
>>
>> OK, I am now as confused as you guys are. I will go back and get an
>> ftrace log out of the failure.
>>
> OK, let's forget about the PHY_HALTED discussion.
>
>
> Consider instead the case of a Marvell phy with no interrupts connected
> on a v4.9.43 kernel, single CPU:
>
>
> 0) | phy_disconnect() {
> 0) | phy_stop_machine() {
> 0) | cancel_delayed_work_sync() {
> 0) + 23.986 us | } /* cancel_delayed_work_sync */
> 0) | phy_state_machine() {
> 0) | phy_start_aneg_priv() {
Thanks for providing the trace, I think I have an idea of what's going
on, see below.
> 0) | marvell_config_aneg() {
> 0) ! 240.538 us | } /* marvell_config_aneg */
> 0) ! 244.971 us | } /* phy_start_aneg_priv */
> 0) | queue_delayed_work_on() {
> 0) + 18.016 us | } /* queue_delayed_work_on */
> 0) ! 268.184 us | } /* phy_state_machine */
> 0) ! 297.394 us | } /* phy_stop_machine */
> 0) | phy_detach() {
> 0) | phy_suspend() {
> 0) | phy_ethtool_get_wol() {
> 0) 0.677 us | } /* phy_ethtool_get_wol */
> 0) | genphy_suspend() {
> 0) + 71.250 us | } /* genphy_suspend */
> 0) + 74.197 us | } /* phy_suspend */
> 0) + 80.302 us | } /* phy_detach */
> 0) ! 380.072 us | } /* phy_disconnect */
> .
> .
> .
> 0) | process_one_work() {
> 0) | find_worker_executing_work() {
> 0) 0.688 us | } /* find_worker_executing_work */
> 0) | set_work_pool_and_clear_pending() {
> 0) 0.734 us | } /* set_work_pool_and_clear_pending */
> 0) | phy_state_machine() {
> 0) | genphy_read_status() {
> 0) ! 205.721 us | } /* genphy_read_status */
> 0) | netif_carrier_off() {
> 0) | do_page_fault() {
>
>
> The do_page_fault() at the end indicates the NULL pointer dereference.
>
> That added call to phy_state_machine() turns the polling back on
> unconditionally for a phy that should be disconnected. How is that
> correct?
It is not fundamentally correct and I don't think there was any
objection to that to begin with. In fact there is a bug/inefficiency
here in that if we have entered the PHY state machine with PHY_HALTED we
should not re-schedule it period, only applicable to PHY_POLL cases
*and* properly calling phy_stop() followed by phy_disconnect().
What I now think is happening in your case is the following:
phy_stop() was not called, so nothing does set phydev->state to
PHY_HALTED in the first place so we have:
phy_disconnect()
-> phy_stop_machine()
-> cancel_delayed_work_sync() OK
phydev->state is probably RUNNING so we have:
-> phydev->state = PHY_UP
phy_state_machine() is called synchronously
-> PHY_UP -> needs_aneg = true
-> phy_restart_aneg()
-> queue_delayed_work_sync()
-> phydev->adjust_link = NULL
-> phy_deatch() -> boom
Can you confirm whether the driver you are using does call phy_stop()
prior to phy_disconnect()? If that is the case then this whole theory
falls apart, if not, then this needs fixing in both the driver and PHYLIB.
Thanks
--
Florian
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: David Daney @ 2017-09-06 22:51 UTC (permalink / raw)
To: David Daney, Florian Fainelli, Mason
Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
Andrew Lunn, Mans Rullgard
In-Reply-To: <6890a27f-e87e-62c1-a676-e5ddf968adb6@caviumnetworks.com>
On 09/06/2017 01:49 PM, David Daney wrote:
> On 09/06/2017 11:59 AM, Florian Fainelli wrote:
>> On 09/06/2017 11:00 AM, David Daney wrote:
>>> On 08/31/2017 11:29 AM, Florian Fainelli wrote:
>>>> On 08/31/2017 11:12 AM, Mason wrote:
>>>>> On 31/08/2017 19:53, Florian Fainelli wrote:
>>>>>> On 08/31/2017 10:49 AM, Mason wrote:
>>>>>>> On 31/08/2017 18:57, Florian Fainelli wrote:
>>>>>>>> And the race is between phy_detach() setting phydev->attached_dev
>>>>>>>> = NULL
>>>>>>>> and phy_state_machine() running in PHY_HALTED state and calling
>>>>>>>> netif_carrier_off().
>>>>>>>
>>>>>>> I must be missing something.
>>>>>>> (Since a thread cannot race against itself.)
>>>>>>>
>>>>>>> phy_disconnect calls phy_stop_machine which
>>>>>>> 1) stops the work queue from running in a separate thread
>>>>>>> 2) calls phy_state_machine *synchronously*
>>>>>>> which runs the PHY_HALTED case with everything well-defined
>>>>>>> end of phy_stop_machine
>>>>>>>
>>>>>>> phy_disconnect only then calls phy_detach()
>>>>>>> which makes future calls of phy_state_machine perilous.
>>>>>>>
>>>>>>> This all happens in the same thread, so I'm not yet
>>>>>>> seeing where the race happens?
>>>>>>
>>>>>> The race is as described in David's earlier email, so let's recap:
>>>>>>
>>>>>> Thread 1 Thread 2
>>>>>> phy_disconnect()
>>>>>> phy_stop_interrupts()
>>>>>> phy_stop_machine()
>>>>>> phy_state_machine()
>>>>>> -> queue_delayed_work()
>>>>>> phy_detach()
>>>>>> phy_state_machine()
>>>>>> -> netif_carrier_off()
>>>>>>
>>>>>> If phy_detach() finishes earlier than the workqueue had a chance
>>>>>> to be
>>>>>> scheduled and process PHY_HALTED again, then we trigger the NULL
>>>>>> pointer
>>>>>> de-reference.
>>>>>>
>>>>>> workqueues are not tasklets, the CPU scheduling them gets no
>>>>>> guarantee
>>>>>> they will run on the same CPU.
>>>>>
>>>>> Something does not add up.
>>>>>
>>>>> The synchronous call to phy_state_machine() does:
>>>>>
>>>>> case PHY_HALTED:
>>>>> if (phydev->link) {
>>>>> phydev->link = 0;
>>>>> netif_carrier_off(phydev->attached_dev);
>>>>> phy_adjust_link(phydev);
>>>>> do_suspend = true;
>>>>> }
>>>>>
>>>>> then sets phydev->link = 0; therefore subsequent calls to
>>>>> phy_state_machin() will be no-op.
>>>>
>>>> Actually you are right, once phydev->link is set to 0 these would
>>>> become
>>>> no-ops. Still scratching my head as to what happens for David then...
>>>>
>>>>>
>>>>> Also, queue_delayed_work() is only called in polling mode.
>>>>> David stated that he's using interrupt mode.
>>>
>>> Did you see what I wrote?
>>
>> Still not following, see below.
>>
>>>
>>> phy_disconnect() calls phy_stop_interrupts() which puts it into polling
>>> mode. So the polling work gets queued unconditionally.
>>
>> What part of phy_stop_interrupts() is responsible for changing
>> phydev->irq to PHY_POLL? free_irq() cannot touch phydev->irq otherwise
>> subsequent request_irq() calls won't work anymore.
>> phy_disable_interrupts() only calls back into the PHY driver to
>> acknowledge and clear interrupts.
>>
>> If we were using a PHY with PHY_POLL, as Marc said, the first
>> synchronous call to phy_state_machine() would have acted on PHY_HALTED
>> and even if we incorrectly keep re-scheduling the state machine from
>> PHY_HALTED to PHY_HALTED the second time around nothing can happen.
>>
>> What are we missing here?
>>
>
> OK, I am now as confused as you guys are. I will go back and get an
> ftrace log out of the failure.
>
OK, let's forget about the PHY_HALTED discussion.
Consider instead the case of a Marvell phy with no interrupts connected
on a v4.9.43 kernel, single CPU:
0) | phy_disconnect() {
0) | phy_stop_machine() {
0) | cancel_delayed_work_sync() {
0) + 23.986 us | } /* cancel_delayed_work_sync */
0) | phy_state_machine() {
0) | phy_start_aneg_priv() {
0) | marvell_config_aneg() {
0) ! 240.538 us | } /* marvell_config_aneg */
0) ! 244.971 us | } /* phy_start_aneg_priv */
0) | queue_delayed_work_on() {
0) + 18.016 us | } /* queue_delayed_work_on */
0) ! 268.184 us | } /* phy_state_machine */
0) ! 297.394 us | } /* phy_stop_machine */
0) | phy_detach() {
0) | phy_suspend() {
0) | phy_ethtool_get_wol() {
0) 0.677 us | } /* phy_ethtool_get_wol */
0) | genphy_suspend() {
0) + 71.250 us | } /* genphy_suspend */
0) + 74.197 us | } /* phy_suspend */
0) + 80.302 us | } /* phy_detach */
0) ! 380.072 us | } /* phy_disconnect */
.
.
.
0) | process_one_work() {
0) | find_worker_executing_work() {
0) 0.688 us | } /* find_worker_executing_work */
0) | set_work_pool_and_clear_pending() {
0) 0.734 us | } /* set_work_pool_and_clear_pending */
0) | phy_state_machine() {
0) | genphy_read_status() {
0) ! 205.721 us | } /* genphy_read_status */
0) | netif_carrier_off() {
0) | do_page_fault() {
The do_page_fault() at the end indicates the NULL pointer dereference.
That added call to phy_state_machine() turns the polling back on
unconditionally for a phy that should be disconnected. How is that correct?
David.
^ permalink raw reply
* [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
From: Kosuke Tatsukawa @ 2017-09-06 22:47 UTC (permalink / raw)
To: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Reinis Rozitis
Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
balance-alb mode") tried to fix transmit dynamic load balancing in
balance-alb mode, which wasn't working after commit 8b426dc54cf4
("bonding: remove hardcoded value").
It turned out that my previous patch only fixed the case when
balance-alb was specified as bonding module parameter, and not when
balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
common usage). In the latter case, tlb_dynamic_lb was set up according
to the default mode of the bonding interface, which happens to be
balance-rr.
This additional patch addresses this issue by setting up tlb_dynamic_lb
to 1 if "mode" is set to balance-alb through the sysfs interface.
I didn't add code to change tlb_balance_lb back to the default value for
other modes, because "mode" is usually set up only once during
initialization, and it's not worthwhile to change the static variable
bonding_defaults in bond_main.c to a global variable just for this
purpose.
Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
balance-tlb mode if it is set up using the sysfs interface. I didn't
change that behavior, because the value of tlb_balance_lb can be changed
using the sysfs interface for balance-tlb, and I didn't like changing
the default value back and forth for balance-tlb.
As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
written to. However, I think balance-alb with tlb_dynamic_lb set to 0
is not an intended usage, so there is little use making it writable at
this moment.
Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
Reported-by: Reinis Rozitis <r@roze.lv>
Signed-off-by: Kosuke Tatsukawa <tatsu@ab.jp.nec.com>
Cc: stable@vger.kernel.org # v4.12+
---
drivers/net/bonding/bond_options.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index a12d603..5931aa2 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -754,6 +754,9 @@ static int bond_option_mode_set(struct bonding *bond,
bond->params.miimon);
}
+ if (newval->value == BOND_MODE_ALB)
+ bond->params.tlb_dynamic_lb = 1;
+
/* don't cache arp_validate between modes */
bond->params.arp_validate = BOND_ARP_VALIDATE_NONE;
bond->params.mode = newval->value;
^ permalink raw reply related
* Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode
From: Kosuke Tatsukawa @ 2017-09-06 22:45 UTC (permalink / raw)
To: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Reinis Rozitis
In-Reply-To: <17EC94B0A072C34B8DCF0D30AD16044A0296FEF5@BPXM09GP.gisp.nec.co.jp>
Reinis Rozitis reported that tlb_dynamic_lb was still 0 in balance-alb
mode even when using linux-4.12.10 kernels, which includes this patch.
It turned out that my previous patch only fixed the case when
balance-alb mode was specified as bonding module parameter, and not when
balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
common usage). In the latter case, tlb_dynamic_lb was set up according
to the default mode of the bonding interface, which happens to be
balance-rr.
I'll send another patch to address this case.
In the meantime, a workaround for this issue is to specify balance-tlb
mode as a module parameter for bonding. This will change the value of
tlb_balance_lb to 1, and make balance-alb work like pre-4.12 kernels.
Best regards.
> balance-alb mode used to have transmit dynamic load balancing feature
> enabled by default. However, transmit dynamic load balancing no longer
> works in balance-alb after commit 8b426dc54cf4 ("bonding: remove
> hardcoded value").
>
> Both balance-tlb and balance-alb use the function bond_do_alb_xmit() to
> send packets. This function uses the parameter tlb_dynamic_lb.
> tlb_dynamic_lb used to have the default value of 1 for balance-alb, but
> now the value is set to 0 except in balance-tlb.
>
> Re-enable transmit dyanmic load balancing by initializing tlb_dynamic_lb
> for balance-alb similar to balance-tlb.
>
> Signed-off-by: Kosuke Tatsukawa <tatsu@ab.jp.nec.com>
> Cc: stable@vger.kernel.org
> ---
> drivers/net/bonding/bond_main.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 14ff622..181839d 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4596,7 +4596,7 @@ static int bond_check_params(struct bond_params *params)
> }
> ad_user_port_key = valptr->value;
>
> - if (bond_mode == BOND_MODE_TLB) {
> + if ((bond_mode == BOND_MODE_TLB) || (bond_mode == BOND_MODE_ALB)) {
> bond_opt_initstr(&newval, "default");
> valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB),
> &newval);
---
Kosuke TATSUKAWA | 1st Platform Software Division
| NEC Solution Innovators
| tatsu@ab.jp.nec.com
^ permalink raw reply
* Re: Fwd: DA850-evm MAC Address is random
From: Adam Ford @ 2017-09-06 21:41 UTC (permalink / raw)
To: Sekhar Nori; +Cc: Tony Lindgren, Grygorii Strashko, linux-omap, netdev
In-Reply-To: <1865772b-482e-042e-7c66-d33290f0d8fb@ti.com>
On Mon, Sep 4, 2017 at 11:42 PM, Sekhar Nori <nsekhar@ti.com> wrote:
> Hi Adam,
>
> On Wednesday 30 August 2017 11:08 AM, Sekhar Nori wrote:
>>> I wonder if U-Boot isn't pushing something to Linux because it doesn't
>>> appear to be running some of the da850 specific code even when I run
>>> linux-next. Can you tell me what verision of U-Boot you're using?
>>> Other than using davinci_all_defconfig, did you change the
>>> configuration at all?
>
>> I am using U-Boot 2017.01. Yes, the kernel was built using
>> davinci_all_defconfig and no other config change. Before booting kernel,
>> can you confirm that ethaddr is set in U-Boot environment? This is what
>> fdt_fixup_ethernet() reads to fixup the FDT before boot.
>>
>> Here is my complete boot log with environment variable dump.
>>
>> http://pastebin.ubuntu.com/25430265/
>
> Were you able to get rid of the random mac address problem?
Not yet. I haven't been able to rebuild Arago using TI's instructions
on the Wiki. I am not sure if it's a dependency issue or something
else. When I run Linux 4.13 using Buildroot as the rootfs, it does
not appear to run da850_evm_m25p80_notify_add(). I am going to
investigate whether or not da850_evm_init() is getting called. I was
wondering if you had some insight as to what calls that function? It
looks like it's defined as part of MACHINE_START(DAVINCI_DA850_EVM,
"DaVinci DA850/OMAP-L138/AM18x EVM"), but I don't know how it gets
called.
thanks
adam
>
> Thanks,
> Sekhar
^ permalink raw reply
* [PATCH net] hv_netvsc: fix deadlock on hotplug
From: Stephen Hemminger @ 2017-09-06 21:11 UTC (permalink / raw)
To: davem; +Cc: haiyangz, netdev, Stephen Hemminger
When a virtual device is added dynamically (via host console), then
the vmbus sends an offer message for the primary channel. The processing
of this message for networking causes the network device to then
initialize the sub channels.
The problem is that setting up the sub channels needs to wait until
the subsequent subchannel offers have been processed. These offers
come in on the same ring buffer and work queue as where the primary
offer is being processed; leading to a deadlock.
This did not happen in older kernels, because the sub channel waiting
logic was broken (it wasn't really waiting).
The solution is to do the sub channel setup in its own work queue
context that is scheduled by the primary channel setup; and then
happens later.
Fixes: 732e49850c5e ("netvsc: fix race on sub channel creation")
Reported-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
This is version for 4.13 (linux-net)
drivers/net/hyperv/hyperv_net.h | 3 +
drivers/net/hyperv/netvsc.c | 3 +
drivers/net/hyperv/netvsc_drv.c | 18 +-----
drivers/net/hyperv/rndis_filter.c | 118 +++++++++++++++++++++++++++-----------
4 files changed, 94 insertions(+), 48 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 12cc64bfcff8..b82119be14e9 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -200,6 +200,8 @@ int netvsc_recv_callback(struct net_device *net,
const struct ndis_pkt_8021q_info *vlan);
void netvsc_channel_cb(void *context);
int netvsc_poll(struct napi_struct *napi, int budget);
+
+void rndis_set_subchannel(struct work_struct *w);
int rndis_filter_open(struct netvsc_device *nvdev);
int rndis_filter_close(struct netvsc_device *nvdev);
int rndis_filter_device_add(struct hv_device *dev,
@@ -766,6 +768,7 @@ struct netvsc_device {
u32 num_chn;
atomic_t open_chn;
+ struct work_struct subchan_work;
wait_queue_head_t subchan_open;
struct rndis_device *extension;
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index d18c3326a1f7..76ef14cf55a1 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -79,6 +79,7 @@ static struct netvsc_device *alloc_net_device(void)
net_device->pkt_align = RNDIS_PKT_ALIGN_DEFAULT;
init_completion(&net_device->channel_init_wait);
init_waitqueue_head(&net_device->subchan_open);
+ INIT_WORK(&net_device->subchan_work, rndis_set_subchannel);
return net_device;
}
@@ -553,6 +554,8 @@ void netvsc_device_remove(struct hv_device *device)
struct netvsc_device *net_device = net_device_ctx->nvdev;
int i;
+ cancel_work_sync(&net_device->subchan_work);
+
netvsc_disconnect_vsp(device);
RCU_INIT_POINTER(net_device_ctx->nvdev, NULL);
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index d91cbc6c3ca4..9c8dd86a663e 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -740,24 +740,13 @@ static int netvsc_set_queues(struct net_device *net, struct hv_device *dev,
u32 num_chn)
{
struct netvsc_device_info device_info;
- int ret;
memset(&device_info, 0, sizeof(device_info));
device_info.num_chn = num_chn;
device_info.ring_size = ring_size;
device_info.max_num_vrss_chns = num_chn;
- ret = rndis_filter_device_add(dev, &device_info);
- if (ret)
- return ret;
-
- ret = netif_set_real_num_tx_queues(net, num_chn);
- if (ret)
- return ret;
-
- ret = netif_set_real_num_rx_queues(net, num_chn);
-
- return ret;
+ return rndis_filter_device_add(dev, &device_info);
}
static int netvsc_set_channels(struct net_device *net,
@@ -1573,8 +1562,6 @@ static int netvsc_probe(struct hv_device *dev,
/* RCU not necessary here, device not registered */
nvdev = net_device_ctx->nvdev;
- netif_set_real_num_tx_queues(net, nvdev->num_chn);
- netif_set_real_num_rx_queues(net, nvdev->num_chn);
/* MTU range: 68 - 1500 or 65521 */
net->min_mtu = NETVSC_MTU_MIN;
@@ -1616,11 +1603,10 @@ static int netvsc_remove(struct hv_device *dev)
* removed. Also blocks mtu and channel changes.
*/
rtnl_lock();
+ unregister_netdevice(net);
rndis_filter_device_remove(dev, ndev_ctx->nvdev);
rtnl_unlock();
- unregister_netdev(net);
-
hv_set_drvdata(dev, NULL);
free_netdev(net);
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index d6308ffda53e..015a8e2b733c 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -1037,8 +1037,6 @@ static void netvsc_sc_open(struct vmbus_channel *new_sc)
/* Set the channel before opening.*/
nvchan->channel = new_sc;
- netif_napi_add(ndev, &nvchan->napi,
- netvsc_poll, NAPI_POLL_WEIGHT);
ret = vmbus_open(new_sc, nvscdev->ring_size * PAGE_SIZE,
nvscdev->ring_size * PAGE_SIZE, NULL, 0,
@@ -1046,12 +1044,90 @@ static void netvsc_sc_open(struct vmbus_channel *new_sc)
if (ret == 0)
napi_enable(&nvchan->napi);
else
- netif_napi_del(&nvchan->napi);
+ netdev_notice(ndev, "sub channel open failed: %d\n", ret);
atomic_inc(&nvscdev->open_chn);
wake_up(&nvscdev->subchan_open);
}
+
+/* Open sub-channels after completing the handling of the device probe.
+ * This breaks overlap of processing the host message for the
+ * new primary channel with the initialization of sub-channels.
+ */
+void rndis_set_subchannel(struct work_struct *w)
+{
+ struct netvsc_device *nvdev
+ = container_of(w, struct netvsc_device, subchan_work);
+ struct nvsp_message *init_packet = &nvdev->channel_init_pkt;
+ struct net_device_context *ndev_ctx;
+ struct rndis_device *rdev;
+ struct net_device *ndev;
+ struct hv_device *hv_dev;
+ int i, ret;
+
+ if (!rtnl_trylock()) {
+ schedule_work(w);
+ return;
+ }
+
+ rdev = nvdev->extension;
+ if (!rdev)
+ goto unlock; /* device was removed */
+
+ ndev = rdev->ndev;
+ ndev_ctx = netdev_priv(ndev);
+ hv_dev = ndev_ctx->device_ctx;
+
+ memset(init_packet, 0, sizeof(struct nvsp_message));
+ init_packet->hdr.msg_type = NVSP_MSG5_TYPE_SUBCHANNEL;
+ init_packet->msg.v5_msg.subchn_req.op = NVSP_SUBCHANNEL_ALLOCATE;
+ init_packet->msg.v5_msg.subchn_req.num_subchannels =
+ nvdev->num_chn - 1;
+ ret = vmbus_sendpacket(hv_dev->channel, init_packet,
+ sizeof(struct nvsp_message),
+ (unsigned long)init_packet,
+ VM_PKT_DATA_INBAND,
+ VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+ if (ret) {
+ netdev_err(ndev, "sub channel allocate send failed: %d\n", ret);
+ goto failed;
+ }
+
+ wait_for_completion(&nvdev->channel_init_wait);
+ if (init_packet->msg.v5_msg.subchn_comp.status != NVSP_STAT_SUCCESS) {
+ netdev_err(ndev, "sub channel request failed\n");
+ goto failed;
+ }
+
+ nvdev->num_chn = 1 +
+ init_packet->msg.v5_msg.subchn_comp.num_subchannels;
+
+ /* wait for all sub channels to open */
+ wait_event(nvdev->subchan_open,
+ atomic_read(&nvdev->open_chn) == nvdev->num_chn);
+
+ /* ignore failues from setting rss parameters, still have channels */
+ rndis_filter_set_rss_param(rdev, netvsc_hash_key,
+ nvdev->num_chn);
+
+ netif_set_real_num_tx_queues(ndev, nvdev->num_chn);
+ netif_set_real_num_rx_queues(ndev, nvdev->num_chn);
+
+ rtnl_unlock();
+ return;
+
+failed:
+ /* fallback to only primary channel */
+ for (i = 1; i < nvdev->num_chn; i++)
+ netif_napi_del(&nvdev->chan_table[i].napi);
+
+ nvdev->max_chn = 1;
+ nvdev->num_chn = 1;
+unlock:
+ rtnl_unlock();
+}
+
int rndis_filter_device_add(struct hv_device *dev,
struct netvsc_device_info *device_info)
{
@@ -1061,7 +1137,6 @@ int rndis_filter_device_add(struct hv_device *dev,
struct rndis_device *rndis_device;
struct ndis_offload hwcaps;
struct ndis_offload_params offloads;
- struct nvsp_message *init_packet;
struct ndis_recv_scale_cap rsscap;
u32 rsscap_size = sizeof(struct ndis_recv_scale_cap);
unsigned int gso_max_size = GSO_MAX_SIZE;
@@ -1226,37 +1301,16 @@ int rndis_filter_device_add(struct hv_device *dev,
vmbus_set_sc_create_callback(dev->channel, netvsc_sc_open);
- init_packet = &net_device->channel_init_pkt;
- memset(init_packet, 0, sizeof(struct nvsp_message));
- init_packet->hdr.msg_type = NVSP_MSG5_TYPE_SUBCHANNEL;
- init_packet->msg.v5_msg.subchn_req.op = NVSP_SUBCHANNEL_ALLOCATE;
- init_packet->msg.v5_msg.subchn_req.num_subchannels =
- net_device->num_chn - 1;
- ret = vmbus_sendpacket(dev->channel, init_packet,
- sizeof(struct nvsp_message),
- (unsigned long)init_packet,
- VM_PKT_DATA_INBAND,
- VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
- if (ret)
- goto out;
- wait_for_completion(&net_device->channel_init_wait);
- if (init_packet->msg.v5_msg.subchn_comp.status != NVSP_STAT_SUCCESS) {
- ret = -ENODEV;
- goto out;
- }
+ for (i = 1; i < net_device->num_chn; i++)
+ netif_napi_add(net, &net_device->chan_table[i].napi,
+ netvsc_poll, NAPI_POLL_WEIGHT);
- net_device->num_chn = 1 +
- init_packet->msg.v5_msg.subchn_comp.num_subchannels;
-
- /* wait for all sub channels to open */
- wait_event(net_device->subchan_open,
- atomic_read(&net_device->open_chn) == net_device->num_chn);
+ if (net_device->num_chn > 1)
+ schedule_work(&net_device->subchan_work);
- /* ignore failues from setting rss parameters, still have channels */
- rndis_filter_set_rss_param(rndis_device, netvsc_hash_key,
- net_device->num_chn);
out:
+ /* if unavailable, just proceed with one queue */
if (ret) {
net_device->max_chn = 1;
net_device->num_chn = 1;
@@ -1277,10 +1331,10 @@ void rndis_filter_device_remove(struct hv_device *dev,
/* Halt and release the rndis device */
rndis_filter_halt_device(rndis_dev);
- kfree(rndis_dev);
net_dev->extension = NULL;
netvsc_device_remove(dev);
+ kfree(rndis_dev);
}
int rndis_filter_open(struct netvsc_device *nvdev)
--
2.11.0
^ permalink raw reply related
* [PATCH v2 net-next 2/2] hv_netvsc: avoid unnecessary wakeups on subchannel creation
From: Stephen Hemminger @ 2017-09-06 20:53 UTC (permalink / raw)
To: kys, haiyangz, sthemmin; +Cc: devel, netdev
In-Reply-To: <20170906205306.10541-1-sthemmin@microsoft.com>
Only need to wakeup the initiator after all sub-channels
are opened.
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
drivers/net/hyperv/rndis_filter.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index 731bc7cc6f43..065b204d8e17 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -1048,8 +1048,8 @@ static void netvsc_sc_open(struct vmbus_channel *new_sc)
else
netdev_notice(ndev, "sub channel open failed: %d\n", ret);
- atomic_inc(&nvscdev->open_chn);
- wake_up(&nvscdev->subchan_open);
+ if (atomic_inc_return(&nvscdev->open_chn) == nvscdev->num_chn)
+ wake_up(&nvscdev->subchan_open);
}
/* Open sub-channels after completing the handling of the device probe.
--
2.11.0
^ permalink raw reply related
* [PATCH v2 net-next 1/2] hv_netvsc: fix deadlock on hotplug
From: Stephen Hemminger @ 2017-09-06 20:53 UTC (permalink / raw)
To: kys, haiyangz, sthemmin; +Cc: devel, netdev
In-Reply-To: <20170906205306.10541-1-sthemmin@microsoft.com>
When a virtual device is added dynamically (via host console), then
the vmbus sends an offer message for the primary channel. The processing
of this message for networking causes the network device to then
initialize the sub channels.
The problem is that setting up the sub channels needs to wait until
the subsequent subchannel offers have been processed. These offers
come in on the same ring buffer and work queue as where the primary
offer is being processed; leading to a deadlock.
This did not happen in older kernels, because the sub channel waiting
logic was broken (it wasn't really waiting).
The solution is to do the sub channel setup in its own work queue
context that is scheduled by the primary channel setup; and then
happens later.
Fixes: 732e49850c5e ("netvsc: fix race on sub channel creation")
Reported-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
v2 - fix module removal race with new work queue
drivers/net/hyperv/hyperv_net.h | 3 +
drivers/net/hyperv/netvsc.c | 3 +
drivers/net/hyperv/netvsc_drv.c | 11 +---
drivers/net/hyperv/rndis_filter.c | 122 ++++++++++++++++++++++++++------------
4 files changed, 94 insertions(+), 45 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index ec546da86683..d98cdfb1536b 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -204,6 +204,8 @@ int netvsc_recv_callback(struct net_device *net,
const struct ndis_pkt_8021q_info *vlan);
void netvsc_channel_cb(void *context);
int netvsc_poll(struct napi_struct *napi, int budget);
+
+void rndis_set_subchannel(struct work_struct *w);
bool rndis_filter_opened(const struct netvsc_device *nvdev);
int rndis_filter_open(struct netvsc_device *nvdev);
int rndis_filter_close(struct netvsc_device *nvdev);
@@ -782,6 +784,7 @@ struct netvsc_device {
u32 num_chn;
atomic_t open_chn;
+ struct work_struct subchan_work;
wait_queue_head_t subchan_open;
struct rndis_device *extension;
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 0062b802676f..a5511b7326af 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -81,6 +81,7 @@ static struct netvsc_device *alloc_net_device(void)
init_completion(&net_device->channel_init_wait);
init_waitqueue_head(&net_device->subchan_open);
+ INIT_WORK(&net_device->subchan_work, rndis_set_subchannel);
return net_device;
}
@@ -557,6 +558,8 @@ void netvsc_device_remove(struct hv_device *device)
= rtnl_dereference(net_device_ctx->nvdev);
int i;
+ cancel_work_sync(&net_device->subchan_work);
+
netvsc_disconnect_vsp(device);
RCU_INIT_POINTER(net_device_ctx->nvdev, NULL);
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 165ba4b3b423..c538a4f15f3b 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -853,10 +853,7 @@ static int netvsc_set_channels(struct net_device *net,
rndis_filter_device_remove(dev, nvdev);
nvdev = rndis_filter_device_add(dev, &device_info);
- if (!IS_ERR(nvdev)) {
- netif_set_real_num_tx_queues(net, nvdev->num_chn);
- netif_set_real_num_rx_queues(net, nvdev->num_chn);
- } else {
+ if (IS_ERR(nvdev)) {
ret = PTR_ERR(nvdev);
device_info.num_chn = orig;
nvdev = rndis_filter_device_add(dev, &device_info);
@@ -1954,9 +1951,6 @@ static int netvsc_probe(struct hv_device *dev,
NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX;
net->vlan_features = net->features;
- netif_set_real_num_tx_queues(net, nvdev->num_chn);
- netif_set_real_num_rx_queues(net, nvdev->num_chn);
-
netdev_lockdep_set_classes(net);
/* MTU range: 68 - 1500 or 65521 */
@@ -2012,9 +2006,10 @@ static int netvsc_remove(struct hv_device *dev)
if (vf_netdev)
netvsc_unregister_vf(vf_netdev);
+ unregister_netdevice(net);
+
rndis_filter_device_remove(dev,
rtnl_dereference(ndev_ctx->nvdev));
- unregister_netdevice(net);
rtnl_unlock();
hv_set_drvdata(dev, NULL);
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index 69c40b8fccc3..731bc7cc6f43 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -1039,8 +1039,6 @@ static void netvsc_sc_open(struct vmbus_channel *new_sc)
/* Set the channel before opening.*/
nvchan->channel = new_sc;
- netif_napi_add(ndev, &nvchan->napi,
- netvsc_poll, NAPI_POLL_WEIGHT);
ret = vmbus_open(new_sc, nvscdev->ring_size * PAGE_SIZE,
nvscdev->ring_size * PAGE_SIZE, NULL, 0,
@@ -1048,12 +1046,88 @@ static void netvsc_sc_open(struct vmbus_channel *new_sc)
if (ret == 0)
napi_enable(&nvchan->napi);
else
- netif_napi_del(&nvchan->napi);
+ netdev_notice(ndev, "sub channel open failed: %d\n", ret);
atomic_inc(&nvscdev->open_chn);
wake_up(&nvscdev->subchan_open);
}
+/* Open sub-channels after completing the handling of the device probe.
+ * This breaks overlap of processing the host message for the
+ * new primary channel with the initialization of sub-channels.
+ */
+void rndis_set_subchannel(struct work_struct *w)
+{
+ struct netvsc_device *nvdev
+ = container_of(w, struct netvsc_device, subchan_work);
+ struct nvsp_message *init_packet = &nvdev->channel_init_pkt;
+ struct net_device_context *ndev_ctx;
+ struct rndis_device *rdev;
+ struct net_device *ndev;
+ struct hv_device *hv_dev;
+ int i, ret;
+
+ if (!rtnl_trylock()) {
+ schedule_work(w);
+ return;
+ }
+
+ rdev = nvdev->extension;
+ if (!rdev)
+ goto unlock; /* device was removed */
+
+ ndev = rdev->ndev;
+ ndev_ctx = netdev_priv(ndev);
+ hv_dev = ndev_ctx->device_ctx;
+
+ memset(init_packet, 0, sizeof(struct nvsp_message));
+ init_packet->hdr.msg_type = NVSP_MSG5_TYPE_SUBCHANNEL;
+ init_packet->msg.v5_msg.subchn_req.op = NVSP_SUBCHANNEL_ALLOCATE;
+ init_packet->msg.v5_msg.subchn_req.num_subchannels =
+ nvdev->num_chn - 1;
+ ret = vmbus_sendpacket(hv_dev->channel, init_packet,
+ sizeof(struct nvsp_message),
+ (unsigned long)init_packet,
+ VM_PKT_DATA_INBAND,
+ VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+ if (ret) {
+ netdev_err(ndev, "sub channel allocate send failed: %d\n", ret);
+ goto failed;
+ }
+
+ wait_for_completion(&nvdev->channel_init_wait);
+ if (init_packet->msg.v5_msg.subchn_comp.status != NVSP_STAT_SUCCESS) {
+ netdev_err(ndev, "sub channel request failed\n");
+ goto failed;
+ }
+
+ nvdev->num_chn = 1 +
+ init_packet->msg.v5_msg.subchn_comp.num_subchannels;
+
+ /* wait for all sub channels to open */
+ wait_event(nvdev->subchan_open,
+ atomic_read(&nvdev->open_chn) == nvdev->num_chn);
+
+ /* ignore failues from setting rss parameters, still have channels */
+ rndis_filter_set_rss_param(rdev, netvsc_hash_key);
+
+ netif_set_real_num_tx_queues(ndev, nvdev->num_chn);
+ netif_set_real_num_rx_queues(ndev, nvdev->num_chn);
+
+ rtnl_unlock();
+ return;
+
+failed:
+ /* fallback to only primary channel */
+ for (i = 1; i < nvdev->num_chn; i++)
+ netif_napi_del(&nvdev->chan_table[i].napi);
+
+ nvdev->max_chn = 1;
+ nvdev->num_chn = 1;
+unlock:
+ rtnl_unlock();
+}
+
struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,
struct netvsc_device_info *device_info)
{
@@ -1063,7 +1137,6 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,
struct rndis_device *rndis_device;
struct ndis_offload hwcaps;
struct ndis_offload_params offloads;
- struct nvsp_message *init_packet;
struct ndis_recv_scale_cap rsscap;
u32 rsscap_size = sizeof(struct ndis_recv_scale_cap);
unsigned int gso_max_size = GSO_MAX_SIZE;
@@ -1215,9 +1288,7 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,
net_device->num_chn);
atomic_set(&net_device->open_chn, 1);
-
- if (net_device->num_chn == 1)
- return net_device;
+ vmbus_set_sc_create_callback(dev->channel, netvsc_sc_open);
for (i = 1; i < net_device->num_chn; i++) {
ret = netvsc_alloc_recv_comp_ring(net_device, i);
@@ -1228,38 +1299,15 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,
}
}
- vmbus_set_sc_create_callback(dev->channel, netvsc_sc_open);
+ for (i = 1; i < net_device->num_chn; i++)
+ netif_napi_add(net, &net_device->chan_table[i].napi,
+ netvsc_poll, NAPI_POLL_WEIGHT);
- init_packet = &net_device->channel_init_pkt;
- memset(init_packet, 0, sizeof(struct nvsp_message));
- init_packet->hdr.msg_type = NVSP_MSG5_TYPE_SUBCHANNEL;
- init_packet->msg.v5_msg.subchn_req.op = NVSP_SUBCHANNEL_ALLOCATE;
- init_packet->msg.v5_msg.subchn_req.num_subchannels =
- net_device->num_chn - 1;
- ret = vmbus_sendpacket(dev->channel, init_packet,
- sizeof(struct nvsp_message),
- (unsigned long)init_packet,
- VM_PKT_DATA_INBAND,
- VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
- if (ret)
- goto out;
-
- wait_for_completion(&net_device->channel_init_wait);
- if (init_packet->msg.v5_msg.subchn_comp.status != NVSP_STAT_SUCCESS) {
- ret = -ENODEV;
- goto out;
- }
+ if (net_device->num_chn > 1)
+ schedule_work(&net_device->subchan_work);
- net_device->num_chn = 1 +
- init_packet->msg.v5_msg.subchn_comp.num_subchannels;
-
- /* wait for all sub channels to open */
- wait_event(net_device->subchan_open,
- atomic_read(&net_device->open_chn) == net_device->num_chn);
-
- /* ignore failues from setting rss parameters, still have channels */
- rndis_filter_set_rss_param(rndis_device, netvsc_hash_key);
out:
+ /* if unavailable, just proceed with one queue */
if (ret) {
net_device->max_chn = 1;
net_device->num_chn = 1;
@@ -1280,10 +1328,10 @@ void rndis_filter_device_remove(struct hv_device *dev,
/* Halt and release the rndis device */
rndis_filter_halt_device(rndis_dev);
- kfree(rndis_dev);
net_dev->extension = NULL;
netvsc_device_remove(dev);
+ kfree(rndis_dev);
}
int rndis_filter_open(struct netvsc_device *nvdev)
--
2.11.0
^ permalink raw reply related
* [PATCH v2 net-next 0/2] hv_netvsc: sub channel initialization fixes
From: Stephen Hemminger @ 2017-09-06 20:53 UTC (permalink / raw)
To: kys, haiyangz, sthemmin; +Cc: devel, netdev
One serious deadlock, and one minor optimization.
Stephen Hemminger (2):
hv_netvsc: fix deadlock on hotplug
hv_netvsc: avoid unnecessary wakeups on subchannel creation
drivers/net/hyperv/hyperv_net.h | 3 +
drivers/net/hyperv/netvsc.c | 3 +
drivers/net/hyperv/netvsc_drv.c | 11 +---
drivers/net/hyperv/rndis_filter.c | 126 ++++++++++++++++++++++++++------------
4 files changed, 96 insertions(+), 47 deletions(-)
--
2.11.0
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: David Daney @ 2017-09-06 20:49 UTC (permalink / raw)
To: Florian Fainelli, Mason
Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
Andrew Lunn, Mans Rullgard
In-Reply-To: <572f49fd-f623-f064-a551-e243c57cef7f@gmail.com>
On 09/06/2017 11:59 AM, Florian Fainelli wrote:
> On 09/06/2017 11:00 AM, David Daney wrote:
>> On 08/31/2017 11:29 AM, Florian Fainelli wrote:
>>> On 08/31/2017 11:12 AM, Mason wrote:
>>>> On 31/08/2017 19:53, Florian Fainelli wrote:
>>>>> On 08/31/2017 10:49 AM, Mason wrote:
>>>>>> On 31/08/2017 18:57, Florian Fainelli wrote:
>>>>>>> And the race is between phy_detach() setting phydev->attached_dev
>>>>>>> = NULL
>>>>>>> and phy_state_machine() running in PHY_HALTED state and calling
>>>>>>> netif_carrier_off().
>>>>>>
>>>>>> I must be missing something.
>>>>>> (Since a thread cannot race against itself.)
>>>>>>
>>>>>> phy_disconnect calls phy_stop_machine which
>>>>>> 1) stops the work queue from running in a separate thread
>>>>>> 2) calls phy_state_machine *synchronously*
>>>>>> which runs the PHY_HALTED case with everything well-defined
>>>>>> end of phy_stop_machine
>>>>>>
>>>>>> phy_disconnect only then calls phy_detach()
>>>>>> which makes future calls of phy_state_machine perilous.
>>>>>>
>>>>>> This all happens in the same thread, so I'm not yet
>>>>>> seeing where the race happens?
>>>>>
>>>>> The race is as described in David's earlier email, so let's recap:
>>>>>
>>>>> Thread 1 Thread 2
>>>>> phy_disconnect()
>>>>> phy_stop_interrupts()
>>>>> phy_stop_machine()
>>>>> phy_state_machine()
>>>>> -> queue_delayed_work()
>>>>> phy_detach()
>>>>> phy_state_machine()
>>>>> -> netif_carrier_off()
>>>>>
>>>>> If phy_detach() finishes earlier than the workqueue had a chance to be
>>>>> scheduled and process PHY_HALTED again, then we trigger the NULL
>>>>> pointer
>>>>> de-reference.
>>>>>
>>>>> workqueues are not tasklets, the CPU scheduling them gets no guarantee
>>>>> they will run on the same CPU.
>>>>
>>>> Something does not add up.
>>>>
>>>> The synchronous call to phy_state_machine() does:
>>>>
>>>> case PHY_HALTED:
>>>> if (phydev->link) {
>>>> phydev->link = 0;
>>>> netif_carrier_off(phydev->attached_dev);
>>>> phy_adjust_link(phydev);
>>>> do_suspend = true;
>>>> }
>>>>
>>>> then sets phydev->link = 0; therefore subsequent calls to
>>>> phy_state_machin() will be no-op.
>>>
>>> Actually you are right, once phydev->link is set to 0 these would become
>>> no-ops. Still scratching my head as to what happens for David then...
>>>
>>>>
>>>> Also, queue_delayed_work() is only called in polling mode.
>>>> David stated that he's using interrupt mode.
>>
>> Did you see what I wrote?
>
> Still not following, see below.
>
>>
>> phy_disconnect() calls phy_stop_interrupts() which puts it into polling
>> mode. So the polling work gets queued unconditionally.
>
> What part of phy_stop_interrupts() is responsible for changing
> phydev->irq to PHY_POLL? free_irq() cannot touch phydev->irq otherwise
> subsequent request_irq() calls won't work anymore.
> phy_disable_interrupts() only calls back into the PHY driver to
> acknowledge and clear interrupts.
>
> If we were using a PHY with PHY_POLL, as Marc said, the first
> synchronous call to phy_state_machine() would have acted on PHY_HALTED
> and even if we incorrectly keep re-scheduling the state machine from
> PHY_HALTED to PHY_HALTED the second time around nothing can happen.
>
> What are we missing here?
>
OK, I am now as confused as you guys are. I will go back and get an
ftrace log out of the failure.
David.
^ permalink raw reply
* Re: [PATCH] netfilter: xt_hashlimit: avoid 64-bit division
From: Arnd Bergmann @ 2017-09-06 20:48 UTC (permalink / raw)
To: Vishwanath Pai
Cc: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal,
David S. Miller, Josh Hunt, netfilter-devel, coreteam, Networking,
Linux Kernel Mailing List
In-Reply-To: <7791dee1-64a9-dcaa-c5d9-0b04a44bc526@akamai.com>
On Wed, Sep 6, 2017 at 10:22 PM, Vishwanath Pai <vpai@akamai.com> wrote:
> On 09/06/2017 03:57 PM, Arnd Bergmann wrote:
>> 64-bit division is expensive on 32-bit architectures, and
>> requires a special function call to avoid a link error like:
>>
>> net/netfilter/xt_hashlimit.o: In function `hashlimit_mt_common':
>> xt_hashlimit.c:(.text+0x1328): undefined reference to `__aeabi_uldivmod'
>>
>> In the case of hashlimit_mt_common, we don't actually need a
>> 64-bit operation, we can simply rewrite the function slightly
>> to make that clear to the compiler.
>>
>> Fixes: bea74641e378 ("netfilter: xt_hashlimit: add rate match mode")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>> net/netfilter/xt_hashlimit.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
>> index 10d48234f5f4..50b53d86eef5 100644
>> --- a/net/netfilter/xt_hashlimit.c
>> +++ b/net/netfilter/xt_hashlimit.c
>> @@ -531,7 +531,10 @@ static u64 user2rate_bytes(u64 user)
>> {
>> u64 r;
>>
>> - r = user ? 0xFFFFFFFFULL / user : 0xFFFFFFFFULL;
>> + if (user > 0xFFFFFFFFULL)
>> + return 0;
>> +
>> + r = user ? 0xFFFFFFFFULL / (u32)user : 0xFFFFFFFFULL;
>> r = (r - 1) << 4;
>> return r;
>> }
>>
>
> I have submitted another patch to fix this:
> https://patchwork.ozlabs.org/patch/809881/
>
> We have seen this problem before, I was careful not to introduce this
> again in the new patch but clearly I overlooked this particular line :(
>
> In the other cases we fixed it by replacing division with div64_u64().
div64_u64() seems needlessly expensive here since the dividend
is known to be a 32-bit number. I guess the function is not called
frequently though, so it doesn't matter much.
Arnd
^ permalink raw reply
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