[PATCH net 0/9] hv_netvsc: minor bug fixes

[PATCH net 0/9] hv_netvsc: minor bug fixes

[Stephen Hemminger]

These are improvements to netvsc driver. They aren't functionality
changes so not targeting net-next; and they are not show stopper
bugs that need to go to stable either.

Stephen Hemminger (9):
  hv_netvsc: avoid retry on send during shutdown
  hv_netvsc: only wake transmit queue if link is up
  hv_netvsc: fix error unwind handling if vmbus_open fails
  hv_netvsc: cancel subchannel setup before halting device
  hv_netvsc: fix race in napi poll when rescheduling
  hv_netvsc: use napi_schedule_irqoff
  hv_netvsc: don't need full irqsave for request_lock
  hv_netvsc: propagate rx filters to VF
  hv_netvsc: defer queue selection to VF

 drivers/net/hyperv/hyperv_net.h   |  4 +--
 drivers/net/hyperv/netvsc.c       | 33 +++++++++--------------
 drivers/net/hyperv/netvsc_drv.c   | 57 +++++++++++++++++++++++++++++++--------
 drivers/net/hyperv/rndis_filter.c | 54 +++++++++++++++----------------------
 4 files changed, 80 insertions(+), 68 deletions(-)

-- 
2.16.1

[PATCH net 1/9] hv_netvsc: avoid retry on send during shutdown

[Stephen Hemminger]

Change the initialization order so that the device is ready to transmit
(ie connect vsp is completed) before setting the internal reference
to the device with RCU.

This avoids any races on initialization and prevents retry issues
on shutdown.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/hyperv/netvsc.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 17e529af79dc..686900d61374 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -852,13 +852,6 @@ int netvsc_send(struct net_device *ndev,
 	if (unlikely(!net_device || net_device->destroy))
 		return -ENODEV;
 
-	/* We may race with netvsc_connect_vsp()/netvsc_init_buf() and get
-	 * here before the negotiation with the host is finished and
-	 * send_section_map may not be allocated yet.
-	 */
-	if (unlikely(!net_device->send_section_map))
-		return -EAGAIN;
-
 	nvchan = &net_device->chan_table[packet->q_idx];
 	packet->send_buf_index = NETVSC_INVALID_INDEX;
 	packet->cp_partial = false;
@@ -866,10 +859,8 @@ int netvsc_send(struct net_device *ndev,
 	/* Send control message directly without accessing msd (Multi-Send
 	 * Data) field which may be changed during data packet processing.
 	 */
-	if (!skb) {
-		cur_send = packet;
-		goto send_now;
-	}
+	if (!skb)
+		return netvsc_send_pkt(device, packet, net_device, pb, skb);
 
 	/* batch packets in send buffer if possible */
 	msdp = &nvchan->msd;
@@ -953,7 +944,6 @@ int netvsc_send(struct net_device *ndev,
 		}
 	}
 
-send_now:
 	if (cur_send)
 		ret = netvsc_send_pkt(device, cur_send, net_device, pb, skb);
 
@@ -1306,11 +1296,6 @@ struct netvsc_device *netvsc_device_add(struct hv_device *device,
 
 	napi_enable(&net_device->chan_table[0].napi);
 
-	/* Writing nvdev pointer unlocks netvsc_send(), make sure chn_table is
-	 * populated.
-	 */
-	rcu_assign_pointer(net_device_ctx->nvdev, net_device);
-
 	/* Connect with the NetVsp */
 	ret = netvsc_connect_vsp(device, net_device, device_info);
 	if (ret != 0) {
@@ -1319,6 +1304,11 @@ struct netvsc_device *netvsc_device_add(struct hv_device *device,
 		goto close;
 	}
 
+	/* Writing nvdev pointer unlocks netvsc_send(), make sure chn_table is
+	 * populated.
+	 */
+	rcu_assign_pointer(net_device_ctx->nvdev, net_device);
+
 	return net_device;
 
 close:
-- 
2.16.1

[PATCH net 2/9] hv_netvsc: only wake transmit queue if link is up

[Stephen Hemminger]

Don't wake transmit queues if link is not up yet.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/hyperv/netvsc_drv.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index c5584c2d440e..fa6cf18e7719 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -91,12 +91,11 @@ static int netvsc_open(struct net_device *net)
 		return ret;
 	}
 
-	netif_tx_wake_all_queues(net);
-
 	rdev = nvdev->extension;
-
-	if (!rdev->link_state)
+	if (!rdev->link_state) {
 		netif_carrier_on(net);
+		netif_tx_wake_all_queues(net);
+	}
 
 	if (vf_netdev) {
 		/* Setting synthetic device up transparently sets
-- 
2.16.1

[PATCH net 3/9] hv_netvsc: fix error unwind handling if vmbus_open fails

[Stephen Hemminger]

Need to delete NAPI association if vmbus_open fails.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/hyperv/netvsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 686900d61374..ff97a85b2e9d 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -1286,7 +1286,6 @@ struct netvsc_device *netvsc_device_add(struct hv_device *device,
 			 netvsc_channel_cb, net_device->chan_table);
 
 	if (ret != 0) {
-		netif_napi_del(&net_device->chan_table[0].napi);
 		netdev_err(ndev, "unable to open channel: %d\n", ret);
 		goto cleanup;
 	}
@@ -1319,6 +1318,7 @@ struct netvsc_device *netvsc_device_add(struct hv_device *device,
 	vmbus_close(device->channel);
 
 cleanup:
+	netif_napi_del(&net_device->chan_table[0].napi);
 	free_netvsc_device(&net_device->rcu);
 
 	return ERR_PTR(ret);
-- 
2.16.1

[PATCH net 4/9] hv_netvsc: cancel subchannel setup before halting device

[Stephen Hemminger]

Block setup of multiple channels earlier in the teardown
process. This avoids possible races between halt and subchannel
initialization.

Suggested-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/hyperv/rndis_filter.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index c3ca191fea7f..1cba767c6453 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -1340,6 +1340,9 @@ void rndis_filter_device_remove(struct hv_device *dev,
 {
 	struct rndis_device *rndis_dev = net_dev->extension;
 
+	/* Don't try and setup sub channels if about to halt */
+	cancel_work_sync(&net_dev->subchan_work);
+
 	/* Halt and release the rndis device */
 	rndis_filter_halt_device(rndis_dev);
 
-- 
2.16.1

[PATCH net 5/9] hv_netvsc: fix race in napi poll when rescheduling

[Stephen Hemminger]

There is a race between napi_reschedule and re-enabling interrupts
which could lead to missed host interrrupts.  This occurs when
interrupts are re-enabled (hv_end_read) and vmbus irq callback
(netvsc_channel_cb) has already scheduled NAPI.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/hyperv/netvsc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index ff97a85b2e9d..4237cedc4f08 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -1207,9 +1207,10 @@ int netvsc_poll(struct napi_struct *napi, int budget)
 	if (send_recv_completions(ndev, net_device, nvchan) == 0 &&
 	    work_done < budget &&
 	    napi_complete_done(napi, work_done) &&
-	    hv_end_read(&channel->inbound)) {
+	    hv_end_read(&channel->inbound) &&
+	    napi_schedule_prep(napi)) {
 		hv_begin_read(&channel->inbound);
-		napi_reschedule(napi);
+		__napi_schedule(napi);
 	}
 
 	/* Driver may overshoot since multiple packets per descriptor */
-- 
2.16.1

[PATCH net 6/9] hv_netvsc: use napi_schedule_irqoff

[Stephen Hemminger]

Since the netvsc_channel_cb is already called in interrupt
context from vmbus, there is no need to do irqsave/restore.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/hyperv/netvsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 4237cedc4f08..0265d703eb03 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -1233,7 +1233,7 @@ void netvsc_channel_cb(void *context)
 		/* disable interupts from host */
 		hv_begin_read(rbi);
 
-		__napi_schedule(&nvchan->napi);
+		__napi_schedule_irqoff(&nvchan->napi);
 	}
 }
 
-- 
2.16.1

[PATCH net 7/9] hv_netvsc: don't need full irqsave for request_lock

[Stephen Hemminger]

Since the driver incoming handling is done with NAPI.
This path is run in softirq (not hardirq) therefore don't
need to do full local irq save/restore.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/hyperv/rndis_filter.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index 1cba767c6453..3ce11361634f 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -92,7 +92,6 @@ static struct rndis_request *get_rndis_request(struct rndis_device *dev,
 	struct rndis_request *request;
 	struct rndis_message *rndis_msg;
 	struct rndis_set_request *set;
-	unsigned long flags;
 
 	request = kzalloc(sizeof(struct rndis_request), GFP_KERNEL);
 	if (!request)
@@ -115,9 +114,9 @@ static struct rndis_request *get_rndis_request(struct rndis_device *dev,
 	set->req_id = atomic_inc_return(&dev->new_req_id);
 
 	/* Add to the request list */
-	spin_lock_irqsave(&dev->request_lock, flags);
+	spin_lock_bh(&dev->request_lock);
 	list_add_tail(&request->list_ent, &dev->req_list);
-	spin_unlock_irqrestore(&dev->request_lock, flags);
+	spin_unlock_bh(&dev->request_lock);
 
 	return request;
 }
@@ -125,11 +124,9 @@ static struct rndis_request *get_rndis_request(struct rndis_device *dev,
 static void put_rndis_request(struct rndis_device *dev,
 			    struct rndis_request *req)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&dev->request_lock, flags);
+	spin_lock_bh(&dev->request_lock);
 	list_del(&req->list_ent);
-	spin_unlock_irqrestore(&dev->request_lock, flags);
+	spin_unlock_bh(&dev->request_lock);
 
 	kfree(req);
 }
@@ -269,10 +266,9 @@ static void rndis_filter_receive_response(struct rndis_device *dev,
 {
 	struct rndis_request *request = NULL;
 	bool found = false;
-	unsigned long flags;
 	struct net_device *ndev = dev->ndev;
 
-	spin_lock_irqsave(&dev->request_lock, flags);
+	spin_lock_bh(&dev->request_lock);
 	list_for_each_entry(request, &dev->req_list, list_ent) {
 		/*
 		 * All request/response message contains RequestId as the 1st
@@ -284,7 +280,7 @@ static void rndis_filter_receive_response(struct rndis_device *dev,
 			break;
 		}
 	}
-	spin_unlock_irqrestore(&dev->request_lock, flags);
+	spin_unlock_bh(&dev->request_lock);
 
 	if (found) {
 		if (resp->msg_len <=
-- 
2.16.1

[PATCH net 8/9] hv_netvsc: propagate rx filters to VF

[Stephen Hemminger]

The netvsc device should propagate filters to the SR-IOV VF
device (if present). The flags also need to be propagated to the
VF device as well. This only really matters on local Hyper-V
since Azure does not support multiple addresses.

The rx filter management in netvsc device does not need to be done
in a work queue since it is called with RTNL held.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/hyperv/hyperv_net.h   |  4 +---
 drivers/net/hyperv/netvsc_drv.c   | 35 ++++++++++++++++++++++++++++++-----
 drivers/net/hyperv/rndis_filter.c | 35 ++++++++++++-----------------------
 3 files changed, 43 insertions(+), 31 deletions(-)

diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 0db3bd1ea06f..a4e1ebba84cf 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -172,8 +172,6 @@ struct rndis_device {
 	spinlock_t request_lock;
 	struct list_head req_list;
 
-	struct work_struct mcast_work;
-
 	bool link_state;        /* 0 - link up, 1 - link down */
 
 	u8 hw_mac_adr[ETH_ALEN];
@@ -216,7 +214,7 @@ int rndis_filter_open(struct netvsc_device *nvdev);
 int rndis_filter_close(struct netvsc_device *nvdev);
 struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,
 					      struct netvsc_device_info *info);
-void rndis_filter_update(struct netvsc_device *nvdev);
+void rndis_filter_update(struct netvsc_device *nvdev, unsigned int flags);
 void rndis_filter_device_remove(struct hv_device *dev,
 				struct netvsc_device *nvdev);
 int rndis_filter_set_rss_param(struct rndis_device *rdev,
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index fa6cf18e7719..1909c58e0a7e 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -66,12 +66,36 @@ static int debug = -1;
 module_param(debug, int, S_IRUGO);
 MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
 
-static void netvsc_set_multicast_list(struct net_device *net)
+static void netvsc_change_rx_flags(struct net_device *net, int change)
 {
-	struct net_device_context *net_device_ctx = netdev_priv(net);
-	struct netvsc_device *nvdev = rtnl_dereference(net_device_ctx->nvdev);
+	struct net_device_context *ndev_ctx = netdev_priv(net);
+	struct net_device *vf_netdev = rtnl_dereference(ndev_ctx->vf_netdev);
+	struct netvsc_device *nvdev = rtnl_dereference(ndev_ctx->nvdev);
+
+	if (vf_netdev) {
+		if (change & IFF_PROMISC)
+			dev_set_promiscuity(net,
+				    (net->flags & IFF_PROMISC) ? 1 : -1);
+
+		if (change & IFF_ALLMULTI)
+			dev_set_allmulti(net,
+				 (net->flags & IFF_ALLMULTI) ? 1 : -1);
+	}
 
-	rndis_filter_update(nvdev);
+	if (change & (IFF_PROMISC | IFF_ALLMULTI))
+		rndis_filter_update(nvdev, net->flags);
+}
+
+/* Hyper-V vswitch does not support filtering but VF does */
+static void netvsc_set_rx_mode(struct net_device *net)
+{
+	struct net_device_context *ndev_ctx = netdev_priv(net);
+	struct net_device *vf_netdev = rtnl_dereference(ndev_ctx->vf_netdev);
+
+	if (vf_netdev) {
+		dev_uc_sync(vf_netdev, net);
+		dev_mc_sync(vf_netdev, net);
+	}
 }
 
 static int netvsc_open(struct net_device *net)
@@ -1575,7 +1599,8 @@ static const struct net_device_ops device_ops = {
 	.ndo_open =			netvsc_open,
 	.ndo_stop =			netvsc_close,
 	.ndo_start_xmit =		netvsc_start_xmit,
-	.ndo_set_rx_mode =		netvsc_set_multicast_list,
+	.ndo_change_rx_flags =		netvsc_change_rx_flags,
+	.ndo_set_rx_mode =		netvsc_set_rx_mode,
 	.ndo_change_mtu =		netvsc_change_mtu,
 	.ndo_validate_addr =		eth_validate_addr,
 	.ndo_set_mac_address =		netvsc_set_mac_addr,
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index 3ce11361634f..199ec6da6bde 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -32,8 +32,6 @@
 
 #include "hyperv_net.h"
 
-static void rndis_set_multicast(struct work_struct *w);
-
 #define RNDIS_EXT_LEN PAGE_SIZE
 struct rndis_request {
 	struct list_head list_ent;
@@ -78,7 +76,6 @@ static struct rndis_device *get_rndis_device(void)
 	spin_lock_init(&device->request_lock);
 
 	INIT_LIST_HEAD(&device->req_list);
-	INIT_WORK(&device->mcast_work, rndis_set_multicast);
 
 	device->state = RNDIS_DEV_UNINITIALIZED;
 
@@ -846,26 +843,21 @@ static int rndis_filter_set_packet_filter(struct rndis_device *dev,
 	return ret;
 }
 
-static void rndis_set_multicast(struct work_struct *w)
+void rndis_filter_update(struct netvsc_device *nvdev,
+			 unsigned int flags)
 {
-	struct rndis_device *rdev
-		= container_of(w, struct rndis_device, mcast_work);
+	u32 filter = NDIS_PACKET_TYPE_DIRECTED;
 
-	if (rdev->ndev->flags & IFF_PROMISC)
-		rndis_filter_set_packet_filter(rdev,
-					       NDIS_PACKET_TYPE_PROMISCUOUS);
-	else
-		rndis_filter_set_packet_filter(rdev,
-					       NDIS_PACKET_TYPE_BROADCAST |
-					       NDIS_PACKET_TYPE_ALL_MULTICAST |
-					       NDIS_PACKET_TYPE_DIRECTED);
-}
-
-void rndis_filter_update(struct netvsc_device *nvdev)
-{
-	struct rndis_device *rdev = nvdev->extension;
+	if (flags & IFF_PROMISC) {
+		filter = NDIS_PACKET_TYPE_PROMISCUOUS;
+	} else {
+		if (flags & IFF_ALLMULTI)
+			flags |= NDIS_PACKET_TYPE_ALL_MULTICAST;
+		if (flags & IFF_BROADCAST)
+			flags |= NDIS_PACKET_TYPE_BROADCAST;
+	}
 
-	schedule_work(&rdev->mcast_work);
+	rndis_filter_set_packet_filter(nvdev->extension, filter);
 }
 
 static int rndis_filter_init_device(struct rndis_device *dev,
@@ -995,9 +987,6 @@ static int rndis_filter_close_device(struct rndis_device *dev)
 	if (dev->state != RNDIS_DEV_DATAINITIALIZED)
 		return 0;
 
-	/* Make sure rndis_set_multicast doesn't re-enable filter! */
-	cancel_work_sync(&dev->mcast_work);
-
 	ret = rndis_filter_set_packet_filter(dev, 0);
 	if (ret == -ENODEV)
 		ret = 0;
-- 
2.16.1

[PATCH net 9/9] hv_netvsc: defer queue selection to VF

[Stephen Hemminger]

When VF is used for accelerated networking it will likely have
more queues (and different policy) than the synthetic NIC.
This patch defers the queue policy to the VF so that all the
queues can be used. This impacts workloads like local generate UDP.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/hyperv/netvsc_drv.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 1909c58e0a7e..e36f3ae6893b 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -322,8 +322,19 @@ static u16 netvsc_select_queue(struct net_device *ndev, struct sk_buff *skb,
 	rcu_read_lock();
 	vf_netdev = rcu_dereference(ndc->vf_netdev);
 	if (vf_netdev) {
-		txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0;
-		qdisc_skb_cb(skb)->slave_dev_queue_mapping = skb->queue_mapping;
+		const struct net_device_ops *vf_ops = vf_netdev->netdev_ops;
+
+		if (vf_ops->ndo_select_queue)
+			txq = vf_ops->ndo_select_queue(vf_netdev, skb,
+						       accel_priv, fallback);
+		else
+			txq = fallback(vf_netdev, skb);
+
+		/* Record the queue selected by VF so that it can be
+		 * used for common case where VF has more queues than
+		 * the synthetic device.
+		 */
+		qdisc_skb_cb(skb)->slave_dev_queue_mapping = txq;
 	} else {
 		txq = netvsc_pick_tx(ndev, skb);
 	}
-- 
2.16.1

Re: [PATCH net 8/9] hv_netvsc: propagate rx filters to VF

[Jakub Kicinski]

On Thu,  1 Mar 2018 10:27:55 -0800, Stephen Hemminger wrote:
> The netvsc device should propagate filters to the SR-IOV VF
> device (if present). The flags also need to be propagated to the
> VF device as well. This only really matters on local Hyper-V
> since Azure does not support multiple addresses.
> 
> The rx filter management in netvsc device does not need to be done
> in a work queue since it is called with RTNL held.
> 
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>

Do you also propagate them when VF gets unplugged and plugged back in?

Re: [PATCH net 8/9] hv_netvsc: propagate rx filters to VF

[Stephen Hemminger]

On Thu,  1 Mar 2018 10:27:55 -0800
Stephen Hemminger <stephen@networkplumber.org> wrote:

> +		if (change & IFF_PROMISC)
> +			dev_set_promiscuity(net,
> +				    (net->flags & IFF_PROMISC) ? 1 : -1);

This should be vf_netdev here.