Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial numbers

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Dec 08, 2016 at 12:33:43AM -0800, kys@exchange.microsoft.com wrote:
> From: Haiyang Zhang <haiyangz@microsoft.com>
> 
> We currently use MAC address to match VF and synthetic NICs. Hyper-V
> provides a serial number to both devices for this purpose. This patch
> implements the matching based on VF serial numbers. This is the way
> specified by the protocol and more reliable.
> 
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
>  drivers/net/hyperv/netvsc_drv.c |   55 ++++++++++++++++++++++++++++++++++++---
>  1 files changed, 51 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 9522763..c5778cf 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -1165,9 +1165,10 @@ static void netvsc_free_netdev(struct net_device *netdev)
>  	free_netdev(netdev);
>  }
>  
> -static struct net_device *get_netvsc_bymac(const u8 *mac)
> +static struct net_device *get_netvsc_byvfser(u32 vfser)
>  {
>  	struct net_device *dev;
> +	struct net_device_context *ndev_ctx;
>  
>  	ASSERT_RTNL();
>  
> @@ -1175,7 +1176,8 @@ static void netvsc_free_netdev(struct net_device *netdev)
>  		if (dev->netdev_ops != &device_ops)
>  			continue;	/* not a netvsc device */
>  
> -		if (ether_addr_equal(mac, dev->perm_addr))
> +		ndev_ctx = netdev_priv(dev);
> +		if (ndev_ctx->vf_serial == vfser)
>  			return dev;
>  	}
>  
> @@ -1205,21 +1207,66 @@ static void netvsc_free_netdev(struct net_device *netdev)
>  	return NULL;
>  }
>  
> +static u32 netvsc_get_vfser(struct net_device *vf_netdev)
> +{
> +	struct device *dev;
> +	struct hv_device *hdev;
> +	struct hv_pcibus_device *hbus = NULL;
> +	struct list_head *iter;
> +	struct hv_pci_dev *hpdev;
> +	unsigned long flags;
> +	u32 vfser = 0;
> +	u32 count = 0;
> +
> +	for (dev = &vf_netdev->dev; dev; dev = dev->parent) {

You are going to walk the whole device tree backwards?  That's crazy.
And foolish.  And racy and broken (what happens if the tree changes
while you do this?)  Where is the lock being grabbed while this happens?
What about reference counts?  Do you see other drivers ever doing this
(if you do, point them out and I'll go yell at them too...)

> +		if (!dev_is_vmbus(dev))
> +			continue;

Ick.

Why isn't your parent pointer a vmbus device all the time?  How could
you get burried down in the device hierarchy when you are the driver for
a specific bus type in the first place?  How could this function ever be
called for a device that is NOT of this type?

thanks,

greg k-h