2014-06-20 00:35:39

by Haiyang Zhang

[permalink] [raw]
Subject: [PATCH net-next] hyperv: Add handler for RNDIS_STATUS_NETWORK_CHANGE event

The RNDIS_STATUS_NETWORK_CHANGE event is received after the Hyper-V host
sleep or hibernation. We refresh network at this time.
MS-TFS: 135162

Signed-off-by: Haiyang Zhang <[email protected]>
Reviewed-by: K. Y. Srinivasan <[email protected]>

---
drivers/net/hyperv/hyperv_net.h | 3 ++-
drivers/net/hyperv/netvsc_drv.c | 30 ++++++++++++++++++++++++++----
drivers/net/hyperv/rndis_filter.c | 21 +--------------------
include/linux/rndis.h | 1 +
4 files changed, 30 insertions(+), 25 deletions(-)

diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 6cc37c1..24441ae 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -170,6 +170,7 @@ struct rndis_device {

enum rndis_device_state state;
bool link_state;
+ bool link_change;
atomic_t new_req_id;

spinlock_t request_lock;
@@ -185,7 +186,7 @@ int netvsc_device_remove(struct hv_device *device);
int netvsc_send(struct hv_device *device,
struct hv_netvsc_packet *packet);
void netvsc_linkstatus_callback(struct hv_device *device_obj,
- unsigned int status);
+ struct rndis_message *resp);
int netvsc_recv_callback(struct hv_device *device_obj,
struct hv_netvsc_packet *packet,
struct ndis_tcp_ip_checksum_info *csum_info);
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 4fd71b7..9b27ca8 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -579,8 +579,9 @@ drop:
* netvsc_linkstatus_callback - Link up/down notification
*/
void netvsc_linkstatus_callback(struct hv_device *device_obj,
- unsigned int status)
+ struct rndis_message *resp)
{
+ struct rndis_indicate_status *indicate = &resp->msg.indicate_status;
struct net_device *net;
struct net_device_context *ndev_ctx;
struct netvsc_device *net_device;
@@ -589,7 +590,19 @@ void netvsc_linkstatus_callback(struct hv_device *device_obj,
net_device = hv_get_drvdata(device_obj);
rdev = net_device->extension;

- rdev->link_state = status != 1;
+ switch (indicate->status) {
+ case RNDIS_STATUS_MEDIA_CONNECT:
+ rdev->link_state = false;
+ break;
+ case RNDIS_STATUS_MEDIA_DISCONNECT:
+ rdev->link_state = true;
+ break;
+ case RNDIS_STATUS_NETWORK_CHANGE:
+ rdev->link_change = true;
+ break;
+ default:
+ return;
+ }

net = net_device->ndev;

@@ -597,7 +610,7 @@ void netvsc_linkstatus_callback(struct hv_device *device_obj,
return;

ndev_ctx = netdev_priv(net);
- if (status == 1) {
+ if (!rdev->link_state) {
schedule_delayed_work(&ndev_ctx->dwork, 0);
schedule_delayed_work(&ndev_ctx->dwork, msecs_to_jiffies(20));
} else {
@@ -767,7 +780,9 @@ static void netvsc_link_change(struct work_struct *w)
struct net_device *net;
struct netvsc_device *net_device;
struct rndis_device *rdev;
- bool notify;
+ bool notify, refresh = false;
+ char *argv[] = { "/etc/init.d/network", "restart", NULL };
+ char *envp[] = { "HOME=/", "PATH=/sbin:/usr/sbin:/bin:/usr/bin", NULL };

rtnl_lock();

@@ -782,10 +797,17 @@ static void netvsc_link_change(struct work_struct *w)
} else {
netif_carrier_on(net);
notify = true;
+ if (rdev->link_change) {
+ rdev->link_change = false;
+ refresh = true;
+ }
}

rtnl_unlock();

+ if (refresh)
+ call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
+
if (notify)
netdev_notify_peers(net);
}
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index 99c527a..2b86f0b 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -320,25 +320,6 @@ static void rndis_filter_receive_response(struct rndis_device *dev,
}
}

-static void rndis_filter_receive_indicate_status(struct rndis_device *dev,
- struct rndis_message *resp)
-{
- struct rndis_indicate_status *indicate =
- &resp->msg.indicate_status;
-
- if (indicate->status == RNDIS_STATUS_MEDIA_CONNECT) {
- netvsc_linkstatus_callback(
- dev->net_dev->dev, 1);
- } else if (indicate->status == RNDIS_STATUS_MEDIA_DISCONNECT) {
- netvsc_linkstatus_callback(
- dev->net_dev->dev, 0);
- } else {
- /*
- * TODO:
- */
- }
-}
-
/*
* Get the Per-Packet-Info with the specified type
* return NULL if not found.
@@ -464,7 +445,7 @@ int rndis_filter_receive(struct hv_device *dev,

case RNDIS_MSG_INDICATE:
/* notification msgs */
- rndis_filter_receive_indicate_status(rndis_dev, rndis_msg);
+ netvsc_linkstatus_callback(dev, rndis_msg);
break;
default:
netdev_err(ndev,
diff --git a/include/linux/rndis.h b/include/linux/rndis.h
index 0c8dc71..93c0a64 100644
--- a/include/linux/rndis.h
+++ b/include/linux/rndis.h
@@ -65,6 +65,7 @@
#define RNDIS_STATUS_MEDIA_SPECIFIC_INDICATION 0x40010012
#define RNDIS_STATUS_WW_INDICATION RDIA_SPECIFIC_INDICATION
#define RNDIS_STATUS_LINK_SPEED_CHANGE 0x40010013L
+#define RNDIS_STATUS_NETWORK_CHANGE 0x40010018

#define RNDIS_STATUS_NOT_RESETTABLE 0x80010001
#define RNDIS_STATUS_SOFT_ERRORS 0x80010003
--
1.7.4.1


2014-06-20 04:22:00

by David Miller

[permalink] [raw]
Subject: Re: [PATCH net-next] hyperv: Add handler for RNDIS_STATUS_NETWORK_CHANGE event

From: Haiyang Zhang <[email protected]>
Date: Thu, 19 Jun 2014 18:34:36 -0700

> The RNDIS_STATUS_NETWORK_CHANGE event is received after the Hyper-V host
> sleep or hibernation. We refresh network at this time.
> MS-TFS: 135162
>
> Signed-off-by: Haiyang Zhang <[email protected]>
> Reviewed-by: K. Y. Srinivasan <[email protected]>

Applied, thanks.

2014-06-20 04:57:10

by Olaf Hering

[permalink] [raw]
Subject: Re: [PATCH net-next] hyperv: Add handler for RNDIS_STATUS_NETWORK_CHANGE event

On Thu, Jun 19, Haiyang Zhang wrote:

> The RNDIS_STATUS_NETWORK_CHANGE event is received after the Hyper-V host
> sleep or hibernation. We refresh network at this time.

> + char *argv[] = { "/etc/init.d/network", "restart", NULL };

What happens if that file does not exist? Dead network in the guest?
I tend to think if a VM with PV drivers goes to sleep it has to go
through the whole suspend/resume cycle, very much like the "LID closed"
event. So I think this and the other fbdev change that is floating
around is wrong.

Olaf

2014-06-20 05:12:26

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH net-next] hyperv: Add handler for RNDIS_STATUS_NETWORK_CHANGE event

On Fri, Jun 20, 2014 at 06:57:04AM +0200, Olaf Hering wrote:
> On Thu, Jun 19, Haiyang Zhang wrote:
>
> > The RNDIS_STATUS_NETWORK_CHANGE event is received after the Hyper-V host
> > sleep or hibernation. We refresh network at this time.
>
> > + char *argv[] = { "/etc/init.d/network", "restart", NULL };
>
> What happens if that file does not exist? Dead network in the guest?
> I tend to think if a VM with PV drivers goes to sleep it has to go
> through the whole suspend/resume cycle, very much like the "LID closed"
> event. So I think this and the other fbdev change that is floating
> around is wrong.

Ah, and what about systems with no /etc/init.d/ at all (like
systemd-based ones)? You can't have a kernel driver ask userspace to
restart all networking connections, that seems really wrong.

greg k-h

2014-06-20 08:42:07

by Dan Carpenter

[permalink] [raw]
Subject: Re: [PATCH net-next] hyperv: Add handler for RNDIS_STATUS_NETWORK_CHANGE event

On Thu, Jun 19, 2014 at 06:34:36PM -0700, Haiyang Zhang wrote:
> @@ -589,7 +590,19 @@ void netvsc_linkstatus_callback(struct hv_device *device_obj,
> net_device = hv_get_drvdata(device_obj);
> rdev = net_device->extension;
>
> - rdev->link_state = status != 1;
> + switch (indicate->status) {
> + case RNDIS_STATUS_MEDIA_CONNECT:
> + rdev->link_state = false;

link_state false means that we want to connect?

> + break;
> + case RNDIS_STATUS_MEDIA_DISCONNECT:
> + rdev->link_state = true;

link_state true means that we are disconnecting.

> + break;
> + case RNDIS_STATUS_NETWORK_CHANGE:
> + rdev->link_change = true;
> + break;
> + default:
> + return;
> + }
>
> net = net_device->ndev;
>
> @@ -597,7 +610,7 @@ void netvsc_linkstatus_callback(struct hv_device *device_obj,

> @@ -782,10 +797,17 @@ static void netvsc_link_change(struct work_struct *w)
> } else {
> netif_carrier_on(net);
> notify = true;
> + if (rdev->link_change) {
> + rdev->link_change = false;
> + refresh = true;
> + }

How do we know that we received a RNDIS_STATUS_MEDIA_CONNECT before we
received the RNDIS_STATUS_NETWORK_CHANGE? In other words, why does
RNDIS_STATUS_NETWORK_CHANGE imply that the link_state is false?

> }
>
> rtnl_unlock();
>
> + if (refresh)
> + call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);

You may as well use UMH_NO_WAIT since there is no error handling if
/etc/init.d/network is not found.

regards,
dan carpenter

2014-06-20 15:49:25

by Haiyang Zhang

[permalink] [raw]
Subject: RE: [PATCH net-next] hyperv: Add handler for RNDIS_STATUS_NETWORK_CHANGE event



> -----Original Message-----
> From: Dan Carpenter [mailto:[email protected]]
> Sent: Friday, June 20, 2014 4:42 AM
> To: Haiyang Zhang
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH net-next] hyperv: Add handler for
> RNDIS_STATUS_NETWORK_CHANGE event
>
> On Thu, Jun 19, 2014 at 06:34:36PM -0700, Haiyang Zhang wrote:
> > @@ -589,7 +590,19 @@ void netvsc_linkstatus_callback(struct hv_device
> *device_obj,
> > net_device = hv_get_drvdata(device_obj);
> > rdev = net_device->extension;
> >
> > - rdev->link_state = status != 1;
> > + switch (indicate->status) {
> > + case RNDIS_STATUS_MEDIA_CONNECT:
> > + rdev->link_state = false;
>
> link_state false means that we want to connect?
Yes

>
> > + break;
> > + case RNDIS_STATUS_MEDIA_DISCONNECT:
> > + rdev->link_state = true;
>
> link_state true means that we are disconnecting.
Yes.

> > @@ -782,10 +797,17 @@ static void netvsc_link_change(struct
> work_struct *w)
> > } else {
> > netif_carrier_on(net);
> > notify = true;
> > + if (rdev->link_change) {
> > + rdev->link_change = false;
> > + refresh = true;
> > + }
>
> How do we know that we received a RNDIS_STATUS_MEDIA_CONNECT before we
> received the RNDIS_STATUS_NETWORK_CHANGE? In other words, why does
> RNDIS_STATUS_NETWORK_CHANGE imply that the link_state is false?

After host sleep, both RNDIS_STATUS_MEDIA_CONNECT and
RNDIS_STATUS_NETWORK_CHANGE events are received, but not necessarily in
this order. If RNDIS_STATUS_MEDIA_CONNECT arrives later, the flag saved
in rdev->link_change previously will trigger the refresh, not in the
RNDIS_STATUS_NETWORK_CHANGE event, but in the latter RNDIS_STATUS_MEDIA_CONNECT
event.

>
> > }
> >
> > rtnl_unlock();
> >
> > + if (refresh)
> > + call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
>
> You may as well use UMH_NO_WAIT since there is no error handling if
> /etc/init.d/network is not found.

I previously tried UMH_NO_WAIT, but not working. We need to wait for the
exec (not process completion) in this case. Since it's in the work queue,
a bit of waiting is OK.

Thanks,
- Haiyang

2014-06-20 16:10:45

by Haiyang Zhang

[permalink] [raw]
Subject: RE: [PATCH net-next] hyperv: Add handler for RNDIS_STATUS_NETWORK_CHANGE event



> -----Original Message-----
> From: Greg KH [mailto:[email protected]]
> Sent: Friday, June 20, 2014 1:12 AM
> To: Olaf Hering
> Cc: Haiyang Zhang; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH net-next] hyperv: Add handler for
> RNDIS_STATUS_NETWORK_CHANGE event
>
> On Fri, Jun 20, 2014 at 06:57:04AM +0200, Olaf Hering wrote:
> > On Thu, Jun 19, Haiyang Zhang wrote:
> >
> > > The RNDIS_STATUS_NETWORK_CHANGE event is received after the Hyper-V
> host
> > > sleep or hibernation. We refresh network at this time.
> >
> > > + char *argv[] = { "/etc/init.d/network", "restart", NULL };
> >
> > What happens if that file does not exist? Dead network in the guest?
> > I tend to think if a VM with PV drivers goes to sleep it has to go
> > through the whole suspend/resume cycle, very much like the "LID
> closed"
> > event. So I think this and the other fbdev change that is floating
> > around is wrong.
>
> Ah, and what about systems with no /etc/init.d/ at all (like
> systemd-based ones)? You can't have a kernel driver ask userspace to
> restart all networking connections, that seems really wrong.

Olaf and Greg,

Thanks for your reviews.
On Server Hyper-V, the host sleep/hibernation is not supported. So this
event won't happen on most deployment of Hyper-V.

On Client Hyper-V (e.g. Windows 8+ on a laptop), the host sleep/hibernation
is supported. If the laptop is moved to another network during sleep, we
previously need to manually refresh the network (network restart) to renew
DHCP. With this patch, the network is refreshed on the event.

This command ("/etc/init.d/network restart") exists on our supported distros
currently. We will also look into some better ways to refresh the network for
the distros without this command. I have tried setting IF_OPER_DORMANT then
IF_OPER_UP, but not working. I will look into the suspend/resume cycle as
well.

Thanks,
- Haiyang

2014-06-23 08:02:50

by Olaf Hering

[permalink] [raw]
Subject: Re: [PATCH net-next] hyperv: Add handler for RNDIS_STATUS_NETWORK_CHANGE event

On Fri, Jun 20, Haiyang Zhang wrote:

> This command ("/etc/init.d/network restart") exists on our supported distros
> currently. We will also look into some better ways to refresh the network for
> the distros without this command. I have tried setting IF_OPER_DORMANT then
> IF_OPER_UP, but not working. I will look into the suspend/resume cycle as
> well.

I think its reasonable to expect guest config changes on this new kind
of host. Would a link-down/link-up event work? I'm sure it will, there
is enough code floating around in the guests which handles cable unplug.

Olaf

2014-06-23 12:48:05

by Haiyang Zhang

[permalink] [raw]
Subject: RE: [PATCH net-next] hyperv: Add handler for RNDIS_STATUS_NETWORK_CHANGE event



> -----Original Message-----
> From: Olaf Hering [mailto:[email protected]]
> Sent: Monday, June 23, 2014 4:03 AM
> To: Haiyang Zhang
> Cc: Greg KH; [email protected]; [email protected]; driverdev-
> [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH net-next] hyperv: Add handler for
> RNDIS_STATUS_NETWORK_CHANGE event
>
> On Fri, Jun 20, Haiyang Zhang wrote:
>
> > This command ("/etc/init.d/network restart") exists on our supported
> distros
> > currently. We will also look into some better ways to refresh the
> network for
> > the distros without this command. I have tried setting IF_OPER_DORMANT
> then
> > IF_OPER_UP, but not working. I will look into the suspend/resume cycle
> as
> > well.
>
> I think its reasonable to expect guest config changes on this new kind
> of host. Would a link-down/link-up event work? I'm sure it will, there
> is enough code floating around in the guests which handles cable unplug.

Do you mean netif_carrier_off() / netif_carrier_on()? They are already
called in the code before this patch, but DHCP renew is not triggered by
them.

Thanks,
- Haiyang

????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2014-06-23 13:17:30

by Olaf Hering

[permalink] [raw]
Subject: Re: [PATCH net-next] hyperv: Add handler for RNDIS_STATUS_NETWORK_CHANGE event

On Mon, Jun 23, Haiyang Zhang wrote:

> > I think its reasonable to expect guest config changes on this new kind
> > of host. Would a link-down/link-up event work? I'm sure it will, there
> > is enough code floating around in the guests which handles cable unplug.
>
> Do you mean netif_carrier_off() / netif_carrier_on()? They are already
> called in the code before this patch, but DHCP renew is not triggered by
> them.

I do not know how to simulate a cable unplug. The point is that calling
/etc/init.d/network will fail, at least in SLES12.
Maybe some sort of "DHCP refresh required" event is required?
Maybe the DHCP clients need to renew on cable unplug?
No idea what the solution to the issue really is.

Olaf

2014-06-23 16:10:22

by Haiyang Zhang

[permalink] [raw]
Subject: RE: [PATCH net-next] hyperv: Add handler for RNDIS_STATUS_NETWORK_CHANGE event



> -----Original Message-----
> From: Olaf Hering [mailto:[email protected]]
> Sent: Monday, June 23, 2014 9:17 AM
> To: Haiyang Zhang
> Cc: Greg KH; [email protected]; [email protected]; driverdev-
> [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH net-next] hyperv: Add handler for
> RNDIS_STATUS_NETWORK_CHANGE event
>
> On Mon, Jun 23, Haiyang Zhang wrote:
>
> > > I think its reasonable to expect guest config changes on this new
> > > kind of host. Would a link-down/link-up event work? I'm sure it
> > > will, there is enough code floating around in the guests which handles cable
> unplug.
> >
> > Do you mean netif_carrier_off() / netif_carrier_on()? They are already
> > called in the code before this patch, but DHCP renew is not triggered
> > by them.
>
> I do not know how to simulate a cable unplug. The point is that calling
> /etc/init.d/network will fail, at least in SLES12.
> Maybe some sort of "DHCP refresh required" event is required?
> Maybe the DHCP clients need to renew on cable unplug?
> No idea what the solution to the issue really is.

Yes, it will be great if there is such a "DHCP refresh required" event, or DHCP clients
are triggered when netif_carrier_off() then netif_carrier_on().

I have tried some possibilities, like IF_OPER_DORMANT then IF_OPER_UP with
netdev_state_change() etc. but not able to trigger DHCP review. I will look at this
further...

So, what's the equivalent or similar command to "network restart" on SLES12? Could
you update the command line for the usermodehelper when porting this patch to SLES
12?

Thanks,
- Haiyang

????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2014-06-23 16:27:12

by Olaf Hering

[permalink] [raw]
Subject: Re: [PATCH net-next] hyperv: Add handler for RNDIS_STATUS_NETWORK_CHANGE event

On Mon, Jun 23, Haiyang Zhang wrote:

> I have tried some possibilities, like IF_OPER_DORMANT then IF_OPER_UP with
> netdev_state_change() etc. but not able to trigger DHCP review. I will look at this
> further...

Is there a link down/up event anyway?
If the interface is configured to "on hotplug" or "on cable plugged in"
or whatever the setting name is, the network scripts should do
something. Even if its not a DHCP renew.

> So, what's the equivalent or similar command to "network restart" on SLES12? Could
> you update the command line for the usermodehelper when porting this patch to SLES
> 12?

I see a /sbin/rcnetwork in SLES, which links either to
'/etc/init.d/network' or 'service'. This could be used as a quick
workaround.

Olaf

2014-06-23 16:29:23

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH net-next] hyperv: Add handler for RNDIS_STATUS_NETWORK_CHANGE event

On Mon, Jun 23, 2014 at 04:09:59PM +0000, Haiyang Zhang wrote:
>
>
> > -----Original Message-----
> > From: Olaf Hering [mailto:[email protected]]
> > Sent: Monday, June 23, 2014 9:17 AM
> > To: Haiyang Zhang
> > Cc: Greg KH; [email protected]; [email protected]; driverdev-
> > [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [PATCH net-next] hyperv: Add handler for
> > RNDIS_STATUS_NETWORK_CHANGE event
> >
> > On Mon, Jun 23, Haiyang Zhang wrote:
> >
> > > > I think its reasonable to expect guest config changes on this new
> > > > kind of host. Would a link-down/link-up event work? I'm sure it
> > > > will, there is enough code floating around in the guests which handles cable
> > unplug.
> > >
> > > Do you mean netif_carrier_off() / netif_carrier_on()? They are already
> > > called in the code before this patch, but DHCP renew is not triggered
> > > by them.
> >
> > I do not know how to simulate a cable unplug. The point is that calling
> > /etc/init.d/network will fail, at least in SLES12.
> > Maybe some sort of "DHCP refresh required" event is required?
> > Maybe the DHCP clients need to renew on cable unplug?
> > No idea what the solution to the issue really is.
>
> Yes, it will be great if there is such a "DHCP refresh required" event, or DHCP clients
> are triggered when netif_carrier_off() then netif_carrier_on().
>
> I have tried some possibilities, like IF_OPER_DORMANT then IF_OPER_UP with
> netdev_state_change() etc. but not able to trigger DHCP review. I will look at this
> further...
>
> So, what's the equivalent or similar command to "network restart" on SLES12? Could
> you update the command line for the usermodehelper when porting this patch to SLES
> 12?

Given that this change will fail on all future distro releases, and
almost all of the community distros today, I don't see how this is
acceptable at all. Nor would it be any better if you switch to a
systemd command line script as well. You should just work like any
other network device works in this situation when it comes to
enabling/disabling the device. Worse case, just tear down the whole
network device at suspend time, and recreate it at resume.

greg k-h

2014-06-23 18:21:25

by Haiyang Zhang

[permalink] [raw]
Subject: RE: [PATCH net-next] hyperv: Add handler for RNDIS_STATUS_NETWORK_CHANGE event



> -----Original Message-----
> From: Olaf Hering [mailto:[email protected]]
> Sent: Monday, June 23, 2014 12:27 PM
> To: Haiyang Zhang
> Cc: Greg KH; [email protected]; [email protected]; driverdev-
> [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH net-next] hyperv: Add handler for
> RNDIS_STATUS_NETWORK_CHANGE event
>
> On Mon, Jun 23, Haiyang Zhang wrote:
>
> > I have tried some possibilities, like IF_OPER_DORMANT then IF_OPER_UP
> > with
> > netdev_state_change() etc. but not able to trigger DHCP review. I will
> > look at this further...
>
> Is there a link down/up event anyway?
> If the interface is configured to "on hotplug" or "on cable plugged in"
> or whatever the setting name is, the network scripts should do something. Even
> if its not a DHCP renew.

Yes, there is a link down/up event from the host, we currently call netif_carrier_off()
/ netif_carrier_on() with these events. Will hotplug scripts be triggered by
netif_carrier_off/on? Where are the scripts located at (SLES)?

>
> > So, what's the equivalent or similar command to "network restart" on
> > SLES12? Could you update the command line for the usermodehelper when
> > porting this patch to SLES 12?
>
> I see a /sbin/rcnetwork in SLES, which links either to '/etc/init.d/network' or
> 'service'. This could be used as a quick workaround.

Thanks for the info!

Thanks,
- Haiyang
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2014-06-23 18:24:06

by Haiyang Zhang

[permalink] [raw]
Subject: RE: [PATCH net-next] hyperv: Add handler for RNDIS_STATUS_NETWORK_CHANGE event



> -----Original Message-----
> From: Greg KH [mailto:[email protected]]
> Sent: Monday, June 23, 2014 12:29 PM
> To: Haiyang Zhang
> Cc: Olaf Hering; [email protected]; [email protected]; driverdev-
> [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH net-next] hyperv: Add handler for
> RNDIS_STATUS_NETWORK_CHANGE event
>
> On Mon, Jun 23, 2014 at 04:09:59PM +0000, Haiyang Zhang wrote:
> >
> >
> > > -----Original Message-----
> > > From: Olaf Hering [mailto:[email protected]]
> > > Sent: Monday, June 23, 2014 9:17 AM
> > > To: Haiyang Zhang
> > > Cc: Greg KH; [email protected]; [email protected]; driverdev-
> > > [email protected]; [email protected];
> > > [email protected]
> > > Subject: Re: [PATCH net-next] hyperv: Add handler for
> > > RNDIS_STATUS_NETWORK_CHANGE event
> > >
> > > On Mon, Jun 23, Haiyang Zhang wrote:
> > >
> > > > > I think its reasonable to expect guest config changes on this
> > > > > new kind of host. Would a link-down/link-up event work? I'm sure
> > > > > it will, there is enough code floating around in the guests
> > > > > which handles cable
> > > unplug.
> > > >
> > > > Do you mean netif_carrier_off() / netif_carrier_on()? They are
> > > > already called in the code before this patch, but DHCP renew is
> > > > not triggered by them.
> > >
> > > I do not know how to simulate a cable unplug. The point is that
> > > calling /etc/init.d/network will fail, at least in SLES12.
> > > Maybe some sort of "DHCP refresh required" event is required?
> > > Maybe the DHCP clients need to renew on cable unplug?
> > > No idea what the solution to the issue really is.
> >
> > Yes, it will be great if there is such a "DHCP refresh required"
> > event, or DHCP clients are triggered when netif_carrier_off() then
> netif_carrier_on().
> >
> > I have tried some possibilities, like IF_OPER_DORMANT then IF_OPER_UP
> > with
> > netdev_state_change() etc. but not able to trigger DHCP review. I will
> > look at this further...
> >
> > So, what's the equivalent or similar command to "network restart" on
> > SLES12? Could you update the command line for the usermodehelper when
> > porting this patch to SLES 12?
>
> Given that this change will fail on all future distro releases, and almost all of
> the community distros today, I don't see how this is acceptable at all. Nor
> would it be any better if you switch to a systemd command line script as well.
> You should just work like any other network device works in this situation when
> it comes to enabling/disabling the device. Worse case, just tear down the
> whole network device at suspend time, and recreate it at resume.

I will look into these options.

Thanks,
- Haiyang

2014-06-23 20:06:11

by David Miller

[permalink] [raw]
Subject: Re: [PATCH net-next] hyperv: Add handler for RNDIS_STATUS_NETWORK_CHANGE event

From: Olaf Hering <[email protected]>
Date: Mon, 23 Jun 2014 15:17:23 +0200

> On Mon, Jun 23, Haiyang Zhang wrote:
>
>> > I think its reasonable to expect guest config changes on this new kind
>> > of host. Would a link-down/link-up event work? I'm sure it will, there
>> > is enough code floating around in the guests which handles cable unplug.
>>
>> Do you mean netif_carrier_off() / netif_carrier_on()? They are already
>> called in the code before this patch, but DHCP renew is not triggered by
>> them.
>
> I do not know how to simulate a cable unplug. The point is that calling
> /etc/init.d/network will fail, at least in SLES12.
> Maybe some sort of "DHCP refresh required" event is required?
> Maybe the DHCP clients need to renew on cable unplug?
> No idea what the solution to the issue really is.

What's in the driver right now is definitely not it, and I want this
network restart code removed immediately.

2014-06-23 20:10:22

by David Miller

[permalink] [raw]
Subject: Re: [PATCH net-next] hyperv: Add handler for RNDIS_STATUS_NETWORK_CHANGE event

From: Haiyang Zhang <[email protected]>
Date: Mon, 23 Jun 2014 16:09:59 +0000

> So, what's the equivalent or similar command to "network restart" on SLES12? Could
> you update the command line for the usermodehelper when porting this patch to SLES
> 12?

No, you are not going to keep the usermodehelper invocation in your driver
please remove it. It is absolutely inappropriate, and I strictly do not want
to keep it in there because other people will copy it and then we'll have a
real mess on our hands.

2014-06-23 20:11:49

by David Miller

[permalink] [raw]
Subject: Re: [PATCH net-next] hyperv: Add handler for RNDIS_STATUS_NETWORK_CHANGE event

From: Greg KH <[email protected]>
Date: Mon, 23 Jun 2014 12:29:11 -0400

> Given that this change will fail on all future distro releases, and
> almost all of the community distros today, I don't see how this is
> acceptable at all. Nor would it be any better if you switch to a
> systemd command line script as well. You should just work like any
> other network device works in this situation when it comes to
> enabling/disabling the device. Worse case, just tear down the whole
> network device at suspend time, and recreate it at resume.

Agreed, and I've strongly asked them to remove this code immediately.

2014-06-23 20:18:06

by Haiyang Zhang

[permalink] [raw]
Subject: RE: [PATCH net-next] hyperv: Add handler for RNDIS_STATUS_NETWORK_CHANGE event



> -----Original Message-----
> From: David Miller [mailto:[email protected]]
> Sent: Monday, June 23, 2014 4:10 PM
> To: Haiyang Zhang
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH net-next] hyperv: Add handler for
> RNDIS_STATUS_NETWORK_CHANGE event
>
> From: Haiyang Zhang <[email protected]>
> Date: Mon, 23 Jun 2014 16:09:59 +0000
>
> > So, what's the equivalent or similar command to "network restart" on
> > SLES12? Could you update the command line for the usermodehelper when
> > porting this patch to SLES 12?
>
> No, you are not going to keep the usermodehelper invocation in your driver
> please remove it. It is absolutely inappropriate, and I strictly do not want to
> keep it in there because other people will copy it and then we'll have a real
> mess on our hands.

I'm working on it, and will have a patch to replace the usermodehelper soon.

Thanks,
- Haiyang

2014-06-26 08:52:09

by Olaf Hering

[permalink] [raw]
Subject: Re: [PATCH net-next] hyperv: Add handler for RNDIS_STATUS_NETWORK_CHANGE event

On Mon, Jun 23, Haiyang Zhang wrote:

> Yes, there is a link down/up event from the host, we currently call netif_carrier_off()
> / netif_carrier_on() with these events. Will hotplug scripts be triggered by
> netif_carrier_off/on? Where are the scripts located at (SLES)?

In /etc/sysconfig/network/ifcfg-eth0 STARTMODE= should be set to
"ifplugd", this will most likely fix this sort of setup.

Olaf

2014-06-26 14:56:13

by Haiyang Zhang

[permalink] [raw]
Subject: RE: [PATCH net-next] hyperv: Add handler for RNDIS_STATUS_NETWORK_CHANGE event



> -----Original Message-----
> From: Olaf Hering [mailto:[email protected]]
> Sent: Thursday, June 26, 2014 4:46 AM
> To: Haiyang Zhang
> Cc: Greg KH; [email protected]; [email protected]; driverdev-
> [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH net-next] hyperv: Add handler for
> RNDIS_STATUS_NETWORK_CHANGE event
>
> On Mon, Jun 23, Haiyang Zhang wrote:
>
> > Yes, there is a link down/up event from the host, we currently call
> > netif_carrier_off() / netif_carrier_on() with these events. Will
> > hotplug scripts be triggered by netif_carrier_off/on? Where are the scripts
> located at (SLES)?
>
> In /etc/sysconfig/network/ifcfg-eth0 STARTMODE= should be set to "ifplugd",
> this will most likely fix this sort of setup.

Thanks for the info! I'll let our team know.

- Haiyang
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?