Patchwork [net-next,13/15] ixgbe: implement SFF diagnostic monitoring via ethtool

login
register
mail settings
Submitter Jeff Kirsher
Date Feb. 16, 2013, 8:33 a.m.
Message ID <1361003616-3422-14-git-send-email-jeffrey.t.kirsher@intel.com>
Download mbox | patch
Permalink /patch/220946/
State Accepted
Delegated to: David Miller
Headers show

Comments

Jeff Kirsher - Feb. 16, 2013, 8:33 a.m.
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(+)
=?ISO-8859-2?Q?Micha=B3_Miros=B3aw?= - Feb. 16, 2013, 10:53 a.m.
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
Tantilov, Emil S - Feb. 17, 2013, 6:16 a.m.
>-----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
=?ISO-8859-2?Q?Micha=B3_Miros=B3aw?= - Feb. 17, 2013, 1:41 p.m.
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
Ben Hutchings - Feb. 19, 2013, 10:14 p.m.
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.
Tantilov, Emil S - Feb. 20, 2013, 12:26 a.m.
>-----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

Patch

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;