[PATCH net-next 3/4] mlx4: Do not mask failure accessing page A2h (0x51)

[PATCH net-next 3/4] mlx4: Do not mask failure accessing page A2h (0x51)

[Krzysztof Olędzki]

Due to HW/FW limitation, page A2h (I2C 0x51) may not be available.
Do not mask the problem so the userspace can properly handle it.

When returning the error to the userspace, use -EIO instead of
"err" because it holds MAD_STATUS.

Fixes: f5826c8c9d57 ("net/mlx4_en: Fix wrong return value on ioctl EEPROM query failure")
Fixes: 32a173c7f9e9 ("net/mlx4_core: Introduce mlx4_get_module_info for cable module info reading")
Signed-off-by: Krzysztof Piotr Oledzki <ole@ans.pl>
---
 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 2 +-
 drivers/net/ethernet/mellanox/mlx4/port.c       | 9 +--------
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
index 4c985d62af12..677917168bd5 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
@@ -2094,7 +2094,7 @@ static int mlx4_en_get_module_eeprom(struct net_device *dev,
 			en_err(priv,
 			       "mlx4_get_module_info i(%d) offset(%d) bytes_to_read(%d) - FAILED (0x%x)\n",
 			       i, offset, ee->len - i, ret);
-			return ret;
+			return -EIO;
 		}
 
 		i += ret;
diff --git a/drivers/net/ethernet/mellanox/mlx4/port.c b/drivers/net/ethernet/mellanox/mlx4/port.c
index 1ebd459d1d21..8c2a384404f9 100644
--- a/drivers/net/ethernet/mellanox/mlx4/port.c
+++ b/drivers/net/ethernet/mellanox/mlx4/port.c
@@ -2198,14 +2198,7 @@ int mlx4_get_module_info(struct mlx4_dev *dev, u8 port,
 			  MLX4_ATTR_CABLE_INFO, port, i2c_addr, offset, size,
 			  ret, cable_info_mad_err_str(ret));
 
-		if (i2c_addr == I2C_ADDR_HIGH &&
-		    MAD_STATUS_2_CABLE_ERR(ret) == CABLE_INF_I2C_ADDR)
-			/* Some SFP cables do not support i2c slave
-			 * address 0x51 (high page), abort silently.
-			 */
-			ret = 0;
-		else
-			ret = -ret;
+		ret = -ret;
 		goto out;
 	}
 	cable_info = (struct mlx4_cable_info *)outmad->data;
-- 
2.46.0

Re: [PATCH net-next 3/4] mlx4: Do not mask failure accessing page A2h (0x51)

[Przemek Kitszel]

On 9/12/24 08:41, Krzysztof Olędzki wrote:
> Due to HW/FW limitation, page A2h (I2C 0x51) may not be available.
> Do not mask the problem so the userspace can properly handle it.
> 
> When returning the error to the userspace, use -EIO instead of
> "err" because it holds MAD_STATUS.
> 
> Fixes: f5826c8c9d57 ("net/mlx4_en: Fix wrong return value on ioctl EEPROM query failure")
> Fixes: 32a173c7f9e9 ("net/mlx4_core: Introduce mlx4_get_module_info for cable module info reading")
> Signed-off-by: Krzysztof Piotr Oledzki <ole@ans.pl>
> ---
>   drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 2 +-
>   drivers/net/ethernet/mellanox/mlx4/port.c       | 9 +--------
>   2 files changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> index 4c985d62af12..677917168bd5 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> @@ -2094,7 +2094,7 @@ static int mlx4_en_get_module_eeprom(struct net_device *dev,
>   			en_err(priv,
>   			       "mlx4_get_module_info i(%d) offset(%d) bytes_to_read(%d) - FAILED (0x%x)\n",
>   			       i, offset, ee->len - i, ret);
> -			return ret;
> +			return -EIO;

here you are masking also all other explicit error paths of
mlx4_get_module_info(), what is not good in general, I would instead
mask below (see next comment)

>   		}
>   
>   		i += ret;
> diff --git a/drivers/net/ethernet/mellanox/mlx4/port.c b/drivers/net/ethernet/mellanox/mlx4/port.c
> index 1ebd459d1d21..8c2a384404f9 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/port.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/port.c
> @@ -2198,14 +2198,7 @@ int mlx4_get_module_info(struct mlx4_dev *dev, u8 port,
>   			  MLX4_ATTR_CABLE_INFO, port, i2c_addr, offset, size,
>   			  ret, cable_info_mad_err_str(ret));
>   
> -		if (i2c_addr == I2C_ADDR_HIGH &&
> -		    MAD_STATUS_2_CABLE_ERR(ret) == CABLE_INF_I2C_ADDR)
> -			/* Some SFP cables do not support i2c slave
> -			 * address 0x51 (high page), abort silently.
> -			 */
> -			ret = 0;
> -		else
> -			ret = -ret;
> +		ret = -ret;

this is the only place that mlx4_get_module_info() returns non standard
error code so, I believe, it's here where we want to overwrite with -EIO

then you could limit to just a single Fixes: tag (the second one)

Re: [PATCH net-next 3/4] mlx4: Do not mask failure accessing page A2h (0x51)

[Krzysztof Olędzki]

On 16.09.2024 at 02:44, Przemek Kitszel wrote:
> On 9/12/24 08:41, Krzysztof Olędzki wrote:
>> Due to HW/FW limitation, page A2h (I2C 0x51) may not be available.
>> Do not mask the problem so the userspace can properly handle it.
>>
>> When returning the error to the userspace, use -EIO instead of
>> "err" because it holds MAD_STATUS.
>>
>> Fixes: f5826c8c9d57 ("net/mlx4_en: Fix wrong return value on ioctl EEPROM query failure")
>> Fixes: 32a173c7f9e9 ("net/mlx4_core: Introduce mlx4_get_module_info for cable module info reading")
>> Signed-off-by: Krzysztof Piotr Oledzki <ole@ans.pl>
>> ---
>>   drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 2 +-
>>   drivers/net/ethernet/mellanox/mlx4/port.c       | 9 +--------
>>   2 files changed, 2 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
>> index 4c985d62af12..677917168bd5 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
>> @@ -2094,7 +2094,7 @@ static int mlx4_en_get_module_eeprom(struct net_device *dev,
>>   			en_err(priv,
>>   			       "mlx4_get_module_info i(%d) offset(%d) bytes_to_read(%d) - FAILED (0x%x)\n",
>>   			       i, offset, ee->len - i, ret);
>> -			return ret;
>> +			return -EIO;
> 
> here you are masking also all other explicit error paths of
> mlx4_get_module_info(), what is not good in general, I would instead
> mask below (see next comment)

I agree, for this reason mlx4_get_module_info() seems to be a much better choice.
Will update, thanks!

> 
>>   		}
>>   
>>   		i += ret;
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/port.c b/drivers/net/ethernet/mellanox/mlx4/port.c
>> index 1ebd459d1d21..8c2a384404f9 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/port.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/port.c
>> @@ -2198,14 +2198,7 @@ int mlx4_get_module_info(struct mlx4_dev *dev, u8 port,
>>   			  MLX4_ATTR_CABLE_INFO, port, i2c_addr, offset, size,
>>   			  ret, cable_info_mad_err_str(ret));
>>   
>> -		if (i2c_addr == I2C_ADDR_HIGH &&
>> -		    MAD_STATUS_2_CABLE_ERR(ret) == CABLE_INF_I2C_ADDR)
>> -			/* Some SFP cables do not support i2c slave
>> -			 * address 0x51 (high page), abort silently.
>> -			 */
>> -			ret = 0;
>> -		else
>> -			ret = -ret;
>> +		ret = -ret;
> 
> this is the only place that mlx4_get_module_info() returns non standard
> error code so, I believe, it's here where we want to overwrite with -EIO
> 
> then you could limit to just a single Fixes: tag (the second one)

Sure, I can handle it in mlx4_get_module_info(). I will also need to
cover the return from the call to mlx4_get_module_id() however.

Thanks,
 Krzysztof