On 17/09/2024 7:19, Krzysztof Olędzki wrote:
> 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;
> }
>
>
The amount of diff lines is not a very interesting metric, I would drop
the changes that try to make both drivers look the same.
>>
>>>>> 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.
I would do 1+2 for both drivers, 3+4 for both drivers, and drop 5.
Thanks
