Message ID | 1303001651-4074-1-git-send-email-decot@google.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Sat, 2011-04-16 at 17:54 -0700, David Decotigny wrote: > The 'ep' argument of ethtool_cmd_speed is not altered: advertise it in > protoype. +Indentation fix. Also add comments to advise using the > ethtool_cmd_speed API to get/set the link speed. > > Signed-off-by: David Decotigny <decot@google.com> > --- > include/linux/ethtool.h | 14 ++++++++++---- > 1 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index 9de3127..170439e 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -24,7 +24,10 @@ struct ethtool_cmd { > __u32 cmd; > __u32 supported; /* Features this interface supports */ > __u32 advertising; /* Features this interface advertises */ > - __u16 speed; /* The forced speed, 10Mb, 100Mb, gigabit */ > + __u16 speed; /* The forced speed (lower bits): see > + * SPEED_ macros below. Please use > + * ethtool_cmd_speed()/_set() to > + * access it */ [...] Don't encourage use of the SPEED_* macros. The speed is just a value in units of Mbit/s. Ben.
Hi all, This is the second version of a patch series aimed at making sure the speed field of struct ethtool_cmd is accessed as a 32b integer throughout the kernel. Thanks to Ben for his careful review! Below are a few comments before sending the series. On 04/16/11 19:06, Ben Hutchings wrote: > Don't encourage use of the SPEED_* macros. The speed is just a value in > units of Mbit/s. Updated. Thanks! >> This makes sure that when a driver calls the ethtool's >> get/set_settings() callback of another driver, > > Many of them are calling themselves, and they can ignore speed_hi > because they will never set it. Right. Here is what I replied to Ben in private: I think it makes sense to have a uniform & consistent use of the API without exception nor assumption, even though it borrows additional useless CPU cycles (+ cache lines probably). This comes down to trade-off between code management and performance, for which I privileged the former because I don't think the ethtool api is supposed to be heavily used throughout normal kernel operation. I left the initial changes in place. >> the data passed to it >> is clean. This guarantees that speed_hi will be zeroed correctly if >> the called callback doesn't explicitely set it: we are sure we don't >> get a corrupted speed from the underlying driver. We also take care of >> setting the cmd field appropriately (ETHTOOL_GSET/SSET). > > I think this initialisation ought to be done in > dev_ethtool_get_settings(), and that function moved into net/core/dev.c, > to avoid code bloat. (Yes it's minimal in this case, but these things > add up.) Done. This is in the 2nd patch of the series. Any feedback welcome, I am not entirely convinced that my additions are pertinent. > Maybe also in mii_ethtool_gset(). I don't think this is equivalent to dev_ethtool_get_settings(). My understanding is that mii_ethtool_gset() (and likewise for mdio45_) is meant to be wrapped almost transparently inside a driver's get_settings(), so it should make the same assumptions as a normal get_settings(). However, dev_ethtool_get_settings() is meant to be used to _call_ some driver's get_settings(), so it should make sure that it enforces the assumptions this driver is making. In the end, I didn't change mii_ethtool_gset() the same way I did for dev_ethtool_get_settings (no memset). >> --- a/arch/mips/txx9/generic/setup_tx4939.c >> +++ b/arch/mips/txx9/generic/setup_tx4939.c [...] > If you're going to rewrite the whole function then at least get rid of > this stupid conditional and return ethtool_cmd_speed(&cmd). Updated. Thanks! [...] > e100 is just getting its own settings and doesn't need to be changed. Right. I still did change it for the sake of consistency (see remark above). [...] >> diff --git a/drivers/net/pcnet32.c b/drivers/net/pcnet32.c >> @@ -2763,11 +2763,12 @@ static void pcnet32_check_media(struct net_device *dev, int verbose) >> netif_carrier_on(dev); >> if (lp->mii) { >> if (netif_msg_link(lp)) { >> - struct ethtool_cmd ecmd; >> + struct ethtool_cmd ecmd = { >> + .cmd = ETHTOOL_GSET }; >> mii_ethtool_gset(&lp->mii_if, &ecmd); >> netdev_info(dev, "link up, %sMbps, %s-duplex\n", >> - (ecmd.speed == SPEED_100) >> - ? "100" : "10", >> + (ethtool_cmd_speed(&ecmd) >> + == SPEED_100) ? "100" : "10", > > Just use ethtool_cmd_speed() and %u rather than this conditional. Updated. Thanks! [...] >> diff --git a/drivers/net/stmmac/stmmac_ethtool.c b/drivers/net/stmmac/stmmac_ethtool.c >> index 0e61ac8..33cf0b1 100644 >> --- a/drivers/net/stmmac/stmmac_ethtool.c >> +++ b/drivers/net/stmmac/stmmac_ethtool.c >> @@ -237,13 +237,13 @@ stmmac_set_pauseparam(struct net_device *netdev, >> >> if (phy->autoneg) { >> if (netif_running(netdev)) { >> - struct ethtool_cmd cmd; >> + struct ethtool_cmd cmd = { .cmd = ETHTOOL_SSET }; >> /* auto-negotiation automatically restarted */ >> cmd.cmd = ETHTOOL_NWAY_RST; > > This line immediately changes cmd.cmd. The original author is clearly > confused, as the phy_ethtool_sset() ignores cmd.cmd and ETHTOOL_NWAY_RST > does not take any parameters. True. I made this a bit more elegant. > I think this whole block should really just be: > > ret = phy_start_aneg(phy); Right, but I prefer not to do this in this series. > [...] >> diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c >> index e2e6475..6df9540 100644 >> --- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c >> +++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c >> @@ -664,22 +664,28 @@ static void bnx2fc_link_speed_update(struct fc_lport *lport) [...] > This restructuring looks entirely unnecessary, and obscures the real > change being made. That's definitely true. Made my changes more transparent. Thanks! >> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c >> index bde6ee5..ba9e84a 100644 >> --- a/drivers/scsi/fcoe/fcoe.c >> +++ b/drivers/scsi/fcoe/fcoe.c >> @@ -2026,25 +2026,30 @@ out_nodev: >> int fcoe_link_speed_update(struct fc_lport *lport) >> { > [...] > > Ditto here. Same here. On 04/16/11 19:02, Ben Hutchings wrote: > On Sat, 2011-04-16 at 17:54 -0700, David Decotigny wrote: >> This makes sure the ethtool's set_settings() callback of network >> drivers don't ignore the 16 most significant bits when ethtool calls >> their set_settings(). >> >> All the driver compiled with make allyesconfig on x86_64 have been >> updated. > > You missed one generic function, mdio45_ethtool_gset_npage() in > drivers/net/mdio.c. I am not sure about this one. But I think it falls in the same category as mii_ethtool_gset() above. > > [...] >> diff --git a/drivers/net/dl2k.c b/drivers/net/dl2k.c >> index c05db60..ba6c151 100644 >> --- a/drivers/net/dl2k.c >> +++ b/drivers/net/dl2k.c >> @@ -1219,11 +1219,11 @@ static int rio_set_settings(struct net_device *dev, struct ethtool_cmd *cmd) >> } else { >> np->an_enable = 0; >> if (np->speed == 1000) { >> - cmd->speed = SPEED_100; >> + ethtool_cmd_speed_set(cmd, SPEED_100); >> cmd->duplex = DUPLEX_FULL; >> printk("Warning!! Can't disable Auto negotiation in 1000Mbps, change to Manual 100Mbps, Full duplex.\n"); >> } >> - switch(cmd->speed + cmd->duplex) { >> + switch (ethtool_cmd_speed(cmd) + cmd->duplex) { > [...] > > If you're going to stop these drivers ignoring speed_hi, maybe you > should also stop them ignoring the difference between speed and duplex! > Currently the user can ask for 99 Mbit/s full-duplex and get 100 Mbit/s > half-duplex. Yes, that is completely right. Updated. Thanks! > > There are several others that use this trick. Yes, I noticed too (e1000(e)) but I think it would deserve another set of patches. David Decotigny (4): ethtool: cosmetics: enforce const-ness in ethtool_cmd_speed ethtool: Call ethtool's get/set_settings callbacks with cleaned data ethtool: Use the full 32 bit speed range in ethtool's set_settings ethtool: cosmetic: Use ethtool ethtool_cmd_speed API arch/mips/txx9/generic/setup_tx4939.c | 21 ++++++----------- drivers/infiniband/hw/nes/nes_nic.c | 4 +- drivers/net/3c509.c | 2 +- drivers/net/acenic.c | 10 ++++---- drivers/net/arm/etherh.c | 5 ++- drivers/net/arm/ks8695net.c | 7 +++-- drivers/net/atl1c/atl1c_ethtool.c | 9 ++++--- drivers/net/atl1e/atl1e_ethtool.c | 4 +- drivers/net/atlx/atl1.c | 9 ++++--- drivers/net/atlx/atl2.c | 4 +- drivers/net/b44.c | 13 ++++++----- drivers/net/bcm63xx_enet.c | 3 +- drivers/net/benet/be_ethtool.c | 16 ++++++------ drivers/net/bna/bnad_ethtool.c | 7 +++-- drivers/net/bnx2.c | 19 +++++++-------- drivers/net/cassini.c | 37 ++++++++++++++++--------------- drivers/net/chelsio/cxgb2.c | 11 +++++---- drivers/net/cxgb3/cxgb3_main.c | 14 ++++++----- drivers/net/cxgb4/cxgb4_main.c | 14 ++++++----- drivers/net/cxgb4vf/cxgb4vf_main.c | 3 +- drivers/net/dl2k.c | 29 ++++++++++-------------- drivers/net/e100.c | 10 +++++--- drivers/net/e1000/e1000_ethtool.c | 10 +++++--- drivers/net/e1000e/ethtool.c | 15 +++++++----- drivers/net/eepro.c | 2 +- drivers/net/ehea/ehea_ethtool.c | 23 +++++++++++++++---- drivers/net/enc28j60.c | 5 ++- drivers/net/enic/enic_main.c | 4 +- drivers/net/ewrk3.c | 2 +- drivers/net/forcedeth.c | 25 ++++++++++++-------- drivers/net/ibmveth.c | 2 +- drivers/net/igb/igb_ethtool.c | 11 +++++---- drivers/net/igbvf/ethtool.c | 8 +++--- drivers/net/ixgb/ixgb_ethtool.c | 7 +++-- drivers/net/ixgbe/ixgbe_ethtool.c | 11 +++++---- drivers/net/ixgbevf/ethtool.c | 8 ++++-- drivers/net/jme.c | 3 +- drivers/net/ksz884x.c | 9 ++++--- drivers/net/mdio.c | 22 +++++++++++------ drivers/net/mii.c | 31 ++++++++++++++++---------- drivers/net/mlx4/en_ethtool.c | 7 +++-- drivers/net/mv643xx_eth.c | 6 ++-- drivers/net/myri10ge/myri10ge.c | 2 +- drivers/net/natsemi.c | 11 +++++---- drivers/net/netxen/netxen_nic_ethtool.c | 15 ++++++------ drivers/net/niu.c | 4 +- drivers/net/ns83820.c | 8 +++--- drivers/net/pch_gbe/pch_gbe_ethtool.c | 9 ++++--- drivers/net/pch_gbe/pch_gbe_main.c | 6 ++-- drivers/net/pch_gbe/pch_gbe_phy.c | 4 +- drivers/net/pcmcia/smc91c92_cs.c | 6 ++-- drivers/net/pcnet32.c | 16 ++++++------ drivers/net/phy/phy.c | 12 ++++++---- drivers/net/ps3_gelic_net.c | 8 +++--- drivers/net/qla3xxx.c | 2 +- drivers/net/qlcnic/qlcnic_ethtool.c | 12 +++++----- drivers/net/qlge/qlge_ethtool.c | 2 +- drivers/net/r8169.c | 5 ++- drivers/net/s2io.c | 6 ++-- drivers/net/sc92031.c | 8 ++++-- drivers/net/sfc/ethtool.c | 5 ++- drivers/net/sfc/mcdi_phy.c | 6 ++-- drivers/net/sfc/mdio_10g.c | 4 +- drivers/net/sfc/tenxpress.c | 2 +- drivers/net/skge.c | 6 ++-- drivers/net/sky2.c | 8 +++--- drivers/net/smc911x.c | 4 +- drivers/net/smc91x.c | 4 +- drivers/net/spider_net_ethtool.c | 2 +- drivers/net/stmmac/stmmac_ethtool.c | 5 +-- drivers/net/sungem.c | 15 ++++++------ drivers/net/sunhme.c | 19 +++++++-------- drivers/net/tehuti.c | 2 +- drivers/net/tg3.c | 13 ++++++----- drivers/net/tulip/de2104x.c | 11 +++++---- drivers/net/tulip/uli526x.c | 6 ++-- drivers/net/tun.c | 2 +- drivers/net/typhoon.c | 19 ++++++++------- drivers/net/usb/asix.c | 28 ++++++++++++----------- drivers/net/usb/catc.c | 2 +- drivers/net/usb/dm9601.c | 6 ++-- drivers/net/usb/rtl8150.c | 11 +++++---- drivers/net/usb/smsc75xx.c | 7 +++-- drivers/net/usb/smsc95xx.c | 7 +++-- drivers/net/veth.c | 2 +- drivers/net/via-velocity.c | 21 +++++++++++------ drivers/net/vmxnet3/vmxnet3_ethtool.c | 4 +- drivers/net/vxge/vxge-ethtool.c | 7 +++-- drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 11 ++++++--- drivers/scsi/fcoe/fcoe.c | 11 ++++++--- include/linux/ethtool.h | 18 +++++++++++---- include/linux/netdevice.h | 9 +------ include/rdma/ib_addr.h | 13 ++++++----- net/batman-adv/soft-interface.c | 2 +- net/bridge/br_if.c | 4 +- net/core/dev.c | 24 ++++++++++++++++++++ net/core/net-sysfs.c | 24 ++++++++----------- 97 files changed, 522 insertions(+), 420 deletions(-)
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index 9de3127..170439e 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -24,7 +24,10 @@ struct ethtool_cmd { __u32 cmd; __u32 supported; /* Features this interface supports */ __u32 advertising; /* Features this interface advertises */ - __u16 speed; /* The forced speed, 10Mb, 100Mb, gigabit */ + __u16 speed; /* The forced speed (lower bits): see + * SPEED_ macros below. Please use + * ethtool_cmd_speed()/_set() to + * access it */ __u8 duplex; /* Duplex, half or full */ __u8 port; /* Which connector port */ __u8 phy_address; @@ -33,7 +36,10 @@ struct ethtool_cmd { __u8 mdio_support; __u32 maxtxpkt; /* Tx pkts before generating tx int */ __u32 maxrxpkt; /* Rx pkts before generating rx int */ - __u16 speed_hi; + __u16 speed_hi; /* The forced speed (upper + * bits). Please use + * ethtool_cmd_speed()/_set() to + * access it */ __u8 eth_tp_mdix; __u8 reserved2; __u32 lp_advertising; /* Features the link partner advertises */ @@ -41,14 +47,14 @@ struct ethtool_cmd { }; static inline void ethtool_cmd_speed_set(struct ethtool_cmd *ep, - __u32 speed) + __u32 speed) { ep->speed = (__u16)speed; ep->speed_hi = (__u16)(speed >> 16); } -static inline __u32 ethtool_cmd_speed(struct ethtool_cmd *ep) +static inline __u32 ethtool_cmd_speed(const struct ethtool_cmd *ep) { return (ep->speed_hi << 16) | ep->speed; }
The 'ep' argument of ethtool_cmd_speed is not altered: advertise it in protoype. +Indentation fix. Also add comments to advise using the ethtool_cmd_speed API to get/set the link speed. Signed-off-by: David Decotigny <decot@google.com> --- include/linux/ethtool.h | 14 ++++++++++---- 1 files changed, 10 insertions(+), 4 deletions(-)