[PATCH v2 08/16] mlx4_en: Reporting HW revision in ethtool -i

HW revision is derived from device ID and rev id.

Signed-off-by: Eugenia Emantayev <redacted>
Signed-off-by: Yevgeny Petrilin <redacted>
---
 drivers/infiniband/hw/mlx4/main.c |    1 -
 drivers/net/mlx4/en_ethtool.c     |   15 ++++++++++++++-
 drivers/net/mlx4/main.c           |    2 ++
 drivers/net/mlx4/mlx4_en.h        |    3 +++
 include/linux/mlx4/device.h       |    2 +-
 5 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index c7a6213..66e3eec 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c

@@ -721,7 +721,6 @@ static int init_node_data(struct mlx4_ib_dev *dev)
 	if (err)
 		goto out;
 
-	dev->dev->rev_id = be32_to_cpup((__be32 *) (out_mad->data + 32));
 	memcpy(&dev->ib_dev.node_guid, out_mad->data + 12, 8);
 
 out:

diff --git a/drivers/net/mlx4/en_ethtool.c b/drivers/net/mlx4/en_ethtool.c
index c1f351f..62ace6c 100644
--- a/drivers/net/mlx4/en_ethtool.c
+++ b/drivers/net/mlx4/en_ethtool.c

@@ -45,7 +45,20 @@ mlx4_en_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *drvinfo)
 	struct mlx4_en_priv *priv = netdev_priv(dev);
 	struct mlx4_en_dev *mdev = priv->mdev;
 
-	sprintf(drvinfo->driver, DRV_NAME " (%s)", mdev->dev->board_id);
+	switch (mdev->dev->rev_id) {
+	case 0xa0:
+		if (dev->dev_id >= MLX4_EN_CX3_LOW_ID && dev->dev_id <= MLX4_EN_CX3_HIGH_ID)
+			sprintf(drvinfo->driver, DRV_NAME " (%s_CX-3)", mdev->dev->board_id);
+		else
+			sprintf(drvinfo->driver, DRV_NAME " (%s_CX)", mdev->dev->board_id);
+		break;
+	case 0xb0:
+		sprintf(drvinfo->driver, DRV_NAME " (%s_CX-2)", mdev->dev->board_id);
+		break;
+	default:
+		sprintf(drvinfo->driver, DRV_NAME " (%s)", mdev->dev->board_id);
+		break;
+	}
 	strncpy(drvinfo->version, DRV_VERSION " (" DRV_RELDATE ")", 32);
 	sprintf(drvinfo->fw_version, "%d.%d.%d",
 		(u16) (mdev->dev->caps.fw_ver >> 32),

diff --git a/drivers/net/mlx4/main.c b/drivers/net/mlx4/main.c
index 42d4fb4..5bebb88 100644
--- a/drivers/net/mlx4/main.c
+++ b/drivers/net/mlx4/main.c

@@ -1139,6 +1139,8 @@ static int __mlx4_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	INIT_LIST_HEAD(&priv->pgdir_list);
 	mutex_init(&priv->pgdir_mutex);
 
+	pci_read_config_byte(pdev, PCI_REVISION_ID, &dev->rev_id);
+
 	/*
 	 * Now reset the HCA before we touch the PCI capabilities or
 	 * attempt a firmware command, since a boot ROM may have left

diff --git a/drivers/net/mlx4/mlx4_en.h b/drivers/net/mlx4/mlx4_en.h
index 07aea8d..ad4df66 100644
--- a/drivers/net/mlx4/mlx4_en.h
+++ b/drivers/net/mlx4/mlx4_en.h

@@ -216,6 +216,9 @@ struct mlx4_en_tx_desc {
 
 #define MLX4_EN_USE_SRQ		0x01000000
 
+#define MLX4_EN_CX3_LOW_ID	0x1000
+#define MLX4_EN_CX3_HIGH_ID	0x1005
+
 struct mlx4_en_rx_alloc {
 	struct page *page;
 	u16 offset;

diff --git a/include/linux/mlx4/device.h b/include/linux/mlx4/device.h
index 2460356..fe2a3a3 100644
--- a/include/linux/mlx4/device.h
+++ b/include/linux/mlx4/device.h

@@ -422,7 +422,7 @@ struct mlx4_dev {
 	unsigned long		flags;
 	struct mlx4_caps	caps;
 	struct radix_tree_root	qp_table_tree;
-	u32			rev_id;
+	u8			rev_id;
 	char			board_id[MLX4_BOARD_ID_LEN];
 };
 

-- 
1.6.0.2

On Wed, 2011-03-23 at 10:38 +0200, Yevgeny Petrilin wrote:

HW revision is derived from device ID and rev id.

[...]

-	sprintf(drvinfo->driver, DRV_NAME " (%s)", mdev->dev->board_id);
+	switch (mdev->dev->rev_id) {
+	case 0xa0:
+		if (dev->dev_id >= MLX4_EN_CX3_LOW_ID && dev->dev_id <= MLX4_EN_CX3_HIGH_ID)
+			sprintf(drvinfo->driver, DRV_NAME " (%s_CX-3)", mdev->dev->board_id);
+		else
+			sprintf(drvinfo->driver, DRV_NAME " (%s_CX)", mdev->dev->board_id);
+		break;
+	case 0xb0:
+		sprintf(drvinfo->driver, DRV_NAME " (%s_CX-2)", mdev->dev->board_id);
+		break;
+	default:
+		sprintf(drvinfo->driver, DRV_NAME " (%s)", mdev->dev->board_id);
+		break;

[...]

This is an abuse of the ethtool_drvinfo::driver field.

Your users can use lspci -v, can't they?

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

This is an abuse of the ethtool_drvinfo::driver field.

Your users can use lspci -v, can't they?

I don't think there is a problem here.
We have always reported the HW model via Ethtool, we just expanded the information
we provide.
Our users prefer to see the information in ethtool.

Thanks,
Yevgeny

On Wed, 2011-03-23 at 15:10 +0000, Yevgeny Petrilin wrote:

quoted

This is an abuse of the ethtool_drvinfo::driver field.

Your users can use lspci -v, can't they?

I don't think there is a problem here.
We have always reported the HW model via Ethtool, we just expanded the information
we provide.
Our users prefer to see the information in ethtool.

Do you mean 'we documented ethtool -i as the way to get hardware
identification'?  That would be a bug in your documentation.

Ben.

-- 
Ben Hutchings, Senior Software 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.

On Wed, 2011-03-23 at 15:10 +0000, Yevgeny Petrilin wrote:

quoted

quoted

This is an abuse of the ethtool_drvinfo::driver field.

Your users can use lspci -v, can't they?

I don't think there is a problem here.
We have always reported the HW model via Ethtool, we just expanded

the information

quoted

we provide.
Our users prefer to see the information in ethtool.

Do you mean 'we documented ethtool -i as the way to get hardware
identification'?  That would be a bug in your documentation.

Ben.

This is not what I mean, All the required information can be found in lspci,
There are some requests to see part of this information also via ethtool

Yevgeny

On Wed, 23 Mar 2011 15:46:45 +0000
Ben Hutchings [off-list ref] wrote:

On Wed, 2011-03-23 at 15:10 +0000, Yevgeny Petrilin wrote:

quoted

quoted

This is an abuse of the ethtool_drvinfo::driver field.

Your users can use lspci -v, can't they?

I don't think there is a problem here.
We have always reported the HW model via Ethtool, we just expanded the information
we provide.
Our users prefer to see the information in ethtool.

Do you mean 'we documented ethtool -i as the way to get hardware
identification'?  That would be a bug in your documentation.

We need consistency among drivers in these fields.

On Wed, 2011-03-23 at 15:54 +0000, Yevgeny Petrilin wrote:

quoted

On Wed, 2011-03-23 at 15:10 +0000, Yevgeny Petrilin wrote:

quoted

quoted

This is an abuse of the ethtool_drvinfo::driver field.

Your users can use lspci -v, can't they?

I don't think there is a problem here.
We have always reported the HW model via Ethtool, we just expanded

the information

quoted

we provide.
Our users prefer to see the information in ethtool.

Do you mean 'we documented ethtool -i as the way to get hardware
identification'?  That would be a bug in your documentation.

Ben.

This is not what I mean, All the required information can be found in lspci,
There are some requests to see part of this information also via ethtool

As Stephen says, the issue here is consistency between drivers.
Sometimes you just have to say no to customer requests that you abuse a
standard API.

You could perhaps include some sort of hardware type distinction in the
firmware version string, if it doesn't already incorporate that.

Ben.

-- 
Ben Hutchings, Senior Software 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.

On Wed, 23 Mar 2011 16:08:12 +0000
Ben Hutchings [off-list ref] wrote:

On Wed, 2011-03-23 at 15:54 +0000, Yevgeny Petrilin wrote:

quoted

quoted

On Wed, 2011-03-23 at 15:10 +0000, Yevgeny Petrilin wrote:

quoted

quoted

This is an abuse of the ethtool_drvinfo::driver field.

Your users can use lspci -v, can't they?

I don't think there is a problem here.
We have always reported the HW model via Ethtool, we just expanded

the information

quoted

we provide.
Our users prefer to see the information in ethtool.

Do you mean 'we documented ethtool -i as the way to get hardware
identification'?  That would be a bug in your documentation.

Ben.

This is not what I mean, All the required information can be found in lspci,
There are some requests to see part of this information also via ethtool

As Stephen says, the issue here is consistency between drivers.
Sometimes you just have to say no to customer requests that you abuse a
standard API.

You could perhaps include some sort of hardware type distinction in the
firmware version string, if it doesn't already incorporate that.

The pci info is already in bus_info and that can be used by tools.
Alternatively, many drivers splat revision/config info out to dmesg.

From: Ben Hutchings <redacted>
Date: Wed, 23 Mar 2011 14:04:14 +0000

On Wed, 2011-03-23 at 10:38 +0200, Yevgeny Petrilin wrote:

quoted

HW revision is derived from device ID and rev id.

[...]

quoted

-	sprintf(drvinfo->driver, DRV_NAME " (%s)", mdev->dev->board_id);
+	switch (mdev->dev->rev_id) {
+	case 0xa0:
+		if (dev->dev_id >= MLX4_EN_CX3_LOW_ID && dev->dev_id <= MLX4_EN_CX3_HIGH_ID)
+			sprintf(drvinfo->driver, DRV_NAME " (%s_CX-3)", mdev->dev->board_id);
+		else
+			sprintf(drvinfo->driver, DRV_NAME " (%s_CX)", mdev->dev->board_id);
+		break;
+	case 0xb0:
+		sprintf(drvinfo->driver, DRV_NAME " (%s_CX-2)", mdev->dev->board_id);
+		break;
+	default:
+		sprintf(drvinfo->driver, DRV_NAME " (%s)", mdev->dev->board_id);
+		break;

[...]

This is an abuse of the ethtool_drvinfo::driver field.

Your users can use lspci -v, can't they?

Agreed, mlx4 folks please send me a follow-up patch that removes this
conditional string.

The driver string is only meant to identify the software, not the
hardware variant.

From: Yevgeny Petrilin <redacted>
Date: Wed, 23 Mar 2011 15:10:34 +0000

quoted

This is an abuse of the ethtool_drvinfo::driver field.

Your users can use lspci -v, can't they?

I don't think there is a problem here.
We have always reported the HW model via Ethtool, we just expanded the information
we provide.
Our users prefer to see the information in ethtool.

This doesn't matter, we strive for consistency across drivers rather
than have special cases like this.

Please remove the chip variant information from the ethtool driver
string, it is absolutely not appropriate.

On Wed, 2011-03-23 at 15:10 +0000, Yevgeny Petrilin wrote:

quoted

This is an abuse of the ethtool_drvinfo::driver field.

Your users can use lspci -v, can't they?

I don't think there is a problem here.
We have always reported the HW model via Ethtool, we just expanded the information
we provide.
Our users prefer to see the information in ethtool.

I can provide a patch for David if you won't.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

I will send the patch removing the HW information.

Yevgeny

-----Original Message-----
From: Ben Hutchings [mailto:bhutchings@solarflare.com]
Sent: Thursday, March 24, 2011 5:40 AM
To: Yevgeny Petrilin
Cc: davem@davemloft.net; netdev@vger.kernel.org; Eugenia Emantayev
Subject: RE: [PATCH v2 08/16] mlx4_en: Reporting HW revision in ethtool
-i

On Wed, 2011-03-23 at 15:10 +0000, Yevgeny Petrilin wrote:

quoted

quoted

This is an abuse of the ethtool_drvinfo::driver field.

Your users can use lspci -v, can't they?

I don't think there is a problem here.
We have always reported the HW model via Ethtool, we just expanded

the information

quoted

we provide.
Our users prefer to see the information in ethtool.

I can provide a patch for David if you won't.

Ben.

--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.