Re: [PATCH net 10/17] selftests: forwarding: ethtool_mm: Skip when using veth pairs

[Ido Schimmel <idosch@idosch.org>]

On Wed, Aug 02, 2023 at 02:27:49PM +0200, Petr Machata wrote:
> 
> Vladimir Oltean <vladimir.oltean@nxp.com> writes:
> 
> > Hi Ido,
> >
> > On Wed, Aug 02, 2023 at 10:51:11AM +0300, Ido Schimmel wrote:
> >> MAC Merge cannot be tested with veth pairs, resulting in failures:
> >> 
> >>  # ./ethtool_mm.sh
> >>  [...]
> >>  TEST: Manual configuration with verification: swp1 to swp2          [FAIL]
> >>          Verification did not succeed
> >> 
> >> Fix by skipping the test when used with veth pairs.
> >> 
> >> Fixes: e6991384ace5 ("selftests: forwarding: add a test for MAC Merge layer")
> >> Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
> >> Closes: https://lore.kernel.org/netdev/adc5e40d-d040-a65e-eb26-edf47dac5b02@alu.unizg.hr/
> >> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> >> Reviewed-by: Petr Machata <petrm@nvidia.com>
> >> Tested-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
> >> ---
> >
> > That will skip the selftest just for veth pairs. This will skip it for
> > any device that doesn't support the MAC Merge layer:
> >
> > diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> > index c580ad623848..5432848a3c59 100755
> > --- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> > +++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> > @@ -224,6 +224,8 @@ h1_create()
> >  		hw 1
> >
> >  	ethtool --set-mm $h1 pmac-enabled on tx-enabled off verify-enabled off
> > +
> > +	h1_created=yes
> >  }
> >
> >  h2_create()
> > @@ -236,10 +238,16 @@ h2_create()
> >  		queues 1@0 1@1 1@2 1@3 \
> >  		fp P E E E \
> >  		hw 1
> > +
> > +	h2_created=yes
> >  }
> >
> >  h1_destroy()
> >  {
> > +	if ! [[ $h1_created = yes ]]; then
> > +		return
> > +	fi
> > +
> >  	ethtool --set-mm $h1 pmac-enabled off tx-enabled off verify-enabled off
> >
> >  	tc qdisc del dev $h1 root
> > @@ -249,6 +257,10 @@ h1_destroy()
> >
> >  h2_destroy()
> >  {
> > +	if ! [[ $h2_created = yes ]]; then
> > +		return
> > +	fi
> > +
> >  	tc qdisc del dev $h2 root
> >
> >  	ethtool --set-mm $h2 pmac-enabled off tx-enabled off verify-enabled off
> > @@ -266,6 +278,14 @@ setup_prepare()
> >  	h1=${NETIFS[p1]}
> >  	h2=${NETIFS[p2]}
> >
> > +	for netif in ${NETIFS[@]}; do
> > +		ethtool --show-mm $netif 2>&1 &> /dev/null
> > +		if [[ $? -ne 0 ]]; then
> > +			echo "SKIP: $netif does not support MAC Merge"
> > +			exit $ksft_skip
> > +		fi
> > +	done
> > +
> 
> Ido, if you decide to go this route, just hoist the loop to the global
> scope before registering the trap, then you don't need tho hX_created
> business.

I think the idea was to run this check after verifying that ethtool
supports MAC Merge in setup_prepare(). How about moving all these checks
before doing any configuration and registering a trap handler?

diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
index 4331e2161e8d..39e736f30322 100755
--- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh
+++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
@@ -258,11 +258,6 @@ h2_destroy()
 
 setup_prepare()
 {
-       check_ethtool_mm_support
-       check_tc_fp_support
-       require_command lldptool
-       bail_on_lldpad "autoconfigure the MAC Merge layer" "configure it manually"
-
        h1=${NETIFS[p1]}
        h2=${NETIFS[p2]}
 
@@ -278,7 +273,18 @@ cleanup()
        h1_destroy
 }
 
-skip_on_veth
+check_ethtool_mm_support
+check_tc_fp_support
+require_command lldptool
+bail_on_lldpad "autoconfigure the MAC Merge layer" "configure it manually"
+
+for netif in ${NETIFS[@]}; do
+       ethtool --show-mm $netif 2>&1 &> /dev/null
+       if [[ $? -ne 0 ]]; then
+               echo "SKIP: $netif does not support MAC Merge"
+               exit $ksft_skip
+       fi
+done
 
 trap cleanup EXIT

> 
> >  	h1_create
> >  	h2_create
> >  }
> 
>