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] [thread-next>] [day] [month] [year] [list]

Message-ID: <55ffb761-170b-4a1c-8565-7e6f531d423c@nvidia.com>
Date: Mon, 16 Sep 2024 11:44:25 +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 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@....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.

> 
>>
>>>  	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.