[Patch net-next-2.6] netpoll: disable netpoll when enslave a device

[Patch net-next-2.6] netpoll: disable netpoll when enslave a device

[Amerigo Wang]

Currently we do nothing when we enslave a net device which is running netconsole.
Neil pointed out that we may get weird results in such case, so let's disable
netpoll on the device being enslaved. I think it is too harsh to prevent
the device being ensalved if it is running netconsole.

By the way, this patch also removes the NETDEV_GOING_DOWN from netconsole
netdev notifier, because netpoll will check if the device is running or not
and we don't handle NETDEV_PRE_UP neither.

Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: Neil Horman <nhorman@redhat.com>

---
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 088fd84..b9c70c5 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1640,6 +1640,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		}
 	}
 
+	netdev_bonding_change(slave_dev, NETDEV_ENSLAVE);
+
 	/* If this is the first slave, then we need to set the master's hardware
 	 * address to be the same as the slave's. */
 	if (is_zero_ether_addr(bond->dev->dev_addr))
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index a83e101..0c3e8de 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -621,7 +621,8 @@ static int netconsole_netdev_event(struct notifier_block *this,
 	bool stopped = false;
 
 	if (!(event == NETDEV_CHANGENAME || event == NETDEV_UNREGISTER ||
-	      event == NETDEV_BONDING_DESLAVE || event == NETDEV_GOING_DOWN))
+	      event == NETDEV_BONDING_DESLAVE || event == NETDEV_GOING_DOWN ||
+	      event == NETDEV_ENSLAVE))
 		goto done;
 
 	spin_lock_irqsave(&target_list_lock, flags);
@@ -650,8 +651,8 @@ restart:
 					goto restart;
 				}
 				/* Fall through */
-			case NETDEV_GOING_DOWN:
 			case NETDEV_BONDING_DESLAVE:
+			case NETDEV_ENSLAVE:
 				nt->enabled = 0;
 				stopped = true;
 				break;
@@ -660,10 +661,21 @@ restart:
 		netconsole_target_put(nt);
 	}
 	spin_unlock_irqrestore(&target_list_lock, flags);
-	if (stopped && (event == NETDEV_UNREGISTER || event == NETDEV_BONDING_DESLAVE))
+	if (stopped) {
 		printk(KERN_INFO "netconsole: network logging stopped on "
-			"interface %s as it %s\n",  dev->name,
-			event == NETDEV_UNREGISTER ? "unregistered" : "released slaves");
+		       "interface %s as it ", dev->name);
+		switch (event) {
+		case NETDEV_UNREGISTER:
+			printk(KERN_CONT "unregistered\n");
+			break;
+		case NETDEV_BONDING_DESLAVE:
+			printk(KERN_CONT "released slaves\n");
+			break;
+		case NETDEV_ENSLAVE:
+			printk(KERN_CONT "is enslaved\n");
+			break;
+		}
+	}
 
 done:
 	return NOTIFY_DONE;
diff --git a/include/linux/notifier.h b/include/linux/notifier.h
index 621dfa1..3d82867 100644
--- a/include/linux/notifier.h
+++ b/include/linux/notifier.h
@@ -211,6 +211,7 @@ static inline int notifier_to_errno(int ret)
 #define NETDEV_UNREGISTER_BATCH 0x0011
 #define NETDEV_BONDING_DESLAVE  0x0012
 #define NETDEV_NOTIFY_PEERS	0x0013
+#define NETDEV_ENSLAVE		0x0014
 
 #define SYS_DOWN	0x0001	/* Notify of system down */
 #define SYS_RESTART	SYS_DOWN

Re: [Patch net-next-2.6] netpoll: disable netpoll when enslave a device

[Neil Horman]

On Wed, May 18, 2011 at 06:00:35PM +0800, Amerigo Wang wrote:
> Currently we do nothing when we enslave a net device which is running netconsole.
> Neil pointed out that we may get weird results in such case, so let's disable
> netpoll on the device being enslaved. I think it is too harsh to prevent
> the device being ensalved if it is running netconsole.
> 
> By the way, this patch also removes the NETDEV_GOING_DOWN from netconsole
> netdev notifier, because netpoll will check if the device is running or not
> and we don't handle NETDEV_PRE_UP neither.
> 
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Cc: Neil Horman <nhorman@redhat.com>
> 
> ---
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 088fd84..b9c70c5 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1640,6 +1640,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>  		}
>  	}
>  
> +	netdev_bonding_change(slave_dev, NETDEV_ENSLAVE);
> +
>  	/* If this is the first slave, then we need to set the master's hardware
>  	 * address to be the same as the slave's. */
>  	if (is_zero_ether_addr(bond->dev->dev_addr))
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index a83e101..0c3e8de 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -621,7 +621,8 @@ static int netconsole_netdev_event(struct notifier_block *this,
>  	bool stopped = false;
>  
>  	if (!(event == NETDEV_CHANGENAME || event == NETDEV_UNREGISTER ||
> -	      event == NETDEV_BONDING_DESLAVE || event == NETDEV_GOING_DOWN))
> +	      event == NETDEV_BONDING_DESLAVE || event == NETDEV_GOING_DOWN ||
> +	      event == NETDEV_ENSLAVE))
>  		goto done;
>  
>  	spin_lock_irqsave(&target_list_lock, flags);
> @@ -650,8 +651,8 @@ restart:
>  					goto restart;
>  				}
>  				/* Fall through */
> -			case NETDEV_GOING_DOWN:
>  			case NETDEV_BONDING_DESLAVE:
> +			case NETDEV_ENSLAVE:
>  				nt->enabled = 0;
>  				stopped = true;
>  				break;
This wasn't introduced by this patch, but looking at it made me realize that
nt->enabled, if it passes through this code path, doesn't properly track weather
or not netpoll_setup has been called on this interface.  If you look at
drop_netconsole_target, you'll see we only call netpoll_cleanup_target if
nt->enabled is set.  We should probably change the nt->enabled check there, and
in store_enabled to be if (nt->np.dev), like we do in the NETDEV_UNREGISTER case
in netconsole_netdev_event.

> @@ -660,10 +661,21 @@ restart:
>  		netconsole_target_put(nt);
>  	}
>  	spin_unlock_irqrestore(&target_list_lock, flags);
> -	if (stopped && (event == NETDEV_UNREGISTER || event == NETDEV_BONDING_DESLAVE))
> +	if (stopped) {
>  		printk(KERN_INFO "netconsole: network logging stopped on "
> -			"interface %s as it %s\n",  dev->name,
> -			event == NETDEV_UNREGISTER ? "unregistered" : "released slaves");
> +		       "interface %s as it ", dev->name);
> +		switch (event) {
> +		case NETDEV_UNREGISTER:
> +			printk(KERN_CONT "unregistered\n");
> +			break;
> +		case NETDEV_BONDING_DESLAVE:
> +			printk(KERN_CONT "released slaves\n");
> +			break;
> +		case NETDEV_ENSLAVE:
> +			printk(KERN_CONT "is enslaved\n");
> +			break;
> +		}
> +	}
>  
>  done:
>  	return NOTIFY_DONE;
> diff --git a/include/linux/notifier.h b/include/linux/notifier.h
> index 621dfa1..3d82867 100644
> --- a/include/linux/notifier.h
> +++ b/include/linux/notifier.h
> @@ -211,6 +211,7 @@ static inline int notifier_to_errno(int ret)
>  #define NETDEV_UNREGISTER_BATCH 0x0011
>  #define NETDEV_BONDING_DESLAVE  0x0012
>  #define NETDEV_NOTIFY_PEERS	0x0013
> +#define NETDEV_ENSLAVE		0x0014
>  
Nit:
Shouldn't this be NETDEV_BONDING_ENSLAVE, to keep it in line with
NETDEV_BONDING_DESLAVE above?

>  #define SYS_DOWN	0x0001	/* Notify of system down */
>  #define SYS_RESTART	SYS_DOWN
> 


Other than those two points, this looks good to me
Thanks!
Neil

Re: [Patch net-next-2.6] netpoll: disable netpoll when enslave a device

[Cong Wang]

于 2011年05月18日 18:56, Neil Horman 写道:
> On Wed, May 18, 2011 at 06:00:35PM +0800, Amerigo Wang wrote:
...
>> -			case NETDEV_GOING_DOWN:
>>   			case NETDEV_BONDING_DESLAVE:
>> +			case NETDEV_ENSLAVE:
>>   				nt->enabled = 0;
>>   				stopped = true;
>>   				break;
> This wasn't introduced by this patch, but looking at it made me realize that
> nt->enabled, if it passes through this code path, doesn't properly track weather
> or not netpoll_setup has been called on this interface.  If you look at
> drop_netconsole_target, you'll see we only call netpoll_cleanup_target if
> nt->enabled is set.  We should probably change the nt->enabled check there, and
> in store_enabled to be if (nt->np.dev), like we do in the NETDEV_UNREGISTER case
> in netconsole_netdev_event.

Yeah, also note that we can change ->enabled via configfs too.
I guess we probably need to fix this in another patch...


>> +#define NETDEV_ENSLAVE		0x0014
>>
> Nit:
> Shouldn't this be NETDEV_BONDING_ENSLAVE, to keep it in line with
> NETDEV_BONDING_DESLAVE above?

Actually that is my first thought, but I plan to use this in bridge
case too, because using netconsole on a device underlying a bridge
makes little sense too. Thus, I prefer NETDEV_ENSLAVE to
NETDEV_BONDING_ENSLAVE.

>
>>   #define SYS_DOWN	0x0001	/* Notify of system down */
>>   #define SYS_RESTART	SYS_DOWN
>>
>
>
> Other than those two points, this looks good to me

Thanks for review.

Re: [Patch net-next-2.6] netpoll: disable netpoll when enslave a device

[Neil Horman]

On Thu, May 19, 2011 at 01:13:29PM +0800, Cong Wang wrote:
> 于 2011年05月18日 18:56, Neil Horman 写道:
> >On Wed, May 18, 2011 at 06:00:35PM +0800, Amerigo Wang wrote:
> ...
> >>-			case NETDEV_GOING_DOWN:
> >>  			case NETDEV_BONDING_DESLAVE:
> >>+			case NETDEV_ENSLAVE:
> >>  				nt->enabled = 0;
> >>  				stopped = true;
> >>  				break;
> >This wasn't introduced by this patch, but looking at it made me realize that
> >nt->enabled, if it passes through this code path, doesn't properly track weather
> >or not netpoll_setup has been called on this interface.  If you look at
> >drop_netconsole_target, you'll see we only call netpoll_cleanup_target if
> >nt->enabled is set.  We should probably change the nt->enabled check there, and
> >in store_enabled to be if (nt->np.dev), like we do in the NETDEV_UNREGISTER case
> >in netconsole_netdev_event.
> 
> Yeah, also note that we can change ->enabled via configfs too.
> I guess we probably need to fix this in another patch...
> 
Yeah, or you can roll it into this one, I think this is the only location that
needs fixing.

> 
> >>+#define NETDEV_ENSLAVE		0x0014
> >>
> >Nit:
> >Shouldn't this be NETDEV_BONDING_ENSLAVE, to keep it in line with
> >NETDEV_BONDING_DESLAVE above?
> 
> Actually that is my first thought, but I plan to use this in bridge
> case too, because using netconsole on a device underlying a bridge
> makes little sense too. Thus, I prefer NETDEV_ENSLAVE to
> NETDEV_BONDING_ENSLAVE.
> 
That seems reasonable, but if its going to be more generic, could you change
NETDEV_BONDING_DESLAVE to NETDEV_DESLAVE?

> >
> >>  #define SYS_DOWN	0x0001	/* Notify of system down */
> >>  #define SYS_RESTART	SYS_DOWN
> >>
> >
> >
> >Other than those two points, this looks good to me
> 
> Thanks for review.
Thank you!

[V2 Patch net-next-2.6] netpoll: disable netpoll when enslave a device

[Amerigo Wang]

Currently we do nothing when we enslave a net device which is running netconsole.
Neil pointed out that we may get weird results in such case, so let's disable
netpoll on the device being enslaved. I think it is too harsh to prevent
the device being ensalved if it is running netconsole.

By the way, this patch also removes the NETDEV_GOING_DOWN from netconsole
netdev notifier, because netpoll will check if the device is running or not
and we don't handle NETDEV_PRE_UP neither.

Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: Neil Horman <nhorman@redhat.com>

---
 drivers/net/bonding/bond_main.c |    2 ++
 drivers/net/netconsole.c        |   26 +++++++++++++++++---------
 include/linux/notifier.h        |    1 +
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 088fd84..b9c70c5 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1640,6 +1640,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		}
 	}
 
+	netdev_bonding_change(slave_dev, NETDEV_ENSLAVE);
+
 	/* If this is the first slave, then we need to set the master's hardware
 	 * address to be the same as the slave's. */
 	if (is_zero_ether_addr(bond->dev->dev_addr))
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index a83e101..c2a8230 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -621,11 +621,10 @@ static int netconsole_netdev_event(struct notifier_block *this,
 	bool stopped = false;
 
 	if (!(event == NETDEV_CHANGENAME || event == NETDEV_UNREGISTER ||
-	      event == NETDEV_BONDING_DESLAVE || event == NETDEV_GOING_DOWN))
+	      event == NETDEV_BONDING_DESLAVE || event == NETDEV_ENSLAVE))
 		goto done;
 
 	spin_lock_irqsave(&target_list_lock, flags);
-restart:
 	list_for_each_entry(nt, &target_list, list) {
 		netconsole_target_get(nt);
 		if (nt->np.dev == dev) {
@@ -633,6 +632,8 @@ restart:
 			case NETDEV_CHANGENAME:
 				strlcpy(nt->np.dev_name, dev->name, IFNAMSIZ);
 				break;
+			case NETDEV_BONDING_DESLAVE:
+			case NETDEV_ENSLAVE:
 			case NETDEV_UNREGISTER:
 				/*
 				 * rtnl_lock already held
@@ -647,11 +648,7 @@ restart:
 					dev_put(nt->np.dev);
 					nt->np.dev = NULL;
 					netconsole_target_put(nt);
-					goto restart;
 				}
-				/* Fall through */
-			case NETDEV_GOING_DOWN:
-			case NETDEV_BONDING_DESLAVE:
 				nt->enabled = 0;
 				stopped = true;
 				break;
@@ -660,10 +657,21 @@ restart:
 		netconsole_target_put(nt);
 	}
 	spin_unlock_irqrestore(&target_list_lock, flags);
-	if (stopped && (event == NETDEV_UNREGISTER || event == NETDEV_BONDING_DESLAVE))
+	if (stopped) {
 		printk(KERN_INFO "netconsole: network logging stopped on "
-			"interface %s as it %s\n",  dev->name,
-			event == NETDEV_UNREGISTER ? "unregistered" : "released slaves");
+		       "interface %s as it ", dev->name);
+		switch (event) {
+		case NETDEV_UNREGISTER:
+			printk(KERN_CONT "unregistered\n");
+			break;
+		case NETDEV_BONDING_DESLAVE:
+			printk(KERN_CONT "released slaves\n");
+			break;
+		case NETDEV_ENSLAVE:
+			printk(KERN_CONT "is enslaved\n");
+			break;
+		}
+	}
 
 done:
 	return NOTIFY_DONE;
diff --git a/include/linux/notifier.h b/include/linux/notifier.h
index 621dfa1..3d82867 100644
--- a/include/linux/notifier.h
+++ b/include/linux/notifier.h
@@ -211,6 +211,7 @@ static inline int notifier_to_errno(int ret)
 #define NETDEV_UNREGISTER_BATCH 0x0011
 #define NETDEV_BONDING_DESLAVE  0x0012
 #define NETDEV_NOTIFY_PEERS	0x0013
+#define NETDEV_ENSLAVE		0x0014
 
 #define SYS_DOWN	0x0001	/* Notify of system down */
 #define SYS_RESTART	SYS_DOWN

[Patch] bridge: call NETDEV_ENSLAVE notifiers when adding a slave

[Amerigo Wang]

In the previous patch I added NETDEV_ENSLAVE, now
we can notify netconsole when adding a device to a bridge too.

By the way, s/netdev_bonding_change/call_netdevice_notifiers/ in
bond_main.c, since this is not bonding specific.

Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: Neil Horman <nhorman@redhat.com>

---
 drivers/net/bonding/bond_main.c |    2 +-
 net/bridge/br_if.c              |    2 ++
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index b9c70c5..765fdcf 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1640,7 +1640,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		}
 	}
 
-	netdev_bonding_change(slave_dev, NETDEV_ENSLAVE);
+	call_netdevice_notifiers(NETDEV_ENSLAVE, slave_dev);
 
 	/* If this is the first slave, then we need to set the master's hardware
 	 * address to be the same as the slave's. */
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 5dbdfdf..b44fae5 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -338,6 +338,8 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 	if (IS_ERR(p))
 		return PTR_ERR(p);
 
+	call_netdevice_notifiers(NETDEV_ENSLAVE, dev);
+
 	err = dev_set_promiscuity(dev, 1);
 	if (err)
 		goto put_back;

[Bridge] [Patch] bridge: call NETDEV_ENSLAVE notifiers when adding a slave

[Amerigo Wang]

In the previous patch I added NETDEV_ENSLAVE, now
we can notify netconsole when adding a device to a bridge too.

By the way, s/netdev_bonding_change/call_netdevice_notifiers/ in
bond_main.c, since this is not bonding specific.

Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: Neil Horman <nhorman@redhat.com>

---
 drivers/net/bonding/bond_main.c |    2 +-
 net/bridge/br_if.c              |    2 ++
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index b9c70c5..765fdcf 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1640,7 +1640,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		}
 	}
 
-	netdev_bonding_change(slave_dev, NETDEV_ENSLAVE);
+	call_netdevice_notifiers(NETDEV_ENSLAVE, slave_dev);
 
 	/* If this is the first slave, then we need to set the master's hardware
 	 * address to be the same as the slave's. */
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 5dbdfdf..b44fae5 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -338,6 +338,8 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 	if (IS_ERR(p))
 		return PTR_ERR(p);
 
+	call_netdevice_notifiers(NETDEV_ENSLAVE, dev);
+
 	err = dev_set_promiscuity(dev, 1);
 	if (err)
 		goto put_back;

Re: [Patch] bridge: call NETDEV_ENSLAVE notifiers when adding a slave

[Stephen Hemminger]

On Thu, 19 May 2011 18:24:17 +0800
Amerigo Wang <amwang@redhat.com> wrote:

> In the previous patch I added NETDEV_ENSLAVE, now
> we can notify netconsole when adding a device to a bridge too.
> 
> By the way, s/netdev_bonding_change/call_netdevice_notifiers/ in
> bond_main.c, since this is not bonding specific.
> 
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Cc: Neil Horman <nhorman@redhat.com>
> 

Is there a usage for this? What listens for this notification?

Re: [Bridge] [Patch] bridge: call NETDEV_ENSLAVE notifiers when adding a slave

[Stephen Hemminger]

On Thu, 19 May 2011 18:24:17 +0800
Amerigo Wang <amwang@redhat.com> wrote:

> In the previous patch I added NETDEV_ENSLAVE, now
> we can notify netconsole when adding a device to a bridge too.
> 
> By the way, s/netdev_bonding_change/call_netdevice_notifiers/ in
> bond_main.c, since this is not bonding specific.
> 
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Cc: Neil Horman <nhorman@redhat.com>
> 

Is there a usage for this? What listens for this notification?

Re: [Bridge] [Patch] bridge: call NETDEV_ENSLAVE notifiers when adding a slave

[Stephen Hemminger]

On Thu, 19 May 2011 08:12:13 -0700
Stephen Hemminger <shemminger@linux-foundation.org> wrote:

> On Thu, 19 May 2011 18:24:17 +0800
> Amerigo Wang <amwang@redhat.com> wrote:
> 
> > In the previous patch I added NETDEV_ENSLAVE, now
> > we can notify netconsole when adding a device to a bridge too.
> > 
> > By the way, s/netdev_bonding_change/call_netdevice_notifiers/ in
> > bond_main.c, since this is not bonding specific.
> > 
> > Signed-off-by: WANG Cong <amwang@redhat.com>
> > Cc: Neil Horman <nhorman@redhat.com>
> > 
> 
> Is there a usage for this? What listens for this notification?

Never mind it was in the first patch which you did not send.
You should always put a number on group of patches and send
to all parties.

Also, sending networking patches to LKML is a waste of bandwidth
please don't bother.

Re: [Bridge] [Patch] bridge: call NETDEV_ENSLAVE notifiers when adding a slave

[Stephen Hemminger]

On Thu, 19 May 2011 08:12:13 -0700
Stephen Hemminger <shemminger@linux-foundation.org> wrote:

> On Thu, 19 May 2011 18:24:17 +0800
> Amerigo Wang <amwang@redhat.com> wrote:
> 
> > In the previous patch I added NETDEV_ENSLAVE, now
> > we can notify netconsole when adding a device to a bridge too.
> > 
> > By the way, s/netdev_bonding_change/call_netdevice_notifiers/ in
> > bond_main.c, since this is not bonding specific.
> > 
> > Signed-off-by: WANG Cong <amwang@redhat.com>
> > Cc: Neil Horman <nhorman@redhat.com>
> > 
> 
> Is there a usage for this? What listens for this notification?

Never mind it was in the first patch which you did not send.
You should always put a number on group of patches and send
to all parties.

Also, sending networking patches to LKML is a waste of bandwidth
please don't bother.

Re: [Bridge] [Patch] bridge: call NETDEV_ENSLAVE notifiers when adding a slave

[Cong Wang]

于 2011年05月20日 00:04, Stephen Hemminger 写道:
> On Thu, 19 May 2011 08:12:13 -0700
> Stephen Hemminger<shemminger@linux-foundation.org>  wrote:
>
>> On Thu, 19 May 2011 18:24:17 +0800
>> Amerigo Wang<amwang@redhat.com>  wrote:
>>
>>> In the previous patch I added NETDEV_ENSLAVE, now
>>> we can notify netconsole when adding a device to a bridge too.
>>>
>>> By the way, s/netdev_bonding_change/call_netdevice_notifiers/ in
>>> bond_main.c, since this is not bonding specific.
>>>
>>> Signed-off-by: WANG Cong<amwang@redhat.com>
>>> Cc: Neil Horman<nhorman@redhat.com>
>>>
>>
>> Is there a usage for this? What listens for this notification?
>
> Never mind it was in the first patch which you did not send.
> You should always put a number on group of patches and send
> to all parties.

Ah, sorry, my script simply run get_maintainers.pl to
get the Cc list, so bridge list was not in included in
the first patch.

>
> Also, sending networking patches to LKML is a waste of bandwidth
> please don't bother.

Ok, will fix my script.

Thanks.

Re: [Bridge] [Patch] bridge: call NETDEV_ENSLAVE notifiers when adding a slave

[Cong Wang]

于 2011年05月20日 00:04, Stephen Hemminger 写道:
> On Thu, 19 May 2011 08:12:13 -0700
> Stephen Hemminger<shemminger@linux-foundation.org>  wrote:
>
>> On Thu, 19 May 2011 18:24:17 +0800
>> Amerigo Wang<amwang@redhat.com>  wrote:
>>
>>> In the previous patch I added NETDEV_ENSLAVE, now
>>> we can notify netconsole when adding a device to a bridge too.
>>>
>>> By the way, s/netdev_bonding_change/call_netdevice_notifiers/ in
>>> bond_main.c, since this is not bonding specific.
>>>
>>> Signed-off-by: WANG Cong<amwang@redhat.com>
>>> Cc: Neil Horman<nhorman@redhat.com>
>>>
>>
>> Is there a usage for this? What listens for this notification?
>
> Never mind it was in the first patch which you did not send.
> You should always put a number on group of patches and send
> to all parties.

Ah, sorry, my script simply run get_maintainers.pl to
get the Cc list, so bridge list was not in included in
the first patch.

>
> Also, sending networking patches to LKML is a waste of bandwidth
> please don't bother.

Ok, will fix my script.

Thanks.

Re: [V2 Patch net-next-2.6] netpoll: disable netpoll when enslave a device

[Andy Gospodarek]

On Thu, May 19, 2011 at 04:39:53PM +0800, Amerigo Wang wrote:
[...]
> diff --git a/include/linux/notifier.h b/include/linux/notifier.h
> index 621dfa1..3d82867 100644
> --- a/include/linux/notifier.h
> +++ b/include/linux/notifier.h
> @@ -211,6 +211,7 @@ static inline int notifier_to_errno(int ret)
>  #define NETDEV_UNREGISTER_BATCH 0x0011
>  #define NETDEV_BONDING_DESLAVE  0x0012
>  #define NETDEV_NOTIFY_PEERS	0x0013
> +#define NETDEV_ENSLAVE		0x0014
>  
>  #define SYS_DOWN	0x0001	/* Notify of system down */
>  #define SYS_RESTART	SYS_DOWN

Neil just noted the same concern I had -- the asymmetry between
NETDEV_ENSLAVE and NETDEV_BONDING_DESLAVE bothers me a bit.  I also
don't really like the followup patch that uses 'ENSLAVE' in the bridging
code when we typically use that language for bonding only.

What about changing NETDEV_BONDING_DESLAVE to NETDEV_RELEASE and create
NETDEV_JOIN instead of NETDEV_ENSLAVE?  I would prefer that or something
else that might use more generic language that could be applied to all
for stacked interfaces.

Re: [V2 Patch net-next-2.6] netpoll: disable netpoll when enslave a device

[Neil Horman]

On Thu, May 19, 2011 at 07:31:27AM -0400, Andy Gospodarek wrote:
> On Thu, May 19, 2011 at 04:39:53PM +0800, Amerigo Wang wrote:
> [...]
> > diff --git a/include/linux/notifier.h b/include/linux/notifier.h
> > index 621dfa1..3d82867 100644
> > --- a/include/linux/notifier.h
> > +++ b/include/linux/notifier.h
> > @@ -211,6 +211,7 @@ static inline int notifier_to_errno(int ret)
> >  #define NETDEV_UNREGISTER_BATCH 0x0011
> >  #define NETDEV_BONDING_DESLAVE  0x0012
> >  #define NETDEV_NOTIFY_PEERS	0x0013
> > +#define NETDEV_ENSLAVE		0x0014
> >  
> >  #define SYS_DOWN	0x0001	/* Notify of system down */
> >  #define SYS_RESTART	SYS_DOWN
> 
> Neil just noted the same concern I had -- the asymmetry between
> NETDEV_ENSLAVE and NETDEV_BONDING_DESLAVE bothers me a bit.  I also
> don't really like the followup patch that uses 'ENSLAVE' in the bridging
> code when we typically use that language for bonding only.
> 
> What about changing NETDEV_BONDING_DESLAVE to NETDEV_RELEASE and create
> NETDEV_JOIN instead of NETDEV_ENSLAVE?  I would prefer that or something
> else that might use more generic language that could be applied to all
> for stacked interfaces.
JOIN and RELEASE (or perhaps LEAVE) sounds good to me.
Neil

Re: [V2 Patch net-next-2.6] netpoll: disable netpoll when enslave a device

[Cong Wang]

于 2011年05月19日 21:25, Neil Horman 写道:
> On Thu, May 19, 2011 at 07:31:27AM -0400, Andy Gospodarek wrote:
>> On Thu, May 19, 2011 at 04:39:53PM +0800, Amerigo Wang wrote:
>> [...]
>>> diff --git a/include/linux/notifier.h b/include/linux/notifier.h
>>> index 621dfa1..3d82867 100644
>>> --- a/include/linux/notifier.h
>>> +++ b/include/linux/notifier.h
>>> @@ -211,6 +211,7 @@ static inline int notifier_to_errno(int ret)
>>>   #define NETDEV_UNREGISTER_BATCH 0x0011
>>>   #define NETDEV_BONDING_DESLAVE  0x0012
>>>   #define NETDEV_NOTIFY_PEERS	0x0013
>>> +#define NETDEV_ENSLAVE		0x0014
>>>
>>>   #define SYS_DOWN	0x0001	/* Notify of system down */
>>>   #define SYS_RESTART	SYS_DOWN
>>
>> Neil just noted the same concern I had -- the asymmetry between
>> NETDEV_ENSLAVE and NETDEV_BONDING_DESLAVE bothers me a bit.  I also
>> don't really like the followup patch that uses 'ENSLAVE' in the bridging
>> code when we typically use that language for bonding only.
>>
>> What about changing NETDEV_BONDING_DESLAVE to NETDEV_RELEASE and create
>> NETDEV_JOIN instead of NETDEV_ENSLAVE?  I would prefer that or something
>> else that might use more generic language that could be applied to all
>> for stacked interfaces.
> JOIN and RELEASE (or perhaps LEAVE) sounds good to me.

Thanks, Andy and Neil! I will rename them to JOIN and RELEASE.