| 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: <1ad0402c-9c6a-44bc-9776-cb02c8e55a87@ans.pl>
Date: Mon, 16 Sep 2024 21:19:29 -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 01:44, Gal Pressman wrote:
> On 16/09/2024 10:30, Krzysztof Olędzki wrote:
>> 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.
>
> mlx4 and mlx5 don't necessarily have to be similar to each other.
Agreed, however it was not a random goal. My motivation was that since
the functions are doing exactly the same thing, it would be beneficial
for them to look the same, so if more changes are needed in the future,
it should be easier to make them.
Here is BTW the diff between them after all the changes:
-static int mlx4_en_get_module_info(struct net_device *dev,
- struct ethtool_modinfo *modinfo)
+static int mlx5e_get_module_info(struct net_device *netdev,
+ struct ethtool_modinfo *modinfo)
{
- struct mlx4_en_priv *priv = netdev_priv(dev);
- struct mlx4_en_dev *mdev = priv->mdev;
+ struct mlx5e_priv *priv = netdev_priv(netdev);
+ struct mlx5_core_dev *dev = priv->mdev;
int ret;
- u8 data[4];
+ u8 data[4] = {0};
/* Read first 2 bytes to get Module & REV ID */
- ret = mlx4_get_module_info(mdev->dev, priv->port,
- 0 /*offset*/, 2 /*size*/, data);
+ ret = mlx5_query_module_eeprom(dev,
+ 0 /*offset*/, 2 /*size*/, data);
if (ret < 2)
return -EIO;
@@ -2057,7 +1932,7 @@
modinfo->eeprom_len = ETH_MODULE_SFF_8472_LEN;
break;
default:
- netdev_err(dev, "%s: cable type not recognized: 0x%x\n",
+ netdev_err(priv->netdev, "%s: cable type not recognized: 0x%x\n",
__func__, data[0]);
return -EINVAL;
}
@@ -2065,113 +1940,715 @@
return 0;
}
>
>>>> Simplify the logic for selecting SFF_8436 vs SFF_8636.
>
> This commit message reflects my main issue with this patch, patches
> should be concise, this patch tries to achieve (at least) three
> different things in one.
Fair. So, what we really have here:
1. Use SFF8024 constants defined in linux/sfp.h instead of private ones.
2. Simplify the logic for selecting SFF_8436 vs SFF_8636
3. Improving coding style
4. Adding extra logging in mlx4_en_get_module_info(), which is also what mlx5e_get_module_info() does.
5. Make mlx4_en_get_module_info() and mlx5e_get_module_info() to look as close as possible to each other.
As requested, I'm splitting mlx4 vs mlx5 changes into separate patches
Could you please advise if it is OK for them to cover all of the above
*as long as I correctly capture this in the description*, or should
I split them even more?
Perhaps 1+2 (x2: mlx4+mlx5), 4, 3+5 (mlx4+mlx5) - so 5 total?
>>>>
>>>> Signed-off-by: Krzysztof Piotr Oledzki <ole@....pl>
>>>> @@ -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?
>
> I didn't see the commit message mention anything about changing coding
> style.
My bad. I assumed it was a minor thing that would be fine to be just added there and
does not require additional comments. Will address in v2.
>>
>>>
>>>> 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.
>
> Continuing my previous comment, I didn't see the commit message mention
> anything about adding new prints.
Right, will also fix in v2. My [apparently incorrect] assumption was that since
we have it in mlx5e_get_module_info(), simply saying "Make mlx4_en_get_module_info() and
mlx5e_get_module_info() to look as close as possible to each other." would be sufficient.
I agree I should have called it explicitly.
Thanks,
Krzysztof
Powered by blists - more mailing lists