Patchwork [net-next,6/6] ixgbe: change the eeprom version reported by ethtool

login
register
mail settings
Submitter Jeff Kirsher
Date Oct. 17, 2011, 12:21 p.m.
Message ID <1318854062-3628-7-git-send-email-jeffrey.t.kirsher@intel.com>
Download mbox | patch
Permalink /patch/120201/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Jeff Kirsher - Oct. 17, 2011, 12:21 p.m.
From: Emil Tantilov <emil.s.tantilov@intel.com>

Use 32bit value starting at offset 0x2d for displaying the firmware
version in ethtool. This should work for all current ixgbe HW

Signed-off-by: Emil Tantilov <emil.s.tantilov@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.h         |    3 ++-
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c |   13 +++++++------
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    |    7 ++++---
 3 files changed, 13 insertions(+), 10 deletions(-)
Joe Perches - Oct. 17, 2011, 3:57 p.m.
On Mon, 2011-10-17 at 05:21 -0700, Jeff Kirsher wrote:
> From: Emil Tantilov <emil.s.tantilov@intel.com>
> 
> Use 32bit value starting at offset 0x2d for displaying the firmware
> version in ethtool. This should work for all current ixgbe HW
[]
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
[]
> -	snprintf(firmware_version, sizeof(firmware_version), "%d.%d-%d",
> -	         (adapter->eeprom_version & 0xF000) >> 12,
> -	         (adapter->eeprom_version & 0x0FF0) >> 4,
> -	         adapter->eeprom_version & 0x000F);
> +	nvm_track_id = (adapter->eeprom_verh << 16) |
> +			adapter->eeprom_verl;
> +	snprintf(firmware_version, sizeof(firmware_version), "0x%08x",
> +		 nvm_track_id);

Is ethtool output like proc output considered an abi
that should not be changed?


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - Oct. 17, 2011, 5:16 p.m.
On Mon, 2011-10-17 at 08:57 -0700, Joe Perches wrote:
> On Mon, 2011-10-17 at 05:21 -0700, Jeff Kirsher wrote:
> > From: Emil Tantilov <emil.s.tantilov@intel.com>
> > 
> > Use 32bit value starting at offset 0x2d for displaying the firmware
> > version in ethtool. This should work for all current ixgbe HW
> []
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> []
> > -	snprintf(firmware_version, sizeof(firmware_version), "%d.%d-%d",
> > -	         (adapter->eeprom_version & 0xF000) >> 12,
> > -	         (adapter->eeprom_version & 0x0FF0) >> 4,
> > -	         adapter->eeprom_version & 0x000F);
> > +	nvm_track_id = (adapter->eeprom_verh << 16) |
> > +			adapter->eeprom_verl;
> > +	snprintf(firmware_version, sizeof(firmware_version), "0x%08x",
> > +		 nvm_track_id);
> 
> Is ethtool output like proc output considered an abi
> that should not be changed?

No-one should make any assumptions about the format of firmware_version
strings.  However they ought to be consistent with vendor documentation,
update programs, etc.

Ben.
Tantilov, Emil S - Oct. 17, 2011, 6:02 p.m.
>-----Original Message-----

>From: Ben Hutchings [mailto:bhutchings@solarflare.com]

>Sent: Monday, October 17, 2011 10:17 AM

>To: Joe Perches

>Cc: Kirsher, Jeffrey T; davem@davemloft.net; Tantilov, Emil S;

>netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com

>Subject: Re: [net-next 6/6] ixgbe: change the eeprom version reported by

>ethtool

>

>On Mon, 2011-10-17 at 08:57 -0700, Joe Perches wrote:

>> On Mon, 2011-10-17 at 05:21 -0700, Jeff Kirsher wrote:

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

>> >

>> > Use 32bit value starting at offset 0x2d for displaying the firmware

>> > version in ethtool. This should work for all current ixgbe HW

>> []

>> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c

>b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c

>> []

>> > -	snprintf(firmware_version, sizeof(firmware_version), "%d.%d-%d",

>> > -	         (adapter->eeprom_version & 0xF000) >> 12,

>> > -	         (adapter->eeprom_version & 0x0FF0) >> 4,

>> > -	         adapter->eeprom_version & 0x000F);

>> > +	nvm_track_id = (adapter->eeprom_verh << 16) |

>> > +			adapter->eeprom_verl;

>> > +	snprintf(firmware_version, sizeof(firmware_version), "0x%08x",

>> > +		 nvm_track_id);

>>

>> Is ethtool output like proc output considered an abi

>> that should not be changed?

>

>No-one should make any assumptions about the format of firmware_version

>strings.  However they ought to be consistent with vendor documentation,

>update programs, etc.


The old value was only marginally useful as it was possible to have different 
images with the same version. The 32 bit value shown by this patch is what 
is being used by the FW team to record the revisions of the images.

The words used to hold the 32 bit value are not described in the datasheet
for 82598/9, but will be in X540 once it becomes available and I have requested that this information be added to the 82598/9 docs as well.

Thanks,
Emil

>

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

Patch

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 9e6635e..4881807 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -518,7 +518,8 @@  struct ixgbe_adapter {
 	u64 rsc_total_count;
 	u64 rsc_total_flush;
 	u32 wol;
-	u16 eeprom_version;
+	u16 eeprom_verh;
+	u16 eeprom_verl;
 	u16 eeprom_cap;
 
 	int node;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index 7acfce3..70d58c3 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -889,21 +889,22 @@  static void ixgbe_get_drvinfo(struct net_device *netdev,
 {
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
 	char firmware_version[32];
+	u32 nvm_track_id;
 
 	strncpy(drvinfo->driver, ixgbe_driver_name,
 	        sizeof(drvinfo->driver) - 1);
 	strncpy(drvinfo->version, ixgbe_driver_version,
 	        sizeof(drvinfo->version) - 1);
 
-	snprintf(firmware_version, sizeof(firmware_version), "%d.%d-%d",
-	         (adapter->eeprom_version & 0xF000) >> 12,
-	         (adapter->eeprom_version & 0x0FF0) >> 4,
-	         adapter->eeprom_version & 0x000F);
+	nvm_track_id = (adapter->eeprom_verh << 16) |
+			adapter->eeprom_verl;
+	snprintf(firmware_version, sizeof(firmware_version), "0x%08x",
+		 nvm_track_id);
 
 	strncpy(drvinfo->fw_version, firmware_version,
-	        sizeof(drvinfo->fw_version));
+		sizeof(drvinfo->fw_version) - 1);
 	strncpy(drvinfo->bus_info, pci_name(adapter->pdev),
-	        sizeof(drvinfo->bus_info));
+		sizeof(drvinfo->bus_info) - 1);
 	drvinfo->n_stats = IXGBE_STATS_LEN;
 	drvinfo->testinfo_len = IXGBE_TEST_LEN;
 	drvinfo->regdump_len = ixgbe_get_regs_len(netdev);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index c92c3a7..31f4f53 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -8066,6 +8066,10 @@  static int __devinit ixgbe_probe(struct pci_dev *pdev,
 	}
 	device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
 
+	/* save off EEPROM version number */
+	hw->eeprom.ops.read(hw, 0x2e, &adapter->eeprom_verh);
+	hw->eeprom.ops.read(hw, 0x2d, &adapter->eeprom_verl);
+
 	/* pick up the PCI bus settings for reporting later */
 	hw->mac.ops.get_bus_info(hw);
 
@@ -8115,9 +8119,6 @@  static int __devinit ixgbe_probe(struct pci_dev *pdev,
 			   "is required.\n");
 	}
 
-	/* save off EEPROM version number */
-	hw->eeprom.ops.read(hw, 0x29, &adapter->eeprom_version);
-
 	/* reset the hardware with the new settings */
 	err = hw->mac.ops.start_hw(hw);