| Message ID | 4D89B16F.4040008@mellanox.co.il |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
> > 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: > > > > 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.
> On Wed, 2011-03-23 at 15:10 +0000, Yevgeny Petrilin wrote: > > > > > > 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. 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 <bhutchings@solarflare.com> wrote: > On Wed, 2011-03-23 at 15:10 +0000, Yevgeny Petrilin wrote: > > > > > > 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. -- 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
On Wed, 2011-03-23 at 15:54 +0000, Yevgeny Petrilin wrote: > > On Wed, 2011-03-23 at 15:10 +0000, Yevgeny Petrilin wrote: > > > > > > > > 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. > > 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.
On Wed, 23 Mar 2011 16:08:12 +0000 Ben Hutchings <bhutchings@solarflare.com> wrote: > On Wed, 2011-03-23 at 15:54 +0000, Yevgeny Petrilin wrote: > > > On Wed, 2011-03-23 at 15:10 +0000, Yevgeny Petrilin wrote: > > > > > > > > > > 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. > > > > 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. -- 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
From: Ben Hutchings <bhutchings@solarflare.com> Date: Wed, 23 Mar 2011 14:04:14 +0000 > 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? 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. -- 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
From: Yevgeny Petrilin <yevgenyp@mellanox.co.il> Date: Wed, 23 Mar 2011 15:10:34 +0000 >> >> 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. -- 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
On Wed, 2011-03-23 at 15:10 +0000, Yevgeny Petrilin wrote: > > > > 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.
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: > > > > > > 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.
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]; };