| Message ID | 20250209190827.29128-7-gerhard@engleder-embedded.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Support loopback mode speed selection | expand |
On Sun, Feb 09, 2025 at 08:08:26PM +0100, Gerhard Engleder wrote: > net_selftests() provides a generic set of selftests for netdevs with > PHY. Those selftests rely on an existing link to inherit the speed for > the loopback mode. > > net_selftests() is not designed to extend existing selftests of drivers, > but with net_test_phy_loopback_* it contains useful test infrastructure. It might not of originally been designed for that, but i think it can be used as an extension. I've done the same for statistics, which uses the same API. For get_sset_count() you call net_selftest_get_count() and then add on the number of driver specific tests. For ethtool_get_strings() first call net_selftest_get_strings() and then append the driver tests afterwards. For self_test, first call net_selftest() and then do the driver specific tests. I also think you can extend these tests to cover different speeds. There are plenty of ethtool_test_flags bit free, so you can use one of them to indicate the reserved value in ethtool_test contains a speed. Everybody then gains from your work. Andrew
On 12.02.25 03:23, Andrew Lunn wrote: > On Sun, Feb 09, 2025 at 08:08:26PM +0100, Gerhard Engleder wrote: >> net_selftests() provides a generic set of selftests for netdevs with >> PHY. Those selftests rely on an existing link to inherit the speed for >> the loopback mode. >> >> net_selftests() is not designed to extend existing selftests of drivers, >> but with net_test_phy_loopback_* it contains useful test infrastructure. > > It might not of originally been designed for that, but i think it can > be used as an extension. I've done the same for statistics, which uses > the same API. > > For get_sset_count() you call net_selftest_get_count() and then add on > the number of driver specific tests. For ethtool_get_strings() first > call net_selftest_get_strings() and then append the driver tests > afterwards. For self_test, first call net_selftest() and then do the > driver specific tests. > > I also think you can extend these tests to cover different speeds. > There are plenty of ethtool_test_flags bit free, so you can use one of > them to indicate the reserved value in ethtool_test contains a speed. > Everybody then gains from your work. Reusing the complete test set as extension is not feasible as the first test requires an existing link and for loopback test no link is necessary. But yes, I can do better and rework it to reusable tests. I'm not sure if I will use ethtool_test_flags as IMO ideally all tests run always to ensure easy use. Thank you for the review! Gerhard
> Reusing the complete test set as extension is not feasible as the first > test requires an existing link and for loopback test no link is > necessary. But yes, I can do better and rework it to reusable tests. > I'm not sure if I will use ethtool_test_flags as IMO ideally all tests > run always to ensure easy use. We try to ensure backwards/forwards compatibly between ethtool and the kernel. The point about ethtool_test_flags is that for older versions of ethtool, you have no idea what the reserved field contains. Has it always been set to zero? If there is a flag indicating reserved contains a value, you then know it is safe to use it. At some point, the API needs moving to netlink sockets. It then becomes a lot easier to add extra parameters, as attributes. There is also an interesting overlap here and: https://netdevconf.info/0x19/sessions/talk/open-source-tooling-for-phy-management-and-testing.html What you are doing is not too dissimilar, although the PRBS is an 802.3 standard concept and typically part of the PCS. There needs to be some sort of API for configuring the PRBS, maybe as part of ethtool self tests? If so, the API is going to need extensions. Andrew
On 12.02.25 21:46, Andrew Lunn wrote: >> Reusing the complete test set as extension is not feasible as the first >> test requires an existing link and for loopback test no link is >> necessary. But yes, I can do better and rework it to reusable tests. >> I'm not sure if I will use ethtool_test_flags as IMO ideally all tests >> run always to ensure easy use. > > We try to ensure backwards/forwards compatibly between ethtool and the > kernel. > > The point about ethtool_test_flags is that for older versions of > ethtool, you have no idea what the reserved field contains. Has it > always been set to zero? If there is a flag indicating reserved > contains a value, you then know it is safe to use it. I'm not sure if I understand the last sentence. Do you mean it is safe to use a flag that was previously marked as reserved if the clients did set it to zero, but for ethtool_test_flags this is not the case? > At some point, the API needs moving to netlink sockets. It then > becomes a lot easier to add extra parameters, as attributes. > > There is also an interesting overlap here and: > > https://netdevconf.info/0x19/sessions/talk/open-source-tooling-for-phy-management-and-testing.html > > What you are doing is not too dissimilar, although the PRBS is an > 802.3 standard concept and typically part of the PCS. There needs to > be some sort of API for configuring the PRBS, maybe as part of ethtool > self tests? If so, the API is going to need extensions. Sounds also similar to bit error testing of transceivers. I want to stress a data link with all supported modes and this is similar. For me it is sufficient if the driver tests all supported modes, without any configuration from user space. But extending the API to enable advanced testing during development and production is reasonable. Gerhard
On Wed, Feb 12, 2025 at 10:36:00PM +0100, Gerhard Engleder wrote: > On 12.02.25 21:46, Andrew Lunn wrote: > > > Reusing the complete test set as extension is not feasible as the first > > > test requires an existing link and for loopback test no link is > > > necessary. But yes, I can do better and rework it to reusable tests. > > > I'm not sure if I will use ethtool_test_flags as IMO ideally all tests > > > run always to ensure easy use. > > > > We try to ensure backwards/forwards compatibly between ethtool and the > > kernel. > > > > The point about ethtool_test_flags is that for older versions of > > ethtool, you have no idea what the reserved field contains. Has it > > always been set to zero? If there is a flag indicating reserved > > contains a value, you then know it is safe to use it. > > I'm not sure if I understand the last sentence. Do you mean it is safe > to use a flag that was previously marked as reserved if the clients did > set it to zero, but for ethtool_test_flags this is not the case? We have: struct ethtool_test { __u32 cmd; __u32 flags; __u32 reserved; __u32 len; __u64 data[]; }; where flags takes a bitmap from: enum ethtool_test_flags { ETH_TEST_FL_OFFLINE = (1 << 0), ETH_TEST_FL_FAILED = (1 << 1), ETH_TEST_FL_EXTERNAL_LB = (1 << 2), ETH_TEST_FL_EXTERNAL_LB_DONE = (1 << 3), }; I've not looked at ethtool, but it is possible the struct ethtool_test is on the stack, or the heap, and not zeroed. So reserved contains random junk. flags is however given a value, bad things would happen otherwise. So we know the bits which are currently unused should be zero. A new flag can be added, which indicates user space wants to set the speed, and that the speed is in the reserved field. Without the flag being set, reserved contains random junk, with it set, we know we have a version of ethtool which sets it to a specific value. It might however be that we cannot rename reserved, it is now part of the kAPI, and changing it to another name would break the compilation of something. That is one of the advantages of netlink API, you can add new attributes without having to worry about breaking older builds. Unfortunately, the self test is still using the IOCTL, it has not been converted to netlink. Andrew
On Wed, Feb 12, 2025 at 11:34:14PM +0100, Andrew Lunn wrote: > It might however be that we cannot rename reserved, it is now part of > the kAPI, and changing it to another name would break the compilation > of something. That is one of the advantages of netlink API, you can > add new attributes without having to worry about breaking older > builds. Unfortunately, the self test is still using the IOCTL, it has > not been converted to netlink. If you wanted to introduce "speed" where "reserved" is, then could be done is: struct ethtool_test { __u32 cmd; __u32 flags; union { __u32 reserved; __u32 speed; }; __u32 len; __u64 data[]; }; That would mean anyone assigning to ethtool_test.reserved would still continue to work as the union is anonymous.
diff --git a/include/net/selftests.h b/include/net/selftests.h index e65e8d230d33..a13237c33e58 100644 --- a/include/net/selftests.h +++ b/include/net/selftests.h @@ -6,6 +6,10 @@ #if IS_ENABLED(CONFIG_NET_SELFTESTS) +int net_test_phy_loopback_udp(struct net_device *ndev); +int net_test_phy_loopback_udp_mtu(struct net_device *ndev); +int net_test_phy_loopback_tcp(struct net_device *ndev); + void net_selftest(struct net_device *ndev, struct ethtool_test *etest, u64 *buf); int net_selftest_get_count(void); @@ -13,6 +17,21 @@ void net_selftest_get_strings(u8 *data); #else +static inline int net_test_phy_loopback_udp(struct net_device *ndev) +{ + return 0; +} + +static inline int net_test_phy_loopback_udp_mtu(struct net_device *ndev) +{ + return 0; +} + +static inline int net_test_phy_loopback_tcp(struct net_device *ndev) +{ + return 0; +} + static inline void net_selftest(struct net_device *ndev, struct ethtool_test *etest, u64 *buf) { diff --git a/net/core/selftests.c b/net/core/selftests.c index e99ae983fca9..d4e0e2eff991 100644 --- a/net/core/selftests.c +++ b/net/core/selftests.c @@ -310,15 +310,16 @@ static int net_test_phy_loopback_disable(struct net_device *ndev) return phy_loopback(ndev->phydev, false, 0); } -static int net_test_phy_loopback_udp(struct net_device *ndev) +int net_test_phy_loopback_udp(struct net_device *ndev) { struct net_packet_attrs attr = { }; attr.dst = ndev->dev_addr; return __net_test_loopback(ndev, &attr); } +EXPORT_SYMBOL_GPL(net_test_phy_loopback_udp); -static int net_test_phy_loopback_udp_mtu(struct net_device *ndev) +int net_test_phy_loopback_udp_mtu(struct net_device *ndev) { struct net_packet_attrs attr = { }; @@ -326,8 +327,9 @@ static int net_test_phy_loopback_udp_mtu(struct net_device *ndev) attr.max_size = ndev->mtu; return __net_test_loopback(ndev, &attr); } +EXPORT_SYMBOL_GPL(net_test_phy_loopback_udp_mtu); -static int net_test_phy_loopback_tcp(struct net_device *ndev) +int net_test_phy_loopback_tcp(struct net_device *ndev) { struct net_packet_attrs attr = { }; @@ -335,6 +337,7 @@ static int net_test_phy_loopback_tcp(struct net_device *ndev) attr.tcp = true; return __net_test_loopback(ndev, &attr); } +EXPORT_SYMBOL_GPL(net_test_phy_loopback_tcp); static const struct net_test { char name[ETH_GSTRING_LEN];
net_selftests() provides a generic set of selftests for netdevs with PHY. Those selftests rely on an existing link to inherit the speed for the loopback mode. net_selftests() is not designed to extend existing selftests of drivers, but with net_test_phy_loopback_* it contains useful test infrastructure. Export net_test_phy_loopback_* to enable reuse in existing selftests of other drivers. This also enables driver specific loopback modes, which don't rely on an existing link. Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com> CC: Oleksij Rempel <o.rempel@pengutronix.de> --- include/net/selftests.h | 19 +++++++++++++++++++ net/core/selftests.c | 9 ++++++--- 2 files changed, 25 insertions(+), 3 deletions(-)