* [PATCH net-next 0/6] Support setting lanes via ethtool
@ 2020-10-10 15:41 Ido Schimmel
2020-10-10 15:41 ` [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes Ido Schimmel
` (5 more replies)
0 siblings, 6 replies; 45+ messages in thread
From: Ido Schimmel @ 2020-10-10 15:41 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, jiri, danieller, andrew, f.fainelli, mkubecek,
mlxsw, Ido Schimmel
From: Ido Schimmel <idosch@nvidia.com>
Danielle says:
Some speeds can be achieved with different number of lanes. For example,
100Gbps can be achieved using two lanes of 50Gbps or four lanes of
25Gbps. This patch set adds a new selector that allows ethtool to
advertise link modes according to their number of lanes and also force a
specific number of lanes when autonegotiation is off.
Advertising all link modes with a speed of 100Gbps that use two lanes:
# ethtool -s swp1 speed 100000 lanes 2 autoneg on
Forcing a speed of 100Gbps using four lanes:
# ethtool -s swp1 speed 100000 lanes 4 autoneg off
Patch set overview:
Patch #1 allows user space to configure the desired number of lanes.
Patch #2 adjusts ethtool to dump to user space the number of lanes
currently in use.
Patches #3-#5 add support for lanes configuration in mlxsw.
Patch #6 adds a selftest.
Danielle Ratson (6):
ethtool: Extend link modes settings uAPI with lanes
ethtool: Expose the number of lanes in use
mlxsw: ethtool: Remove max lanes filtering
mlxsw: ethtool: Add support for setting lanes when autoneg is off
mlxsw: ethtool: Expose the number of lanes in use
net: selftests: Add lanes setting test
Documentation/networking/ethtool-netlink.rst | 16 +-
.../net/ethernet/mellanox/mlxsw/spectrum.h | 13 +-
.../mellanox/mlxsw/spectrum_ethtool.c | 156 ++++++++----
include/linux/ethtool.h | 4 +
include/uapi/linux/ethtool.h | 8 +
include/uapi/linux/ethtool_netlink.h | 1 +
net/ethtool/linkmodes.c | 232 +++++++++++-------
net/ethtool/netlink.h | 2 +-
.../selftests/net/forwarding/ethtool_lanes.sh | 224 +++++++++++++++++
.../selftests/net/forwarding/ethtool_lib.sh | 34 +++
tools/testing/selftests/net/forwarding/lib.sh | 28 +++
11 files changed, 571 insertions(+), 147 deletions(-)
create mode 100755 tools/testing/selftests/net/forwarding/ethtool_lanes.sh
--
2.26.2
^ permalink raw reply [flat|nested] 45+ messages in thread* [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-10 15:41 [PATCH net-next 0/6] Support setting lanes via ethtool Ido Schimmel @ 2020-10-10 15:41 ` Ido Schimmel 2020-10-11 22:37 ` Jakub Kicinski ` (2 more replies) 2020-10-10 15:41 ` [PATCH net-next 2/6] ethtool: Expose the number of lanes in use Ido Schimmel ` (4 subsequent siblings) 5 siblings, 3 replies; 45+ messages in thread From: Ido Schimmel @ 2020-10-10 15:41 UTC (permalink / raw) To: netdev Cc: davem, kuba, jiri, danieller, andrew, f.fainelli, mkubecek, mlxsw, Ido Schimmel From: Danielle Ratson <danieller@nvidia.com> Currently, when auto negotiation is on, the user can advertise all the linkmodes which correspond to a specific speed, but does not have a similar selector for the number of lanes. This is significant when a specific speed can be achieved using different number of lanes. For example, 2x50 or 4x25. Add 'ETHTOOL_A_LINKMODES_LANES' attribute and expand 'struct ethtool_link_settings' with lanes field in order to implement a new lanes-selector that will enable the user to advertise a specific number of lanes as well. When auto negotiation is off, lanes parameter can be forced only if the driver supports it. Add a capability bit in 'struct ethtool_ops' that allows ethtool know if the driver can handle the lanes parameter when auto negotiation is off, so if it does not, an error message will be returned when trying to set lanes. Example: $ ethtool -s swp1 lanes 4 $ ethtool swp1 Settings for swp1: Supported ports: [ FIBRE ] Supported link modes: 1000baseKX/Full 10000baseKR/Full 40000baseCR4/Full 40000baseSR4/Full 40000baseLR4/Full 25000baseCR/Full 25000baseSR/Full 50000baseCR2/Full 100000baseSR4/Full 100000baseCR4/Full Supported pause frame use: Symmetric Receive-only Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: 40000baseCR4/Full 40000baseSR4/Full 40000baseLR4/Full 100000baseSR4/Full 100000baseCR4/Full Advertised pause frame use: No Advertised auto-negotiation: Yes Advertised FEC modes: Not reported Speed: Unknown! Duplex: Unknown! (255) Auto-negotiation: on Port: Direct Attach Copper PHYAD: 0 Transceiver: internal Link detected: no Signed-off-by: Danielle Ratson <danieller@nvidia.com> Reviewed-by: Jiri Pirko <jiri@nvidia.com> Signed-off-by: Ido Schimmel <idosch@nvidia.com> --- Documentation/networking/ethtool-netlink.rst | 11 +- include/linux/ethtool.h | 4 + include/uapi/linux/ethtool.h | 8 + include/uapi/linux/ethtool_netlink.h | 1 + net/ethtool/linkmodes.c | 227 +++++++++++-------- net/ethtool/netlink.h | 2 +- 6 files changed, 158 insertions(+), 95 deletions(-) diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst index 30b98245979f..05073482db05 100644 --- a/Documentation/networking/ethtool-netlink.rst +++ b/Documentation/networking/ethtool-netlink.rst @@ -431,16 +431,17 @@ Request contents: ``ETHTOOL_A_LINKMODES_SPEED`` u32 link speed (Mb/s) ``ETHTOOL_A_LINKMODES_DUPLEX`` u8 duplex mode ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG`` u8 Master/slave port mode + ``ETHTOOL_A_LINKMODES_LANES`` u32 lanes ========================================== ====== ========================== ``ETHTOOL_A_LINKMODES_OURS`` bit set allows setting advertised link modes. If autonegotiation is on (either set now or kept from before), advertised modes are not changed (no ``ETHTOOL_A_LINKMODES_OURS`` attribute) and at least one -of speed and duplex is specified, kernel adjusts advertised modes to all -supported modes matching speed, duplex or both (whatever is specified). This -autoselection is done on ethtool side with ioctl interface, netlink interface -is supposed to allow requesting changes without knowing what exactly kernel -supports. +of speed, duplex and lanes is specified, kernel adjusts advertised modes to all +supported modes matching speed, duplex, lanes or all (whatever is specified). +This autoselection is done on ethtool side with ioctl interface, netlink +interface is supposed to allow requesting changes without knowing what exactly +kernel supports. LINKSTATE_GET diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index 6408b446051f..4945ce487f01 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -128,6 +128,7 @@ struct ethtool_link_ksettings { __ETHTOOL_DECLARE_LINK_MODE_MASK(advertising); __ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising); } link_modes; + u32 lanes; }; /** @@ -241,6 +242,8 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32, ETHTOOL_COALESCE_PKT_RATE_LOW | ETHTOOL_COALESCE_PKT_RATE_HIGH | \ ETHTOOL_COALESCE_RATE_SAMPLE_INTERVAL) +#define ETHTOOL_CAP_LINK_LANES_SUPPORTED BIT(0) + #define ETHTOOL_STAT_NOT_SET (~0ULL) /** @@ -419,6 +422,7 @@ struct ethtool_pause_stats { * of the generic netdev features interface. */ struct ethtool_ops { + u32 capabilities; u32 supported_coalesce_params; void (*get_drvinfo)(struct net_device *, struct ethtool_drvinfo *); int (*get_regs_len)(struct net_device *); diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h index 9ca87bc73c44..2125b93ecee0 100644 --- a/include/uapi/linux/ethtool.h +++ b/include/uapi/linux/ethtool.h @@ -1738,6 +1738,14 @@ static inline int ethtool_validate_speed(__u32 speed) return speed <= INT_MAX || speed == (__u32)SPEED_UNKNOWN; } +/* Lanes, 1, 2, 4 or 8. */ +#define ETHTOOL_LANES_1 1 +#define ETHTOOL_LANES_2 2 +#define ETHTOOL_LANES_4 4 +#define ETHTOOL_LANES_8 8 + +#define ETHTOOL_LANES_UNKNOWN 0 + /* Duplex, half or full. */ #define DUPLEX_HALF 0x00 #define DUPLEX_FULL 0x01 diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h index e2bf36e6964b..a286635ac9b8 100644 --- a/include/uapi/linux/ethtool_netlink.h +++ b/include/uapi/linux/ethtool_netlink.h @@ -227,6 +227,7 @@ enum { ETHTOOL_A_LINKMODES_DUPLEX, /* u8 */ ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG, /* u8 */ ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE, /* u8 */ + ETHTOOL_A_LINKMODES_LANES, /* u32 */ /* add new constants above here */ __ETHTOOL_A_LINKMODES_CNT, diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c index c5bcb9abc8b9..a1f02896ed8b 100644 --- a/net/ethtool/linkmodes.c +++ b/net/ethtool/linkmodes.c @@ -152,12 +152,14 @@ const struct ethnl_request_ops ethnl_linkmodes_request_ops = { struct link_mode_info { int speed; + int lanes; u8 duplex; }; -#define __DEFINE_LINK_MODE_PARAMS(_speed, _type, _duplex) \ +#define __DEFINE_LINK_MODE_PARAMS(_speed, _type, _lanes, _duplex) \ [ETHTOOL_LINK_MODE(_speed, _type, _duplex)] = { \ .speed = SPEED_ ## _speed, \ + .lanes = ETHTOOL_LANES_ ## _lanes, \ .duplex = __DUPLEX_ ## _duplex \ } #define __DUPLEX_Half DUPLEX_HALF @@ -165,105 +167,106 @@ struct link_mode_info { #define __DEFINE_SPECIAL_MODE_PARAMS(_mode) \ [ETHTOOL_LINK_MODE_ ## _mode ## _BIT] = { \ .speed = SPEED_UNKNOWN, \ + .lanes = ETHTOOL_LANES_UNKNOWN, \ .duplex = DUPLEX_UNKNOWN, \ } static const struct link_mode_info link_mode_params[] = { - __DEFINE_LINK_MODE_PARAMS(10, T, Half), - __DEFINE_LINK_MODE_PARAMS(10, T, Full), - __DEFINE_LINK_MODE_PARAMS(100, T, Half), - __DEFINE_LINK_MODE_PARAMS(100, T, Full), - __DEFINE_LINK_MODE_PARAMS(1000, T, Half), - __DEFINE_LINK_MODE_PARAMS(1000, T, Full), + __DEFINE_LINK_MODE_PARAMS(10, T, 1, Half), + __DEFINE_LINK_MODE_PARAMS(10, T, 1, Full), + __DEFINE_LINK_MODE_PARAMS(100, T, 1, Half), + __DEFINE_LINK_MODE_PARAMS(100, T, 1, Full), + __DEFINE_LINK_MODE_PARAMS(1000, T, 1, Half), + __DEFINE_LINK_MODE_PARAMS(1000, T, 1, Full), __DEFINE_SPECIAL_MODE_PARAMS(Autoneg), __DEFINE_SPECIAL_MODE_PARAMS(TP), __DEFINE_SPECIAL_MODE_PARAMS(AUI), __DEFINE_SPECIAL_MODE_PARAMS(MII), __DEFINE_SPECIAL_MODE_PARAMS(FIBRE), __DEFINE_SPECIAL_MODE_PARAMS(BNC), - __DEFINE_LINK_MODE_PARAMS(10000, T, Full), + __DEFINE_LINK_MODE_PARAMS(10000, T, 1, Full), __DEFINE_SPECIAL_MODE_PARAMS(Pause), __DEFINE_SPECIAL_MODE_PARAMS(Asym_Pause), - __DEFINE_LINK_MODE_PARAMS(2500, X, Full), + __DEFINE_LINK_MODE_PARAMS(2500, X, 1, Full), __DEFINE_SPECIAL_MODE_PARAMS(Backplane), - __DEFINE_LINK_MODE_PARAMS(1000, KX, Full), - __DEFINE_LINK_MODE_PARAMS(10000, KX4, Full), - __DEFINE_LINK_MODE_PARAMS(10000, KR, Full), + __DEFINE_LINK_MODE_PARAMS(1000, KX, 1, Full), + __DEFINE_LINK_MODE_PARAMS(10000, KX4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(10000, KR, 1, Full), [ETHTOOL_LINK_MODE_10000baseR_FEC_BIT] = { .speed = SPEED_10000, .duplex = DUPLEX_FULL, }, - __DEFINE_LINK_MODE_PARAMS(20000, MLD2, Full), - __DEFINE_LINK_MODE_PARAMS(20000, KR2, Full), - __DEFINE_LINK_MODE_PARAMS(40000, KR4, Full), - __DEFINE_LINK_MODE_PARAMS(40000, CR4, Full), - __DEFINE_LINK_MODE_PARAMS(40000, SR4, Full), - __DEFINE_LINK_MODE_PARAMS(40000, LR4, Full), - __DEFINE_LINK_MODE_PARAMS(56000, KR4, Full), - __DEFINE_LINK_MODE_PARAMS(56000, CR4, Full), - __DEFINE_LINK_MODE_PARAMS(56000, SR4, Full), - __DEFINE_LINK_MODE_PARAMS(56000, LR4, Full), - __DEFINE_LINK_MODE_PARAMS(25000, CR, Full), - __DEFINE_LINK_MODE_PARAMS(25000, KR, Full), - __DEFINE_LINK_MODE_PARAMS(25000, SR, Full), - __DEFINE_LINK_MODE_PARAMS(50000, CR2, Full), - __DEFINE_LINK_MODE_PARAMS(50000, KR2, Full), - __DEFINE_LINK_MODE_PARAMS(100000, KR4, Full), - __DEFINE_LINK_MODE_PARAMS(100000, SR4, Full), - __DEFINE_LINK_MODE_PARAMS(100000, CR4, Full), - __DEFINE_LINK_MODE_PARAMS(100000, LR4_ER4, Full), - __DEFINE_LINK_MODE_PARAMS(50000, SR2, Full), - __DEFINE_LINK_MODE_PARAMS(1000, X, Full), - __DEFINE_LINK_MODE_PARAMS(10000, CR, Full), - __DEFINE_LINK_MODE_PARAMS(10000, SR, Full), - __DEFINE_LINK_MODE_PARAMS(10000, LR, Full), - __DEFINE_LINK_MODE_PARAMS(10000, LRM, Full), - __DEFINE_LINK_MODE_PARAMS(10000, ER, Full), - __DEFINE_LINK_MODE_PARAMS(2500, T, Full), - __DEFINE_LINK_MODE_PARAMS(5000, T, Full), + __DEFINE_LINK_MODE_PARAMS(20000, MLD2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(20000, KR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(40000, KR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(40000, CR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(40000, SR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(40000, LR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(56000, KR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(56000, CR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(56000, SR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(56000, LR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(25000, CR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(25000, KR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(25000, SR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(50000, CR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(50000, KR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(100000, KR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(100000, SR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(100000, CR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(100000, LR4_ER4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(50000, SR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(1000, X, 1, Full), + __DEFINE_LINK_MODE_PARAMS(10000, CR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(10000, SR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(10000, LR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(10000, LRM, 1, Full), + __DEFINE_LINK_MODE_PARAMS(10000, ER, 1, Full), + __DEFINE_LINK_MODE_PARAMS(2500, T, 1, Full), + __DEFINE_LINK_MODE_PARAMS(5000, T, 1, Full), __DEFINE_SPECIAL_MODE_PARAMS(FEC_NONE), __DEFINE_SPECIAL_MODE_PARAMS(FEC_RS), __DEFINE_SPECIAL_MODE_PARAMS(FEC_BASER), - __DEFINE_LINK_MODE_PARAMS(50000, KR, Full), - __DEFINE_LINK_MODE_PARAMS(50000, SR, Full), - __DEFINE_LINK_MODE_PARAMS(50000, CR, Full), - __DEFINE_LINK_MODE_PARAMS(50000, LR_ER_FR, Full), - __DEFINE_LINK_MODE_PARAMS(50000, DR, Full), - __DEFINE_LINK_MODE_PARAMS(100000, KR2, Full), - __DEFINE_LINK_MODE_PARAMS(100000, SR2, Full), - __DEFINE_LINK_MODE_PARAMS(100000, CR2, Full), - __DEFINE_LINK_MODE_PARAMS(100000, LR2_ER2_FR2, Full), - __DEFINE_LINK_MODE_PARAMS(100000, DR2, Full), - __DEFINE_LINK_MODE_PARAMS(200000, KR4, Full), - __DEFINE_LINK_MODE_PARAMS(200000, SR4, Full), - __DEFINE_LINK_MODE_PARAMS(200000, LR4_ER4_FR4, Full), - __DEFINE_LINK_MODE_PARAMS(200000, DR4, Full), - __DEFINE_LINK_MODE_PARAMS(200000, CR4, Full), - __DEFINE_LINK_MODE_PARAMS(100, T1, Full), - __DEFINE_LINK_MODE_PARAMS(1000, T1, Full), - __DEFINE_LINK_MODE_PARAMS(400000, KR8, Full), - __DEFINE_LINK_MODE_PARAMS(400000, SR8, Full), - __DEFINE_LINK_MODE_PARAMS(400000, LR8_ER8_FR8, Full), - __DEFINE_LINK_MODE_PARAMS(400000, DR8, Full), - __DEFINE_LINK_MODE_PARAMS(400000, CR8, Full), + __DEFINE_LINK_MODE_PARAMS(50000, KR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(50000, SR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(50000, CR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(50000, LR_ER_FR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(50000, DR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(100000, KR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(100000, SR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(100000, CR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(100000, LR2_ER2_FR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(100000, DR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(200000, KR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(200000, SR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(200000, LR4_ER4_FR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(200000, DR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(200000, CR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(100, T1, 1, Full), + __DEFINE_LINK_MODE_PARAMS(1000, T1, 1, Full), + __DEFINE_LINK_MODE_PARAMS(400000, KR8, 8, Full), + __DEFINE_LINK_MODE_PARAMS(400000, SR8, 8, Full), + __DEFINE_LINK_MODE_PARAMS(400000, LR8_ER8_FR8, 8, Full), + __DEFINE_LINK_MODE_PARAMS(400000, DR8, 8, Full), + __DEFINE_LINK_MODE_PARAMS(400000, CR8, 8, Full), __DEFINE_SPECIAL_MODE_PARAMS(FEC_LLRS), - __DEFINE_LINK_MODE_PARAMS(100000, KR, Full), - __DEFINE_LINK_MODE_PARAMS(100000, SR, Full), - __DEFINE_LINK_MODE_PARAMS(100000, LR_ER_FR, Full), - __DEFINE_LINK_MODE_PARAMS(100000, DR, Full), - __DEFINE_LINK_MODE_PARAMS(100000, CR, Full), - __DEFINE_LINK_MODE_PARAMS(200000, KR2, Full), - __DEFINE_LINK_MODE_PARAMS(200000, SR2, Full), - __DEFINE_LINK_MODE_PARAMS(200000, LR2_ER2_FR2, Full), - __DEFINE_LINK_MODE_PARAMS(200000, DR2, Full), - __DEFINE_LINK_MODE_PARAMS(200000, CR2, Full), - __DEFINE_LINK_MODE_PARAMS(400000, KR4, Full), - __DEFINE_LINK_MODE_PARAMS(400000, SR4, Full), - __DEFINE_LINK_MODE_PARAMS(400000, LR4_ER4_FR4, Full), - __DEFINE_LINK_MODE_PARAMS(400000, DR4, Full), - __DEFINE_LINK_MODE_PARAMS(400000, CR4, Full), - __DEFINE_LINK_MODE_PARAMS(100, FX, Half), - __DEFINE_LINK_MODE_PARAMS(100, FX, Full), + __DEFINE_LINK_MODE_PARAMS(100000, KR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(100000, SR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(100000, LR_ER_FR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(100000, DR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(100000, CR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(200000, KR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(200000, SR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(200000, LR2_ER2_FR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(200000, DR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(200000, CR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(400000, KR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(400000, SR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(400000, LR4_ER4_FR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(400000, DR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(400000, CR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(100, FX, 1, Half), + __DEFINE_LINK_MODE_PARAMS(100, FX, 1, Full), }; const struct nla_policy ethnl_linkmodes_set_policy[] = { @@ -274,16 +277,17 @@ const struct nla_policy ethnl_linkmodes_set_policy[] = { [ETHTOOL_A_LINKMODES_SPEED] = { .type = NLA_U32 }, [ETHTOOL_A_LINKMODES_DUPLEX] = { .type = NLA_U8 }, [ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG] = { .type = NLA_U8 }, + [ETHTOOL_A_LINKMODES_LANES] = { .type = NLA_U32 }, }; -/* Set advertised link modes to all supported modes matching requested speed - * and duplex values. Called when autonegotiation is on, speed or duplex is - * requested but no link mode change. This is done in userspace with ioctl() - * interface, move it into kernel for netlink. +/* Set advertised link modes to all supported modes matching requested speed, + * lanes and duplex values. Called when autonegotiation is on, speed, lanes or + * duplex is requested but no link mode change. This is done in userspace with + * ioctl() interface, move it into kernel for netlink. * Returns true if advertised modes bitmap was modified. */ static bool ethnl_auto_linkmodes(struct ethtool_link_ksettings *ksettings, - bool req_speed, bool req_duplex) + bool req_speed, bool req_lanes, bool req_duplex) { unsigned long *advertising = ksettings->link_modes.advertising; unsigned long *supported = ksettings->link_modes.supported; @@ -302,6 +306,7 @@ static bool ethnl_auto_linkmodes(struct ethtool_link_ksettings *ksettings, continue; if (test_bit(i, supported) && (!req_speed || info->speed == ksettings->base.speed) && + (!req_lanes || info->lanes == ksettings->lanes) && (!req_duplex || info->duplex == ksettings->base.duplex)) set_bit(i, advertising); else @@ -325,12 +330,25 @@ static bool ethnl_validate_master_slave_cfg(u8 cfg) return false; } +static bool ethnl_validate_lanes_cfg(u32 cfg) +{ + switch (cfg) { + case ETHTOOL_LANES_1: + case ETHTOOL_LANES_2: + case ETHTOOL_LANES_4: + case ETHTOOL_LANES_8: + return true; + } + + return false; +} + static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb, struct ethtool_link_ksettings *ksettings, - bool *mod) + bool *mod, const struct net_device *dev) { struct ethtool_link_settings *lsettings = &ksettings->base; - bool req_speed, req_duplex; + bool req_speed, req_lanes, req_duplex; const struct nlattr *master_slave_cfg; int ret; @@ -353,10 +371,39 @@ static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb, *mod = false; req_speed = tb[ETHTOOL_A_LINKMODES_SPEED]; + req_lanes = tb[ETHTOOL_A_LINKMODES_LANES]; req_duplex = tb[ETHTOOL_A_LINKMODES_DUPLEX]; ethnl_update_u8(&lsettings->autoneg, tb[ETHTOOL_A_LINKMODES_AUTONEG], mod); + + if (req_lanes) { + u32 lanes_cfg = nla_get_u32(tb[ETHTOOL_A_LINKMODES_LANES]); + + if (!ethnl_validate_lanes_cfg(lanes_cfg)) { + NL_SET_ERR_MSG_ATTR(info->extack, + tb[ETHTOOL_A_LINKMODES_LANES], + "lanes value is invalid"); + return -EINVAL; + } + + /* If autoneg is off and lanes parameter is not supported by the + * driver, return an error. + */ + if (!lsettings->autoneg && + !(dev->ethtool_ops->capabilities & ETHTOOL_CAP_LINK_LANES_SUPPORTED)) { + NL_SET_ERR_MSG_ATTR(info->extack, + tb[ETHTOOL_A_LINKMODES_LANES], + "lanes configuration not supported by device"); + return -EOPNOTSUPP; + } + } else if (!lsettings->autoneg) { + /* If autoneg is off and lanes parameter is not passed from user, + * set the lanes parameter to UNKNOWN. + */ + ksettings->lanes = ETHTOOL_LANES_UNKNOWN; + } + ret = ethnl_update_bitset(ksettings->link_modes.advertising, __ETHTOOL_LINK_MODE_MASK_NBITS, tb[ETHTOOL_A_LINKMODES_OURS], link_mode_names, @@ -365,13 +412,15 @@ static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb, return ret; ethnl_update_u32(&lsettings->speed, tb[ETHTOOL_A_LINKMODES_SPEED], mod); + ethnl_update_u32(&ksettings->lanes, tb[ETHTOOL_A_LINKMODES_LANES], + mod); ethnl_update_u8(&lsettings->duplex, tb[ETHTOOL_A_LINKMODES_DUPLEX], mod); ethnl_update_u8(&lsettings->master_slave_cfg, master_slave_cfg, mod); if (!tb[ETHTOOL_A_LINKMODES_OURS] && lsettings->autoneg && - (req_speed || req_duplex) && - ethnl_auto_linkmodes(ksettings, req_speed, req_duplex)) + (req_speed || req_lanes || req_duplex) && + ethnl_auto_linkmodes(ksettings, req_speed, req_lanes, req_duplex)) *mod = true; return 0; @@ -409,7 +458,7 @@ int ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info) goto out_ops; } - ret = ethnl_update_linkmodes(info, tb, &ksettings, &mod); + ret = ethnl_update_linkmodes(info, tb, &ksettings, &mod, dev); if (ret < 0) goto out_ops; diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h index d8efec516d86..6eabd58d81bf 100644 --- a/net/ethtool/netlink.h +++ b/net/ethtool/netlink.h @@ -351,7 +351,7 @@ extern const struct nla_policy ethnl_strset_get_policy[ETHTOOL_A_STRSET_COUNTS_O extern const struct nla_policy ethnl_linkinfo_get_policy[ETHTOOL_A_LINKINFO_HEADER + 1]; extern const struct nla_policy ethnl_linkinfo_set_policy[ETHTOOL_A_LINKINFO_TP_MDIX_CTRL + 1]; extern const struct nla_policy ethnl_linkmodes_get_policy[ETHTOOL_A_LINKMODES_HEADER + 1]; -extern const struct nla_policy ethnl_linkmodes_set_policy[ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG + 1]; +extern const struct nla_policy ethnl_linkmodes_set_policy[ETHTOOL_A_LINKMODES_LANES + 1]; extern const struct nla_policy ethnl_linkstate_get_policy[ETHTOOL_A_LINKSTATE_HEADER + 1]; extern const struct nla_policy ethnl_debug_get_policy[ETHTOOL_A_DEBUG_HEADER + 1]; extern const struct nla_policy ethnl_debug_set_policy[ETHTOOL_A_DEBUG_MSGMASK + 1]; -- 2.26.2 ^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-10 15:41 ` [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes Ido Schimmel @ 2020-10-11 22:37 ` Jakub Kicinski 2020-10-12 15:33 ` Danielle Ratson 2020-10-12 19:10 ` Johannes Berg 2020-10-12 17:03 ` Michal Kubecek 2020-11-19 20:38 ` Edwin Peer 2 siblings, 2 replies; 45+ messages in thread From: Jakub Kicinski @ 2020-10-11 22:37 UTC (permalink / raw) To: Ido Schimmel Cc: netdev, davem, jiri, danieller, andrew, f.fainelli, mkubecek, mlxsw, Ido Schimmel, johannes On Sat, 10 Oct 2020 18:41:14 +0300 Ido Schimmel wrote: > From: Danielle Ratson <danieller@nvidia.com> > > Currently, when auto negotiation is on, the user can advertise all the > linkmodes which correspond to a specific speed, but does not have a > similar selector for the number of lanes. This is significant when a > specific speed can be achieved using different number of lanes. For > example, 2x50 or 4x25. > > Add 'ETHTOOL_A_LINKMODES_LANES' attribute and expand 'struct > ethtool_link_settings' with lanes field in order to implement a new > lanes-selector that will enable the user to advertise a specific number > of lanes as well. > > When auto negotiation is off, lanes parameter can be forced only if the > driver supports it. Add a capability bit in 'struct ethtool_ops' that > allows ethtool know if the driver can handle the lanes parameter when > auto negotiation is off, so if it does not, an error message will be > returned when trying to set lanes. What's the use for this in practical terms? Isn't the lane count basically implied by the module that gets plugged in? > +/* Lanes, 1, 2, 4 or 8. */ > +#define ETHTOOL_LANES_1 1 > +#define ETHTOOL_LANES_2 2 > +#define ETHTOOL_LANES_4 4 > +#define ETHTOOL_LANES_8 8 Not an extremely useful set of defines, not sure Michal would agree. > +#define ETHTOOL_LANES_UNKNOWN 0 > struct link_mode_info { > int speed; > + int lanes; why signed? > u8 duplex; > }; > @@ -274,16 +277,17 @@ const struct nla_policy ethnl_linkmodes_set_policy[] = { > [ETHTOOL_A_LINKMODES_SPEED] = { .type = NLA_U32 }, > [ETHTOOL_A_LINKMODES_DUPLEX] = { .type = NLA_U8 }, > [ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG] = { .type = NLA_U8 }, > + [ETHTOOL_A_LINKMODES_LANES] = { .type = NLA_U32 }, NLA_POLICY_VALIDATE_FN(), not sure why the types for this validation_type are limited.. Johannes, just an abundance of caution? > + } else if (!lsettings->autoneg) { > + /* If autoneg is off and lanes parameter is not passed from user, > + * set the lanes parameter to UNKNOWN. > + */ > + ksettings->lanes = ETHTOOL_LANES_UNKNOWN; you assume UNKNOWN is zero by doing !lanes in auto_linkmodes - that's inconsistent. > + } > + > ret = ethnl_update_bitset(ksettings->link_modes.advertising, > __ETHTOOL_LINK_MODE_MASK_NBITS, > tb[ETHTOOL_A_LINKMODES_OURS], link_mode_names, ^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-11 22:37 ` Jakub Kicinski @ 2020-10-12 15:33 ` Danielle Ratson 2020-10-12 15:58 ` Jakub Kicinski 2020-10-12 16:40 ` Michal Kubecek 2020-10-12 19:10 ` Johannes Berg 1 sibling, 2 replies; 45+ messages in thread From: Danielle Ratson @ 2020-10-12 15:33 UTC (permalink / raw) To: Jakub Kicinski, Ido Schimmel Cc: netdev, davem, Jiri Pirko, andrew, f.fainelli, mkubecek, mlxsw, Ido Schimmel, johannes > -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Monday, October 12, 2020 1:38 AM > To: Ido Schimmel <idosch@idosch.org> > Cc: netdev@vger.kernel.org; davem@davemloft.net; Jiri Pirko > <jiri@nvidia.com>; Danielle Ratson <danieller@nvidia.com>; > andrew@lunn.ch; f.fainelli@gmail.com; mkubecek@suse.cz; mlxsw > <mlxsw@nvidia.com>; Ido Schimmel <idosch@nvidia.com>; > johannes@sipsolutions.net > Subject: Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI > with lanes > > On Sat, 10 Oct 2020 18:41:14 +0300 Ido Schimmel wrote: > > From: Danielle Ratson <danieller@nvidia.com> > > > > Currently, when auto negotiation is on, the user can advertise all the > > linkmodes which correspond to a specific speed, but does not have a > > similar selector for the number of lanes. This is significant when a > > specific speed can be achieved using different number of lanes. For > > example, 2x50 or 4x25. > > > > Add 'ETHTOOL_A_LINKMODES_LANES' attribute and expand 'struct > > ethtool_link_settings' with lanes field in order to implement a new > > lanes-selector that will enable the user to advertise a specific > > number of lanes as well. > > > > When auto negotiation is off, lanes parameter can be forced only if > > the driver supports it. Add a capability bit in 'struct ethtool_ops' > > that allows ethtool know if the driver can handle the lanes parameter > > when auto negotiation is off, so if it does not, an error message will > > be returned when trying to set lanes. > > What's the use for this in practical terms? Isn't the lane count basically > implied by the module that gets plugged in? The use is to enable the user to decide how to achieve a certain speed. For example, if he wants to get 100G and the port has 4 lanes, the speed can be achieved it using both 2 lanes of 50G and 4 lanes of 25G, as a port with 4 lanes width can work in 2 lanes mode with double speed each. So, by specifying "lanes 2" he will achieve 100G using 2 lanes of 50G. > > > +/* Lanes, 1, 2, 4 or 8. */ > > +#define ETHTOOL_LANES_1 1 > > +#define ETHTOOL_LANES_2 2 > > +#define ETHTOOL_LANES_4 4 > > +#define ETHTOOL_LANES_8 8 > > Not an extremely useful set of defines, not sure Michal would agree. > > > +#define ETHTOOL_LANES_UNKNOWN 0 > > > struct link_mode_info { > > int speed; > > + int lanes; > > why signed? I have aligned it to the speed. > > > u8 duplex; > > }; > > > @@ -274,16 +277,17 @@ const struct nla_policy > ethnl_linkmodes_set_policy[] = { > > [ETHTOOL_A_LINKMODES_SPEED] = { .type = NLA_U32 }, > > [ETHTOOL_A_LINKMODES_DUPLEX] = { .type = NLA_U8 }, > > [ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG] = { .type = > NLA_U8 }, > > + [ETHTOOL_A_LINKMODES_LANES] = { .type = NLA_U32 }, > > NLA_POLICY_VALIDATE_FN(), not sure why the types for this > validation_type are limited.. Johannes, just an abundance of caution? > > > + } else if (!lsettings->autoneg) { > > + /* If autoneg is off and lanes parameter is not passed from > user, > > + * set the lanes parameter to UNKNOWN. > > + */ > > + ksettings->lanes = ETHTOOL_LANES_UNKNOWN; > > you assume UNKNOWN is zero by doing !lanes in auto_linkmodes - that's > inconsistent. I didn't do !lanes... maybe you mean !req_lanes which is different parameter that specifies if the lanes parameter was passed from user space. > > > + } > > + > > ret = ethnl_update_bitset(ksettings->link_modes.advertising, > > __ETHTOOL_LINK_MODE_MASK_NBITS, > > tb[ETHTOOL_A_LINKMODES_OURS], > link_mode_names, ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-12 15:33 ` Danielle Ratson @ 2020-10-12 15:58 ` Jakub Kicinski 2020-10-13 14:29 ` Danielle Ratson 2020-10-12 16:40 ` Michal Kubecek 1 sibling, 1 reply; 45+ messages in thread From: Jakub Kicinski @ 2020-10-12 15:58 UTC (permalink / raw) To: Danielle Ratson Cc: Ido Schimmel, netdev, davem, Jiri Pirko, andrew, f.fainelli, mkubecek, mlxsw, Ido Schimmel, johannes On Mon, 12 Oct 2020 15:33:45 +0000 Danielle Ratson wrote: > > What's the use for this in practical terms? Isn't the lane count basically > > implied by the module that gets plugged in? > > The use is to enable the user to decide how to achieve a certain speed. > For example, if he wants to get 100G and the port has 4 lanes, the > speed can be achieved it using both 2 lanes of 50G and 4 lanes of > 25G, as a port with 4 lanes width can work in 2 lanes mode with > double speed each. So, by specifying "lanes 2" he will achieve 100G > using 2 lanes of 50G. Can you give a concrete example of serdes capabilities of the port, what SFP gets plugged in and what configuration user wants to select? ^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-12 15:58 ` Jakub Kicinski @ 2020-10-13 14:29 ` Danielle Ratson 2020-10-13 15:43 ` Jakub Kicinski 2020-10-16 22:15 ` Andrew Lunn 0 siblings, 2 replies; 45+ messages in thread From: Danielle Ratson @ 2020-10-13 14:29 UTC (permalink / raw) To: Jakub Kicinski Cc: Ido Schimmel, netdev, davem, Jiri Pirko, andrew, f.fainelli, mkubecek, mlxsw, Ido Schimmel, johannes > -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Monday, October 12, 2020 6:58 PM > To: Danielle Ratson <danieller@nvidia.com> > Cc: Ido Schimmel <idosch@idosch.org>; netdev@vger.kernel.org; > davem@davemloft.net; Jiri Pirko <jiri@nvidia.com>; andrew@lunn.ch; > f.fainelli@gmail.com; mkubecek@suse.cz; mlxsw <mlxsw@nvidia.com>; Ido > Schimmel <idosch@nvidia.com>; johannes@sipsolutions.net > Subject: Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI > with lanes > > On Mon, 12 Oct 2020 15:33:45 +0000 Danielle Ratson wrote: > > > What's the use for this in practical terms? Isn't the lane count > > > basically implied by the module that gets plugged in? > > > > The use is to enable the user to decide how to achieve a certain speed. > > For example, if he wants to get 100G and the port has 4 lanes, the > > speed can be achieved it using both 2 lanes of 50G and 4 lanes of 25G, > > as a port with 4 lanes width can work in 2 lanes mode with double > > speed each. So, by specifying "lanes 2" he will achieve 100G using 2 > > lanes of 50G. > > Can you give a concrete example of serdes capabilities of the port, what SFP > gets plugged in and what configuration user wants to select? Example: - swp1 is a 200G port with 4 lanes. - QSFP28 is plugged in. - The user wants to select configuration of 100G speed using 2 lanes, 50G each. $ ethtool swp1 Settings for swp1: Supported ports: [ FIBRE Backplane ] Supported link modes: 1000baseT/Full 10000baseT/Full 1000baseKX/Full 10000baseKR/Full 10000baseR_FEC 40000baseKR4/Full 40000baseCR4/Full 40000baseSR4/Full 40000baseLR4/Full 25000baseCR/Full 25000baseKR/Full 25000baseSR/Full 50000baseCR2/Full 50000baseKR2/Full 100000baseKR4/Full 100000baseSR4/Full 100000baseCR4/Full 100000baseLR4_ER4/Full 50000baseSR2/Full 10000baseCR/Full 10000baseSR/Full 10000baseLR/Full 10000baseER/Full 50000baseKR/Full 50000baseSR/Full 50000baseCR/Full 50000baseLR_ER_FR/Full 50000baseDR/Full 100000baseKR2/Full 100000baseSR2/Full 100000baseCR2/Full 100000baseLR2_ER2_FR2/Full 100000baseDR2/Full 200000baseKR4/Full 200000baseSR4/Full 200000baseLR4_ER4_FR4/Full 200000baseDR4/Full 200000baseCR4/Full Advertised link modes: 1000baseT/Full 10000baseT/Full 1000baseKX/Full 10000baseKR/Full 10000baseR_FEC 40000baseKR4/Full 40000baseCR4/Full 40000baseSR4/Full 40000baseLR4/Full 25000baseCR/Full 25000baseKR/Full 25000baseSR/Full 50000baseCR2/Full 50000baseKR2/Full 100000baseKR4/Full 100000baseSR4/Full 100000baseCR4/Full 100000baseLR4_ER4/Full 50000baseSR2/Full 10000baseCR/Full 10000baseSR/Full 10000baseLR/Full 10000baseER/Full 50000baseKR/Full 50000baseSR/Full 50000baseCR/Full 50000baseLR_ER_FR/Full 50000baseDR/Full 100000baseKR2/Full 100000baseSR2/Full 100000baseCR2/Full 100000baseLR2_ER2_FR2/Full 100000baseDR2/Full 200000baseKR4/Full 200000baseSR4/Full 200000baseLR4_ER4_FR4/Full 200000baseDR4/Full 200000baseCR4/Full Advertised pause frame use: No Advertised auto-negotiation: Yes Advertised FEC modes: Not reported Speed: Unknown! Lanes: Unknown! Duplex: Unknown! (255) Auto-negotiation: on Port: Direct Attach Copper PHYAD: 0 Transceiver: internal Link detected: no $ ethtool -s swp1 speed 100000 lanes 2 $ ethtool swp1 Settings for swp1: Supported ports: [ FIBRE Backplane ] Supported link modes: 1000baseT/Full 10000baseT/Full 1000baseKX/Full 10000baseKR/Full 10000baseR_FEC 40000baseKR4/Full 40000baseCR4/Full 40000baseSR4/Full 40000baseLR4/Full 25000baseCR/Full 25000baseKR/Full 25000baseSR/Full 50000baseCR2/Full 50000baseKR2/Full 100000baseKR4/Full 100000baseSR4/Full 100000baseCR4/Full 100000baseLR4_ER4/Full 50000baseSR2/Full 10000baseCR/Full 10000baseSR/Full 10000baseLR/Full 10000baseER/Full 50000baseKR/Full 50000baseSR/Full 50000baseCR/Full 50000baseLR_ER_FR/Full 50000baseDR/Full 100000baseKR2/Full 100000baseSR2/Full 100000baseCR2/Full 100000baseLR2_ER2_FR2/Full 100000baseDR2/Full 200000baseKR4/Full 200000baseSR4/Full 200000baseLR4_ER4_FR4/Full 200000baseDR4/Full 200000baseCR4/Full Supported pause frame use: Symmetric Receive-only Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: 100000baseKR2/Full 100000baseSR2/Full 100000baseCR2/Full 100000baseLR2_ER2_FR2/Full 100000baseDR2/Full Advertised pause frame use: No Advertised auto-negotiation: Yes Advertised FEC modes: Not reported ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-13 14:29 ` Danielle Ratson @ 2020-10-13 15:43 ` Jakub Kicinski 2020-10-16 22:15 ` Andrew Lunn 1 sibling, 0 replies; 45+ messages in thread From: Jakub Kicinski @ 2020-10-13 15:43 UTC (permalink / raw) To: Danielle Ratson Cc: Ido Schimmel, netdev, davem, Jiri Pirko, andrew, f.fainelli, mkubecek, mlxsw, Ido Schimmel, johannes On Tue, 13 Oct 2020 14:29:29 +0000 Danielle Ratson wrote: > > On Mon, 12 Oct 2020 15:33:45 +0000 Danielle Ratson wrote: > > > > What's the use for this in practical terms? Isn't the lane count > > > > basically implied by the module that gets plugged in? > > > > > > The use is to enable the user to decide how to achieve a certain speed. > > > For example, if he wants to get 100G and the port has 4 lanes, the > > > speed can be achieved it using both 2 lanes of 50G and 4 lanes of 25G, > > > as a port with 4 lanes width can work in 2 lanes mode with double > > > speed each. So, by specifying "lanes 2" he will achieve 100G using 2 > > > lanes of 50G. > > > > Can you give a concrete example of serdes capabilities of the port, what SFP > > gets plugged in and what configuration user wants to select? > > Example: > - swp1 is a 200G port with 4 lanes. > - QSFP28 is plugged in. > - The user wants to select configuration of 100G speed using 2 lanes, 50G each. I mean.. sounds quite contrived to me. But I'm not opposed if others are fine with the change. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-13 14:29 ` Danielle Ratson 2020-10-13 15:43 ` Jakub Kicinski @ 2020-10-16 22:15 ` Andrew Lunn 2020-10-19 7:19 ` Danielle Ratson 2020-10-19 12:24 ` Jiri Pirko 1 sibling, 2 replies; 45+ messages in thread From: Andrew Lunn @ 2020-10-16 22:15 UTC (permalink / raw) To: Danielle Ratson Cc: Jakub Kicinski, Ido Schimmel, netdev, davem, Jiri Pirko, f.fainelli, mkubecek, mlxsw, Ido Schimmel, johannes > Example: > - swp1 is a 200G port with 4 lanes. > - QSFP28 is plugged in. > - The user wants to select configuration of 100G speed using 2 lanes, 50G each. > > $ ethtool swp1 > Settings for swp1: > Supported ports: [ FIBRE Backplane ] > Supported link modes: 1000baseT/Full > 10000baseT/Full > 1000baseKX/Full > 10000baseKR/Full > 10000baseR_FEC > 40000baseKR4/Full > 40000baseCR4/Full > 40000baseSR4/Full > 40000baseLR4/Full > 25000baseCR/Full > 25000baseKR/Full > 25000baseSR/Full > 50000baseCR2/Full > 50000baseKR2/Full > 100000baseKR4/Full > 100000baseSR4/Full > 100000baseCR4/Full > 100000baseLR4_ER4/Full > 50000baseSR2/Full > 10000baseCR/Full > 10000baseSR/Full > 10000baseLR/Full > 10000baseER/Full > 50000baseKR/Full > 50000baseSR/Full > 50000baseCR/Full > 50000baseLR_ER_FR/Full > 50000baseDR/Full > 100000baseKR2/Full > 100000baseSR2/Full > 100000baseCR2/Full > 100000baseLR2_ER2_FR2/Full > 100000baseDR2/Full I'm not sure i fully understand all these different link modes, but i thought these 5 are all 100G using 2 lanes? So why cannot the user simply do ethtool -s swp1 advertise 100000baseKR2/Full and the driver can figure out it needs to use two lanes at 50G? Andrew ^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-16 22:15 ` Andrew Lunn @ 2020-10-19 7:19 ` Danielle Ratson 2020-10-19 11:04 ` Michal Kubecek 2020-10-19 12:24 ` Jiri Pirko 1 sibling, 1 reply; 45+ messages in thread From: Danielle Ratson @ 2020-10-19 7:19 UTC (permalink / raw) To: Andrew Lunn Cc: Jakub Kicinski, Ido Schimmel, netdev, davem, Jiri Pirko, f.fainelli, mkubecek, mlxsw, Ido Schimmel, johannes > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Saturday, October 17, 2020 1:16 AM > To: Danielle Ratson <danieller@nvidia.com> > Cc: Jakub Kicinski <kuba@kernel.org>; Ido Schimmel <idosch@idosch.org>; > netdev@vger.kernel.org; davem@davemloft.net; Jiri Pirko > <jiri@nvidia.com>; f.fainelli@gmail.com; mkubecek@suse.cz; mlxsw > <mlxsw@nvidia.com>; Ido Schimmel <idosch@nvidia.com>; > johannes@sipsolutions.net > Subject: Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI > with lanes > > > Example: > > - swp1 is a 200G port with 4 lanes. > > - QSFP28 is plugged in. > > - The user wants to select configuration of 100G speed using 2 lanes, 50G > each. > > > > $ ethtool swp1 > > Settings for swp1: > > Supported ports: [ FIBRE Backplane ] > > Supported link modes: 1000baseT/Full > > 10000baseT/Full > > 1000baseKX/Full > > 10000baseKR/Full > > 10000baseR_FEC > > 40000baseKR4/Full > > 40000baseCR4/Full > > 40000baseSR4/Full > > 40000baseLR4/Full > > 25000baseCR/Full > > 25000baseKR/Full > > 25000baseSR/Full > > 50000baseCR2/Full > > 50000baseKR2/Full > > 100000baseKR4/Full > > 100000baseSR4/Full > > 100000baseCR4/Full > > 100000baseLR4_ER4/Full > > 50000baseSR2/Full > > 10000baseCR/Full > > 10000baseSR/Full > > 10000baseLR/Full > > 10000baseER/Full > > 50000baseKR/Full > > 50000baseSR/Full > > 50000baseCR/Full > > 50000baseLR_ER_FR/Full > > 50000baseDR/Full > > > 100000baseKR2/Full > > 100000baseSR2/Full > > 100000baseCR2/Full > > 100000baseLR2_ER2_FR2/Full > > 100000baseDR2/Full > > I'm not sure i fully understand all these different link modes, but i thought > these 5 are all 100G using 2 lanes? So why cannot the user simply do > > ethtool -s swp1 advertise 100000baseKR2/Full > > and the driver can figure out it needs to use two lanes at 50G? > > Andrew Hi Andrew, Thanks for the feedback. I guess you mean " ethtool -s swp1 advertise 100000baseKR2/Full on". First, the idea might work but only for auto negotiation mode, whereas the lanes parameter is a solution for both. Second, the command as you have suggested it, wouldn't change anything in the current situation as I checked. We can disable all the others and leave only the one we want but the command doesn't filter the other link modes but it just turns the mentioned link modes up if they aren't. However, the lanes parameter is a selector, which make it much more user friendly in my opinion. Also, we can't turn only one of them up. But you have to set for example: $ ethtool -s swp1 advertise 100000baseKR2/Full on 100000baseSR2/Full on 100000baseCR2/Full on 100000baseLR2_ER2_FR2/Full on 100000baseDR2/Full on Am I missing something? ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-19 7:19 ` Danielle Ratson @ 2020-10-19 11:04 ` Michal Kubecek 2020-10-19 12:26 ` Jiri Pirko 0 siblings, 1 reply; 45+ messages in thread From: Michal Kubecek @ 2020-10-19 11:04 UTC (permalink / raw) To: Danielle Ratson Cc: Andrew Lunn, Jakub Kicinski, Ido Schimmel, netdev, davem, Jiri Pirko, f.fainelli, mlxsw, Ido Schimmel, johannes On Mon, Oct 19, 2020 at 07:19:34AM +0000, Danielle Ratson wrote: > > -----Original Message----- > > From: Andrew Lunn <andrew@lunn.ch> > > Sent: Saturday, October 17, 2020 1:16 AM > > > > I'm not sure i fully understand all these different link modes, but > > i thought these 5 are all 100G using 2 lanes? So why cannot the user > > simply do > > > > ethtool -s swp1 advertise 100000baseKR2/Full > > > > and the driver can figure out it needs to use two lanes at 50G? > > > > Andrew > > Hi Andrew, > > Thanks for the feedback. > > I guess you mean " ethtool -s swp1 advertise 100000baseKR2/Full on". > > First, the idea might work but only for auto negotiation mode, whereas > the lanes parameter is a solution for both. > > Second, the command as you have suggested it, wouldn't change anything > in the current situation as I checked. We can disable all the others > and leave only the one we want but the command doesn't filter the > other link modes but it just turns the mentioned link modes up if they > aren't. However, the lanes parameter is a selector, which make it much > more user friendly in my opinion. It would be quite easy to extend the ethtool command line parser to allow also ethtool -s <dev> advertise <mode> ... in addition to already supported ethtool -s <dev> advertise <mask> ethtool -s <dev> advertise <mask>/<mask> ethtool -s { <mode> { on | off } } ... Parser converting simple list of values into a maskless bitset is already there so it would be only matter of checking if there are at least two arguments and second is "on" or "off" and using corresponding parser. I think it would be useful independently of this series. > Also, we can't turn only one of them up. But you have to set for > example: > > $ ethtool -s swp1 advertise 100000baseKR2/Full on 100000baseSR2/Full on 100000baseCR2/Full on 100000baseLR2_ER2_FR2/Full on 100000baseDR2/Full on > > Am I missing something? IIUC Jakub's concern is rather about real life need for such selectors, i.e. how realistic is "I want a(ny) 100Gb/s mode with two lanes" as an actual user need; if it wouldn't be mostly (or only) used as a quick way to distinguish between two supported 100Gb/s modes. IMHO if we go this way, we should consider going all the way, i.e. allow also selecting by the remaining part of the mode ("media type", e.g. "LR", not sure what the official name is) and, more important, get full information about link mode in use from driver (we only get speed and duplex at the moment). But that would require changes in the get_linksettings() interface and drivers. Michal ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-19 11:04 ` Michal Kubecek @ 2020-10-19 12:26 ` Jiri Pirko 2020-10-19 13:24 ` Michal Kubecek 0 siblings, 1 reply; 45+ messages in thread From: Jiri Pirko @ 2020-10-19 12:26 UTC (permalink / raw) To: Michal Kubecek Cc: Danielle Ratson, Andrew Lunn, Jakub Kicinski, Ido Schimmel, netdev, davem, Jiri Pirko, f.fainelli, mlxsw, Ido Schimmel, johannes Mon, Oct 19, 2020 at 01:04:22PM CEST, mkubecek@suse.cz wrote: >On Mon, Oct 19, 2020 at 07:19:34AM +0000, Danielle Ratson wrote: >> > -----Original Message----- >> > From: Andrew Lunn <andrew@lunn.ch> >> > Sent: Saturday, October 17, 2020 1:16 AM >> > >> > I'm not sure i fully understand all these different link modes, but >> > i thought these 5 are all 100G using 2 lanes? So why cannot the user >> > simply do >> > >> > ethtool -s swp1 advertise 100000baseKR2/Full >> > >> > and the driver can figure out it needs to use two lanes at 50G? >> > >> > Andrew >> >> Hi Andrew, >> >> Thanks for the feedback. >> >> I guess you mean " ethtool -s swp1 advertise 100000baseKR2/Full on". >> >> First, the idea might work but only for auto negotiation mode, whereas >> the lanes parameter is a solution for both. >> >> Second, the command as you have suggested it, wouldn't change anything >> in the current situation as I checked. We can disable all the others >> and leave only the one we want but the command doesn't filter the >> other link modes but it just turns the mentioned link modes up if they >> aren't. However, the lanes parameter is a selector, which make it much >> more user friendly in my opinion. > >It would be quite easy to extend the ethtool command line parser to >allow also > > ethtool -s <dev> advertise <mode> ... > >in addition to already supported > > ethtool -s <dev> advertise <mask> > ethtool -s <dev> advertise <mask>/<mask> > ethtool -s { <mode> { on | off } } ... > >Parser converting simple list of values into a maskless bitset is >already there so it would be only matter of checking if there are at >least two arguments and second is "on" or "off" and using corresponding >parser. I think it would be useful independently of this series. Understood. So basically you will pass some selectors on cmdline and the uapi would stay intact. How do you imagine the specific lane number selection should look like on the cmdline? > >> Also, we can't turn only one of them up. But you have to set for >> example: >> >> $ ethtool -s swp1 advertise 100000baseKR2/Full on 100000baseSR2/Full on 100000baseCR2/Full on 100000baseLR2_ER2_FR2/Full on 100000baseDR2/Full on >> >> Am I missing something? > >IIUC Jakub's concern is rather about real life need for such selectors, >i.e. how realistic is "I want a(ny) 100Gb/s mode with two lanes" as an >actual user need; if it wouldn't be mostly (or only) used as a quick way >to distinguish between two supported 100Gb/s modes. > >IMHO if we go this way, we should consider going all the way, i.e. allow >also selecting by the remaining part of the mode ("media type", e.g. >"LR", not sure what the official name is) and, more important, get full >information about link mode in use from driver (we only get speed and >duplex at the moment). But that would require changes in the >get_linksettings() interface and drivers. > >Michal ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-19 12:26 ` Jiri Pirko @ 2020-10-19 13:24 ` Michal Kubecek 2020-10-20 7:39 ` Danielle Ratson 0 siblings, 1 reply; 45+ messages in thread From: Michal Kubecek @ 2020-10-19 13:24 UTC (permalink / raw) To: Jiri Pirko Cc: Danielle Ratson, Andrew Lunn, Jakub Kicinski, Ido Schimmel, netdev, davem, Jiri Pirko, f.fainelli, mlxsw, Ido Schimmel, johannes On Mon, Oct 19, 2020 at 02:26:43PM +0200, Jiri Pirko wrote: > Mon, Oct 19, 2020 at 01:04:22PM CEST, mkubecek@suse.cz wrote: > > > >It would be quite easy to extend the ethtool command line parser to > >allow also > > > > ethtool -s <dev> advertise <mode> ... > > > >in addition to already supported > > > > ethtool -s <dev> advertise <mask> > > ethtool -s <dev> advertise <mask>/<mask> > > ethtool -s { <mode> { on | off } } ... This should have been ethtool -s <dev> advertise { <mode> { on | off } } ... > >Parser converting simple list of values into a maskless bitset is > >already there so it would be only matter of checking if there are at > >least two arguments and second is "on" or "off" and using corresponding > >parser. I think it would be useful independently of this series. > > Understood. So basically you will pass some selectors on cmdline and the > uapi would stay intact. > How do you imagine the specific lane number selection should look like > on the cmdline? As I said, I meant the extension suggested in my mail as independent of what this series is about. For lanes count selector, I find proposed ethtool -s <dev> ... lanes <lanes_num> ... the most natural. From purely syntactic/semantic point of view, there are three types of requests: (1) enable specific set of modes, disable the rest (2) enable/disable specific modes, leave the rest as they are (3) enable modes matching a condition (and disable the rest) What I proposed was to allow the use symbolic names instead of masks (which are getting more and more awful with each new mode) also for (1), like they can already be used for (2). The lanes selector is an extension of (3) which I would prefer not to mix with (1) or (2) within one command line, i.e. either "advertise" or "speed / duplex / lanes". IIUC Jakub's and Andrew's comments were not so much about the syntax and semantic (which is quite clear) but rather about the question if the requests like "advertise exactly the modes with (100Gb/s speed and) two lanes" would really address a real life need and wouldn't be more often used as shortcuts for "advertise 100000baseKR2/Full". (On the other hand, I suspect existing speed and duplex selectors are often used the same way.) Michal ^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-19 13:24 ` Michal Kubecek @ 2020-10-20 7:39 ` Danielle Ratson 2020-10-21 7:08 ` Michal Kubecek 0 siblings, 1 reply; 45+ messages in thread From: Danielle Ratson @ 2020-10-20 7:39 UTC (permalink / raw) To: Michal Kubecek, Jiri Pirko Cc: Andrew Lunn, Jakub Kicinski, Ido Schimmel, netdev, davem, Jiri Pirko, f.fainelli, mlxsw, Ido Schimmel, johannes > -----Original Message----- > From: Michal Kubecek <mkubecek@suse.cz> > Sent: Monday, October 19, 2020 4:25 PM > To: Jiri Pirko <jiri@resnulli.us> > Cc: Danielle Ratson <danieller@nvidia.com>; Andrew Lunn > <andrew@lunn.ch>; Jakub Kicinski <kuba@kernel.org>; Ido Schimmel > <idosch@idosch.org>; netdev@vger.kernel.org; davem@davemloft.net; Jiri > Pirko <jiri@nvidia.com>; f.fainelli@gmail.com; mlxsw <mlxsw@nvidia.com>; > Ido Schimmel <idosch@nvidia.com>; johannes@sipsolutions.net > Subject: Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI > with lanes > > On Mon, Oct 19, 2020 at 02:26:43PM +0200, Jiri Pirko wrote: > > Mon, Oct 19, 2020 at 01:04:22PM CEST, mkubecek@suse.cz wrote: > > > > > >It would be quite easy to extend the ethtool command line parser to > > >allow also > > > > > > ethtool -s <dev> advertise <mode> ... > > > > > >in addition to already supported > > > > > > ethtool -s <dev> advertise <mask> > > > ethtool -s <dev> advertise <mask>/<mask> ethtool -s { <mode> { on > > > | off } } ... > > This should have been > > ethtool -s <dev> advertise { <mode> { on | off } } ... > > > >Parser converting simple list of values into a maskless bitset is > > >already there so it would be only matter of checking if there are at > > >least two arguments and second is "on" or "off" and using > > >corresponding parser. I think it would be useful independently of this > series. > > > > Understood. So basically you will pass some selectors on cmdline and > > the uapi would stay intact. > > How do you imagine the specific lane number selection should look like > > on the cmdline? > > As I said, I meant the extension suggested in my mail as independent of what > this series is about. For lanes count selector, I find proposed > > ethtool -s <dev> ... lanes <lanes_num> ... > > the most natural. > > From purely syntactic/semantic point of view, there are three types of > requests: > > (1) enable specific set of modes, disable the rest > (2) enable/disable specific modes, leave the rest as they are > (3) enable modes matching a condition (and disable the rest) > > What I proposed was to allow the use symbolic names instead of masks > (which are getting more and more awful with each new mode) also for (1), > like they can already be used for (2). > > The lanes selector is an extension of (3) which I would prefer not to mix with > (1) or (2) within one command line, i.e. either "advertise" or "speed / duplex > / lanes". > > IIUC Jakub's and Andrew's comments were not so much about the syntax > and semantic (which is quite clear) but rather about the question if the > requests like "advertise exactly the modes with (100Gb/s speed and) two > lanes" would really address a real life need and wouldn't be more often used > as shortcuts for "advertise 100000baseKR2/Full". (On the other hand, I > suspect existing speed and duplex selectors are often used the same way.) > > Michal So, do you want to change the current approach somehow or we are good to go with this one, keeping in mind the future extension you have suggested? Thanks. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-20 7:39 ` Danielle Ratson @ 2020-10-21 7:08 ` Michal Kubecek 2020-10-21 7:20 ` Danielle Ratson 0 siblings, 1 reply; 45+ messages in thread From: Michal Kubecek @ 2020-10-21 7:08 UTC (permalink / raw) To: Danielle Ratson Cc: Jiri Pirko, Andrew Lunn, Jakub Kicinski, Ido Schimmel, netdev, davem, Jiri Pirko, f.fainelli, mlxsw, Ido Schimmel, johannes On Tue, Oct 20, 2020 at 07:39:13AM +0000, Danielle Ratson wrote: > > -----Original Message----- > > From: Michal Kubecek <mkubecek@suse.cz> > > Sent: Monday, October 19, 2020 4:25 PM > > > > As I said, I meant the extension suggested in my mail as independent of what > > this series is about. For lanes count selector, I find proposed > > > > ethtool -s <dev> ... lanes <lanes_num> ... > > > > the most natural. > > > > From purely syntactic/semantic point of view, there are three types of > > requests: > > > > (1) enable specific set of modes, disable the rest > > (2) enable/disable specific modes, leave the rest as they are > > (3) enable modes matching a condition (and disable the rest) > > > > What I proposed was to allow the use symbolic names instead of masks > > (which are getting more and more awful with each new mode) also for (1), > > like they can already be used for (2). > > > > The lanes selector is an extension of (3) which I would prefer not to mix with > > (1) or (2) within one command line, i.e. either "advertise" or "speed / duplex > > / lanes". > > > > IIUC Jakub's and Andrew's comments were not so much about the syntax > > and semantic (which is quite clear) but rather about the question if the > > requests like "advertise exactly the modes with (100Gb/s speed and) two > > lanes" would really address a real life need and wouldn't be more often used > > as shortcuts for "advertise 100000baseKR2/Full". (On the other hand, I > > suspect existing speed and duplex selectors are often used the same way.) > > > > Michal > > So, do you want to change the current approach somehow or we are good > to go with this one, keeping in mind the future extension you have > suggested? As far as I'm concerned, it makes sense as it is. The only thing I'm not happy about is ETHTOOL_A_LINKMODES_LANES being a "write only" attribute (unlike _SPEED and _DUPLEX) but being able to query this information would require extensive changes far beyond the scope of this series. Michal ^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-21 7:08 ` Michal Kubecek @ 2020-10-21 7:20 ` Danielle Ratson 2020-10-21 8:47 ` Michal Kubecek 0 siblings, 1 reply; 45+ messages in thread From: Danielle Ratson @ 2020-10-21 7:20 UTC (permalink / raw) To: Michal Kubecek Cc: Jiri Pirko, Andrew Lunn, Jakub Kicinski, Ido Schimmel, netdev, davem, Jiri Pirko, f.fainelli, mlxsw, Ido Schimmel, johannes > -----Original Message----- > From: Michal Kubecek <mkubecek@suse.cz> > Sent: Wednesday, October 21, 2020 10:08 AM > To: Danielle Ratson <danieller@nvidia.com> > Cc: Jiri Pirko <jiri@resnulli.us>; Andrew Lunn <andrew@lunn.ch>; Jakub > Kicinski <kuba@kernel.org>; Ido Schimmel <idosch@idosch.org>; > netdev@vger.kernel.org; davem@davemloft.net; Jiri Pirko > <jiri@nvidia.com>; f.fainelli@gmail.com; mlxsw <mlxsw@nvidia.com>; Ido > Schimmel <idosch@nvidia.com>; johannes@sipsolutions.net > Subject: Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI > with lanes > > On Tue, Oct 20, 2020 at 07:39:13AM +0000, Danielle Ratson wrote: > > > -----Original Message----- > > > From: Michal Kubecek <mkubecek@suse.cz> > > > Sent: Monday, October 19, 2020 4:25 PM > > > > > > As I said, I meant the extension suggested in my mail as independent > > > of what this series is about. For lanes count selector, I find > > > proposed > > > > > > ethtool -s <dev> ... lanes <lanes_num> ... > > > > > > the most natural. > > > > > > From purely syntactic/semantic point of view, there are three types > > > of > > > requests: > > > > > > (1) enable specific set of modes, disable the rest > > > (2) enable/disable specific modes, leave the rest as they are > > > (3) enable modes matching a condition (and disable the rest) > > > > > > What I proposed was to allow the use symbolic names instead of masks > > > (which are getting more and more awful with each new mode) also for > > > (1), like they can already be used for (2). > > > > > > The lanes selector is an extension of (3) which I would prefer not > > > to mix with > > > (1) or (2) within one command line, i.e. either "advertise" or > > > "speed / duplex / lanes". > > > > > > IIUC Jakub's and Andrew's comments were not so much about the syntax > > > and semantic (which is quite clear) but rather about the question if > > > the requests like "advertise exactly the modes with (100Gb/s speed > > > and) two lanes" would really address a real life need and wouldn't > > > be more often used as shortcuts for "advertise 100000baseKR2/Full". > > > (On the other hand, I suspect existing speed and duplex selectors > > > are often used the same way.) > > > > > > Michal > > > > So, do you want to change the current approach somehow or we are good > > to go with this one, keeping in mind the future extension you have > > suggested? > > As far as I'm concerned, it makes sense as it is. The only thing I'm not happy > about is ETHTOOL_A_LINKMODES_LANES being a "write only" attribute > (unlike _SPEED and _DUPLEX) but being able to query this information would > require extensive changes far beyond the scope of this series. > > Michal If I understood you correctly, patch #5 does that query, isn't it? "mlxsw: ethtool: Expose the number of lanes in use" ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-21 7:20 ` Danielle Ratson @ 2020-10-21 8:47 ` Michal Kubecek 2020-10-22 6:15 ` Danielle Ratson 0 siblings, 1 reply; 45+ messages in thread From: Michal Kubecek @ 2020-10-21 8:47 UTC (permalink / raw) To: Danielle Ratson Cc: Jiri Pirko, Andrew Lunn, Jakub Kicinski, Ido Schimmel, netdev, davem, Jiri Pirko, f.fainelli, mlxsw, Ido Schimmel, johannes On Wed, Oct 21, 2020 at 07:20:22AM +0000, Danielle Ratson wrote: > > -----Original Message----- > > From: Michal Kubecek <mkubecek@suse.cz> > > Sent: Wednesday, October 21, 2020 10:08 AM > > > > On Tue, Oct 20, 2020 at 07:39:13AM +0000, Danielle Ratson wrote: > > > > -----Original Message----- > > > > From: Michal Kubecek <mkubecek@suse.cz> > > > > Sent: Monday, October 19, 2020 4:25 PM > > > > > > > > As I said, I meant the extension suggested in my mail as independent > > > > of what this series is about. For lanes count selector, I find > > > > proposed > > > > > > > > ethtool -s <dev> ... lanes <lanes_num> ... > > > > > > > > the most natural. > > > > > > > > From purely syntactic/semantic point of view, there are three types > > > > of > > > > requests: > > > > > > > > (1) enable specific set of modes, disable the rest > > > > (2) enable/disable specific modes, leave the rest as they are > > > > (3) enable modes matching a condition (and disable the rest) > > > > > > > > What I proposed was to allow the use symbolic names instead of masks > > > > (which are getting more and more awful with each new mode) also for > > > > (1), like they can already be used for (2). > > > > > > > > The lanes selector is an extension of (3) which I would prefer not > > > > to mix with > > > > (1) or (2) within one command line, i.e. either "advertise" or > > > > "speed / duplex / lanes". > > > > > > > > IIUC Jakub's and Andrew's comments were not so much about the syntax > > > > and semantic (which is quite clear) but rather about the question if > > > > the requests like "advertise exactly the modes with (100Gb/s speed > > > > and) two lanes" would really address a real life need and wouldn't > > > > be more often used as shortcuts for "advertise 100000baseKR2/Full". > > > > (On the other hand, I suspect existing speed and duplex selectors > > > > are often used the same way.) > > > > > > > > Michal > > > > > > So, do you want to change the current approach somehow or we are good > > > to go with this one, keeping in mind the future extension you have > > > suggested? > > > > As far as I'm concerned, it makes sense as it is. The only thing I'm not happy > > about is ETHTOOL_A_LINKMODES_LANES being a "write only" attribute > > (unlike _SPEED and _DUPLEX) but being able to query this information would > > require extensive changes far beyond the scope of this series. > > If I understood you correctly, patch #5 does that query, isn't it? > "mlxsw: ethtool: Expose the number of lanes in use" Ah, right, it does. But as you extend struct ethtool_link_ksettings and drivers will need to be updated to provide this information, wouldn't it be more useful to let the driver provide link mode in use instead (and derive number of lanes from it)? Michal ^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-21 8:47 ` Michal Kubecek @ 2020-10-22 6:15 ` Danielle Ratson 2020-10-22 16:27 ` Michal Kubecek 0 siblings, 1 reply; 45+ messages in thread From: Danielle Ratson @ 2020-10-22 6:15 UTC (permalink / raw) To: Michal Kubecek Cc: Jiri Pirko, Andrew Lunn, Jakub Kicinski, Ido Schimmel, netdev, davem, Jiri Pirko, f.fainelli, mlxsw, Ido Schimmel, johannes > -----Original Message----- > From: Michal Kubecek <mkubecek@suse.cz> > Sent: Wednesday, October 21, 2020 11:48 AM > To: Danielle Ratson <danieller@nvidia.com> > Cc: Jiri Pirko <jiri@resnulli.us>; Andrew Lunn <andrew@lunn.ch>; Jakub > Kicinski <kuba@kernel.org>; Ido Schimmel <idosch@idosch.org>; > netdev@vger.kernel.org; davem@davemloft.net; Jiri Pirko > <jiri@nvidia.com>; f.fainelli@gmail.com; mlxsw <mlxsw@nvidia.com>; Ido > Schimmel <idosch@nvidia.com>; johannes@sipsolutions.net > Subject: Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI > with lanes > > On Wed, Oct 21, 2020 at 07:20:22AM +0000, Danielle Ratson wrote: > > > -----Original Message----- > > > From: Michal Kubecek <mkubecek@suse.cz> > > > Sent: Wednesday, October 21, 2020 10:08 AM > > > > > > On Tue, Oct 20, 2020 at 07:39:13AM +0000, Danielle Ratson wrote: > > > > > -----Original Message----- > > > > > From: Michal Kubecek <mkubecek@suse.cz> > > > > > Sent: Monday, October 19, 2020 4:25 PM > > > > > > > > > > As I said, I meant the extension suggested in my mail as > > > > > independent of what this series is about. For lanes count > > > > > selector, I find proposed > > > > > > > > > > ethtool -s <dev> ... lanes <lanes_num> ... > > > > > > > > > > the most natural. > > > > > > > > > > From purely syntactic/semantic point of view, there are three > > > > > types of > > > > > requests: > > > > > > > > > > (1) enable specific set of modes, disable the rest > > > > > (2) enable/disable specific modes, leave the rest as they are > > > > > (3) enable modes matching a condition (and disable the rest) > > > > > > > > > > What I proposed was to allow the use symbolic names instead of > > > > > masks (which are getting more and more awful with each new mode) > > > > > also for (1), like they can already be used for (2). > > > > > > > > > > The lanes selector is an extension of (3) which I would prefer > > > > > not to mix with > > > > > (1) or (2) within one command line, i.e. either "advertise" or > > > > > "speed / duplex / lanes". > > > > > > > > > > IIUC Jakub's and Andrew's comments were not so much about the > > > > > syntax and semantic (which is quite clear) but rather about the > > > > > question if the requests like "advertise exactly the modes with > > > > > (100Gb/s speed > > > > > and) two lanes" would really address a real life need and > > > > > wouldn't be more often used as shortcuts for "advertise > 100000baseKR2/Full". > > > > > (On the other hand, I suspect existing speed and duplex > > > > > selectors are often used the same way.) > > > > > > > > > > Michal > > > > > > > > So, do you want to change the current approach somehow or we are > > > > good to go with this one, keeping in mind the future extension you > > > > have suggested? > > > > > > As far as I'm concerned, it makes sense as it is. The only thing I'm > > > not happy about is ETHTOOL_A_LINKMODES_LANES being a "write only" > > > attribute (unlike _SPEED and _DUPLEX) but being able to query this > > > information would require extensive changes far beyond the scope of > this series. > > > > If I understood you correctly, patch #5 does that query, isn't it? > > "mlxsw: ethtool: Expose the number of lanes in use" > > Ah, right, it does. But as you extend struct ethtool_link_ksettings and drivers > will need to be updated to provide this information, wouldn't it be more > useful to let the driver provide link mode in use instead (and derive number > of lanes from it)? > > Michal This is the way it is done with the speed parameter, so I have aligned it to it. Why the lanes should be done differently comparing to the speed? ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-22 6:15 ` Danielle Ratson @ 2020-10-22 16:27 ` Michal Kubecek 2020-11-23 9:47 ` Danielle Ratson 0 siblings, 1 reply; 45+ messages in thread From: Michal Kubecek @ 2020-10-22 16:27 UTC (permalink / raw) To: Danielle Ratson Cc: Jiri Pirko, Andrew Lunn, Jakub Kicinski, Ido Schimmel, netdev, davem, Jiri Pirko, f.fainelli, mlxsw, Ido Schimmel, johannes On Thu, Oct 22, 2020 at 06:15:48AM +0000, Danielle Ratson wrote: > > -----Original Message----- > > From: Michal Kubecek <mkubecek@suse.cz> > > Sent: Wednesday, October 21, 2020 11:48 AM > > > > Ah, right, it does. But as you extend struct ethtool_link_ksettings > > and drivers will need to be updated to provide this information, > > wouldn't it be more useful to let the driver provide link mode in > > use instead (and derive number of lanes from it)? > > This is the way it is done with the speed parameter, so I have aligned > it to it. Why the lanes should be done differently comparing to the > speed? Speed and duplex have worked this way since ages and the interface was probably introduced back in times when combination of speed and duplex was sufficient to identify the link mode. This is no longer the case and even adding number of lanes wouldn't make the combination unique. So if we are going to extend the interface now and update drivers to provide extra information, I believe it would be more useful to provide full information. Michal ^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-22 16:27 ` Michal Kubecek @ 2020-11-23 9:47 ` Danielle Ratson 2020-11-24 22:12 ` Michal Kubecek 0 siblings, 1 reply; 45+ messages in thread From: Danielle Ratson @ 2020-11-23 9:47 UTC (permalink / raw) To: Michal Kubecek Cc: Jiri Pirko, Andrew Lunn, Jakub Kicinski, Ido Schimmel, netdev, davem, Jiri Pirko, f.fainelli, mlxsw, Ido Schimmel, johannes > -----Original Message----- > From: Michal Kubecek <mkubecek@suse.cz> > Sent: Thursday, October 22, 2020 7:28 PM > To: Danielle Ratson <danieller@nvidia.com> > Cc: Jiri Pirko <jiri@resnulli.us>; Andrew Lunn <andrew@lunn.ch>; Jakub Kicinski <kuba@kernel.org>; Ido Schimmel > <idosch@idosch.org>; netdev@vger.kernel.org; davem@davemloft.net; Jiri Pirko <jiri@nvidia.com>; f.fainelli@gmail.com; mlxsw > <mlxsw@nvidia.com>; Ido Schimmel <idosch@nvidia.com>; johannes@sipsolutions.net > Subject: Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes > > On Thu, Oct 22, 2020 at 06:15:48AM +0000, Danielle Ratson wrote: > > > -----Original Message----- > > > From: Michal Kubecek <mkubecek@suse.cz> > > > Sent: Wednesday, October 21, 2020 11:48 AM > > > > > > Ah, right, it does. But as you extend struct ethtool_link_ksettings > > > and drivers will need to be updated to provide this information, > > > wouldn't it be more useful to let the driver provide link mode in > > > use instead (and derive number of lanes from it)? > > > > This is the way it is done with the speed parameter, so I have aligned > > it to it. Why the lanes should be done differently comparing to the > > speed? > > Speed and duplex have worked this way since ages and the interface was probably introduced back in times when combination of > speed and duplex was sufficient to identify the link mode. This is no longer the case and even adding number of lanes wouldn't make > the combination unique. So if we are going to extend the interface now and update drivers to provide extra information, I believe it > would be more useful to provide full information. > > Michal Hi Michal, What do you think of passing the link modes you have suggested as a bitmask, similar to "supported", that contains only one positive bit? Something like that: diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index afae2beacbc3..dd946c88daa3 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -127,6 +127,7 @@ struct ethtool_link_ksettings { __ETHTOOL_DECLARE_LINK_MODE_MASK(supported); __ETHTOOL_DECLARE_LINK_MODE_MASK(advertising); __ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising); + __ETHTOOL_DECLARE_LINK_MODE_MASK(chosen); } link_modes; u32 lanes; }; Do you have perhaps a better suggestion? And the speed and duplex parameters should be removed from being passed like as well, right? Thanks, Danielle ^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-11-23 9:47 ` Danielle Ratson @ 2020-11-24 22:12 ` Michal Kubecek 2020-11-25 10:35 ` Danielle Ratson 0 siblings, 1 reply; 45+ messages in thread From: Michal Kubecek @ 2020-11-24 22:12 UTC (permalink / raw) To: Danielle Ratson Cc: Jiri Pirko, Andrew Lunn, Jakub Kicinski, Ido Schimmel, netdev, davem, Jiri Pirko, f.fainelli, mlxsw, Ido Schimmel, johannes On Mon, Nov 23, 2020 at 09:47:53AM +0000, Danielle Ratson wrote: > > > > -----Original Message----- > > From: Michal Kubecek <mkubecek@suse.cz> > > Sent: Thursday, October 22, 2020 7:28 PM > > To: Danielle Ratson <danieller@nvidia.com> > > Cc: Jiri Pirko <jiri@resnulli.us>; Andrew Lunn <andrew@lunn.ch>; Jakub Kicinski <kuba@kernel.org>; Ido Schimmel > > <idosch@idosch.org>; netdev@vger.kernel.org; davem@davemloft.net; Jiri Pirko <jiri@nvidia.com>; f.fainelli@gmail.com; mlxsw > > <mlxsw@nvidia.com>; Ido Schimmel <idosch@nvidia.com>; johannes@sipsolutions.net > > Subject: Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes > > > > On Thu, Oct 22, 2020 at 06:15:48AM +0000, Danielle Ratson wrote: > > > > -----Original Message----- > > > > From: Michal Kubecek <mkubecek@suse.cz> > > > > Sent: Wednesday, October 21, 2020 11:48 AM > > > > > > > > Ah, right, it does. But as you extend struct ethtool_link_ksettings > > > > and drivers will need to be updated to provide this information, > > > > wouldn't it be more useful to let the driver provide link mode in > > > > use instead (and derive number of lanes from it)? > > > > > > This is the way it is done with the speed parameter, so I have aligned > > > it to it. Why the lanes should be done differently comparing to the > > > speed? > > > > Speed and duplex have worked this way since ages and the interface was probably introduced back in times when combination of > > speed and duplex was sufficient to identify the link mode. This is no longer the case and even adding number of lanes wouldn't make > > the combination unique. So if we are going to extend the interface now and update drivers to provide extra information, I believe it > > would be more useful to provide full information. > > > > Michal > > Hi Michal, > > What do you think of passing the link modes you have suggested as a > bitmask, similar to "supported", that contains only one positive bit? > Something like that: > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index afae2beacbc3..dd946c88daa3 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -127,6 +127,7 @@ struct ethtool_link_ksettings { > __ETHTOOL_DECLARE_LINK_MODE_MASK(supported); > __ETHTOOL_DECLARE_LINK_MODE_MASK(advertising); > __ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising); > + __ETHTOOL_DECLARE_LINK_MODE_MASK(chosen); > } link_modes; > u32 lanes; > }; > > Do you have perhaps a better suggestion? Not sure if it's better but as we know there is only one mode, we could simply pass the index. We would still need to reserve a special value for none/unknown but getting an index would make lookup easier. > And the speed and duplex parameters should be removed from being > passed like as well, right? We cannot remove them from struct ethtool_link_settings and the ioctl and netlink messages as those are part of UAPI and we have to preserve backward compatibility. But drivers which provide link mode would not need to fill speed and duplex in their ->get_link_ksettings() as the common code could do that for them. Michal ^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-11-24 22:12 ` Michal Kubecek @ 2020-11-25 10:35 ` Danielle Ratson 2020-11-26 21:07 ` Michal Kubecek 0 siblings, 1 reply; 45+ messages in thread From: Danielle Ratson @ 2020-11-25 10:35 UTC (permalink / raw) To: Michal Kubecek Cc: Jiri Pirko, Andrew Lunn, Jakub Kicinski, Ido Schimmel, netdev, davem, Jiri Pirko, f.fainelli, mlxsw, Ido Schimmel, johannes > -----Original Message----- > From: Michal Kubecek <mkubecek@suse.cz> > Sent: Wednesday, November 25, 2020 12:12 AM > To: Danielle Ratson <danieller@nvidia.com> > Cc: Jiri Pirko <jiri@resnulli.us>; Andrew Lunn <andrew@lunn.ch>; Jakub Kicinski <kuba@kernel.org>; Ido Schimmel > <idosch@idosch.org>; netdev@vger.kernel.org; davem@davemloft.net; Jiri Pirko <jiri@nvidia.com>; f.fainelli@gmail.com; mlxsw > <mlxsw@nvidia.com>; Ido Schimmel <idosch@nvidia.com>; johannes@sipsolutions.net > Subject: Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes > > On Mon, Nov 23, 2020 at 09:47:53AM +0000, Danielle Ratson wrote: > > > > > > > -----Original Message----- > > > From: Michal Kubecek <mkubecek@suse.cz> > > > Sent: Thursday, October 22, 2020 7:28 PM > > > To: Danielle Ratson <danieller@nvidia.com> > > > Cc: Jiri Pirko <jiri@resnulli.us>; Andrew Lunn <andrew@lunn.ch>; > > > Jakub Kicinski <kuba@kernel.org>; Ido Schimmel <idosch@idosch.org>; > > > netdev@vger.kernel.org; davem@davemloft.net; Jiri Pirko > > > <jiri@nvidia.com>; f.fainelli@gmail.com; mlxsw <mlxsw@nvidia.com>; > > > Ido Schimmel <idosch@nvidia.com>; johannes@sipsolutions.net > > > Subject: Re: [PATCH net-next 1/6] ethtool: Extend link modes > > > settings uAPI with lanes > > > > > > On Thu, Oct 22, 2020 at 06:15:48AM +0000, Danielle Ratson wrote: > > > > > -----Original Message----- > > > > > From: Michal Kubecek <mkubecek@suse.cz> > > > > > Sent: Wednesday, October 21, 2020 11:48 AM > > > > > > > > > > Ah, right, it does. But as you extend struct > > > > > ethtool_link_ksettings and drivers will need to be updated to > > > > > provide this information, wouldn't it be more useful to let the > > > > > driver provide link mode in use instead (and derive number of lanes from it)? > > > > > > > > This is the way it is done with the speed parameter, so I have > > > > aligned it to it. Why the lanes should be done differently > > > > comparing to the speed? > > > > > > Speed and duplex have worked this way since ages and the interface > > > was probably introduced back in times when combination of speed and > > > duplex was sufficient to identify the link mode. This is no longer > > > the case and even adding number of lanes wouldn't make the combination unique. So if we are going to extend the interface now > and update drivers to provide extra information, I believe it would be more useful to provide full information. > > > > > > Michal > > > > Hi Michal, > > > > What do you think of passing the link modes you have suggested as a > > bitmask, similar to "supported", that contains only one positive bit? > > Something like that: Hi Michal, Thanks for your response. Actually what I said is not very accurate. In ethtool, for speed 100G and 4 lanes for example, there are few link modes that fits: ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT ETHTOOL_LINK_MODE_100000baseSR4_Full_BIT ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT The difference is the media. And in the driver we shrink into one bit. But maybe that makes passing a bitmask more sense, or am I missing something? > > > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index > > afae2beacbc3..dd946c88daa3 100644 > > --- a/include/linux/ethtool.h > > +++ b/include/linux/ethtool.h > > @@ -127,6 +127,7 @@ struct ethtool_link_ksettings { > > __ETHTOOL_DECLARE_LINK_MODE_MASK(supported); > > __ETHTOOL_DECLARE_LINK_MODE_MASK(advertising); > > __ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising); > > + __ETHTOOL_DECLARE_LINK_MODE_MASK(chosen); > > } link_modes; > > u32 lanes; > > }; > > > > Do you have perhaps a better suggestion? > > Not sure if it's better but as we know there is only one mode, we could simply pass the index. We would still need to reserve a special > value for none/unknown but getting an index would make lookup easier. > > > And the speed and duplex parameters should be removed from being > > passed like as well, right? > > We cannot remove them from struct ethtool_link_settings and the ioctl and netlink messages as those are part of UAPI and we have > to preserve backward compatibility. But drivers which provide link mode would not need to fill speed and duplex in their - > >get_link_ksettings() as the common code could do that for them. Yes of course I didn't mean to remove the parameters from the struct, just to not prepare them for passing to ethtool when getting the link settings. Thanks. > > Michal ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-11-25 10:35 ` Danielle Ratson @ 2020-11-26 21:07 ` Michal Kubecek 2020-12-01 17:22 ` Danielle Ratson 0 siblings, 1 reply; 45+ messages in thread From: Michal Kubecek @ 2020-11-26 21:07 UTC (permalink / raw) To: Danielle Ratson Cc: Jiri Pirko, Andrew Lunn, Jakub Kicinski, Ido Schimmel, netdev, davem, Jiri Pirko, f.fainelli, mlxsw, Ido Schimmel, johannes On Wed, Nov 25, 2020 at 10:35:35AM +0000, Danielle Ratson wrote: > > > What do you think of passing the link modes you have suggested as a > > > bitmask, similar to "supported", that contains only one positive bit? > > > Something like that: > > Hi Michal, > > Thanks for your response. > > Actually what I said is not very accurate. > In ethtool, for speed 100G and 4 lanes for example, there are few link modes that fits: > ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT > ETHTOOL_LINK_MODE_100000baseSR4_Full_BIT > ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT > ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT > > The difference is the media. And in the driver we shrink into one bit. > But maybe that makes passing a bitmask more sense, or am I missing something? But as far as I understand, at any moment, only one of these will be actually in use so that's what the driver should report. Or is the problem that the driver cannot identify the media in use? (To be precise: by "cannot identify" I mean "it's not possible for the driver to find out", not "current code does not distinguish".) Michal ^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-11-26 21:07 ` Michal Kubecek @ 2020-12-01 17:22 ` Danielle Ratson 2020-12-02 0:52 ` Edwin Peer 2020-12-02 1:17 ` Edwin Peer 0 siblings, 2 replies; 45+ messages in thread From: Danielle Ratson @ 2020-12-01 17:22 UTC (permalink / raw) To: Michal Kubecek Cc: Jiri Pirko, Andrew Lunn, Jakub Kicinski, Ido Schimmel, netdev, davem, Jiri Pirko, f.fainelli, mlxsw, Ido Schimmel, johannes > -----Original Message----- > From: Michal Kubecek <mkubecek@suse.cz> > Sent: Thursday, November 26, 2020 11:08 PM > To: Danielle Ratson <danieller@nvidia.com> > Cc: Jiri Pirko <jiri@resnulli.us>; Andrew Lunn <andrew@lunn.ch>; Jakub Kicinski <kuba@kernel.org>; Ido Schimmel > <idosch@idosch.org>; netdev@vger.kernel.org; davem@davemloft.net; Jiri Pirko <jiri@nvidia.com>; f.fainelli@gmail.com; mlxsw > <mlxsw@nvidia.com>; Ido Schimmel <idosch@nvidia.com>; johannes@sipsolutions.net > Subject: Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes > > On Wed, Nov 25, 2020 at 10:35:35AM +0000, Danielle Ratson wrote: > > > > What do you think of passing the link modes you have suggested as > > > > a bitmask, similar to "supported", that contains only one positive bit? > > > > Something like that: > > > > Hi Michal, > > > > Thanks for your response. > > > > Actually what I said is not very accurate. > > In ethtool, for speed 100G and 4 lanes for example, there are few link modes that fits: > > ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT > > ETHTOOL_LINK_MODE_100000baseSR4_Full_BIT > > ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT > > ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT > > > > The difference is the media. And in the driver we shrink into one bit. > > But maybe that makes passing a bitmask more sense, or am I missing something? > > But as far as I understand, at any moment, only one of these will be actually in use so that's what the driver should report. Or is the > problem that the driver cannot identify the media in use? (To be > precise: by "cannot identify" I mean "it's not possible for the driver to find out", not "current code does not distinguish".) > > Michal After more investigation, those are my conclusions: We have two types of supported asics in the driver- one of them is able to distinguish between the medias and the other one doesn't. So in the first type I can send one bit as you requested from the driver to ethtool but in the other one I can't. The suggestions I have are: 1. Add a bit that for unknown media for each link (something like ETHTOOL_LINK_MODE_100000unknown_Full_BIT). I am not sure it is even possible or makes sense. 2. Pass the link mode as bitmap. Do you see any other option? Thanks. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-12-01 17:22 ` Danielle Ratson @ 2020-12-02 0:52 ` Edwin Peer 2020-12-02 1:17 ` Edwin Peer 1 sibling, 0 replies; 45+ messages in thread From: Edwin Peer @ 2020-12-02 0:52 UTC (permalink / raw) To: Danielle Ratson Cc: Michal Kubecek, Jiri Pirko, Andrew Lunn, Jakub Kicinski, Ido Schimmel, netdev, davem, Jiri Pirko, f.fainelli, mlxsw, Ido Schimmel, johannes [-- Attachment #1: Type: text/plain, Size: 1585 bytes --] On Wed, Nov 25, 2020 at 10:35:35AM +0000, Danielle Ratson wrote: > > > In ethtool, for speed 100G and 4 lanes for example, there are few link modes that fits: > > > ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT > > > ETHTOOL_LINK_MODE_100000baseSR4_Full_BIT > > > ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT > > > ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT > > > > The suggestions I have are: > 1. Add a bit that for unknown media for each link (something like > ETHTOOL_LINK_MODE_100000unknown_Full_BIT). I am not sure it is even > possible or makes sense. The number of lanes would still need to be specified. You would need at least: ETHTOOL_LINK_MODE_100000xR2_FULL and ETHTOOL_LINK_MODE_100000xR4_FULL to distinguish between PAM4 and NRZ at 100G respectively. And, there's still the cost of maintaining a different enum to ethtool_link_mode_bit_indices. > 2. Pass the link mode as bitmap. The user only wants to specify link speed and encoding, not media. The bitmap has the same problem to solve. Or, should user space set the bits for all possible media types that satisfy the desired speed and encoding? Eeek. Alternatively, the driver would need to accept any bit that implies the desired speed and encoding, ignoring what media the bit specifies (to do so would require maintaining a map of equivalences, yuck). > Do you see any other option? As stated in another sub-thread, I think the encoding can be implied by the speed if the number of lanes is a property of the port configuration. In which case the existing ethtool interface is sufficient. Regards, Edwin Peer [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 4160 bytes --] ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-12-01 17:22 ` Danielle Ratson 2020-12-02 0:52 ` Edwin Peer @ 2020-12-02 1:17 ` Edwin Peer 1 sibling, 0 replies; 45+ messages in thread From: Edwin Peer @ 2020-12-02 1:17 UTC (permalink / raw) To: Danielle Ratson Cc: Michal Kubecek, Jiri Pirko, Andrew Lunn, Jakub Kicinski, Ido Schimmel, netdev, davem, Jiri Pirko, f.fainelli, mlxsw, Ido Schimmel, johannes [-- Attachment #1: Type: text/plain, Size: 1153 bytes --] On Tue, Dec 1, 2020 at 9:23 AM Danielle Ratson <danieller@nvidia.com> wrote: > The suggestions I have are: > 1. Add a bit that for unknown media for each link (something like ETHTOOL_LINK_MODE_100000unknown_Full_BIT). I am not sure it is even possible or makes sense. > 2. Pass the link mode as bitmap. > > Do you see any other option? Other options include: 1) Let the user specify link encoding directly, as we do for FEC. This would imply the number of lanes used at a given speed rather than the other way around. Then you only need a handful of indices in the enum. 2) Instead of an enum, encode the semantics into the link mode bit representation. N bits define the speed. Other bits define the encoding, number of lanes, media, etc. This would solve the problem of decoding the intent from a single user selected link mode index, but would require a v2 UABI. The user could have the same strings, but not much more. And then, it's probably better to give each dimension their own field in a struct and be done, lest we run out of bits. Let the user space tool map the link mode string onto the appropriate struct values. Regards, Edwin Peer [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 4160 bytes --] ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-16 22:15 ` Andrew Lunn 2020-10-19 7:19 ` Danielle Ratson @ 2020-10-19 12:24 ` Jiri Pirko 2020-10-19 12:38 ` Andrew Lunn 1 sibling, 1 reply; 45+ messages in thread From: Jiri Pirko @ 2020-10-19 12:24 UTC (permalink / raw) To: Andrew Lunn Cc: Danielle Ratson, Jakub Kicinski, Ido Schimmel, netdev, davem, Jiri Pirko, f.fainelli, mkubecek, mlxsw, Ido Schimmel, johannes Sat, Oct 17, 2020 at 12:15:53AM CEST, andrew@lunn.ch wrote: >> Example: >> - swp1 is a 200G port with 4 lanes. >> - QSFP28 is plugged in. >> - The user wants to select configuration of 100G speed using 2 lanes, 50G each. >> >> $ ethtool swp1 >> Settings for swp1: >> Supported ports: [ FIBRE Backplane ] >> Supported link modes: 1000baseT/Full >> 10000baseT/Full >> 1000baseKX/Full >> 10000baseKR/Full >> 10000baseR_FEC >> 40000baseKR4/Full >> 40000baseCR4/Full >> 40000baseSR4/Full >> 40000baseLR4/Full >> 25000baseCR/Full >> 25000baseKR/Full >> 25000baseSR/Full >> 50000baseCR2/Full >> 50000baseKR2/Full >> 100000baseKR4/Full >> 100000baseSR4/Full >> 100000baseCR4/Full >> 100000baseLR4_ER4/Full >> 50000baseSR2/Full >> 10000baseCR/Full >> 10000baseSR/Full >> 10000baseLR/Full >> 10000baseER/Full >> 50000baseKR/Full >> 50000baseSR/Full >> 50000baseCR/Full >> 50000baseLR_ER_FR/Full >> 50000baseDR/Full > >> 100000baseKR2/Full >> 100000baseSR2/Full >> 100000baseCR2/Full >> 100000baseLR2_ER2_FR2/Full >> 100000baseDR2/Full > >I'm not sure i fully understand all these different link modes, but i >thought these 5 are all 100G using 2 lanes? So why cannot the user >simply do > >ethtool -s swp1 advertise 100000baseKR2/Full > >and the driver can figure out it needs to use two lanes at 50G? 100000baseKR2 is 2 lanes. No need to figure anything out. What do you mean by that? > > Andrew ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-19 12:24 ` Jiri Pirko @ 2020-10-19 12:38 ` Andrew Lunn 0 siblings, 0 replies; 45+ messages in thread From: Andrew Lunn @ 2020-10-19 12:38 UTC (permalink / raw) To: Jiri Pirko Cc: Danielle Ratson, Jakub Kicinski, Ido Schimmel, netdev, davem, Jiri Pirko, f.fainelli, mkubecek, mlxsw, Ido Schimmel, johannes > >> 100000baseKR2/Full > >> 100000baseSR2/Full > >> 100000baseCR2/Full > >> 100000baseLR2_ER2_FR2/Full > >> 100000baseDR2/Full > > > >I'm not sure i fully understand all these different link modes, but i > >thought these 5 are all 100G using 2 lanes? So why cannot the user > >simply do > > > >ethtool -s swp1 advertise 100000baseKR2/Full > > > >and the driver can figure out it needs to use two lanes at 50G? > > 100000baseKR2 is 2 lanes. No need to figure anything out. What do you > mean by that? I'm just thinking, rather than add a new UAPI for the number of lanes, why not use one we already have, if the number of lanes is implied by the link mode. Andrew ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-12 15:33 ` Danielle Ratson 2020-10-12 15:58 ` Jakub Kicinski @ 2020-10-12 16:40 ` Michal Kubecek 1 sibling, 0 replies; 45+ messages in thread From: Michal Kubecek @ 2020-10-12 16:40 UTC (permalink / raw) To: Danielle Ratson Cc: Jakub Kicinski, Ido Schimmel, netdev, davem, Jiri Pirko, andrew, f.fainelli, mlxsw, Ido Schimmel, johannes On Mon, Oct 12, 2020 at 03:33:45PM +0000, Danielle Ratson wrote: > > > > > +/* Lanes, 1, 2, 4 or 8. */ > > > +#define ETHTOOL_LANES_1 1 > > > +#define ETHTOOL_LANES_2 2 > > > +#define ETHTOOL_LANES_4 4 > > > +#define ETHTOOL_LANES_8 8 > > > > Not an extremely useful set of defines, not sure Michal would agree. I don't see much use for them either. Such defines or enums make sense when the numbers represent some values which have symbolic names but these values only represent number of lanes so a number is IMHO sufficient. > > > +#define ETHTOOL_LANES_UNKNOWN 0 > > > > > struct link_mode_info { > > > int speed; > > > + int lanes; > > > > why signed? > > I have aligned it to the speed. For speed, signed type was used mostly because SPEED_UNKNOWN is defined as -1 (even if it's cast to u32 in most places), I don't think there is a reason to use a signed type. Michal ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-11 22:37 ` Jakub Kicinski 2020-10-12 15:33 ` Danielle Ratson @ 2020-10-12 19:10 ` Johannes Berg 2020-10-12 20:08 ` Jakub Kicinski 1 sibling, 1 reply; 45+ messages in thread From: Johannes Berg @ 2020-10-12 19:10 UTC (permalink / raw) To: Jakub Kicinski, Ido Schimmel Cc: netdev, davem, jiri, danieller, andrew, f.fainelli, mkubecek, mlxsw, Ido Schimmel Hi, Sorry, somehow didn't see this until now. > > +/* Lanes, 1, 2, 4 or 8. */ > > +#define ETHTOOL_LANES_1 1 > > +#define ETHTOOL_LANES_2 2 > > +#define ETHTOOL_LANES_4 4 > > +#define ETHTOOL_LANES_8 8 > > Not an extremely useful set of defines, not sure Michal would agree. > > > +#define ETHTOOL_LANES_UNKNOWN 0 > > struct link_mode_info { > > int speed; > > + int lanes; > > why signed? > > > u8 duplex; > > }; > > @@ -274,16 +277,17 @@ const struct nla_policy ethnl_linkmodes_set_policy[] = { > > [ETHTOOL_A_LINKMODES_SPEED] = { .type = NLA_U32 }, > > [ETHTOOL_A_LINKMODES_DUPLEX] = { .type = NLA_U8 }, > > [ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG] = { .type = NLA_U8 }, > > + [ETHTOOL_A_LINKMODES_LANES] = { .type = NLA_U32 }, > > NLA_POLICY_VALIDATE_FN(), not sure why the types for this > validation_type are limited.. Johannes, just an abundance of caution? So let me see if I got this right - you're saying you'd like to use NLA_POLICY_VALIDATE_FN() for an NLA_U32, to validate against the _LANES being 1, 2, 4 or 8? First of all, you _can_, no? I mean, it's limited by #define NLA_ENSURE_NO_VALIDATION_PTR(tp) \ (__NLA_ENSURE(tp != NLA_BITFIELD32 && \ tp != NLA_REJECT && \ tp != NLA_NESTED && \ tp != NLA_NESTED_ARRAY) + tp) and the reason is sort of encoded in that - the types listed here already use the pointer *regardless of the validation_type*, so you can't have a pointer to the function in the same union. But not sure I understood :-) johannes ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-12 19:10 ` Johannes Berg @ 2020-10-12 20:08 ` Jakub Kicinski 0 siblings, 0 replies; 45+ messages in thread From: Jakub Kicinski @ 2020-10-12 20:08 UTC (permalink / raw) To: Johannes Berg Cc: Ido Schimmel, netdev, davem, jiri, danieller, andrew, f.fainelli, mkubecek, mlxsw, Ido Schimmel On Mon, 12 Oct 2020 21:10:53 +0200 Johannes Berg wrote: > Hi, > > Sorry, somehow didn't see this until now. > > > > +/* Lanes, 1, 2, 4 or 8. */ > > > +#define ETHTOOL_LANES_1 1 > > > +#define ETHTOOL_LANES_2 2 > > > +#define ETHTOOL_LANES_4 4 > > > +#define ETHTOOL_LANES_8 8 > > > > Not an extremely useful set of defines, not sure Michal would agree. > > > > > +#define ETHTOOL_LANES_UNKNOWN 0 > > > struct link_mode_info { > > > int speed; > > > + int lanes; > > > > why signed? > > > > > u8 duplex; > > > }; > > > @@ -274,16 +277,17 @@ const struct nla_policy ethnl_linkmodes_set_policy[] = { > > > [ETHTOOL_A_LINKMODES_SPEED] = { .type = NLA_U32 }, > > > [ETHTOOL_A_LINKMODES_DUPLEX] = { .type = NLA_U8 }, > > > [ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG] = { .type = NLA_U8 }, > > > + [ETHTOOL_A_LINKMODES_LANES] = { .type = NLA_U32 }, > > > > NLA_POLICY_VALIDATE_FN(), not sure why the types for this > > validation_type are limited.. Johannes, just an abundance of caution? > > So let me see if I got this right - you're saying you'd like to use > NLA_POLICY_VALIDATE_FN() for an NLA_U32, to validate against the _LANES > being 1, 2, 4 or 8? > > First of all, you _can_, no? I mean, it's limited by > > #define NLA_ENSURE_NO_VALIDATION_PTR(tp) \ > (__NLA_ENSURE(tp != NLA_BITFIELD32 && \ > tp != NLA_REJECT && \ > tp != NLA_NESTED && \ > tp != NLA_NESTED_ARRAY) + tp) > > and the reason is sort of encoded in that - the types listed here > already use the pointer *regardless of the validation_type*, so you > can't have a pointer to the function in the same union. > > But not sure I understood :-) Yes, you're right. Sorry for the noise, one day I'll learn to read.. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-10 15:41 ` [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes Ido Schimmel 2020-10-11 22:37 ` Jakub Kicinski @ 2020-10-12 17:03 ` Michal Kubecek 2020-11-19 20:38 ` Edwin Peer 2 siblings, 0 replies; 45+ messages in thread From: Michal Kubecek @ 2020-10-12 17:03 UTC (permalink / raw) To: Ido Schimmel Cc: netdev, davem, kuba, jiri, danieller, andrew, f.fainelli, mlxsw, Ido Schimmel On Sat, Oct 10, 2020 at 06:41:14PM +0300, Ido Schimmel wrote: > From: Danielle Ratson <danieller@nvidia.com> > > Currently, when auto negotiation is on, the user can advertise all the > linkmodes which correspond to a specific speed, but does not have a > similar selector for the number of lanes. This is significant when a > specific speed can be achieved using different number of lanes. For > example, 2x50 or 4x25. > > Add 'ETHTOOL_A_LINKMODES_LANES' attribute and expand 'struct > ethtool_link_settings' with lanes field in order to implement a new > lanes-selector that will enable the user to advertise a specific number > of lanes as well. > > When auto negotiation is off, lanes parameter can be forced only if the > driver supports it. Add a capability bit in 'struct ethtool_ops' that > allows ethtool know if the driver can handle the lanes parameter when > auto negotiation is off, so if it does not, an error message will be > returned when trying to set lanes. > > Example: > > $ ethtool -s swp1 lanes 4 > $ ethtool swp1 > Settings for swp1: > Supported ports: [ FIBRE ] > Supported link modes: 1000baseKX/Full > 10000baseKR/Full > 40000baseCR4/Full > 40000baseSR4/Full > 40000baseLR4/Full > 25000baseCR/Full > 25000baseSR/Full > 50000baseCR2/Full > 100000baseSR4/Full > 100000baseCR4/Full > Supported pause frame use: Symmetric Receive-only > Supports auto-negotiation: Yes > Supported FEC modes: Not reported > Advertised link modes: 40000baseCR4/Full > 40000baseSR4/Full > 40000baseLR4/Full > 100000baseSR4/Full > 100000baseCR4/Full > Advertised pause frame use: No > Advertised auto-negotiation: Yes > Advertised FEC modes: Not reported > Speed: Unknown! > Duplex: Unknown! (255) > Auto-negotiation: on > Port: Direct Attach Copper > PHYAD: 0 > Transceiver: internal > Link detected: no > > Signed-off-by: Danielle Ratson <danieller@nvidia.com> > Reviewed-by: Jiri Pirko <jiri@nvidia.com> > Signed-off-by: Ido Schimmel <idosch@nvidia.com> > --- [...] > static const struct link_mode_info link_mode_params[] = { > - __DEFINE_LINK_MODE_PARAMS(10, T, Half), > - __DEFINE_LINK_MODE_PARAMS(10, T, Full), > - __DEFINE_LINK_MODE_PARAMS(100, T, Half), > - __DEFINE_LINK_MODE_PARAMS(100, T, Full), > - __DEFINE_LINK_MODE_PARAMS(1000, T, Half), > - __DEFINE_LINK_MODE_PARAMS(1000, T, Full), > + __DEFINE_LINK_MODE_PARAMS(10, T, 1, Half), > + __DEFINE_LINK_MODE_PARAMS(10, T, 1, Full), > + __DEFINE_LINK_MODE_PARAMS(100, T, 1, Half), > + __DEFINE_LINK_MODE_PARAMS(100, T, 1, Full), > + __DEFINE_LINK_MODE_PARAMS(1000, T, 1, Half), > + __DEFINE_LINK_MODE_PARAMS(1000, T, 1, Full), Technically, 4 may be more appropriate for 1000base-T, 2500base-T, 5000base-T and 10000base-T but it's probably just a formality. While there is 1000base-T1, I'm not sure if we can expect a device which would support e.g. both 1000base-T and 1000base-T1 (or some other colliding combination of modes). Michal ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-10-10 15:41 ` [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes Ido Schimmel 2020-10-11 22:37 ` Jakub Kicinski 2020-10-12 17:03 ` Michal Kubecek @ 2020-11-19 20:38 ` Edwin Peer 2020-11-23 9:40 ` Jiri Pirko 2 siblings, 1 reply; 45+ messages in thread From: Edwin Peer @ 2020-11-19 20:38 UTC (permalink / raw) To: Ido Schimmel Cc: netdev, David S . Miller, Jakub Kicinski, jiri, danieller, andrew, f.fainelli, mkubecek, mlxsw, Ido Schimmel [-- Attachment #1: Type: text/plain, Size: 648 bytes --] On Sat, Oct 10, 2020 at 3:54 PM Ido Schimmel <idosch@idosch.org> wrote: > Add 'ETHTOOL_A_LINKMODES_LANES' attribute and expand 'struct > ethtool_link_settings' with lanes field in order to implement a new > lanes-selector that will enable the user to advertise a specific number > of lanes as well. Why can't this be implied by port break-out configuration? For higher speed signalling modes like PAM4, what's the difference between a port with unused lanes vs the same port split into multiple logical ports? In essence, the driver could then always choose the slowest signalling mode that utilizes all the available lanes. Regards, Edwin Peer [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 4160 bytes --] ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-11-19 20:38 ` Edwin Peer @ 2020-11-23 9:40 ` Jiri Pirko 2020-11-30 17:01 ` Edwin Peer 0 siblings, 1 reply; 45+ messages in thread From: Jiri Pirko @ 2020-11-23 9:40 UTC (permalink / raw) To: Edwin Peer Cc: Ido Schimmel, netdev, David S . Miller, Jakub Kicinski, jiri, danieller, andrew, f.fainelli, mkubecek, mlxsw, Ido Schimmel Thu, Nov 19, 2020 at 09:38:34PM CET, edwin.peer@broadcom.com wrote: >On Sat, Oct 10, 2020 at 3:54 PM Ido Schimmel <idosch@idosch.org> wrote: > >> Add 'ETHTOOL_A_LINKMODES_LANES' attribute and expand 'struct >> ethtool_link_settings' with lanes field in order to implement a new >> lanes-selector that will enable the user to advertise a specific number >> of lanes as well. > >Why can't this be implied by port break-out configuration? For higher >speed signalling modes like PAM4, what's the difference between a >port with unused lanes vs the same port split into multiple logical >ports? In essence, the driver could then always choose the slowest There is a crucial difference. Split port is configured alwasy by user. Each split port has a devlink instace, netdevice associated with it. It is one level above the lanes. >signalling mode that utilizes all the available lanes. > >Regards, >Edwin Peer ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-11-23 9:40 ` Jiri Pirko @ 2020-11-30 17:01 ` Edwin Peer 2020-11-30 17:14 ` Jiri Pirko 0 siblings, 1 reply; 45+ messages in thread From: Edwin Peer @ 2020-11-30 17:01 UTC (permalink / raw) To: Jiri Pirko Cc: Ido Schimmel, netdev, David S . Miller, Jakub Kicinski, jiri, danieller, andrew, f.fainelli, mkubecek, mlxsw, Ido Schimmel [-- Attachment #1: Type: text/plain, Size: 1135 bytes --] On Mon, Nov 23, 2020 at 1:40 AM Jiri Pirko <jiri@resnulli.us> wrote: > >Why can't this be implied by port break-out configuration? For higher > >speed signalling modes like PAM4, what's the difference between a > >port with unused lanes vs the same port split into multiple logical > >ports? In essence, the driver could then always choose the slowest > > There is a crucial difference. Split port is configured alwasy by user. > Each split port has a devlink instace, netdevice associated with it. > It is one level above the lanes. Right, but the one still implies the other. Splitting the port implies fewer lanes available. I understand the concern if the device cannot provide sufficient MAC resources to provide for the additional ports, but leaving a net device unused (with the option to utilize an additional, now spare, port) still seems better to me than leaving lanes unused and always wasted. Otherwise, the earlier suggestion of fully specifying the forced link mode (although I don't think Andrew articulated it quite that way) instead of a forced speed and separate lane mode makes most sense. Regards, Edwin Peer [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 4160 bytes --] ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-11-30 17:01 ` Edwin Peer @ 2020-11-30 17:14 ` Jiri Pirko 2020-11-30 18:00 ` Edwin Peer 0 siblings, 1 reply; 45+ messages in thread From: Jiri Pirko @ 2020-11-30 17:14 UTC (permalink / raw) To: Edwin Peer Cc: Ido Schimmel, netdev, David S . Miller, Jakub Kicinski, jiri, danieller, andrew, f.fainelli, mkubecek, mlxsw, Ido Schimmel Mon, Nov 30, 2020 at 06:01:43PM CET, edwin.peer@broadcom.com wrote: >On Mon, Nov 23, 2020 at 1:40 AM Jiri Pirko <jiri@resnulli.us> wrote: > >> >Why can't this be implied by port break-out configuration? For higher >> >speed signalling modes like PAM4, what's the difference between a >> >port with unused lanes vs the same port split into multiple logical >> >ports? In essence, the driver could then always choose the slowest >> >> There is a crucial difference. Split port is configured alwasy by user. >> Each split port has a devlink instace, netdevice associated with it. >> It is one level above the lanes. > >Right, but the one still implies the other. Splitting the port implies fewer >lanes available. > >I understand the concern if the device cannot provide sufficient MAC >resources to provide for the additional ports, but leaving a net device >unused (with the option to utilize an additional, now spare, port) still >seems better to me than leaving lanes unused and always wasted. I don't follow what exactly are you implying. Could you elaborate a bit more? > >Otherwise, the earlier suggestion of fully specifying the forced link >mode (although I don't think Andrew articulated it quite that way) >instead of a forced speed and separate lane mode makes most >sense. > >Regards, >Edwin Peer ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-11-30 17:14 ` Jiri Pirko @ 2020-11-30 18:00 ` Edwin Peer 2020-12-01 11:22 ` Jiri Pirko 0 siblings, 1 reply; 45+ messages in thread From: Edwin Peer @ 2020-11-30 18:00 UTC (permalink / raw) To: Jiri Pirko Cc: Ido Schimmel, netdev, David S . Miller, Jakub Kicinski, jiri, danieller, andrew, f.fainelli, mkubecek, mlxsw, Ido Schimmel [-- Attachment #1: Type: text/plain, Size: 2056 bytes --] On Mon, Nov 30, 2020 at 9:14 AM Jiri Pirko <jiri@resnulli.us> wrote: > >> There is a crucial difference. Split port is configured alwasy by user. > >> Each split port has a devlink instace, netdevice associated with it. > >> It is one level above the lanes. > > > >Right, but the one still implies the other. Splitting the port implies fewer > >lanes available. > > > >I understand the concern if the device cannot provide sufficient MAC > >resources to provide for the additional ports, but leaving a net device > >unused (with the option to utilize an additional, now spare, port) still > >seems better to me than leaving lanes unused and always wasted. > > I don't follow what exactly are you implying. Could you elaborate a bit > more? Perhaps an example... Consider a physical QSFP connector comprising 4 lanes. Today, if the speed is forced, we would achieve 100G speeds using all 4 lanes with NRZ encoding. If we configure the port for PAM4 encoding at the same speed, then we only require 2 of the available 4 lanes. The remaining 2 lanes are wasted. If we only require 2 of the 4 lanes, why not split the port and request the same speed of one of the now split out ports? Now, this same speed is only achievable using PAM4 encoding (it is implied) and we have a spare, potentially usable, assuming an appropriate break- out cable, port instead of the 2 unused lanes. So concretely, I'm suggesting that if we want to force PAM4 at the lower speeds, split the port and then we don't need an ethtool interface change at all to achieve the same goal. Having a spare (potentially usable) port is better than spare (unusable) lanes. Now, if the port can't be split for some reason (perhaps there aren't sufficient device MAC resources, stats contexts, whatever), then that's a different story. But, even so, perhaps the port lane topology more appropriately belongs as part of a device configuration interface in devlink and the number of lanes available to a port should be a property of the port instead of a link mode knob? Regards, Edwin Peer [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 4160 bytes --] ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-11-30 18:00 ` Edwin Peer @ 2020-12-01 11:22 ` Jiri Pirko 2020-12-02 0:32 ` Edwin Peer 0 siblings, 1 reply; 45+ messages in thread From: Jiri Pirko @ 2020-12-01 11:22 UTC (permalink / raw) To: Edwin Peer Cc: Ido Schimmel, netdev, David S . Miller, Jakub Kicinski, jiri, danieller, andrew, f.fainelli, mkubecek, mlxsw, Ido Schimmel Mon, Nov 30, 2020 at 07:00:16PM CET, edwin.peer@broadcom.com wrote: >On Mon, Nov 30, 2020 at 9:14 AM Jiri Pirko <jiri@resnulli.us> wrote: > >> >> There is a crucial difference. Split port is configured alwasy by user. >> >> Each split port has a devlink instace, netdevice associated with it. >> >> It is one level above the lanes. >> > >> >Right, but the one still implies the other. Splitting the port implies fewer >> >lanes available. >> > >> >I understand the concern if the device cannot provide sufficient MAC >> >resources to provide for the additional ports, but leaving a net device >> >unused (with the option to utilize an additional, now spare, port) still >> >seems better to me than leaving lanes unused and always wasted. >> >> I don't follow what exactly are you implying. Could you elaborate a bit >> more? > >Perhaps an example... > >Consider a physical QSFP connector comprising 4 lanes. Today, if the >speed is forced, we would achieve 100G speeds using all 4 lanes with >NRZ encoding. If we configure the port for PAM4 encoding at the same >speed, then we only require 2 of the available 4 lanes. The remaining 2 >lanes are wasted. If we only require 2 of the 4 lanes, why not split the >port and request the same speed of one of the now split out ports? Now, >this same speed is only achievable using PAM4 encoding (it is implied) >and we have a spare, potentially usable, assuming an appropriate break- >out cable, port instead of the 2 unused lanes. I don't see how this dynamic split port could work in real life to be honest. The split is something admin needs to configure and can rely that netdevice exists all the time and not comes and goes under different circumstances. Multiple obvious reasons why. One way or another, I don't see the relation between this and this patchset. > >So concretely, I'm suggesting that if we want to force PAM4 at the lower >speeds, split the port and then we don't need an ethtool interface change >at all to achieve the same goal. Having a spare (potentially usable) port >is better than spare (unusable) lanes. The admin has to decide, define. > >Now, if the port can't be split for some reason (perhaps there aren't >sufficient device MAC resources, stats contexts, whatever), then that's >a different story. But, even so, perhaps the port lane topology more >appropriately belongs as part of a device configuration interface in >devlink and the number of lanes available to a port should be a >property of the port instead of a link mode knob? > >Regards, >Edwin Peer ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-12-01 11:22 ` Jiri Pirko @ 2020-12-02 0:32 ` Edwin Peer 2020-12-02 10:09 ` Jiri Pirko 0 siblings, 1 reply; 45+ messages in thread From: Edwin Peer @ 2020-12-02 0:32 UTC (permalink / raw) To: Jiri Pirko Cc: Ido Schimmel, netdev, David S . Miller, Jakub Kicinski, jiri, danieller, andrew, f.fainelli, mkubecek, mlxsw, Ido Schimmel [-- Attachment #1: Type: text/plain, Size: 2686 bytes --] On Tue, Dec 1, 2020 at 3:22 AM Jiri Pirko <jiri@resnulli.us> wrote: > >Consider a physical QSFP connector comprising 4 lanes. Today, if the > >speed is forced, we would achieve 100G speeds using all 4 lanes with > >NRZ encoding. If we configure the port for PAM4 encoding at the same > >speed, then we only require 2 of the available 4 lanes. The remaining 2 > >lanes are wasted. If we only require 2 of the 4 lanes, why not split the > >port and request the same speed of one of the now split out ports? Now, > >this same speed is only achievable using PAM4 encoding (it is implied) > >and we have a spare, potentially usable, assuming an appropriate break- > >out cable, port instead of the 2 unused lanes. > > I don't see how this dynamic split port could work in real life to be > honest. The split is something admin needs to configure and can rely > that netdevice exists all the time and not comes and goes under > different circumstances. Multiple obvious reasons why. I'm not suggesting the port split be dynamic at all. I'm suggesting that if the admin wants or needs to force PAM4 on a port that would otherwise be able to achieve the given speed using more lanes with NRZ, then the admin should split the port, so that it has fewer lanes, in order to make that intent clear (or otherwise configure the port to have fewer lanes attached, if you really don't want to or can't create the additional split port). Using this approach, the existing ethtool forced speed interface is sufficient to configure all possible lane encodings, because the encoding that the driver must select is now implicit (note, we don't need to care about media type here). That is, the driver can always select the encoding that maximizes utilization of the lanes available to the port (as defined by the admin). > >So concretely, I'm suggesting that if we want to force PAM4 at the lower > >speeds, split the port and then we don't need an ethtool interface change > >at all to achieve the same goal. Having a spare (potentially usable) port > >is better than spare (unusable) lanes. > > The admin has to decide, define. I'm not sure I understand. The admin would indeed decide. This paragraph merely served to motivate why a rational admin should prefer to have a spare port rather than unused lanes he can't use, because they would be attached to a port using an encoding that doesn't need them. If he wasn't planning on using the additional port, he loses nothing. Otherwise, he gains something he would not otherwise have had (it's win-win). From the perspective of the original port, two unused lanes is no different than two lanes allocated to another logical port. Regards, Edwin Peer [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 4160 bytes --] ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-12-02 0:32 ` Edwin Peer @ 2020-12-02 10:09 ` Jiri Pirko 2020-12-02 17:53 ` Edwin Peer 0 siblings, 1 reply; 45+ messages in thread From: Jiri Pirko @ 2020-12-02 10:09 UTC (permalink / raw) To: Edwin Peer Cc: Ido Schimmel, netdev, David S . Miller, Jakub Kicinski, jiri, danieller, andrew, f.fainelli, mkubecek, mlxsw, Ido Schimmel Wed, Dec 02, 2020 at 01:32:46AM CET, edwin.peer@broadcom.com wrote: >On Tue, Dec 1, 2020 at 3:22 AM Jiri Pirko <jiri@resnulli.us> wrote: > >> >Consider a physical QSFP connector comprising 4 lanes. Today, if the >> >speed is forced, we would achieve 100G speeds using all 4 lanes with >> >NRZ encoding. If we configure the port for PAM4 encoding at the same >> >speed, then we only require 2 of the available 4 lanes. The remaining 2 >> >lanes are wasted. If we only require 2 of the 4 lanes, why not split the >> >port and request the same speed of one of the now split out ports? Now, >> >this same speed is only achievable using PAM4 encoding (it is implied) >> >and we have a spare, potentially usable, assuming an appropriate break- >> >out cable, port instead of the 2 unused lanes. >> >> I don't see how this dynamic split port could work in real life to be >> honest. The split is something admin needs to configure and can rely >> that netdevice exists all the time and not comes and goes under >> different circumstances. Multiple obvious reasons why. > >I'm not suggesting the port split be dynamic at all. I'm suggesting that if >the admin wants or needs to force PAM4 on a port that would otherwise >be able to achieve the given speed using more lanes with NRZ, then the >admin should split the port, so that it has fewer lanes, in order to make >that intent clear (or otherwise configure the port to have fewer lanes >attached, if you really don't want to or can't create the additional split >port). Okay, I see your point now. The thing is, the port split/unsplit causes a great distubance. Meaning, the netdevs all of the port disappear/reappear. Now consider following example: You have a router you have configured routes on many netdevs On one of the netdevs (has routes on it), you for any reason need to force lane number. In your suggestion, the netdev disappears along with the routes, the routing is then broken. I don't see how this could be acceptable. We are talking here about netdev configuration, we have a tool for that, that is ethtool. What you suggest is to take it to different level, I don't believe it is correct/doable. > >Using this approach, the existing ethtool forced speed interface is >sufficient to configure all possible lane encodings, because the >encoding that the driver must select is now implicit (note, we don't >need to care about media type here). That is, the driver can always >select the encoding that maximizes utilization of the lanes available >to the port (as defined by the admin). > >> >So concretely, I'm suggesting that if we want to force PAM4 at the lower >> >speeds, split the port and then we don't need an ethtool interface change >> >at all to achieve the same goal. Having a spare (potentially usable) port >> >is better than spare (unusable) lanes. >> >> The admin has to decide, define. > >I'm not sure I understand. The admin would indeed decide. This paragraph >merely served to motivate why a rational admin should prefer to have a >spare port rather than unused lanes he can't use, because they would be >attached to a port using an encoding that doesn't need them. If he wasn't >planning on using the additional port, he loses nothing. Otherwise, he gains >something he would not otherwise have had (it's win-win). From the >perspective of the original port, two unused lanes is no different than two >lanes allocated to another logical port. > >Regards, >Edwin Peer ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes 2020-12-02 10:09 ` Jiri Pirko @ 2020-12-02 17:53 ` Edwin Peer 0 siblings, 0 replies; 45+ messages in thread From: Edwin Peer @ 2020-12-02 17:53 UTC (permalink / raw) To: Jiri Pirko Cc: Ido Schimmel, netdev, David S . Miller, Jakub Kicinski, Jiri Pirko, Danielle Ratson, Andrew Lunn, f.fainelli, Michal Kubecek, mlxsw, Ido Schimmel [-- Attachment #1: Type: text/plain, Size: 1867 bytes --] On Wed, Dec 2, 2020 at 2:09 AM Jiri Pirko <jiri@resnulli.us> wrote: > >I'm not suggesting the port split be dynamic at all. I'm suggesting that if > >the admin wants or needs to force PAM4 on a port that would otherwise > >be able to achieve the given speed using more lanes with NRZ, then the > >admin should split the port, so that it has fewer lanes, in order to make > >that intent clear (or otherwise configure the port to have fewer lanes > >attached, if you really don't want to or can't create the additional split > >port). > > Okay, I see your point now. The thing is, the port split/unsplit causes > a great distubance. Meaning, the netdevs all of the port > disappear/reappear. Now consider following example: I guess that's a detail of the present implementation and/or device capabilities (I'm not particularly familiar), but presumably it is at least possible, in principle, to modify a device's port config without taking down the netdev. That said, after spending more time thinking about this, I'm coming around to the idea of being able to explicitly set encoding (not lanes) via ethtool. And, in this context, by encoding, I technically mean line code signalling, not symbol encoding. Although it does for today's link modes, the number of lanes does not generally imply signalling mode. In future we may have PAM8 signalling and then the present proposal of deriving the signalling mode for a given speed from the lane count falls down. We should be specifying signalling mode via ethtool instead of lanes. Alternatively, we need to be specifying the fully defined link mode. But, doing so is fraught with other issues, including how to represent it in the interface and the fact that the user doesn't necessarily want to specify physical media in these cases, something that is implied by the full link mode definition. Regards, Edwin Peer [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 4160 bytes --] ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH net-next 2/6] ethtool: Expose the number of lanes in use 2020-10-10 15:41 [PATCH net-next 0/6] Support setting lanes via ethtool Ido Schimmel 2020-10-10 15:41 ` [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes Ido Schimmel @ 2020-10-10 15:41 ` Ido Schimmel 2020-10-10 15:41 ` [PATCH net-next 3/6] mlxsw: ethtool: Remove max lanes filtering Ido Schimmel ` (3 subsequent siblings) 5 siblings, 0 replies; 45+ messages in thread From: Ido Schimmel @ 2020-10-10 15:41 UTC (permalink / raw) To: netdev Cc: davem, kuba, jiri, danieller, andrew, f.fainelli, mkubecek, mlxsw, Ido Schimmel From: Danielle Ratson <danieller@nvidia.com> Currently, ethtool does not expose how many lanes are used when the link is up. After adding a possibility to advertise or force a specific number of lanes, the lanes in use value can be either the maximum width of the port or below. Extend ethtool to expose the number of lanes currently in use for drivers that support it. For example: $ ethtool -s swp1 speed 100000 lanes 4 $ ethtool -s swp2 speed 100000 lanes 4 $ ip link set swp1 up $ ip link set swp2 up $ ethtool swp1 Settings for swp1: Supported ports: [ FIBRE Backplane ] Supported link modes: 1000baseT/Full 10000baseT/Full 1000baseKX/Full 10000baseKR/Full 10000baseR_FEC 40000baseKR4/Full 40000baseCR4/Full 40000baseSR4/Full 40000baseLR4/Full 25000baseCR/Full 25000baseKR/Full 25000baseSR/Full 50000baseCR2/Full 50000baseKR2/Full 100000baseKR4/Full 100000baseSR4/Full 100000baseCR4/Full 100000baseLR4_ER4/Full 50000baseSR2/Full 10000baseCR/Full 10000baseSR/Full 10000baseLR/Full 10000baseER/Full 50000baseKR/Full 50000baseSR/Full 50000baseCR/Full 50000baseLR_ER_FR/Full 50000baseDR/Full 100000baseKR2/Full 100000baseSR2/Full 100000baseCR2/Full 100000baseLR2_ER2_FR2/Full 100000baseDR2/Full 200000baseKR4/Full 200000baseSR4/Full 200000baseLR4_ER4_FR4/Full 200000baseDR4/Full 200000baseCR4/Full Supported pause frame use: Symmetric Receive-only Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: 1000baseT/Full 10000baseT/Full 1000baseKX/Full 1000baseKX/Full 10000baseKR/Full 10000baseR_FEC 40000baseKR4/Full 40000baseCR4/Full 40000baseSR4/Full 40000baseLR4/Full 25000baseCR/Full 25000baseKR/Full 25000baseSR/Full 50000baseCR2/Full 50000baseKR2/Full 100000baseKR4/Full 100000baseSR4/Full 100000baseCR4/Full 100000baseLR4_ER4/Full 50000baseSR2/Full 10000baseCR/Full 10000baseSR/Full 10000baseLR/Full 10000baseER/Full 200000baseKR4/Full 200000baseSR4/Full 200000baseLR4_ER4_FR4/Full 200000baseDR4/Full 200000baseCR4/Full Advertised pause frame use: No Advertised auto-negotiation: Yes Advertised FEC modes: Not reported Advertised link modes: 100000baseKR4/Full 100000baseSR4/Full 100000baseCR4/Full 100000baseLR4_ER4/Full Advertised pause frame use: No Advertised auto-negotiation: Yes Advertised FEC modes: Not reported Speed: 100000Mb/s Lanes: 4 Duplex: Full Auto-negotiation: on Port: Direct Attach Copper PHYAD: 0 Transceiver: internal Link detected: yes Signed-off-by: Danielle Ratson <danieller@nvidia.com> Reviewed-by: Jiri Pirko <jiri@nvidia.com> Signed-off-by: Ido Schimmel <idosch@nvidia.com> --- Documentation/networking/ethtool-netlink.rst | 5 +++-- net/ethtool/linkmodes.c | 5 +++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst index 05073482db05..16638ee9d3f9 100644 --- a/Documentation/networking/ethtool-netlink.rst +++ b/Documentation/networking/ethtool-netlink.rst @@ -388,8 +388,8 @@ LINKMODES_GET ============= Requests link modes (supported, advertised and peer advertised) and related -information (autonegotiation status, link speed and duplex) as provided by -``ETHTOOL_GLINKSETTINGS``. The request does not use any attributes. +information (autonegotiation status, link speed, duplex and lanes) as provided +by ``ETHTOOL_GLINKSETTINGS``. The request does not use any attributes. Request contents: @@ -408,6 +408,7 @@ Kernel response contents: ``ETHTOOL_A_LINKMODES_DUPLEX`` u8 duplex mode ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG`` u8 Master/slave port mode ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE`` u8 Master/slave port state + ``ETHTOOL_A_LINKMODES_LANES`` u32 lanes ========================================== ====== ========================== For ``ETHTOOL_A_LINKMODES_OURS``, value represents advertised modes and mask diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c index a1f02896ed8b..a940cfcd2122 100644 --- a/net/ethtool/linkmodes.c +++ b/net/ethtool/linkmodes.c @@ -43,6 +43,9 @@ static int linkmodes_prepare_data(const struct ethnl_req_info *req_base, goto out; } + if (!(dev->ethtool_ops->capabilities & ETHTOOL_CAP_LINK_LANES_SUPPORTED)) + data->ksettings.lanes = ETHTOOL_LANES_UNKNOWN; + data->peer_empty = bitmap_empty(data->ksettings.link_modes.lp_advertising, __ETHTOOL_LINK_MODE_MASK_NBITS); @@ -63,6 +66,7 @@ static int linkmodes_reply_size(const struct ethnl_req_info *req_base, len = nla_total_size(sizeof(u8)) /* LINKMODES_AUTONEG */ + nla_total_size(sizeof(u32)) /* LINKMODES_SPEED */ + + nla_total_size(sizeof(u32)) /* LINKMODES_LANES */ + nla_total_size(sizeof(u8)) /* LINKMODES_DUPLEX */ + 0; ret = ethnl_bitset_size(ksettings->link_modes.advertising, @@ -120,6 +124,7 @@ static int linkmodes_fill_reply(struct sk_buff *skb, } if (nla_put_u32(skb, ETHTOOL_A_LINKMODES_SPEED, lsettings->speed) || + nla_put_u32(skb, ETHTOOL_A_LINKMODES_LANES, ksettings->lanes) || nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex)) return -EMSGSIZE; -- 2.26.2 ^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH net-next 3/6] mlxsw: ethtool: Remove max lanes filtering 2020-10-10 15:41 [PATCH net-next 0/6] Support setting lanes via ethtool Ido Schimmel 2020-10-10 15:41 ` [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes Ido Schimmel 2020-10-10 15:41 ` [PATCH net-next 2/6] ethtool: Expose the number of lanes in use Ido Schimmel @ 2020-10-10 15:41 ` Ido Schimmel 2020-10-10 15:41 ` [PATCH net-next 4/6] mlxsw: ethtool: Add support for setting lanes when autoneg is off Ido Schimmel ` (2 subsequent siblings) 5 siblings, 0 replies; 45+ messages in thread From: Ido Schimmel @ 2020-10-10 15:41 UTC (permalink / raw) To: netdev Cc: davem, kuba, jiri, danieller, andrew, f.fainelli, mkubecek, mlxsw, Ido Schimmel From: Danielle Ratson <danieller@nvidia.com> Currently, when a speed can be supported by different number of lanes, the supported link modes bitmask contains only link modes with a single number of lanes. This was done in order to prevent auto negotiation on number of lanes after 50G-1-lane and 100G-2-lanes link modes were introduced. For example, if a port's max width is 4, only link modes with 4 lanes will be presented as supported by that port, so 100G is always achieved by 4 lanes of 25G. After the previous patches that allow selection of the number of lanes, auto negotiation on number of lanes becomes practical. Remove that filtering of the maximum number of lanes supported link modes, so indeed all the supported and advertised link modes will be shown. Signed-off-by: Danielle Ratson <danieller@nvidia.com> Reviewed-by: Jiri Pirko <jiri@nvidia.com> Signed-off-by: Ido Schimmel <idosch@nvidia.com> --- .../net/ethernet/mellanox/mlxsw/spectrum.h | 4 +-- .../mellanox/mlxsw/spectrum_ethtool.c | 33 ++++++++----------- 2 files changed, 15 insertions(+), 22 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h index 3e26eb6cb140..7fdebecdc1f2 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h @@ -327,13 +327,13 @@ struct mlxsw_sp_port_type_speed_ops { u32 ptys_eth_proto, struct ethtool_link_ksettings *cmd); void (*from_ptys_link)(struct mlxsw_sp *mlxsw_sp, u32 ptys_eth_proto, - u8 width, unsigned long *mode); + unsigned long *mode); u32 (*from_ptys_speed)(struct mlxsw_sp *mlxsw_sp, u32 ptys_eth_proto); void (*from_ptys_speed_duplex)(struct mlxsw_sp *mlxsw_sp, bool carrier_ok, u32 ptys_eth_proto, struct ethtool_link_ksettings *cmd); int (*ptys_max_speed)(struct mlxsw_sp_port *mlxsw_sp_port, u32 *p_max_speed); - u32 (*to_ptys_advert_link)(struct mlxsw_sp *mlxsw_sp, u8 width, + u32 (*to_ptys_advert_link)(struct mlxsw_sp *mlxsw_sp, const struct ethtool_link_ksettings *cmd); u32 (*to_ptys_speed)(struct mlxsw_sp *mlxsw_sp, u8 width, u32 speed); void (*reg_ptys_eth_pack)(struct mlxsw_sp *mlxsw_sp, char *payload, diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c index 2096b6478958..085e5a0cb654 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c @@ -858,7 +858,7 @@ static int mlxsw_sp_port_get_sset_count(struct net_device *dev, int sset) static void mlxsw_sp_port_get_link_supported(struct mlxsw_sp *mlxsw_sp, u32 eth_proto_cap, - u8 width, struct ethtool_link_ksettings *cmd) + struct ethtool_link_ksettings *cmd) { const struct mlxsw_sp_port_type_speed_ops *ops; @@ -869,13 +869,13 @@ mlxsw_sp_port_get_link_supported(struct mlxsw_sp *mlxsw_sp, u32 eth_proto_cap, ethtool_link_ksettings_add_link_mode(cmd, supported, Pause); ops->from_ptys_supported_port(mlxsw_sp, eth_proto_cap, cmd); - ops->from_ptys_link(mlxsw_sp, eth_proto_cap, width, + ops->from_ptys_link(mlxsw_sp, eth_proto_cap, cmd->link_modes.supported); } static void mlxsw_sp_port_get_link_advertise(struct mlxsw_sp *mlxsw_sp, - u32 eth_proto_admin, bool autoneg, u8 width, + u32 eth_proto_admin, bool autoneg, struct ethtool_link_ksettings *cmd) { const struct mlxsw_sp_port_type_speed_ops *ops; @@ -886,7 +886,7 @@ mlxsw_sp_port_get_link_advertise(struct mlxsw_sp *mlxsw_sp, return; ethtool_link_ksettings_add_link_mode(cmd, advertising, Autoneg); - ops->from_ptys_link(mlxsw_sp, eth_proto_admin, width, + ops->from_ptys_link(mlxsw_sp, eth_proto_admin, cmd->link_modes.advertising); } @@ -960,11 +960,9 @@ static int mlxsw_sp_port_get_link_ksettings(struct net_device *dev, ops = mlxsw_sp->port_type_speed_ops; autoneg = mlxsw_sp_port->link.autoneg; - mlxsw_sp_port_get_link_supported(mlxsw_sp, eth_proto_cap, - mlxsw_sp_port->mapping.width, cmd); + mlxsw_sp_port_get_link_supported(mlxsw_sp, eth_proto_cap, cmd); - mlxsw_sp_port_get_link_advertise(mlxsw_sp, eth_proto_admin, autoneg, - mlxsw_sp_port->mapping.width, cmd); + mlxsw_sp_port_get_link_advertise(mlxsw_sp, eth_proto_admin, autoneg, cmd); cmd->base.autoneg = autoneg ? AUTONEG_ENABLE : AUTONEG_DISABLE; cmd->base.port = mlxsw_sp_port_connector_port(connector_type); @@ -997,8 +995,7 @@ mlxsw_sp_port_set_link_ksettings(struct net_device *dev, autoneg = cmd->base.autoneg == AUTONEG_ENABLE; eth_proto_new = autoneg ? - ops->to_ptys_advert_link(mlxsw_sp, mlxsw_sp_port->mapping.width, - cmd) : + ops->to_ptys_advert_link(mlxsw_sp, cmd) : ops->to_ptys_speed(mlxsw_sp, mlxsw_sp_port->mapping.width, cmd->base.speed); @@ -1198,7 +1195,7 @@ mlxsw_sp1_from_ptys_supported_port(struct mlxsw_sp *mlxsw_sp, static void mlxsw_sp1_from_ptys_link(struct mlxsw_sp *mlxsw_sp, u32 ptys_eth_proto, - u8 width, unsigned long *mode) + unsigned long *mode) { int i; @@ -1260,7 +1257,7 @@ static int mlxsw_sp1_ptys_max_speed(struct mlxsw_sp_port *mlxsw_sp_port, u32 *p_ } static u32 -mlxsw_sp1_to_ptys_advert_link(struct mlxsw_sp *mlxsw_sp, u8 width, +mlxsw_sp1_to_ptys_advert_link(struct mlxsw_sp *mlxsw_sp, const struct ethtool_link_ksettings *cmd) { u32 ptys_proto = 0; @@ -1604,14 +1601,12 @@ mlxsw_sp2_set_bit_ethtool(const struct mlxsw_sp2_port_link_mode *link_mode, static void mlxsw_sp2_from_ptys_link(struct mlxsw_sp *mlxsw_sp, u32 ptys_eth_proto, - u8 width, unsigned long *mode) + unsigned long *mode) { - u8 mask_width = mlxsw_sp_port_mask_width_get(width); int i; for (i = 0; i < MLXSW_SP2_PORT_LINK_MODE_LEN; i++) { - if ((ptys_eth_proto & mlxsw_sp2_port_link_mode[i].mask) && - (mask_width & mlxsw_sp2_port_link_mode[i].mask_width)) + if (ptys_eth_proto & mlxsw_sp2_port_link_mode[i].mask) mlxsw_sp2_set_bit_ethtool(&mlxsw_sp2_port_link_mode[i], mode); } @@ -1683,16 +1678,14 @@ mlxsw_sp2_test_bit_ethtool(const struct mlxsw_sp2_port_link_mode *link_mode, } static u32 -mlxsw_sp2_to_ptys_advert_link(struct mlxsw_sp *mlxsw_sp, u8 width, +mlxsw_sp2_to_ptys_advert_link(struct mlxsw_sp *mlxsw_sp, const struct ethtool_link_ksettings *cmd) { - u8 mask_width = mlxsw_sp_port_mask_width_get(width); u32 ptys_proto = 0; int i; for (i = 0; i < MLXSW_SP2_PORT_LINK_MODE_LEN; i++) { - if ((mask_width & mlxsw_sp2_port_link_mode[i].mask_width) && - mlxsw_sp2_test_bit_ethtool(&mlxsw_sp2_port_link_mode[i], + if (mlxsw_sp2_test_bit_ethtool(&mlxsw_sp2_port_link_mode[i], cmd->link_modes.advertising)) ptys_proto |= mlxsw_sp2_port_link_mode[i].mask; } -- 2.26.2 ^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH net-next 4/6] mlxsw: ethtool: Add support for setting lanes when autoneg is off 2020-10-10 15:41 [PATCH net-next 0/6] Support setting lanes via ethtool Ido Schimmel ` (2 preceding siblings ...) 2020-10-10 15:41 ` [PATCH net-next 3/6] mlxsw: ethtool: Remove max lanes filtering Ido Schimmel @ 2020-10-10 15:41 ` Ido Schimmel 2020-10-10 15:41 ` [PATCH net-next 5/6] mlxsw: ethtool: Expose the number of lanes in use Ido Schimmel 2020-10-10 15:41 ` [PATCH net-next 6/6] net: selftests: Add lanes setting test Ido Schimmel 5 siblings, 0 replies; 45+ messages in thread From: Ido Schimmel @ 2020-10-10 15:41 UTC (permalink / raw) To: netdev Cc: davem, kuba, jiri, danieller, andrew, f.fainelli, mkubecek, mlxsw, Ido Schimmel From: Danielle Ratson <danieller@nvidia.com> Currently, when auto negotiation is set to off, the user can force a specific speed or both speed and duplex. The user cannot influence the number of lanes that will be forced. Add support for setting speed along with lanes so one would be able to choose how many lanes will be forced. When lanes parameter is not passed from user space, the default of the lanes is the width of the port. Signed-off-by: Danielle Ratson <danieller@nvidia.com> Reviewed-by: Jiri Pirko <jiri@nvidia.com> Signed-off-by: Ido Schimmel <idosch@nvidia.com> --- .../net/ethernet/mellanox/mlxsw/spectrum.h | 3 +- .../mellanox/mlxsw/spectrum_ethtool.c | 40 +++++++++++++------ 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h index 7fdebecdc1f2..b8e91792ac08 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h @@ -335,7 +335,8 @@ struct mlxsw_sp_port_type_speed_ops { int (*ptys_max_speed)(struct mlxsw_sp_port *mlxsw_sp_port, u32 *p_max_speed); u32 (*to_ptys_advert_link)(struct mlxsw_sp *mlxsw_sp, const struct ethtool_link_ksettings *cmd); - u32 (*to_ptys_speed)(struct mlxsw_sp *mlxsw_sp, u8 width, u32 speed); + u32 (*to_ptys_speed_lanes)(struct mlxsw_sp *mlxsw_sp, u8 width, + const struct ethtool_link_ksettings *cmd); void (*reg_ptys_eth_pack)(struct mlxsw_sp *mlxsw_sp, char *payload, u8 local_port, u32 proto_admin, bool autoneg); void (*reg_ptys_eth_unpack)(struct mlxsw_sp *mlxsw_sp, char *payload, diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c index 085e5a0cb654..8a1b5d437822 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c @@ -996,12 +996,12 @@ mlxsw_sp_port_set_link_ksettings(struct net_device *dev, autoneg = cmd->base.autoneg == AUTONEG_ENABLE; eth_proto_new = autoneg ? ops->to_ptys_advert_link(mlxsw_sp, cmd) : - ops->to_ptys_speed(mlxsw_sp, mlxsw_sp_port->mapping.width, - cmd->base.speed); + ops->to_ptys_speed_lanes(mlxsw_sp, mlxsw_sp_port->mapping.width, + cmd); eth_proto_new = eth_proto_new & eth_proto_cap; if (!eth_proto_new) { - netdev_err(dev, "No supported speed requested\n"); + netdev_err(dev, "No supported speed or lanes requested\n"); return -EINVAL; } @@ -1060,6 +1060,7 @@ mlxsw_sp_get_ts_info(struct net_device *netdev, struct ethtool_ts_info *info) } const struct ethtool_ops mlxsw_sp_port_ethtool_ops = { + .capabilities = ETHTOOL_CAP_LINK_LANES_SUPPORTED, .get_drvinfo = mlxsw_sp_port_get_drvinfo, .get_link = ethtool_op_get_link, .get_link_ext_state = mlxsw_sp_port_get_link_ext_state, @@ -1271,14 +1272,17 @@ mlxsw_sp1_to_ptys_advert_link(struct mlxsw_sp *mlxsw_sp, return ptys_proto; } -static u32 mlxsw_sp1_to_ptys_speed(struct mlxsw_sp *mlxsw_sp, u8 width, - u32 speed) +static u32 mlxsw_sp1_to_ptys_speed_lanes(struct mlxsw_sp *mlxsw_sp, u8 width, + const struct ethtool_link_ksettings *cmd) { u32 ptys_proto = 0; int i; + if (cmd->lanes > width) + return ptys_proto; + for (i = 0; i < MLXSW_SP1_PORT_LINK_MODE_LEN; i++) { - if (speed == mlxsw_sp1_port_link_mode[i].speed) + if (cmd->base.speed == mlxsw_sp1_port_link_mode[i].speed) ptys_proto |= mlxsw_sp1_port_link_mode[i].mask; } return ptys_proto; @@ -1307,7 +1311,7 @@ const struct mlxsw_sp_port_type_speed_ops mlxsw_sp1_port_type_speed_ops = { .from_ptys_speed_duplex = mlxsw_sp1_from_ptys_speed_duplex, .ptys_max_speed = mlxsw_sp1_ptys_max_speed, .to_ptys_advert_link = mlxsw_sp1_to_ptys_advert_link, - .to_ptys_speed = mlxsw_sp1_to_ptys_speed, + .to_ptys_speed_lanes = mlxsw_sp1_to_ptys_speed_lanes, .reg_ptys_eth_pack = mlxsw_sp1_reg_ptys_eth_pack, .reg_ptys_eth_unpack = mlxsw_sp1_reg_ptys_eth_unpack, }; @@ -1692,16 +1696,28 @@ mlxsw_sp2_to_ptys_advert_link(struct mlxsw_sp *mlxsw_sp, return ptys_proto; } -static u32 mlxsw_sp2_to_ptys_speed(struct mlxsw_sp *mlxsw_sp, - u8 width, u32 speed) +static u32 mlxsw_sp2_to_ptys_speed_lanes(struct mlxsw_sp *mlxsw_sp, u8 width, + const struct ethtool_link_ksettings *cmd) { u8 mask_width = mlxsw_sp_port_mask_width_get(width); u32 ptys_proto = 0; + u8 mask_cmp; int i; + if (cmd->lanes > width) + return ptys_proto; + + if (cmd->lanes == ETHTOOL_LANES_UNKNOWN) + /* If number of lanes was not set by user space, force lanes to + * be width, so the number of lanes won't be negotiated. + */ + mask_cmp = mask_width; + else + mask_cmp = mlxsw_sp_port_mask_width_get(cmd->lanes); + for (i = 0; i < MLXSW_SP2_PORT_LINK_MODE_LEN; i++) { - if ((speed == mlxsw_sp2_port_link_mode[i].speed) && - (mask_width & mlxsw_sp2_port_link_mode[i].mask_width)) + if (cmd->base.speed == mlxsw_sp2_port_link_mode[i].speed && + (mask_cmp & mlxsw_sp2_port_link_mode[i].mask_width)) ptys_proto |= mlxsw_sp2_port_link_mode[i].mask; } return ptys_proto; @@ -1731,7 +1747,7 @@ const struct mlxsw_sp_port_type_speed_ops mlxsw_sp2_port_type_speed_ops = { .from_ptys_speed_duplex = mlxsw_sp2_from_ptys_speed_duplex, .ptys_max_speed = mlxsw_sp2_ptys_max_speed, .to_ptys_advert_link = mlxsw_sp2_to_ptys_advert_link, - .to_ptys_speed = mlxsw_sp2_to_ptys_speed, + .to_ptys_speed_lanes = mlxsw_sp2_to_ptys_speed_lanes, .reg_ptys_eth_pack = mlxsw_sp2_reg_ptys_eth_pack, .reg_ptys_eth_unpack = mlxsw_sp2_reg_ptys_eth_unpack, }; -- 2.26.2 ^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH net-next 5/6] mlxsw: ethtool: Expose the number of lanes in use 2020-10-10 15:41 [PATCH net-next 0/6] Support setting lanes via ethtool Ido Schimmel ` (3 preceding siblings ...) 2020-10-10 15:41 ` [PATCH net-next 4/6] mlxsw: ethtool: Add support for setting lanes when autoneg is off Ido Schimmel @ 2020-10-10 15:41 ` Ido Schimmel 2020-10-10 15:41 ` [PATCH net-next 6/6] net: selftests: Add lanes setting test Ido Schimmel 5 siblings, 0 replies; 45+ messages in thread From: Ido Schimmel @ 2020-10-10 15:41 UTC (permalink / raw) To: netdev Cc: davem, kuba, jiri, danieller, andrew, f.fainelli, mkubecek, mlxsw, Ido Schimmel From: Danielle Ratson <danieller@nvidia.com> Currently, the driver does not expose how many lanes are used when the link is up. Extract the lanes information from the device and expose it to ethtool. Signed-off-by: Danielle Ratson <danieller@nvidia.com> Reviewed-by: Jiri Pirko <jiri@nvidia.com> Signed-off-by: Ido Schimmel <idosch@nvidia.com> --- .../net/ethernet/mellanox/mlxsw/spectrum.h | 6 +- .../mellanox/mlxsw/spectrum_ethtool.c | 83 ++++++++++++++++--- 2 files changed, 74 insertions(+), 15 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h index b8e91792ac08..84aee7d08ab4 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h @@ -329,9 +329,9 @@ struct mlxsw_sp_port_type_speed_ops { void (*from_ptys_link)(struct mlxsw_sp *mlxsw_sp, u32 ptys_eth_proto, unsigned long *mode); u32 (*from_ptys_speed)(struct mlxsw_sp *mlxsw_sp, u32 ptys_eth_proto); - void (*from_ptys_speed_duplex)(struct mlxsw_sp *mlxsw_sp, - bool carrier_ok, u32 ptys_eth_proto, - struct ethtool_link_ksettings *cmd); + void (*from_ptys_speed_lanes_duplex)(struct mlxsw_sp *mlxsw_sp, + bool carrier_ok, u32 ptys_eth_proto, + struct ethtool_link_ksettings *cmd); int (*ptys_max_speed)(struct mlxsw_sp_port *mlxsw_sp_port, u32 *p_max_speed); u32 (*to_ptys_advert_link)(struct mlxsw_sp *mlxsw_sp, const struct ethtool_link_ksettings *cmd); diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c index 8a1b5d437822..6675d5e0d9d4 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c @@ -966,8 +966,8 @@ static int mlxsw_sp_port_get_link_ksettings(struct net_device *dev, cmd->base.autoneg = autoneg ? AUTONEG_ENABLE : AUTONEG_DISABLE; cmd->base.port = mlxsw_sp_port_connector_port(connector_type); - ops->from_ptys_speed_duplex(mlxsw_sp, netif_carrier_ok(dev), - eth_proto_oper, cmd); + ops->from_ptys_speed_lanes_duplex(mlxsw_sp, netif_carrier_ok(dev), + eth_proto_oper, cmd); return 0; } @@ -1081,6 +1081,7 @@ struct mlxsw_sp1_port_link_mode { enum ethtool_link_mode_bit_indices mask_ethtool; u32 mask; u32 speed; + u32 width; }; static const struct mlxsw_sp1_port_link_mode mlxsw_sp1_port_link_mode[] = { @@ -1089,12 +1090,14 @@ static const struct mlxsw_sp1_port_link_mode mlxsw_sp1_port_link_mode[] = { MLXSW_REG_PTYS_ETH_SPEED_1000BASE_KX, .mask_ethtool = ETHTOOL_LINK_MODE_1000baseKX_Full_BIT, .speed = SPEED_1000, + .width = ETHTOOL_LANES_1, }, { .mask = MLXSW_REG_PTYS_ETH_SPEED_10GBASE_CX4 | MLXSW_REG_PTYS_ETH_SPEED_10GBASE_KX4, .mask_ethtool = ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, .speed = SPEED_10000, + .width = ETHTOOL_LANES_4, }, { .mask = MLXSW_REG_PTYS_ETH_SPEED_10GBASE_KR | @@ -1103,71 +1106,85 @@ static const struct mlxsw_sp1_port_link_mode mlxsw_sp1_port_link_mode[] = { MLXSW_REG_PTYS_ETH_SPEED_10GBASE_ER_LR, .mask_ethtool = ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, .speed = SPEED_10000, + .width = ETHTOOL_LANES_1, }, { .mask = MLXSW_REG_PTYS_ETH_SPEED_40GBASE_CR4, .mask_ethtool = ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT, .speed = SPEED_40000, + .width = ETHTOOL_LANES_4, }, { .mask = MLXSW_REG_PTYS_ETH_SPEED_40GBASE_KR4, .mask_ethtool = ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT, .speed = SPEED_40000, + .width = ETHTOOL_LANES_4, }, { .mask = MLXSW_REG_PTYS_ETH_SPEED_40GBASE_SR4, .mask_ethtool = ETHTOOL_LINK_MODE_40000baseSR4_Full_BIT, .speed = SPEED_40000, + .width = ETHTOOL_LANES_4, }, { .mask = MLXSW_REG_PTYS_ETH_SPEED_40GBASE_LR4_ER4, .mask_ethtool = ETHTOOL_LINK_MODE_40000baseLR4_Full_BIT, .speed = SPEED_40000, + .width = ETHTOOL_LANES_4, }, { .mask = MLXSW_REG_PTYS_ETH_SPEED_25GBASE_CR, .mask_ethtool = ETHTOOL_LINK_MODE_25000baseCR_Full_BIT, .speed = SPEED_25000, + .width = ETHTOOL_LANES_1, }, { .mask = MLXSW_REG_PTYS_ETH_SPEED_25GBASE_KR, .mask_ethtool = ETHTOOL_LINK_MODE_25000baseKR_Full_BIT, .speed = SPEED_25000, + .width = ETHTOOL_LANES_1, }, { .mask = MLXSW_REG_PTYS_ETH_SPEED_25GBASE_SR, .mask_ethtool = ETHTOOL_LINK_MODE_25000baseSR_Full_BIT, .speed = SPEED_25000, + .width = ETHTOOL_LANES_1, }, { .mask = MLXSW_REG_PTYS_ETH_SPEED_50GBASE_CR2, .mask_ethtool = ETHTOOL_LINK_MODE_50000baseCR2_Full_BIT, .speed = SPEED_50000, + .width = ETHTOOL_LANES_2, }, { .mask = MLXSW_REG_PTYS_ETH_SPEED_50GBASE_KR2, .mask_ethtool = ETHTOOL_LINK_MODE_50000baseKR2_Full_BIT, .speed = SPEED_50000, + .width = ETHTOOL_LANES_2, }, { .mask = MLXSW_REG_PTYS_ETH_SPEED_50GBASE_SR2, .mask_ethtool = ETHTOOL_LINK_MODE_50000baseSR2_Full_BIT, .speed = SPEED_50000, + .width = ETHTOOL_LANES_2, }, { .mask = MLXSW_REG_PTYS_ETH_SPEED_100GBASE_CR4, .mask_ethtool = ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT, .speed = SPEED_100000, + .width = ETHTOOL_LANES_4, }, { .mask = MLXSW_REG_PTYS_ETH_SPEED_100GBASE_SR4, .mask_ethtool = ETHTOOL_LINK_MODE_100000baseSR4_Full_BIT, .speed = SPEED_100000, + .width = ETHTOOL_LANES_4, }, { .mask = MLXSW_REG_PTYS_ETH_SPEED_100GBASE_KR4, .mask_ethtool = ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT, .speed = SPEED_100000, + .width = ETHTOOL_LANES_4, }, }; @@ -1220,20 +1237,36 @@ mlxsw_sp1_from_ptys_speed(struct mlxsw_sp *mlxsw_sp, u32 ptys_eth_proto) return SPEED_UNKNOWN; } +static u32 +mlxsw_sp1_from_ptys_lanes(struct mlxsw_sp *mlxsw_sp, u32 ptys_eth_proto) +{ + int i; + + for (i = 0; i < MLXSW_SP1_PORT_LINK_MODE_LEN; i++) { + if (ptys_eth_proto & mlxsw_sp1_port_link_mode[i].mask) + return mlxsw_sp1_port_link_mode[i].width; + } + + return ETHTOOL_LANES_UNKNOWN; +} + static void -mlxsw_sp1_from_ptys_speed_duplex(struct mlxsw_sp *mlxsw_sp, bool carrier_ok, - u32 ptys_eth_proto, - struct ethtool_link_ksettings *cmd) +mlxsw_sp1_from_ptys_speed_lanes_duplex(struct mlxsw_sp *mlxsw_sp, bool carrier_ok, + u32 ptys_eth_proto, + struct ethtool_link_ksettings *cmd) { cmd->base.speed = SPEED_UNKNOWN; + cmd->lanes = ETHTOOL_LANES_UNKNOWN; cmd->base.duplex = DUPLEX_UNKNOWN; if (!carrier_ok) return; cmd->base.speed = mlxsw_sp1_from_ptys_speed(mlxsw_sp, ptys_eth_proto); - if (cmd->base.speed != SPEED_UNKNOWN) + if (cmd->base.speed != SPEED_UNKNOWN) { + cmd->lanes = mlxsw_sp1_from_ptys_lanes(mlxsw_sp, ptys_eth_proto); cmd->base.duplex = DUPLEX_FULL; + } } static int mlxsw_sp1_ptys_max_speed(struct mlxsw_sp_port *mlxsw_sp_port, u32 *p_max_speed) @@ -1308,7 +1341,7 @@ const struct mlxsw_sp_port_type_speed_ops mlxsw_sp1_port_type_speed_ops = { .from_ptys_supported_port = mlxsw_sp1_from_ptys_supported_port, .from_ptys_link = mlxsw_sp1_from_ptys_link, .from_ptys_speed = mlxsw_sp1_from_ptys_speed, - .from_ptys_speed_duplex = mlxsw_sp1_from_ptys_speed_duplex, + .from_ptys_speed_lanes_duplex = mlxsw_sp1_from_ptys_speed_lanes_duplex, .ptys_max_speed = mlxsw_sp1_ptys_max_speed, .to_ptys_advert_link = mlxsw_sp1_to_ptys_advert_link, .to_ptys_speed_lanes = mlxsw_sp1_to_ptys_speed_lanes, @@ -1629,20 +1662,46 @@ mlxsw_sp2_from_ptys_speed(struct mlxsw_sp *mlxsw_sp, u32 ptys_eth_proto) return SPEED_UNKNOWN; } +static u32 +mlxsw_sp2_from_ptys_lanes(struct mlxsw_sp *mlxsw_sp, u32 ptys_eth_proto) +{ + u8 width; + int i; + + for (i = 0; i < MLXSW_SP2_PORT_LINK_MODE_LEN; i++) { + if (ptys_eth_proto & mlxsw_sp2_port_link_mode[i].mask) { + width = mlxsw_sp2_port_link_mode[i].mask_width; + if (width & MLXSW_SP_PORT_MASK_WIDTH_1X) + return ETHTOOL_LANES_1; + else if (width & MLXSW_SP_PORT_MASK_WIDTH_2X) + return ETHTOOL_LANES_2; + else if (width & MLXSW_SP_PORT_MASK_WIDTH_4X) + return ETHTOOL_LANES_4; + else if (width & MLXSW_SP_PORT_MASK_WIDTH_8X) + return ETHTOOL_LANES_8; + } + } + + return ETHTOOL_LANES_UNKNOWN; +} + static void -mlxsw_sp2_from_ptys_speed_duplex(struct mlxsw_sp *mlxsw_sp, bool carrier_ok, - u32 ptys_eth_proto, - struct ethtool_link_ksettings *cmd) +mlxsw_sp2_from_ptys_speed_lanes_duplex(struct mlxsw_sp *mlxsw_sp, bool carrier_ok, + u32 ptys_eth_proto, + struct ethtool_link_ksettings *cmd) { cmd->base.speed = SPEED_UNKNOWN; + cmd->lanes = ETHTOOL_LANES_UNKNOWN; cmd->base.duplex = DUPLEX_UNKNOWN; if (!carrier_ok) return; cmd->base.speed = mlxsw_sp2_from_ptys_speed(mlxsw_sp, ptys_eth_proto); - if (cmd->base.speed != SPEED_UNKNOWN) + if (cmd->base.speed != SPEED_UNKNOWN) { + cmd->lanes = mlxsw_sp2_from_ptys_lanes(mlxsw_sp, ptys_eth_proto); cmd->base.duplex = DUPLEX_FULL; + } } static int mlxsw_sp2_ptys_max_speed(struct mlxsw_sp_port *mlxsw_sp_port, u32 *p_max_speed) @@ -1744,7 +1803,7 @@ const struct mlxsw_sp_port_type_speed_ops mlxsw_sp2_port_type_speed_ops = { .from_ptys_supported_port = mlxsw_sp2_from_ptys_supported_port, .from_ptys_link = mlxsw_sp2_from_ptys_link, .from_ptys_speed = mlxsw_sp2_from_ptys_speed, - .from_ptys_speed_duplex = mlxsw_sp2_from_ptys_speed_duplex, + .from_ptys_speed_lanes_duplex = mlxsw_sp2_from_ptys_speed_lanes_duplex, .ptys_max_speed = mlxsw_sp2_ptys_max_speed, .to_ptys_advert_link = mlxsw_sp2_to_ptys_advert_link, .to_ptys_speed_lanes = mlxsw_sp2_to_ptys_speed_lanes, -- 2.26.2 ^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH net-next 6/6] net: selftests: Add lanes setting test 2020-10-10 15:41 [PATCH net-next 0/6] Support setting lanes via ethtool Ido Schimmel ` (4 preceding siblings ...) 2020-10-10 15:41 ` [PATCH net-next 5/6] mlxsw: ethtool: Expose the number of lanes in use Ido Schimmel @ 2020-10-10 15:41 ` Ido Schimmel 5 siblings, 0 replies; 45+ messages in thread From: Ido Schimmel @ 2020-10-10 15:41 UTC (permalink / raw) To: netdev Cc: davem, kuba, jiri, danieller, andrew, f.fainelli, mkubecek, mlxsw, Ido Schimmel From: Danielle Ratson <danieller@nvidia.com> Test that setting lanes parameter is working. First, test that when setting only speed, max width is chosen as the number of lanes. Second, set max speed and max lanes in the list of advertised link modes, and then try to set max speed with the lanes below max lanes if exists in the list. And last, test that setting number of lanes larger than max lanes fails. Do the above for both autoneg on and off. $ ./ethtool_lanes.sh TEST: When no lanes setting, max width is set [ OK ] TEST: 4 lanes is autonegotiated [ OK ] TEST: Lanes number larger then max_width is not set [ OK ] TEST: Autoneg off, 4 lanes detected during force mode [ OK ] TEST: Lanes number larger then max width is not set [ OK ] Signed-off-by: Danielle Ratson <danieller@nvidia.com> Signed-off-by: Ido Schimmel <idosch@nvidia.com> --- .../selftests/net/forwarding/ethtool_lanes.sh | 224 ++++++++++++++++++ .../selftests/net/forwarding/ethtool_lib.sh | 34 +++ tools/testing/selftests/net/forwarding/lib.sh | 28 +++ 3 files changed, 286 insertions(+) create mode 100755 tools/testing/selftests/net/forwarding/ethtool_lanes.sh diff --git a/tools/testing/selftests/net/forwarding/ethtool_lanes.sh b/tools/testing/selftests/net/forwarding/ethtool_lanes.sh new file mode 100755 index 000000000000..33457505df85 --- /dev/null +++ b/tools/testing/selftests/net/forwarding/ethtool_lanes.sh @@ -0,0 +1,224 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 + +ALL_TESTS=" + max_width_no_lanes_set + autoneg + autoneg_force_mode +" + +NUM_NETIFS=2 +: ${TIMEOUT:=30000} # ms +source lib.sh +source ethtool_lib.sh + +setup_prepare() +{ + swp1=${NETIFS[p1]} + swp2=${NETIFS[p2]} + + ip link set dev $swp1 up + ip link set dev $swp2 up + + busywait "$TIMEOUT" wait_for_port_up ethtool $swp2 + check_err $? "ports did not come up" + + local chosen_lanes=$(ethtool $swp1 | grep 'Lanes:') + chosen_lanes=${chosen_lanes#*"Lanes: "} + if [[ $chosen_lanes == "Unknown!" ]]; then + log_test "SKIP: driver does not support lanes setting" + exit 1 + fi + + ip link set dev $swp2 down + ip link set dev $swp1 down +} + +check_lanes() +{ + local dev=$1; shift + local lanes=$1; shift + local max_speed=$1; shift + local chosen_lanes + + chosen_lanes=$(ethtool $dev | grep 'Lanes:') + chosen_lanes=${chosen_lanes#*"Lanes: "} + + ((chosen_lanes == lanes)) + check_err $? "swp1 advertise $max_speed and $lanes, devs sync to $chosen_lanes" +} + +check_unsupported_lanes() +{ + local dev=$1; shift + local max_speed=$1; shift + local max_lanes=$1; shift + local autoneg=$1; shift + local autoneg_str="" + + local unsupported_lanes=$((max_lanes *= 2)) + + if [[ $autoneg -eq 0 ]]; then + autoneg_str="autoneg off" + fi + + ethtool -s $swp1 speed $max_speed lanes $unsupported_lanes $autoneg_str &> /dev/null + check_fail $? "Unsuccessful $unsupported_lanes lanes setting was expected" +} + +max_speed_and_lanes_get() +{ + local dev=$1; shift + local arr=("$@") + local max_lanes + local max_speed + local -a lanes_arr + local -a speeds_arr + local -a max_values + + for ((i=0; i<${#arr[@]}; i+=2)); do + speeds_arr+=("${arr[$i]}") + lanes_arr+=("${arr[i+1]}") + done + + max_values+=($(get_max "${speeds_arr[@]}")) + max_values+=($(get_max "${lanes_arr[@]}")) + + echo ${max_values[@]} +} + +search_linkmode() +{ + local speed=$1; shift + local lanes=$1; shift + local arr=("$@") + + for ((i=0; i<${#arr[@]}; i+=2)); do + if [[ $speed -eq ${arr[$i]} && $lanes -eq ${arr[i+1]} ]]; then + return 1 + fi + done + return 0 +} + +max_width_no_lanes_set() +{ + local max_speed + local chosen_lanes + local max_lanes_for_speed=0 + + local -a linkmodes_params=($(dev_linkmodes_params_get $swp1 1)) + local -a max_values=($(max_speed_and_lanes_get $swp1 "${linkmodes_params[@]}")) + max_speed=${max_values[0]} + + ethtool_set $swp1 speed $max_speed + ip link set dev $swp1 up + ip link set dev $swp2 up + + busywait "$TIMEOUT" wait_for_port_up ethtool $swp2 + check_err $? "ports did not come up" + + chosen_lanes=$(ethtool $swp1 | grep 'Lanes:') + chosen_lanes=${chosen_lanes#*"Lanes: "} + + for ((i=0; i<${#linkmodes_params[@]}; i+=2)); do + if [[ $max_speed == ${linkmodes_params[$i]} ]]; then + if [[ ${linkmodes_params[i+1]} -gt $max_lanes_for_speed ]]; then + max_lanes_for_speed=${linkmodes_params[i+1]} + fi + fi + done + + (( chosen_lanes == max_lanes_for_speed )) + check_err $? "devs did not sync to max width $max_lanes_for_speed" + + log_test "When no lanes setting, max width is set" + + ip link set dev $swp2 down + ip link set dev $swp1 down +} + +autoneg() +{ + RET=0 + + local lanes + local max_speed + local max_lanes + + local -a linkmodes_params=($(dev_linkmodes_params_get $swp1 1)) + local -a max_values=($(max_speed_and_lanes_get $swp1 "${linkmodes_params[@]}")) + max_speed=${max_values[0]} + max_lanes=${max_values[1]} + + lanes=$max_lanes + + while [[ $lanes -ge 1 ]]; do + search_linkmode $max_speed $lanes "${linkmodes_params[@]}" + if [[ $? -eq 1 ]]; then + ethtool_set $swp1 speed $max_speed lanes $lanes + ip link set dev $swp1 up + ip link set dev $swp2 up + busywait "$TIMEOUT" wait_for_port_up ethtool $swp2 + check_err $? "ports did not come up" + + check_lanes $swp1 $lanes $max_speed + log_test "$lanes lanes is autonegotiated" + fi + let $((lanes /= 2)) + done + + check_unsupported_lanes $swp1 $max_speed $max_lanes 1 + log_test "Lanes number larger then max_width is not set" + + ip link set dev $swp2 down + ip link set dev $swp1 down +} + +autoneg_force_mode() +{ + RET=0 + + local lanes + local max_speed + local max_lanes + + local -a linkmodes_params=($(dev_linkmodes_params_get $swp1 1)) + local -a max_values=($(max_speed_and_lanes_get $swp1 "${linkmodes_params[@]}")) + max_speed=${max_values[0]} + max_lanes=${max_values[1]} + + lanes=$max_lanes + + while [[ $lanes -ge 1 ]]; do + search_linkmode $max_speed $lanes "${linkmodes_params[@]}" + if [[ $? -eq 1 ]]; then + ethtool_set $swp1 speed $max_speed lanes $lanes autoneg off + ethtool_set $swp2 speed $max_speed lanes $lanes autoneg off + ip link set dev $swp1 up + ip link set dev $swp2 up + busywait "$TIMEOUT" wait_for_port_up ethtool $swp2 + check_err $? "ports did not come up" + + check_lanes $swp1 $lanes $max_speed + log_test "Autoneg off, $lanes lanes detected during force mode" + fi + let $((lanes /= 2)) + done + + check_unsupported_lanes $swp1 $max_speed $max_lanes 0 + log_test "Lanes number larger then max width is not set" + + ip link set dev $swp2 down + ip link set dev $swp1 down + + ethtool -s $swp2 autoneg on + ethtool -s $swp1 autoneg on +} + +check_ethtool_lanes_support +setup_prepare + +tests_run + +exit $EXIT_STATUS diff --git a/tools/testing/selftests/net/forwarding/ethtool_lib.sh b/tools/testing/selftests/net/forwarding/ethtool_lib.sh index 9188e624dec0..b9bfb45085af 100644 --- a/tools/testing/selftests/net/forwarding/ethtool_lib.sh +++ b/tools/testing/selftests/net/forwarding/ethtool_lib.sh @@ -22,6 +22,40 @@ ethtool_set() check_err $out "error in configuration. $cmd" } +dev_linkmodes_params_get() +{ + local dev=$1; shift + local adver=$1; shift + local -a linkmodes_params + local param_count + local arr + + if (($adver)); then + mode="Advertised link modes" + else + mode="Supported link modes" + fi + + local -a dev_linkmodes=($(dev_speeds_get $dev 1 $adver)) + for ((i=0; i<${#dev_linkmodes[@]}; i++)); do + linkmodes_params[$i]=$(echo -e "${dev_linkmodes[$i]}" | \ + # Replaces all non numbers with spaces + sed -e 's/[^0-9]/ /g' | \ + # Squeeze spaces in sequence to 1 space + tr -s ' ') + # Count how many numbers were found in the linkmode + param_count=$(echo "${linkmodes_params[$i]}" | wc -w) + if [[ $param_count -eq 1 ]]; then + linkmodes_params[$i]="${linkmodes_params[$i]} 1" + elif [[ $param_count -ge 3 ]]; then + arr=(${linkmodes_params[$i]}) + # Take only first two params + linkmodes_params[$i]=$(echo "${arr[@]:0:2}") + fi + done + echo ${linkmodes_params[@]} +} + dev_speeds_get() { local dev=$1; shift diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh index 927f9ba49e08..f5e1585489cf 100644 --- a/tools/testing/selftests/net/forwarding/lib.sh +++ b/tools/testing/selftests/net/forwarding/lib.sh @@ -69,6 +69,15 @@ check_tc_action_hw_stats_support() fi } +check_ethtool_lanes_support() +{ + ethtool --help 2>&1| grep lanes &> /dev/null + if [[ $? -ne 0 ]]; then + echo "SKIP: iproute2 too old; ethtool is missing lanes support" + exit 1 + fi +} + if [[ "$(id -u)" -ne 0 ]]; then echo "SKIP: need root privileges" exit 0 @@ -263,6 +272,20 @@ not() [[ $? != 0 ]] } +get_max() +{ + local arr=("$@") + + max=${arr[0]} + for cur in ${arr[@]}; do + if [[ $cur -gt $max ]]; then + max=$cur + fi + done + + echo $max +} + grep_bridge_fdb() { local addr=$1; shift @@ -279,6 +302,11 @@ grep_bridge_fdb() $@ | grep $addr | grep $flag "$word" } +wait_for_port_up() +{ + "$@" | grep -q "Link detected: yes" +} + wait_for_offload() { "$@" | grep -q offload -- 2.26.2 ^ permalink raw reply related [flat|nested] 45+ messages in thread
end of thread, other threads:[~2020-12-02 17:55 UTC | newest] Thread overview: 45+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-10 15:41 [PATCH net-next 0/6] Support setting lanes via ethtool Ido Schimmel 2020-10-10 15:41 ` [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes Ido Schimmel 2020-10-11 22:37 ` Jakub Kicinski 2020-10-12 15:33 ` Danielle Ratson 2020-10-12 15:58 ` Jakub Kicinski 2020-10-13 14:29 ` Danielle Ratson 2020-10-13 15:43 ` Jakub Kicinski 2020-10-16 22:15 ` Andrew Lunn 2020-10-19 7:19 ` Danielle Ratson 2020-10-19 11:04 ` Michal Kubecek 2020-10-19 12:26 ` Jiri Pirko 2020-10-19 13:24 ` Michal Kubecek 2020-10-20 7:39 ` Danielle Ratson 2020-10-21 7:08 ` Michal Kubecek 2020-10-21 7:20 ` Danielle Ratson 2020-10-21 8:47 ` Michal Kubecek 2020-10-22 6:15 ` Danielle Ratson 2020-10-22 16:27 ` Michal Kubecek 2020-11-23 9:47 ` Danielle Ratson 2020-11-24 22:12 ` Michal Kubecek 2020-11-25 10:35 ` Danielle Ratson 2020-11-26 21:07 ` Michal Kubecek 2020-12-01 17:22 ` Danielle Ratson 2020-12-02 0:52 ` Edwin Peer 2020-12-02 1:17 ` Edwin Peer 2020-10-19 12:24 ` Jiri Pirko 2020-10-19 12:38 ` Andrew Lunn 2020-10-12 16:40 ` Michal Kubecek 2020-10-12 19:10 ` Johannes Berg 2020-10-12 20:08 ` Jakub Kicinski 2020-10-12 17:03 ` Michal Kubecek 2020-11-19 20:38 ` Edwin Peer 2020-11-23 9:40 ` Jiri Pirko 2020-11-30 17:01 ` Edwin Peer 2020-11-30 17:14 ` Jiri Pirko 2020-11-30 18:00 ` Edwin Peer 2020-12-01 11:22 ` Jiri Pirko 2020-12-02 0:32 ` Edwin Peer 2020-12-02 10:09 ` Jiri Pirko 2020-12-02 17:53 ` Edwin Peer 2020-10-10 15:41 ` [PATCH net-next 2/6] ethtool: Expose the number of lanes in use Ido Schimmel 2020-10-10 15:41 ` [PATCH net-next 3/6] mlxsw: ethtool: Remove max lanes filtering Ido Schimmel 2020-10-10 15:41 ` [PATCH net-next 4/6] mlxsw: ethtool: Add support for setting lanes when autoneg is off Ido Schimmel 2020-10-10 15:41 ` [PATCH net-next 5/6] mlxsw: ethtool: Expose the number of lanes in use Ido Schimmel 2020-10-10 15:41 ` [PATCH net-next 6/6] net: selftests: Add lanes setting test Ido Schimmel
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.