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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <33b89cd3-6e13-4ce9-9e80-60353980bc36@nvidia.com>
Date: Tue, 17 Sep 2024 10:04:37 +0300
From: Gal Pressman <gal@...dia.com>
To: Krzysztof Olędzki <ole@....pl>,
 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 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