[PATCH v2] sefltests: net: Add on/off checks for network interface non fixed features

[PATCH v2] sefltests: net: Add on/off checks for network interface non fixed features

[Abhinav Jain]

This patch addresses the TODO (add non fixed feature on/off check).
I have tested it manually on my system and made changes as suggested in v1

Signed-off-by: Abhinav Jain <jain.abhinav177@gmail.com>
---

PATCH v1:
https://lore.kernel.org/all/20240606212714.27472-1-jain.abhinav177@gmail.com/

Changes since v1:
 - Removed the addition of tail command as it was not required after
   below change
 - Used read to parse the temp features file rather than using for loop
   and took out awk/grep/sed from v1
---
 tools/testing/selftests/net/netdevice.sh | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/netdevice.sh b/tools/testing/selftests/net/netdevice.sh
index e3afcb424710..196b7f985db4 100755
--- a/tools/testing/selftests/net/netdevice.sh
+++ b/tools/testing/selftests/net/netdevice.sh
@@ -124,7 +124,27 @@ kci_netdev_ethtool()
 		return 1
 	fi
 	echo "PASS: $netdev: ethtool list features"
-	#TODO for each non fixed features, try to turn them on/off
+
+	while read -r FEATURE VALUE FIXED; do
+		[ "$FEATURE" != "Features" ] || continue # Skip "Features" line
+		[ "$FIXED" != "[fixed]" ] || continue # Skip fixed features
+		feature = "${FEATURE%:*}"
+
+		ethtool --offload "$netdev" "$feature" off
+		if [ $? -eq 0 ]; then
+			echo "PASS: $netdev: Turned off feature: $feature"
+		else
+			echo "FAIL: $netdev: Failed to turn off feature: $feature"
+		fi
+
+		ethtool --offload "$netdev" "$feature" on
+		if [ $? -eq 0 ]; then
+			echo "PASS: $netdev: Turned on feature: $feature"
+		else
+			echo "FAIL: $netdev: Failed to turn on feature: $feature"
+		fi
+	done < "$TMP_ETHTOOL_FEATURES"
+
 	rm "$TMP_ETHTOOL_FEATURES"
 
 	kci_netdev_ethtool_test 74 'dump' "ethtool -d $netdev"
-- 
2.34.1

Re: [PATCH v2] sefltests: net: Add on/off checks for network interface non fixed features

[Jakub Kicinski]

On Sun,  9 Jun 2024 13:21:24 +0000 Abhinav Jain wrote:
> This patch addresses the TODO (add non fixed feature on/off check).
> I have tested it manually on my system and made changes as suggested in v1

>  	echo "PASS: $netdev: ethtool list features"
> -	#TODO for each non fixed features, try to turn them on/off
> +
> +	while read -r FEATURE VALUE FIXED; do
> +		[ "$FEATURE" != "Features" ] || continue # Skip "Features" line
> +		[ "$FIXED" != "[fixed]" ] || continue # Skip fixed features
> +		feature = "${FEATURE%:*}"
> +
> +		ethtool --offload "$netdev" "$feature" off
> +		if [ $? -eq 0 ]; then
> +			echo "PASS: $netdev: Turned off feature: $feature"
> +		else
> +			echo "FAIL: $netdev: Failed to turn off feature: $feature"
> +		fi
> +
> +		ethtool --offload "$netdev" "$feature" on

You do off then on, so you assume the feature was on to begin with.
Not all features will be on. You gotta change the order based on
the initial state so that the feature goes back to what it was.

> +		if [ $? -eq 0 ]; then
> +			echo "PASS: $netdev: Turned on feature: $feature"
> +		else
> +			echo "FAIL: $netdev: Failed to turn on feature: $feature"
> +		fi
> +	done < "$TMP_ETHTOOL_FEATURES"

More importantly, tho, currently in our CI this test passes:

https://netdev.bots.linux.dev/contest.html?test=netdevice-sh

But it doesn't do anything:

https://netdev-3.bots.linux.dev/vmksft-net/results/638724/98-netdevice-sh/stdout
https://netdev-3.bots.linux.dev/vmksft-net-dbg/results/638544/90-netdevice-sh/stdout

Because I'm guessing the VM does not have any interface to test.

Before we extend the test could you please make the test automatically
add a veth or netdevsim device if there is no testable device on the
system? Make sure you remove it before exiting.

You can look at tests in tools/testing/selftests/drivers/net/netdevsim
for inspiration and library scripts which can be reused.

Re: [PATCH v2] sefltests: net: Add on/off checks for network interface non fixed features

[Abhinav Jain]

On Thu, 13 Jun 2024 18:49:38 -0700, Jakub Kicinski wrote:
> You do off then on, so you assume the feature was on to begin with.
> Not all features will be on. You gotta change the order based on
> the initial state so that the feature goes back to what it was.

Thanks for sharing feedback, I have sent a patch v3 addressing this.
I save the initial state of the feature and set it back after I toggle it.

> Because I'm guessing the VM does not have any interface to test.
>
> Before we extend the test could you please make the test automatically
> add a veth or netdevsim device if there is no testable device on the
> system? Make sure you remove it before exiting.
>
> You can look at tests in tools/testing/selftests/drivers/net/netdevsim
> for inspiration and library scripts which can be reused.

I did the same, I have added a veth pair in case if the network device
in the positional argument is not there. If it's there and just a bad
device name though, I am printing an error. Let me know if I should use
the veth pair in those cases as well for testing.

I have sent patch v3 here, please review:
https://lore.kernel.org/all/20240614113240.41550-1-jain.abhinav177@gmail.com/

-Abhinav