From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jay Vosburgh Subject: Re: [PATCH net-next-2.6] bonding: move slave MTU handling from sysfs V2 Date: Wed, 19 May 2010 17:07:41 -0700 Message-ID: <628.1274314061@death.nxdomain.ibm.com> References: <20100518154016.GD2878@psychotron.lab.eng.brq.redhat.com> Cc: netdev@vger.kernel.org, davem@davemloft.net, bonding-devel@lists.sourceforge.net, monis@voltaire.com To: Jiri Pirko Return-path: Received: from e1.ny.us.ibm.com ([32.97.182.141]:58640 "EHLO e1.ny.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751760Ab0ETAHp (ORCPT ); Wed, 19 May 2010 20:07:45 -0400 Received: from d01relay05.pok.ibm.com (d01relay05.pok.ibm.com [9.56.227.237]) by e1.ny.us.ibm.com (8.14.3/8.13.1) with ESMTP id o4K02BhG002079 for ; Wed, 19 May 2010 20:02:11 -0400 Received: from d01av04.pok.ibm.com (d01av04.pok.ibm.com [9.56.224.64]) by d01relay05.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id o4K07iJQ158976 for ; Wed, 19 May 2010 20:07:44 -0400 Received: from d01av04.pok.ibm.com (loopback [127.0.0.1]) by d01av04.pok.ibm.com (8.14.3/8.13.1/NCO v10.0 AVout) with ESMTP id o4K07hR2027144 for ; Wed, 19 May 2010 20:07:43 -0400 In-reply-to: <20100518154016.GD2878@psychotron.lab.eng.brq.redhat.com> Sender: netdev-owner@vger.kernel.org List-ID: Jiri Pirko wrote: >V1->V2: corrected res/ret use > >For some reason, MTU handling (storing, and restoring) is taking place in >bond_sysfs. The correct place for this code is in bond_enslave, bond_release. >So move it there. In principle this looks ok, as do the other patches, but none of them apply to net-next-2.6 for me except for the "optimize tlb_get_least_loaded_slave" patch. It looks like you left out a patch, see below. >Signed-off-by: Jiri Pirko >--- > drivers/net/bonding/bond_main.c | 15 ++++++++++++++- > drivers/net/bonding/bond_sysfs.c | 22 ++-------------------- > 2 files changed, 16 insertions(+), 21 deletions(-) > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index 5e12462..2c3f9db 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -1533,6 +1533,14 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) > */ > new_slave->original_flags = slave_dev->flags; > >+ /* Save slave's original mtu and then set it to match the bond */ >+ new_slave->original_mtu = slave_dev->mtu; >+ res = dev_set_mtu(slave_dev, bond->dev->mtu); >+ if (res) { >+ pr_debug("Error %d calling dev_set_mtu\n", res); >+ goto err_free; >+ } >+ > /* > * Save slave's original ("permanent") mac address for modes > * that need it, and for restoring it upon release, and then >@@ -1550,7 +1558,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) > res = dev_set_mac_address(slave_dev, &addr); > if (res) { > pr_debug("Error %d calling set_mac_address\n", res); >- goto err_free; >+ goto err_restore_mtu; > } > } > >@@ -1785,6 +1793,9 @@ err_restore_mac: > dev_set_mac_address(slave_dev, &addr); > } > >+err_restore_mtu: >+ dev_set_mtu(slave_dev, new_slave->original_mtu); >+ > err_free: > kfree(new_slave); > >@@ -1969,6 +1980,8 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev) > dev_set_mac_address(slave_dev, &addr); > } > >+ dev_set_mtu(slave_dev, slave->original_mtu); >+ > slave_dev->priv_flags &= ~(IFF_MASTER_8023AD | IFF_MASTER_ALB | > IFF_SLAVE_INACTIVE | IFF_BONDING | > IFF_SLAVE_NEEDARP); >diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c >index 392e291..29a7a8a 100644 >--- a/drivers/net/bonding/bond_sysfs.c >+++ b/drivers/net/bonding/bond_sysfs.c >@@ -220,7 +220,6 @@ static ssize_t bonding_store_slaves(struct device *d, > char command[IFNAMSIZ + 1] = { 0, }; > char *ifname; > int i, res, ret = count; >- u32 original_mtu; > struct slave *slave; > struct net_device *dev = NULL; > struct bonding *bond = to_bond(d); This chunk doesn't apply to net-next-2.6 because your context doesn't match; it looks like you've removed the variable "found" in your "before" source. On closer inspection, "found" isn't actually used meaningfully, so I'm guessing you removed it in a prior patch but didn't submit that patch. If that's the case, could you repost the whole series, with sequence numbers? -J >@@ -281,18 +280,7 @@ static ssize_t bonding_store_slaves(struct device *d, > memcpy(bond->dev->dev_addr, dev->dev_addr, > dev->addr_len); > >- /* Set the slave's MTU to match the bond */ >- original_mtu = dev->mtu; >- res = dev_set_mtu(dev, bond->dev->mtu); >- if (res) { >- ret = res; >- goto out; >- } >- > res = bond_enslave(bond->dev, dev); >- bond_for_each_slave(bond, slave, i) >- if (strnicmp(slave->dev->name, ifname, IFNAMSIZ) == 0) >- slave->original_mtu = original_mtu; > if (res) > ret = res; > >@@ -301,23 +289,17 @@ static ssize_t bonding_store_slaves(struct device *d, > > if (command[0] == '-') { > dev = NULL; >- original_mtu = 0; > bond_for_each_slave(bond, slave, i) > if (strnicmp(slave->dev->name, ifname, IFNAMSIZ) == 0) { > dev = slave->dev; >- original_mtu = slave->original_mtu; > break; > } > if (dev) { > pr_info("%s: Removing slave %s\n", > bond->dev->name, dev->name); >- res = bond_release(bond->dev, dev); >- if (res) { >+ res = bond_release(bond->dev, dev); >+ if (res) > ret = res; >- goto out; >- } >- /* set the slave MTU to the default */ >- dev_set_mtu(dev, original_mtu); > } else { > pr_err("unable to remove non-existent slave %s for bond %s.\n", > ifname, bond->dev->name); >-- >1.6.6.1 --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com