From: "Krzysztof Olędzki" <ole@ans.pl>
To: Gal Pressman <gal@nvidia.com>, 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 00:30:32 -0700 [thread overview]
Message-ID: <8a0c724e-d2fb-4ae6-bf5d-74bbd0a7581b@ans.pl> (raw)
In-Reply-To: <12092059-4212-44c5-9b13-dc7698933f76@nvidia.com>
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@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?
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
next prev parent reply other threads:[~2024-09-16 7:31 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
2024-09-16 7:30 ` Krzysztof Olędzki [this message]
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=8a0c724e-d2fb-4ae6-bf5d-74bbd0a7581b@ans.pl \
--to=ole@ans.pl \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=gal@nvidia.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=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