[PATCH 0/2] selftests: net: ethtool_mm: Support devices with higher rx-min-frag-size

[PATCH 0/2] selftests: net: ethtool_mm: Support devices with higher rx-min-frag-size

[Roger Quadros]

Hi,

Some devices e.g. TI SoCs have an errata due to which they cannot support
rx-min-frag-size of ETH_ZLEN (60). Also some devices cannot report
individual pmac/emac stats.

Modify the test so it can pass for such devices.

cheers,
-roger

Roger Quadros (1):
  selftests: forwarding: ethtool_mm: support devices that don't support
    pmac stats

Vladimir Oltean (1):
  selftests: forwarding: ethtool_mm: support devices with higher
    rx-min-frag-size

 .../selftests/net/forwarding/ethtool_mm.sh    | 44 ++++++++++++++++++-
 1 file changed, 42 insertions(+), 2 deletions(-)


base-commit: 70028b2e51c61d8dda0a31985978f4745da6a11b
-- 
2.34.1

[PATCH 1/2] selftests: forwarding: ethtool_mm: support devices with higher rx-min-frag-size

[Roger Quadros]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Some devices have errata due to which they cannot report ETH_ZLEN (60)
in the rx-min-frag-size. This was foreseen of course, and lldpad has
logic that when we request it to advertise addFragSize 0, it will round
it up to the lowest value that is _actually_ supported by the hardware.

The problem is that the selftest expects lldpad to report back to us the
same value as we requested.

Make the selftest smarter by figuring out on its own what is a
reasonable value to expect.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Roger Quadros <rogerq@kernel.org>
Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 .../selftests/net/forwarding/ethtool_mm.sh    | 37 ++++++++++++++++++-
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
index 39e736f30322..6212913f4ad1 100755
--- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh
+++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
@@ -155,15 +155,48 @@ manual_failed_verification_h2_to_h1()
 	manual_failed_verification $h2 $h1
 }
 
+smallest_supported_add_frag_size()
+{
+	local iface=$1
+	local rx_min_frag_size=
+
+	rx_min_frag_size=$(ethtool --json --show-mm $iface | \
+		jq '.[]."rx-min-frag-size"')
+
+	if [ $rx_min_frag_size -le 60 ]; then
+		echo 0
+	elif [ $rx_min_frag_size -le 124 ]; then
+		echo 1
+	elif [ $rx_min_frag_size -le 188 ]; then
+		echo 2
+	elif [ $rx_min_frag_size -le 252 ]; then
+		echo 3
+	else
+		echo "$iface: RX min frag size $rx_min_frag_size cannot be advertised over LLDP"
+		exit 1
+	fi
+}
+
+expected_add_frag_size()
+{
+	local iface=$1
+	local requested=$2
+	local min=$(smallest_supported_add_frag_size $iface)
+
+	[ $requested -le $min ] && echo $min || echo $requested
+}
+
 lldp_change_add_frag_size()
 {
 	local add_frag_size=$1
+	local pattern=
 
 	lldptool -T -i $h1 -V addEthCaps addFragSize=$add_frag_size >/dev/null
 	# Wait for TLVs to be received
 	sleep 2
-	lldptool -i $h2 -t -n -V addEthCaps | \
-		grep -q "Additional fragment size: $add_frag_size"
+	pattern=$(printf "Additional fragment size: %d" \
+			 $(expected_add_frag_size $h1 $add_frag_size))
+	lldptool -i $h2 -t -n -V addEthCaps | grep -q "$pattern"
 }
 
 lldp()
-- 
2.34.1

[PATCH 2/2] selftests: forwarding: ethtool_mm: support devices that don't support pmac stats

[Roger Quadros]

Some devices do not support individual 'pmac' and 'emac' stats.
For such devices, resort to 'aggregate' stats.

Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 tools/testing/selftests/net/forwarding/ethtool_mm.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
index 6212913f4ad1..e3f2e62029ca 100755
--- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh
+++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
@@ -26,6 +26,13 @@ traffic_test()
 	local delta=
 
 	before=$(ethtool_std_stats_get $if "eth-mac" "FramesTransmittedOK" $src)
+	# some devices don't support individual pmac/emac stats,
+	# use aggregate stats for them.
+        if [ "$before" == null ]; then
+                src="aggregate"
+                before=$(ethtool_std_stats_get $if "eth-mac" "FramesTransmittedOO
+K" $src)
+        fi
 
 	$MZ $if -q -c $num_pkts -p 64 -b bcast -t ip -R $PREEMPTIBLE_PRIO
 
-- 
2.34.1

Re: [PATCH 2/2] selftests: forwarding: ethtool_mm: support devices that don't support pmac stats

[Vladimir Oltean]

On Mon, Dec 11, 2023 at 02:01:38PM +0200, Roger Quadros wrote:
> Some devices do not support individual 'pmac' and 'emac' stats.
> For such devices, resort to 'aggregate' stats.
> 
> Signed-off-by: Roger Quadros <rogerq@kernel.org>
> ---
>  tools/testing/selftests/net/forwarding/ethtool_mm.sh | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> index 6212913f4ad1..e3f2e62029ca 100755
> --- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> +++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> @@ -26,6 +26,13 @@ traffic_test()
>  	local delta=
>  
>  	before=$(ethtool_std_stats_get $if "eth-mac" "FramesTransmittedOK" $src)
> +	# some devices don't support individual pmac/emac stats,
> +	# use aggregate stats for them.
> +        if [ "$before" == null ]; then
> +                src="aggregate"
> +                before=$(ethtool_std_stats_get $if "eth-mac" "FramesTransmittedOO
> +K" $src)
> +        fi

1. please follow the existing indentation scheme, don't mix tabs with spaces
2. someone mangled your patch into invalid bash syntax, splitting a line
   into 2
3. "FramesTransmittedOOK" has an extra "O"
4. it would be preferable if you could evaluate only once whether pMAC
   counters are reported, set a global variable, and in traffic_test(),
   if that variable is true, override $src with "aggregate".
5. why did you split the selftest patches out of the am65-cpsw patch
   set? It is for the am65-cpsw that they are needed.

>  
>  	$MZ $if -q -c $num_pkts -p 64 -b bcast -t ip -R $PREEMPTIBLE_PRIO
>  
> -- 
> 2.34.1
>

Something like this?

From ef5688a78908d99b607909fd7c93829c6a018b61 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Mon, 11 Dec 2023 15:21:25 +0200
Subject: [PATCH] selftests: forwarding: ethtool_mm: fall back to aggregate if
 device does not report pMAC stats

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 tools/testing/selftests/net/forwarding/ethtool_mm.sh | 11 +++++++++++
 tools/testing/selftests/net/forwarding/lib.sh        |  8 ++++++++
 2 files changed, 19 insertions(+)

diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
index 39e736f30322..2740133f95ec 100755
--- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh
+++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
@@ -25,6 +25,10 @@ traffic_test()
 	local after=
 	local delta=
 
+	if [ has_pmac_stats[$netif] = false ]; then
+		src="aggregate"
+	fi
+
 	before=$(ethtool_std_stats_get $if "eth-mac" "FramesTransmittedOK" $src)
 
 	$MZ $if -q -c $num_pkts -p 64 -b bcast -t ip -R $PREEMPTIBLE_PRIO
@@ -284,6 +288,13 @@ for netif in ${NETIFS[@]}; do
 		echo "SKIP: $netif does not support MAC Merge"
 		exit $ksft_skip
 	fi
+
+	if check_ethtool_pmac_std_stats_support $netif; then
+		has_pmac_stats[$netif]=true
+	else
+		has_pmac_stats[$netif]=false
+		echo "$netif does not report pMAC statistics, falling back to aggregate"
+	fi
 done
 
 trap cleanup EXIT
diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index 8f6ca458af9a..82ac6a066729 100755
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -146,6 +146,14 @@ check_ethtool_mm_support()
 	fi
 }
 
+check_ethtool_pmac_std_stats_support()
+{
+	local dev=$1; shift
+
+	[ -n "$(ethtool --json -S $dev --all-groups --src pmac 2>/dev/null | \
+		jq '.[]')" ]
+}
+
 check_locked_port_support()
 {
 	if ! bridge -d link show | grep -q " locked"; then
-- 
2.34.1

Re: [PATCH 2/2] selftests: forwarding: ethtool_mm: support devices that don't support pmac stats

[Roger Quadros]

On 11/12/2023 15:24, Vladimir Oltean wrote:
> On Mon, Dec 11, 2023 at 02:01:38PM +0200, Roger Quadros wrote:
>> Some devices do not support individual 'pmac' and 'emac' stats.
>> For such devices, resort to 'aggregate' stats.
>>
>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>> ---
>>  tools/testing/selftests/net/forwarding/ethtool_mm.sh | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
>> index 6212913f4ad1..e3f2e62029ca 100755
>> --- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh
>> +++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
>> @@ -26,6 +26,13 @@ traffic_test()
>>  	local delta=
>>  
>>  	before=$(ethtool_std_stats_get $if "eth-mac" "FramesTransmittedOK" $src)
>> +	# some devices don't support individual pmac/emac stats,
>> +	# use aggregate stats for them.
>> +        if [ "$before" == null ]; then
>> +                src="aggregate"
>> +                before=$(ethtool_std_stats_get $if "eth-mac" "FramesTransmittedOO
>> +K" $src)
>> +        fi
> 
> 1. please follow the existing indentation scheme, don't mix tabs with spaces

Sure. Will fix.

> 2. someone mangled your patch into invalid bash syntax, splitting a line
>    into 2
> 3. "FramesTransmittedOOK" has an extra "O"

Will fix. The mangling happened at my end.

> 4. it would be preferable if you could evaluate only once whether pMAC
>    counters are reported, set a global variable, and in traffic_test(),
>    if that variable is true, override $src with "aggregate".
> 5. why did you split the selftest patches out of the am65-cpsw patch
>    set? It is for the am65-cpsw that they are needed.

Needed for the tests to pass, yes.
I'll add them back at the beginning of the series.

> 
>>  
>>  	$MZ $if -q -c $num_pkts -p 64 -b bcast -t ip -R $PREEMPTIBLE_PRIO
>>  
>> -- 
>> 2.34.1
>>
> 
> Something like this?

Thanks for the hint!

> 
> From ef5688a78908d99b607909fd7c93829c6a018b61 Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Date: Mon, 11 Dec 2023 15:21:25 +0200
> Subject: [PATCH] selftests: forwarding: ethtool_mm: fall back to aggregate if
>  device does not report pMAC stats
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  tools/testing/selftests/net/forwarding/ethtool_mm.sh | 11 +++++++++++
>  tools/testing/selftests/net/forwarding/lib.sh        |  8 ++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> index 39e736f30322..2740133f95ec 100755
> --- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> +++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> @@ -25,6 +25,10 @@ traffic_test()
>  	local after=
>  	local delta=
>  
> +	if [ has_pmac_stats[$netif] = false ]; then
> +		src="aggregate"
> +	fi
> +
>  	before=$(ethtool_std_stats_get $if "eth-mac" "FramesTransmittedOK" $src)
>  
>  	$MZ $if -q -c $num_pkts -p 64 -b bcast -t ip -R $PREEMPTIBLE_PRIO
> @@ -284,6 +288,13 @@ for netif in ${NETIFS[@]}; do
>  		echo "SKIP: $netif does not support MAC Merge"
>  		exit $ksft_skip
>  	fi
> +
> +	if check_ethtool_pmac_std_stats_support $netif; then
> +		has_pmac_stats[$netif]=true
> +	else
> +		has_pmac_stats[$netif]=false
> +		echo "$netif does not report pMAC statistics, falling back to aggregate"
> +	fi
>  done
>  
>  trap cleanup EXIT
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> index 8f6ca458af9a..82ac6a066729 100755
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -146,6 +146,14 @@ check_ethtool_mm_support()
>  	fi
>  }
>  
> +check_ethtool_pmac_std_stats_support()
> +{
> +	local dev=$1; shift
> +
> +	[ -n "$(ethtool --json -S $dev --all-groups --src pmac 2>/dev/null | \
> +		jq '.[]')" ]
> +}
> +
>  check_locked_port_support()
>  {
>  	if ! bridge -d link show | grep -q " locked"; then

-- 
cheers,
-roger

Re: [PATCH 2/2] selftests: forwarding: ethtool_mm: support devices that don't support pmac stats

[Roger Quadros]

Hi Vladimir,

On 11/12/2023 15:24, Vladimir Oltean wrote:
> On Mon, Dec 11, 2023 at 02:01:38PM +0200, Roger Quadros wrote:
>> Some devices do not support individual 'pmac' and 'emac' stats.
>> For such devices, resort to 'aggregate' stats.
>>
>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>> ---
>>  tools/testing/selftests/net/forwarding/ethtool_mm.sh | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
>> index 6212913f4ad1..e3f2e62029ca 100755
>> --- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh
>> +++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
>> @@ -26,6 +26,13 @@ traffic_test()
>>  	local delta=
>>  
>>  	before=$(ethtool_std_stats_get $if "eth-mac" "FramesTransmittedOK" $src)
>> +	# some devices don't support individual pmac/emac stats,
>> +	# use aggregate stats for them.
>> +        if [ "$before" == null ]; then
>> +                src="aggregate"
>> +                before=$(ethtool_std_stats_get $if "eth-mac" "FramesTransmittedOO
>> +K" $src)
>> +        fi
> 
> 1. please follow the existing indentation scheme, don't mix tabs with spaces
> 2. someone mangled your patch into invalid bash syntax, splitting a line
>    into 2
> 3. "FramesTransmittedOOK" has an extra "O"
> 4. it would be preferable if you could evaluate only once whether pMAC
>    counters are reported, set a global variable, and in traffic_test(),
>    if that variable is true, override $src with "aggregate".
> 5. why did you split the selftest patches out of the am65-cpsw patch
>    set? It is for the am65-cpsw that they are needed.
> 
>>  
>>  	$MZ $if -q -c $num_pkts -p 64 -b bcast -t ip -R $PREEMPTIBLE_PRIO
>>  
>> -- 
>> 2.34.1
>>
> 
> Something like this?
> 
> From ef5688a78908d99b607909fd7c93829c6a018b61 Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Date: Mon, 11 Dec 2023 15:21:25 +0200
> Subject: [PATCH] selftests: forwarding: ethtool_mm: fall back to aggregate if
>  device does not report pMAC stats
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  tools/testing/selftests/net/forwarding/ethtool_mm.sh | 11 +++++++++++
>  tools/testing/selftests/net/forwarding/lib.sh        |  8 ++++++++
>  2 files changed, 19 insertions(+)

What is the proper way to run the script?

I've been hardcoding the following in the script.

NETIFS=( "eth0" "eth1" )

in setup_prepare()
	h1=eth0
	h2=eth1

and run the script like so

./run_kselftest.sh -t net/forwarding:ethtool_mm.sh

> 
> diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> index 39e736f30322..2740133f95ec 100755
> --- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> +++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> @@ -25,6 +25,10 @@ traffic_test()
>  	local after=
>  	local delta=
>  
> +	if [ has_pmac_stats[$netif] = false ]; then

This should be
	if [ ${has_pmac_stats[$if]} = false ]; then

otherwise it doesn't work.

> +		src="aggregate"
> +	fi
> +
>  	before=$(ethtool_std_stats_get $if "eth-mac" "FramesTransmittedOK" $src)
>  
>  	$MZ $if -q -c $num_pkts -p 64 -b bcast -t ip -R $PREEMPTIBLE_PRIO
> @@ -284,6 +288,13 @@ for netif in ${NETIFS[@]}; do
>  		echo "SKIP: $netif does not support MAC Merge"
>  		exit $ksft_skip
>  	fi
> +
> +	if check_ethtool_pmac_std_stats_support $netif; then
> +		has_pmac_stats[$netif]=true


> +	else
> +		has_pmac_stats[$netif]=false
> +		echo "$netif does not report pMAC statistics, falling back to aggregate"
> +	fi
>  done
>  
>  trap cleanup EXIT
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> index 8f6ca458af9a..82ac6a066729 100755
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -146,6 +146,14 @@ check_ethtool_mm_support()
>  	fi
>  }
>  
> +check_ethtool_pmac_std_stats_support()
> +{
> +	local dev=$1; shift
> +
> +	[ -n "$(ethtool --json -S $dev --all-groups --src pmac 2>/dev/null | \
> +		jq '.[]')" ]

This is evaluating to true instead of false on my platform so something needs to be fixed here.

Below is the output of "ethtool --json -S eth0 --all-groups --src pmac"
                                                                                                                                                                     
[ {
        "ifname": "eth0",
        "eth-phy": {},
        "eth-mac": {},
        "eth-ctrl": {},
        "rmon": {}
    } ]

I suppose we want to check if eth-mac has anything or not.

Something like this works

	[ 0 -ne $(ethtool --json -S $dev --all-groups --src pmac 2>/dev/null \
		| jq '.[]."eth-mac" | length') ]

OK?

> +}
> +
>  check_locked_port_support()
>  {
>  	if ! bridge -d link show | grep -q " locked"; then

also I had to revert a recent commit 

25ae948b4478 ("selftests/net: add lib.sh")

else i get an error message syaing ../lib.sh not found.
Looks like that is not getting deployed on kselftest-install

I will report this in the original patch thread as well.

-- 
cheers,
-roger

Re: [PATCH 2/2] selftests: forwarding: ethtool_mm: support devices that don't support pmac stats

[Vladimir Oltean]

On Tue, Dec 12, 2023 at 04:07:18PM +0200, Roger Quadros wrote:
> What is the proper way to run the script?
> 
> I've been hardcoding the following in the script.
> 
> NETIFS=( "eth0" "eth1" )
> 
> in setup_prepare()
> 	h1=eth0
> 	h2=eth1
> 
> and run the script like so
> 
> ./run_kselftest.sh -t net/forwarding:ethtool_mm.sh

IDK. I rsync the selftest dir to my board and do:

$ cd selftests/net/forwarding
$ ./ethtool.mm eth0 eth1

Running through run_kselftest.sh is probably better. I think that also
supports passing the network interfaces as arguments, no need to hack up
the script.

> > diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> > index 39e736f30322..2740133f95ec 100755
> > --- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> > +++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> > @@ -25,6 +25,10 @@ traffic_test()
> >  	local after=
> >  	local delta=
> >  
> > +	if [ has_pmac_stats[$netif] = false ]; then
> 
> This should be
> 	if [ ${has_pmac_stats[$if]} = false ]; then
> 
> otherwise it doesn't work.

Makes sense.

> > +		src="aggregate"
> > +	fi
> > +
> >  	before=$(ethtool_std_stats_get $if "eth-mac" "FramesTransmittedOK" $src)
> >  
> >  	$MZ $if -q -c $num_pkts -p 64 -b bcast -t ip -R $PREEMPTIBLE_PRIO
> > @@ -284,6 +288,13 @@ for netif in ${NETIFS[@]}; do
> >  		echo "SKIP: $netif does not support MAC Merge"
> >  		exit $ksft_skip
> >  	fi
> > +
> > +	if check_ethtool_pmac_std_stats_support $netif; then
> > +		has_pmac_stats[$netif]=true
> 
> 
> > +	else
> > +		has_pmac_stats[$netif]=false
> > +		echo "$netif does not report pMAC statistics, falling back to aggregate"
> > +	fi
> >  done
> >  
> >  trap cleanup EXIT
> > diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> > index 8f6ca458af9a..82ac6a066729 100755
> > --- a/tools/testing/selftests/net/forwarding/lib.sh
> > +++ b/tools/testing/selftests/net/forwarding/lib.sh
> > @@ -146,6 +146,14 @@ check_ethtool_mm_support()
> >  	fi
> >  }
> >  
> > +check_ethtool_pmac_std_stats_support()
> > +{
> > +	local dev=$1; shift
> > +
> > +	[ -n "$(ethtool --json -S $dev --all-groups --src pmac 2>/dev/null | \
> > +		jq '.[]')" ]
> 
> This is evaluating to true instead of false on my platform so something needs to be fixed here.
> 
> Below is the output of "ethtool --json -S eth0 --all-groups --src pmac"
> 
> [ {
>         "ifname": "eth0",
>         "eth-phy": {},
>         "eth-mac": {},
>         "eth-ctrl": {},
>         "rmon": {}
>     } ]
> 
> I suppose we want to check if eth-mac has anything or not.
> 
> Something like this works
> 
> 	[ 0 -ne $(ethtool --json -S $dev --all-groups --src pmac 2>/dev/null \
> 		| jq '.[]."eth-mac" | length') ]
> 
> OK?

Maybe giving the stats group as argument instead of hardcoding "eth-mac"
would make sense. I hoped we could avoid hardcoding one particular group
of counters in check_ethtool_pmac_std_stats_support().

> > +}
> > +
> >  check_locked_port_support()
> >  {
> >  	if ! bridge -d link show | grep -q " locked"; then
> 
> also I had to revert a recent commit 
> 
> 25ae948b4478 ("selftests/net: add lib.sh")
> 
> else i get an error message syaing ../lib.sh not found.
> Looks like that is not getting deployed on kselftest-install
> 
> I will report this in the original patch thread as well.

I'm not using that part, so I didn't notice it :)

Re: [PATCH 2/2] selftests: forwarding: ethtool_mm: support devices that don't support pmac stats

[Roger Quadros]

On 12/12/2023 16:57, Vladimir Oltean wrote:
> On Tue, Dec 12, 2023 at 04:07:18PM +0200, Roger Quadros wrote:
>> What is the proper way to run the script?
>>
>> I've been hardcoding the following in the script.
>>
>> NETIFS=( "eth0" "eth1" )
>>
>> in setup_prepare()
>> 	h1=eth0
>> 	h2=eth1
>>
>> and run the script like so
>>
>> ./run_kselftest.sh -t net/forwarding:ethtool_mm.sh
> 
> IDK. I rsync the selftest dir to my board and do:
> 
> $ cd selftests/net/forwarding
> $ ./ethtool.mm eth0 eth1
> 
> Running through run_kselftest.sh is probably better. I think that also
> supports passing the network interfaces as arguments, no need to hack up
> the script.
> 
>>> diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
>>> index 39e736f30322..2740133f95ec 100755
>>> --- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh
>>> +++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
>>> @@ -25,6 +25,10 @@ traffic_test()
>>>  	local after=
>>>  	local delta=
>>>  
>>> +	if [ has_pmac_stats[$netif] = false ]; then
>>
>> This should be
>> 	if [ ${has_pmac_stats[$if]} = false ]; then
>>
>> otherwise it doesn't work.
> 
> Makes sense.
> 
>>> +		src="aggregate"
>>> +	fi
>>> +
>>>  	before=$(ethtool_std_stats_get $if "eth-mac" "FramesTransmittedOK" $src)
>>>  
>>>  	$MZ $if -q -c $num_pkts -p 64 -b bcast -t ip -R $PREEMPTIBLE_PRIO
>>> @@ -284,6 +288,13 @@ for netif in ${NETIFS[@]}; do
>>>  		echo "SKIP: $netif does not support MAC Merge"
>>>  		exit $ksft_skip
>>>  	fi
>>> +
>>> +	if check_ethtool_pmac_std_stats_support $netif; then
>>> +		has_pmac_stats[$netif]=true
>>
>>
>>> +	else
>>> +		has_pmac_stats[$netif]=false
>>> +		echo "$netif does not report pMAC statistics, falling back to aggregate"
>>> +	fi
>>>  done
>>>  
>>>  trap cleanup EXIT
>>> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
>>> index 8f6ca458af9a..82ac6a066729 100755
>>> --- a/tools/testing/selftests/net/forwarding/lib.sh
>>> +++ b/tools/testing/selftests/net/forwarding/lib.sh
>>> @@ -146,6 +146,14 @@ check_ethtool_mm_support()
>>>  	fi
>>>  }
>>>  
>>> +check_ethtool_pmac_std_stats_support()
>>> +{
>>> +	local dev=$1; shift
>>> +
>>> +	[ -n "$(ethtool --json -S $dev --all-groups --src pmac 2>/dev/null | \
>>> +		jq '.[]')" ]
>>
>> This is evaluating to true instead of false on my platform so something needs to be fixed here.
>>
>> Below is the output of "ethtool --json -S eth0 --all-groups --src pmac"
>>
>> [ {
>>         "ifname": "eth0",
>>         "eth-phy": {},
>>         "eth-mac": {},
>>         "eth-ctrl": {},
>>         "rmon": {}
>>     } ]
>>
>> I suppose we want to check if eth-mac has anything or not.
>>
>> Something like this works
>>
>> 	[ 0 -ne $(ethtool --json -S $dev --all-groups --src pmac 2>/dev/null \
>> 		| jq '.[]."eth-mac" | length') ]
>>
>> OK?
> 
> Maybe giving the stats group as argument instead of hardcoding "eth-mac"
> would make sense. I hoped we could avoid hardcoding one particular group
> of counters in check_ethtool_pmac_std_stats_support().

You mean like this?

check_ethtool_pmac_std_stats_support()
{
	local dev=$1; shift
	local grp=$1; shift

	[ 0 -ne $(ethtool --json -S $dev --all-groups --src pmac 2>/dev/null \
		| jq '.[]."$grp" | length') ]
}

Caller will call like so
	check_ethtool_pmac_std_stats_support $netif eth-mac

-- 
cheers,
-roger