[ethtool] ethtool: check mac type from ethtool_regs.version

[ethtool] ethtool: check mac type from ethtool_regs.version

[Jeff Kirsher]

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

This patch cleans up the mac type checks by using ethtool_regs.version
provided by the driver. This change eliminates the need to add device IDs
every time they are added to the driver.

This patch depends on a driver change that will add the mac_type to the
upper 8 bits of ethtool_regs.version.

Note that when using ethtool with this patch with a version of ixgbe that
does not provide the mac_type in ethtool_regs.version the register dump
may be incomplete. However this issue would've existed previously for
device IDs that were not added to ethtool.

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>
---
 ixgbe.c | 88 ++---------------------------------------------------------------
 1 file changed, 2 insertions(+), 86 deletions(-)

diff --git a/ixgbe.c b/ixgbe.c
index dae11d4..ef87d53 100644
--- a/ixgbe.c
+++ b/ixgbe.c
@@ -31,37 +31,6 @@
 #define IXGBE_MFLCN_RPFCE          0x00000004 /* Receive Priority FC Enable */
 #define IXGBE_MFLCN_RFCE           0x00000008 /* Receive FC Enable */
 
-/* Device IDs */
-#define IXGBE_DEV_ID_82598               0x10B6
-#define IXGBE_DEV_ID_82598_BX            0x1508
-#define IXGBE_DEV_ID_82598AF_DUAL_PORT   0x10C6
-#define IXGBE_DEV_ID_82598AF_SINGLE_PORT 0x10C7
-#define IXGBE_DEV_ID_82598EB_SFP_LOM     0x10DB
-#define IXGBE_DEV_ID_82598AT             0x10C8
-#define IXGBE_DEV_ID_82598AT2            0x150B
-#define IXGBE_DEV_ID_82598EB_CX4         0x10DD
-#define IXGBE_DEV_ID_82598_CX4_DUAL_PORT 0x10EC
-#define IXGBE_DEV_ID_82598_DA_DUAL_PORT  0x10F1
-#define IXGBE_DEV_ID_82598_SR_DUAL_PORT_EM      0x10E1
-#define IXGBE_DEV_ID_82598EB_XF_LR       0x10F4
-#define IXGBE_DEV_ID_82599_KX4           0x10F7
-#define IXGBE_DEV_ID_82599_KX4_MEZZ      0x1514
-#define IXGBE_DEV_ID_82599_KR            0x1517
-#define IXGBE_DEV_ID_82599_T3_LOM        0x151C
-#define IXGBE_DEV_ID_82599_CX4           0x10F9
-#define IXGBE_DEV_ID_82599_SFP           0x10FB
-#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_DEV_ID_82599_SFP_EM        0x1507
-#define IXGBE_DEV_ID_82599_SFP_SF2       0x154D
-#define IXGBE_DEV_ID_82599EN_SFP         0x1557
-#define IXGBE_DEV_ID_82599_XAUI_LOM      0x10FC
-#define IXGBE_DEV_ID_82599_COMBO_BACKPLANE 0x10F8
-#define IXGBE_SUBDEV_ID_82599_KX4_KR_MEZZ  0x000C
-#define IXGBE_DEV_ID_82599_LS            0x154F
-#define IXGBE_DEV_ID_X540T               0x1528
-
 /*
  * Enumerated types specific to the ixgbe hardware
  * Media Access Controlers
@@ -74,70 +43,17 @@ enum ixgbe_mac_type {
 	ixgbe_num_macs
 };
 
-enum ixgbe_mac_type
-ixgbe_get_mac_type(u16 device_id)
-{
-	enum ixgbe_mac_type mac_type = ixgbe_mac_unknown;
-
-	switch (device_id) {
-	case IXGBE_DEV_ID_82598:
-	case IXGBE_DEV_ID_82598_BX:
-	case IXGBE_DEV_ID_82598AF_DUAL_PORT:
-	case IXGBE_DEV_ID_82598AF_SINGLE_PORT:
-	case IXGBE_DEV_ID_82598EB_SFP_LOM:
-	case IXGBE_DEV_ID_82598AT:
-	case IXGBE_DEV_ID_82598AT2:
-	case IXGBE_DEV_ID_82598EB_CX4:
-	case IXGBE_DEV_ID_82598_CX4_DUAL_PORT:
-	case IXGBE_DEV_ID_82598_DA_DUAL_PORT:
-	case IXGBE_DEV_ID_82598_SR_DUAL_PORT_EM:
-	case IXGBE_DEV_ID_82598EB_XF_LR:
-		mac_type = ixgbe_mac_82598EB;
-		break;
-	case IXGBE_DEV_ID_82599_KX4:
-	case IXGBE_DEV_ID_82599_KX4_MEZZ:
-	case IXGBE_DEV_ID_82599_KR:
-	case IXGBE_DEV_ID_82599_T3_LOM:
-	case IXGBE_DEV_ID_82599_CX4:
-	case IXGBE_DEV_ID_82599_SFP:
-	case IXGBE_DEV_ID_82599_BACKPLANE_FCOE:
-	case IXGBE_DEV_ID_82599_SFP_FCOE:
-	case IXGBE_SUBDEV_ID_82599_SFP:
-	case IXGBE_DEV_ID_82599_SFP_EM:
-	case IXGBE_DEV_ID_82599_SFP_SF2:
-	case IXGBE_DEV_ID_82599EN_SFP:
-	case IXGBE_DEV_ID_82599_XAUI_LOM:
-	case IXGBE_DEV_ID_82599_COMBO_BACKPLANE:
-	case IXGBE_SUBDEV_ID_82599_KX4_KR_MEZZ:
-	case IXGBE_DEV_ID_82599_LS:
-		mac_type = ixgbe_mac_82599EB;
-		break;
-	case IXGBE_DEV_ID_X540T:
-		mac_type = ixgbe_mac_X540;
-		break;
-	default:
-		mac_type = ixgbe_mac_82598EB;
-		break;
-	}
-
-	return mac_type;
-}
-
 int
 ixgbe_dump_regs(struct ethtool_drvinfo *info, struct ethtool_regs *regs)
 {
 	u32 *regs_buff = (u32 *)regs->data;
 	u32 reg;
-	u16 hw_device_id = (u16) regs->version;
-	u8 version = (u8)(regs->version >> 24);
+	u8 mac_type = (u8)(regs->version >> 24);
 	u8 i;
-	enum ixgbe_mac_type mac_type;
 
-	if (version != 1)
+	if (mac_type == ixgbe_mac_unknown)
 		return -1;
 
-	mac_type = ixgbe_get_mac_type(hw_device_id);
-
 	reg = regs_buff[1065];
 	fprintf(stdout,
 	"0x042A4: LINKS (Link Status register)                 0x%08X\n"
-- 
1.7.11.7

Re: [ethtool] ethtool: check mac type from ethtool_regs.version

[Ben Hutchings]

On Thu, 2013-04-25 at 22:12 -0700, Jeff Kirsher wrote:
> From: Emil Tantilov <emil.s.tantilov@intel.com>
> 
> This patch cleans up the mac type checks by using ethtool_regs.version
> provided by the driver. This change eliminates the need to add device IDs
> every time they are added to the driver.
> 
> This patch depends on a driver change that will add the mac_type to the
> upper 8 bits of ethtool_regs.version.
> 
> Note that when using ethtool with this patch with a version of ixgbe that
> does not provide the mac_type in ethtool_regs.version the register dump
> may be incomplete. However this issue would've existed previously for
> device IDs that were not added to ethtool.
[...]

I don't think this is acceptable; ethtool should remain backward
compatible if at all possible.

It seems like this would work with both old and new drivers:

diff --git a/ixgbe.c b/ixgbe.c
index dae11d4..9b005f2 100644
--- a/ixgbe.c
+++ b/ixgbe.c
@@ -133,10 +133,13 @@ ixgbe_dump_regs(struct ethtool_drvinfo *info, struct ethtool_regs *regs)
 	u8 i;
 	enum ixgbe_mac_type mac_type;
 
-	if (version != 1)
+	if (version == 0)
 		return -1;
 
-	mac_type = ixgbe_get_mac_type(hw_device_id);
+	/* The current driver reports the MAC type, but older versions
+	 * only report the device ID so we have to infer the MAC type.
+	 */
+	mac_type = version > 1 ? version : ixgbe_get_mac_type(hw_device_id);
 
 	reg = regs_buff[1065];
 	fprintf(stdout,

-- 
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: [ethtool] ethtool: check mac type from ethtool_regs.version

[Tantilov, Emil S]

>-----Original Message-----
>From: Ben Hutchings [mailto:bhutchings@solarflare.com]
>Sent: Tuesday, April 30, 2013 8:24 AM
>To: Kirsher, Jeffrey T; Tantilov, Emil S
>Cc: netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com
>Subject: Re: [ethtool] ethtool: check mac type from ethtool_regs.version
>
>On Thu, 2013-04-25 at 22:12 -0700, Jeff Kirsher wrote:
>> From: Emil Tantilov <emil.s.tantilov@intel.com>
>>
>> This patch cleans up the mac type checks by using ethtool_regs.version
>> provided by the driver. This change eliminates the need to add device IDs
>> every time they are added to the driver.
>>
>> This patch depends on a driver change that will add the mac_type to the
>> upper 8 bits of ethtool_regs.version.
>>
>> Note that when using ethtool with this patch with a version of ixgbe that
>> does not provide the mac_type in ethtool_regs.version the register dump
>> may be incomplete. However this issue would've existed previously for
>> device IDs that were not added to ethtool.
>[...]
>
>I don't think this is acceptable; ethtool should remain backward
>compatible if at all possible.
>
>It seems like this would work with both old and new drivers:
>
>diff --git a/ixgbe.c b/ixgbe.c
>index dae11d4..9b005f2 100644
>--- a/ixgbe.c
>+++ b/ixgbe.c
>@@ -133,10 +133,13 @@ ixgbe_dump_regs(struct ethtool_drvinfo *info, struct
>ethtool_regs *regs)
> 	u8 i;
> 	enum ixgbe_mac_type mac_type;
>
>-	if (version != 1)
>+	if (version == 0)
> 		return -1;
>
>-	mac_type = ixgbe_get_mac_type(hw_device_id);
>+	/* The current driver reports the MAC type, but older versions
>+	 * only report the device ID so we have to infer the MAC type.
>+	 */
>+	mac_type = version > 1 ? version : ixgbe_get_mac_type(hw_device_id);
>
> 	reg = regs_buff[1065];
> 	fprintf(stdout,

Yeah, this seems like a better way to go.

Thanks a lot,
Emil

>
>--
>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: [ethtool] ethtool: check mac type from ethtool_regs.version

[Ben Hutchings]

On Tue, 2013-04-30 at 18:12 +0000, Tantilov, Emil S wrote:
[...]
> >It seems like this would work with both old and new drivers:
> >
> >diff --git a/ixgbe.c b/ixgbe.c
> >index dae11d4..9b005f2 100644
> >--- a/ixgbe.c
> >+++ b/ixgbe.c
> >@@ -133,10 +133,13 @@ ixgbe_dump_regs(struct ethtool_drvinfo *info, struct
> >ethtool_regs *regs)
> > 	u8 i;
> > 	enum ixgbe_mac_type mac_type;
> >
> >-	if (version != 1)
> >+	if (version == 0)
> > 		return -1;
> >
> >-	mac_type = ixgbe_get_mac_type(hw_device_id);
> >+	/* The current driver reports the MAC type, but older versions
> >+	 * only report the device ID so we have to infer the MAC type.
> >+	 */
> >+	mac_type = version > 1 ? version : ixgbe_get_mac_type(hw_device_id);
> >
> > 	reg = regs_buff[1065];
> > 	fprintf(stdout,
> 
> Yeah, this seems like a better way to go.
> 
> Thanks a lot,
> Emil

OK, so I've applied this:

---
From: Ben Hutchings <bhutchings@solarflare.com>
Subject: ixgbe: check mac type from ethtool_regs.version

This patch cleans up the mac type checks by using ethtool_regs.version
provided by the driver. This change eliminates the need to add device IDs
every time they are added to the driver.

Note that when using ethtool with this patch with a version of ixgbe that
does not provide the mac_type in ethtool_regs.version the register dump
may be incomplete. However this issue would've existed previously for
device IDs that were not added to ethtool.

Original patch and description by Emil Tantilov <emil.s.tantilov@intel.com>.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 ixgbe.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/ixgbe.c b/ixgbe.c
index dae11d4..9b005f2 100644
--- a/ixgbe.c
+++ b/ixgbe.c
@@ -133,10 +133,13 @@ ixgbe_dump_regs(struct ethtool_drvinfo *info, struct ethtool_regs *regs)
 	u8 i;
 	enum ixgbe_mac_type mac_type;
 
-	if (version != 1)
+	if (version == 0)
 		return -1;
 
-	mac_type = ixgbe_get_mac_type(hw_device_id);
+	/* The current driver reports the MAC type, but older versions
+	 * only report the device ID so we have to infer the MAC type.
+	 */
+	mac_type = version > 1 ? version : ixgbe_get_mac_type(hw_device_id);
 
 	reg = regs_buff[1065];
 	fprintf(stdout,


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