Re: [PATCH net-next repost v2 2/7] ethtool: Get link mode in use instead of speed and duplex parameters

[Jakub Kicinski <kuba@kernel.org>]

On Wed,  6 Jan 2021 15:06:17 +0200 Danielle Ratson wrote:
> From: Danielle Ratson <danieller@nvidia.com>
> 
> Currently, when user space queries the link's parameters, as speed and
> duplex, each parameter is passed from the driver to ethtool.
> 
> Instead, get the link mode bit in use, and derive each of the parameters
> from it in ethtool.
> 
> Signed-off-by: Danielle Ratson <danieller@nvidia.com>
> ---
>  Documentation/networking/ethtool-netlink.rst |   1 +
>  include/linux/ethtool.h                      |   1 +
>  include/uapi/linux/ethtool.h                 |   2 +
>  net/ethtool/linkmodes.c                      | 252 ++++++++++---------
>  4 files changed, 137 insertions(+), 119 deletions(-)
> 
> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index 05073482db05..c21e71e0c0e8 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -406,6 +406,7 @@ Kernel response contents:
>    ``ETHTOOL_A_LINKMODES_PEER``                bitset  partner link modes
>    ``ETHTOOL_A_LINKMODES_SPEED``               u32     link speed (Mb/s)
>    ``ETHTOOL_A_LINKMODES_DUPLEX``              u8      duplex mode
> +  ``ETHTOOL_A_LINKMODES_LINK_MODE``           u8      link mode

Are there other places in the uapi we already limit ourselves to 
u8 / max 255? Otherwise u32 is better, the nlattr will be padded,
anyway.

>    ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG``    u8      Master/slave port mode
>    ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE``  u8      Master/slave port state
>    ==========================================  ======  ==========================
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index afae2beacbc3..668a7737a483 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -129,6 +129,7 @@ struct ethtool_link_ksettings {
>  		__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising);
>  	} link_modes;
>  	u32	lanes;
> +	enum ethtool_link_mode_bit_indices link_mode;
>  };
>  
>  /**
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 80edae2c24f7..f61f726d1567 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -1733,6 +1733,8 @@ enum ethtool_link_mode_bit_indices {
>  
>  #define SPEED_UNKNOWN		-1
>  
> +#define LINK_MODE_UNKNOWN	-1

Why do we need this? Link mode is output only, so just don't report 
the nlattr when it's unknown.

>  static inline int ethtool_validate_speed(__u32 speed)
>  {
>  	return speed <= INT_MAX || speed == (__u32)SPEED_UNKNOWN;


> @@ -29,7 +150,9 @@ static int linkmodes_prepare_data(const struct ethnl_req_info *req_base,
>  {
>  	struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
>  	struct net_device *dev = reply_base->dev;
> +	const struct link_mode_info *link_info;
>  	int ret;
> +	unsigned int i;

rev xmas tree

>  	data->lsettings = &data->ksettings.base;
>  
> @@ -43,6 +166,16 @@ static int linkmodes_prepare_data(const struct ethnl_req_info *req_base,
>  		goto out;
>  	}
>  
> +	if (data->ksettings.link_mode) {
> +		for (i = 0; i < __ETHTOOL_LINK_MODE_MASK_NBITS; i++) {
> +			if (data->ksettings.link_mode == i) {

I stared at this for a minute, the loop is entirely pointless, right?

> +				link_info = &link_mode_params[i];
> +				data->lsettings->speed = link_info->speed;
> +				data->lsettings->duplex = link_info->duplex;
> +			}
> +		}
> +	}
> +
>  	data->peer_empty =
>  		bitmap_empty(data->ksettings.link_modes.lp_advertising,
>  			     __ETHTOOL_LINK_MODE_MASK_NBITS);