[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <156f9ba7-7b85-46eb-9e70-606b0c4e0498@ans.pl>
Date: Mon, 16 Sep 2024 09:37:33 -0700
From: Krzysztof Olędzki <ole@....pl>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>,
Tariq Toukan <tariqt@...dia.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
linux-rdma@...r.kernel.org, Ido Schimmel <idosch@...dia.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Gal Pressman <gal@...dia.com>,
Amir Vadai <amirv@...lanox.com>, Saeed Mahameed <saeedm@...lanox.com>
Subject: Re: [PATCH net-next 3/4] mlx4: Do not mask failure accessing page A2h
(0x51)
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@....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
Powered by blists - more mailing lists