On Wed, 20 Jan 2021 11:37:07 +0200 Danielle Ratson wrote:
> 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.
> Signed-off-by: Danielle Ratson <daniel...@nvidia.com>
> 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?
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.
> /* 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.
> u8 duplex;
> };
> @@ -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?
Please use req_lanes variable where possible.
> +
> + 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).
> + } 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,
