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

[Vladimir Oltean <vladimir.oltean@nxp.com>]

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 :)