| Message ID | 2aa0787e-a148-456e-b1b5-8f1e9785ed04@ans.pl (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [net-next,1/4] mlx4/mlx5: {mlx4,mlx5e}_en_get_module_info cleanup | expand |
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)
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
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;
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(-)