-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>
Sent: Friday, January 22, 2021 5:45 AM
To: Danielle Ratson <redacted>
Cc: netdev@vger.kernel.org; davem@davemloft.net; Jiri Pirko <redacted>; andrew@lunn.ch; f.fainelli@gmail.com;
mkubecek@suse.cz; mlxsw [off-list ref]; Ido Schimmel [off-list ref]
Subject: Re: [PATCH net-next v3 1/7] ethtool: Extend link modes settings uAPI with lanes

On Wed, 20 Jan 2021 11:37:07 +0200 Danielle Ratson wrote:

quoted

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.

quoted

Signed-off-by: Danielle Ratson <redacted>

quoted

diff --git a/include/uapi/linux/ethtool.h

b/include/uapi/linux/ethtool.h index cde753bb2093..80edae2c24f7 100644

--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h

@@ -1738,6 +1738,8 @@ static inline int ethtool_validate_speed(__u32 speed)
 	return speed <= INT_MAX || speed == (__u32)SPEED_UNKNOWN;  }

+#define ETHTOOL_LANES_UNKNOWN		0

I already complained about these unnecessary uAPI constants, did you reply to that and I missed it?

I guess I missed it, sorry, will fix.

Don't report the nlattr if it's unknown, we have netlink now, those constants are from times when we returned structures and all
fields had to have a value.

quoted

 /* 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..fb7d73250864 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;
+	u32				lanes;

This is not uapi, we can make it u8 now, save a few (hundred?) bytes of memory and bump it to u16 later.

quoted

 	u8				duplex;
 };

quoted

@@ -353,10 +358,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]);

req_lanes == tb[ETHTOOL_A_LINKMODES_LANES], right? 

Yes, but req_lanes is a bool and doesn't fit to nla_get_u32. Do you want me to change the req_lanes type and name?

Please use req_lanes variable where possible.

quoted

+
+		if (!is_power_of_2(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->cap_link_lanes_supported) {
+			NL_SET_ERR_MSG_ATTR(info->extack,
+					    tb[ETHTOOL_A_LINKMODES_LANES],
+					    "lanes configuration not supported by device");
+			return -EOPNOTSUPP;
+		}

This validation does not depend on the current settings at all, it's just input validation, it can be done before rtnl_lock is taken (in a
new function).

You can move ethnl_validate_master_slave_cfg() to that function as well (as a cleanup before this patch).

Do you mean to move the ethnl_validate_master_slave_cfg() if from that function? Doesn't it depend on the current settings, as opposed to the supported lanes param that you wanted me to move as well?
Not sure I understand the second part of the request...

quoted

+	} 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,