| Message ID | 14a24f93-f8d6-4bc7-8b87-86489bcedb28@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 Wed, 11 Sep 2024 23:38:45 -0700 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. > > Simplify the logic for selecting SFF_8436 vs SFF_8636. Minor process suggestion, I think you may be sending the patches one by one. It's best to format them into a new directory and send all at once with git send-email. Add a cover letter, too.
On 13.09.2024 at 13:55, Jakub Kicinski wrote: > On Wed, 11 Sep 2024 23:38:45 -0700 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. >> >> Simplify the logic for selecting SFF_8436 vs SFF_8636. > > Minor process suggestion, I think you may be sending the patches one by > one. It's best to format them into a new directory and send all at once > with git send-email. Add a cover letter, too. > Thanks, yes, will do for v2. I assume this needs to wait for about two weeks for net-next to re-open? Krzysztof
On Fri, 13 Sep 2024 19:12:01 -0700 Krzysztof Olędzki wrote: > On 13.09.2024 at 13:55, Jakub Kicinski wrote: > > On Wed, 11 Sep 2024 23:38:45 -0700 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. > >> > >> Simplify the logic for selecting SFF_8436 vs SFF_8636. > > > > Minor process suggestion, I think you may be sending the patches one by > > one. It's best to format them into a new directory and send all at once > > with git send-email. Add a cover letter, too. > > > > Thanks, yes, will do for v2. I assume this needs to wait for about > two weeks for net-next to re-open? The cleanups - yes, but if patch 3 works you should make it independent and send as a fix (and trees never close for fixes).
On Fri, Sep 13, 2024 at 07:48:08PM -0700, Jakub Kicinski wrote: > On Fri, 13 Sep 2024 19:12:01 -0700 Krzysztof Olędzki wrote: > > On 13.09.2024 at 13:55, Jakub Kicinski wrote: > > > On Wed, 11 Sep 2024 23:38:45 -0700 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. > > >> > > >> Simplify the logic for selecting SFF_8436 vs SFF_8636. > > > > > > Minor process suggestion, I think you may be sending the patches one by > > > one. It's best to format them into a new directory and send all at once > > > with git send-email. Add a cover letter, too. > > > > > > > Thanks, yes, will do for v2. I assume this needs to wait for about > > two weeks for net-next to re-open? > > The cleanups - yes, but if patch 3 works you should make it independent > and send as a fix (and trees never close for fixes). Hi Krzysztof, Just to expand on what Jakub wrote a little. In general fixes should have a Fixes tag and be targeted at the net tree. Subject: [PATCH net] ... Link: https://docs.kernel.org/process/maintainer-netdev.html
On 14.09.2024 at 01:21, Simon Horman wrote: > On Fri, Sep 13, 2024 at 07:48:08PM -0700, Jakub Kicinski wrote: >> On Fri, 13 Sep 2024 19:12:01 -0700 Krzysztof Olędzki wrote: >>> On 13.09.2024 at 13:55, Jakub Kicinski wrote: >>>> On Wed, 11 Sep 2024 23:38:45 -0700 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. >>>>> >>>>> Simplify the logic for selecting SFF_8436 vs SFF_8636. >>>> >>>> Minor process suggestion, I think you may be sending the patches one by >>>> one. It's best to format them into a new directory and send all at once >>>> with git send-email. Add a cover letter, too. >>>> >>> >>> Thanks, yes, will do for v2. I assume this needs to wait for about >>> two weeks for net-next to re-open? >> >> The cleanups - yes, but if patch 3 works you should make it independent >> and send as a fix (and trees never close for fixes). > > Hi Krzysztof, > > Just to expand on what Jakub wrote a little. In general fixes should have a > Fixes tag and be targeted at the net tree. > > Subject: [PATCH net] ... > > Link: https://docs.kernel.org/process/maintainer-netdev.html Yes, thank you Simon for the additional feedback. I initially targeted net-next following Ido's request: https://lore.kernel.org/netdev/Ztna8O1ZGUc4kvKJ@shredder.mtl.com/ If we all believe "net" is the right target, I'm more than happy to update it and re-send that single patch now. Should I mark it as "v2" even if no difference because of the tree change? Also, I did include Fixes in that patch: 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") See: https://lore.kernel.org/netdev/2aa0787e-a148-456e-b1b5-8f1e9785ed04@ans.pl/ Thanks, Krzysztof
Hi Krzysztof, 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. Please split mlx4 and mlx5 changes to two patches. > > Simplify the logic for selecting SFF_8436 vs SFF_8636. > > Signed-off-by: Krzysztof Piotr Oledzki <ole@ans.pl> > --- > .../net/ethernet/mellanox/mlx4/en_ethtool.c | 33 ++++++++++--------- > drivers/net/ethernet/mellanox/mlx4/port.c | 9 ++--- > .../ethernet/mellanox/mlx5/core/en_ethtool.c | 28 +++++++++------- > .../net/ethernet/mellanox/mlx5/core/port.c | 9 ++--- > include/linux/mlx4/device.h | 7 ---- > include/linux/mlx5/port.h | 8 ----- > 6 files changed, 44 insertions(+), 50 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c > index cd17a3f4faf8..4c985d62af12 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c > +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c > @@ -40,6 +40,7 @@ > #include <net/ip.h> > #include <linux/bitmap.h> > #include <linux/mii.h> > +#include <linux/sfp.h> > > #include "mlx4_en.h" > #include "en_port.h" > @@ -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? > 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. > return -EINVAL; > } > > diff --git a/drivers/net/ethernet/mellanox/mlx4/port.c b/drivers/net/ethernet/mellanox/mlx4/port.c > index 4e43f4a7d246..6dbd505e7f30 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/port.c > +++ b/drivers/net/ethernet/mellanox/mlx4/port.c > @@ -34,6 +34,7 @@ > #include <linux/if_ether.h> > #include <linux/if_vlan.h> > #include <linux/export.h> > +#include <linux/sfp.h> > > #include <linux/mlx4/cmd.h> > > @@ -2139,12 +2140,12 @@ int mlx4_get_module_info(struct mlx4_dev *dev, u8 port, > return ret; > > switch (module_id) { > - case MLX4_MODULE_ID_SFP: > + case SFF8024_ID_SFP: > mlx4_sfp_eeprom_params_set(&i2c_addr, &page_num, &offset); > break; > - case MLX4_MODULE_ID_QSFP: > - case MLX4_MODULE_ID_QSFP_PLUS: > - case MLX4_MODULE_ID_QSFP28: > + case SFF8024_ID_QSFP_8438: > + case SFF8024_ID_QSFP_8436_8636: > + case SFF8024_ID_QSFP28_8636: > mlx4_qsfp_eeprom_params_set(&i2c_addr, &page_num, &offset); > break; > default: > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c > index 4d123dae912c..12a22e5c60ae 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c > @@ -32,6 +32,7 @@ > > #include <linux/dim.h> > #include <linux/ethtool_netlink.h> > +#include <linux/sfp.h> > > #include "en.h" > #include "en/channels.h" > @@ -1899,36 +1900,39 @@ static int mlx5e_get_module_info(struct net_device *netdev, > { > struct mlx5e_priv *priv = netdev_priv(netdev); > struct mlx5_core_dev *dev = priv->mdev; > - int size_read = 0; > + int ret; Why did you rename this variable? > u8 data[4] = {0}; > > - size_read = mlx5_query_module_eeprom(dev, 0, 2, data); > - if (size_read < 2) > + /* Read first 2 bytes to get Module & REV ID */ > + ret = mlx5_query_module_eeprom(dev, > + 0 /*offset*/, 2 /*size*/, data); > + if (ret < 2) This whole hunk is not needed. > return -EIO; > > /* data[0] = identifier byte */ > switch (data[0]) { > - case MLX5_MODULE_ID_QSFP: > + case SFF8024_ID_QSFP_8438: > modinfo->type = ETH_MODULE_SFF_8436; > modinfo->eeprom_len = ETH_MODULE_SFF_8436_MAX_LEN; > break; > - case MLX5_MODULE_ID_QSFP_PLUS: > - case MLX5_MODULE_ID_QSFP28: > + case SFF8024_ID_QSFP_8436_8636: > /* data[1] = revision id */ > - if (data[0] == MLX5_MODULE_ID_QSFP28 || data[1] >= 0x3) { > - modinfo->type = ETH_MODULE_SFF_8636; > - modinfo->eeprom_len = ETH_MODULE_SFF_8636_MAX_LEN; > - } else { > + if (data[1] < 0x3) { > modinfo->type = ETH_MODULE_SFF_8436; > modinfo->eeprom_len = ETH_MODULE_SFF_8436_MAX_LEN; > + break; > } > + fallthrough; > + case SFF8024_ID_QSFP28_8636: > + modinfo->type = ETH_MODULE_SFF_8636; > + modinfo->eeprom_len = ETH_MODULE_SFF_8636_MAX_LEN; > break; > - case MLX5_MODULE_ID_SFP: > + case SFF8024_ID_SFP: > modinfo->type = ETH_MODULE_SFF_8472; > modinfo->eeprom_len = ETH_MODULE_SFF_8472_LEN; > break; > default: > - netdev_err(priv->netdev, "%s: cable type not recognized:0x%x\n", > + netdev_err(priv->netdev, "%s: cable type not recognized: 0x%x\n", Unrelated to this patch, but OK.
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. > > Please split mlx4 and mlx5 changes to two patches. Sure, will do. >> Simplify the logic for selecting SFF_8436 vs SFF_8636. >> >> Signed-off-by: Krzysztof Piotr Oledzki <ole@ans.pl> >> --- >> .../net/ethernet/mellanox/mlx4/en_ethtool.c | 33 ++++++++++--------- >> drivers/net/ethernet/mellanox/mlx4/port.c | 9 ++--- >> .../ethernet/mellanox/mlx5/core/en_ethtool.c | 28 +++++++++------- >> .../net/ethernet/mellanox/mlx5/core/port.c | 9 ++--- >> include/linux/mlx4/device.h | 7 ---- >> include/linux/mlx5/port.h | 8 ----- >> 6 files changed, 44 insertions(+), 50 deletions(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c >> index cd17a3f4faf8..4c985d62af12 100644 >> --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c >> +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c >> @@ -40,6 +40,7 @@ >> #include <net/ip.h> >> #include <linux/bitmap.h> >> #include <linux/mii.h> >> +#include <linux/sfp.h> >> >> #include "mlx4_en.h" >> #include "en_port.h" >> @@ -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? > >> 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. >> return -EINVAL; >> } >> >> diff --git a/drivers/net/ethernet/mellanox/mlx4/port.c b/drivers/net/ethernet/mellanox/mlx4/port.c >> index 4e43f4a7d246..6dbd505e7f30 100644 >> --- a/drivers/net/ethernet/mellanox/mlx4/port.c >> +++ b/drivers/net/ethernet/mellanox/mlx4/port.c >> @@ -34,6 +34,7 @@ >> #include <linux/if_ether.h> >> #include <linux/if_vlan.h> >> #include <linux/export.h> >> +#include <linux/sfp.h> >> >> #include <linux/mlx4/cmd.h> >> >> @@ -2139,12 +2140,12 @@ int mlx4_get_module_info(struct mlx4_dev *dev, u8 port, >> return ret; >> >> switch (module_id) { >> - case MLX4_MODULE_ID_SFP: >> + case SFF8024_ID_SFP: >> mlx4_sfp_eeprom_params_set(&i2c_addr, &page_num, &offset); >> break; >> - case MLX4_MODULE_ID_QSFP: >> - case MLX4_MODULE_ID_QSFP_PLUS: >> - case MLX4_MODULE_ID_QSFP28: >> + case SFF8024_ID_QSFP_8438: >> + case SFF8024_ID_QSFP_8436_8636: >> + case SFF8024_ID_QSFP28_8636: >> mlx4_qsfp_eeprom_params_set(&i2c_addr, &page_num, &offset); >> break; >> default: >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c >> index 4d123dae912c..12a22e5c60ae 100644 >> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c >> @@ -32,6 +32,7 @@ >> >> #include <linux/dim.h> >> #include <linux/ethtool_netlink.h> >> +#include <linux/sfp.h> >> >> #include "en.h" >> #include "en/channels.h" >> @@ -1899,36 +1900,39 @@ static int mlx5e_get_module_info(struct net_device *netdev, >> { >> struct mlx5e_priv *priv = netdev_priv(netdev); >> struct mlx5_core_dev *dev = priv->mdev; >> - int size_read = 0; >> + int ret; > > Why did you rename this variable? To be consistent with the mlx4 variant of this function and because it can be either the size or the error code, so just "ret" looked better for me. Would you prefer to keep it as size_read here but rename it in mlx4_en_get_module_info()? >> u8 data[4] = {0}; >> >> - size_read = mlx5_query_module_eeprom(dev, 0, 2, data); >> - if (size_read < 2) >> + /* Read first 2 bytes to get Module & REV ID */ >> + ret = mlx5_query_module_eeprom(dev, >> + 0 /*offset*/, 2 /*size*/, data); >> + if (ret < 2) > > This whole hunk is not needed. You mean the rename? Again, I did this for the consistency between mlx4_en_get_module_info() and mlx5e_en_get_module_info(). >> return -EIO; >> >> /* data[0] = identifier byte */ >> switch (data[0]) { >> - case MLX5_MODULE_ID_QSFP: >> + case SFF8024_ID_QSFP_8438: >> modinfo->type = ETH_MODULE_SFF_8436; >> modinfo->eeprom_len = ETH_MODULE_SFF_8436_MAX_LEN; >> break; >> - case MLX5_MODULE_ID_QSFP_PLUS: >> - case MLX5_MODULE_ID_QSFP28: >> + case SFF8024_ID_QSFP_8436_8636: >> /* data[1] = revision id */ >> - if (data[0] == MLX5_MODULE_ID_QSFP28 || data[1] >= 0x3) { >> - modinfo->type = ETH_MODULE_SFF_8636; >> - modinfo->eeprom_len = ETH_MODULE_SFF_8636_MAX_LEN; >> - } else { >> + if (data[1] < 0x3) { >> modinfo->type = ETH_MODULE_SFF_8436; >> modinfo->eeprom_len = ETH_MODULE_SFF_8436_MAX_LEN; >> + break; >> } >> + fallthrough; >> + case SFF8024_ID_QSFP28_8636: >> + modinfo->type = ETH_MODULE_SFF_8636; >> + modinfo->eeprom_len = ETH_MODULE_SFF_8636_MAX_LEN; >> break; >> - case MLX5_MODULE_ID_SFP: >> + case SFF8024_ID_SFP: >> modinfo->type = ETH_MODULE_SFF_8472; >> modinfo->eeprom_len = ETH_MODULE_SFF_8472_LEN; >> break; >> default: >> - netdev_err(priv->netdev, "%s: cable type not recognized:0x%x\n", >> + netdev_err(priv->netdev, "%s: cable type not recognized: 0x%x\n", > > Unrelated to this patch, but OK. I assume you also want it to be "%#x"? Thanks, Krzysztof
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.
On 9/15/24 04:21, Krzysztof Olędzki wrote: > On 14.09.2024 at 01:21, Simon Horman wrote: >> On Fri, Sep 13, 2024 at 07:48:08PM -0700, Jakub Kicinski wrote: >>> On Fri, 13 Sep 2024 19:12:01 -0700 Krzysztof Olędzki wrote: >>>> On 13.09.2024 at 13:55, Jakub Kicinski wrote: >>>>> On Wed, 11 Sep 2024 23:38:45 -0700 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. >>>>>> >>>>>> Simplify the logic for selecting SFF_8436 vs SFF_8636. >>>>> >>>>> Minor process suggestion, I think you may be sending the patches one by >>>>> one. It's best to format them into a new directory and send all at once >>>>> with git send-email. Add a cover letter, too. >>>>> >>>> >>>> Thanks, yes, will do for v2. I assume this needs to wait for about >>>> two weeks for net-next to re-open? >>> >>> The cleanups - yes, but if patch 3 works you should make it independent >>> and send as a fix (and trees never close for fixes). >> >> Hi Krzysztof, >> >> Just to expand on what Jakub wrote a little. In general fixes should have a >> Fixes tag and be targeted at the net tree. >> >> Subject: [PATCH net] ... >> >> Link: https://docs.kernel.org/process/maintainer-netdev.html > > Yes, thank you Simon for the additional feedback. > > I initially targeted net-next following Ido's request: > https://lore.kernel.org/netdev/Ztna8O1ZGUc4kvKJ@shredder.mtl.com/ > > If we all believe "net" is the right target, I'm more than happy to update it and re-send that single > patch now. Should I mark it as "v2" even if no difference because of the tree change? yes, additionally please provide a changelog, which will say "retarget to -net" and it's most welcomed if you also provide links to past discussions > > Also, I did include Fixes in that patch: > 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") those two fixes tags are correct picks for your two changes, but I think that it could be combined into one place, I will reply in the current "patch 3" thread (now you see why it's beneficial to send whole series together :)) > > See: https://lore.kernel.org/netdev/2aa0787e-a148-456e-b1b5-8f1e9785ed04@ans.pl/ > > Thanks, > Krzysztof >
On 16.09.2024 at 01:44, Gal Pressman wrote: > 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. Agreed, however it was not a random goal. My motivation was that since the functions are doing exactly the same thing, it would be beneficial for them to look the same, so if more changes are needed in the future, it should be easier to make them. Here is BTW the diff between them after all the changes: -static int mlx4_en_get_module_info(struct net_device *dev, - struct ethtool_modinfo *modinfo) +static int mlx5e_get_module_info(struct net_device *netdev, + struct ethtool_modinfo *modinfo) { - struct mlx4_en_priv *priv = netdev_priv(dev); - struct mlx4_en_dev *mdev = priv->mdev; + struct mlx5e_priv *priv = netdev_priv(netdev); + struct mlx5_core_dev *dev = priv->mdev; int ret; - u8 data[4]; + u8 data[4] = {0}; /* Read first 2 bytes to get Module & REV ID */ - ret = mlx4_get_module_info(mdev->dev, priv->port, - 0 /*offset*/, 2 /*size*/, data); + ret = mlx5_query_module_eeprom(dev, + 0 /*offset*/, 2 /*size*/, data); if (ret < 2) return -EIO; @@ -2057,7 +1932,7 @@ modinfo->eeprom_len = ETH_MODULE_SFF_8472_LEN; break; default: - netdev_err(dev, "%s: cable type not recognized: 0x%x\n", + netdev_err(priv->netdev, "%s: cable type not recognized: 0x%x\n", __func__, data[0]); return -EINVAL; } @@ -2065,113 +1940,715 @@ return 0; } > >>>> 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. Fair. So, what we really have here: 1. Use SFF8024 constants defined in linux/sfp.h instead of private ones. 2. Simplify the logic for selecting SFF_8436 vs SFF_8636 3. Improving coding style 4. Adding extra logging in mlx4_en_get_module_info(), which is also what mlx5e_get_module_info() does. 5. Make mlx4_en_get_module_info() and mlx5e_get_module_info() to look as close as possible to each other. As requested, I'm splitting mlx4 vs mlx5 changes into separate patches Could you please advise if it is OK for them to cover all of the above *as long as I correctly capture this in the description*, or should I split them even more? Perhaps 1+2 (x2: mlx4+mlx5), 4, 3+5 (mlx4+mlx5) - so 5 total? >>>> >>>> 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. My bad. I assumed it was a minor thing that would be fine to be just added there and does not require additional comments. Will address in v2. >> >>> >>>> 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. Right, will also fix in v2. My [apparently incorrect] assumption was that since we have it in mlx5e_get_module_info(), simply saying "Make mlx4_en_get_module_info() and mlx5e_get_module_info() to look as close as possible to each other." would be sufficient. I agree I should have called it explicitly. Thanks, Krzysztof
On 17/09/2024 7:19, Krzysztof Olędzki wrote: > On 16.09.2024 at 01:44, Gal Pressman wrote: >> 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. > > Agreed, however it was not a random goal. My motivation was that since > the functions are doing exactly the same thing, it would be beneficial > for them to look the same, so if more changes are needed in the future, > it should be easier to make them. > > Here is BTW the diff between them after all the changes: > > -static int mlx4_en_get_module_info(struct net_device *dev, > - struct ethtool_modinfo *modinfo) > +static int mlx5e_get_module_info(struct net_device *netdev, > + struct ethtool_modinfo *modinfo) > { > - struct mlx4_en_priv *priv = netdev_priv(dev); > - struct mlx4_en_dev *mdev = priv->mdev; > + struct mlx5e_priv *priv = netdev_priv(netdev); > + struct mlx5_core_dev *dev = priv->mdev; > int ret; > - u8 data[4]; > + u8 data[4] = {0}; > > /* Read first 2 bytes to get Module & REV ID */ > - ret = mlx4_get_module_info(mdev->dev, priv->port, > - 0 /*offset*/, 2 /*size*/, data); > + ret = mlx5_query_module_eeprom(dev, > + 0 /*offset*/, 2 /*size*/, data); > if (ret < 2) > return -EIO; > > @@ -2057,7 +1932,7 @@ > modinfo->eeprom_len = ETH_MODULE_SFF_8472_LEN; > break; > default: > - netdev_err(dev, "%s: cable type not recognized: 0x%x\n", > + netdev_err(priv->netdev, "%s: cable type not recognized: 0x%x\n", > __func__, data[0]); > return -EINVAL; > } > @@ -2065,113 +1940,715 @@ > return 0; > } > > The amount of diff lines is not a very interesting metric, I would drop the changes that try to make both drivers look the same. >> >>>>> 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. > > Fair. So, what we really have here: > 1. Use SFF8024 constants defined in linux/sfp.h instead of private ones. > 2. Simplify the logic for selecting SFF_8436 vs SFF_8636 > 3. Improving coding style > 4. Adding extra logging in mlx4_en_get_module_info(), which is also what mlx5e_get_module_info() does. > 5. Make mlx4_en_get_module_info() and mlx5e_get_module_info() to look as close as possible to each other. I would do 1+2 for both drivers, 3+4 for both drivers, and drop 5. Thanks
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c index cd17a3f4faf8..4c985d62af12 100644 --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c @@ -40,6 +40,7 @@ #include <net/ip.h> #include <linux/bitmap.h> #include <linux/mii.h> +#include <linux/sfp.h> #include "mlx4_en.h" #include "en_port.h" @@ -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); 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]); return -EINVAL; } diff --git a/drivers/net/ethernet/mellanox/mlx4/port.c b/drivers/net/ethernet/mellanox/mlx4/port.c index 4e43f4a7d246..6dbd505e7f30 100644 --- a/drivers/net/ethernet/mellanox/mlx4/port.c +++ b/drivers/net/ethernet/mellanox/mlx4/port.c @@ -34,6 +34,7 @@ #include <linux/if_ether.h> #include <linux/if_vlan.h> #include <linux/export.h> +#include <linux/sfp.h> #include <linux/mlx4/cmd.h> @@ -2139,12 +2140,12 @@ int mlx4_get_module_info(struct mlx4_dev *dev, u8 port, return ret; switch (module_id) { - case MLX4_MODULE_ID_SFP: + case SFF8024_ID_SFP: mlx4_sfp_eeprom_params_set(&i2c_addr, &page_num, &offset); break; - case MLX4_MODULE_ID_QSFP: - case MLX4_MODULE_ID_QSFP_PLUS: - case MLX4_MODULE_ID_QSFP28: + case SFF8024_ID_QSFP_8438: + case SFF8024_ID_QSFP_8436_8636: + case SFF8024_ID_QSFP28_8636: mlx4_qsfp_eeprom_params_set(&i2c_addr, &page_num, &offset); break; default: diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c index 4d123dae912c..12a22e5c60ae 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c @@ -32,6 +32,7 @@ #include <linux/dim.h> #include <linux/ethtool_netlink.h> +#include <linux/sfp.h> #include "en.h" #include "en/channels.h" @@ -1899,36 +1900,39 @@ static int mlx5e_get_module_info(struct net_device *netdev, { struct mlx5e_priv *priv = netdev_priv(netdev); struct mlx5_core_dev *dev = priv->mdev; - int size_read = 0; + int ret; u8 data[4] = {0}; - size_read = mlx5_query_module_eeprom(dev, 0, 2, data); - if (size_read < 2) + /* Read first 2 bytes to get Module & REV ID */ + ret = mlx5_query_module_eeprom(dev, + 0 /*offset*/, 2 /*size*/, data); + if (ret < 2) return -EIO; /* data[0] = identifier byte */ switch (data[0]) { - case MLX5_MODULE_ID_QSFP: + case SFF8024_ID_QSFP_8438: modinfo->type = ETH_MODULE_SFF_8436; modinfo->eeprom_len = ETH_MODULE_SFF_8436_MAX_LEN; break; - case MLX5_MODULE_ID_QSFP_PLUS: - case MLX5_MODULE_ID_QSFP28: + case SFF8024_ID_QSFP_8436_8636: /* data[1] = revision id */ - if (data[0] == MLX5_MODULE_ID_QSFP28 || data[1] >= 0x3) { - modinfo->type = ETH_MODULE_SFF_8636; - modinfo->eeprom_len = ETH_MODULE_SFF_8636_MAX_LEN; - } else { + if (data[1] < 0x3) { modinfo->type = ETH_MODULE_SFF_8436; modinfo->eeprom_len = ETH_MODULE_SFF_8436_MAX_LEN; + break; } + fallthrough; + case SFF8024_ID_QSFP28_8636: + modinfo->type = ETH_MODULE_SFF_8636; + modinfo->eeprom_len = ETH_MODULE_SFF_8636_MAX_LEN; break; - case MLX5_MODULE_ID_SFP: + case SFF8024_ID_SFP: modinfo->type = ETH_MODULE_SFF_8472; modinfo->eeprom_len = ETH_MODULE_SFF_8472_LEN; break; default: - netdev_err(priv->netdev, "%s: cable type not recognized:0x%x\n", + netdev_err(priv->netdev, "%s: cable type not recognized: 0x%x\n", __func__, data[0]); return -EINVAL; } diff --git a/drivers/net/ethernet/mellanox/mlx5/core/port.c b/drivers/net/ethernet/mellanox/mlx5/core/port.c index 50931584132b..4258489a5782 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/port.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/port.c @@ -30,6 +30,7 @@ * SOFTWARE. */ +#include <linux/sfp.h> #include <linux/mlx5/port.h> #include "mlx5_core.h" @@ -425,12 +426,12 @@ int mlx5_query_module_eeprom(struct mlx5_core_dev *dev, return err; switch (module_id) { - case MLX5_MODULE_ID_SFP: + case SFF8024_ID_SFP: mlx5_sfp_eeprom_params_set(&query.i2c_address, &query.page, &offset); break; - case MLX5_MODULE_ID_QSFP: - case MLX5_MODULE_ID_QSFP_PLUS: - case MLX5_MODULE_ID_QSFP28: + case SFF8024_ID_QSFP_8438: + case SFF8024_ID_QSFP_8436_8636: + case SFF8024_ID_QSFP28_8636: mlx5_qsfp_eeprom_params_set(&query.i2c_address, &query.page, &offset); break; default: diff --git a/include/linux/mlx4/device.h b/include/linux/mlx4/device.h index 27f42f713c89..a75bfb2a4438 100644 --- a/include/linux/mlx4/device.h +++ b/include/linux/mlx4/device.h @@ -487,13 +487,6 @@ enum { #define MSTR_SM_CHANGE_MASK (MLX4_EQ_PORT_INFO_MSTR_SM_SL_CHANGE_MASK | \ MLX4_EQ_PORT_INFO_MSTR_SM_LID_CHANGE_MASK) -enum mlx4_module_id { - MLX4_MODULE_ID_SFP = 0x3, - MLX4_MODULE_ID_QSFP = 0xC, - MLX4_MODULE_ID_QSFP_PLUS = 0xD, - MLX4_MODULE_ID_QSFP28 = 0x11, -}; - enum { /* rl */ MLX4_QP_RATE_LIMIT_NONE = 0, MLX4_QP_RATE_LIMIT_KBS = 1, diff --git a/include/linux/mlx5/port.h b/include/linux/mlx5/port.h index e68d42b8ce65..e9495271160f 100644 --- a/include/linux/mlx5/port.h +++ b/include/linux/mlx5/port.h @@ -40,14 +40,6 @@ enum mlx5_beacon_duration { MLX5_BEACON_DURATION_INF = 0xffff, }; -enum mlx5_module_id { - MLX5_MODULE_ID_SFP = 0x3, - MLX5_MODULE_ID_QSFP = 0xC, - MLX5_MODULE_ID_QSFP_PLUS = 0xD, - MLX5_MODULE_ID_QSFP28 = 0x11, - MLX5_MODULE_ID_DSFP = 0x1B, -}; - enum mlx5_an_status { MLX5_AN_UNAVAILABLE = 0, MLX5_AN_COMPLETE = 1,
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. Simplify the logic for selecting SFF_8436 vs SFF_8636. Signed-off-by: Krzysztof Piotr Oledzki <ole@ans.pl> --- .../net/ethernet/mellanox/mlx4/en_ethtool.c | 33 ++++++++++--------- drivers/net/ethernet/mellanox/mlx4/port.c | 9 ++--- .../ethernet/mellanox/mlx5/core/en_ethtool.c | 28 +++++++++------- .../net/ethernet/mellanox/mlx5/core/port.c | 9 ++--- include/linux/mlx4/device.h | 7 ---- include/linux/mlx5/port.h | 8 ----- 6 files changed, 44 insertions(+), 50 deletions(-)