netdev - Re: [PATCH net-next v9 6/8] net: selftests: Support selftest sets

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <36e2c2b6-e8d2-4304-87d9-8dc79e4e5482@engleder-embedded.com>
Date: Tue, 4 Mar 2025 06:55:01 +0100
From: Gerhard Engleder <gerhard@...leder-embedded.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: andrew@...n.ch, hkallweit1@...il.com, linux@...linux.org.uk,
 davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
 netdev@...r.kernel.org, Oleksij Rempel <o.rempel@...gutronix.de>
Subject: Re: [PATCH net-next v9 6/8] net: selftests: Support selftest sets

On 04.03.25 02:41, Jakub Kicinski wrote:
> On Thu, 27 Feb 2025 21:31:36 +0100 Gerhard Engleder wrote:
>> +/**
>> + * enum net_selftest - selftest set ID
>> + * @NET_SELFTEST_LOOPBACK_CARRIER: Loopback tests based on carrier speed
> 
> why are these "tests based on carrier speed"?
> these tests use default parameters AFAICT, I'm not seeing the relevance
> of carrier.. Maybe you can explain.

genphy_loopback() and most PHY specific loopback implementations
use the speed of the current carrier to determine the speed of
the to be configured loopback mode. Therefore, a carrier/linkup is
required to execute these tests successfully and I assume that's why
the already existing test set checks for carrier first.

I will enrich the comment of this test set.

> 
>> + */
>> +enum net_selftest {
>> +	NET_SELFTEST_LOOPBACK_CARRIER = 0,
>> +};
> 
>> diff --git a/net/core/selftests.c b/net/core/selftests.c
>> index e99ae983fca9..ec9bb149a378 100644
>> --- a/net/core/selftests.c
>> +++ b/net/core/selftests.c
>> @@ -14,6 +14,10 @@
>>   #include <net/tcp.h>
>>   #include <net/udp.h>
>>   
>> +struct net_test_ctx {
>> +	u8 next_id;
>> +};
>> +
>>   struct net_packet_attrs {
>>   	const unsigned char *src;
>>   	const unsigned char *dst;
>> @@ -44,14 +48,13 @@ struct netsfhdr {
>>   	u8 id;
>>   } __packed;
>>   
>> -static u8 net_test_next_id;
> 
> The removal of the global state seems loosely connected to the rest,
> the global state is okay because we hold RTNL across, AFAIU, which
> will still be true for tests varying speed. Not using global state
> is a worthwhile cleanup IMO, but I think you should split this patch
> in 2.

I will split this into a separate commit.

Thank you for the review!

Gerhard