| Message ID | 1361003616-3422-14-git-send-email-jeffrey.t.kirsher@intel.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
2013/2/16 Jeff Kirsher <jeffrey.t.kirsher@intel.com>: > From: Aurélien Guillaume <footplus@gmail.com> > > This patch adds support for reading data from SFP+ modules over i2c. > > Signed-off-by: Aurélien Guillaume <footplus@gmail.com> > Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com> > Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > --- > drivers/net/ethernet/intel/ixgbe/ixgbe.h | 1 + > drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 114 +++++++++++++++++++++++ > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 + > 3 files changed, 119 insertions(+) > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h > index b91f9b6..196002b 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h > @@ -620,6 +620,7 @@ enum ixgbe_state_t { > __IXGBE_DOWN, > __IXGBE_SERVICE_SCHED, > __IXGBE_IN_SFP_INIT, > + __IXGBE_READ_I2C, > }; > > struct ixgbe_cb { > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c > index 7349a8b..e6cebdc 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c > @@ -39,6 +39,7 @@ > #include <linux/uaccess.h> > > #include "ixgbe.h" > +#include "ixgbe_phy.h" > > > #define IXGBE_ALL_RAR_ENTRIES 16 > @@ -2839,6 +2840,117 @@ static int ixgbe_set_channels(struct net_device *dev, > return ixgbe_setup_tc(dev, netdev_get_num_tc(dev)); > } > > +static int ixgbe_get_module_info(struct net_device *dev, > + struct ethtool_modinfo *modinfo) > +{ > + struct ixgbe_adapter *adapter = netdev_priv(dev); > + struct ixgbe_hw *hw = &adapter->hw; > + u32 status; > + u8 sff8472_rev, addr_mode; > + int ret_val = 0; > + bool page_swap = false; > + > + /* avoid concurent i2c reads */ > + while (test_bit(__IXGBE_IN_SFP_INIT, &adapter->state)) > + msleep(100); > + > + /* used by the service task */ > + set_bit(__IXGBE_READ_I2C, &adapter->state); This is racy. Why do you need another bit? while (test_and_set_bit(__IXGBE_IN_SFP_INIT, &adapter->state)) msleep(100); ... clear_bit(__IXGBE_IN_SFP_INIT, &adapter->state) Best Regards, Michał Mirosław -- 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
>-----Original Message----- >From: Michał Mirosław [mailto:mirqus@gmail.com] >Sent: Saturday, February 16, 2013 2:54 AM >To: Kirsher, Jeffrey T >Cc: davem@davemloft.net; Aurélien Guillaume; netdev@vger.kernel.org; >gospo@redhat.com; sassmann@redhat.com; Tantilov, Emil S >Subject: Re: [net-next 13/15] ixgbe: implement SFF diagnostic monitoring >via ethtool > >2013/2/16 Jeff Kirsher <jeffrey.t.kirsher@intel.com>: >> From: Aurélien Guillaume <footplus@gmail.com> >> >> This patch adds support for reading data from SFP+ modules over i2c. >> >> Signed-off-by: Aurélien Guillaume <footplus@gmail.com> >> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com> >> Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com> >> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> >> --- >> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 1 + >> drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 114 >+++++++++++++++++++++++ >> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 + >> 3 files changed, 119 insertions(+) >> >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h >b/drivers/net/ethernet/intel/ixgbe/ixgbe.h >> index b91f9b6..196002b 100644 >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h >> @@ -620,6 +620,7 @@ enum ixgbe_state_t { >> __IXGBE_DOWN, >> __IXGBE_SERVICE_SCHED, >> __IXGBE_IN_SFP_INIT, >> + __IXGBE_READ_I2C, >> }; >> >> struct ixgbe_cb { >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c >b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c >> index 7349a8b..e6cebdc 100644 >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c >> @@ -39,6 +39,7 @@ >> #include <linux/uaccess.h> >> >> #include "ixgbe.h" >> +#include "ixgbe_phy.h" >> >> >> #define IXGBE_ALL_RAR_ENTRIES 16 >> @@ -2839,6 +2840,117 @@ static int ixgbe_set_channels(struct net_device >*dev, >> return ixgbe_setup_tc(dev, netdev_get_num_tc(dev)); >> } >> >> +static int ixgbe_get_module_info(struct net_device *dev, >> + struct ethtool_modinfo *modinfo) >> +{ >> + struct ixgbe_adapter *adapter = netdev_priv(dev); >> + struct ixgbe_hw *hw = &adapter->hw; >> + u32 status; >> + u8 sff8472_rev, addr_mode; >> + int ret_val = 0; >> + bool page_swap = false; >> + >> + /* avoid concurent i2c reads */ >> + while (test_bit(__IXGBE_IN_SFP_INIT, &adapter->state)) >> + msleep(100); >> + >> + /* used by the service task */ >> + set_bit(__IXGBE_READ_I2C, &adapter->state); > >This is racy. Why do you need another bit? The I2C bit helps to reduce the delay in the service task relative to the initialization of the SFP modules. > > while (test_and_set_bit(__IXGBE_IN_SFP_INIT, &adapter->state)) > msleep(100); >... > clear_bit(__IXGBE_IN_SFP_INIT, &adapter->state) This is what I had initially, but the i2c reads can take a long time on some parts and __IXGBE_IN_SFP_INIT protects portions of the code that have nothing to do with I2C reads. Setting __IXGBE_IN_SFP_INIT in ethtool while dumping the SFF data can introduce needlessly long delays in the SFP initialization path. Thanks, Emil >Best Regards, >Michał Mirosław
2013/2/17 Tantilov, Emil S <emil.s.tantilov@intel.com>: >>> @@ -2839,6 +2840,117 @@ static int ixgbe_set_channels(struct net_device >>*dev, >>> return ixgbe_setup_tc(dev, netdev_get_num_tc(dev)); >>> } >>> >>> +static int ixgbe_get_module_info(struct net_device *dev, >>> + struct ethtool_modinfo *modinfo) >>> +{ >>> + struct ixgbe_adapter *adapter = netdev_priv(dev); >>> + struct ixgbe_hw *hw = &adapter->hw; >>> + u32 status; >>> + u8 sff8472_rev, addr_mode; >>> + int ret_val = 0; >>> + bool page_swap = false; >>> + >>> + /* avoid concurent i2c reads */ >>> + while (test_bit(__IXGBE_IN_SFP_INIT, &adapter->state)) >>> + msleep(100); >>> + >>> + /* used by the service task */ >>> + set_bit(__IXGBE_READ_I2C, &adapter->state); >> >>This is racy. Why do you need another bit? > > The I2C bit helps to reduce the delay in the service task relative to the initialization of the SFP modules. > >> >> while (test_and_set_bit(__IXGBE_IN_SFP_INIT, &adapter->state)) >> msleep(100); >>... >> clear_bit(__IXGBE_IN_SFP_INIT, &adapter->state) > > This is what I had initially, but the i2c reads can take a long time on some parts and __IXGBE_IN_SFP_INIT protects portions of the code that have nothing to do with I2C reads. Setting __IXGBE_IN_SFP_INIT in ethtool while dumping the SFF data can introduce needlessly long delays in the SFP initialization path. Maybe it would be enough to protect the body of read_i2c_eeprom() usign a mutex? It seems you want __IXGBE_READ_I2C to work like a mutex, but you test it in non-atomic way. Best Regards, Michał Mirosław -- 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 Sat, 2013-02-16 at 00:33 -0800, Jeff Kirsher wrote: > From: Aurélien Guillaume <footplus@gmail.com> > > This patch adds support for reading data from SFP+ modules over i2c. [...] > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c [...] > +static int ixgbe_get_module_eeprom(struct net_device *dev, > + struct ethtool_eeprom *ee, > + u8 *data) > +{ > + struct ixgbe_adapter *adapter = netdev_priv(dev); > + struct ixgbe_hw *hw = &adapter->hw; > + u32 status = IXGBE_ERR_PHY_ADDR_INVALID; > + u8 databyte = 0xFF; > + int i = 0; > + int ret_val = 0; > + > + /* ixgbe_get_module_info is called before this function in all > + * cases, so we do not need any checks we already do above, > + * and can trust ee->len to be a known value. > + */ [...] Whatever makes you think that? All you are guaranteed is that: ee->offset <= ee->offset + ee->len <= eeprom_len Same as for the regular EEPROM read function. This implementation doesn't result in a heap overrun at present because the caller allocates a whole page as a buffer. But you should not assume that it's safe to write more than ee->len bytes, nor that ee->offset == 0. Ben.
>-----Original Message----- >From: Ben Hutchings [mailto:bhutchings@solarflare.com] >Sent: Tuesday, February 19, 2013 2:14 PM >To: Kirsher, Jeffrey T; Aurélien Guillaume >Cc: davem@davemloft.net; netdev@vger.kernel.org; gospo@redhat.com; >sassmann@redhat.com; Tantilov, Emil S >Subject: Re: [net-next 13/15] ixgbe: implement SFF diagnostic monitoring >via ethtool > >On Sat, 2013-02-16 at 00:33 -0800, Jeff Kirsher wrote: >> From: Aurélien Guillaume <footplus@gmail.com> >> >> This patch adds support for reading data from SFP+ modules over i2c. >[...] >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c >[...] >> +static int ixgbe_get_module_eeprom(struct net_device *dev, >> + struct ethtool_eeprom *ee, >> + u8 *data) >> +{ >> + struct ixgbe_adapter *adapter = netdev_priv(dev); >> + struct ixgbe_hw *hw = &adapter->hw; >> + u32 status = IXGBE_ERR_PHY_ADDR_INVALID; >> + u8 databyte = 0xFF; >> + int i = 0; >> + int ret_val = 0; >> + >> + /* ixgbe_get_module_info is called before this function in all >> + * cases, so we do not need any checks we already do above, >> + * and can trust ee->len to be a known value. >> + */ >[...] > >Whatever makes you think that? > >All you are guaranteed is that: > ee->offset <= ee->offset + ee->len <= eeprom_len >Same as for the regular EEPROM read function. > >This implementation doesn't result in a heap overrun at present because >the caller allocates a whole page as a buffer. But you should not >assume that it's safe to write more than ee->len bytes, nor that >ee->offset == 0. > >Ben. Thanks Ben, we'll address this in a follow up patch. Emil
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h index b91f9b6..196002b 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h @@ -620,6 +620,7 @@ enum ixgbe_state_t { __IXGBE_DOWN, __IXGBE_SERVICE_SCHED, __IXGBE_IN_SFP_INIT, + __IXGBE_READ_I2C, }; struct ixgbe_cb { diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c index 7349a8b..e6cebdc 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c @@ -39,6 +39,7 @@ #include <linux/uaccess.h> #include "ixgbe.h" +#include "ixgbe_phy.h" #define IXGBE_ALL_RAR_ENTRIES 16 @@ -2839,6 +2840,117 @@ static int ixgbe_set_channels(struct net_device *dev, return ixgbe_setup_tc(dev, netdev_get_num_tc(dev)); } +static int ixgbe_get_module_info(struct net_device *dev, + struct ethtool_modinfo *modinfo) +{ + struct ixgbe_adapter *adapter = netdev_priv(dev); + struct ixgbe_hw *hw = &adapter->hw; + u32 status; + u8 sff8472_rev, addr_mode; + int ret_val = 0; + bool page_swap = false; + + /* avoid concurent i2c reads */ + while (test_bit(__IXGBE_IN_SFP_INIT, &adapter->state)) + msleep(100); + + /* used by the service task */ + set_bit(__IXGBE_READ_I2C, &adapter->state); + + /* Check whether we support SFF-8472 or not */ + status = hw->phy.ops.read_i2c_eeprom(hw, + IXGBE_SFF_SFF_8472_COMP, + &sff8472_rev); + if (status != 0) { + ret_val = -EIO; + goto err_out; + } + + /* addressing mode is not supported */ + status = hw->phy.ops.read_i2c_eeprom(hw, + IXGBE_SFF_SFF_8472_SWAP, + &addr_mode); + if (status != 0) { + ret_val = -EIO; + goto err_out; + } + + if (addr_mode & IXGBE_SFF_ADDRESSING_MODE) { + e_err(drv, "Address change required to access page 0xA2, but not supported. Please report the module type to the driver maintainers.\n"); + page_swap = true; + } + + if (sff8472_rev == IXGBE_SFF_SFF_8472_UNSUP || page_swap) { + /* We have a SFP, but it does not support SFF-8472 */ + modinfo->type = ETH_MODULE_SFF_8079; + modinfo->eeprom_len = ETH_MODULE_SFF_8079_LEN; + } else { + /* We have a SFP which supports a revision of SFF-8472. */ + modinfo->type = ETH_MODULE_SFF_8472; + modinfo->eeprom_len = ETH_MODULE_SFF_8472_LEN; + } + +err_out: + clear_bit(__IXGBE_READ_I2C, &adapter->state); + return ret_val; +} + +static int ixgbe_get_module_eeprom(struct net_device *dev, + struct ethtool_eeprom *ee, + u8 *data) +{ + struct ixgbe_adapter *adapter = netdev_priv(dev); + struct ixgbe_hw *hw = &adapter->hw; + u32 status = IXGBE_ERR_PHY_ADDR_INVALID; + u8 databyte = 0xFF; + int i = 0; + int ret_val = 0; + + /* ixgbe_get_module_info is called before this function in all + * cases, so we do not need any checks we already do above, + * and can trust ee->len to be a known value. + */ + + while (test_bit(__IXGBE_IN_SFP_INIT, &adapter->state)) + msleep(100); + set_bit(__IXGBE_READ_I2C, &adapter->state); + + /* Read the first block, SFF-8079 */ + for (i = 0; i < ETH_MODULE_SFF_8079_LEN; i++) { + status = hw->phy.ops.read_i2c_eeprom(hw, i, &databyte); + if (status != 0) { + /* Error occured while reading module */ + ret_val = -EIO; + goto err_out; + } + data[i] = databyte; + } + + /* If the second block is requested, check if SFF-8472 is supported. */ + if (ee->len == ETH_MODULE_SFF_8472_LEN) { + if (data[IXGBE_SFF_SFF_8472_COMP] == IXGBE_SFF_SFF_8472_UNSUP) + return -EOPNOTSUPP; + + /* Read the second block, SFF-8472 */ + for (i = ETH_MODULE_SFF_8079_LEN; + i < ETH_MODULE_SFF_8472_LEN; i++) { + status = hw->phy.ops.read_i2c_sff8472(hw, + i - ETH_MODULE_SFF_8079_LEN, &databyte); + if (status != 0) { + /* Error occured while reading module */ + ret_val = -EIO; + goto err_out; + } + data[i] = databyte; + } + } + +err_out: + clear_bit(__IXGBE_READ_I2C, &adapter->state); + + return ret_val; +} + static const struct ethtool_ops ixgbe_ethtool_ops = { .get_settings = ixgbe_get_settings, .set_settings = ixgbe_set_settings, @@ -2870,6 +2982,8 @@ static const struct ethtool_ops ixgbe_ethtool_ops = { .get_channels = ixgbe_get_channels, .set_channels = ixgbe_set_channels, .get_ts_info = ixgbe_get_ts_info, + .get_module_info = ixgbe_get_module_info, + .get_module_eeprom = ixgbe_get_module_eeprom, }; void ixgbe_set_ethtool_ops(struct net_device *netdev) diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c index aea252a..b0b72fc 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c @@ -5709,6 +5709,10 @@ static void ixgbe_sfp_detection_subtask(struct ixgbe_adapter *adapter) !(adapter->flags2 & IXGBE_FLAG2_SFP_NEEDS_RESET)) return; + /* concurent i2c reads are not supported */ + if (test_bit(__IXGBE_READ_I2C, &adapter->state)) + return; + /* someone else is in init, wait until next service event */ if (test_and_set_bit(__IXGBE_IN_SFP_INIT, &adapter->state)) return;