| Message ID | 20211018183709.124744-1-erik@kryo.se (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | sfc: Export fibre-specific link modes for 1/10G | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | fail | Errors and warnings before: 4 this patch: 4 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 24 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | No static functions without inline keyword in header files |
On Mon, Oct 18, 2021 at 08:37:08PM +0200, Erik Ekman wrote: > These modes were added to ethtool.h in 5711a98221443 ("net: ethtool: add support > for 1000BaseX and missing 10G link modes") back in 2016. > > Only setting CR mode for 10G, similar to how 25/40/50/100G modes are set up. > > Tested using SFN5122F-R7 (with 2 SFP+ ports) and a 1000BASE-BX10 SFP module. Did you test with a Copper SFP modules? > +++ b/drivers/net/ethernet/sfc/mcdi_port_common.c > @@ -133,9 +133,9 @@ void mcdi_to_ethtool_linkset(u32 media, u32 cap, unsigned long *linkset) > case MC_CMD_MEDIA_QSFP_PLUS: > SET_BIT(FIBRE); > if (cap & (1 << MC_CMD_PHY_CAP_1000FDX_LBN)) > - SET_BIT(1000baseT_Full); > + SET_BIT(1000baseX_Full); I'm wondering if you should have both? The MAC is doing 1000BaseX. But it could then be connected to a copper PHY which then does 1000baseT_Full? At 1G, it is however more likely to be using SGMII, not 1000BaseX. Andrew
On Tue, Oct 19, 2021 at 05:31:52PM +0200, Andrew Lunn wrote: > On Mon, Oct 18, 2021 at 08:37:08PM +0200, Erik Ekman wrote: > > These modes were added to ethtool.h in 5711a98221443 ("net: ethtool: add support > > for 1000BaseX and missing 10G link modes") back in 2016. > > > > Only setting CR mode for 10G, similar to how 25/40/50/100G modes are set up. > > > > Tested using SFN5122F-R7 (with 2 SFP+ ports) and a 1000BASE-BX10 SFP module. > > Did you test with a Copper SFP modules? > > > +++ b/drivers/net/ethernet/sfc/mcdi_port_common.c > > @@ -133,9 +133,9 @@ void mcdi_to_ethtool_linkset(u32 media, u32 cap, unsigned long *linkset) > > case MC_CMD_MEDIA_QSFP_PLUS: > > SET_BIT(FIBRE); > > if (cap & (1 << MC_CMD_PHY_CAP_1000FDX_LBN)) > > - SET_BIT(1000baseT_Full); > > + SET_BIT(1000baseX_Full); > > I'm wondering if you should have both? The MAC is doing 1000BaseX. But > it could then be connected to a copper PHY which then does > 1000baseT_Full? At 1G, it is however more likely to be using SGMII, > not 1000BaseX. Yes, they should both be set. We actually did a 10Gbase-T version of Siena, the SFN51x1T. Martin
On Tue, 19 Oct 2021 at 17:53, Martin Habets <habetsm.xilinx@gmail.com> wrote: > > On Tue, Oct 19, 2021 at 05:31:52PM +0200, Andrew Lunn wrote: > > On Mon, Oct 18, 2021 at 08:37:08PM +0200, Erik Ekman wrote: > > > These modes were added to ethtool.h in 5711a98221443 ("net: ethtool: add support > > > for 1000BaseX and missing 10G link modes") back in 2016. > > > > > > Only setting CR mode for 10G, similar to how 25/40/50/100G modes are set up. > > > > > > Tested using SFN5122F-R7 (with 2 SFP+ ports) and a 1000BASE-BX10 SFP module. > > > > Did you test with a Copper SFP modules? > > I have tested it with a copper SFP PHY at 1G and that works fine. I don't have the hardware to test copper 10G (RJ45). > > > +++ b/drivers/net/ethernet/sfc/mcdi_port_common.c > > > @@ -133,9 +133,9 @@ void mcdi_to_ethtool_linkset(u32 media, u32 cap, unsigned long *linkset) > > > case MC_CMD_MEDIA_QSFP_PLUS: > > > SET_BIT(FIBRE); > > > if (cap & (1 << MC_CMD_PHY_CAP_1000FDX_LBN)) > > > - SET_BIT(1000baseT_Full); > > > + SET_BIT(1000baseX_Full); > > > > I'm wondering if you should have both? The MAC is doing 1000BaseX. But > > it could then be connected to a copper PHY which then does > > 1000baseT_Full? At 1G, it is however more likely to be using SGMII, > > not 1000BaseX. Yes, we can return both. Similarly, is there a reason only CR modes are set for fiber ports, when I expect LR/SR/etc to work as well? I can set the modes for 10G similar to the example in 5711a98221443 ("net: ethtool: add support for 1000BaseX and missing 10G link modes"): CR/SR/LR/ER I inserted a LR SFP+ module and ethtool -m could list its settings at least. > > Yes, they should both be set. We actually did a 10Gbase-T version of Siena, > the SFN51x1T. > For the baseT-version of the cards the supported modes are set further down in the file (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/sfc/mcdi_port_common.c#n149), so they are not affected by this change. Thanks /Erik
On Tue, 19 Oct 2021 at 19:34, Erik Ekman <erik@kryo.se> wrote: > > On Tue, 19 Oct 2021 at 17:53, Martin Habets <habetsm.xilinx@gmail.com> wrote: > > > > On Tue, Oct 19, 2021 at 05:31:52PM +0200, Andrew Lunn wrote: > > > On Mon, Oct 18, 2021 at 08:37:08PM +0200, Erik Ekman wrote: > > > > These modes were added to ethtool.h in 5711a98221443 ("net: ethtool: add support > > > > for 1000BaseX and missing 10G link modes") back in 2016. > > > > > > > > Only setting CR mode for 10G, similar to how 25/40/50/100G modes are set up. > > > > > > > > Tested using SFN5122F-R7 (with 2 SFP+ ports) and a 1000BASE-BX10 SFP module. > > > > > > Did you test with a Copper SFP modules? > > > > > I have tested it with a copper SFP PHY at 1G and that works fine. > I don't have the hardware to test copper 10G (RJ45). > > > > > +++ b/drivers/net/ethernet/sfc/mcdi_port_common.c > > > > @@ -133,9 +133,9 @@ void mcdi_to_ethtool_linkset(u32 media, u32 cap, unsigned long *linkset) > > > > case MC_CMD_MEDIA_QSFP_PLUS: > > > > SET_BIT(FIBRE); > > > > if (cap & (1 << MC_CMD_PHY_CAP_1000FDX_LBN)) > > > > - SET_BIT(1000baseT_Full); > > > > + SET_BIT(1000baseX_Full); > > > > > > I'm wondering if you should have both? The MAC is doing 1000BaseX. But > > > it could then be connected to a copper PHY which then does > > > 1000baseT_Full? At 1G, it is however more likely to be using SGMII, > > > not 1000BaseX. If you mean that the card has only copper phys on the card, then a different case in the switch statement is run (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/sfc/mcdi_port_common.c#n149). Should we still return both if the only baseT support is via an SFP module? > > Yes, we can return both. > > Similarly, is there a reason only CR modes are set for fiber ports, > when I expect LR/SR/etc to work as well? > I can set the modes for 10G similar to the example in 5711a98221443 > ("net: ethtool: add support > for 1000BaseX and missing 10G link modes"): CR/SR/LR/ER > I inserted a LR SFP+ module and ethtool -m could list its settings at least. > > > > > Yes, they should both be set. We actually did a 10Gbase-T version of Siena, > > the SFN51x1T. > > > > For the baseT-version of the cards the supported modes are set further > down in the file > (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/sfc/mcdi_port_common.c#n149), > so they are not affected by this change. > > Thanks > /Erik
On Tue, Oct 19, 2021 at 07:34:16PM +0200, Erik Ekman wrote: > On Tue, 19 Oct 2021 at 17:53, Martin Habets <habetsm.xilinx@gmail.com> wrote: > > > > On Tue, Oct 19, 2021 at 05:31:52PM +0200, Andrew Lunn wrote: > > > On Mon, Oct 18, 2021 at 08:37:08PM +0200, Erik Ekman wrote: > > > > These modes were added to ethtool.h in 5711a98221443 ("net: ethtool: add support > > > > for 1000BaseX and missing 10G link modes") back in 2016. > > > > > > > > Only setting CR mode for 10G, similar to how 25/40/50/100G modes are set up. > > > > > > > > Tested using SFN5122F-R7 (with 2 SFP+ ports) and a 1000BASE-BX10 SFP module. > > > > > > Did you test with a Copper SFP modules? > > > > > I have tested it with a copper SFP PHY at 1G and that works fine. Meaning ethtool returns 1000BaseT_FULL? Does the SFP also support 10/100? Does ethtool list 10BaseT_Half, 10BaseT_Full, etc. Andrew
On Tue, 19 Oct 2021 at 20:27, Andrew Lunn <andrew@lunn.ch> wrote: > > On Tue, Oct 19, 2021 at 07:34:16PM +0200, Erik Ekman wrote: > > On Tue, 19 Oct 2021 at 17:53, Martin Habets <habetsm.xilinx@gmail.com> wrote: > > > > > > On Tue, Oct 19, 2021 at 05:31:52PM +0200, Andrew Lunn wrote: > > > > On Mon, Oct 18, 2021 at 08:37:08PM +0200, Erik Ekman wrote: > > > > > These modes were added to ethtool.h in 5711a98221443 ("net: ethtool: add support > > > > > for 1000BaseX and missing 10G link modes") back in 2016. > > > > > > > > > > Only setting CR mode for 10G, similar to how 25/40/50/100G modes are set up. > > > > > > > > > > Tested using SFN5122F-R7 (with 2 SFP+ ports) and a 1000BASE-BX10 SFP module. > > > > > > > > Did you test with a Copper SFP modules? > > > > > > > > I have tested it with a copper SFP PHY at 1G and that works fine. > > Meaning ethtool returns 1000BaseT_FULL? Does the SFP also support > 10/100? Does ethtool list 10BaseT_Half, 10BaseT_Full, etc. > The supported modes do not change based on the module used. From the code it looks to only be based on the phy capability bits (MC_CMD_PHY_CAP_1000FDX_LBN, MC_CMD_PHY_CAP_10000FDX_LBN). The copper SFP does not link at 100Mbps, only at 1Gbps (from my testing). From the 'Solarflare Server Adapter User Guide': "SFP 1000BASE‐T module, Autonegotiation: No, Speed: 1G, Comment: These modules support only 1G and will not link up at 100Mbps" 10G SFP+ Base-T modules are not mentioned, maybe they did not exist back then. Do you think the 1000BaseT_Full should be used because of this? Reading the user guide further I see lists of supported SFP+ -LR and -SR modules as well as QSFP+ SR4 modules so I am planning to add them. /Erik
On Mon, Oct 18, 2021 at 08:37:08PM +0200, Erik Ekman wrote: > These modes were added to ethtool.h in 5711a98221443 ("net: ethtool: add support > for 1000BaseX and missing 10G link modes") back in 2016. > > Only setting CR mode for 10G, similar to how 25/40/50/100G modes are set up. > > Tested using SFN5122F-R7 (with 2 SFP+ ports) and a 1000BASE-BX10 SFP module. > Before: > > $ ethtool ext > Settings for ext: > Supported ports: [ FIBRE ] > Supported link modes: 1000baseT/Full > 10000baseT/Full > Supported pause frame use: Symmetric Receive-only > Supports auto-negotiation: No > Supported FEC modes: Not reported > Advertised link modes: Not reported > Advertised pause frame use: No > Advertised auto-negotiation: No > Advertised FEC modes: Not reported > Link partner advertised link modes: Not reported > Link partner advertised pause frame use: No > Link partner advertised auto-negotiation: No > Link partner advertised FEC modes: Not reported > Speed: 1000Mb/s > Duplex: Full > Auto-negotiation: off > Port: FIBRE > PHYAD: 255 > Transceiver: internal > Current message level: 0x000020f7 (8439) > drv probe link ifdown ifup rx_err tx_err hw > Link detected: yes > > After: > > $ ethtool ext > Settings for ext: > Supported ports: [ FIBRE ] > Supported link modes: 1000baseX/Full > 10000baseCR/Full > Supported pause frame use: Symmetric Receive-only > Supports auto-negotiation: No > Supported FEC modes: Not reported > Advertised link modes: Not reported > Advertised pause frame use: No > Advertised auto-negotiation: No > Advertised FEC modes: Not reported > Link partner advertised link modes: Not reported > Link partner advertised pause frame use: No > Link partner advertised auto-negotiation: No > Link partner advertised FEC modes: Not reported > Speed: 1000Mb/s > Duplex: Full > Auto-negotiation: off > Port: FIBRE > PHYAD: 255 > Transceiver: internal > Supports Wake-on: g > Wake-on: d > Current message level: 0x000020f7 (8439) > drv probe link ifdown ifup rx_err tx_err hw > Link detected: yes > > Signed-off-by: Erik Ekman <erik@kryo.se> Acked-by: Martin Habets <habetsm.xilinx@gmail.com> > --- > drivers/net/ethernet/sfc/mcdi_port_common.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/sfc/mcdi_port_common.c b/drivers/net/ethernet/sfc/mcdi_port_common.c > index 4bd3ef8f3384..e84cdcb6a595 100644 > --- a/drivers/net/ethernet/sfc/mcdi_port_common.c > +++ b/drivers/net/ethernet/sfc/mcdi_port_common.c > @@ -133,9 +133,9 @@ void mcdi_to_ethtool_linkset(u32 media, u32 cap, unsigned long *linkset) > case MC_CMD_MEDIA_QSFP_PLUS: > SET_BIT(FIBRE); > if (cap & (1 << MC_CMD_PHY_CAP_1000FDX_LBN)) > - SET_BIT(1000baseT_Full); > + SET_BIT(1000baseX_Full); > if (cap & (1 << MC_CMD_PHY_CAP_10000FDX_LBN)) > - SET_BIT(10000baseT_Full); > + SET_BIT(10000baseCR_Full); > if (cap & (1 << MC_CMD_PHY_CAP_40000FDX_LBN)) > SET_BIT(40000baseCR4_Full); > if (cap & (1 << MC_CMD_PHY_CAP_100000FDX_LBN)) > @@ -192,9 +192,11 @@ u32 ethtool_linkset_to_mcdi_cap(const unsigned long *linkset) > result |= (1 << MC_CMD_PHY_CAP_100FDX_LBN); > if (TEST_BIT(1000baseT_Half)) > result |= (1 << MC_CMD_PHY_CAP_1000HDX_LBN); > - if (TEST_BIT(1000baseT_Full) || TEST_BIT(1000baseKX_Full)) > + if (TEST_BIT(1000baseT_Full) || TEST_BIT(1000baseKX_Full) || > + TEST_BIT(1000baseX_Full)) > result |= (1 << MC_CMD_PHY_CAP_1000FDX_LBN); > - if (TEST_BIT(10000baseT_Full) || TEST_BIT(10000baseKX4_Full)) > + if (TEST_BIT(10000baseT_Full) || TEST_BIT(10000baseKX4_Full) || > + TEST_BIT(10000baseCR_Full)) > result |= (1 << MC_CMD_PHY_CAP_10000FDX_LBN); > if (TEST_BIT(40000baseCR4_Full) || TEST_BIT(40000baseKR4_Full)) > result |= (1 << MC_CMD_PHY_CAP_40000FDX_LBN); > -- > 2.31.1
> >From the 'Solarflare Server Adapter User Guide': "SFP 1000BASE‐T > module, Autonegotiation: No, Speed: 1G, Comment: These modules > support only 1G and will not link up at 100Mbps" > 10G SFP+ Base-T modules are not mentioned, maybe they did not exist > back then. Do you think the 1000BaseT_Full should be used because of > this? With a MAC connected to an SFP cage, i would list the T modes supported, since you have no idea what SFP module the user will install, copper or fibre. Andrew
On Tue, 19 Oct 2021 at 21:18, Martin Habets <habetsm.xilinx@gmail.com> wrote: > > On Mon, Oct 18, 2021 at 08:37:08PM +0200, Erik Ekman wrote: > > These modes were added to ethtool.h in 5711a98221443 ("net: ethtool: add support > > for 1000BaseX and missing 10G link modes") back in 2016. > > > > Only setting CR mode for 10G, similar to how 25/40/50/100G modes are set up. > > > > Tested using SFN5122F-R7 (with 2 SFP+ ports) and a 1000BASE-BX10 SFP module. > > Before: > > > > $ ethtool ext > > Settings for ext: > > Supported ports: [ FIBRE ] > > Supported link modes: 1000baseT/Full > > 10000baseT/Full > > Supported pause frame use: Symmetric Receive-only > > Supports auto-negotiation: No > > Supported FEC modes: Not reported > > Advertised link modes: Not reported > > Advertised pause frame use: No > > Advertised auto-negotiation: No > > Advertised FEC modes: Not reported > > Link partner advertised link modes: Not reported > > Link partner advertised pause frame use: No > > Link partner advertised auto-negotiation: No > > Link partner advertised FEC modes: Not reported > > Speed: 1000Mb/s > > Duplex: Full > > Auto-negotiation: off > > Port: FIBRE > > PHYAD: 255 > > Transceiver: internal > > Current message level: 0x000020f7 (8439) > > drv probe link ifdown ifup rx_err tx_err hw > > Link detected: yes > > > > After: > > > > $ ethtool ext > > Settings for ext: > > Supported ports: [ FIBRE ] > > Supported link modes: 1000baseX/Full > > 10000baseCR/Full > > Supported pause frame use: Symmetric Receive-only > > Supports auto-negotiation: No > > Supported FEC modes: Not reported > > Advertised link modes: Not reported > > Advertised pause frame use: No > > Advertised auto-negotiation: No > > Advertised FEC modes: Not reported > > Link partner advertised link modes: Not reported > > Link partner advertised pause frame use: No > > Link partner advertised auto-negotiation: No > > Link partner advertised FEC modes: Not reported > > Speed: 1000Mb/s > > Duplex: Full > > Auto-negotiation: off > > Port: FIBRE > > PHYAD: 255 > > Transceiver: internal > > Supports Wake-on: g > > Wake-on: d > > Current message level: 0x000020f7 (8439) > > drv probe link ifdown ifup rx_err tx_err hw > > Link detected: yes > > > > Signed-off-by: Erik Ekman <erik@kryo.se> > > Acked-by: Martin Habets <habetsm.xilinx@gmail.com> > I will send a v2 patch with more modes marked supported. /Erik
diff --git a/drivers/net/ethernet/sfc/mcdi_port_common.c b/drivers/net/ethernet/sfc/mcdi_port_common.c index 4bd3ef8f3384..e84cdcb6a595 100644 --- a/drivers/net/ethernet/sfc/mcdi_port_common.c +++ b/drivers/net/ethernet/sfc/mcdi_port_common.c @@ -133,9 +133,9 @@ void mcdi_to_ethtool_linkset(u32 media, u32 cap, unsigned long *linkset) case MC_CMD_MEDIA_QSFP_PLUS: SET_BIT(FIBRE); if (cap & (1 << MC_CMD_PHY_CAP_1000FDX_LBN)) - SET_BIT(1000baseT_Full); + SET_BIT(1000baseX_Full); if (cap & (1 << MC_CMD_PHY_CAP_10000FDX_LBN)) - SET_BIT(10000baseT_Full); + SET_BIT(10000baseCR_Full); if (cap & (1 << MC_CMD_PHY_CAP_40000FDX_LBN)) SET_BIT(40000baseCR4_Full); if (cap & (1 << MC_CMD_PHY_CAP_100000FDX_LBN)) @@ -192,9 +192,11 @@ u32 ethtool_linkset_to_mcdi_cap(const unsigned long *linkset) result |= (1 << MC_CMD_PHY_CAP_100FDX_LBN); if (TEST_BIT(1000baseT_Half)) result |= (1 << MC_CMD_PHY_CAP_1000HDX_LBN); - if (TEST_BIT(1000baseT_Full) || TEST_BIT(1000baseKX_Full)) + if (TEST_BIT(1000baseT_Full) || TEST_BIT(1000baseKX_Full) || + TEST_BIT(1000baseX_Full)) result |= (1 << MC_CMD_PHY_CAP_1000FDX_LBN); - if (TEST_BIT(10000baseT_Full) || TEST_BIT(10000baseKX4_Full)) + if (TEST_BIT(10000baseT_Full) || TEST_BIT(10000baseKX4_Full) || + TEST_BIT(10000baseCR_Full)) result |= (1 << MC_CMD_PHY_CAP_10000FDX_LBN); if (TEST_BIT(40000baseCR4_Full) || TEST_BIT(40000baseKR4_Full)) result |= (1 << MC_CMD_PHY_CAP_40000FDX_LBN);
These modes were added to ethtool.h in 5711a98221443 ("net: ethtool: add support for 1000BaseX and missing 10G link modes") back in 2016. Only setting CR mode for 10G, similar to how 25/40/50/100G modes are set up. Tested using SFN5122F-R7 (with 2 SFP+ ports) and a 1000BASE-BX10 SFP module. Before: $ ethtool ext Settings for ext: Supported ports: [ FIBRE ] Supported link modes: 1000baseT/Full 10000baseT/Full Supported pause frame use: Symmetric Receive-only Supports auto-negotiation: No Supported FEC modes: Not reported Advertised link modes: Not reported Advertised pause frame use: No Advertised auto-negotiation: No Advertised FEC modes: Not reported Link partner advertised link modes: Not reported Link partner advertised pause frame use: No Link partner advertised auto-negotiation: No Link partner advertised FEC modes: Not reported Speed: 1000Mb/s Duplex: Full Auto-negotiation: off Port: FIBRE PHYAD: 255 Transceiver: internal Current message level: 0x000020f7 (8439) drv probe link ifdown ifup rx_err tx_err hw Link detected: yes After: $ ethtool ext Settings for ext: Supported ports: [ FIBRE ] Supported link modes: 1000baseX/Full 10000baseCR/Full Supported pause frame use: Symmetric Receive-only Supports auto-negotiation: No Supported FEC modes: Not reported Advertised link modes: Not reported Advertised pause frame use: No Advertised auto-negotiation: No Advertised FEC modes: Not reported Link partner advertised link modes: Not reported Link partner advertised pause frame use: No Link partner advertised auto-negotiation: No Link partner advertised FEC modes: Not reported Speed: 1000Mb/s Duplex: Full Auto-negotiation: off Port: FIBRE PHYAD: 255 Transceiver: internal Supports Wake-on: g Wake-on: d Current message level: 0x000020f7 (8439) drv probe link ifdown ifup rx_err tx_err hw Link detected: yes Signed-off-by: Erik Ekman <erik@kryo.se> --- drivers/net/ethernet/sfc/mcdi_port_common.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)