* When can a net device get its setting correctly ?
@ 2011-10-31 2:53 WeipingPan
2011-10-31 14:19 ` [PATCH] bonding:update speed/duplex for NETDEV_CHANGE Weiping Pan
0 siblings, 1 reply; 9+ messages in thread
From: WeipingPan @ 2011-10-31 2:53 UTC (permalink / raw)
To: open list:NETWORKING [GENERAL]
Hi, all,
BUG DESCRIPTION:
Zheng Liang(lzheng@redhat.com) found a problem that if we config bonding
with arp monitor,
and enslave 10G cards, bonding driver cannot get the speed and duplex
from them,
it will assume to be 100Mb/sec and Full.
I test kernel upstream, commit ec7ae517537a(Merge
git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi-misc-2.6),
it also has this problem.
And not only 10G cards have this problem, when I use 1Gb(igb), the
problem is the same.
[root@dell-p390n-01 ~]# uname -a
Linux dell-p390n-01.lab.bos.redhat.com 3.1.0+ #1 SMP Fri Oct 28 23:38:59
EDT 2011 i686 i686 i386 GNU/Linux
[root@dell-p390n-01 ~]# dmesg |grep p4p1
udev: renamed network interface eth0 to p4p1
ADDRCONF(NETDEV_UP): p4p1: link is not ready
igb: p4p1 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: RX
ADDRCONF(NETDEV_CHANGE): p4p1: link becomes ready
[root@dell-p390n-01 ~]# ethtool p4p1
Settings for p4p1:
Supported ports: [ TP ]
Supported link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Full
Supports auto-negotiation: Yes
Advertised link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Full
Advertised pause frame use: No
Advertised auto-negotiation: Yes
Speed: 1000Mb/s
Duplex: Full
Port: Twisted Pair
PHYAD: 1
Transceiver: internal
Auto-negotiation: on
MDI-X: Unknown
Supports Wake-on: pumbg
Wake-on: d
Current message level: 0x00000003 (3)
Link detected: yes
[root@dell-p390n-01 ~]# modprobe bonding mode=1 arp_interval=100
arp_ip_target=10.66.12.130
[root@dell-p390n-01 ~]# ifconfig bond0 up
[root@dell-p390n-01 ~]# ifenslave bond0 p4p1
[root@dell-p390n-01 ~]# dmesg
bonding: Ethernet Channel Bonding Driver: v3.7.1 (April 27, 2011)
bonding: ARP monitoring set to 100 ms, validate none, with 1 target(s):
bonding: 10.66.12.130
bonding:
ADDRCONF(NETDEV_UP): bond0: link is not ready
bonding: bond0: Warning: failed to get speed and duplex from p4p1,
assumed to be 100Mb/sec and Full.<-----bug
bonding: bond0: making interface p4p1 the new active one.
bonding: bond0: first active interface up!
bonding: bond0: enslaving p4p1 as an active interface with an up link.
ADDRCONF(NETDEV_CHANGE): bond0: link becomes ready
bonding: bond0: link status definitely down for interface p4p1, disabling it
bonding: bond0: now running without any active interface !
igb: p4p1 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: RX
[root@dell-p390n-01 ~]# cat /proc/net/bonding/bond0
Ethernet Channel Bonding Driver: v3.7.1 (April 27, 2011)
Bonding Mode: fault-tolerance (active-backup)
Primary Slave: None
Currently Active Slave: None
MII Status: down
MII Polling Interval (ms): 0
Up Delay (ms): 0
Down Delay (ms): 0
ARP Polling Interval (ms): 100
ARP IP target/s (n.n.n.n form): 10.66.12.130
Slave Interface: p4p1
MII Status: down
Speed: 100 Mbps <------ bug
Duplex: full
Link Failure Count: 1
Permanent HW addr: 00:1b:21:66:d8:a0
Slave queue ID: 0
But there is no such problem when use miimon.
[root@dell-p390n-01 ~]# modprobe bonding mode=1 miimon=100
[root@dell-p390n-01 ~]# ifconfig bond0 up
[root@dell-p390n-01 ~]# ifenslave bond0 p4p1
[root@dell-p390n-01 ~]# dmesg
bonding: Ethernet Channel Bonding Driver: v3.7.1 (April 27, 2011)
bonding: MII link monitoring set to 100 ms
ADDRCONF(NETDEV_UP): bond0: link is not ready
bonding: bond0: enslaving p4p1 as a backup interface with a down link.
igb: p4p1 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: RX
bonding: bond0: link status definitely up for interface p4p1, 1000 Mbps
full duplex.
bonding: bond0: making interface p4p1 the new active one.
bonding: bond0: first active interface up!
ADDRCONF(NETDEV_CHANGE): bond0: link becomes ready
BUG ANALYSIS:
First, when uses arp monitor, the call trace is:
1485 int bond_enslave(struct net_device *bond_dev, struct net_device
*slave_dev)
1652 res = dev_open(slave_dev);
1761 if (bond_update_speed_duplex(new_slave) &&
And when calling bond_update_speed_duplex(), this message, "igb: p4p1
NIC Link
is Up 1000 Mbps Full Duplex, Flow Control: RX", doesn't show up.
So I think even we call dev_open(), but the device is not ready to get its
setting.
Second, when uses miimon, the call trace is:
1485 int bond_enslave(struct net_device *bond_dev, struct net_device
*slave_dev)
1652 res = dev_open(slave_dev);
2419 static void bond_miimon_commit(struct bonding *bond)
2444 bond_update_speed_duplex(slave);
And when calling bond_update_speed_duplex(), it gets correct setting.
QUESTION:
When can a net device get its setting correctly ?
Maybe dev_open() is not enough.
thanks
Weiping Pan
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH] bonding:update speed/duplex for NETDEV_CHANGE 2011-10-31 2:53 When can a net device get its setting correctly ? WeipingPan @ 2011-10-31 14:19 ` Weiping Pan 2011-10-31 18:15 ` Ben Hutchings 0 siblings, 1 reply; 9+ messages in thread From: Weiping Pan @ 2011-10-31 14:19 UTC (permalink / raw) To: netdev; +Cc: fubar, andy, linux-kernel, Weiping Pan Zheng Liang(lzheng@redhat.com) found a bug that if we config bonding with arp monitor, sometimes bonding driver cannot get the speed and duplex from its slaves, it will assume them to be 100Mb/sec and Full, please see /proc/net/bonding/bond0. But there is no such problem when uses miimon. (Take igb for example) I find that the reason is that after dev_open() in bond_enslave(), bond_update_speed_duplex() will call igb_get_settings() , but in that function, it runs ethtool_cmd_speed_set(ecmd, -1); ecmd->duplex = -1; because igb get an error value of status. So even dev_open() is called, but the device is not really ready to get its settings. Maybe it is safe for us to call igb_get_settings() only after this message shows up, that is "igb: p4p1 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: RX". So I prefer to update the speed and duplex for a slave when reseices NETDEV_CHANGE/NETDEV_UP event. Signed-off-by: Weiping Pan <wpan@redhat.com> --- drivers/net/bonding/bond_main.c | 19 ++++++++----------- 1 files changed, 8 insertions(+), 11 deletions(-) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index c34cc1e..f5458eb 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -3220,6 +3220,7 @@ static int bond_slave_netdev_event(unsigned long event, { struct net_device *bond_dev = slave_dev->master; struct bonding *bond = netdev_priv(bond_dev); + struct slave *slave = NULL; switch (event) { case NETDEV_UNREGISTER: @@ -3230,20 +3231,16 @@ static int bond_slave_netdev_event(unsigned long event, bond_release(bond_dev, slave_dev); } break; + case NETDEV_UP: case NETDEV_CHANGE: - if (bond->params.mode == BOND_MODE_8023AD || bond_is_lb(bond)) { - struct slave *slave; - - slave = bond_get_slave_by_dev(bond, slave_dev); - if (slave) { - u32 old_speed = slave->speed; - u8 old_duplex = slave->duplex; - - bond_update_speed_duplex(slave); + slave = bond_get_slave_by_dev(bond, slave_dev); + if (slave) { + u32 old_speed = slave->speed; + u8 old_duplex = slave->duplex; - if (bond_is_lb(bond)) - break; + bond_update_speed_duplex(slave); + if (bond->params.mode == BOND_MODE_8023AD) { if (old_speed != slave->speed) bond_3ad_adapter_speed_changed(slave); if (old_duplex != slave->duplex) -- 1.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] bonding:update speed/duplex for NETDEV_CHANGE 2011-10-31 14:19 ` [PATCH] bonding:update speed/duplex for NETDEV_CHANGE Weiping Pan @ 2011-10-31 18:15 ` Ben Hutchings 2011-10-31 20:32 ` Jay Vosburgh 0 siblings, 1 reply; 9+ messages in thread From: Ben Hutchings @ 2011-10-31 18:15 UTC (permalink / raw) To: Weiping Pan; +Cc: netdev, fubar, andy, linux-kernel On Mon, 2011-10-31 at 22:19 +0800, Weiping Pan wrote: > Zheng Liang(lzheng@redhat.com) found a bug that if we config bonding with > arp monitor, sometimes bonding driver cannot get the speed and duplex from > its slaves, it will assume them to be 100Mb/sec and Full, please see > /proc/net/bonding/bond0. > But there is no such problem when uses miimon. > > (Take igb for example) > I find that the reason is that after dev_open() in bond_enslave(), > bond_update_speed_duplex() will call igb_get_settings() > , but in that function, > it runs ethtool_cmd_speed_set(ecmd, -1); ecmd->duplex = -1; > because igb get an error value of status. > So even dev_open() is called, but the device is not really ready to get its > settings. > > Maybe it is safe for us to call igb_get_settings() only after > this message shows up, that is "igb: p4p1 NIC Link is Up 1000 Mbps Full Duplex, > Flow Control: RX". [...] For any device with autonegotiation enabled, you generally cannot get the speed and duplex settings until the link is up. While the link is down, you may see a value of 0, ~0, or the best mode currently advertised. So I think that the bonding driver should avoid updating the slave speed and duplex values whenever autoneg is enabled and the link is down. Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] bonding:update speed/duplex for NETDEV_CHANGE 2011-10-31 18:15 ` Ben Hutchings @ 2011-10-31 20:32 ` Jay Vosburgh 2011-10-31 20:48 ` Ben Hutchings 2011-11-01 3:20 ` Weiping Pan 0 siblings, 2 replies; 9+ messages in thread From: Jay Vosburgh @ 2011-10-31 20:32 UTC (permalink / raw) To: Ben Hutchings; +Cc: Weiping Pan, netdev, andy, linux-kernel Ben Hutchings <bhutchings@solarflare.com> wrote: >On Mon, 2011-10-31 at 22:19 +0800, Weiping Pan wrote: >> Zheng Liang(lzheng@redhat.com) found a bug that if we config bonding with >> arp monitor, sometimes bonding driver cannot get the speed and duplex from >> its slaves, it will assume them to be 100Mb/sec and Full, please see >> /proc/net/bonding/bond0. >> But there is no such problem when uses miimon. >> >> (Take igb for example) >> I find that the reason is that after dev_open() in bond_enslave(), >> bond_update_speed_duplex() will call igb_get_settings() >> , but in that function, >> it runs ethtool_cmd_speed_set(ecmd, -1); ecmd->duplex = -1; >> because igb get an error value of status. >> So even dev_open() is called, but the device is not really ready to get its >> settings. >> >> Maybe it is safe for us to call igb_get_settings() only after >> this message shows up, that is "igb: p4p1 NIC Link is Up 1000 Mbps Full Duplex, >> Flow Control: RX". >[...] I'll first point out that this patch is somewhat cosmetic, and really only affects what shows up in /proc/net/bonding/bond0 for speed and duplex. The reason being that the modes that actually need to use the speed and duplex information require the miimon for link state checking, and that code path does the right thing already. This has probably been wrong all along, but relatively recently code was added to show the speed and duplex in /proc/net/bonding/bond0, so it now has a visible effect. So, the patch is ok as far as it goes, in that it will keep the values displayed in the /proc file up to date. However, I'm not sure that faking the speed/duplex to 100/Full is still the correct thing to do. For the modes that use the information, the ethtool state won't be queried if carrier is down (and in those cases, if the speed / duplex returns an error while carrier up, we should probably pay attention). For the modes that the information is merely cosmetic, displaying "unknown" as ethtool does is probably a more accurate representation. Can you additionally remove the "fake to 100/Full" logic? This involves changing bond_update_speed_duplex to not fake the speed and duplex, changing bond_enslave to not issue that warning, and changing bond_info_show_slave to handle "bad" speed and duplex values. Anybody see a problem with doing that? >For any device with autonegotiation enabled, you generally cannot get >the speed and duplex settings until the link is up. While the link is >down, you may see a value of 0, ~0, or the best mode currently >advertised. So I think that the bonding driver should avoid updating >the slave speed and duplex values whenever autoneg is enabled and the >link is down. Well, it's a little more complicated than that. Bonding already generally avoids checking the speed and duplex if the slave isn't up (or at least normally won't complain if it fails). This particular case arises only during enslavement. The call to bond_update_speed_duplex call has failed, but the device is marked by bonding to be up. Bonding complains that the device isn't down, but it cannot get speed and duplex, and therefore is assuming them to be 100/Full. The catch is that this happens only for the ARP monitor, because it initially presumes a slave to be up regardless of actual carrier state (for historical reasons related to very old 10 or 10/100 drivers, prior to the introduction of netif_carrier_*). -J --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] bonding:update speed/duplex for NETDEV_CHANGE 2011-10-31 20:32 ` Jay Vosburgh @ 2011-10-31 20:48 ` Ben Hutchings 2011-10-31 21:23 ` Jay Vosburgh 2011-11-01 3:20 ` Weiping Pan 1 sibling, 1 reply; 9+ messages in thread From: Ben Hutchings @ 2011-10-31 20:48 UTC (permalink / raw) To: Jay Vosburgh; +Cc: Weiping Pan, netdev, andy, linux-kernel On Mon, 2011-10-31 at 13:32 -0700, Jay Vosburgh wrote: [...] > This particular case arises only during enslavement. The call > to bond_update_speed_duplex call has failed, but the device is marked by > bonding to be up. Bonding complains that the device isn't down, but it > cannot get speed and duplex, and therefore is assuming them to be > 100/Full. > > The catch is that this happens only for the ARP monitor, because > it initially presumes a slave to be up regardless of actual carrier > state (for historical reasons related to very old 10 or 10/100 drivers, > prior to the introduction of netif_carrier_*). Right, I gathered that. Is there any reason to use the ARP monitor when all slaves support link state notification? Maybe the bonding documentation should recommend miimon in section 7, not just in section 2. Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] bonding:update speed/duplex for NETDEV_CHANGE 2011-10-31 20:48 ` Ben Hutchings @ 2011-10-31 21:23 ` Jay Vosburgh 2011-10-31 22:41 ` Ben Hutchings 0 siblings, 1 reply; 9+ messages in thread From: Jay Vosburgh @ 2011-10-31 21:23 UTC (permalink / raw) To: Ben Hutchings; +Cc: Weiping Pan, netdev, andy, linux-kernel Ben Hutchings <bhutchings@solarflare.com> wrote: >On Mon, 2011-10-31 at 13:32 -0700, Jay Vosburgh wrote: >[...] >> This particular case arises only during enslavement. The call >> to bond_update_speed_duplex call has failed, but the device is marked by >> bonding to be up. Bonding complains that the device isn't down, but it >> cannot get speed and duplex, and therefore is assuming them to be >> 100/Full. >> >> The catch is that this happens only for the ARP monitor, because >> it initially presumes a slave to be up regardless of actual carrier >> state (for historical reasons related to very old 10 or 10/100 drivers, >> prior to the introduction of netif_carrier_*). > >Right, I gathered that. Is there any reason to use the ARP monitor when >all slaves support link state notification? Maybe the bonding >documentation should recommend miimon in section 7, not just in section >2. The ARP monitor can validate that traffic actually flows from the slave to some destination in the switch domain (and back), so, for example, it's useful in cases that multiple switch hops exist between the host and the local router. A link failure in the middle of the path won't affect carrier on the local device, but still may cause a communications break. -J --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] bonding:update speed/duplex for NETDEV_CHANGE 2011-10-31 21:23 ` Jay Vosburgh @ 2011-10-31 22:41 ` Ben Hutchings 0 siblings, 0 replies; 9+ messages in thread From: Ben Hutchings @ 2011-10-31 22:41 UTC (permalink / raw) To: Jay Vosburgh; +Cc: Weiping Pan, netdev, andy, linux-kernel On Mon, 2011-10-31 at 14:23 -0700, Jay Vosburgh wrote: > Ben Hutchings <bhutchings@solarflare.com> wrote: > > >On Mon, 2011-10-31 at 13:32 -0700, Jay Vosburgh wrote: > >[...] > >> This particular case arises only during enslavement. The call > >> to bond_update_speed_duplex call has failed, but the device is marked by > >> bonding to be up. Bonding complains that the device isn't down, but it > >> cannot get speed and duplex, and therefore is assuming them to be > >> 100/Full. > >> > >> The catch is that this happens only for the ARP monitor, because > >> it initially presumes a slave to be up regardless of actual carrier > >> state (for historical reasons related to very old 10 or 10/100 drivers, > >> prior to the introduction of netif_carrier_*). > > > >Right, I gathered that. Is there any reason to use the ARP monitor when > >all slaves support link state notification? Maybe the bonding > >documentation should recommend miimon in section 7, not just in section > >2. > > The ARP monitor can validate that traffic actually flows from > the slave to some destination in the switch domain (and back), so, for > example, it's useful in cases that multiple switch hops exist between > the host and the local router. A link failure in the middle of the path > won't affect carrier on the local device, but still may cause a > communications break. Then the ARP monitor should gracefully handle the case where a new slave has link down, as proposed. Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] bonding:update speed/duplex for NETDEV_CHANGE 2011-10-31 20:32 ` Jay Vosburgh 2011-10-31 20:48 ` Ben Hutchings @ 2011-11-01 3:20 ` Weiping Pan 2011-11-01 21:53 ` David Miller 1 sibling, 1 reply; 9+ messages in thread From: Weiping Pan @ 2011-11-01 3:20 UTC (permalink / raw) To: netdev; +Cc: fubar, andy, linux-kernel, Weiping Pan Zheng Liang(lzheng@redhat.com) found a bug that if we config bonding with arp monitor, sometimes bonding driver cannot get the speed and duplex from its slaves, it will assume them to be 100Mb/sec and Full, please see /proc/net/bonding/bond0. But there is no such problem when uses miimon. (Take igb for example) I find that the reason is that after dev_open() in bond_enslave(), bond_update_speed_duplex() will call igb_get_settings() , but in that function, it runs ethtool_cmd_speed_set(ecmd, -1); ecmd->duplex = -1; because igb get an error value of status. So even dev_open() is called, but the device is not really ready to get its settings. Maybe it is safe for us to call igb_get_settings() only after this message shows up, that is "igb: p4p1 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: RX". So I prefer to update the speed and duplex for a slave when reseices NETDEV_CHANGE/NETDEV_UP event. Changelog V2: 1 remove the "fake 100/Full" logic in bond_update_speed_duplex(), set speed and duplex to -1 when it gets error value of speed and duplex. 2 delete the warning in bond_enslave() if bond_update_speed_duplex() returns error. 3 make bond_info_show_slave() handle bad values of speed and duplex. Signed-off-by: Weiping Pan <wpan@redhat.com> --- drivers/net/bonding/bond_main.c | 37 ++++++++++++------------------------- drivers/net/bonding/bond_procfs.c | 12 ++++++++++-- 2 files changed, 22 insertions(+), 27 deletions(-) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index c34cc1e..b2b9109 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -550,7 +550,7 @@ down: /* * Get link speed and duplex from the slave's base driver * using ethtool. If for some reason the call fails or the - * values are invalid, fake speed and duplex to 100/Full + * values are invalid, set speed and duplex to -1, * and return error. */ static int bond_update_speed_duplex(struct slave *slave) @@ -560,9 +560,8 @@ static int bond_update_speed_duplex(struct slave *slave) u32 slave_speed; int res; - /* Fake speed and duplex */ - slave->speed = SPEED_100; - slave->duplex = DUPLEX_FULL; + slave->speed = -1; + slave->duplex = -1; res = __ethtool_get_settings(slave_dev, &ecmd); if (res < 0) @@ -1751,16 +1750,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) new_slave->link = BOND_LINK_DOWN; } - if (bond_update_speed_duplex(new_slave) && - (new_slave->link != BOND_LINK_DOWN)) { - pr_warning("%s: Warning: failed to get speed and duplex from %s, assumed to be 100Mb/sec and Full.\n", - bond_dev->name, new_slave->dev->name); - - if (bond->params.mode == BOND_MODE_8023AD) { - pr_warning("%s: Warning: Operation of 802.3ad mode requires ETHTOOL support in base driver for proper aggregator selection.\n", - bond_dev->name); - } - } + bond_update_speed_duplex(new_slave); if (USES_PRIMARY(bond->params.mode) && bond->params.primary[0]) { /* if there is a primary slave, remember it */ @@ -3220,6 +3210,7 @@ static int bond_slave_netdev_event(unsigned long event, { struct net_device *bond_dev = slave_dev->master; struct bonding *bond = netdev_priv(bond_dev); + struct slave *slave = NULL; switch (event) { case NETDEV_UNREGISTER: @@ -3230,20 +3221,16 @@ static int bond_slave_netdev_event(unsigned long event, bond_release(bond_dev, slave_dev); } break; + case NETDEV_UP: case NETDEV_CHANGE: - if (bond->params.mode == BOND_MODE_8023AD || bond_is_lb(bond)) { - struct slave *slave; + slave = bond_get_slave_by_dev(bond, slave_dev); + if (slave) { + u32 old_speed = slave->speed; + u8 old_duplex = slave->duplex; - slave = bond_get_slave_by_dev(bond, slave_dev); - if (slave) { - u32 old_speed = slave->speed; - u8 old_duplex = slave->duplex; - - bond_update_speed_duplex(slave); - - if (bond_is_lb(bond)) - break; + bond_update_speed_duplex(slave); + if (bond->params.mode == BOND_MODE_8023AD) { if (old_speed != slave->speed) bond_3ad_adapter_speed_changed(slave); if (old_duplex != slave->duplex) diff --git a/drivers/net/bonding/bond_procfs.c b/drivers/net/bonding/bond_procfs.c index 95de93b..d2ff52e 100644 --- a/drivers/net/bonding/bond_procfs.c +++ b/drivers/net/bonding/bond_procfs.c @@ -157,8 +157,16 @@ static void bond_info_show_slave(struct seq_file *seq, seq_printf(seq, "\nSlave Interface: %s\n", slave->dev->name); seq_printf(seq, "MII Status: %s\n", (slave->link == BOND_LINK_UP) ? "up" : "down"); - seq_printf(seq, "Speed: %d Mbps\n", slave->speed); - seq_printf(seq, "Duplex: %s\n", slave->duplex ? "full" : "half"); + if (slave->speed == -1) + seq_printf(seq, "Speed: %s\n", "Unknown"); + else + seq_printf(seq, "Speed: %d Mbps\n", slave->speed); + + if (slave->duplex == -1) + seq_printf(seq, "Duplex: %s\n", "Unknown"); + else + seq_printf(seq, "Duplex: %s\n", slave->duplex ? "full" : "half"); + seq_printf(seq, "Link Failure Count: %u\n", slave->link_failure_count); -- 1.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] bonding:update speed/duplex for NETDEV_CHANGE 2011-11-01 3:20 ` Weiping Pan @ 2011-11-01 21:53 ` David Miller 0 siblings, 0 replies; 9+ messages in thread From: David Miller @ 2011-11-01 21:53 UTC (permalink / raw) To: wpan; +Cc: netdev, fubar, andy, linux-kernel From: Weiping Pan <wpan@redhat.com> Date: Tue, 1 Nov 2011 11:20:48 +0800 > Zheng Liang(lzheng@redhat.com) found a bug that if we config bonding with > arp monitor, sometimes bonding driver cannot get the speed and duplex from > its slaves, it will assume them to be 100Mb/sec and Full, please see > /proc/net/bonding/bond0. > But there is no such problem when uses miimon. > > (Take igb for example) > I find that the reason is that after dev_open() in bond_enslave(), > bond_update_speed_duplex() will call igb_get_settings() > , but in that function, > it runs ethtool_cmd_speed_set(ecmd, -1); ecmd->duplex = -1; > because igb get an error value of status. > So even dev_open() is called, but the device is not really ready to get its > settings. > > Maybe it is safe for us to call igb_get_settings() only after > this message shows up, that is "igb: p4p1 NIC Link is Up 1000 Mbps Full Duplex, > Flow Control: RX". > > So I prefer to update the speed and duplex for a slave when reseices > NETDEV_CHANGE/NETDEV_UP event. > > Changelog > V2: > 1 remove the "fake 100/Full" logic in bond_update_speed_duplex(), > set speed and duplex to -1 when it gets error value of speed and duplex. > 2 delete the warning in bond_enslave() if bond_update_speed_duplex() returns > error. > 3 make bond_info_show_slave() handle bad values of speed and duplex. > > Signed-off-by: Weiping Pan <wpan@redhat.com> Looks good, applied, thanks! ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-11-01 21:53 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-10-31 2:53 When can a net device get its setting correctly ? WeipingPan 2011-10-31 14:19 ` [PATCH] bonding:update speed/duplex for NETDEV_CHANGE Weiping Pan 2011-10-31 18:15 ` Ben Hutchings 2011-10-31 20:32 ` Jay Vosburgh 2011-10-31 20:48 ` Ben Hutchings 2011-10-31 21:23 ` Jay Vosburgh 2011-10-31 22:41 ` Ben Hutchings 2011-11-01 3:20 ` Weiping Pan 2011-11-01 21:53 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).