[v2,2/2] hv_netvsc: pair VF based on serial number

Message ID 20180914195457.20433-3-sthemmin@microsoft.com
State Accepted
Delegated to: David Miller
Headers show
Series
  • hv_netvsc: associate VF and PV device by serial number
Related show

Commit Message

Stephen Hemminger Sept. 14, 2018, 7:54 p.m.
Matching network device based on MAC address is problematic
since a non VF network device can be creted with a duplicate MAC
address causing confusion and problems.  The VMBus API does provide
a serial number that is a better matching method.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/hyperv/netvsc.c     |  3 ++
 drivers/net/hyperv/netvsc_drv.c | 58 +++++++++++++++++++--------------
 2 files changed, 36 insertions(+), 25 deletions(-)

Comments

Lorenzo Pieralisi Sept. 20, 2018, 1:43 p.m. | #1
On Fri, Sep 14, 2018 at 12:54:57PM -0700, Stephen Hemminger wrote:
> Matching network device based on MAC address is problematic
> since a non VF network device can be creted with a duplicate MAC
> address causing confusion and problems.  The VMBus API does provide
> a serial number that is a better matching method.
> 
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
>  drivers/net/hyperv/netvsc.c     |  3 ++
>  drivers/net/hyperv/netvsc_drv.c | 58 +++++++++++++++++++--------------
>  2 files changed, 36 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index 31c3d77b4733..fe01e141c8f8 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -1203,6 +1203,9 @@ static void netvsc_send_vf(struct net_device *ndev,
>  
>  	net_device_ctx->vf_alloc = nvmsg->msg.v4_msg.vf_assoc.allocated;
>  	net_device_ctx->vf_serial = nvmsg->msg.v4_msg.vf_assoc.serial;
> +	netdev_info(ndev, "VF slot %u %s\n",
> +		    net_device_ctx->vf_serial,
> +		    net_device_ctx->vf_alloc ? "added" : "removed");
>  }
>  
>  static  void netvsc_receive_inband(struct net_device *ndev,
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 1121a1ec407c..9dedc1463e88 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -1894,20 +1894,6 @@ static void netvsc_link_change(struct work_struct *w)
>  	rtnl_unlock();
>  }
>  
> -static struct net_device *get_netvsc_bymac(const u8 *mac)
> -{
> -	struct net_device_context *ndev_ctx;
> -
> -	list_for_each_entry(ndev_ctx, &netvsc_dev_list, list) {
> -		struct net_device *dev = hv_get_drvdata(ndev_ctx->device_ctx);
> -
> -		if (ether_addr_equal(mac, dev->perm_addr))
> -			return dev;
> -	}
> -
> -	return NULL;
> -}
> -
>  static struct net_device *get_netvsc_byref(struct net_device *vf_netdev)
>  {
>  	struct net_device_context *net_device_ctx;
> @@ -2036,26 +2022,48 @@ static void netvsc_vf_setup(struct work_struct *w)
>  	rtnl_unlock();
>  }
>  
> +/* Find netvsc by VMBus serial number.
> + * The PCI hyperv controller records the serial number as the slot.
> + */
> +static struct net_device *get_netvsc_byslot(const struct net_device *vf_netdev)
> +{
> +	struct device *parent = vf_netdev->dev.parent;
> +	struct net_device_context *ndev_ctx;
> +	struct pci_dev *pdev;
> +
> +	if (!parent || !dev_is_pci(parent))
> +		return NULL; /* not a PCI device */
> +
> +	pdev = to_pci_dev(parent);
> +	if (!pdev->slot) {
> +		netdev_notice(vf_netdev, "no PCI slot information\n");
> +		return NULL;
> +	}
> +
> +	list_for_each_entry(ndev_ctx, &netvsc_dev_list, list) {
> +		if (!ndev_ctx->vf_alloc)
> +			continue;
> +
> +		if (ndev_ctx->vf_serial == pdev->slot->number)
> +			return hv_get_drvdata(ndev_ctx->device_ctx);

In patch 1, pdev->slot->number is set to:

PCI_SLOT(wslot_to_devfn(hpdev->desc.win_slot.slot))

so I assume vf_serial is initialized (I have no knowledge of VMBUS and
hyper-V internals) to that value, somehow.

I also do not know how the wslot stuff is handled but I assume 5 bits
(ie dev bits in devfn) are enough.

BTW, I have noticed this patch (and patch 1) are already in -next so I will
drop them from the PCI patch queue.

Lorenzo

> +	}
> +
> +	netdev_notice(vf_netdev,
> +		      "no netdev found for slot %u\n", pdev->slot->number);
> +	return NULL;
> +}
> +
>  static int netvsc_register_vf(struct net_device *vf_netdev)
>  {
> -	struct net_device *ndev;
>  	struct net_device_context *net_device_ctx;
> -	struct device *pdev = vf_netdev->dev.parent;
>  	struct netvsc_device *netvsc_dev;
> +	struct net_device *ndev;
>  	int ret;
>  
>  	if (vf_netdev->addr_len != ETH_ALEN)
>  		return NOTIFY_DONE;
>  
> -	if (!pdev || !dev_is_pci(pdev) || dev_is_pf(pdev))
> -		return NOTIFY_DONE;
> -
> -	/*
> -	 * We will use the MAC address to locate the synthetic interface to
> -	 * associate with the VF interface. If we don't find a matching
> -	 * synthetic interface, move on.
> -	 */
> -	ndev = get_netvsc_bymac(vf_netdev->perm_addr);
> +	ndev = get_netvsc_byslot(vf_netdev);
>  	if (!ndev)
>  		return NOTIFY_DONE;
>  
> -- 
> 2.18.0
>

Patch

diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 31c3d77b4733..fe01e141c8f8 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -1203,6 +1203,9 @@  static void netvsc_send_vf(struct net_device *ndev,
 
 	net_device_ctx->vf_alloc = nvmsg->msg.v4_msg.vf_assoc.allocated;
 	net_device_ctx->vf_serial = nvmsg->msg.v4_msg.vf_assoc.serial;
+	netdev_info(ndev, "VF slot %u %s\n",
+		    net_device_ctx->vf_serial,
+		    net_device_ctx->vf_alloc ? "added" : "removed");
 }
 
 static  void netvsc_receive_inband(struct net_device *ndev,
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 1121a1ec407c..9dedc1463e88 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1894,20 +1894,6 @@  static void netvsc_link_change(struct work_struct *w)
 	rtnl_unlock();
 }
 
-static struct net_device *get_netvsc_bymac(const u8 *mac)
-{
-	struct net_device_context *ndev_ctx;
-
-	list_for_each_entry(ndev_ctx, &netvsc_dev_list, list) {
-		struct net_device *dev = hv_get_drvdata(ndev_ctx->device_ctx);
-
-		if (ether_addr_equal(mac, dev->perm_addr))
-			return dev;
-	}
-
-	return NULL;
-}
-
 static struct net_device *get_netvsc_byref(struct net_device *vf_netdev)
 {
 	struct net_device_context *net_device_ctx;
@@ -2036,26 +2022,48 @@  static void netvsc_vf_setup(struct work_struct *w)
 	rtnl_unlock();
 }
 
+/* Find netvsc by VMBus serial number.
+ * The PCI hyperv controller records the serial number as the slot.
+ */
+static struct net_device *get_netvsc_byslot(const struct net_device *vf_netdev)
+{
+	struct device *parent = vf_netdev->dev.parent;
+	struct net_device_context *ndev_ctx;
+	struct pci_dev *pdev;
+
+	if (!parent || !dev_is_pci(parent))
+		return NULL; /* not a PCI device */
+
+	pdev = to_pci_dev(parent);
+	if (!pdev->slot) {
+		netdev_notice(vf_netdev, "no PCI slot information\n");
+		return NULL;
+	}
+
+	list_for_each_entry(ndev_ctx, &netvsc_dev_list, list) {
+		if (!ndev_ctx->vf_alloc)
+			continue;
+
+		if (ndev_ctx->vf_serial == pdev->slot->number)
+			return hv_get_drvdata(ndev_ctx->device_ctx);
+	}
+
+	netdev_notice(vf_netdev,
+		      "no netdev found for slot %u\n", pdev->slot->number);
+	return NULL;
+}
+
 static int netvsc_register_vf(struct net_device *vf_netdev)
 {
-	struct net_device *ndev;
 	struct net_device_context *net_device_ctx;
-	struct device *pdev = vf_netdev->dev.parent;
 	struct netvsc_device *netvsc_dev;
+	struct net_device *ndev;
 	int ret;
 
 	if (vf_netdev->addr_len != ETH_ALEN)
 		return NOTIFY_DONE;
 
-	if (!pdev || !dev_is_pci(pdev) || dev_is_pf(pdev))
-		return NOTIFY_DONE;
-
-	/*
-	 * We will use the MAC address to locate the synthetic interface to
-	 * associate with the VF interface. If we don't find a matching
-	 * synthetic interface, move on.
-	 */
-	ndev = get_netvsc_bymac(vf_netdev->perm_addr);
+	ndev = get_netvsc_byslot(vf_netdev);
 	if (!ndev)
 		return NOTIFY_DONE;