Re: [PATCH net-next 2/2] selftests: bonding: Add a test for 802.3ad with coupled control disabled

[Ido Schimmel <idosch@idosch.org>]

tl;dr - I believe the patch is fine as-is.

On Thu, Nov 12, 2026 at 08:21:58PM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] The bonding selftest config fragment
>   (tools/testing/selftests/drivers/net/bonding/config) now asks for…
> - [Low] In bond_lacp_independent_control.sh, collecting() reads the total
>   packet count of the bond1 ingress ICMP echo-request filter (handle…
> 
> --
> 
> selftests: bonding: Add a test for 802.3ad with coupled control disabled
> 
> This adds a VRF based selftest which checks that a bond port with
> coupled_control off receives packets in COLLECTING state. It keeps the
> port in COLLECTING state by clearing the Collecting bit in received
> LACPDUs, and then checks that ping works in DISTRIBUTING state. The
> bonding selftest config fragment gains the VRF, pedit and policy routing
> options the test needs.
> 
> > diff --git a/tools/testing/selftests/drivers/net/bonding/bond_lacp_independent_control.sh b/tools/testing/selftests/drivers/net/bonding/bond_lacp_independent_control.sh
> > new file mode 100755
> > index 0000000000000..c29712e01e49e
> > --- /dev/null
> > +++ b/tools/testing/selftests/drivers/net/bonding/bond_lacp_independent_control.sh
> 
> [ ... ]
> 
> > +h1_create()
> > +{
> > +	adf_ip_link_add bond1 type bond mode 802.3ad lacp_rate fast \
> > +		coupled_control off
> > +	adf_ip_link_set_master "$h1" bond1
> > +	adf_simple_if_init bond1 192.0.2.1/28
> > +
> > +	tc qdisc add dev "$h1" clsact
> > +	defer tc qdisc del dev "$h1" clsact
> > +
> > +	tc qdisc add dev bond1 clsact
> > +	defer tc qdisc del dev bond1 clsact
> > +
> > +	tc filter add dev bond1 ingress pref 1 handle 101 proto ip \
> > +		flower skip_hw src_ip 192.0.2.2 dst_ip 192.0.2.1 \
> > +		ip_proto icmp type 8 action pass
> > +	defer tc filter del dev bond1 ingress pref 1 handle 101 flower
> > +}
> 
> [ ... ]
> 
> > +collecting()
> > +{
> 
> [ ... ]
> 
> > +	ping_do bond2 192.0.2.1
> > +	pkts=$(tc_rule_handle_stats_get "dev bond1 ingress" 101)
> > +	((pkts >= PING_COUNT))
> > +	check_err $? "Packets not received in COLLECTING state"
> 
> [Severity: Low]
> Can this check pass when no packets were received in COLLECTING state?
> 
> The bond1 ingress filter with handle 101 is installed once by
> h1_create() from setup_prepare(), outside the per-test defer scopes, so
> its counter keeps growing across tests. collecting() compares the
> absolute counter with PING_COUNT and does not take a baseline before
> ping_do.
> 
> tests_run() in net/lib.sh iterates over ${TESTS:-$ALL_TESTS}. Take
> TESTS="distributing collecting" as an example:
> 
> distributing()
>     ping_do bond2 192.0.2.1    <- PING_COUNT echo requests hit filter 101
> collecting()
>     port moved to COLLECTING state
>     ping_do bond2 192.0.2.1    <- nothing received without the fix
>     pkts >= PING_COUNT         <- still true from the earlier test
> 
> The same thing happens if collecting is listed twice. The default
> ALL_TESTS order runs collecting first, so the default run is not
> affected.
> 
> Since the ping_do return value is ignored here, this counter is the only
> check of the fix. Would it make sense to read the counter before ping_do
> and check the delta, or to create the filter inside collecting() with a
> matching defer?

Noise. This is not a problem with the default test order and I don't
think it's worth handling other orders.

> 
> > +
> > +	log_test "Receive in COLLECTING state"
> > +}
> 
> [ ... ]
> 
> > diff --git a/tools/testing/selftests/drivers/net/bonding/config b/tools/testing/selftests/drivers/net/bonding/config
> > index b62c707152930..71c6d69d98a18 100644
> > --- a/tools/testing/selftests/drivers/net/bonding/config
> > +++ b/tools/testing/selftests/drivers/net/bonding/config
> 
> [ ... ]
> 
> > @@ -15,7 +19,9 @@ CONFIG_NETCONSOLE_DYNAMIC=y
> >  CONFIG_NETCONSOLE_EXTENDED_LOG=y
> >  CONFIG_NETDEVSIM=m
> >  CONFIG_NET_IPGRE=y
> > +CONFIG_NET_L3_MASTER_DEV=y
> >  CONFIG_NET_SCH_INGRESS=y
> > +CONFIG_NET_VRF=y
> 
> [Severity: Low]
> Should this fragment also enable CONFIG_IP_ADVANCED_ROUTER=y and
> CONFIG_IP_MULTIPLE_TABLES=y?
> 
> NET_VRF depends on IP_MULTIPLE_TABLES in drivers/net/Kconfig:
> 
> config NET_VRF
> 	tristate "Virtual Routing and Forwarding (Lite)"
> 	depends on IP_MULTIPLE_TABLES
> 	depends on NET_L3_MASTER_DEV
> 	depends on IPV6_MULTIPLE_TABLES || IPV6=n
> 
> and in net/ipv4/Kconfig, IP_MULTIPLE_TABLES depends on IP_ADVANCED_ROUTER
> and has no default:
> 
> config IP_MULTIPLE_TABLES
> 	bool "IP: policy routing"
> 	depends on IP_ADVANCED_ROUTER
> 
> The patch adds the NET_L3_MASTER_DEV and IPV6_MULTIPLE_TABLES
> dependencies but not the IPv4 ones. If the fragment is merged onto a base
> config that lacks them, Kconfig silently drops NET_VRF. arm64 defconfig
> is one example: it sets neither symbol, while x86_64_defconfig sets both.
> 
> In that case setup_prepare()->adf_vrf_prepare()->vrf_prepare() runs:
> 
> 	ip -4 rule add pref 32765 table local
> 
> which needs IPv4 policy routing, and adf_simple_if_init() cannot create
> the VRF devices. The test then fails during setup and never exercises the
> bonding change.
> 
> Several other net selftest fragments have the same gap, while
> net/mptcp/config lists IP_MULTIPLE_TABLES explicitly.

Both options are enabled by x86_64_defconfig and other config files that
enable CONFIG_NET_VRF do not list them either (e.g.,
net/forwarding/config). The test is running fine in the netdev CI.