As Jakub Kicinski suggested in ethtool netlink v7 discussion, this
submission consists only of preliminary patches which raised no objections;
first four patches already have Acked-by or Reviewed-by.
- patch 1 exposes permanent hardware address (as shown by "ethtool -P")
via rtnetlink
- patch 2 is renames existing netlink helper to a better name
- patch 3 and 4 reorganize existing ethtool code (no functional change)
- patch 5 makes the table of link mode names available as an ethtool string
set (will be needed for the netlink interface)
Once we get these out of the way, v8 of the first part of the ethtool
netlink interface will follow.
Michal Kubecek (5):
rtnetlink: provide permanent hardware address in RTM_NEWLINK
netlink: rename nl80211_validate_nested() to nla_validate_nested()
ethtool: move to its own directory
ethtool: move string arrays into common file
ethtool: provide link mode names as a string set
include/linux/ethtool.h | 4 +
include/net/netlink.h | 8 +-
include/uapi/linux/ethtool.h | 2 +
include/uapi/linux/if_link.h | 1 +
net/Makefile | 2 +-
net/core/Makefile | 2 +-
net/core/rtnetlink.c | 5 +
net/ethtool/Makefile | 3 +
net/ethtool/common.c | 171 ++++++++++++++++++++++++
net/ethtool/common.h | 19 +++
net/{core/ethtool.c => ethtool/ioctl.c} | 89 +-----------
net/wireless/nl80211.c | 3 +-
12 files changed, 219 insertions(+), 90 deletions(-)
create mode 100644 net/ethtool/Makefile
create mode 100644 net/ethtool/common.c
create mode 100644 net/ethtool/common.h
rename net/{core/ethtool.c => ethtool/ioctl.c} (95%)
--
2.24.0
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.
Once the string set is available, make it also accessible via ioctl.
Signed-off-by: Michal Kubecek <[email protected]>
---
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",
+ __DEFINE_LINK_MODE_NAME(20000, MLD2, Full),
+ __DEFINE_LINK_MODE_NAME(20000, KR2, Full),
+ __DEFINE_LINK_MODE_NAME(40000, KR4, Full),
+ __DEFINE_LINK_MODE_NAME(40000, CR4, Full),
+ __DEFINE_LINK_MODE_NAME(40000, SR4, Full),
+ __DEFINE_LINK_MODE_NAME(40000, LR4, Full),
+ __DEFINE_LINK_MODE_NAME(56000, KR4, Full),
+ __DEFINE_LINK_MODE_NAME(56000, CR4, Full),
+ __DEFINE_LINK_MODE_NAME(56000, SR4, Full),
+ __DEFINE_LINK_MODE_NAME(56000, LR4, Full),
+ __DEFINE_LINK_MODE_NAME(25000, CR, Full),
+ __DEFINE_LINK_MODE_NAME(25000, KR, Full),
+ __DEFINE_LINK_MODE_NAME(25000, SR, Full),
+ __DEFINE_LINK_MODE_NAME(50000, CR2, Full),
+ __DEFINE_LINK_MODE_NAME(50000, KR2, Full),
+ __DEFINE_LINK_MODE_NAME(100000, KR4, Full),
+ __DEFINE_LINK_MODE_NAME(100000, SR4, Full),
+ __DEFINE_LINK_MODE_NAME(100000, CR4, Full),
+ __DEFINE_LINK_MODE_NAME(100000, LR4_ER4, Full),
+ __DEFINE_LINK_MODE_NAME(50000, SR2, Full),
+ __DEFINE_LINK_MODE_NAME(1000, X, Full),
+ __DEFINE_LINK_MODE_NAME(10000, CR, Full),
+ __DEFINE_LINK_MODE_NAME(10000, SR, Full),
+ __DEFINE_LINK_MODE_NAME(10000, LR, Full),
+ __DEFINE_LINK_MODE_NAME(10000, LRM, Full),
+ __DEFINE_LINK_MODE_NAME(10000, ER, Full),
+ __DEFINE_LINK_MODE_NAME(2500, T, Full),
+ __DEFINE_LINK_MODE_NAME(5000, T, Full),
+ __DEFINE_SPECIAL_MODE_NAME(FEC_NONE, "None"),
+ __DEFINE_SPECIAL_MODE_NAME(FEC_RS, "RS"),
+ __DEFINE_SPECIAL_MODE_NAME(FEC_BASER, "BASER"),
+ __DEFINE_LINK_MODE_NAME(50000, KR, Full),
+ __DEFINE_LINK_MODE_NAME(50000, SR, Full),
+ __DEFINE_LINK_MODE_NAME(50000, CR, Full),
+ __DEFINE_LINK_MODE_NAME(50000, LR_ER_FR, Full),
+ __DEFINE_LINK_MODE_NAME(50000, DR, Full),
+ __DEFINE_LINK_MODE_NAME(100000, KR2, Full),
+ __DEFINE_LINK_MODE_NAME(100000, SR2, Full),
+ __DEFINE_LINK_MODE_NAME(100000, CR2, Full),
+ __DEFINE_LINK_MODE_NAME(100000, LR2_ER2_FR2, Full),
+ __DEFINE_LINK_MODE_NAME(100000, DR2, Full),
+ __DEFINE_LINK_MODE_NAME(200000, KR4, Full),
+ __DEFINE_LINK_MODE_NAME(200000, SR4, Full),
+ __DEFINE_LINK_MODE_NAME(200000, LR4_ER4_FR4, Full),
+ __DEFINE_LINK_MODE_NAME(200000, DR4, Full),
+ __DEFINE_LINK_MODE_NAME(200000, CR4, Full),
+ __DEFINE_LINK_MODE_NAME(100, T1, Full),
+ __DEFINE_LINK_MODE_NAME(1000, T1, Full),
+ __DEFINE_LINK_MODE_NAME(400000, KR8, Full),
+ __DEFINE_LINK_MODE_NAME(400000, SR8, Full),
+ __DEFINE_LINK_MODE_NAME(400000, LR8_ER8_FR8, Full),
+ __DEFINE_LINK_MODE_NAME(400000, DR8, Full),
+ __DEFINE_LINK_MODE_NAME(400000, CR8, Full),
+};
diff --git a/net/ethtool/common.h b/net/ethtool/common.h
index 41b2efc1e4e1..351e019b8d85 100644
--- a/net/ethtool/common.h
+++ b/net/ethtool/common.h
@@ -13,5 +13,7 @@ extern const char
tunable_strings[__ETHTOOL_TUNABLE_COUNT][ETH_GSTRING_LEN];
extern const char
phy_tunable_strings[__ETHTOOL_PHY_TUNABLE_COUNT][ETH_GSTRING_LEN];
+extern const char
+link_mode_names[__ETHTOOL_LINK_MODE_MASK_NBITS][ETH_GSTRING_LEN];
#endif /* _ETHTOOL_COMMON_H */
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index b262db5a1d91..9274d70c496b 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -154,6 +154,9 @@ static int __ethtool_get_sset_count(struct net_device *dev, int sset)
!ops->get_ethtool_phy_stats)
return phy_ethtool_get_sset_count(dev->phydev);
+ if (sset == ETH_SS_LINK_MODES)
+ return __ETHTOOL_LINK_MODE_MASK_NBITS;
+
if (ops->get_sset_count && ops->get_strings)
return ops->get_sset_count(dev, sset);
else
@@ -178,6 +181,8 @@ static void __ethtool_get_strings(struct net_device *dev,
else if (stringset == ETH_SS_PHY_STATS && dev->phydev &&
!ops->get_ethtool_phy_stats)
phy_ethtool_get_strings(dev->phydev, data);
+ else if (stringset == ETH_SS_LINK_MODES)
+ memcpy(data, link_mode_names, sizeof(link_mode_names));
else
/* ops->get_strings is valid because checked earlier */
ops->get_strings(dev, stringset, data);
--
2.24.0
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 <[email protected]>
> ---
> 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
On Mon, Dec 09, 2019 at 09:15:11PM +0100, Andrew Lunn wrote:
> 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.
Thanks for noticing. The BUILD_BUG_ON() check got lost when I moved the
patch to an earlier place and changed the declaration to fixed size
array like other name tables have. But you are right, losing the check
would be too high price for the uniformity.
> > +#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?
Right, it would (with double quotes for second argument). As I wrote the
macro for bits which don't represent actual link modes, it never occured
to me that it can be also used for this special case.
I'll wait until tomorrow to see if there are more comments and send v2.
Michal