[net-next 0/9][pull request] Intel Wired LAN Driver Updates

[net-next 0/9][pull request] Intel Wired LAN Driver Updates

[Jeff Kirsher]

The following series contains updates to igb, ixgbe and ixgbevf.  Most of
the changes are adding support of some kind.  There are 3 fixes, one fix
for ixgbevf to fix register defines.  The other two fixes are for ixgbe,
one being a minor comment spelling fix and the other is to fix register
reads.

Here is a list of the new support added:
 - 2 new device id's in ixgbe
 - igb flow control advertising to ethtool
 - ixgbe thermal data sensor

The following are changes since commit 2c64580046a122fa15bb586d8ca4fd5e4b69a1e7:
  netlink: wake up netlink listeners sooner (v2)
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/net-next master

Alexander Duyck (1):
  ixgbevf: Fix register defines to correctly handle complex expressions

Carolyn Wyborny (1):
  igb: Add flow control advertising to ethtool setting.

Don Skidmore (3):
  ixgbe: add support functions for gathering thermal data sensor
  ixgbe: add interface to export thermal data
  ixgbe: add support for new 82599 device.

Emil Tantilov (3):
  ixgbe: fix incorrect PHY register reads
  ixgbe: add write flush in ixgbe_clock_out_i2c_byte()
  ixgbe: add support for new 82599 device id

Stephen Hemminger (1):
  ixgbe: fix typo's

 drivers/net/ethernet/intel/igb/igb_ethtool.c     |    6 +-
 drivers/net/ethernet/intel/ixgbe/Makefile        |    2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe.h         |    4 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c   |    3 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_common.c  |  158 +++++++++++-
 drivers/net/ethernet/intel/ixgbe/ixgbe_common.h  |   13 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c |   15 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    |   32 ++-
 drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c     |    1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c   |  305 ++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_type.h    |   40 +++
 drivers/net/ethernet/intel/ixgbevf/mbx.h         |    4 +-
 drivers/net/ethernet/intel/ixgbevf/regs.h        |   42 ++--
 13 files changed, 583 insertions(+), 42 deletions(-)
 create mode 100644 drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c

-- 
1.7.7.4

[net-next 1/9] ixgbevf: Fix register defines to correctly handle complex expressions

[Jeff Kirsher]

From: Alexander Duyck <alexander.h.duyck@intel.com>

This patch is meant to address possible issues with the IXGBEVF register
defines generating incorrect values when given a complex expression for the
register offset.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgbevf/mbx.h  |    4 +-
 drivers/net/ethernet/intel/ixgbevf/regs.h |   42 ++++++++++++++--------------
 2 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbevf/mbx.h b/drivers/net/ethernet/intel/ixgbevf/mbx.h
index ea393eb..9d38a94 100644
--- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
+++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h
@@ -47,8 +47,8 @@
 #define IXGBE_VFMAILBOX_RSTD     0x00000080 /* PF has indicated reset done */
 #define IXGBE_VFMAILBOX_R2C_BITS 0x000000B0 /* All read to clear bits */
 
-#define IXGBE_PFMAILBOX(x)          (0x04B00 + (4 * x))
-#define IXGBE_PFMBMEM(vfn)          (0x13000 + (64 * vfn))
+#define IXGBE_PFMAILBOX(x)          (0x04B00 + (4 * (x)))
+#define IXGBE_PFMBMEM(vfn)          (0x13000 + (64 * (vfn)))
 
 #define IXGBE_PFMAILBOX_STS   0x00000001 /* Initiate message send to VF */
 #define IXGBE_PFMAILBOX_ACK   0x00000002 /* Ack message recv'd from VF */
diff --git a/drivers/net/ethernet/intel/ixgbevf/regs.h b/drivers/net/ethernet/intel/ixgbevf/regs.h
index 189200e..5e4d5e5 100644
--- a/drivers/net/ethernet/intel/ixgbevf/regs.h
+++ b/drivers/net/ethernet/intel/ixgbevf/regs.h
@@ -39,29 +39,29 @@
 #define IXGBE_VTEIMC           0x0010C
 #define IXGBE_VTEIAC           0x00110
 #define IXGBE_VTEIAM           0x00114
-#define IXGBE_VTEITR(x)        (0x00820 + (4 * x))
-#define IXGBE_VTIVAR(x)        (0x00120 + (4 * x))
+#define IXGBE_VTEITR(x)        (0x00820 + (4 * (x)))
+#define IXGBE_VTIVAR(x)        (0x00120 + (4 * (x)))
 #define IXGBE_VTIVAR_MISC      0x00140
-#define IXGBE_VTRSCINT(x)      (0x00180 + (4 * x))
-#define IXGBE_VFRDBAL(x)       (0x01000 + (0x40 * x))
-#define IXGBE_VFRDBAH(x)       (0x01004 + (0x40 * x))
-#define IXGBE_VFRDLEN(x)       (0x01008 + (0x40 * x))
-#define IXGBE_VFRDH(x)         (0x01010 + (0x40 * x))
-#define IXGBE_VFRDT(x)         (0x01018 + (0x40 * x))
-#define IXGBE_VFRXDCTL(x)      (0x01028 + (0x40 * x))
-#define IXGBE_VFSRRCTL(x)      (0x01014 + (0x40 * x))
-#define IXGBE_VFRSCCTL(x)      (0x0102C + (0x40 * x))
+#define IXGBE_VTRSCINT(x)      (0x00180 + (4 * (x)))
+#define IXGBE_VFRDBAL(x)       (0x01000 + (0x40 * (x)))
+#define IXGBE_VFRDBAH(x)       (0x01004 + (0x40 * (x)))
+#define IXGBE_VFRDLEN(x)       (0x01008 + (0x40 * (x)))
+#define IXGBE_VFRDH(x)         (0x01010 + (0x40 * (x)))
+#define IXGBE_VFRDT(x)         (0x01018 + (0x40 * (x)))
+#define IXGBE_VFRXDCTL(x)      (0x01028 + (0x40 * (x)))
+#define IXGBE_VFSRRCTL(x)      (0x01014 + (0x40 * (x)))
+#define IXGBE_VFRSCCTL(x)      (0x0102C + (0x40 * (x)))
 #define IXGBE_VFPSRTYPE        0x00300
-#define IXGBE_VFTDBAL(x)       (0x02000 + (0x40 * x))
-#define IXGBE_VFTDBAH(x)       (0x02004 + (0x40 * x))
-#define IXGBE_VFTDLEN(x)       (0x02008 + (0x40 * x))
-#define IXGBE_VFTDH(x)         (0x02010 + (0x40 * x))
-#define IXGBE_VFTDT(x)         (0x02018 + (0x40 * x))
-#define IXGBE_VFTXDCTL(x)      (0x02028 + (0x40 * x))
-#define IXGBE_VFTDWBAL(x)      (0x02038 + (0x40 * x))
-#define IXGBE_VFTDWBAH(x)      (0x0203C + (0x40 * x))
-#define IXGBE_VFDCA_RXCTRL(x)  (0x0100C + (0x40 * x))
-#define IXGBE_VFDCA_TXCTRL(x)  (0x0200c + (0x40 * x))
+#define IXGBE_VFTDBAL(x)       (0x02000 + (0x40 * (x)))
+#define IXGBE_VFTDBAH(x)       (0x02004 + (0x40 * (x)))
+#define IXGBE_VFTDLEN(x)       (0x02008 + (0x40 * (x)))
+#define IXGBE_VFTDH(x)         (0x02010 + (0x40 * (x)))
+#define IXGBE_VFTDT(x)         (0x02018 + (0x40 * (x)))
+#define IXGBE_VFTXDCTL(x)      (0x02028 + (0x40 * (x)))
+#define IXGBE_VFTDWBAL(x)      (0x02038 + (0x40 * (x)))
+#define IXGBE_VFTDWBAH(x)      (0x0203C + (0x40 * (x)))
+#define IXGBE_VFDCA_RXCTRL(x)  (0x0100C + (0x40 * (x)))
+#define IXGBE_VFDCA_TXCTRL(x)  (0x0200c + (0x40 * (x)))
 #define IXGBE_VFGPRC           0x0101C
 #define IXGBE_VFGPTC           0x0201C
 #define IXGBE_VFGORC_LSB       0x01020
-- 
1.7.7.4

[net-next 2/9] igb: Add flow control advertising to ethtool setting.

[Jeff Kirsher]

From: Carolyn Wyborny <carolyn.wyborny@intel.com>

Added pause flag for bi-directional flow control advertising to ethtool
settings.

Signed-off-by: Carolyn Wyborny <carolyn.wyborny@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_ethtool.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index e9335ef..f1206be 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -148,7 +148,8 @@ static int igb_get_settings(struct net_device *netdev, struct ethtool_cmd *ecmd)
 				   SUPPORTED_1000baseT_Full|
 				   SUPPORTED_Autoneg |
 				   SUPPORTED_TP);
-		ecmd->advertising = ADVERTISED_TP;
+		ecmd->advertising = (ADVERTISED_TP |
+				     ADVERTISED_Pause);
 
 		if (hw->mac.autoneg == 1) {
 			ecmd->advertising |= ADVERTISED_Autoneg;
@@ -165,7 +166,8 @@ static int igb_get_settings(struct net_device *netdev, struct ethtool_cmd *ecmd)
 
 		ecmd->advertising = (ADVERTISED_1000baseT_Full |
 				     ADVERTISED_FIBRE |
-				     ADVERTISED_Autoneg);
+				     ADVERTISED_Autoneg |
+				     ADVERTISED_Pause);
 
 		ecmd->port = PORT_FIBRE;
 	}
-- 
1.7.7.4

[net-next 3/9] ixgbe: fix incorrect PHY register reads

[Jeff Kirsher]

From: Emil Tantilov <emil.s.tantilov@intel.com>

Fix some register reads that had the opcode and register parameters swapped.
Also use define instead of a magic (0x3) number.

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_common.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
index bdf535a..a3aa633 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
@@ -266,10 +266,10 @@ s32 ixgbe_clear_hw_cntrs_generic(struct ixgbe_hw *hw)
 	if (hw->mac.type == ixgbe_mac_X540) {
 		if (hw->phy.id == 0)
 			hw->phy.ops.identify(hw);
-		hw->phy.ops.read_reg(hw, 0x3, IXGBE_PCRC8ECL, &i);
-		hw->phy.ops.read_reg(hw, 0x3, IXGBE_PCRC8ECH, &i);
-		hw->phy.ops.read_reg(hw, 0x3, IXGBE_LDPCECL, &i);
-		hw->phy.ops.read_reg(hw, 0x3, IXGBE_LDPCECH, &i);
+		hw->phy.ops.read_reg(hw, IXGBE_PCRC8ECL, MDIO_MMD_PCS, &i);
+		hw->phy.ops.read_reg(hw, IXGBE_PCRC8ECH, MDIO_MMD_PCS, &i);
+		hw->phy.ops.read_reg(hw, IXGBE_LDPCECL, MDIO_MMD_PCS, &i);
+		hw->phy.ops.read_reg(hw, IXGBE_LDPCECH, MDIO_MMD_PCS, &i);
 	}
 
 	return 0;
-- 
1.7.7.4

[net-next 4/9] ixgbe: fix typo's

[Jeff Kirsher]

From: Stephen Hemminger <shemminger@vyatta.com>

Saw typo in one message, so decided to run spell checker.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.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_main.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index fcf8d4e..cd1f893 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -146,7 +146,7 @@ static void ixgbe_service_event_complete(struct ixgbe_adapter *adapter)
 {
 	BUG_ON(!test_bit(__IXGBE_SERVICE_SCHED, &adapter->state));
 
-	/* flush memory to make sure state is correct before next watchog */
+	/* flush memory to make sure state is correct before next watchdog */
 	smp_mb__before_clear_bit();
 	clear_bit(__IXGBE_SERVICE_SCHED, &adapter->state);
 }
@@ -2156,7 +2156,7 @@ static irqreturn_t ixgbe_intr(int irq, void *data)
 	IXGBE_WRITE_REG(hw, IXGBE_EIMC, IXGBE_IRQ_CLEAR_MASK);
 
 	/* for NAPI, using EIAM to auto-mask tx/rx interrupt bits on read
-	 * therefore no explict interrupt disable is necessary */
+	 * therefore no explicit interrupt disable is necessary */
 	eicr = IXGBE_READ_REG(hw, IXGBE_EICR);
 	if (!eicr) {
 		/*
@@ -3606,7 +3606,7 @@ static inline bool ixgbe_is_sfp(struct ixgbe_hw *hw)
 static void ixgbe_sfp_link_config(struct ixgbe_adapter *adapter)
 {
 	/*
-	 * We are assuming the worst case scenerio here, and that
+	 * We are assuming the worst case scenario here, and that
 	 * is that an SFP was inserted/removed after the reset
 	 * but before SFP detection was enabled.  As such the best
 	 * solution is to just start searching as soon as we start
@@ -3828,7 +3828,7 @@ void ixgbe_reset(struct ixgbe_adapter *adapter)
 	case IXGBE_ERR_EEPROM_VERSION:
 		/* We are running on a pre-production device, log a warning */
 		e_dev_warn("This device is a pre-production adapter/LOM. "
-			   "Please be aware there may be issuesassociated with "
+			   "Please be aware there may be issues associated with "
 			   "your hardware.  If you are experiencing problems "
 			   "please contact your Intel or hardware "
 			   "representative who provided you with this "
@@ -5792,9 +5792,9 @@ static void ixgbe_fdir_reinit_subtask(struct ixgbe_adapter *adapter)
  * @adapter - pointer to the device adapter structure
  *
  * This function serves two purposes.  First it strobes the interrupt lines
- * in order to make certain interrupts are occuring.  Secondly it sets the
+ * in order to make certain interrupts are occurring.  Secondly it sets the
  * bits needed to check for TX hangs.  As a result we should immediately
- * determine if a hang has occured.
+ * determine if a hang has occurred.
  */
 static void ixgbe_check_hang_subtask(struct ixgbe_adapter *adapter)
 {
@@ -7132,7 +7132,7 @@ int ixgbe_setup_tc(struct net_device *dev, u8 tc)
 		return -EINVAL;
 
 	/* Hardware has to reinitialize queues and interrupts to
-	 * match packet buffer alignment. Unfortunantly, the
+	 * match packet buffer alignment. Unfortunately, the
 	 * hardware is not flexible enough to do this dynamically.
 	 */
 	if (netif_running(dev))
-- 
1.7.7.4

[net-next 5/9] ixgbe: add write flush in ixgbe_clock_out_i2c_byte()

[Jeff Kirsher]

From: Emil Tantilov <emil.s.tantilov@intel.com>

I2C access is timing critical. Always do a write flush after writing
to the I2CCTL register.

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_phy.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
index 8b113e3..7cf1e1f 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
@@ -1457,6 +1457,7 @@ static s32 ixgbe_clock_out_i2c_byte(struct ixgbe_hw *hw, u8 data)
 	i2cctl = IXGBE_READ_REG(hw, IXGBE_I2CCTL);
 	i2cctl |= IXGBE_I2C_DATA_OUT;
 	IXGBE_WRITE_REG(hw, IXGBE_I2CCTL, i2cctl);
+	IXGBE_WRITE_FLUSH(hw);
 
 	return status;
 }
-- 
1.7.7.4

[net-next 6/9] ixgbe: add support for new 82599 device id

[Jeff Kirsher]

From: Emil Tantilov <emil.s.tantilov@intel.com>

Support for new 82599 based quad port adapter.

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_82599.c |    1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |    1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_type.h  |    1 +
 3 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
index 4ae26a7..7720721 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
@@ -356,6 +356,7 @@ static enum ixgbe_media_type ixgbe_get_media_type_82599(struct ixgbe_hw *hw)
 	case IXGBE_DEV_ID_82599_SFP_FCOE:
 	case IXGBE_DEV_ID_82599_SFP_EM:
 	case IXGBE_DEV_ID_82599_SFP_SF2:
+	case IXGBE_DEV_ID_82599_SFP_SF_QP:
 	case IXGBE_DEV_ID_82599EN_SFP:
 		media_type = ixgbe_media_type_fiber;
 		break;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index cd1f893..e27e4d1 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -106,6 +106,7 @@ static DEFINE_PCI_DEVICE_TABLE(ixgbe_pci_tbl) = {
 	{PCI_VDEVICE(INTEL, IXGBE_DEV_ID_82599_SFP_SF2), board_82599 },
 	{PCI_VDEVICE(INTEL, IXGBE_DEV_ID_82599_LS), board_82599 },
 	{PCI_VDEVICE(INTEL, IXGBE_DEV_ID_82599EN_SFP), board_82599 },
+	{PCI_VDEVICE(INTEL, IXGBE_DEV_ID_82599_SFP_SF_QP), board_82599 },
 	/* required last entry */
 	{0, }
 };
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
index 242643a..7c5817f 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
@@ -65,6 +65,7 @@
 #define IXGBE_SUBDEV_ID_82599_KX4_KR_MEZZ  0x000C
 #define IXGBE_DEV_ID_82599_LS            0x154F
 #define IXGBE_DEV_ID_X540T               0x1528
+#define IXGBE_DEV_ID_82599_SFP_SF_QP     0x154A
 
 /* VF Device IDs */
 #define IXGBE_DEV_ID_82599_VF           0x10ED
-- 
1.7.7.4

[net-next 7/9] ixgbe: add support functions for gathering thermal data sensor

[Jeff Kirsher]

From: Don Skidmore <donald.c.skidmore@intel.com>

Some 82599 adapters contain thermal data that we can get to via
an i2c interface.  These functions provide support to get at that
data.  A following patch will export this data.

Signed-off-by: Don Skidmore <donald.c.skidmore@intel.com>
Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Tested-by: Stephen Ko <stephen.s.ko@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_common.c |  150 +++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_common.h |   13 ++
 drivers/net/ethernet/intel/ixgbe/ixgbe_type.h   |   38 ++++++
 3 files changed, 201 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
index a3aa633..05aa156 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
@@ -3526,3 +3526,153 @@ void ixgbe_clear_tx_pending(struct ixgbe_hw *hw)
 	IXGBE_WRITE_REG(hw, IXGBE_GCR_EXT, gcr_ext);
 	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, hlreg0);
 }
+
+static const u8 ixgbe_emc_temp_data[4] = {
+	IXGBE_EMC_INTERNAL_DATA,
+	IXGBE_EMC_DIODE1_DATA,
+	IXGBE_EMC_DIODE2_DATA,
+	IXGBE_EMC_DIODE3_DATA
+};
+static const u8 ixgbe_emc_therm_limit[4] = {
+	IXGBE_EMC_INTERNAL_THERM_LIMIT,
+	IXGBE_EMC_DIODE1_THERM_LIMIT,
+	IXGBE_EMC_DIODE2_THERM_LIMIT,
+	IXGBE_EMC_DIODE3_THERM_LIMIT
+};
+
+/**
+ *  ixgbe_get_thermal_sensor_data - Gathers thermal sensor data
+ *  @hw: pointer to hardware structure
+ *  @data: pointer to the thermal sensor data structure
+ *
+ *  Returns the thermal sensor data structure
+ **/
+s32 ixgbe_get_thermal_sensor_data_generic(struct ixgbe_hw *hw)
+{
+	s32 status = 0;
+	u16 ets_offset;
+	u16 ets_cfg;
+	u16 ets_sensor;
+	u8  num_sensors;
+	u8  sensor_index;
+	u8  sensor_location;
+	u8  i;
+	struct ixgbe_thermal_sensor_data *data = &hw->mac.thermal_sensor_data;
+
+	/* Only support thermal sensors attached to 82599 physical port 0 */
+	if ((hw->mac.type != ixgbe_mac_82599EB) ||
+	     (IXGBE_READ_REG(hw, IXGBE_STATUS) & IXGBE_STATUS_LAN_ID_1)) {
+		status = IXGBE_NOT_IMPLEMENTED;
+		goto out;
+	}
+
+	status = hw->eeprom.ops.read(hw, IXGBE_ETS_CFG, &ets_offset);
+	if (status)
+		goto out;
+
+	if ((ets_offset == 0x0000) || (ets_offset == 0xFFFF)) {
+		status = IXGBE_NOT_IMPLEMENTED;
+		goto out;
+	}
+
+	status = hw->eeprom.ops.read(hw, ets_offset, &ets_cfg);
+	if (status)
+		goto out;
+
+	if (((ets_cfg & IXGBE_ETS_TYPE_MASK) >> IXGBE_ETS_TYPE_SHIFT)
+	    != IXGBE_ETS_TYPE_EMC) {
+		status = IXGBE_NOT_IMPLEMENTED;
+		goto out;
+	}
+
+	num_sensors = (ets_cfg & IXGBE_ETS_NUM_SENSORS_MASK);
+	if (num_sensors > IXGBE_MAX_SENSORS)
+		num_sensors = IXGBE_MAX_SENSORS;
+
+	for (i = 0; i < num_sensors; i++) {
+		status = hw->eeprom.ops.read(hw, (ets_offset + 1 + i),
+					     &ets_sensor);
+		if (status)
+			goto out;
+
+		sensor_index = ((ets_sensor & IXGBE_ETS_DATA_INDEX_MASK) >>
+				IXGBE_ETS_DATA_INDEX_SHIFT);
+		sensor_location = ((ets_sensor & IXGBE_ETS_DATA_LOC_MASK) >>
+				   IXGBE_ETS_DATA_LOC_SHIFT);
+
+		if (sensor_location != 0) {
+			status = hw->phy.ops.read_i2c_byte(hw,
+					ixgbe_emc_temp_data[sensor_index],
+					IXGBE_I2C_THERMAL_SENSOR_ADDR,
+					&data->sensor[i].temp);
+		if (status)
+			goto out;
+		}
+	}
+out:
+	return status;
+}
+
+/**
+ * ixgbe_init_thermal_sensor_thresh_generic - Inits thermal sensor thresholds
+ * @hw: pointer to hardware structure
+ *
+ * Inits the thermal sensor thresholds according to the NVM map
+ * and save off the threshold and location values into mac.thermal_sensor_data
+ **/
+s32 ixgbe_init_thermal_sensor_thresh_generic(struct ixgbe_hw *hw)
+{
+	s32 status = 0;
+	u16 ets_offset;
+	u16 ets_cfg;
+	u16 ets_sensor;
+	u8  low_thresh_delta;
+	u8  num_sensors;
+	u8  sensor_index;
+	u8  sensor_location;
+	u8  therm_limit;
+	u8  i;
+	struct ixgbe_thermal_sensor_data *data = &hw->mac.thermal_sensor_data;
+
+	memset(data, 0, sizeof(struct ixgbe_thermal_sensor_data));
+
+	/* Only support thermal sensors attached to 82599 physical port 0 */
+	if ((hw->mac.type != ixgbe_mac_82599EB) ||
+	    (IXGBE_READ_REG(hw, IXGBE_STATUS) & IXGBE_STATUS_LAN_ID_1))
+		return IXGBE_NOT_IMPLEMENTED;
+
+	hw->eeprom.ops.read(hw, IXGBE_ETS_CFG, &ets_offset);
+	if ((ets_offset == 0x0000) || (ets_offset == 0xFFFF))
+		return IXGBE_NOT_IMPLEMENTED;
+
+	hw->eeprom.ops.read(hw, ets_offset, &ets_cfg);
+	if (((ets_cfg & IXGBE_ETS_TYPE_MASK) >> IXGBE_ETS_TYPE_SHIFT)
+	    != IXGBE_ETS_TYPE_EMC)
+		return IXGBE_NOT_IMPLEMENTED;
+
+	low_thresh_delta = ((ets_cfg & IXGBE_ETS_LTHRES_DELTA_MASK) >>
+			     IXGBE_ETS_LTHRES_DELTA_SHIFT);
+	num_sensors = (ets_cfg & IXGBE_ETS_NUM_SENSORS_MASK);
+
+	for (i = 0; i < num_sensors; i++) {
+		hw->eeprom.ops.read(hw, (ets_offset + 1 + i), &ets_sensor);
+		sensor_index = ((ets_sensor & IXGBE_ETS_DATA_INDEX_MASK) >>
+				IXGBE_ETS_DATA_INDEX_SHIFT);
+		sensor_location = ((ets_sensor & IXGBE_ETS_DATA_LOC_MASK) >>
+				   IXGBE_ETS_DATA_LOC_SHIFT);
+		therm_limit = ets_sensor & IXGBE_ETS_DATA_HTHRESH_MASK;
+
+		hw->phy.ops.write_i2c_byte(hw,
+			ixgbe_emc_therm_limit[sensor_index],
+			IXGBE_I2C_THERMAL_SENSOR_ADDR, therm_limit);
+
+		if ((i < IXGBE_MAX_SENSORS) && (sensor_location != 0)) {
+			data->sensor[i].location = sensor_location;
+			data->sensor[i].caution_thresh = therm_limit;
+			data->sensor[i].max_op_thresh = therm_limit -
+							low_thresh_delta;
+		}
+	}
+	return status;
+}
+
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
index 863f9c1..876c7f3 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
@@ -105,6 +105,19 @@ void ixgbe_clear_tx_pending(struct ixgbe_hw *hw);
 void ixgbe_set_rxpba_generic(struct ixgbe_hw *hw, int num_pb,
 			     u32 headroom, int strategy);
 
+#define IXGBE_I2C_THERMAL_SENSOR_ADDR	0xF8
+#define IXGBE_EMC_INTERNAL_DATA		0x00
+#define IXGBE_EMC_INTERNAL_THERM_LIMIT	0x20
+#define IXGBE_EMC_DIODE1_DATA		0x01
+#define IXGBE_EMC_DIODE1_THERM_LIMIT	0x19
+#define IXGBE_EMC_DIODE2_DATA		0x23
+#define IXGBE_EMC_DIODE2_THERM_LIMIT	0x1A
+#define IXGBE_EMC_DIODE3_DATA		0x2A
+#define IXGBE_EMC_DIODE3_THERM_LIMIT	0x30
+
+s32 ixgbe_get_thermal_sensor_data_generic(struct ixgbe_hw *hw);
+s32 ixgbe_init_thermal_sensor_thresh_generic(struct ixgbe_hw *hw);
+
 #define IXGBE_WRITE_REG(a, reg, value) writel((value), ((a)->hw_addr + (reg)))
 
 #ifndef writeq
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
index 7c5817f..c5fba00 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
@@ -109,6 +109,26 @@
 #define IXGBE_I2C_CLK_OUT   0x00000002
 #define IXGBE_I2C_DATA_IN   0x00000004
 #define IXGBE_I2C_DATA_OUT  0x00000008
+#define IXGBE_I2C_THERMAL_SENSOR_ADDR   0xF8
+#define IXGBE_EMC_INTERNAL_DATA         0x00
+#define IXGBE_EMC_INTERNAL_THERM_LIMIT  0x20
+#define IXGBE_EMC_DIODE1_DATA           0x01
+#define IXGBE_EMC_DIODE1_THERM_LIMIT    0x19
+#define IXGBE_EMC_DIODE2_DATA           0x23
+#define IXGBE_EMC_DIODE2_THERM_LIMIT    0x1A
+
+#define IXGBE_MAX_SENSORS               3
+
+struct ixgbe_thermal_diode_data {
+	u8 location;
+	u8 temp;
+	u8 caution_thresh;
+	u8 max_op_thresh;
+};
+
+struct ixgbe_thermal_sensor_data {
+	 struct ixgbe_thermal_diode_data sensor[IXGBE_MAX_SENSORS];
+};
 
 /* Interrupt Registers */
 #define IXGBE_EICR      0x00800
@@ -1674,6 +1694,21 @@ enum {
 #define IXGBE_PBANUM0_PTR       0x15
 #define IXGBE_PBANUM1_PTR       0x16
 #define IXGBE_FREE_SPACE_PTR    0X3E
+
+/* External Thermal Sensor Config */
+#define IXGBE_ETS_CFG                   0x26
+#define IXGBE_ETS_LTHRES_DELTA_MASK     0x07C0
+#define IXGBE_ETS_LTHRES_DELTA_SHIFT    6
+#define IXGBE_ETS_TYPE_MASK             0x0038
+#define IXGBE_ETS_TYPE_SHIFT            3
+#define IXGBE_ETS_TYPE_EMC              0x000
+#define IXGBE_ETS_NUM_SENSORS_MASK      0x0007
+#define IXGBE_ETS_DATA_LOC_MASK         0x3C00
+#define IXGBE_ETS_DATA_LOC_SHIFT        10
+#define IXGBE_ETS_DATA_INDEX_MASK       0x0300
+#define IXGBE_ETS_DATA_INDEX_SHIFT      8
+#define IXGBE_ETS_DATA_HTHRESH_MASK     0x00FF
+
 #define IXGBE_SAN_MAC_ADDR_PTR  0x28
 #define IXGBE_DEVICE_CAPS       0x2C
 #define IXGBE_SERIAL_NUMBER_MAC_ADDR 0x11
@@ -2767,6 +2802,8 @@ struct ixgbe_mac_operations {
 
 	/* Manageability interface */
 	s32 (*set_fw_drv_ver)(struct ixgbe_hw *, u8, u8, u8, u8);
+	s32 (*get_thermal_sensor_data)(struct ixgbe_hw *);
+	s32 (*init_thermal_sensor_thresh)(struct ixgbe_hw *hw);
 };
 
 struct ixgbe_phy_operations {
@@ -2824,6 +2861,7 @@ struct ixgbe_mac_info {
 	bool                            orig_link_settings_stored;
 	bool                            autotry_restart;
 	u8                              flags;
+	struct ixgbe_thermal_sensor_data  thermal_sensor_data;
 };
 
 struct ixgbe_phy_info {
-- 
1.7.7.4

Re: [net-next 7/9] ixgbe: add support functions for gathering thermal data sensor

[Francois Romieu]

Jeff Kirsher <jeffrey.t.kirsher@intel.com> :
> From: Don Skidmore <donald.c.skidmore@intel.com>
> 
> Some 82599 adapters contain thermal data that we can get to via
> an i2c interface.  These functions provide support to get at that
> data.  A following patch will export this data.
> 
> Signed-off-by: Don Skidmore <donald.c.skidmore@intel.com>
> Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> Tested-by: Stephen Ko <stephen.s.ko@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_common.c |  150 +++++++++++++++++++++++
>  drivers/net/ethernet/intel/ixgbe/ixgbe_common.h |   13 ++
>  drivers/net/ethernet/intel/ixgbe/ixgbe_type.h   |   38 ++++++
>  3 files changed, 201 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
> index a3aa633..05aa156 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
> @@ -3526,3 +3526,153 @@ void ixgbe_clear_tx_pending(struct ixgbe_hw *hw)
>  	IXGBE_WRITE_REG(hw, IXGBE_GCR_EXT, gcr_ext);
>  	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, hlreg0);
>  }
> +
> +static const u8 ixgbe_emc_temp_data[4] = {
> +	IXGBE_EMC_INTERNAL_DATA,
> +	IXGBE_EMC_DIODE1_DATA,
> +	IXGBE_EMC_DIODE2_DATA,
> +	IXGBE_EMC_DIODE3_DATA
> +};
> +static const u8 ixgbe_emc_therm_limit[4] = {
> +	IXGBE_EMC_INTERNAL_THERM_LIMIT,
> +	IXGBE_EMC_DIODE1_THERM_LIMIT,
> +	IXGBE_EMC_DIODE2_THERM_LIMIT,
> +	IXGBE_EMC_DIODE3_THERM_LIMIT
> +};
> +
> +/**
> + *  ixgbe_get_thermal_sensor_data - Gathers thermal sensor data
> + *  @hw: pointer to hardware structure
> + *  @data: pointer to the thermal sensor data structure
> + *
> + *  Returns the thermal sensor data structure
> + **/
> +s32 ixgbe_get_thermal_sensor_data_generic(struct ixgbe_hw *hw)
> +{
> +	s32 status = 0;
> +	u16 ets_offset;
> +	u16 ets_cfg;
> +	u16 ets_sensor;
> +	u8  num_sensors;
> +	u8  sensor_index;
> +	u8  sensor_location;
> +	u8  i;
> +	struct ixgbe_thermal_sensor_data *data = &hw->mac.thermal_sensor_data;
> +
> +	/* Only support thermal sensors attached to 82599 physical port 0 */
> +	if ((hw->mac.type != ixgbe_mac_82599EB) ||
> +	     (IXGBE_READ_REG(hw, IXGBE_STATUS) & IXGBE_STATUS_LAN_ID_1)) {
> +		status = IXGBE_NOT_IMPLEMENTED;

While defined, IXGBE_NOT_IMPLEMENTED is currently unused in both davem-next
and the remaining patches of this series. Can't you remove it completely and
use a standard error code, say -EOPNOTSUPP ?

> +		goto out;
> +	}
> +
> +	status = hw->eeprom.ops.read(hw, IXGBE_ETS_CFG, &ets_offset);
> +	if (status)
> +		goto out;
> +
> +	if ((ets_offset == 0x0000) || (ets_offset == 0xFFFF)) {
> +		status = IXGBE_NOT_IMPLEMENTED;
> +		goto out;
> +	}
> +
> +	status = hw->eeprom.ops.read(hw, ets_offset, &ets_cfg);
> +	if (status)
> +		goto out;
> +
> +	if (((ets_cfg & IXGBE_ETS_TYPE_MASK) >> IXGBE_ETS_TYPE_SHIFT)
> +	    != IXGBE_ETS_TYPE_EMC) {
            ^^ -> should appear in the previous line

> +		status = IXGBE_NOT_IMPLEMENTED;
> +		goto out;
> +	}
> +
> +	num_sensors = (ets_cfg & IXGBE_ETS_NUM_SENSORS_MASK);
> +	if (num_sensors > IXGBE_MAX_SENSORS)
> +		num_sensors = IXGBE_MAX_SENSORS;

ets_cfg is not used beyond this point and most of this code is duplicated
in ixgbe_init_thermal_sensor_thresh_generic.

You may refactor the code with xyz_get_ets_offset() and xyz_get_num_sensors()
methods, add some eth_type local variable and avoid the duplication while
keeping the lines short.

> +
> +	for (i = 0; i < num_sensors; i++) {

sensor_index and sensor_location should be declared here.

> +		status = hw->eeprom.ops.read(hw, (ets_offset + 1 + i),
> +					     &ets_sensor);
> +		if (status)
> +			goto out;
> +
> +		sensor_index = ((ets_sensor & IXGBE_ETS_DATA_INDEX_MASK) >>
> +				IXGBE_ETS_DATA_INDEX_SHIFT);
> +		sensor_location = ((ets_sensor & IXGBE_ETS_DATA_LOC_MASK) >>
> +				   IXGBE_ETS_DATA_LOC_SHIFT);
> +
> +		if (sensor_location != 0) {
> +			status = hw->phy.ops.read_i2c_byte(hw,
> +					ixgbe_emc_temp_data[sensor_index],
> +					IXGBE_I2C_THERMAL_SENSOR_ADDR,
> +					&data->sensor[i].temp);
> +		if (status)
> +			goto out;
> +		}

Broken indentation.

> +	}
> +out:
> +	return status;
> +}
> +
> +/**
> + * ixgbe_init_thermal_sensor_thresh_generic - Inits thermal sensor thresholds
> + * @hw: pointer to hardware structure
> + *
> + * Inits the thermal sensor thresholds according to the NVM map
> + * and save off the threshold and location values into mac.thermal_sensor_data
> + **/
> +s32 ixgbe_init_thermal_sensor_thresh_generic(struct ixgbe_hw *hw)
> +{
> +	s32 status = 0;
> +	u16 ets_offset;
> +	u16 ets_cfg;
> +	u16 ets_sensor;
> +	u8  low_thresh_delta;
> +	u8  num_sensors;
> +	u8  sensor_index;
> +	u8  sensor_location;
> +	u8  therm_limit;
> +	u8  i;
> +	struct ixgbe_thermal_sensor_data *data = &hw->mac.thermal_sensor_data;
> +
> +	memset(data, 0, sizeof(struct ixgbe_thermal_sensor_data));
> +
> +	/* Only support thermal sensors attached to 82599 physical port 0 */
> +	if ((hw->mac.type != ixgbe_mac_82599EB) ||
> +	    (IXGBE_READ_REG(hw, IXGBE_STATUS) & IXGBE_STATUS_LAN_ID_1))
> +		return IXGBE_NOT_IMPLEMENTED;
> +
> +	hw->eeprom.ops.read(hw, IXGBE_ETS_CFG, &ets_offset);
> +	if ((ets_offset == 0x0000) || (ets_offset == 0xFFFF))
> +		return IXGBE_NOT_IMPLEMENTED;
> +
> +	hw->eeprom.ops.read(hw, ets_offset, &ets_cfg);
> +	if (((ets_cfg & IXGBE_ETS_TYPE_MASK) >> IXGBE_ETS_TYPE_SHIFT)
> +	    != IXGBE_ETS_TYPE_EMC)
            ^^ sic

> +		return IXGBE_NOT_IMPLEMENTED;
> +
> +	low_thresh_delta = ((ets_cfg & IXGBE_ETS_LTHRES_DELTA_MASK) >>
> +			     IXGBE_ETS_LTHRES_DELTA_SHIFT);
> +	num_sensors = (ets_cfg & IXGBE_ETS_NUM_SENSORS_MASK);
> +
> +	for (i = 0; i < num_sensors; i++) {
> +		hw->eeprom.ops.read(hw, (ets_offset + 1 + i), &ets_sensor);
> +		sensor_index = ((ets_sensor & IXGBE_ETS_DATA_INDEX_MASK) >>
> +				IXGBE_ETS_DATA_INDEX_SHIFT);
> +		sensor_location = ((ets_sensor & IXGBE_ETS_DATA_LOC_MASK) >>
> +				   IXGBE_ETS_DATA_LOC_SHIFT);
> +		therm_limit = ets_sensor & IXGBE_ETS_DATA_HTHRESH_MASK;
> +
> +		hw->phy.ops.write_i2c_byte(hw,
> +			ixgbe_emc_therm_limit[sensor_index],
> +			IXGBE_I2C_THERMAL_SENSOR_ADDR, therm_limit);
> +
> +		if ((i < IXGBE_MAX_SENSORS) && (sensor_location != 0)) {

The patch would be easier to review if num_sensors was capped as in
ixgbe_get_thermal_sensor_data_generic...

> +			data->sensor[i].location = sensor_location;
> +			data->sensor[i].caution_thresh = therm_limit;
> +			data->sensor[i].max_op_thresh = therm_limit -
> +							low_thresh_delta;

... and the code would gain an extra tab level, thus avoiding the line
break if there was a 'continue' statement when sensor_location fails the
test.

-- 
Ueimor

RE: [net-next 7/9] ixgbe: add support functions for gathering thermal data sensor

[Skidmore, Donald C]

>-----Original Message-----
>From: Francois Romieu [mailto:romieu@fr.zoreil.com]
>Sent: Friday, December 23, 2011 5:01 AM
>To: Kirsher, Jeffrey T
>Cc: davem@davemloft.net; Skidmore, Donald C; netdev@vger.kernel.org;
>gospo@redhat.com; sassmann@redhat.com; Waskiewicz Jr, Peter P
>Subject: Re: [net-next 7/9] ixgbe: add support functions for gathering
>thermal data sensor
>
>Jeff Kirsher <jeffrey.t.kirsher@intel.com> :
>> From: Don Skidmore <donald.c.skidmore@intel.com>
>>
>> Some 82599 adapters contain thermal data that we can get to via
>> an i2c interface.  These functions provide support to get at that
>> data.  A following patch will export this data.
>>
>> Signed-off-by: Don Skidmore <donald.c.skidmore@intel.com>
>> Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
>> Tested-by: Stephen Ko <stephen.s.ko@intel.com>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> ---
>>  drivers/net/ethernet/intel/ixgbe/ixgbe_common.c |  150
>+++++++++++++++++++++++
>>  drivers/net/ethernet/intel/ixgbe/ixgbe_common.h |   13 ++
>>  drivers/net/ethernet/intel/ixgbe/ixgbe_type.h   |   38 ++++++
>>  3 files changed, 201 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
>b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
>> index a3aa633..05aa156 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
>> @@ -3526,3 +3526,153 @@ void ixgbe_clear_tx_pending(struct ixgbe_hw
>*hw)
>>  	IXGBE_WRITE_REG(hw, IXGBE_GCR_EXT, gcr_ext);
>>  	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, hlreg0);
>>  }
>> +
>> +static const u8 ixgbe_emc_temp_data[4] = {
>> +	IXGBE_EMC_INTERNAL_DATA,
>> +	IXGBE_EMC_DIODE1_DATA,
>> +	IXGBE_EMC_DIODE2_DATA,
>> +	IXGBE_EMC_DIODE3_DATA
>> +};
>> +static const u8 ixgbe_emc_therm_limit[4] = {
>> +	IXGBE_EMC_INTERNAL_THERM_LIMIT,
>> +	IXGBE_EMC_DIODE1_THERM_LIMIT,
>> +	IXGBE_EMC_DIODE2_THERM_LIMIT,
>> +	IXGBE_EMC_DIODE3_THERM_LIMIT
>> +};
>> +
>> +/**
>> + *  ixgbe_get_thermal_sensor_data - Gathers thermal sensor data
>> + *  @hw: pointer to hardware structure
>> + *  @data: pointer to the thermal sensor data structure
>> + *
>> + *  Returns the thermal sensor data structure
>> + **/
>> +s32 ixgbe_get_thermal_sensor_data_generic(struct ixgbe_hw *hw)
>> +{
>> +	s32 status = 0;
>> +	u16 ets_offset;
>> +	u16 ets_cfg;
>> +	u16 ets_sensor;
>> +	u8  num_sensors;
>> +	u8  sensor_index;
>> +	u8  sensor_location;
>> +	u8  i;
>> +	struct ixgbe_thermal_sensor_data *data = &hw-
>>mac.thermal_sensor_data;
>> +
>> +	/* Only support thermal sensors attached to 82599 physical port 0
>*/
>> +	if ((hw->mac.type != ixgbe_mac_82599EB) ||
>> +	     (IXGBE_READ_REG(hw, IXGBE_STATUS) & IXGBE_STATUS_LAN_ID_1)) {
>> +		status = IXGBE_NOT_IMPLEMENTED;
>
>While defined, IXGBE_NOT_IMPLEMENTED is currently unused in both davem-
>next
>and the remaining patches of this series. Can't you remove it completely
>and
>use a standard error code, say -EOPNOTSUPP ?

Previously we have only been using EOPNOTSUPP and it's like as a return value seen outside the driver particularly to user space.  Looking through a few other device drivers (in no way an extensive search) they 'seem' to be following that model too.  Still that said I'm not aware of this being a BKM it's just what I noticed. 


>
>> +		goto out;
>> +	}
>> +
>> +	status = hw->eeprom.ops.read(hw, IXGBE_ETS_CFG, &ets_offset);
>> +	if (status)
>> +		goto out;
>> +
>> +	if ((ets_offset == 0x0000) || (ets_offset == 0xFFFF)) {
>> +		status = IXGBE_NOT_IMPLEMENTED;
>> +		goto out;
>> +	}
>> +
>> +	status = hw->eeprom.ops.read(hw, ets_offset, &ets_cfg);
>> +	if (status)
>> +		goto out;
>> +
>> +	if (((ets_cfg & IXGBE_ETS_TYPE_MASK) >> IXGBE_ETS_TYPE_SHIFT)
>> +	    != IXGBE_ETS_TYPE_EMC) {
>            ^^ -> should appear in the previous line
>

I'm probably just missing your point, but if I added this to the previous line I would be over 80 chars?  

if (((ets_cfg & IXGBE_ETS_TYPE_MASK) >> IXGBE_ETS_TYPE_SHIFT) != IXGBE_ETS_TYPE_EMC) {

Or are you talking about just moving the "!=" to the previous line?

if (((ets_cfg & IXGBE_ETS_TYPE_MASK) >> IXGBE_ETS_TYPE_SHIFT) !=
    IXGBE_ETS_TYPE_EMC) {

I have a strong feeling I'm just not getting what you're suggesting here.  Please clue me in.

>> +		status = IXGBE_NOT_IMPLEMENTED;
>> +		goto out;
>> +	}
>> +
>> +	num_sensors = (ets_cfg & IXGBE_ETS_NUM_SENSORS_MASK);
>> +	if (num_sensors > IXGBE_MAX_SENSORS)
>> +		num_sensors = IXGBE_MAX_SENSORS;
>
>ets_cfg is not used beyond this point and most of this code is
>duplicated
>in 	.
>
>You may refactor the code with xyz_get_ets_offset() and
>xyz_get_num_sensors()
>methods, add some eth_type local variable and avoid the duplication
>while
>keeping the lines short.
>

While I like this idea and it would remove some duplicate code, I need to use ets_cfg in ixgbe_init_thermal_sensor_thresh_generic() to get the low_thresh_delta.  This makes the support function a little less useful as it would either need to return two values (num_sensors and low_thresh_delta) or just the ets_cfs itself.   For the latter case the support function would just be making two reads and we would still need the local variables.  Still might be worth it though.

>> +
>> +	for (i = 0; i < num_sensors; i++) {
>
>sensor_index and sensor_location should be declared here.
>

Agreed. :)

>> +		status = hw->eeprom.ops.read(hw, (ets_offset + 1 + i),
>> +					     &ets_sensor);
>> +		if (status)
>> +			goto out;
>> +
>> +		sensor_index = ((ets_sensor & IXGBE_ETS_DATA_INDEX_MASK) >>
>> +				IXGBE_ETS_DATA_INDEX_SHIFT);
>> +		sensor_location = ((ets_sensor & IXGBE_ETS_DATA_LOC_MASK) >>
>> +				   IXGBE_ETS_DATA_LOC_SHIFT);
>> +
>> +		if (sensor_location != 0) {
>> +			status = hw->phy.ops.read_i2c_byte(hw,
>> +					ixgbe_emc_temp_data[sensor_index],
>> +					IXGBE_I2C_THERMAL_SENSOR_ADDR,
>> +					&data->sensor[i].temp);
>> +		if (status)
>> +			goto out;
>> +		}
>
>Broken indentation.

Glad you noticed this I will correct it.

>
>> +	}
>> +out:
>> +	return status;
>> +}
>> +
>> +/**
>> + * ixgbe_init_thermal_sensor_thresh_generic - Inits thermal sensor
>thresholds
>> + * @hw: pointer to hardware structure
>> + *
>> + * Inits the thermal sensor thresholds according to the NVM map
>> + * and save off the threshold and location values into
>mac.thermal_sensor_data
>> + **/
>> +s32 ixgbe_init_thermal_sensor_thresh_generic(struct ixgbe_hw *hw)
>> +{
>> +	s32 status = 0;
>> +	u16 ets_offset;
>> +	u16 ets_cfg;
>> +	u16 ets_sensor;
>> +	u8  low_thresh_delta;
>> +	u8  num_sensors;
>> +	u8  sensor_index;
>> +	u8  sensor_location;
>> +	u8  therm_limit;
>> +	u8  i;
>> +	struct ixgbe_thermal_sensor_data *data = &hw-
>>mac.thermal_sensor_data;
>> +
>> +	memset(data, 0, sizeof(struct ixgbe_thermal_sensor_data));
>> +
>> +	/* Only support thermal sensors attached to 82599 physical port 0
>*/
>> +	if ((hw->mac.type != ixgbe_mac_82599EB) ||
>> +	    (IXGBE_READ_REG(hw, IXGBE_STATUS) & IXGBE_STATUS_LAN_ID_1))
>> +		return IXGBE_NOT_IMPLEMENTED;
>> +
>> +	hw->eeprom.ops.read(hw, IXGBE_ETS_CFG, &ets_offset);
>> +	if ((ets_offset == 0x0000) || (ets_offset == 0xFFFF))
>> +		return IXGBE_NOT_IMPLEMENTED;
>> +
>> +	hw->eeprom.ops.read(hw, ets_offset, &ets_cfg);
>> +	if (((ets_cfg & IXGBE_ETS_TYPE_MASK) >> IXGBE_ETS_TYPE_SHIFT)
>> +	    != IXGBE_ETS_TYPE_EMC)
>            ^^ sic
>

Like I mentioned about I don't think I understand what you're pointing out here.


>> +		return IXGBE_NOT_IMPLEMENTED;
>> +
>> +	low_thresh_delta = ((ets_cfg & IXGBE_ETS_LTHRES_DELTA_MASK) >>
>> +			     IXGBE_ETS_LTHRES_DELTA_SHIFT);
>> +	num_sensors = (ets_cfg & IXGBE_ETS_NUM_SENSORS_MASK);
>> +
>> +	for (i = 0; i < num_sensors; i++) {
>> +		hw->eeprom.ops.read(hw, (ets_offset + 1 + i), &ets_sensor);
>> +		sensor_index = ((ets_sensor & IXGBE_ETS_DATA_INDEX_MASK) >>
>> +				IXGBE_ETS_DATA_INDEX_SHIFT);
>> +		sensor_location = ((ets_sensor & IXGBE_ETS_DATA_LOC_MASK) >>
>> +				   IXGBE_ETS_DATA_LOC_SHIFT);
>> +		therm_limit = ets_sensor & IXGBE_ETS_DATA_HTHRESH_MASK;
>> +
>> +		hw->phy.ops.write_i2c_byte(hw,
>> +			ixgbe_emc_therm_limit[sensor_index],
>> +			IXGBE_I2C_THERMAL_SENSOR_ADDR, therm_limit);
>> +
>> +		if ((i < IXGBE_MAX_SENSORS) && (sensor_location != 0)) {
>
>The patch would be easier to review if num_sensors was capped as in
>ixgbe_get_thermal_sensor_data_generic...
>
>> +			data->sensor[i].location = sensor_location;
>> +			data->sensor[i].caution_thresh = therm_limit;
>> +			data->sensor[i].max_op_thresh = therm_limit -
>> +							low_thresh_delta;
>
>... and the code would gain an extra tab level, thus avoiding the line
>break if there was a 'continue' statement when sensor_location fails the
>test.

I like this too and will update the patch.

Thanks for review I do really appreciate it. 

-Don Skidmore <donald.c.skidmore@intel.com>

>
>--
>Ueimor

Re: [net-next 7/9] ixgbe: add support functions for gathering thermal data sensor

[Francois Romieu]

Skidmore, Donald C <donald.c.skidmore@intel.com> :
[...]
> >While defined, IXGBE_NOT_IMPLEMENTED is currently unused in both davem-
> >next
> >and the remaining patches of this series. Can't you remove it completely
> >and
> >use a standard error code, say -EOPNOTSUPP ?
> 
> Previously we have only been using EOPNOTSUPP and it's like as a return
> value seen outside the driver particularly to user space.  Looking through
> a few other device drivers (in no way an extensive search) they 'seem' to
> be following that model too.  Still that said I'm not aware of this being
> a BKM it's just what I noticed. 

Your call. As long as the code does not end cluttered with:

	rc = (status != MY_OWN_PRIVATE_ERROR_STATUS) ? 0 : -Esomething;

[...]
> if (((ets_cfg & IXGBE_ETS_TYPE_MASK) >> IXGBE_ETS_TYPE_SHIFT) != IXGBE_ETS_TYPE_EMC) {
> 
> Or are you talking about just moving the "!=" to the previous line?

Just the "!=".

You may include the shift in the definition of IXGBE_ETS_TYPE_EMC itself
so that it disappears here.

[...]
> While I like this idea and it would remove some duplicate code, I need
> to use ets_cfg in ixgbe_init_thermal_sensor_thresh_generic() to get the
> low_thresh_delta.

You are right, I missed it.

> This makes the support function a little less useful as it would either
> need to return two values (num_sensors and low_thresh_delta) or just the
> ets_cfs itself.   For the latter case the support function would just be
> making two reads and we would still need the local variables. Still might
> be worth it though.

Something like a stack allocated struct ets { u16 cfg; u16 sensor; } may
help.

-- 
Ueimor

[net-next 8/9] ixgbe: add interface to export thermal data

[Jeff Kirsher]

From: Don Skidmore <donald.c.skidmore@intel.com>

Some of our adapters have thermal data available, this patch exports this
data via a read-only sysfs interface.  More patches will follow that will
contain additional information to be exported.

Signed-off-by: Don Skidmore <donald.c.skidmore@intel.com>
Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Tested-by: Stephen Ko <stephen.s.ko@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/Makefile      |    2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe.h       |    4 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c |    2 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |    6 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c |  305 ++++++++++++++++++++++++
 5 files changed, 318 insertions(+), 1 deletions(-)
 create mode 100644 drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c

diff --git a/drivers/net/ethernet/intel/ixgbe/Makefile b/drivers/net/ethernet/intel/ixgbe/Makefile
index 7d7387f..852bf86 100644
--- a/drivers/net/ethernet/intel/ixgbe/Makefile
+++ b/drivers/net/ethernet/intel/ixgbe/Makefile
@@ -34,7 +34,7 @@ obj-$(CONFIG_IXGBE) += ixgbe.o
 
 ixgbe-objs := ixgbe_main.o ixgbe_common.o ixgbe_ethtool.o \
               ixgbe_82599.o ixgbe_82598.o ixgbe_phy.o ixgbe_sriov.o \
-              ixgbe_mbx.o ixgbe_x540.o
+              ixgbe_mbx.o ixgbe_x540.o ixgbe_sysfs.o
 
 ixgbe-$(CONFIG_IXGBE_DCB) +=  ixgbe_dcb.o ixgbe_dcb_82598.o \
                               ixgbe_dcb_82599.o ixgbe_dcb_nl.o
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index a8368d5..3531dc9 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -518,6 +518,8 @@ struct ixgbe_adapter {
 	int fdir_filter_count;
 	u32 timer_event_accumulator;
 	u32 vferr_refcount;
+	struct kobject *info_kobj;
+	struct kobject *therm_kobj[IXGBE_MAX_SENSORS];
 };
 
 struct ixgbe_fdir_filter {
@@ -606,6 +608,8 @@ extern void ixgbe_set_rx_mode(struct net_device *netdev);
 extern int ixgbe_setup_tc(struct net_device *dev, u8 tc);
 extern void ixgbe_tx_ctxtdesc(struct ixgbe_ring *, u32, u32, u32, u32);
 extern void ixgbe_do_reset(struct net_device *netdev);
+extern void ixgbe_sysfs_exit(struct ixgbe_adapter *adapter);
+extern int ixgbe_sysfs_init(struct ixgbe_adapter *adapter);
 #ifdef IXGBE_FCOE
 extern void ixgbe_configure_fcoe(struct ixgbe_adapter *adapter);
 extern int ixgbe_fso(struct ixgbe_ring *tx_ring, struct sk_buff *skb,
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
index 7720721..1a3810e 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
@@ -2137,6 +2137,8 @@ static struct ixgbe_mac_operations mac_ops_82599 = {
 	.set_vlan_anti_spoofing = &ixgbe_set_vlan_anti_spoofing,
 	.acquire_swfw_sync      = &ixgbe_acquire_swfw_sync,
 	.release_swfw_sync      = &ixgbe_release_swfw_sync,
+	.get_thermal_sensor_data = &ixgbe_get_thermal_sensor_data_generic,
+	.init_thermal_sensor_thresh = &ixgbe_init_thermal_sensor_thresh_generic,
 
 };
 
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index e27e4d1..b5cef7e 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7717,6 +7717,10 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
 
 	e_dev_info("Intel(R) 10 Gigabit Network Connection\n");
 	cards_found++;
+
+	if (ixgbe_sysfs_init(adapter))
+		e_err(probe, "failed to allocate sysfs resources\n");
+
 	return 0;
 
 err_register:
@@ -7764,6 +7768,8 @@ static void __devexit ixgbe_remove(struct pci_dev *pdev)
 	}
 
 #endif
+	ixgbe_sysfs_exit(adapter);
+
 #ifdef IXGBE_FCOE
 	if (adapter->flags & IXGBE_FLAG_FCOE_ENABLED)
 		ixgbe_cleanup_fcoe(adapter);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
new file mode 100644
index 0000000..db818ae
--- /dev/null
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
@@ -0,0 +1,305 @@
+/*******************************************************************************
+
+  Intel 10 Gigabit PCI Express Linux driver
+  Copyright(c) 1999 - 2011 Intel Corporation.
+
+  This program is free software; you can redistribute it and/or modify it
+  under the terms and conditions of the GNU General Public License,
+  version 2, as published by the Free Software Foundation.
+
+  This program is distributed in the hope it will be useful, but WITHOUT
+  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+  FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+  more details.
+
+  You should have received a copy of the GNU General Public License along with
+  this program; if not, write to the Free Software Foundation, Inc.,
+  51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+
+  The full GNU General Public License is included in this distribution in
+  the file called "COPYING".
+
+  Contact Information:
+  e1000-devel Mailing List <e1000-devel@lists.sourceforge.net>
+  Intel Corporation, 5200 N.E. Elam Young Parkway, Hillsboro, OR 97124-6497
+
+*******************************************************************************/
+
+#include "ixgbe.h"
+#include "ixgbe_common.h"
+#include "ixgbe_type.h"
+
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/sysfs.h>
+#include <linux/kobject.h>
+#include <linux/device.h>
+#include <linux/netdevice.h>
+
+/*
+ * This file provides a sysfs interface to export information from the
+ * driver.  The information presented is READ-ONLY.
+ */
+
+static struct net_device *ixgbe_get_netdev(struct kobject *kobj)
+{
+	struct net_device *netdev;
+	struct device *device_info_kobj;
+
+	if (kobj == NULL)
+		return NULL;
+
+	device_info_kobj = container_of(kobj->parent, struct device, kobj);
+	if (device_info_kobj == NULL)
+		return NULL;
+
+	netdev = container_of(device_info_kobj, struct net_device, dev);
+	return netdev;
+}
+
+static struct ixgbe_adapter *ixgbe_get_adapter(struct kobject *kobj)
+{
+	struct ixgbe_adapter *adapter;
+	struct net_device *netdev = ixgbe_get_netdev(kobj);
+
+	if (netdev == NULL)
+		return NULL;
+
+	adapter = netdev_priv(netdev);
+	return adapter;
+}
+
+static bool ixgbe_thermal_present(struct kobject *kobj)
+{
+	s32 status;
+	struct ixgbe_adapter *adapter = ixgbe_get_adapter(kobj);
+
+	if (adapter == NULL)
+		return false;
+
+	status = ixgbe_init_thermal_sensor_thresh_generic(&(adapter->hw));
+	if (status != 0)
+		return false;
+
+	return true;
+}
+
+/*
+ * ixgbe_name_to_idx - Convert the directory name to the sensor offset.
+ * @ c: pointer to the directory name string
+ *
+ * The directory name is in the form "sensor_n" where n is '0' -
+ * 'IXGBE_MAX_SENSORS'.  IXGBE_MAX_SENSORS will never be greater than
+ * 9.  This function takes advantage of that to keep it simple.
+ */
+static int ixgbe_name_to_idx(const char *c)
+{
+	/* find first digit */
+	while (*c < '0' || *c > '9') {
+		if (*c == '\n')
+			return -1;
+		c++;
+	}
+
+	return (int)(*c - '0');
+}
+
+static s32 ixgbe_sysfs_get_thermal_data(struct kobject *kobj, char *buf)
+{
+	struct ixgbe_adapter *adapter = ixgbe_get_adapter(kobj->parent);
+	s32 status;
+
+	if (adapter == NULL) {
+		snprintf(buf, PAGE_SIZE, "error: missing adapter\n");
+		return 0;
+	}
+
+	if (&adapter->hw == NULL) {
+		snprintf(buf, PAGE_SIZE, "error: missing hw\n");
+		return 0;
+	}
+
+	status = ixgbe_get_thermal_sensor_data_generic(&adapter->hw);
+
+	return status;
+}
+
+static ssize_t ixgbe_sysfs_location(struct kobject *kobj,
+				    struct kobj_attribute *attr, char *buf)
+{
+	struct ixgbe_adapter *adapter = ixgbe_get_adapter(kobj->parent);
+	int idx;
+
+	if (adapter == NULL)
+		return snprintf(buf, PAGE_SIZE, "error: no adapter\n");
+
+	idx = ixgbe_name_to_idx(kobj->name);
+	if (idx == -1)
+		return snprintf(buf, PAGE_SIZE,
+			"error: invalid sensor name %s\n", kobj->name);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n",
+		adapter->hw.mac.thermal_sensor_data.sensor[idx].location);
+}
+
+static ssize_t ixgbe_sysfs_temp(struct kobject *kobj,
+				struct kobj_attribute *attr, char *buf)
+{
+	struct ixgbe_adapter *adapter = ixgbe_get_adapter(kobj->parent);
+	int idx;
+
+	s32 status = ixgbe_sysfs_get_thermal_data(kobj, buf);
+
+	if (status != 0)
+		return snprintf(buf, PAGE_SIZE, "error: status %d returned",
+				status);
+
+	idx = ixgbe_name_to_idx(kobj->name);
+	if (idx == -1)
+		return snprintf(buf, PAGE_SIZE,
+			"error: invalid sensor name %s\n", kobj->name);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n",
+		adapter->hw.mac.thermal_sensor_data.sensor[idx].temp);
+}
+
+static ssize_t ixgbe_sysfs_maxopthresh(struct kobject *kobj,
+				       struct kobj_attribute *attr, char *buf)
+{
+	struct ixgbe_adapter *adapter = ixgbe_get_adapter(kobj->parent);
+	int idx;
+
+	if (adapter == NULL)
+		return snprintf(buf, PAGE_SIZE, "error: no adapter\n");
+
+	idx = ixgbe_name_to_idx(kobj->name);
+	if (idx == -1)
+		return snprintf(buf, PAGE_SIZE,
+			"error: invalid sensor name %s\n", kobj->name);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n",
+		adapter->hw.mac.thermal_sensor_data.sensor[idx].max_op_thresh);
+}
+
+static ssize_t ixgbe_sysfs_cautionthresh(struct kobject *kobj,
+					 struct kobj_attribute *attr, char *buf)
+{
+	struct ixgbe_adapter *adapter = ixgbe_get_adapter(kobj->parent);
+	int idx;
+
+	if (adapter == NULL)
+		return snprintf(buf, PAGE_SIZE, "error: no adapter\n");
+
+	idx = ixgbe_name_to_idx(kobj->name);
+	if (idx == -1)
+		return snprintf(buf, PAGE_SIZE,
+			"error: invalid sensor name %s\n", kobj->name);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n",
+		adapter->hw.mac.thermal_sensor_data.sensor[idx].caution_thresh);
+}
+
+/* Initialize the attributes */
+static struct kobj_attribute ixgbe_sysfs_location_attr =
+	__ATTR(location, 0444, ixgbe_sysfs_location, NULL);
+static struct kobj_attribute ixgbe_sysfs_temp_attr =
+	__ATTR(temp, 0444, ixgbe_sysfs_temp, NULL);
+static struct kobj_attribute ixgbe_sysfs_cautionthresh_attr =
+	__ATTR(cautionthresh, 0444, ixgbe_sysfs_cautionthresh, NULL);
+static struct kobj_attribute ixgbe_sysfs_maxopthresh_attr =
+	__ATTR(maxopthresh, 0444, ixgbe_sysfs_maxopthresh, NULL);
+
+/* Add the attributes into an array, to be added to a group */
+static struct attribute *therm_attrs[] = {
+	&ixgbe_sysfs_location_attr.attr,
+	&ixgbe_sysfs_temp_attr.attr,
+	&ixgbe_sysfs_cautionthresh_attr.attr,
+	&ixgbe_sysfs_maxopthresh_attr.attr,
+	NULL
+};
+
+/* add attributes to a group */
+static struct attribute_group therm_attr_group = {
+	.attrs = therm_attrs,
+};
+
+static void ixgbe_del_adapter(struct ixgbe_adapter *adapter)
+{
+	int i;
+
+	if (adapter == NULL)
+		return;
+
+	for (i = 0; i < IXGBE_MAX_SENSORS; i++) {
+		if (adapter->therm_kobj[i] == NULL)
+			continue;
+		sysfs_remove_group(adapter->therm_kobj[i], &therm_attr_group);
+		kobject_put(adapter->therm_kobj[i]);
+	}
+	if (adapter->info_kobj != NULL)
+		kobject_put(adapter->info_kobj);
+}
+
+/* called from ixgbe_main.c */
+void ixgbe_sysfs_exit(struct ixgbe_adapter *adapter)
+{
+	ixgbe_del_adapter(adapter);
+}
+
+/* called from ixgbe_main.c */
+int ixgbe_sysfs_init(struct ixgbe_adapter *adapter)
+{
+	struct net_device *netdev;
+	int rc = 0;
+	int i;
+	char buf[16];
+
+	if (adapter == NULL)
+		goto err;
+	netdev = adapter->netdev;
+	if (netdev == NULL)
+		goto err;
+
+	adapter->info_kobj = NULL;
+	for (i = 0; i < IXGBE_MAX_SENSORS; i++)
+		adapter->therm_kobj[i] = NULL;
+
+	/* create info kobj and attribute listings in kobj */
+	adapter->info_kobj = kobject_create_and_add("info",
+					&(netdev->dev.kobj));
+	if (adapter->info_kobj == NULL)
+		goto err;
+
+	/* Don't create thermal subkobjs if no data present */
+	if (ixgbe_thermal_present(adapter->info_kobj) != true)
+		goto exit;
+
+	for (i = 0; i < IXGBE_MAX_SENSORS; i++) {
+
+		/*
+		 * Likewise only create individual kobjs that have
+		 * meaningful data.
+		 */
+		if (adapter->hw.mac.thermal_sensor_data.sensor[i].location == 0)
+			continue;
+
+		/* directory named after sensor offset */
+		snprintf(buf, sizeof(buf), "sensor_%d", i);
+		adapter->therm_kobj[i] =
+			kobject_create_and_add(buf, adapter->info_kobj);
+		if (adapter->therm_kobj[i] == NULL)
+			goto err;
+		if (sysfs_create_group(adapter->therm_kobj[i],
+				       &therm_attr_group))
+			goto err;
+	}
+
+	goto exit;
+
+err:
+	ixgbe_del_adapter(adapter);
+	rc = -1;
+exit:
+	return rc;
+}
+
-- 
1.7.7.4

Re: [net-next 8/9] ixgbe: add interface to export thermal data

[Francois Romieu]

Jeff Kirsher <jeffrey.t.kirsher@intel.com> :
> From: Don Skidmore <donald.c.skidmore@intel.com>
> 
> Some of our adapters have thermal data available, this patch exports this
> data via a read-only sysfs interface.  More patches will follow that will
> contain additional information to be exported.
> 
> Signed-off-by: Don Skidmore <donald.c.skidmore@intel.com>
> Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> Tested-by: Stephen Ko <stephen.s.ko@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/Makefile      |    2 +-
>  drivers/net/ethernet/intel/ixgbe/ixgbe.h       |    4 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c |    2 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |    6 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c |  305 ++++++++++++++++++++++++
[...]
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
> index 7720721..1a3810e 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
> @@ -2137,6 +2137,8 @@ static struct ixgbe_mac_operations mac_ops_82599 = {
>  	.set_vlan_anti_spoofing = &ixgbe_set_vlan_anti_spoofing,
>  	.acquire_swfw_sync      = &ixgbe_acquire_swfw_sync,
>  	.release_swfw_sync      = &ixgbe_release_swfw_sync,
> +	.get_thermal_sensor_data = &ixgbe_get_thermal_sensor_data_generic,
> +	.init_thermal_sensor_thresh = &ixgbe_init_thermal_sensor_thresh_generic,

.get_thermal_sensor_data and .init_thermal_sensor_thresh do not seem to
be read anywhere.

>  
>  };
>  
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index e27e4d1..b5cef7e 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -7717,6 +7717,10 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
>  
>  	e_dev_info("Intel(R) 10 Gigabit Network Connection\n");
>  	cards_found++;
> +
> +	if (ixgbe_sysfs_init(adapter))
> +		e_err(probe, "failed to allocate sysfs resources\n");
> +
>  	return 0;
>  
>  err_register:
> @@ -7764,6 +7768,8 @@ static void __devexit ixgbe_remove(struct pci_dev *pdev)
>  	}
>  
>  #endif
> +	ixgbe_sysfs_exit(adapter);
> +
>  #ifdef IXGBE_FCOE
>  	if (adapter->flags & IXGBE_FLAG_FCOE_ENABLED)
>  		ixgbe_cleanup_fcoe(adapter);
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
> new file mode 100644
> index 0000000..db818ae
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
[...]
> +static void ixgbe_del_adapter(struct ixgbe_adapter *adapter)

Please name it ixgbe_sysfs_del_adapter or anything better.

> +{
> +	int i;
> +
> +	if (adapter == NULL)
> +		return;
> +
> +	for (i = 0; i < IXGBE_MAX_SENSORS; i++) {
> +		if (adapter->therm_kobj[i] == NULL)
> +			continue;
> +		sysfs_remove_group(adapter->therm_kobj[i], &therm_attr_group);
> +		kobject_put(adapter->therm_kobj[i]);
> +	}
> +	if (adapter->info_kobj != NULL)
> +		kobject_put(adapter->info_kobj);
> +}
> +
> +/* called from ixgbe_main.c */
> +void ixgbe_sysfs_exit(struct ixgbe_adapter *adapter)
> +{
> +	ixgbe_del_adapter(adapter);
> +}
> +
> +/* called from ixgbe_main.c */
> +int ixgbe_sysfs_init(struct ixgbe_adapter *adapter)
> +{
> +	struct net_device *netdev;
> +	int rc = 0;
> +	int i;
> +	char buf[16];
> +
> +	if (adapter == NULL)
> +		goto err;

ixgbe_sysfs_init is only used in ixgbe_probe at a place where adapter can
not be NULL.

> +	netdev = adapter->netdev;
> +	if (netdev == NULL)
> +		goto err;

netdev can not be NULL.

> +
> +	adapter->info_kobj = NULL;
> +	for (i = 0; i < IXGBE_MAX_SENSORS; i++)
> +		adapter->therm_kobj[i] = NULL;
> +
> +	/* create info kobj and attribute listings in kobj */
> +	adapter->info_kobj = kobject_create_and_add("info", 
> +					&(netdev->dev.kobj));

Remove comment and parenthesis. Everything can fit in a single line.

> +	if (adapter->info_kobj == NULL)
> +		goto err;
> +
> +	/* Don't create thermal subkobjs if no data present */
> +	if (ixgbe_thermal_present(adapter->info_kobj) != true)
> +		goto exit;
> +
> +	for (i = 0; i < IXGBE_MAX_SENSORS; i++) {

'buf' ought to be declared here.

> +
> +		/*
> +		 * Likewise only create individual kobjs that have
> +		 * meaningful data.
> +		 */
> +		if (adapter->hw.mac.thermal_sensor_data.sensor[i].location == 0)
> +			continue;
> +
> +		/* directory named after sensor offset */
> +		snprintf(buf, sizeof(buf), "sensor_%d", i);
> +		adapter->therm_kobj[i] =
> +			kobject_create_and_add(buf, adapter->info_kobj);
> +		if (adapter->therm_kobj[i] == NULL)
> +			goto err;
> +		if (sysfs_create_group(adapter->therm_kobj[i],
> +				       &therm_attr_group))
> +			goto err;
> +	}
> +
> +	goto exit;
> +
> +err:
> +	ixgbe_del_adapter(adapter);
> +	rc = -1;
> +exit:
> +	return rc;
> +}
> +
> -- 
> 1.7.7.4
> 
> --
> 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

RE: [net-next 8/9] ixgbe: add interface to export thermal data

[Skidmore, Donald C]

>-----Original Message-----
>From: Francois Romieu [mailto:romieu@fr.zoreil.com]
>Sent: Friday, December 23, 2011 5:02 AM
>To: Kirsher, Jeffrey T
>Cc: davem@davemloft.net; Skidmore, Donald C; netdev@vger.kernel.org;
>gospo@redhat.com; sassmann@redhat.com; Waskiewicz Jr, Peter P
>Subject: Re: [net-next 8/9] ixgbe: add interface to export thermal data
>
>Jeff Kirsher <jeffrey.t.kirsher@intel.com> :
>> From: Don Skidmore <donald.c.skidmore@intel.com>
>>
>> Some of our adapters have thermal data available, this patch exports
>this
>> data via a read-only sysfs interface.  More patches will follow that
>will
>> contain additional information to be exported.
>>
>> Signed-off-by: Don Skidmore <donald.c.skidmore@intel.com>
>> Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
>> Tested-by: Stephen Ko <stephen.s.ko@intel.com>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> ---
>>  drivers/net/ethernet/intel/ixgbe/Makefile      |    2 +-
>>  drivers/net/ethernet/intel/ixgbe/ixgbe.h       |    4 +
>>  drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c |    2 +
>>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |    6 +
>>  drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c |  305
>++++++++++++++++++++++++
>[...]
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
>b/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
>> index 7720721..1a3810e 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
>> @@ -2137,6 +2137,8 @@ static struct ixgbe_mac_operations mac_ops_82599
>= {
>>  	.set_vlan_anti_spoofing = &ixgbe_set_vlan_anti_spoofing,
>>  	.acquire_swfw_sync      = &ixgbe_acquire_swfw_sync,
>>  	.release_swfw_sync      = &ixgbe_release_swfw_sync,
>> +	.get_thermal_sensor_data = &ixgbe_get_thermal_sensor_data_generic,
>> +	.init_thermal_sensor_thresh =
>&ixgbe_init_thermal_sensor_thresh_generic,
>
>.get_thermal_sensor_data and .init_thermal_sensor_thresh do not seem to
>be read anywhere.
>
>>
>>  };
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index e27e4d1..b5cef7e 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -7717,6 +7717,10 @@ static int __devinit ixgbe_probe(struct pci_dev
>*pdev,
>>
>>  	e_dev_info("Intel(R) 10 Gigabit Network Connection\n");
>>  	cards_found++;
>> +
>> +	if (ixgbe_sysfs_init(adapter))
>> +		e_err(probe, "failed to allocate sysfs resources\n");
>> +
>>  	return 0;
>>
>>  err_register:
>> @@ -7764,6 +7768,8 @@ static void __devexit ixgbe_remove(struct
>pci_dev *pdev)
>>  	}
>>
>>  #endif
>> +	ixgbe_sysfs_exit(adapter);
>> +
>>  #ifdef IXGBE_FCOE
>>  	if (adapter->flags & IXGBE_FLAG_FCOE_ENABLED)
>>  		ixgbe_cleanup_fcoe(adapter);
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
>b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
>> new file mode 100644
>> index 0000000..db818ae
>> --- /dev/null
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
>[...]
>> +static void ixgbe_del_adapter(struct ixgbe_adapter *adapter)
>
>Please name it ixgbe_sysfs_del_adapter or anything better.
>
>> +{
>> +	int i;
>> +
>> +	if (adapter == NULL)
>> +		return;
>> +
>> +	for (i = 0; i < IXGBE_MAX_SENSORS; i++) {
>> +		if (adapter->therm_kobj[i] == NULL)
>> +			continue;
>> +		sysfs_remove_group(adapter->therm_kobj[i],
>&therm_attr_group);
>> +		kobject_put(adapter->therm_kobj[i]);
>> +	}
>> +	if (adapter->info_kobj != NULL)
>> +		kobject_put(adapter->info_kobj);
>> +}
>> +
>> +/* called from ixgbe_main.c */
>> +void ixgbe_sysfs_exit(struct ixgbe_adapter *adapter)
>> +{
>> +	ixgbe_del_adapter(adapter);
>> +}
>> +
>> +/* called from ixgbe_main.c */
>> +int ixgbe_sysfs_init(struct ixgbe_adapter *adapter)
>> +{
>> +	struct net_device *netdev;
>> +	int rc = 0;
>> +	int i;
>> +	char buf[16];
>> +
>> +	if (adapter == NULL)
>> +		goto err;
>
>ixgbe_sysfs_init is only used in ixgbe_probe at a place where adapter
>can
>not be NULL.
>
>> +	netdev = adapter->netdev;
>> +	if (netdev == NULL)
>> +		goto err;
>
>netdev can not be NULL.
>
>> +
>> +	adapter->info_kobj = NULL;
>> +	for (i = 0; i < IXGBE_MAX_SENSORS; i++)
>> +		adapter->therm_kobj[i] = NULL;
>> +
>> +	/* create info kobj and attribute listings in kobj */
>> +	adapter->info_kobj = kobject_create_and_add("info",
>> +					&(netdev->dev.kobj));
>
>Remove comment and parenthesis. Everything can fit in a single line.
>
>> +	if (adapter->info_kobj == NULL)
>> +		goto err;
>> +
>> +	/* Don't create thermal subkobjs if no data present */
>> +	if (ixgbe_thermal_present(adapter->info_kobj) != true)
>> +		goto exit;
>> +
>> +	for (i = 0; i < IXGBE_MAX_SENSORS; i++) {
>
>'buf' ought to be declared here.
>
>> +
>> +		/*
>> +		 * Likewise only create individual kobjs that have
>> +		 * meaningful data.
>> +		 */
>> +		if (adapter->hw.mac.thermal_sensor_data.sensor[i].location
>== 0)
>> +			continue;
>> +
>> +		/* directory named after sensor offset */
>> +		snprintf(buf, sizeof(buf), "sensor_%d", i);
>> +		adapter->therm_kobj[i] =
>> +			kobject_create_and_add(buf, adapter->info_kobj);
>> +		if (adapter->therm_kobj[i] == NULL)
>> +			goto err;
>> +		if (sysfs_create_group(adapter->therm_kobj[i],
>> +				       &therm_attr_group))
>> +			goto err;
>> +	}
>> +
>> +	goto exit;
>> +
>> +err:
>> +	ixgbe_del_adapter(adapter);
>> +	rc = -1;
>> +exit:
>> +	return rc;
>> +}
>> +
>> --
>> 1.7.7.4
>>
>> --
>> 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


I like all your suggestions and will do the clean up.

Thanks again for the great review. :)

-Don Skidmore <donald.c.skidmore@intel.com>

Re: [net-next 8/9] ixgbe: add interface to export thermal data

[Francois Romieu]

Skidmore, Donald C <donald.c.skidmore@intel.com> :
[...]
> I like all your suggestions and will do the clean up.
> 
> Thanks again for the great review. :)

Depending on how you plan to use ixgbe_init_thermal_sensor_thresh_generic,
the memset(..., 0, ...) may be removed since it is applied within the
device own kzalloced struct ixgbe_hw.

I must confess I have not looked too closely at the sysfs code.

-- 
Ueimor

Re: [net-next 8/9] ixgbe: add interface to export thermal data

[Michał Mirosław]

2011/12/23 Jeff Kirsher <jeffrey.t.kirsher@intel.com>:
> From: Don Skidmore <donald.c.skidmore@intel.com>
>
> Some of our adapters have thermal data available, this patch exports this
> data via a read-only sysfs interface.

Just curious: can't this use the hwmon subsystem to be consistent with
other system monitoring devices?

Best Regards,
Michał Mirosław

Re: [net-next 8/9] ixgbe: add interface to export thermal data

[Ben Hutchings]

On Fri, 2011-12-23 at 18:58 +0100, Michał Mirosław wrote:
> 2011/12/23 Jeff Kirsher <jeffrey.t.kirsher@intel.com>:
> > From: Don Skidmore <donald.c.skidmore@intel.com>
> >
> > Some of our adapters have thermal data available, this patch exports this
> > data via a read-only sysfs interface.
> 
> Just curious: can't this use the hwmon subsystem to be consistent with
> other system monitoring devices?

That would certainly be the proper way to expose this...

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.

RE: [net-next 8/9] ixgbe: add interface to export thermal data

[Skidmore, Donald C]

>-----Original Message-----
>From: Michał Mirosław [mailto:mirqus@gmail.com]
>Sent: Friday, December 23, 2011 9:59 AM
>To: Kirsher, Jeffrey T
>Cc: davem@davemloft.net; Skidmore, Donald C; netdev@vger.kernel.org;
>gospo@redhat.com; sassmann@redhat.com; Waskiewicz Jr, Peter P
>Subject: Re: [net-next 8/9] ixgbe: add interface to export thermal data
>
>2011/12/23 Jeff Kirsher <jeffrey.t.kirsher@intel.com>:
>> From: Don Skidmore <donald.c.skidmore@intel.com>
>>
>> Some of our adapters have thermal data available, this patch exports
>this
>> data via a read-only sysfs interface.
>
>Just curious: can't this use the hwmon subsystem to be consistent with
>other system monitoring devices?
>
>Best Regards,
>Michał Mirosław

Sorry about the slow response, first vacation then I hadn't heard of hwmon and wanted to look into it a bit.  I can see why you mentioned it as it looks to be close to what I'm trying to do here.  However I don't think it quite matches.  I'll list my thoughts below:

- We are trying to export a large set of data that our customers are requesting.  The thermals were just the first patch and the other data items wouldn't really fit well in the hwmon (i.e. FW version, secondary MAC address).
- Didn't seem like we had much data to offer hwmon anyway just sensor temp, caution threshold, maxop threshold and location of sensor.  All the other data (which you haven't seen yet so couldn't have known :) wasn't related.
- The thermal data we do have is defined in our FW and could change (number of sensors) based on that FW.  I wasn't sure whether that would be an issue for hwmon.
- I went with sysfs based on a conversation with Peter Waskiewicz.  He mentioned that there was discussion on how to export generic data at netconf and sysfs was brought up as the best choice. 

Thanks for reviewing the patch and bring up this question. :)
-Don Skidmore <donald.c.skidmore@intel.com>




 

RE: [net-next 8/9] ixgbe: add interface to export thermal data

[Ben Hutchings]

On Thu, 2012-01-05 at 17:53 +0000, Skidmore, Donald C wrote:
> >-----Original Message-----
> >From: Michał Mirosław [mailto:mirqus@gmail.com]
> >Sent: Friday, December 23, 2011 9:59 AM
> >To: Kirsher, Jeffrey T
> >Cc: davem@davemloft.net; Skidmore, Donald C; netdev@vger.kernel.org;
> >gospo@redhat.com; sassmann@redhat.com; Waskiewicz Jr, Peter P
> >Subject: Re: [net-next 8/9] ixgbe: add interface to export thermal data
> >
> >2011/12/23 Jeff Kirsher <jeffrey.t.kirsher@intel.com>:
> >> From: Don Skidmore <donald.c.skidmore@intel.com>
> >>
> >> Some of our adapters have thermal data available, this patch exports
> >this
> >> data via a read-only sysfs interface.
> >
> >Just curious: can't this use the hwmon subsystem to be consistent with
> >other system monitoring devices?
> >
> >Best Regards,
> >Michał Mirosław
> 
> Sorry about the slow response, first vacation then I hadn't heard of
> hwmon and wanted to look into it a bit.  I can see why you mentioned
> it as it looks to be close to what I'm trying to do here.  However I
> don't think it quite matches.  I'll list my thoughts below:
> 
> - We are trying to export a large set of data that our customers are
> requesting.  The thermals were just the first patch and the other data
> items wouldn't really fit well in the hwmon (i.e. FW version,
> secondary MAC address).

Nevertheless, there is a specific way that the thermal information
should be exposed.

> - Didn't seem like we had much data to offer hwmon anyway just sensor
> temp, caution threshold, maxop threshold and location of sensor.  All
> the other data (which you haven't seen yet so couldn't have known :)
> wasn't related.
> - The thermal data we do have is defined in our FW and could change
> (number of sensors) based on that FW.  I wasn't sure whether that
> would be an issue for hwmon.

I have the same issue with the current Solarflare controllers, as the
driver has no static information about specific boards.  It is possible
to generate hwmon attributes dynamically though I've not yet got round
to completing my implementation of this.  (Since firmware is also
responsible for thermal shutdown, handling any of this stuff in the
driver has been a low priority.)

> - I went with sysfs based on a conversation with Peter Waskiewicz.  He
> mentioned that there was discussion on how to export generic data at
> netconf and sysfs was brought up as the best choice. 

Sensors are also exposed through sysfs, but following a specific naming
convention and using a separate hwmon device.

(Oddly, though, the sensor attributes are must be attached to the hwmon
device's parent - which will be your PCI device.  So you may not need to
make many changes.)

Ben.

> Thanks for reviewing the patch and bring up this question. :)
> -Don Skidmore <donald.c.skidmore@intel.com>

-- 
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.

RE: [net-next 8/9] ixgbe: add interface to export thermal data

[Skidmore, Donald C]

>-----Original Message-----
>From: Ben Hutchings [mailto:bhutchings@solarflare.com]
>Sent: Thursday, January 05, 2012 10:36 AM
>To: Skidmore, Donald C
>Cc: Michal Miroslaw; Kirsher, Jeffrey T; davem@davemloft.net;
>netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com;
>Waskiewicz Jr, Peter P
>Subject: RE: [net-next 8/9] ixgbe: add interface to export thermal data
>
>On Thu, 2012-01-05 at 17:53 +0000, Skidmore, Donald C wrote:
>> >-----Original Message-----
>> >From: Michał Mirosław [mailto:mirqus@gmail.com]
>> >Sent: Friday, December 23, 2011 9:59 AM
>> >To: Kirsher, Jeffrey T
>> >Cc: davem@davemloft.net; Skidmore, Donald C; netdev@vger.kernel.org;
>> >gospo@redhat.com; sassmann@redhat.com; Waskiewicz Jr, Peter P
>> >Subject: Re: [net-next 8/9] ixgbe: add interface to export thermal
>data
>> >
>> >2011/12/23 Jeff Kirsher <jeffrey.t.kirsher@intel.com>:
>> >> From: Don Skidmore <donald.c.skidmore@intel.com>
>> >>
>> >> Some of our adapters have thermal data available, this patch
>exports
>> >this
>> >> data via a read-only sysfs interface.
>> >
>> >Just curious: can't this use the hwmon subsystem to be consistent
>with
>> >other system monitoring devices?
>> >
>> >Best Regards,
>> >Michał Mirosław
>>
>> Sorry about the slow response, first vacation then I hadn't heard of
>> hwmon and wanted to look into it a bit.  I can see why you mentioned
>> it as it looks to be close to what I'm trying to do here.  However I
>> don't think it quite matches.  I'll list my thoughts below:
>>
>> - We are trying to export a large set of data that our customers are
>> requesting.  The thermals were just the first patch and the other data
>> items wouldn't really fit well in the hwmon (i.e. FW version,
>> secondary MAC address).
>
>Nevertheless, there is a specific way that the thermal information
>should be exposed.
>
>> - Didn't seem like we had much data to offer hwmon anyway just sensor
>> temp, caution threshold, maxop threshold and location of sensor.  All
>> the other data (which you haven't seen yet so couldn't have known :)
>> wasn't related.
>> - The thermal data we do have is defined in our FW and could change
>> (number of sensors) based on that FW.  I wasn't sure whether that
>> would be an issue for hwmon.
>
>I have the same issue with the current Solarflare controllers, as the
>driver has no static information about specific boards.  It is possible
>to generate hwmon attributes dynamically though I've not yet got round
>to completing my implementation of this.  (Since firmware is also
>responsible for thermal shutdown, handling any of this stuff in the
>driver has been a low priority.)
>
>> - I went with sysfs based on a conversation with Peter Waskiewicz.  He
>> mentioned that there was discussion on how to export generic data at
>> netconf and sysfs was brought up as the best choice.
>
>Sensors are also exposed through sysfs, but following a specific naming
>convention and using a separate hwmon device.
>
>(Oddly, though, the sensor attributes are must be attached to the hwmon
>device's parent - which will be your PCI device.  So you may not need to
>make many changes.)
>
>Ben.
>

Hey Ben,

Thanks for all the clarifications.  I think I have a better idea what would be required from an hwmon interface. 

My one real concern with this implementation is that the ixgbe driver will now have a dependency on hwmon.  The customers that are requesting this data aren't interested in using hwmon and would most likely just grab the thermal information (which is a small portion of the data they are asking for us to export) directly from sysfs.  Originally they wanted the FW to export the data directly, when this was found to be impossible the driver became the fall back option.

I do see your point on this being a defined way to expose thermals and I want to do this the correct way.  It just seems to put a lot of overhead on the driver (dependence on hwmon) for 4 fields that the user of this data won't even be using hwmon to look at.  I guess I would feel better if we had more data to provide hwmon then its support would seem more useful.

Thanks again for bring all this up.
-Don Skidmore <donald.c.skidmore@intel.com>

Re: [net-next 8/9] ixgbe: add interface to export thermal data

[Michał Mirosław]

2012/1/5 Skidmore, Donald C <donald.c.skidmore@intel.com>:
>>-----Original Message-----
>>From: Ben Hutchings [mailto:bhutchings@solarflare.com]
>>Sent: Thursday, January 05, 2012 10:36 AM
>>To: Skidmore, Donald C
>>Cc: Michal Miroslaw; Kirsher, Jeffrey T; davem@davemloft.net;
>>netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com;
>>Waskiewicz Jr, Peter P
>>Subject: RE: [net-next 8/9] ixgbe: add interface to export thermal data
>>
>>On Thu, 2012-01-05 at 17:53 +0000, Skidmore, Donald C wrote:
>>> >-----Original Message-----
>>> >From: Michał Mirosław [mailto:mirqus@gmail.com]
>>> >Sent: Friday, December 23, 2011 9:59 AM
>>> >To: Kirsher, Jeffrey T
>>> >Cc: davem@davemloft.net; Skidmore, Donald C; netdev@vger.kernel.org;
>>> >gospo@redhat.com; sassmann@redhat.com; Waskiewicz Jr, Peter P
>>> >Subject: Re: [net-next 8/9] ixgbe: add interface to export thermal
>>data
>>> >
>>> >2011/12/23 Jeff Kirsher <jeffrey.t.kirsher@intel.com>:
>>> >> From: Don Skidmore <donald.c.skidmore@intel.com>
>>> >>
>>> >> Some of our adapters have thermal data available, this patch
>>exports
>>> >this
>>> >> data via a read-only sysfs interface.
>>> >
>>> >Just curious: can't this use the hwmon subsystem to be consistent
>>with
>>> >other system monitoring devices?
>>> >
>>> >Best Regards,
>>> >Michał Mirosław
>>>
>>> Sorry about the slow response, first vacation then I hadn't heard of
>>> hwmon and wanted to look into it a bit.  I can see why you mentioned
>>> it as it looks to be close to what I'm trying to do here.  However I
>>> don't think it quite matches.  I'll list my thoughts below:
>>>
>>> - We are trying to export a large set of data that our customers are
>>> requesting.  The thermals were just the first patch and the other data
>>> items wouldn't really fit well in the hwmon (i.e. FW version,
>>> secondary MAC address).
>>
>>Nevertheless, there is a specific way that the thermal information
>>should be exposed.
>>
>>> - Didn't seem like we had much data to offer hwmon anyway just sensor
>>> temp, caution threshold, maxop threshold and location of sensor.  All
>>> the other data (which you haven't seen yet so couldn't have known :)
>>> wasn't related.
>>> - The thermal data we do have is defined in our FW and could change
>>> (number of sensors) based on that FW.  I wasn't sure whether that
>>> would be an issue for hwmon.
>>
>>I have the same issue with the current Solarflare controllers, as the
>>driver has no static information about specific boards.  It is possible
>>to generate hwmon attributes dynamically though I've not yet got round
>>to completing my implementation of this.  (Since firmware is also
>>responsible for thermal shutdown, handling any of this stuff in the
>>driver has been a low priority.)
>>
>>> - I went with sysfs based on a conversation with Peter Waskiewicz.  He
>>> mentioned that there was discussion on how to export generic data at
>>> netconf and sysfs was brought up as the best choice.
>>
>>Sensors are also exposed through sysfs, but following a specific naming
>>convention and using a separate hwmon device.
>>
>>(Oddly, though, the sensor attributes are must be attached to the hwmon
>>device's parent - which will be your PCI device.  So you may not need to
>>make many changes.)
>>
>>Ben.
>>
>
> Hey Ben,
>
> Thanks for all the clarifications.  I think I have a better idea what would be required from an hwmon interface.
>
> My one real concern with this implementation is that the ixgbe driver will now have a dependency on hwmon.  The customers that are requesting this data aren't interested in using hwmon and would most likely just grab the thermal information (which is a small portion of the data they are asking for us to export) directly from sysfs.  Originally they wanted the FW to export the data directly, when this was found to be impossible the driver became the fall back option.
>
> I do see your point on this being a defined way to expose thermals and I want to do this the correct way.  It just seems to put a lot of overhead on the driver (dependence on hwmon) for 4 fields that the user of this data won't even be using hwmon to look at.  I guess I would feel better if we had more data to provide hwmon then its support would seem more useful.

Drivers outside of drivers/hwmon just select HWMON in Kconfig. This
adds a 3kB .c file to the kernel build for the first one that needs
it.

Best Regards,
Michał Mirosław

RE: [net-next 8/9] ixgbe: add interface to export thermal data

[Waskiewicz Jr, Peter P]

[-- Attachment #1: Type: text/plain, Size: 1641 bytes --]

> -----Original Message-----
> From: Michał Mirosław [mailto:mirqus@gmail.com]
> Sent: Thursday, January 05, 2012 3:51 PM
> To: Skidmore, Donald C
> Cc: Ben Hutchings; Kirsher, Jeffrey T; davem@davemloft.net;
> netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com;
> Waskiewicz Jr, Peter P
> Subject: Re: [net-next 8/9] ixgbe: add interface to export thermal data
> 
> Drivers outside of drivers/hwmon just select HWMON in Kconfig. This adds a
> 3kB .c file to the kernel build for the first one that needs it.

This is where we run into issues though.  We can put this dependency in, but
customers don't pull upstream kernels, they rely on the OSV's to distribute
updates.  The customer doesn't want HWMON, and if their kernel doesn't ship
with HWMON support for ixgbe, then we're sunk.

The point is what we're trying to implement, based on what our customers'
requirements are, is something completely different than what HWMON is
providing.  Trying to wedge HWMON onto this framework we're trying to
provide just overcomplicates the entire thing we're trying to provide.  It's
a generic interface to generic data in our drivers, which just happens to
include thermal data on this round of patches.  If we put this under HWMON,
then we'll have multiple places for this generic data, which just
complicates the whole thing, which is what we're trying to avoid.

We did dig into the HWMON specifics on how to set it up and use it, and it's
not the right tool for what we're doing.  I don't see how changing these
patches to use HWMON improves anything, and would rather not.

Cheers,
-PJ Waskiewicz

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5264 bytes --]

RE: [net-next 8/9] ixgbe: add interface to export thermal data

[Ben Hutchings]

On Fri, 2012-01-06 at 17:55 +0000, Waskiewicz Jr, Peter P wrote:
> > -----Original Message-----
> > From: Michał Mirosław [mailto:mirqus@gmail.com]
> > Sent: Thursday, January 05, 2012 3:51 PM
> > To: Skidmore, Donald C
> > Cc: Ben Hutchings; Kirsher, Jeffrey T; davem@davemloft.net;
> > netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com;
> > Waskiewicz Jr, Peter P
> > Subject: Re: [net-next 8/9] ixgbe: add interface to export thermal data
> > 
> > Drivers outside of drivers/hwmon just select HWMON in Kconfig. This adds a
> > 3kB .c file to the kernel build for the first one that needs it.
> 
> This is where we run into issues though.  We can put this dependency in, but
> customers don't pull upstream kernels, they rely on the OSV's to distribute
> updates.  The customer doesn't want HWMON, and if their kernel doesn't ship
> with HWMON support for ixgbe, then we're sunk.

So if !IS_ENABLED(CONFIG_HWMON), don't try to create an hwmon device,
but create the attributes anyway.

> The point is what we're trying to implement, based on what our customers'
> requirements are, is something completely different than what HWMON is
> providing.  Trying to wedge HWMON onto this framework we're trying to
> provide just overcomplicates the entire thing we're trying to provide.  It's
> a generic interface to generic data in our drivers,
[...]

It sounds like you want to provide a "generic" ixgbe interface on
different operating systems.  But that is not a valid argument for an
in-tree driver.

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.

RE: [net-next 8/9] ixgbe: add interface to export thermal data

[Waskiewicz Jr, Peter P]

[-- Attachment #1: Type: text/plain, Size: 3554 bytes --]

> -----Original Message-----
> From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> Sent: Friday, January 06, 2012 10:09 AM
> To: Waskiewicz Jr, Peter P
> Cc: Michal Miroslaw; Skidmore, Donald C; Kirsher, Jeffrey T;
> davem@davemloft.net; netdev@vger.kernel.org; gospo@redhat.com;
> sassmann@redhat.com
> Subject: RE: [net-next 8/9] ixgbe: add interface to export thermal data
> 
> On Fri, 2012-01-06 at 17:55 +0000, Waskiewicz Jr, Peter P wrote:
> > > -----Original Message-----
> > > From: Michał Mirosław [mailto:mirqus@gmail.com]
> > > Sent: Thursday, January 05, 2012 3:51 PM
> > > To: Skidmore, Donald C
> > > Cc: Ben Hutchings; Kirsher, Jeffrey T; davem@davemloft.net;
> > > netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com;
> > > Waskiewicz Jr, Peter P
> > > Subject: Re: [net-next 8/9] ixgbe: add interface to export thermal
> > > data
> > >
> > > Drivers outside of drivers/hwmon just select HWMON in Kconfig. This
> > > adds a 3kB .c file to the kernel build for the first one that needs it.
> >
> > This is where we run into issues though.  We can put this dependency
> > in, but customers don't pull upstream kernels, they rely on the OSV's
> > to distribute updates.  The customer doesn't want HWMON, and if their
> > kernel doesn't ship with HWMON support for ixgbe, then we're sunk.
> 
> So if !IS_ENABLED(CONFIG_HWMON), don't try to create an hwmon device,
> but create the attributes anyway.

And if CONFIG_HWMON is not enabled, are we then responsible for creating the hwmon area in sysfs to link in our attributes?  That would make even less sense to include HWMON attributes if HWMON isn't built in.

> > The point is what we're trying to implement, based on what our customers'
> > requirements are, is something completely different than what HWMON is
> > providing.  Trying to wedge HWMON onto this framework we're trying to
> > provide just overcomplicates the entire thing we're trying to provide.
> > It's a generic interface to generic data in our drivers,
> [...]
> 
> It sounds like you want to provide a "generic" ixgbe interface on different
> operating systems.  But that is not a valid argument for an in-tree driver.

No, we're trying to provide a generic ixgbe interface on Linux systems.  The reality is different OSV's pull our driver from upstream to build their distros, and they cherry pick patches from our drivers to achieve this.  Upstream feeds the OSV's.  Ignoring that is wrong.  We're using a common interface, sysfs, which we discussed at NetConf, and the general consensus supported these types of data to be exported by a driver.  Wrapping HWMON around this just for the sake of wrapping it around it makes no sense.  There are no plans for anyone using ixgbe to use or deploy HWMON.  So it just adds extra useless crap to our drivers and the kernel, and complicates our job of maintaining the driver by using a framework that provides no benefit to our driver or those using it.

The reality is these attributes of our thermal sensors are about 5% of the data we need to export.  The other 95% of the data, which Don stated are coming soon (still finalizing everything and testing), have nothing to do with thermal sensors.  So from a maintenance perspective, it makes it much uglier to maintain all this exported data when it's in multiple locations.  That's why we chose a contained model within the driver's sysfs space.  It doesn't affect anything else in the kernel space, and it keeps it tidy and easy to find all the data.

-PJ

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5264 bytes --]

RE: [net-next 8/9] ixgbe: add interface to export thermal data

[Ben Hutchings]

On Fri, 2012-01-06 at 18:20 +0000, Waskiewicz Jr, Peter P wrote:
> > -----Original Message-----
> > From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> > Sent: Friday, January 06, 2012 10:09 AM
> > To: Waskiewicz Jr, Peter P
> > Cc: Michal Miroslaw; Skidmore, Donald C; Kirsher, Jeffrey T;
> > davem@davemloft.net; netdev@vger.kernel.org; gospo@redhat.com;
> > sassmann@redhat.com
> > Subject: RE: [net-next 8/9] ixgbe: add interface to export thermal data
> > 
> > On Fri, 2012-01-06 at 17:55 +0000, Waskiewicz Jr, Peter P wrote:
> > > > -----Original Message-----
> > > > From: Michał Mirosław [mailto:mirqus@gmail.com]
> > > > Sent: Thursday, January 05, 2012 3:51 PM
> > > > To: Skidmore, Donald C
> > > > Cc: Ben Hutchings; Kirsher, Jeffrey T; davem@davemloft.net;
> > > > netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com;
> > > > Waskiewicz Jr, Peter P
> > > > Subject: Re: [net-next 8/9] ixgbe: add interface to export thermal
> > > > data
> > > >
> > > > Drivers outside of drivers/hwmon just select HWMON in Kconfig. This
> > > > adds a 3kB .c file to the kernel build for the first one that needs it.
> > >
> > > This is where we run into issues though.  We can put this dependency
> > > in, but customers don't pull upstream kernels, they rely on the OSV's
> > > to distribute updates.  The customer doesn't want HWMON, and if their
> > > kernel doesn't ship with HWMON support for ixgbe, then we're sunk.
> > 
> > So if !IS_ENABLED(CONFIG_HWMON), don't try to create an hwmon device,
> > but create the attributes anyway.
> 
> And if CONFIG_HWMON is not enabled, are we then responsible for
> creating the hwmon area in sysfs to link in our attributes?  That
> would make even less sense to include HWMON attributes if HWMON isn't
> built in.

No, the attributes actually appear under the parent device.

> > > The point is what we're trying to implement, based on what our customers'
> > > requirements are, is something completely different than what HWMON is
> > > providing.  Trying to wedge HWMON onto this framework we're trying to
> > > provide just overcomplicates the entire thing we're trying to provide.
> > > It's a generic interface to generic data in our drivers,
> > [...]
> > 
> > It sounds like you want to provide a "generic" ixgbe interface on different
> > operating systems.  But that is not a valid argument for an in-tree driver.
> 
> No, we're trying to provide a generic ixgbe interface on Linux
> systems.  The reality is different OSV's pull our driver from upstream
> to build their distros,

Yes we know.

[...]
> The reality is these attributes of our thermal sensors are about 5% of
> the data we need to export.
[...]

And I think all you need to do is (1) give these particular attributes
the right names and (2) create a child hwmon device if possible.  Is
that so hard?

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.

RE: [net-next 8/9] ixgbe: add interface to export thermal data

[Waskiewicz Jr, Peter P]

[-- Attachment #1: Type: text/plain, Size: 4149 bytes --]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Ben Hutchings
> Sent: Friday, January 06, 2012 10:25 AM
> To: Waskiewicz Jr, Peter P
> Cc: Michal Miroslaw; Skidmore, Donald C; Kirsher, Jeffrey T;
> davem@davemloft.net; netdev@vger.kernel.org; gospo@redhat.com;
> sassmann@redhat.com
> Subject: RE: [net-next 8/9] ixgbe: add interface to export thermal data
> 
> On Fri, 2012-01-06 at 18:20 +0000, Waskiewicz Jr, Peter P wrote:
> > > -----Original Message-----
> > > From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> > > Sent: Friday, January 06, 2012 10:09 AM
> > > To: Waskiewicz Jr, Peter P
> > > Cc: Michal Miroslaw; Skidmore, Donald C; Kirsher, Jeffrey T;
> > > davem@davemloft.net; netdev@vger.kernel.org; gospo@redhat.com;
> > > sassmann@redhat.com
> > > Subject: RE: [net-next 8/9] ixgbe: add interface to export thermal
> > > data
> > >
> > > On Fri, 2012-01-06 at 17:55 +0000, Waskiewicz Jr, Peter P wrote:
> > > > > -----Original Message-----
> > > > > From: Michał Mirosław [mailto:mirqus@gmail.com]
> > > > > Sent: Thursday, January 05, 2012 3:51 PM
> > > > > To: Skidmore, Donald C
> > > > > Cc: Ben Hutchings; Kirsher, Jeffrey T; davem@davemloft.net;
> > > > > netdev@vger.kernel.org; gospo@redhat.com;
> sassmann@redhat.com;
> > > > > Waskiewicz Jr, Peter P
> > > > > Subject: Re: [net-next 8/9] ixgbe: add interface to export
> > > > > thermal data
> > > > >
> > > > > Drivers outside of drivers/hwmon just select HWMON in Kconfig.
> > > > > This adds a 3kB .c file to the kernel build for the first one that needs it.
> > > >
> > > > This is where we run into issues though.  We can put this
> > > > dependency in, but customers don't pull upstream kernels, they
> > > > rely on the OSV's to distribute updates.  The customer doesn't
> > > > want HWMON, and if their kernel doesn't ship with HWMON support
> for ixgbe, then we're sunk.
> > >
> > > So if !IS_ENABLED(CONFIG_HWMON), don't try to create an hwmon
> > > device, but create the attributes anyway.
> >
> > And if CONFIG_HWMON is not enabled, are we then responsible for
> > creating the hwmon area in sysfs to link in our attributes?  That
> > would make even less sense to include HWMON attributes if HWMON isn't
> > built in.
> 
> No, the attributes actually appear under the parent device.

Ok, so how we have it now in the current patches.

> > > > The point is what we're trying to implement, based on what our
> customers'
> > > > requirements are, is something completely different than what
> > > > HWMON is providing.  Trying to wedge HWMON onto this framework
> > > > we're trying to provide just overcomplicates the entire thing we're trying
> to provide.
> > > > It's a generic interface to generic data in our drivers,
> > > [...]
> > >
> > > It sounds like you want to provide a "generic" ixgbe interface on
> > > different operating systems.  But that is not a valid argument for an in-
> tree driver.
> >
> > No, we're trying to provide a generic ixgbe interface on Linux
> > systems.  The reality is different OSV's pull our driver from upstream
> > to build their distros,
> 
> Yes we know.
> 
> [...]
> > The reality is these attributes of our thermal sensors are about 5% of
> > the data we need to export.
> [...]
> 
> And I think all you need to do is (1) give these particular attributes the right
> names and (2) create a child hwmon device if possible.  Is that so hard?

Yes, it is.  The sensors that we have defined for this SKU are defined by the customers who this board was built for, so it fits with their BMC.  We don't have much control over the names.

Again, I'm failing to see why you are requiring us to put in this support when we don't benefit at all from it, we don't need it, and don't want it.  Perhaps you can add the HWMON support to ixgbe when you get around to adding the same support to your Solarflare driver that hasn't been completed yet?  We'd be happy to accept those patches.  Please don't hold us to a standard you won't hold your own driver to.

-PJ

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5264 bytes --]

RE: [net-next 8/9] ixgbe: add interface to export thermal data

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 5077 bytes --]

On Mon, 2012-01-09 at 18:08 +0000, Waskiewicz Jr, Peter P wrote:
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org [mailto:netdev-
> > owner@vger.kernel.org] On Behalf Of Ben Hutchings
> > Sent: Friday, January 06, 2012 10:25 AM
> > To: Waskiewicz Jr, Peter P
> > Cc: Michal Miroslaw; Skidmore, Donald C; Kirsher, Jeffrey T;
> > davem@davemloft.net; netdev@vger.kernel.org; gospo@redhat.com;
> > sassmann@redhat.com
> > Subject: RE: [net-next 8/9] ixgbe: add interface to export thermal data
> > 
> > On Fri, 2012-01-06 at 18:20 +0000, Waskiewicz Jr, Peter P wrote:
> > > > -----Original Message-----
> > > > From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> > > > Sent: Friday, January 06, 2012 10:09 AM
> > > > To: Waskiewicz Jr, Peter P
> > > > Cc: Michal Miroslaw; Skidmore, Donald C; Kirsher, Jeffrey T;
> > > > davem@davemloft.net; netdev@vger.kernel.org; gospo@redhat.com;
> > > > sassmann@redhat.com
> > > > Subject: RE: [net-next 8/9] ixgbe: add interface to export thermal
> > > > data
> > > >
> > > > On Fri, 2012-01-06 at 17:55 +0000, Waskiewicz Jr, Peter P wrote:
> > > > > > -----Original Message-----
> > > > > > From: Michał Mirosław [mailto:mirqus@gmail.com]
> > > > > > Sent: Thursday, January 05, 2012 3:51 PM
> > > > > > To: Skidmore, Donald C
> > > > > > Cc: Ben Hutchings; Kirsher, Jeffrey T; davem@davemloft.net;
> > > > > > netdev@vger.kernel.org; gospo@redhat.com;
> > sassmann@redhat.com;
> > > > > > Waskiewicz Jr, Peter P
> > > > > > Subject: Re: [net-next 8/9] ixgbe: add interface to export
> > > > > > thermal data
> > > > > >
> > > > > > Drivers outside of drivers/hwmon just select HWMON in Kconfig.
> > > > > > This adds a 3kB .c file to the kernel build for the first one that needs it.
> > > > >
> > > > > This is where we run into issues though.  We can put this
> > > > > dependency in, but customers don't pull upstream kernels, they
> > > > > rely on the OSV's to distribute updates.  The customer doesn't
> > > > > want HWMON, and if their kernel doesn't ship with HWMON support
> > for ixgbe, then we're sunk.
> > > >
> > > > So if !IS_ENABLED(CONFIG_HWMON), don't try to create an hwmon
> > > > device, but create the attributes anyway.
> > >
> > > And if CONFIG_HWMON is not enabled, are we then responsible for
> > > creating the hwmon area in sysfs to link in our attributes?  That
> > > would make even less sense to include HWMON attributes if HWMON isn't
> > > built in.
> > 
> > No, the attributes actually appear under the parent device.
> 
> Ok, so how we have it now in the current patches.

Only with different names.

> > > > > The point is what we're trying to implement, based on what our
> > customers'
> > > > > requirements are, is something completely different than what
> > > > > HWMON is providing.  Trying to wedge HWMON onto this framework
> > > > > we're trying to provide just overcomplicates the entire thing we're trying
> > to provide.
> > > > > It's a generic interface to generic data in our drivers,
> > > > [...]
> > > >
> > > > It sounds like you want to provide a "generic" ixgbe interface on
> > > > different operating systems.  But that is not a valid argument for an in-
> > tree driver.
> > >
> > > No, we're trying to provide a generic ixgbe interface on Linux
> > > systems.  The reality is different OSV's pull our driver from upstream
> > > to build their distros,
> > 
> > Yes we know.
> > 
> > [...]
> > > The reality is these attributes of our thermal sensors are about 5% of
> > > the data we need to export.
> > [...]
>
> > And I think all you need to do is (1) give these particular attributes the right
> > names and (2) create a child hwmon device if possible.  Is that so hard?
> 
> Yes, it is.  The sensors that we have defined for this SKU are defined
> by the customers who this board was built for, so it fits with their
> BMC.  We don't have much control over the names.

You should expose the names as '<type><index>_label' attributes.

> Again, I'm failing to see why you are requiring us to put in this
> support when we don't benefit at all from it, we don't need it, and
> don't want it.  Perhaps you can add the HWMON support to ixgbe when
> you get around to adding the same support to your Solarflare driver
> that hasn't been completed yet?

I already did that for the SFC4000 boards, using existing hwmon drivers.
I'm attaching the implementation for SFC9000-family boards (though it's
not immediately applicable as it needs an update to the firmware
protocol header).  Oddly enough I'm not inclined to spend time on your
boards!  But maybe this can serve as an example of how to expose sensors
that are discoverd at run-time.

> We'd be happy to accept those patches.  Please don't hold us to a
> standard you won't hold your own driver to.

I'm not the one proposing to add a driver-specific interface for
something that's already standardised!

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.

[-- Attachment #2: 0001-sfc-Add-hardware-monitor-for-sensors-managed-by-firm.patch --]
[-- Type: text/x-patch, Size: 16610 bytes --]

>From a64b65e8e5efee3b176ae395e30132bff7564440 Mon Sep 17 00:00:00 2001
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Fri, 6 Jan 2012 20:25:39 +0000
Subject: [PATCH net-next] sfc: Add hardware monitor for sensors managed by
 firmware
To: David Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org, linux-net-drivers@solarflare.com

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/ethernet/sfc/Kconfig    |    7 +
 drivers/net/ethernet/sfc/Makefile   |    1 +
 drivers/net/ethernet/sfc/mcdi.h     |   27 +++
 drivers/net/ethernet/sfc/mcdi_mon.c |  368 +++++++++++++++++++++++++++++++++++
 drivers/net/ethernet/sfc/nic.h      |   14 ++
 drivers/net/ethernet/sfc/siena.c    |    6 +
 6 files changed, 423 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/ethernet/sfc/mcdi_mon.c

diff --git a/drivers/net/ethernet/sfc/Kconfig b/drivers/net/ethernet/sfc/Kconfig
index 5d18841..ae40b66 100644
--- a/drivers/net/ethernet/sfc/Kconfig
+++ b/drivers/net/ethernet/sfc/Kconfig
@@ -19,3 +19,10 @@ config SFC_MTD
 	  This exposes the on-board flash memory as MTD devices (e.g.
 	  /dev/mtd1).  This makes it possible to upload new firmware
 	  to the NIC.
+config SFC_MCDI_MON
+	bool "Solarflare SFC9000-family hwmon support"
+	depends on SFC && HWMON && !(SFC=y && HWMON=m)
+	default y
+	----help---
+	  This exposes the on-board firmware-managed sensors as a
+	  hardware monitor device.
diff --git a/drivers/net/ethernet/sfc/Makefile b/drivers/net/ethernet/sfc/Makefile
index ab31c71..3a1aa84 100644
--- a/drivers/net/ethernet/sfc/Makefile
+++ b/drivers/net/ethernet/sfc/Makefile
@@ -4,5 +4,6 @@ sfc-y			+= efx.o nic.o falcon.o siena.o tx.o rx.o filter.o \
 			   tenxpress.o txc43128_phy.o falcon_boards.o \
 			   mcdi.o mcdi_phy.o
 sfc-$(CONFIG_SFC_MTD)	+= mtd.o
+sfc-$(CONFIG_SFC_MCDI_MON) += mcdi_mon.o
 
 obj-$(CONFIG_SFC)	+= sfc.o
diff --git a/drivers/net/ethernet/sfc/mcdi.h b/drivers/net/ethernet/sfc/mcdi.h
index 4dd39fc..0af1bd4 100644
--- a/drivers/net/ethernet/sfc/mcdi.h
+++ b/drivers/net/ethernet/sfc/mcdi.h
@@ -56,6 +56,15 @@ struct efx_mcdi_iface {
 	size_t resplen;
 };
 
+struct efx_mcdi_mon {
+	struct efx_buffer dma_buf;
+	struct mutex update_lock;
+	unsigned long last_update;
+	struct device *device;
+	struct efx_mcdi_mon_attribute *attrs;
+	unsigned int n_attrs;
+};
+
 extern void efx_mcdi_init(struct efx_nic *efx);
 
 extern int efx_mcdi_rpc(struct efx_nic *efx, unsigned cmd, const u8 *inbuf,
@@ -83,6 +92,10 @@ extern void efx_mcdi_process_event(struct efx_channel *channel,
 
 #define MCDI_PTR(_buf, _ofst)						\
 	MCDI_PTR2(_buf, MC_CMD_ ## _ofst ## _OFST)
+#define MCDI_ARRAY_PTR(_buf, _field, _type, _index)			\
+	MCDI_PTR2(_buf,							\
+		  MC_CMD_ ## _field ## _OFST +				\
+		  (_index) * MC_CMD_ ## _type ## _TYPEDEF_LEN)
 #define MCDI_SET_DWORD(_buf, _ofst, _value)				\
 	MCDI_SET_DWORD2(_buf, MC_CMD_ ## _ofst ## _OFST, _value)
 #define MCDI_DWORD(_buf, _ofst)						\
@@ -92,6 +105,12 @@ extern void efx_mcdi_process_event(struct efx_channel *channel,
 
 #define MCDI_EVENT_FIELD(_ev, _field)			\
 	EFX_QWORD_FIELD(_ev, MCDI_EVENT_ ## _field)
+#define MCDI_ARRAY_FIELD(_buf, _field1, _type, _index, _field2)		\
+	EFX_DWORD_FIELD(						\
+		*((efx_dword_t *)					\
+		  (MCDI_ARRAY_PTR(_buf, _field1, _type, _index) +	\
+		   (MC_CMD_ ## _type ## _TYPEDEF_ ## _field2 ## _OFST & ~3))), \
+		MC_CMD_ ## _type ## _TYPEDEF_ ## _field2)
 
 extern void efx_mcdi_print_fwver(struct efx_nic *efx, char *buf, size_t len);
 extern int efx_mcdi_drv_attach(struct efx_nic *efx, bool driver_operating,
@@ -131,4 +150,12 @@ extern int efx_mcdi_mac_stats(struct efx_nic *efx, dma_addr_t dma_addr,
 extern int efx_mcdi_mac_reconfigure(struct efx_nic *efx);
 extern bool efx_mcdi_mac_check_fault(struct efx_nic *efx);
 
+#ifdef CONFIG_SFC_MCDI_MON
+extern int efx_mcdi_mon_probe(struct efx_nic *efx);
+extern void efx_mcdi_mon_remove(struct efx_nic *efx);
+#else
+static inline int efx_mcdi_mon_probe(struct efx_nic *efx) { return 0; }
+static inline void efx_mcdi_mon_remove(struct efx_nic *efx) {}
+#endif
+
 #endif /* EFX_MCDI_H */
diff --git a/drivers/net/ethernet/sfc/mcdi_mon.c b/drivers/net/ethernet/sfc/mcdi_mon.c
new file mode 100644
index 0000000..ce4e49e
--- /dev/null
+++ b/drivers/net/ethernet/sfc/mcdi_mon.c
@@ -0,0 +1,368 @@
+/****************************************************************************
+ * Driver for Solarflare Solarstorm network controllers and boards
+ * Copyright 2011 Solarflare Communications Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation, incorporated herein by reference.
+ */
+
+#include <linux/bitops.h>
+#include <linux/slab.h>
+#include <linux/hwmon.h>
+#include <linux/stat.h>
+
+#include "net_driver.h"
+#include "mcdi.h"
+#include "mcdi_pcol.h"
+#include "nic.h"
+
+enum efx_hwmon_type {
+	EFX_HWMON_UNKNOWN,
+	EFX_HWMON_TEMP,         /* temperature */
+	EFX_HWMON_COOL,         /* cooling device, probably a heatsink */
+	EFX_HWMON_IN            /* input voltage */
+};
+
+static const struct {
+	const char *label;
+	enum efx_hwmon_type hwmon_type;
+	int port;
+} efx_mcdi_sensor_type[MC_CMD_SENSOR_ENTRY_MAXNUM] = {
+#define SENSOR_TYPE(name, label, hwmon_type, port)			\
+	[MC_CMD_SENSOR_##name] = { label, hwmon_type, port }
+	SENSOR_TYPE(CONTROLLER_TEMP,	"NIC temp.",	EFX_HWMON_TEMP,	-1),
+	SENSOR_TYPE(PHY_COMMON_TEMP,	"PHY temp.",	EFX_HWMON_TEMP,	-1),
+	SENSOR_TYPE(CONTROLLER_COOLING,	"NIC cooling",	EFX_HWMON_COOL,	-1),
+	SENSOR_TYPE(PHY0_TEMP,		"PHY temp.",	EFX_HWMON_TEMP,	0),
+	SENSOR_TYPE(PHY0_COOLING,	"PHY cooling",	EFX_HWMON_COOL,	0),
+	SENSOR_TYPE(PHY1_TEMP,		"PHY temp.",	EFX_HWMON_TEMP,	1),
+	SENSOR_TYPE(PHY1_COOLING, 	"PHY cooling",	EFX_HWMON_COOL,	1),
+	SENSOR_TYPE(IN_1V0,		"1.0V",		EFX_HWMON_IN,	-1),
+	SENSOR_TYPE(IN_1V2,		"1.2V",		EFX_HWMON_IN,	-1),
+	SENSOR_TYPE(IN_1V8,		"1.8V",		EFX_HWMON_IN,	-1),
+	SENSOR_TYPE(IN_2V5,		"2.5V",		EFX_HWMON_IN,	-1),
+	SENSOR_TYPE(IN_3V3,		"3.3V",		EFX_HWMON_IN,	-1),
+	SENSOR_TYPE(IN_12V0,		"12.0V",	EFX_HWMON_IN,	-1),
+	SENSOR_TYPE(IN_1V2A,		"1.2V analogue", EFX_HWMON_IN,	-1),
+	SENSOR_TYPE(IN_VREF,		"ref. voltage",	EFX_HWMON_IN,	-1),
+};
+
+struct efx_mcdi_mon_attribute {
+	struct device_attribute dev_attr;
+	unsigned int index;
+	unsigned int limit_value;
+	char name[12];
+};
+
+static int efx_mcdi_mon_update(struct efx_nic *efx)
+{
+	struct efx_mcdi_mon *hwmon = efx_mcdi_mon(efx);
+	u8 inbuf[MC_CMD_READ_SENSORS_IN_LEN];
+	int rc;
+
+	MCDI_SET_DWORD(inbuf, READ_SENSORS_IN_DMA_ADDR_LO,
+		       hwmon->dma_buf.dma_addr & 0xffffffff);
+	MCDI_SET_DWORD(inbuf, READ_SENSORS_IN_DMA_ADDR_HI,
+		       (u64)hwmon->dma_buf.dma_addr >> 32);
+
+	rc = efx_mcdi_rpc(efx, MC_CMD_READ_SENSORS,
+			  inbuf, sizeof(inbuf), NULL, 0, NULL);
+	if (rc == 0)
+		hwmon->last_update = jiffies;
+	return rc;
+}
+
+static ssize_t efx_mcdi_mon_show_name(struct device *dev,
+				      struct device_attribute *attr,
+				      char *buf)
+{
+	return sprintf(buf, "%s\n", KBUILD_MODNAME);
+}
+
+static int efx_mcdi_mon_get_entry(struct device *dev, unsigned int index,
+				  efx_dword_t *entry)
+{
+	struct efx_nic *efx = dev_get_drvdata(dev);
+	struct efx_mcdi_mon *hwmon = efx_mcdi_mon(efx);
+	int rc;
+
+	BUILD_BUG_ON(MC_CMD_READ_SENSORS_OUT_LEN != 0);
+
+	mutex_lock(&hwmon->update_lock);
+
+	/* Use cached value if last update was < 1 s ago */
+	if (time_before(jiffies, hwmon->last_update + HZ))
+		rc = 0;
+	else
+		rc = efx_mcdi_mon_update(efx);
+
+	/* Copy out the requested entry */
+	*entry = ((efx_dword_t *)hwmon->dma_buf.addr)[index];
+
+	mutex_unlock(&hwmon->update_lock);
+
+	return rc;
+}
+
+static ssize_t efx_mcdi_mon_show_value(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	struct efx_mcdi_mon_attribute *mon_attr =
+		container_of(attr, struct efx_mcdi_mon_attribute, dev_attr);
+	efx_dword_t entry;
+	unsigned int value;
+	int rc;
+
+	rc = efx_mcdi_mon_get_entry(dev, mon_attr->index, &entry);
+	if (rc)
+		return rc;
+
+	value = EFX_DWORD_FIELD(entry, MC_CMD_SENSOR_VALUE_ENTRY_TYPEDEF_VALUE);
+
+	/* Convert temperature from degrees to milli-degrees Celsius */
+	if (efx_mcdi_sensor_type[mon_attr->index].hwmon_type == EFX_HWMON_TEMP)
+		value *= 1000;
+
+	return sprintf(buf, "%u\n", value);
+}
+
+static ssize_t efx_mcdi_mon_show_limit(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	struct efx_mcdi_mon_attribute *mon_attr =
+		container_of(attr, struct efx_mcdi_mon_attribute, dev_attr);
+	unsigned int value;
+
+	value = mon_attr->limit_value;
+
+	/* Convert temperature from degrees to milli-degrees Celsius */
+	if (efx_mcdi_sensor_type[mon_attr->index].hwmon_type == EFX_HWMON_TEMP)
+		value *= 1000;
+
+	return sprintf(buf, "%u\n", value);
+}
+
+static ssize_t efx_mcdi_mon_show_alarm(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	struct efx_mcdi_mon_attribute *mon_attr =
+		container_of(attr, struct efx_mcdi_mon_attribute, dev_attr);
+	efx_dword_t entry;
+	int state;
+	int rc;
+
+	rc = efx_mcdi_mon_get_entry(dev, mon_attr->index, &entry);
+	if (rc)
+		return rc;
+
+	state = EFX_DWORD_FIELD(entry, MC_CMD_SENSOR_VALUE_ENTRY_TYPEDEF_STATE);
+	return sprintf(buf, "%d\n", state != MC_CMD_SENSOR_STATE_OK);
+}
+
+static ssize_t efx_mcdi_mon_show_label(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	struct efx_mcdi_mon_attribute *mon_attr =
+		container_of(attr, struct efx_mcdi_mon_attribute, dev_attr);
+	return sprintf(buf, "%s\n",
+		       efx_mcdi_sensor_type[mon_attr->index].label);
+}
+
+static int
+efx_mcdi_mon_add_attr(struct efx_nic *efx, const char *name,
+		      ssize_t (*reader)(struct device *,
+					struct device_attribute *, char *),
+		      unsigned int index, unsigned int limit_value)
+{
+	struct efx_mcdi_mon *hwmon = efx_mcdi_mon(efx);
+	struct efx_mcdi_mon_attribute *attr = &hwmon->attrs[hwmon->n_attrs];
+	int rc;
+
+	strlcpy(attr->name, name, sizeof(attr->name));
+	attr->index = index;
+	attr->limit_value = limit_value;
+	attr->dev_attr.attr.name = attr->name;
+	attr->dev_attr.attr.mode = S_IRUGO;
+	attr->dev_attr.show = reader;
+	rc = device_create_file(&efx->pci_dev->dev, &attr->dev_attr);
+	if (rc == 0)
+		++hwmon->n_attrs;
+	return rc;
+}
+
+int efx_mcdi_mon_probe(struct efx_nic *efx)
+{
+	struct efx_mcdi_mon *hwmon = efx_mcdi_mon(efx);
+	unsigned int n_attrs, n_temp = 0, n_cool = 0, n_in = 0;
+	u8 outbuf[MC_CMD_SENSOR_INFO_OUT_LENMAX];
+	size_t outlen;
+	char name[12];
+	u32 mask;
+	int rc, i;
+
+	BUILD_BUG_ON(MC_CMD_SENSOR_INFO_IN_LEN != 0);
+
+	rc = efx_mcdi_rpc(efx, MC_CMD_SENSOR_INFO, NULL, 0,
+			  outbuf, sizeof(outbuf), &outlen);
+	if (rc)
+		return rc;
+	if (outlen < MC_CMD_SENSOR_INFO_OUT_LENMIN)
+		return -EIO;
+
+	/* Find out which sensors are present.  Don't create a device
+	 * if there are none.
+	 */
+	mask = MCDI_DWORD(outbuf, SENSOR_INFO_OUT_MASK);
+	if (mask == 0)
+		return 0;
+
+	rc = efx_nic_alloc_buffer(efx, &hwmon->dma_buf,
+				  4 * MC_CMD_SENSOR_ENTRY_MAXNUM);
+	if (rc)
+		return rc;
+
+	mutex_init(&hwmon->update_lock);
+	efx_mcdi_mon_update(efx);
+
+	/* Allocate space for the maximum possible number of
+	 * attributes for this set of sensors: name of the driver plus
+	 * value, min, max, crit, alarm and label for each sensor.
+	 */
+	n_attrs = 1 + 6 * hweight32(mask);
+	hwmon->attrs = kcalloc(n_attrs, sizeof(*hwmon->attrs), GFP_KERNEL);
+	if (!hwmon->attrs) {
+		rc = -ENOMEM;
+		goto fail;
+	}
+
+	hwmon->device = hwmon_device_register(&efx->pci_dev->dev);
+	if (IS_ERR(hwmon->device)) {
+		rc = PTR_ERR(hwmon->device);
+		goto fail;
+	}
+
+	rc = efx_mcdi_mon_add_attr(efx, "name", efx_mcdi_mon_show_name, 0, 0);
+	if (rc)
+		goto fail;
+
+	for (i = 0; MC_CMD_SENSOR_INFO_OUT_LEN(i + 1) <= outlen; i++) {
+		const char *hwmon_prefix;
+		unsigned hwmon_index;
+		u16 min1, max1, min2, max2;
+
+		if (!(mask & (1 << i)))
+			continue;
+
+		/* Skip sensors specific to a different port */
+		if (efx_mcdi_sensor_type[i].hwmon_type != EFX_HWMON_UNKNOWN &&
+		    efx_mcdi_sensor_type[i].port >= 0 &&
+		    efx_mcdi_sensor_type[i].port != efx_port_num(efx))
+			continue;
+
+		switch (efx_mcdi_sensor_type[i].hwmon_type) {
+		case EFX_HWMON_TEMP:
+			hwmon_prefix = "temp";
+			hwmon_index = ++n_temp; /* 1-based */
+			break;
+		case EFX_HWMON_COOL:
+			/* This is likely to be a heatsink, but there
+			 * is no convention for representing cooling
+			 * devices other than fans.
+			 */
+			hwmon_prefix = "fan";
+			hwmon_index = ++n_cool; /* 1-based */
+			break;
+		default:
+			hwmon_prefix = "in";
+			hwmon_index = n_in++; /* 0-based */
+			break;
+		}
+
+		min1 = MCDI_ARRAY_FIELD(outbuf, SENSOR_ENTRY,
+					SENSOR_INFO_ENTRY, i, MIN1);
+		max1 = MCDI_ARRAY_FIELD(outbuf, SENSOR_ENTRY,
+					SENSOR_INFO_ENTRY, i, MAX1);
+		min2 = MCDI_ARRAY_FIELD(outbuf, SENSOR_ENTRY,
+					SENSOR_INFO_ENTRY, i, MIN2);
+		max2 = MCDI_ARRAY_FIELD(outbuf, SENSOR_ENTRY,
+					SENSOR_INFO_ENTRY, i, MAX2);
+
+		if (min1 != max1) {
+			snprintf(name, sizeof(name), "%s%u_input",
+				 hwmon_prefix, hwmon_index);
+			rc = efx_mcdi_mon_add_attr(
+				efx, name, efx_mcdi_mon_show_value, i, 0);
+			if (rc)
+				goto fail;
+
+			snprintf(name, sizeof(name), "%s%u_min",
+				 hwmon_prefix, hwmon_index);
+			rc = efx_mcdi_mon_add_attr(
+				efx, name, efx_mcdi_mon_show_limit, i, min1);
+			if (rc)
+				goto fail;
+
+			snprintf(name, sizeof(name), "%s%u_max",
+				 hwmon_prefix, hwmon_index);
+			rc = efx_mcdi_mon_add_attr(
+				efx, name, efx_mcdi_mon_show_limit, i, max1);
+			if (rc)
+				goto fail;
+
+			if (min2 != max2) {
+				/* Assume max2 is critical value.
+				 * But we have no good way to expose min2.
+				 */
+				snprintf(name, sizeof(name), "%s%u_crit",
+					 hwmon_prefix, hwmon_index);
+				rc = efx_mcdi_mon_add_attr(
+					efx, name, efx_mcdi_mon_show_limit, i,
+					max2);
+				if (rc)
+					goto fail;
+			}
+		}
+
+		snprintf(name, sizeof(name), "%s%u_alarm",
+			 hwmon_prefix, hwmon_index);
+		rc = efx_mcdi_mon_add_attr(
+			efx, name, efx_mcdi_mon_show_alarm, i, 0);
+		if (rc)
+			goto fail;
+
+		if (efx_mcdi_sensor_type[i].label) {
+			snprintf(name, sizeof(name), "%s%u_label",
+				 hwmon_prefix, hwmon_index);
+			rc = efx_mcdi_mon_add_attr(
+				efx, name, efx_mcdi_mon_show_label, i, 0);
+			if (rc)
+				goto fail;
+		}
+	}
+
+	return 0;
+
+fail:
+	efx_mcdi_mon_remove(efx);
+	return rc;
+}
+
+void efx_mcdi_mon_remove(struct efx_nic *efx)
+{
+	struct siena_nic_data *nic_data = efx->nic_data;
+	struct efx_mcdi_mon *hwmon = &nic_data->hwmon;
+	unsigned int i;
+
+	for (i = 0; i < hwmon->n_attrs; i++)
+		device_remove_file(&efx->pci_dev->dev,
+				   &hwmon->attrs[i].dev_attr);
+	kfree(hwmon->attrs);
+	if (hwmon->device)
+		hwmon_device_unregister(hwmon->device);
+	efx_nic_free_buffer(efx, &hwmon->dma_buf);
+}
diff --git a/drivers/net/ethernet/sfc/nic.h b/drivers/net/ethernet/sfc/nic.h
index a3ccd0b..905a187 100644
--- a/drivers/net/ethernet/sfc/nic.h
+++ b/drivers/net/ethernet/sfc/nic.h
@@ -144,12 +144,26 @@ static inline struct falcon_board *falcon_board(struct efx_nic *efx)
  * struct siena_nic_data - Siena NIC state
  * @mcdi: Management-Controller-to-Driver Interface
  * @wol_filter_id: Wake-on-LAN packet filter id
+ * @hwmon: Hardware monitor state
  */
 struct siena_nic_data {
 	struct efx_mcdi_iface mcdi;
 	int wol_filter_id;
+#ifdef CONFIG_SFC_MCDI_MON
+	struct efx_mcdi_mon hwmon;
+#endif
 };
 
+#ifdef CONFIG_SFC_MCDI_MON
+static inline struct efx_mcdi_mon *efx_mcdi_mon(struct efx_nic *efx)
+{
+	struct siena_nic_data *nic_data;
+	EFX_BUG_ON_PARANOID(efx_nic_rev(efx) < EFX_REV_SIENA_A0);
+	nic_data = efx->nic_data;
+	return &nic_data->hwmon;
+}
+#endif
+
 extern const struct efx_nic_type falcon_a1_nic_type;
 extern const struct efx_nic_type falcon_b0_nic_type;
 extern const struct efx_nic_type siena_a0_nic_type;
diff --git a/drivers/net/ethernet/sfc/siena.c b/drivers/net/ethernet/sfc/siena.c
index f054258..d3c4169 100644
--- a/drivers/net/ethernet/sfc/siena.c
+++ b/drivers/net/ethernet/sfc/siena.c
@@ -300,6 +300,10 @@ static int siena_probe_nic(struct efx_nic *efx)
 		goto fail5;
 	}
 
+	rc = efx_mcdi_mon_probe(efx);
+	if (rc)
+		goto fail5;
+
 	return 0;
 
 fail5:
@@ -387,6 +391,8 @@ static int siena_init_nic(struct efx_nic *efx)
 
 static void siena_remove_nic(struct efx_nic *efx)
 {
+	efx_mcdi_mon_remove(efx);
+
 	efx_nic_free_buffer(efx, &efx->irq_status);
 
 	siena_reset_hw(efx, RESET_TYPE_ALL);
-- 
1.7.7.5

Re: [net-next 8/9] ixgbe: add interface to export thermal data

[Stephen Hemminger]

On Fri, 23 Dec 2011 18:58:48 +0100
Michał Mirosław <mirqus@gmail.com> wrote:

> 2011/12/23 Jeff Kirsher <jeffrey.t.kirsher@intel.com>:
> > From: Don Skidmore <donald.c.skidmore@intel.com>
> >
> > Some of our adapters have thermal data available, this patch exports this
> > data via a read-only sysfs interface.
> 
> Just curious: can't this use the hwmon subsystem to be consistent with
> other system monitoring devices?
> 
> Best Regards,
> Michał Mirosław

There is also libsensors support in net-snmp, so if you use standard API's
the values can be monitored with standard management tools.

[net-next 9/9] ixgbe: add support for new 82599 device.

[Jeff Kirsher]

From: Don Skidmore <donald.c.skidmore@intel.com>

This device uses an already existing DevID but since it supports
WoL we need to add the Sub DevID.  It's support of WoL is limited
to the first port.

Signed-off-by: Don Skidmore <donald.c.skidmore@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_ethtool.c |   15 ++++++++++++---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    |   11 +++++++++--
 drivers/net/ethernet/intel/ixgbe/ixgbe_type.h    |    1 +
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index 91f871b..da7e580 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -1955,12 +1955,21 @@ static int ixgbe_wol_exclusion(struct ixgbe_adapter *adapter,
 	/* WOL not supported except for the following */
 	switch(hw->device_id) {
 	case IXGBE_DEV_ID_82599_SFP:
-		/* Only this subdevice supports WOL */
-		if (hw->subsystem_device_id != IXGBE_SUBDEV_ID_82599_SFP) {
+		/* Only these subdevices could supports WOL */
+		switch (hw->subsystem_device_id) {
+		case IXGBE_SUBDEV_ID_82599_560FLR:
+			/* only support first port */
+			if (hw->bus.func != 0) {
+				wol->supported = 0;
+				break;
+			}
+		case IXGBE_SUBDEV_ID_82599_SFP:
+			retval = 0;
+			break;
+		default:
 			wol->supported = 0;
 			break;
 		}
-		retval = 0;
 		break;
 	case IXGBE_DEV_ID_82599_COMBO_BACKPLANE:
 		/* All except this subdevice support WOL */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index b5cef7e..492396a 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7605,9 +7605,16 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
 	adapter->wol = 0;
 	switch (pdev->device) {
 	case IXGBE_DEV_ID_82599_SFP:
-		/* Only this subdevice supports WOL */
-		if (pdev->subsystem_device == IXGBE_SUBDEV_ID_82599_SFP)
+		/* Only these subdevice supports WOL */
+		switch (pdev->subsystem_device) {
+		case IXGBE_SUBDEV_ID_82599_560FLR:
+			/* only support first port */
+			if (hw->bus.func != 0)
+				break;
+		case IXGBE_SUBDEV_ID_82599_SFP:
 			adapter->wol = IXGBE_WUFC_MAG;
+			break;
+		}
 		break;
 	case IXGBE_DEV_ID_82599_COMBO_BACKPLANE:
 		/* All except this subdevice support WOL */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
index c5fba00..03b1ecf 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
@@ -57,6 +57,7 @@
 #define IXGBE_DEV_ID_82599_BACKPLANE_FCOE       0x152a
 #define IXGBE_DEV_ID_82599_SFP_FCOE      0x1529
 #define IXGBE_SUBDEV_ID_82599_SFP        0x11A9
+#define IXGBE_SUBDEV_ID_82599_560FLR     0x17D0
 #define IXGBE_DEV_ID_82599_SFP_EM        0x1507
 #define IXGBE_DEV_ID_82599_SFP_SF2       0x154D
 #define IXGBE_DEV_ID_82599EN_SFP         0x1557
-- 
1.7.7.4