On Wed, 6 Jan 2021 15:06:17 +0200 Danielle Ratson wrote:
> From: Danielle Ratson <[email protected]>
>
> 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 <[email protected]>
> ---
> 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);
