Re: [PATCH net-next 1/4] mlx4/mlx5: {mlx4,mlx5e}_en_get_module_info cleanup

[Gal Pressman <gal@nvidia.com>]

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