[PATCH net-next 0/3] mlxsw: Add speed and auto-negotiation test

[PATCH net-next 0/3] mlxsw: Add speed and auto-negotiation test

[Ido Schimmel]

From: Ido Schimmel <idosch@mellanox.com>

Amit says:

This patchset adds a selftest which tests different speeds and
auto-negotiation.

Patch #1 Adds functions that retrieve information from ethtool.

Patch #2 Adds option to wait for device with limit of iterations.

Patch #3 Adds the test.

Amit Cohen (3):
  selftests: mlxsw: Add ethtool_lib.sh
  selftests: mlxsw: lib.sh: Add wait for dev with timeout
  selftests: mlxsw: Add speed and auto-negotiation test

 .../selftests/drivers/net/mlxsw/ethtool.sh    | 308 ++++++++++++++++++
 .../selftests/net/forwarding/ethtool_lib.sh   |  91 ++++++
 tools/testing/selftests/net/forwarding/lib.sh |  28 +-
 3 files changed, 424 insertions(+), 3 deletions(-)
 create mode 100755 tools/testing/selftests/drivers/net/mlxsw/ethtool.sh
 create mode 100755 tools/testing/selftests/net/forwarding/ethtool_lib.sh

-- 
2.20.1

[PATCH net-next 1/3] selftests: mlxsw: Add ethtool_lib.sh

[Ido Schimmel]

From: Amit Cohen <amitc@mellanox.com>

Functions:
1. ethtool_set:
	params: cmd
	The function runs ethtool by cmd (ethtool -s cmd) and checks if there
	 was an error in configuration

2. speeds_get:
	params: dev, with_mode (0 or 1)
	return value: Array of supported link modes with/without mode.

	* Example 1:
	speeds_get swp1 0
	return: 1000 10000 40000
	* Example 2:
	speeds_get swp1 1
	return: 1000baseKX/Full 10000baseKR/Full 40000baseCR4/Full 40000baseSR4/Full

3. common_speeds_get:
	params: dev1, dev2, with_mode (0 or 1)
	return value: Array of common speeds of dev1 and dev2.

	* Example:
	common_speeds_get swp1 swp2 0
	return: 1000 10000
	(assume that swp1 supports 1000 10000 40000 and swp2 supports 1000 10000)

Signed-off-by: Amit Cohen <amitc@mellanox.com>
Reviewed-by: Petr Machata <petrm@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 .../selftests/net/forwarding/ethtool_lib.sh   | 91 +++++++++++++++++++
 1 file changed, 91 insertions(+)
 create mode 100755 tools/testing/selftests/net/forwarding/ethtool_lib.sh

diff --git a/tools/testing/selftests/net/forwarding/ethtool_lib.sh b/tools/testing/selftests/net/forwarding/ethtool_lib.sh
new file mode 100755
index 000000000000..6dcbbd228047
--- /dev/null
+++ b/tools/testing/selftests/net/forwarding/ethtool_lib.sh
@@ -0,0 +1,91 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+declare -A speed_values
+
+speed_values=(	[10baseT/Half]=0x001
+		[10baseT/Full]=0x002
+		[100baseT/Half]=0x004
+		[100baseT/Full]=0x008
+		[1000baseT/Half]=0x010
+		[1000baseT/Full]=0x020
+		[1000baseKX/Full]=0x20000
+		[1000baseX/Full]=0x20000000000
+		[2500baseT/Full]=0x800000000000
+		[2500baseX/Full]=0x8000
+		[5000baseT/Full]=0x1000000000000
+		[10000baseT/Full]=0x1000
+		[10000baseKX4/Full]=0x40000
+		[10000baseKR/Full]=0x80000
+		[10000baseCR/Full]=0x40000000000
+		[10000baseSR/Full]=0x80000000000
+		[10000baseLR/Full]=0x100000000000
+		[10000baseLRM/Full]=0x200000000000
+		[10000baseER/Full]=0x400000000000
+		[20000baseMLD2/Full]=0x200000
+		[20000baseKR2/Full]=0x400000
+		[25000baseCR/Full]=0x80000000
+		[25000baseKR/Full]=0x100000000
+		[25000baseSR/Full]=0x200000000
+		[40000baseKR4/Full]=0x800000
+		[40000baseCR4/Full]=0x1000000
+		[40000baseSR4/Full]=0x2000000
+		[40000baseLR4/Full]=0x4000000
+		[50000baseCR2/Full]=0x400000000
+		[40000baseSR4/Full]=0x2000000
+		[40000baseLR4/Full]=0x4000000
+		[50000baseCR2/Full]=0x400000000
+		[50000baseKR2/Full]=0x800000000
+		[50000baseSR2/Full]=0x10000000000
+		[56000baseKR4/Full]=0x8000000
+		[56000baseCR4/Full]=0x10000000
+		[56000baseSR4/Full]=0x20000000
+		[56000baseLR4/Full]=0x40000000
+		[100000baseKR4/Full]=0x1000000000
+		[100000baseSR4/Full]=0x2000000000
+		[100000baseCR4/Full]=0x4000000000
+		[100000baseLR4_ER4/Full]=0x8000000000)
+
+ethtool_set()
+{
+	local cmd="$@"
+	local out=$(ethtool -s $cmd 2>&1 | wc -l)
+	check_err $out "error in configuration. $cmd"
+}
+
+speeds_get()
+{
+	local dev=$1; shift
+	local with_mode=$1; shift
+
+	local speeds_str=$(ethtool "$dev" | \
+		# Snip everything before the link modes section.
+		sed -n '/Supported link modes:/,$p' | \
+		# Quit processing the rest at the start of the next section.
+		# When checking, skip the header of this section (hence the 2,).
+		sed -n '2,${/^[\t][^ \t]/q};p' | \
+		# Drop the section header of the current section.
+		cut -d':' -f2)
+
+	local -a speeds_arr=($speeds_str)
+	if [[ $with_mode -eq 0 ]]; then
+		for ((i=0; i<${#speeds_arr[@]}; i++)); do
+			speeds_arr[$i]=${speeds_arr[$i]%base*}
+		done
+	fi
+	echo ${speeds_arr[@]}
+}
+
+common_speeds_get()
+{
+	dev1=$1; shift
+	dev2=$1; shift
+	with_mode=$1; shift
+
+	local -a dev1_speeds=($(speeds_get $dev1 $with_mode))
+	local -a dev2_speeds=($(speeds_get $dev2 $with_mode))
+
+	comm -12 \
+		<(printf '%s\n' "${dev1_speeds[@]}" | sort -u) \
+		<(printf '%s\n' "${dev2_speeds[@]}" | sort -u)
+}
-- 
2.20.1

[PATCH net-next 2/3] selftests: mlxsw: lib.sh: Add wait for dev with timeout

[Ido Schimmel]

From: Amit Cohen <amitc@mellanox.com>

Add a function that waits for device with maximum number of iterations.
It enables to limit the waiting and prevent infinite loop.

This will be used by the subsequent patch which will set two ports to
different speeds in order to make sure they cannot negotiate a link.

Signed-off-by: Amit Cohen <amitc@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 tools/testing/selftests/net/forwarding/lib.sh | 28 +++++++++++++++++--
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index 9385dc971269..cd362d14d6c6 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -18,6 +18,7 @@ NETIF_CREATE=${NETIF_CREATE:=yes}
 MCD=${MCD:=smcrouted}
 MC_CLI=${MC_CLI:=smcroutectl}
 PING_TIMEOUT=${PING_TIMEOUT:=5}
+WAIT_TIMEOUT=${WAIT_TIMEOUT:=20}
 
 relative_path="${BASH_SOURCE%/*}"
 if [[ "$relative_path" == "${BASH_SOURCE}" ]]; then
@@ -226,24 +227,45 @@ log_info()
 setup_wait_dev()
 {
 	local dev=$1; shift
+	local wait_time=${1:-$WAIT_TIME}; shift
 
-	while true; do
+	setup_wait_dev_with_timeout "$dev" 600 $wait_time
+
+	if (($?)); then
+		check_err 1
+		log_test setup_wait_dev ": Interface $dev does not come up."
+		exit 0
+	fi
+}
+
+setup_wait_dev_with_timeout()
+{
+	local dev=$1; shift
+	local max_iterations=${1:-$WAIT_TIMEOUT}; shift
+	local wait_time=${1:-$WAIT_TIME}; shift
+	local i
+
+	for ((i = 1; i <= $max_iterations; ++i)); do
 		ip link show dev $dev up \
 			| grep 'state UP' &> /dev/null
 		if [[ $? -ne 0 ]]; then
 			sleep 1
 		else
-			break
+			sleep $wait_time
+			return 0
 		fi
 	done
+
+	return 1
 }
 
 setup_wait()
 {
 	local num_netifs=${1:-$NUM_NETIFS}
+	local i
 
 	for ((i = 1; i <= num_netifs; ++i)); do
-		setup_wait_dev ${NETIFS[p$i]}
+		setup_wait_dev ${NETIFS[p$i]} 0
 	done
 
 	# Make sure links are ready.
-- 
2.20.1

[PATCH net-next 3/3] selftests: mlxsw: Add speed and auto-negotiation test

[Ido Schimmel]

From: Amit Cohen <amitc@mellanox.com>

Check configurations and packets transference
with different variations of autoneg and speed.

Test plan:
1. Test force of same speed with autoneg off
2. Test force of different speeds with autoneg off (should fail)
3. One side is autoneg on and other side sets force of common speeds
4. One side is autoneg on and other side only advertises a subset of the common
   speeds (one speed of the subset.)
5. One side is autoneg on and other side only advertises a subset of the
   common speeds. Check that highest speed is negotiated
6. Test autoneg on, but each side advertises different speeds (should fail)

Signed-off-by: Amit Cohen <amitc@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 .../selftests/drivers/net/mlxsw/ethtool.sh    | 308 ++++++++++++++++++
 1 file changed, 308 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/net/mlxsw/ethtool.sh

diff --git a/tools/testing/selftests/drivers/net/mlxsw/ethtool.sh b/tools/testing/selftests/drivers/net/mlxsw/ethtool.sh
new file mode 100755
index 000000000000..cbf8dbcc375a
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/mlxsw/ethtool.sh
@@ -0,0 +1,308 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+lib_dir=$(dirname $0)/../../../net/forwarding
+
+ALL_TESTS="
+	same_speeds_autoneg_off
+	different_speeds_autoneg_off
+	combination_of_neg_on_and_off
+	advertise_subset_of_speeds
+	check_highest_speed_is_chosen
+	different_speeds_autoneg_on
+"
+NUM_NETIFS=2
+source $lib_dir/lib.sh
+source $lib_dir/ethtool_lib.sh
+
+h1_create()
+{
+	simple_if_init $h1 192.0.2.1/24
+}
+
+h1_destroy()
+{
+	simple_if_fini $h1 192.0.2.1/24
+}
+
+h2_create()
+{
+	simple_if_init $h2 192.0.2.2/24
+}
+
+h2_destroy()
+{
+	simple_if_fini $h2 192.0.2.2/24
+}
+
+setup_prepare()
+{
+	h1=${NETIFS[p1]}
+	h2=${NETIFS[p2]}
+
+	h1_create
+	h2_create
+}
+
+cleanup()
+{
+	pre_cleanup
+
+	h2_destroy
+	h1_destroy
+}
+
+different_speeds_get()
+{
+	local dev1=$1; shift
+	local dev2=$1; shift
+	local with_mode=$1; shift
+
+	local -a speeds_arr
+
+	speeds_arr=($(common_speeds_get $dev1 $dev2 $with_mode))
+
+	if [[ ${#speeds_arr[@]} < 2 ]]; then
+		check_err 1 "cannot check different speeds. There are not enough speeds"
+	fi
+
+	echo ${speeds_arr[0]} ${speeds_arr[1]}
+}
+
+same_speeds_autoneg_off()
+{
+	# Check that when each of the reported speeds
+	# is forced, the link comes up and is operational.
+	local -a speeds_arr=($(common_speeds_get $h1 $h2 0))
+	for speed in "${speeds_arr[@]}"; do
+		# Skip 56G because this speed isn't supported with autoneg off.
+		if [[ $speed == 56000 ]]; then
+			continue
+		fi
+
+		RET=0
+		ethtool_set $h1 speed $speed autoneg off
+		ethtool_set $h2 speed $speed autoneg off
+
+		setup_wait_dev_with_timeout $h1
+		setup_wait_dev_with_timeout $h2
+		ping_do $h1 192.0.2.2
+		check_err $? " speed $speed autoneg off"
+		log_test "force of same speed ($speed) autoneg off"
+	done
+
+	ethtool -s $h2 autoneg on
+	ethtool -s $h1 autoneg on
+}
+
+different_speeds_autoneg_off()
+{
+	# Test that when we force different speeds,
+	# links aren't up and ping fails.
+	RET=0
+
+	local -a speeds_arr=($(different_speeds_get $h1 $h2 0))
+	local speed1=${speeds_arr[0]}
+	local speed2=${speeds_arr[1]}
+
+	ethtool_set $h1 speed $speed1 autoneg off
+	ethtool_set $h2 speed $speed2 autoneg off
+
+	setup_wait_dev_with_timeout $h1
+	setup_wait_dev_with_timeout $h2
+	ping_do $h1 192.0.2.2
+	check_fail $? "ping with different speeds"
+
+	log_test "force of different speeds autoneg off"
+
+	ethtool -s $h2 autoneg on
+	ethtool -s $h1 autoneg on
+}
+
+combination_of_neg_on_and_off()
+{
+	# Test that when one dev is forced to a speed supported
+	# by both endpoints and the other dev is configured to autoneg on,
+	# the links are up and ping succeeds.
+	local -a speeds_arr=($(common_speeds_get $h1 $h2 0))
+
+	for speed in "${speeds_arr[@]}"; do
+		# Skip 56G because this speed isn't supported with autoneg off.
+		if [[ $speed == 56000 ]]; then
+			continue
+		fi
+
+		RET=0
+		ethtool_set $h1 speed $speed autoneg off
+
+		setup_wait_dev_with_timeout $h1
+		setup_wait_dev_with_timeout $h2
+		ping_do $h1 192.0.2.2
+		check_err $? "h1-speed=$speed autoneg off, h2 autoneg on"
+		log_test "one side with autoneg off (speed = $speed) and another with autoneg on"
+	done
+
+	ethtool -s $h1 autoneg on
+}
+
+subset_of_common_speeds_get()
+{
+	local dev1=$1; shift
+	local dev2=$1; shift
+
+	local -a speeds_arr=($(common_speeds_get $dev1 $dev2 0))
+	local speed_to_advertise=0
+	local speed_to_remove=${speeds_arr[0]}
+	# If speed_to_remove=x we don't want to remove also speeds
+	# that start with x000.. so add 'base' to limit the speed.
+	speed_to_remove+='base'
+
+	local -a speeds_mode_arr=($(common_speeds_get $dev1 $dev2 1))
+
+	for speed in ${speeds_mode_arr[@]}; do
+		if [[ $speed != $speed_to_remove* ]]; then
+			speed_to_advertise=$(($speed_to_advertise | \
+				${speed_values[$speed]}))
+		fi
+
+	done
+	# Convert to hex base
+	printf "%#x" "$speed_to_advertise"
+}
+
+speed_to_advertise_get()
+{
+	# The function returns the hex number that is appropriate to
+	# all modes of the parameter - speed_without_mode
+	local speed_without_mode=$1; shift
+	local supported_speeds=("$@"); shift
+	local speed_to_advertise=0
+
+	# If speed_without_mode=x we don't want to match also speeds
+	# that start with x000.. so add 'base' to limit the speed.
+	speed_without_mode+='base'
+	for speed in ${supported_speeds[@]}; do
+		if [[ $speed == $speed_without_mode* ]]; then
+			speed_to_advertise=$(($speed_to_advertise | \
+				${speed_values[$speed]}))
+		fi
+
+	done
+
+	# Convert to hex base
+	printf "%#x" "$speed_to_advertise"
+}
+advertise_subset_of_speeds()
+{
+	# Test that when one dev advertises a subset of speeds
+	# and another advertises a specific speed (but all modes of this speed),
+	# the links are up and ping success.
+	RET=0
+
+	local speed_1_to_advertise=$(subset_of_common_speeds_get $h1 $h2)
+	ethtool_set $h1 advertise $speed_1_to_advertise
+
+	if [ $RET != 0 ]; then
+		log_test "advertise subset of speeds"
+		return
+	fi
+
+	local -a speeds_arr_without_mode=($(common_speeds_get $h1 $h2 0))
+	# Check only speeds that h1 advertised. remove the first speed.
+	unset speeds_arr_without_mode[0]
+	local -a speeds_arr_with_mode=($(common_speeds_get $h1 $h2 1))
+
+	for speed_value in ${speeds_arr_without_mode[@]}; do
+		RET=0
+		local speed_2_to_advertise=$(speed_to_advertise_get $speed_value \
+			"${speeds_arr_with_mode[@]}")
+		ethtool_set $h2 advertise $speed_2_to_advertise
+
+		setup_wait_dev_with_timeout $h1
+		setup_wait_dev_with_timeout $h2
+		ping_do $h1 192.0.2.2
+		check_err $? "h1=$speed_1_to_advertise, h2=$speed_2_to_advertise ($speed_value)"
+
+		log_test "advertise subset of speeds (h1=$speed_1_to_advertise, h2=$speed_2_to_advertise)"
+	done
+
+	ethtool -s $h2 autoneg on
+	ethtool -s $h1 autoneg on
+}
+
+check_highest_speed_is_chosen()
+{
+	# Test that when one dev advertise subset of speeds,
+	# the other chooses the highest speed.
+	# This test checks configuration without traffic.
+	RET=0
+
+	local max_speed
+	local chosen_speed
+	local speed_to_advertise=$(subset_of_common_speeds_get $h1 $h2)
+
+	ethtool_set $h1 advertise $speed_to_advertise
+
+	if [ $RET != 0 ]; then
+		log_test "check highest speed."
+		return
+	fi
+
+	local -a speeds_arr=($(common_speeds_get $h1 $h2 0))
+	# Remove the first speed, h1 does not advertise this speed.
+	unset speeds_arr[0]
+
+	max_speed=${speeds_arr[0]}
+	for current in ${speeds_arr[@]}; do
+		if [[ $current -gt $max_speed ]]; then
+			max_speed=$current
+		fi
+	done
+
+	setup_wait_dev_with_timeout $h1
+	setup_wait_dev_with_timeout $h2
+	chosen_speed=$(ethtool $h1 | grep 'Speed:')
+	chosen_speed=${chosen_speed%"Mb/s"*}
+	chosen_speed=${chosen_speed#*"Speed: "}
+	((chosen_speed == max_speed))
+	check_err $? "h1 advertise $speed_to_advertise, h2 sync to speed $chosen_speed"
+
+	log_test "check highest speed"
+
+	ethtool -s $h2 autoneg on
+	ethtool -s $h1 autoneg on
+}
+
+different_speeds_autoneg_on()
+{
+	# Test that when we configure links to advertise different speeds,
+	# link aren't up and ping fails.
+	RET=0
+
+	local -a speeds=($(different_speeds_get $h1 $h2 1))
+	local speed1=${speeds[0]}
+	local speed2=${speeds[1]}
+
+	ethtool_set $h1 advertise ${speed_values[$speed1]}
+	ethtool_set $h2 advertise ${speed_values[$speed2]}
+
+	if (($RET)); then
+		setup_wait_dev_with_timeout $h1
+		setup_wait_dev_with_timeout $h2
+		ping_do $h1 192.0.2.2
+		check_fail $? "ping with different speeds autoneg on"
+	fi
+
+	log_test "advertise different speeds autoneg on"
+
+	ethtool -s $h2 autoneg on
+	ethtool -s $h1 autoneg on
+}
+
+trap cleanup EXIT
+
+setup_prepare
+setup_wait
+
+tests_run
+
+exit $EXIT_STATUS
-- 
2.20.1

Re: [PATCH net-next 1/3] selftests: mlxsw: Add ethtool_lib.sh

[Andrew Lunn]

On Mon, Jun 10, 2019 at 11:40:43AM +0300, Ido Schimmel wrote:
> From: Amit Cohen <amitc@mellanox.com>
> +declare -A speed_values
> +
> +speed_values=(	[10baseT/Half]=0x001
> +		[10baseT/Full]=0x002
> +		[100baseT/Half]=0x004
> +		[100baseT/Full]=0x008
> +		[1000baseT/Half]=0x010
> +		[1000baseT/Full]=0x020

Hi Ido, Amit

100BaseT1 and 1000BaseT1 were added recently.

	  Andrew

Re: [PATCH net-next 3/3] selftests: mlxsw: Add speed and auto-negotiation test

[Andrew Lunn]

> +		# Skip 56G because this speed isn't supported with autoneg off.
> +		if [[ $speed == 56000 ]]; then
> +			continue
> +		fi

Interesting. How is 56000 represented in ethtool? Listed in both
"Supported link modes" and "Advertised link modes"?

Thanks
	Andrew

Re: [PATCH net-next 1/3] selftests: mlxsw: Add ethtool_lib.sh

[Ido Schimmel]

On Mon, Jun 10, 2019 at 03:35:38PM +0200, Andrew Lunn wrote:
> On Mon, Jun 10, 2019 at 11:40:43AM +0300, Ido Schimmel wrote:
> > From: Amit Cohen <amitc@mellanox.com>
> > +declare -A speed_values
> > +
> > +speed_values=(	[10baseT/Half]=0x001
> > +		[10baseT/Full]=0x002
> > +		[100baseT/Half]=0x004
> > +		[100baseT/Full]=0x008
> > +		[1000baseT/Half]=0x010
> > +		[1000baseT/Full]=0x020
> 
> Hi Ido, Amit
> 
> 100BaseT1 and 1000BaseT1 were added recently.

Hi Andrew,

Didn't see them in the man page, so didn't include them. I now see your
patches are in the queue. Will add these speeds in v2.

Thanks

Re: [PATCH net-next 3/3] selftests: mlxsw: Add speed and auto-negotiation test

[Ido Schimmel]

On Mon, Jun 10, 2019 at 03:48:20PM +0200, Andrew Lunn wrote:
> > +		# Skip 56G because this speed isn't supported with autoneg off.
> > +		if [[ $speed == 56000 ]]; then
> > +			continue
> > +		fi
> 
> Interesting. How is 56000 represented in ethtool? Listed in both
> "Supported link modes" and "Advertised link modes"?

Hi Andrew,

Yes. We recently sent a patch to error out if autoneg is off: Commit
275e928f1911 ("mlxsw: spectrum: Prevent force of 56G").

Re: [PATCH net-next 1/3] selftests: mlxsw: Add ethtool_lib.sh

[Andrew Lunn]

> +speeds_get()
> +{
> +	local dev=$1; shift
> +	local with_mode=$1; shift
> +
> +	local speeds_str=$(ethtool "$dev" | \
> +		# Snip everything before the link modes section.
> +		sed -n '/Supported link modes:/,$p' | \
> +		# Quit processing the rest at the start of the next section.
> +		# When checking, skip the header of this section (hence the 2,).
> +		sed -n '2,${/^[\t][^ \t]/q};p' | \
> +		# Drop the section header of the current section.
> +		cut -d':' -f2)

ethtool gives you two lists of link modes:

$ sudo ethtool eth17
Settings for eth17:
         Supported ports: [ TP ]
         Supported link modes:   10baseT/Half 10baseT/Full 
                                 100baseT/Half 100baseT/Full 
                                 1000baseT/Full 
         Supported pause frame use: No
         Supports auto-negotiation: Yes
         Supported FEC modes: Not reported
         Advertised link modes:  10baseT/Half 10baseT/Full 
                                 100baseT/Half 100baseT/Full 
                                 1000baseT/Full

and if auto-neg has completed, there is potentially a third list, what
the peer is advertising.

Since this test is all about auto-neg, you should be using Advertised
link modes, not Supported link modes. There can be supported link
modes which you cannot advertise.

   Andrew

Re: [PATCH net-next 3/3] selftests: mlxsw: Add speed and auto-negotiation test

[Andrew Lunn]

On Mon, Jun 10, 2019 at 04:58:48PM +0300, Ido Schimmel wrote:
> On Mon, Jun 10, 2019 at 03:48:20PM +0200, Andrew Lunn wrote:
> > > +		# Skip 56G because this speed isn't supported with autoneg off.
> > > +		if [[ $speed == 56000 ]]; then
> > > +			continue
> > > +		fi
> > 
> > Interesting. How is 56000 represented in ethtool? Listed in both
> > "Supported link modes" and "Advertised link modes"?
> 
> Hi Andrew,
> 
> Yes. We recently sent a patch to error out if autoneg is off: Commit
> 275e928f1911 ("mlxsw: spectrum: Prevent force of 56G").

I never get access to high speed links like this. Do you have a
reference to why 56G needs auto-neg? What makes it different to every
other link mode?

Thanks
	Andrew

Re: [PATCH net-next 1/3] selftests: mlxsw: Add ethtool_lib.sh

[Ido Schimmel]

On Mon, Jun 10, 2019 at 03:59:14PM +0200, Andrew Lunn wrote:
> > +speeds_get()
> > +{
> > +	local dev=$1; shift
> > +	local with_mode=$1; shift
> > +
> > +	local speeds_str=$(ethtool "$dev" | \
> > +		# Snip everything before the link modes section.
> > +		sed -n '/Supported link modes:/,$p' | \
> > +		# Quit processing the rest at the start of the next section.
> > +		# When checking, skip the header of this section (hence the 2,).
> > +		sed -n '2,${/^[\t][^ \t]/q};p' | \
> > +		# Drop the section header of the current section.
> > +		cut -d':' -f2)
> 
> ethtool gives you two lists of link modes:
> 
> $ sudo ethtool eth17
> Settings for eth17:
>          Supported ports: [ TP ]
>          Supported link modes:   10baseT/Half 10baseT/Full 
>                                  100baseT/Half 100baseT/Full 
>                                  1000baseT/Full 
>          Supported pause frame use: No
>          Supports auto-negotiation: Yes
>          Supported FEC modes: Not reported
>          Advertised link modes:  10baseT/Half 10baseT/Full 
>                                  100baseT/Half 100baseT/Full 
>                                  1000baseT/Full
> 
> and if auto-neg has completed, there is potentially a third list, what
> the peer is advertising.
> 
> Since this test is all about auto-neg, you should be using Advertised
> link modes, not Supported link modes. There can be supported link
> modes which you cannot advertise.

Andrew, are you suggestion to split speeds_get() into
supported_speeds_get() and advertised_speeds_get() and use each where
appropriate? Note that not all the tests are testing with autoneg on.

Re: [PATCH net-next 1/3] selftests: mlxsw: Add ethtool_lib.sh

[Andrew Lunn]

> Andrew, are you suggestion to split speeds_get() into
> supported_speeds_get() and advertised_speeds_get() and use each where
> appropriate? Note that not all the tests are testing with autoneg on.

Hi Ido

Yes.

You should be able to force all speeds in supported speeds. But if you
try to auto-neg an speed which is not listed in advertised speeds when
all are enabled, i would expect an error.

    Andrew

Re: [PATCH net-next 1/3] selftests: mlxsw: Add ethtool_lib.sh

[Florian Fainelli]

On 6/10/2019 6:56 AM, Ido Schimmel wrote:
> On Mon, Jun 10, 2019 at 03:35:38PM +0200, Andrew Lunn wrote:
>> On Mon, Jun 10, 2019 at 11:40:43AM +0300, Ido Schimmel wrote:
>>> From: Amit Cohen <amitc@mellanox.com>
>>> +declare -A speed_values
>>> +
>>> +speed_values=(	[10baseT/Half]=0x001
>>> +		[10baseT/Full]=0x002
>>> +		[100baseT/Half]=0x004
>>> +		[100baseT/Full]=0x008
>>> +		[1000baseT/Half]=0x010
>>> +		[1000baseT/Full]=0x020
>>
>> Hi Ido, Amit
>>
>> 100BaseT1 and 1000BaseT1 were added recently.
> 
> Hi Andrew,
> 
> Didn't see them in the man page, so didn't include them. I now see your
> patches are in the queue. Will add these speeds in v2.

Could we extract the values from include/uapi/linux/ethtool.h, that way
we would not have to have to update the selftest speed_values() array here?
-- 
Florian

Re: [PATCH net-next 3/3] selftests: mlxsw: Add speed and auto-negotiation test

[Ido Schimmel]

On Mon, Jun 10, 2019 at 04:06:33PM +0200, Andrew Lunn wrote:
> On Mon, Jun 10, 2019 at 04:58:48PM +0300, Ido Schimmel wrote:
> > On Mon, Jun 10, 2019 at 03:48:20PM +0200, Andrew Lunn wrote:
> > > > +		# Skip 56G because this speed isn't supported with autoneg off.
> > > > +		if [[ $speed == 56000 ]]; then
> > > > +			continue
> > > > +		fi
> > > 
> > > Interesting. How is 56000 represented in ethtool? Listed in both
> > > "Supported link modes" and "Advertised link modes"?
> > 
> > Hi Andrew,
> > 
> > Yes. We recently sent a patch to error out if autoneg is off: Commit
> > 275e928f1911 ("mlxsw: spectrum: Prevent force of 56G").
> 
> I never get access to high speed links like this. Do you have a
> reference to why 56G needs auto-neg? What makes it different to every
> other link mode?

Hi Andrew,

I verified with PHY engineers and this limitation is specific to our
hardware, so no external reference I can provide.

Re: [PATCH net-next 1/3] selftests: mlxsw: Add ethtool_lib.sh

[Ido Schimmel]

On Mon, Jun 10, 2019 at 08:29:54AM -0700, Florian Fainelli wrote:
> On 6/10/2019 6:56 AM, Ido Schimmel wrote:
> > On Mon, Jun 10, 2019 at 03:35:38PM +0200, Andrew Lunn wrote:
> >> On Mon, Jun 10, 2019 at 11:40:43AM +0300, Ido Schimmel wrote:
> >>> From: Amit Cohen <amitc@mellanox.com>
> >>> +declare -A speed_values
> >>> +
> >>> +speed_values=(	[10baseT/Half]=0x001
> >>> +		[10baseT/Full]=0x002
> >>> +		[100baseT/Half]=0x004
> >>> +		[100baseT/Full]=0x008
> >>> +		[1000baseT/Half]=0x010
> >>> +		[1000baseT/Full]=0x020
> >>
> >> Hi Ido, Amit
> >>
> >> 100BaseT1 and 1000BaseT1 were added recently.
> > 
> > Hi Andrew,
> > 
> > Didn't see them in the man page, so didn't include them. I now see your
> > patches are in the queue. Will add these speeds in v2.
> 
> Could we extract the values from include/uapi/linux/ethtool.h, that way
> we would not have to have to update the selftest speed_values() array here?

Hi Florian,

Sounds reasonable. Will try this out.

Re: [PATCH net-next 3/3] selftests: mlxsw: Add speed and auto-negotiation test

[Andrew Lunn]

On Tue, Jun 11, 2019 at 09:35:26AM +0300, Ido Schimmel wrote:
> On Mon, Jun 10, 2019 at 04:06:33PM +0200, Andrew Lunn wrote:
> > On Mon, Jun 10, 2019 at 04:58:48PM +0300, Ido Schimmel wrote:
> > > On Mon, Jun 10, 2019 at 03:48:20PM +0200, Andrew Lunn wrote:
> > > > > +		# Skip 56G because this speed isn't supported with autoneg off.
> > > > > +		if [[ $speed == 56000 ]]; then
> > > > > +			continue
> > > > > +		fi
> > > > 
> > > > Interesting. How is 56000 represented in ethtool? Listed in both
> > > > "Supported link modes" and "Advertised link modes"?
> > > 
> > > Hi Andrew,
> > > 
> > > Yes. We recently sent a patch to error out if autoneg is off: Commit
> > > 275e928f1911 ("mlxsw: spectrum: Prevent force of 56G").
> > 
> > I never get access to high speed links like this. Do you have a
> > reference to why 56G needs auto-neg? What makes it different to every
> > other link mode?
> 
> Hi Andrew,
> 
> I verified with PHY engineers and this limitation is specific to our
> hardware, so no external reference I can provide.

Hi Ido

Could you detect your own hardware and only enable this exception when
needed?

Thanks
	Andrew

Re: [PATCH net-next 3/3] selftests: mlxsw: Add speed and auto-negotiation test

[Ido Schimmel]

On Tue, Jun 11, 2019 at 02:22:55PM +0200, Andrew Lunn wrote:
> On Tue, Jun 11, 2019 at 09:35:26AM +0300, Ido Schimmel wrote:
> > On Mon, Jun 10, 2019 at 04:06:33PM +0200, Andrew Lunn wrote:
> > > On Mon, Jun 10, 2019 at 04:58:48PM +0300, Ido Schimmel wrote:
> > > > On Mon, Jun 10, 2019 at 03:48:20PM +0200, Andrew Lunn wrote:
> > > > > > +		# Skip 56G because this speed isn't supported with autoneg off.
> > > > > > +		if [[ $speed == 56000 ]]; then
> > > > > > +			continue
> > > > > > +		fi
> > > > > 
> > > > > Interesting. How is 56000 represented in ethtool? Listed in both
> > > > > "Supported link modes" and "Advertised link modes"?
> > > > 
> > > > Hi Andrew,
> > > > 
> > > > Yes. We recently sent a patch to error out if autoneg is off: Commit
> > > > 275e928f1911 ("mlxsw: spectrum: Prevent force of 56G").
> > > 
> > > I never get access to high speed links like this. Do you have a
> > > reference to why 56G needs auto-neg? What makes it different to every
> > > other link mode?
> > 
> > Hi Andrew,
> > 
> > I verified with PHY engineers and this limitation is specific to our
> > hardware, so no external reference I can provide.
> 
> Hi Ido
> 
> Could you detect your own hardware and only enable this exception when
> needed?

The test currently resides under
tools/testing/selftests/drivers/net/mlxsw/, so it's specific to mlxsw.

I believe the 56G quirk is the only thing in the test that is specific
to mlxsw. Should be possible to move it to
tools/testing/selftests/net/forwarding/ and skip 56G for mlxsw.

Re: [PATCH net-next 3/3] selftests: mlxsw: Add speed and auto-negotiation test

[Andrew Lunn]

> The test currently resides under
> tools/testing/selftests/drivers/net/mlxsw/, so it's specific to mlxsw.
> 
> I believe the 56G quirk is the only thing in the test that is specific
> to mlxsw. Should be possible to move it to
> tools/testing/selftests/net/forwarding/ and skip 56G for mlxsw.

Hi Ido

That would be good. I don't see why this test should not work for any
interface. I expect there are a lot of 10/100/1000 interfaces which
could run this test.

Thanks
      Andrew