| 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: <8a0c724e-d2fb-4ae6-bf5d-74bbd0a7581b@ans.pl>
Date: Mon, 16 Sep 2024 00:30:32 -0700
From: Krzysztof Olędzki <ole@....pl>
To: Gal Pressman <gal@...dia.com>, 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
On 16.09.2024 at 00:16, Gal Pressman wrote:
> Hi Krzysztof,
Hi Gal,
Thank you so much for your prompt review!
> 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.
Sure, will do.
>> 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?
I tried to be consistent with the other places, some examples:
fw.c: err = mlx4_cmd(dev, mailbox->dma, 0x01 /* subn mgmt class */,
en_tx.c: 0, 0 /* Non-NAPI caller */);
Would you like me to drop this part of the change?
>
>> 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.
Ah, sure.
>> 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?
To be consistent with the mlx4 variant of this function and because it can be either
the size or the error code, so just "ret" looked better for me. Would you prefer
to keep it as size_read here but rename it in mlx4_en_get_module_info()?
>> 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.
You mean the rename? Again, I did this for the consistency between mlx4_en_get_module_info()
and mlx5e_en_get_module_info().
>> 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.
I assume you also want it to be "%#x"?
Thanks,
Krzysztof
Powered by blists - more mailing lists