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.
>>> 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.
>>>
>>> Signed-off-by: Krzysztof Piotr Oledzki <ole@xxxxxx>
>>> @@ -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.
>
>>
>>> 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.
