| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <12092059-4212-44c5-9b13-dc7698933f76@nvidia.com>
Date: Mon, 16 Sep 2024 10:16:30 +0300
From: Gal Pressman <gal@...dia.com>
To: Krzysztof Olędzki <ole@....pl>,
Ido Schimmel <idosch@...dia.com>, Tariq Toukan <tariqt@...dia.com>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Saeed Mahameed <saeedm@...dia.com>, Leon Romanovsky <leon@...nel.org>,
Yishai Hadas <yishaih@...dia.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
linux-rdma@...r.kernel.org
Subject: Re: [PATCH net-next 1/4] mlx4/mlx5: {mlx4,mlx5e}_en_get_module_info
cleanup
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@....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.
Powered by blists - more mailing lists