| Message ID | 1392252867-21146-1-git-send-email-haiyangz@microsoft.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On 02/13/2014 08:54 AM, Haiyang Zhang wrote: > Without this patch, the "cat /sys/class/net/ethN/operstate" shows > "unknown", and "ethtool ethN" shows "Link detected: yes", when VM > boots up with or without vNIC connected. > > This patch fixed the problem. > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> > Reviewed-by: K. Y. Srinivasan <kys@microsoft.com> > --- > drivers/net/hyperv/netvsc_drv.c | 53 ++++++++++++++++++++++++++++----------- > 1 files changed, 38 insertions(+), 15 deletions(-) > > diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c > index 7756118..7141a19 100644 > --- a/drivers/net/hyperv/netvsc_drv.c > +++ b/drivers/net/hyperv/netvsc_drv.c > @@ -88,8 +88,12 @@ static int netvsc_open(struct net_device *net) > { > struct net_device_context *net_device_ctx = netdev_priv(net); > struct hv_device *device_obj = net_device_ctx->device_ctx; > + struct netvsc_device *nvdev; > + struct rndis_device *rdev; > int ret = 0; > > + netif_carrier_off(net); > + > /* Open up the device */ > ret = rndis_filter_open(device_obj); > if (ret != 0) { > @@ -99,6 +103,11 @@ static int netvsc_open(struct net_device *net) > > netif_start_queue(net); > > + nvdev = hv_get_drvdata(device_obj); > + rdev = nvdev->extension; > + if (!rdev->link_state) > + netif_carrier_on(net); > + Maybe you can just schedule the work here and then you can drop the rtnl_lock in netvsc_link_change() ? > return ret; > } > > @@ -229,23 +238,24 @@ void netvsc_linkstatus_callback(struct hv_device *device_obj, > struct net_device *net; > struct net_device_context *ndev_ctx; > struct netvsc_device *net_device; > + struct rndis_device *rdev; > > net_device = hv_get_drvdata(device_obj); > + rdev = net_device->extension; > + > + rdev->link_state = status != 1; > + > net = net_device->ndev; > > - if (!net) { > - netdev_err(net, "got link status but net device " > - "not initialized yet\n"); > + if (!net || net->reg_state != NETREG_REGISTERED) > return; > - } > > + ndev_ctx = netdev_priv(net); > if (status == 1) { > - netif_carrier_on(net); > - ndev_ctx = netdev_priv(net); > schedule_delayed_work(&ndev_ctx->dwork, 0); > schedule_delayed_work(&ndev_ctx->dwork, msecs_to_jiffies(20)); > } else { > - netif_carrier_off(net); > + schedule_delayed_work(&ndev_ctx->dwork, 0); > } > } > > @@ -388,17 +398,35 @@ static const struct net_device_ops device_ops = { > * current context when receiving RNDIS_STATUS_MEDIA_CONNECT event. So, add > * another netif_notify_peers() into a delayed work, otherwise GARP packet > * will not be sent after quick migration, and cause network disconnection. > + * Also, we update the carrier status here. > */ > -static void netvsc_send_garp(struct work_struct *w) > +static void netvsc_link_change(struct work_struct *w) > { > struct net_device_context *ndev_ctx; > struct net_device *net; > struct netvsc_device *net_device; > + struct rndis_device *rdev; > + bool notify; > + > + rtnl_lock(); > > ndev_ctx = container_of(w, struct net_device_context, dwork.work); > net_device = hv_get_drvdata(ndev_ctx->device_ctx); > + rdev = net_device->extension; > net = net_device->ndev; > - netdev_notify_peers(net); > + > + if (rdev->link_state) { > + netif_carrier_off(net); > + notify = false; > + } else { > + netif_carrier_on(net); > + notify = true; > + } > + > + rtnl_unlock(); > + > + if (notify) > + netdev_notify_peers(net); > } > Looks like this forces arp_notify here. Is it expected? Other looks good. > > @@ -414,13 +442,10 @@ static int netvsc_probe(struct hv_device *dev, > if (!net) > return -ENOMEM; > > - /* Set initial state */ > - netif_carrier_off(net); > - > net_device_ctx = netdev_priv(net); > net_device_ctx->device_ctx = dev; > hv_set_drvdata(dev, net); > - INIT_DELAYED_WORK(&net_device_ctx->dwork, netvsc_send_garp); > + INIT_DELAYED_WORK(&net_device_ctx->dwork, netvsc_link_change); > INIT_WORK(&net_device_ctx->work, do_set_multicast); > > net->netdev_ops = &device_ops; > @@ -443,8 +468,6 @@ static int netvsc_probe(struct hv_device *dev, > } > memcpy(net->dev_addr, device_info.mac_adr, ETH_ALEN); > > - netif_carrier_on(net); > - > ret = register_netdev(net); > if (ret != 0) { > pr_err("Unable to register netdev.\n"); -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Jason Wang [mailto:jasowang@redhat.com] > Sent: Wednesday, February 12, 2014 10:52 PM > To: Haiyang Zhang; davem@davemloft.net; netdev@vger.kernel.org > Cc: KY Srinivasan; olaf@aepfle.de; linux-kernel@vger.kernel.org; driverdev- > devel@linuxdriverproject.org > Subject: Re: [PATCH net,v3] hyperv: Fix the carrier status setting > > On 02/13/2014 08:54 AM, Haiyang Zhang wrote: > > Without this patch, the "cat /sys/class/net/ethN/operstate" shows > > "unknown", and "ethtool ethN" shows "Link detected: yes", when VM > > boots up with or without vNIC connected. > > > > This patch fixed the problem. > > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> > > Reviewed-by: K. Y. Srinivasan <kys@microsoft.com> > > --- > > drivers/net/hyperv/netvsc_drv.c | 53 > ++++++++++++++++++++++++++++----------- > > 1 files changed, 38 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/net/hyperv/netvsc_drv.c > > b/drivers/net/hyperv/netvsc_drv.c index 7756118..7141a19 100644 > > --- a/drivers/net/hyperv/netvsc_drv.c > > +++ b/drivers/net/hyperv/netvsc_drv.c > > @@ -88,8 +88,12 @@ static int netvsc_open(struct net_device *net) { > > struct net_device_context *net_device_ctx = netdev_priv(net); > > struct hv_device *device_obj = net_device_ctx->device_ctx; > > + struct netvsc_device *nvdev; > > + struct rndis_device *rdev; > > int ret = 0; > > > > + netif_carrier_off(net); > > + > > /* Open up the device */ > > ret = rndis_filter_open(device_obj); > > if (ret != 0) { > > @@ -99,6 +103,11 @@ static int netvsc_open(struct net_device *net) > > > > netif_start_queue(net); > > > > + nvdev = hv_get_drvdata(device_obj); > > + rdev = nvdev->extension; > > + if (!rdev->link_state) > > + netif_carrier_on(net); > > + > > Maybe you can just schedule the work here and then you can drop the > rtnl_lock in netvsc_link_change() ? The rtnl_lock will still be necessary in the netvsc_link_change(), because we want to prevent it getting wrong rdev pointer when netvsc_change_mtu is removing/adding rndis device. > > + > > + if (notify) > > + netdev_notify_peers(net); > > } > > > > Looks like this forces arp_notify here. Is it expected? Yes, this is expected. It's required after live migration. Thanks, - Haiyang -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/13/2014 11:04 PM, Haiyang Zhang wrote: > >> -----Original Message----- >> From: Jason Wang [mailto:jasowang@redhat.com] >> Sent: Wednesday, February 12, 2014 10:52 PM >> To: Haiyang Zhang; davem@davemloft.net; netdev@vger.kernel.org >> Cc: KY Srinivasan; olaf@aepfle.de; linux-kernel@vger.kernel.org; driverdev- >> devel@linuxdriverproject.org >> Subject: Re: [PATCH net,v3] hyperv: Fix the carrier status setting >> >> On 02/13/2014 08:54 AM, Haiyang Zhang wrote: >>> Without this patch, the "cat /sys/class/net/ethN/operstate" shows >>> "unknown", and "ethtool ethN" shows "Link detected: yes", when VM >>> boots up with or without vNIC connected. >>> >>> This patch fixed the problem. >>> >>> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> >>> Reviewed-by: K. Y. Srinivasan <kys@microsoft.com> >>> --- >>> drivers/net/hyperv/netvsc_drv.c | 53 >> ++++++++++++++++++++++++++++----------- >>> 1 files changed, 38 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/net/hyperv/netvsc_drv.c >>> b/drivers/net/hyperv/netvsc_drv.c index 7756118..7141a19 100644 >>> --- a/drivers/net/hyperv/netvsc_drv.c >>> +++ b/drivers/net/hyperv/netvsc_drv.c >>> @@ -88,8 +88,12 @@ static int netvsc_open(struct net_device *net) { >>> struct net_device_context *net_device_ctx = netdev_priv(net); >>> struct hv_device *device_obj = net_device_ctx->device_ctx; >>> + struct netvsc_device *nvdev; >>> + struct rndis_device *rdev; >>> int ret = 0; >>> >>> + netif_carrier_off(net); >>> + >>> /* Open up the device */ >>> ret = rndis_filter_open(device_obj); >>> if (ret != 0) { >>> @@ -99,6 +103,11 @@ static int netvsc_open(struct net_device *net) >>> >>> netif_start_queue(net); >>> >>> + nvdev = hv_get_drvdata(device_obj); >>> + rdev = nvdev->extension; >>> + if (!rdev->link_state) >>> + netif_carrier_on(net); >>> + >> Maybe you can just schedule the work here and then you can drop the >> rtnl_lock in netvsc_link_change() ? > The rtnl_lock will still be necessary in the netvsc_link_change(), because > we want to prevent it getting wrong rdev pointer when netvsc_change_mtu > is removing/adding rndis device. Ok. >>> + >>> + if (notify) >>> + netdev_notify_peers(net); >>> } >>> >> Looks like this forces arp_notify here. Is it expected? > Yes, this is expected. It's required after live migration. > > Thanks, > - Haiyang Yes, this does not change the current behaviour. (arp_notify is meaningless for netvsc). Acked-by: Jason Wang <jasowang@redhat.com> The patch is also needed for stable. Thanks > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Haiyang Zhang <haiyangz@microsoft.com> Date: Wed, 12 Feb 2014 16:54:27 -0800 > Without this patch, the "cat /sys/class/net/ethN/operstate" shows > "unknown", and "ethtool ethN" shows "Link detected: yes", when VM > boots up with or without vNIC connected. > > This patch fixed the problem. > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> > Reviewed-by: K. Y. Srinivasan <kys@microsoft.com> Applied and queued up for -stable, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c index 7756118..7141a19 100644 --- a/drivers/net/hyperv/netvsc_drv.c +++ b/drivers/net/hyperv/netvsc_drv.c @@ -88,8 +88,12 @@ static int netvsc_open(struct net_device *net) { struct net_device_context *net_device_ctx = netdev_priv(net); struct hv_device *device_obj = net_device_ctx->device_ctx; + struct netvsc_device *nvdev; + struct rndis_device *rdev; int ret = 0; + netif_carrier_off(net); + /* Open up the device */ ret = rndis_filter_open(device_obj); if (ret != 0) { @@ -99,6 +103,11 @@ static int netvsc_open(struct net_device *net) netif_start_queue(net); + nvdev = hv_get_drvdata(device_obj); + rdev = nvdev->extension; + if (!rdev->link_state) + netif_carrier_on(net); + return ret; } @@ -229,23 +238,24 @@ void netvsc_linkstatus_callback(struct hv_device *device_obj, struct net_device *net; struct net_device_context *ndev_ctx; struct netvsc_device *net_device; + struct rndis_device *rdev; net_device = hv_get_drvdata(device_obj); + rdev = net_device->extension; + + rdev->link_state = status != 1; + net = net_device->ndev; - if (!net) { - netdev_err(net, "got link status but net device " - "not initialized yet\n"); + if (!net || net->reg_state != NETREG_REGISTERED) return; - } + ndev_ctx = netdev_priv(net); if (status == 1) { - netif_carrier_on(net); - ndev_ctx = netdev_priv(net); schedule_delayed_work(&ndev_ctx->dwork, 0); schedule_delayed_work(&ndev_ctx->dwork, msecs_to_jiffies(20)); } else { - netif_carrier_off(net); + schedule_delayed_work(&ndev_ctx->dwork, 0); } } @@ -388,17 +398,35 @@ static const struct net_device_ops device_ops = { * current context when receiving RNDIS_STATUS_MEDIA_CONNECT event. So, add * another netif_notify_peers() into a delayed work, otherwise GARP packet * will not be sent after quick migration, and cause network disconnection. + * Also, we update the carrier status here. */ -static void netvsc_send_garp(struct work_struct *w) +static void netvsc_link_change(struct work_struct *w) { struct net_device_context *ndev_ctx; struct net_device *net; struct netvsc_device *net_device; + struct rndis_device *rdev; + bool notify; + + rtnl_lock(); ndev_ctx = container_of(w, struct net_device_context, dwork.work); net_device = hv_get_drvdata(ndev_ctx->device_ctx); + rdev = net_device->extension; net = net_device->ndev; - netdev_notify_peers(net); + + if (rdev->link_state) { + netif_carrier_off(net); + notify = false; + } else { + netif_carrier_on(net); + notify = true; + } + + rtnl_unlock(); + + if (notify) + netdev_notify_peers(net); } @@ -414,13 +442,10 @@ static int netvsc_probe(struct hv_device *dev, if (!net) return -ENOMEM; - /* Set initial state */ - netif_carrier_off(net); - net_device_ctx = netdev_priv(net); net_device_ctx->device_ctx = dev; hv_set_drvdata(dev, net); - INIT_DELAYED_WORK(&net_device_ctx->dwork, netvsc_send_garp); + INIT_DELAYED_WORK(&net_device_ctx->dwork, netvsc_link_change); INIT_WORK(&net_device_ctx->work, do_set_multicast); net->netdev_ops = &device_ops; @@ -443,8 +468,6 @@ static int netvsc_probe(struct hv_device *dev, } memcpy(net->dev_addr, device_info.mac_adr, ETH_ALEN); - netif_carrier_on(net); - ret = register_netdev(net); if (ret != 0) { pr_err("Unable to register netdev.\n");