Message ID | 20111104182138.GB5796@elgon.mountain |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
Dan Carpenter <dan.carpenter@oracle.com> wrote: >slave->duplex is a u8 type so the in bond_info_show_slave() when we >check "if (slave->duplex == -1)", it's always false. > >Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index b2b9109..b0c5772 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -560,8 +560,8 @@ static int bond_update_speed_duplex(struct slave *slave) > u32 slave_speed; > int res; > >- slave->speed = -1; >- slave->duplex = -1; >+ slave->speed = SPEED_UNKNOWN; >+ slave->duplex = DUPLEX_UNKNOWN; > > res = __ethtool_get_settings(slave_dev, &ecmd); > if (res < 0) >diff --git a/drivers/net/bonding/bond_procfs.c b/drivers/net/bonding/bond_procfs.c >index 2acf0b0..ad284ba 100644 >--- a/drivers/net/bonding/bond_procfs.c >+++ b/drivers/net/bonding/bond_procfs.c >@@ -158,12 +158,12 @@ 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"); >- if (slave->speed == -1) >+ if (slave->speed == SPEED_UNKNOWN) > seq_printf(seq, "Speed: %s\n", "Unknown"); > else > seq_printf(seq, "Speed: %d Mbps\n", slave->speed); Since you #define SPEED_UNKNOWN to -1 (below), how does this actually change anything? Did you mean 0xffff (because struct ethtool_cmd's speed is a u16)? Running on a moderately recent net-next (without the very recent change to bond_update_speed_duplex), I see that bonding indeed doesn't get the speed or duplex correct after a cable pull: Slave Interface: eth2 MII Status: down Speed: 100 Mbps Duplex: full so perhaps a rational (unsigned-friendly) SPEED_UNKNOWN and DUPLEX_UNKNOWN are needed, but I'm not sure how this #define actually would change any behavior in the bonding case. >- if (slave->duplex == -1) >+ if (slave->duplex == DUPLEX_UNKNOWN) > seq_printf(seq, "Duplex: %s\n", "Unknown"); > else > seq_printf(seq, "Duplex: %s\n", slave->duplex ? "full" : "half"); This one might "work," but it seems to depend on the fact that the integral conversion of -1 to an 8 bit unsigned type will be 255 (0xff). I believe that's true (according to the ISO C copy I have handy), but I'm not sure that kind of implicit assumption should be built into the code. At least not without some explanation. -J >diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >index 45f00b6..de33de1 100644 >--- a/include/linux/ethtool.h >+++ b/include/linux/ethtool.h >@@ -1097,10 +1097,12 @@ struct ethtool_ops { > #define SPEED_1000 1000 > #define SPEED_2500 2500 > #define SPEED_10000 10000 >+#define SPEED_UNKNOWN -1 > > /* Duplex, half or full. */ > #define DUPLEX_HALF 0x00 > #define DUPLEX_FULL 0x01 >+#define DUPLEX_UNKNOWN 0xff > > /* Which connector port. */ > #define PORT_TP 0x00 --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com -- 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 Fri, 2011-11-04 at 13:02 -0700, Jay Vosburgh wrote: > Dan Carpenter <dan.carpenter@oracle.com> wrote: > >slave->duplex is a u8 type so the in bond_info_show_slave() when we > >check "if (slave->duplex == -1)", it's always false. > > > >Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > >index b2b9109..b0c5772 100644 > >--- a/drivers/net/bonding/bond_main.c > >+++ b/drivers/net/bonding/bond_main.c > >@@ -560,8 +560,8 @@ static int bond_update_speed_duplex(struct slave *slave) > > u32 slave_speed; > > int res; > > > >- slave->speed = -1; > >- slave->duplex = -1; > >+ slave->speed = SPEED_UNKNOWN; > >+ slave->duplex = DUPLEX_UNKNOWN; > > > > res = __ethtool_get_settings(slave_dev, &ecmd); > > if (res < 0) > >diff --git a/drivers/net/bonding/bond_procfs.c b/drivers/net/bonding/bond_procfs.c > >index 2acf0b0..ad284ba 100644 > >--- a/drivers/net/bonding/bond_procfs.c > >+++ b/drivers/net/bonding/bond_procfs.c > >@@ -158,12 +158,12 @@ 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"); > >- if (slave->speed == -1) > >+ if (slave->speed == SPEED_UNKNOWN) > > seq_printf(seq, "Speed: %s\n", "Unknown"); > > else > > seq_printf(seq, "Speed: %d Mbps\n", slave->speed); > > Since you #define SPEED_UNKNOWN to -1 (below), how does this > actually change anything? Did you mean 0xffff (because struct > ethtool_cmd's speed is a u16)? The speed in ethtool_cmd is 32 bits divided between two fields. > Running on a moderately recent net-next (without the very recent > change to bond_update_speed_duplex), I see that bonding indeed doesn't > get the speed or duplex correct after a cable pull: > > Slave Interface: eth2 > MII Status: down > Speed: 100 Mbps > Duplex: full > > so perhaps a rational (unsigned-friendly) SPEED_UNKNOWN and > DUPLEX_UNKNOWN are needed, but I'm not sure how this #define actually > would change any behavior in the bonding case. Agree that they should be defined somewhere. The ethtool utility recognises speed values of 0, (u16)(-1) and (u32)(-1) as 'unknown'. Personally I think 0 makes more sense than (u32)(-1) but it doesn't matter much. > >- if (slave->duplex == -1) > >+ if (slave->duplex == DUPLEX_UNKNOWN) > > seq_printf(seq, "Duplex: %s\n", "Unknown"); > > else > > seq_printf(seq, "Duplex: %s\n", slave->duplex ? "full" : "half"); > > This one might "work," but it seems to depend on the fact that > the integral conversion of -1 to an 8 bit unsigned type will be 255 > (0xff). I believe that's true (according to the ISO C copy I have > handy), but I'm not sure that kind of implicit assumption should be > built into the code. At least not without some explanation. [...] It's true and does not need explanation. Quite why anyone expected a negative value to survive conversion to u8 and back to int, now that deserves explanation... Ben.
On Fri, Nov 04, 2011 at 01:02:01PM -0700, Jay Vosburgh wrote: > > Since you #define SPEED_UNKNOWN to -1 (below), how does this > actually change anything? Did you mean 0xffff (because struct > ethtool_cmd's speed is a u16)? > Sorry I could have explained this better in the changelog. The slave->speed is stored in a u32 and the -1 works fine as is. Obviously, as you point out the define doesn't change anything. I just changed it so it would look symetric with DUPLEX_UNKNOWN. But I think you missed that I defined #define DUPLEX_UNKNOWN 0xff. Before it we used a -1 for both and that didn't work. I can resend this with a note about the SPEED_UNKNOWN cleanup if you'd like. I'll do that tomorrow or Sunday. regards, dan carpenter
From: Dan Carpenter <dan.carpenter@oracle.com> Date: Fri, 4 Nov 2011 23:53:51 +0300 > On Fri, Nov 04, 2011 at 01:02:01PM -0700, Jay Vosburgh wrote: >> >> Since you #define SPEED_UNKNOWN to -1 (below), how does this >> actually change anything? Did you mean 0xffff (because struct >> ethtool_cmd's speed is a u16)? >> > > Sorry I could have explained this better in the changelog. The > slave->speed is stored in a u32 and the -1 works fine as is. > Obviously, as you point out the define doesn't change anything. I > just changed it so it would look symetric with DUPLEX_UNKNOWN. > > But I think you missed that I defined #define DUPLEX_UNKNOWN 0xff. > Before it we used a -1 for both and that didn't work. > > I can resend this with a note about the SPEED_UNKNOWN cleanup if > you'd like. I'll do that tomorrow or Sunday. That won't be necessary, I'll apply your patch as-is. Thanks Dan. -- 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/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index b2b9109..b0c5772 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -560,8 +560,8 @@ static int bond_update_speed_duplex(struct slave *slave) u32 slave_speed; int res; - slave->speed = -1; - slave->duplex = -1; + slave->speed = SPEED_UNKNOWN; + slave->duplex = DUPLEX_UNKNOWN; res = __ethtool_get_settings(slave_dev, &ecmd); if (res < 0) diff --git a/drivers/net/bonding/bond_procfs.c b/drivers/net/bonding/bond_procfs.c index 2acf0b0..ad284ba 100644 --- a/drivers/net/bonding/bond_procfs.c +++ b/drivers/net/bonding/bond_procfs.c @@ -158,12 +158,12 @@ 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"); - if (slave->speed == -1) + if (slave->speed == SPEED_UNKNOWN) seq_printf(seq, "Speed: %s\n", "Unknown"); else seq_printf(seq, "Speed: %d Mbps\n", slave->speed); - if (slave->duplex == -1) + if (slave->duplex == DUPLEX_UNKNOWN) seq_printf(seq, "Duplex: %s\n", "Unknown"); else seq_printf(seq, "Duplex: %s\n", slave->duplex ? "full" : "half"); diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index 45f00b6..de33de1 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -1097,10 +1097,12 @@ struct ethtool_ops { #define SPEED_1000 1000 #define SPEED_2500 2500 #define SPEED_10000 10000 +#define SPEED_UNKNOWN -1 /* Duplex, half or full. */ #define DUPLEX_HALF 0x00 #define DUPLEX_FULL 0x01 +#define DUPLEX_UNKNOWN 0xff /* Which connector port. */ #define PORT_TP 0x00
slave->duplex is a u8 type so the in bond_info_show_slave() when we check "if (slave->duplex == -1)", it's always false. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> -- 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