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

[Roger Quadros <rogerq@kernel.org>]

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