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

[Gal Pressman <gal@nvidia.com>]

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