From: Gal Pressman <gal@nvidia.com>
To: "Krzysztof Olędzki" <ole@ans.pl>,
"Ido Schimmel" <idosch@nvidia.com>,
"Tariq Toukan" <tariqt@nvidia.com>,
"David S. Miller" <davem@davemloft.net>,
"Eric Dumazet" <edumazet@google.com>,
"Jakub Kicinski" <kuba@kernel.org>,
"Paolo Abeni" <pabeni@redhat.com>,
"Saeed Mahameed" <saeedm@nvidia.com>,
"Leon Romanovsky" <leon@kernel.org>,
"Yishai Hadas" <yishaih@nvidia.com>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
linux-rdma@vger.kernel.org
Subject: Re: [PATCH net-next 1/4] mlx4/mlx5: {mlx4,mlx5e}_en_get_module_info cleanup
Date: Mon, 16 Sep 2024 10:16:30 +0300 [thread overview]
Message-ID: <12092059-4212-44c5-9b13-dc7698933f76@nvidia.com> (raw)
In-Reply-To: <14a24f93-f8d6-4bc7-8b87-86489bcedb28@ans.pl>
Hi Krzysztof,
On 12/09/2024 9:38, Krzysztof Olędzki wrote:
> Use SFF8024 constants defined in linux/sfp.h instead of private ones.
>
> Make mlx4_en_get_module_info() and mlx5e_get_module_info() to look
> as close as possible to each other.
Please split mlx4 and mlx5 changes to two patches.
>
> Simplify the logic for selecting SFF_8436 vs SFF_8636.
>
> Signed-off-by: Krzysztof Piotr Oledzki <ole@ans.pl>
> ---
> .../net/ethernet/mellanox/mlx4/en_ethtool.c | 33 ++++++++++---------
> drivers/net/ethernet/mellanox/mlx4/port.c | 9 ++---
> .../ethernet/mellanox/mlx5/core/en_ethtool.c | 28 +++++++++-------
> .../net/ethernet/mellanox/mlx5/core/port.c | 9 ++---
> include/linux/mlx4/device.h | 7 ----
> include/linux/mlx5/port.h | 8 -----
> 6 files changed, 44 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> index cd17a3f4faf8..4c985d62af12 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> @@ -40,6 +40,7 @@
> #include <net/ip.h>
> #include <linux/bitmap.h>
> #include <linux/mii.h>
> +#include <linux/sfp.h>
>
> #include "mlx4_en.h"
> #include "en_port.h"
> @@ -2029,33 +2030,35 @@ static int mlx4_en_get_module_info(struct net_device *dev,
>
> /* Read first 2 bytes to get Module & REV ID */
> ret = mlx4_get_module_info(mdev->dev, priv->port,
> - 0/*offset*/, 2/*size*/, data);
> + 0 /*offset*/, 2 /*size*/, data);
Why?
> if (ret < 2)
> return -EIO;
>
> - switch (data[0] /* identifier */) {
> - case MLX4_MODULE_ID_QSFP:
> - modinfo->type = ETH_MODULE_SFF_8436;
> + /* data[0] = identifier byte */
> + switch (data[0]) {
> + case SFF8024_ID_QSFP_8438:
> + modinfo->type = ETH_MODULE_SFF_8436;
> modinfo->eeprom_len = ETH_MODULE_SFF_8436_MAX_LEN;
> break;
> - case MLX4_MODULE_ID_QSFP_PLUS:
> - if (data[1] >= 0x3) { /* revision id */
> - modinfo->type = ETH_MODULE_SFF_8636;
> - modinfo->eeprom_len = ETH_MODULE_SFF_8636_MAX_LEN;
> - } else {
> - modinfo->type = ETH_MODULE_SFF_8436;
> + case SFF8024_ID_QSFP_8436_8636:
> + /* data[1] = revision id */
> + if (data[1] < 0x3) {
> + modinfo->type = ETH_MODULE_SFF_8436;
> modinfo->eeprom_len = ETH_MODULE_SFF_8436_MAX_LEN;
> + break;
> }
> - break;
> - case MLX4_MODULE_ID_QSFP28:
> - modinfo->type = ETH_MODULE_SFF_8636;
> + fallthrough;
> + case SFF8024_ID_QSFP28_8636:
> + modinfo->type = ETH_MODULE_SFF_8636;
> modinfo->eeprom_len = ETH_MODULE_SFF_8636_MAX_LEN;
> break;
> - case MLX4_MODULE_ID_SFP:
> - modinfo->type = ETH_MODULE_SFF_8472;
> + case SFF8024_ID_SFP:
> + modinfo->type = ETH_MODULE_SFF_8472;
> modinfo->eeprom_len = ETH_MODULE_SFF_8472_LEN;
> break;
> default:
> + netdev_err(dev, "%s: cable type not recognized: 0x%x\n",
> + __func__, data[0]);
0x%x -> %#x.
> return -EINVAL;
> }
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/port.c b/drivers/net/ethernet/mellanox/mlx4/port.c
> index 4e43f4a7d246..6dbd505e7f30 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/port.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/port.c
> @@ -34,6 +34,7 @@
> #include <linux/if_ether.h>
> #include <linux/if_vlan.h>
> #include <linux/export.h>
> +#include <linux/sfp.h>
>
> #include <linux/mlx4/cmd.h>
>
> @@ -2139,12 +2140,12 @@ int mlx4_get_module_info(struct mlx4_dev *dev, u8 port,
> return ret;
>
> switch (module_id) {
> - case MLX4_MODULE_ID_SFP:
> + case SFF8024_ID_SFP:
> mlx4_sfp_eeprom_params_set(&i2c_addr, &page_num, &offset);
> break;
> - case MLX4_MODULE_ID_QSFP:
> - case MLX4_MODULE_ID_QSFP_PLUS:
> - case MLX4_MODULE_ID_QSFP28:
> + case SFF8024_ID_QSFP_8438:
> + case SFF8024_ID_QSFP_8436_8636:
> + case SFF8024_ID_QSFP28_8636:
> mlx4_qsfp_eeprom_params_set(&i2c_addr, &page_num, &offset);
> break;
> default:
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> index 4d123dae912c..12a22e5c60ae 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> @@ -32,6 +32,7 @@
>
> #include <linux/dim.h>
> #include <linux/ethtool_netlink.h>
> +#include <linux/sfp.h>
>
> #include "en.h"
> #include "en/channels.h"
> @@ -1899,36 +1900,39 @@ static int mlx5e_get_module_info(struct net_device *netdev,
> {
> struct mlx5e_priv *priv = netdev_priv(netdev);
> struct mlx5_core_dev *dev = priv->mdev;
> - int size_read = 0;
> + int ret;
Why did you rename this variable?
> u8 data[4] = {0};
>
> - size_read = mlx5_query_module_eeprom(dev, 0, 2, data);
> - if (size_read < 2)
> + /* Read first 2 bytes to get Module & REV ID */
> + ret = mlx5_query_module_eeprom(dev,
> + 0 /*offset*/, 2 /*size*/, data);
> + if (ret < 2)
This whole hunk is not needed.
> return -EIO;
>
> /* data[0] = identifier byte */
> switch (data[0]) {
> - case MLX5_MODULE_ID_QSFP:
> + case SFF8024_ID_QSFP_8438:
> modinfo->type = ETH_MODULE_SFF_8436;
> modinfo->eeprom_len = ETH_MODULE_SFF_8436_MAX_LEN;
> break;
> - case MLX5_MODULE_ID_QSFP_PLUS:
> - case MLX5_MODULE_ID_QSFP28:
> + case SFF8024_ID_QSFP_8436_8636:
> /* data[1] = revision id */
> - if (data[0] == MLX5_MODULE_ID_QSFP28 || data[1] >= 0x3) {
> - modinfo->type = ETH_MODULE_SFF_8636;
> - modinfo->eeprom_len = ETH_MODULE_SFF_8636_MAX_LEN;
> - } else {
> + if (data[1] < 0x3) {
> modinfo->type = ETH_MODULE_SFF_8436;
> modinfo->eeprom_len = ETH_MODULE_SFF_8436_MAX_LEN;
> + break;
> }
> + fallthrough;
> + case SFF8024_ID_QSFP28_8636:
> + modinfo->type = ETH_MODULE_SFF_8636;
> + modinfo->eeprom_len = ETH_MODULE_SFF_8636_MAX_LEN;
> break;
> - case MLX5_MODULE_ID_SFP:
> + case SFF8024_ID_SFP:
> modinfo->type = ETH_MODULE_SFF_8472;
> modinfo->eeprom_len = ETH_MODULE_SFF_8472_LEN;
> break;
> default:
> - netdev_err(priv->netdev, "%s: cable type not recognized:0x%x\n",
> + netdev_err(priv->netdev, "%s: cable type not recognized: 0x%x\n",
Unrelated to this patch, but OK.
next prev parent reply other threads:[~2024-09-16 7:16 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-09-12 6:38 [PATCH net-next 1/4] mlx4/mlx5: {mlx4,mlx5e}_en_get_module_info cleanup Krzysztof Olędzki
2024-09-13 20:55 ` Jakub Kicinski
2024-09-14 2:12 ` Krzysztof Olędzki
2024-09-14 2:48 ` Jakub Kicinski
2024-09-14 8:21 ` Simon Horman
2024-09-15 2:21 ` Krzysztof Olędzki
2024-09-16 9:38 ` Przemek Kitszel
2024-09-16 7:16 ` Gal Pressman [this message]
2024-09-16 7:30 ` Krzysztof Olędzki
2024-09-16 8:44 ` Gal Pressman
2024-09-17 4:19 ` Krzysztof Olędzki
2024-09-17 7:04 ` Gal Pressman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=12092059-4212-44c5-9b13-dc7698933f76@nvidia.com \
--to=gal@nvidia.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=idosch@nvidia.com \
--cc=kuba@kernel.org \
--cc=leon@kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=ole@ans.pl \
--cc=pabeni@redhat.com \
--cc=saeedm@nvidia.com \
--cc=tariqt@nvidia.com \
--cc=yishaih@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox