Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [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; 29+ 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] 29+ 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
  2020-10-12 17:03   ` Michal Kubecek
  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, 2 replies; 29+ 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	[flat|nested] 29+ 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; 29+ 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	[flat|nested] 29+ 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; 29+ 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	[flat|nested] 29+ 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; 29+ 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	[flat|nested] 29+ 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; 29+ 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	[flat|nested] 29+ 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; 29+ 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	[flat|nested] 29+ 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
  1 sibling, 2 replies; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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
  1 sibling, 0 replies; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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
  0 siblings, 0 replies; 29+ 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] 29+ messages in thread

end of thread, back to index

Thread overview: 29+ 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-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-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

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git