Re: [PATCH net-next 5/5] ethtool: provide link mode names as a string set

Andrew Lunn
2019-12-09T20:15:26Z

On Mon, Dec 09, 2019 at 08:55:45PM +0100, Michal Kubecek wrote:
> Unlike e.g. netdev features, the ethtool ioctl interface requires link mode
> table to be in sync between kernel and userspace for userspace to be able
> to display and set all link modes supported by kernel. The way arbitrary
> length bitsets are implemented in netlink interface, this will be no longer
> needed.
>
> To allow userspace to access all link modes running kernel supports, add
> table of ethernet link mode names and make it available as a string set to
> userspace GET_STRSET requests. Add build time check to make sure names
> are defined for all modes declared in enum ethtool_link_mode_bit_indices.

Hi Michal

Having a build time check is a good idea. However, i don't see it in
the code. Please could you point it out.
>
> Once the string set is available, make it also accessible via ioctl.
>
> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
> ---
> include/linux/ethtool.h | 4 ++
> include/uapi/linux/ethtool.h | 2 +
> net/ethtool/common.c | 86 ++++++++++++++++++++++++++++++++++++
> net/ethtool/common.h | 2 +
> net/ethtool/ioctl.c | 5 +++
> 5 files changed, 99 insertions(+)
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 95991e4300bf..5caef65d93d6 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -102,6 +102,10 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
> #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name) \
> DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)
>
> +/* compose link mode index from speed, type and duplex */
> +#define ETHTOOL_LINK_MODE(speed, type, duplex) \
> + ETHTOOL_LINK_MODE_ ## speed ## base ## type ## _ ## duplex ## _BIT
> +
> /* drivers must ignore base.cmd and base.link_mode_masks_nwords
> * fields, but they are allowed to overwrite them (will be ignored).
> */
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index d4591792f0b4..f44155840b07 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -593,6 +593,7 @@ struct ethtool_pauseparam {
> * @ETH_SS_RSS_HASH_FUNCS: RSS hush function names
> * @ETH_SS_PHY_STATS: Statistic names, for use with %ETHTOOL_GPHYSTATS
> * @ETH_SS_PHY_TUNABLES: PHY tunable names
> + * @ETH_SS_LINK_MODES: link mode names
> */
> enum ethtool_stringset {
> ETH_SS_TEST = 0,
> @@ -604,6 +605,7 @@ enum ethtool_stringset {
> ETH_SS_TUNABLES,
> ETH_SS_PHY_STATS,
> ETH_SS_PHY_TUNABLES,
> + ETH_SS_LINK_MODES,
> };
>
> /**
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index 220d6b539180..be1b26970eb1 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -83,3 +83,89 @@ phy_tunable_strings[__ETHTOOL_PHY_TUNABLE_COUNT][ETH_GSTRING_LEN] = {
> [ETHTOOL_PHY_FAST_LINK_DOWN] = "phy-fast-link-down",
> [ETHTOOL_PHY_EDPD] = "phy-energy-detect-power-down",
> };
> +
> +#define __LINK_MODE_NAME(speed, type, duplex) \
> + #speed "base" #type "/" #duplex
> +#define __DEFINE_LINK_MODE_NAME(speed, type, duplex) \
> + [ETHTOOL_LINK_MODE(speed, type, duplex)] = \
> + __LINK_MODE_NAME(speed, type, duplex)
> +#define __DEFINE_SPECIAL_MODE_NAME(_mode, _name) \
> + [ETHTOOL_LINK_MODE_ ## _mode ## _BIT] = _name
> +
> +const char
> +link_mode_names[__ETHTOOL_LINK_MODE_MASK_NBITS][ETH_GSTRING_LEN] = {
> + __DEFINE_LINK_MODE_NAME(10, T, Half),
> + __DEFINE_LINK_MODE_NAME(10, T, Full),
> + __DEFINE_LINK_MODE_NAME(100, T, Half),
> + __DEFINE_LINK_MODE_NAME(100, T, Full),
> + __DEFINE_LINK_MODE_NAME(1000, T, Half),
> + __DEFINE_LINK_MODE_NAME(1000, T, Full),
> + __DEFINE_SPECIAL_MODE_NAME(Autoneg, "Autoneg"),
> + __DEFINE_SPECIAL_MODE_NAME(TP, "TP"),
> + __DEFINE_SPECIAL_MODE_NAME(AUI, "AUI"),
> + __DEFINE_SPECIAL_MODE_NAME(MII, "MII"),
> + __DEFINE_SPECIAL_MODE_NAME(FIBRE, "FIBRE"),
> + __DEFINE_SPECIAL_MODE_NAME(BNC, "BNC"),
> + __DEFINE_LINK_MODE_NAME(10000, T, Full),
> + __DEFINE_SPECIAL_MODE_NAME(Pause, "Pause"),
> + __DEFINE_SPECIAL_MODE_NAME(Asym_Pause, "Asym_Pause"),
> + __DEFINE_LINK_MODE_NAME(2500, X, Full),
> + __DEFINE_SPECIAL_MODE_NAME(Backplane, "Backplane"),
> + __DEFINE_LINK_MODE_NAME(1000, KX, Full),
> + __DEFINE_LINK_MODE_NAME(10000, KX4, Full),
> + __DEFINE_LINK_MODE_NAME(10000, KR, Full),
> + [ETHTOOL_LINK_MODE_10000baseR_FEC_BIT] = "10000baseR_FEC",

Would

__DEFINE_SPECIAL_MODE_NAME(10000baseR_FEC, 10000baseR_FEC),

work here?

Andrew