Re: [PATCH net-next] selftests: net: Change the indication for iface is up

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

Hi Danielle,

On Thu, Jun 24, 2021 at 06:15:15PM +0300, Danielle Ratson wrote:
> Currently, the indication that an iface is up, is the mark 'state UP' in
> the iface info. That situation can be achieved also when the carrier is not
> ready, and therefore after the state was found as 'up', it can be still
> changed to 'down'.
> 
> For example, the below presents a part of a test output with one of the
> ifaces involved detailed info and timestamps:
> 
> In setup_wait()
> 45: swp13: <BROADCAST,MULTICAST,UP> mtu 1500 qdisc fq_codel master
>     vswp13 state UP mode DEFAULT group default qlen 1000
>     link/ether 7c:fe:90:fc:7d:f1 brd ff:ff:ff:ff:ff:ff promiscuity 0
> minmtu 0 maxmtu 65535
>     vrf_slave table 1 addrgenmode eui64 numtxqueues 1 numrxqueues 1
> gso_max_size 65536 gso_max_segs 65535 portname p13 switchid 7cfe90fc7dc0
> 17:58:27:242634417
> 
> In dst_mac_match_test()

What is dst_mac_match_test()?

> 45: swp13: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc fq_codel
>     master vswp13 state DOWN mode DEFAULT group default qlen 1000
>     link/ether 7c:fe:90:fc:7d:f1 brd ff:ff:ff:ff:ff:ff promiscuity 0
> minmtu 0 maxmtu 65535
>     vrf_slave table 1 addrgenmode eui64 numtxqueues 1 numrxqueues 1
> gso_max_size 65536 gso_max_segs 65535 portname p13 switchid 7cfe90fc7dc0
> 17:58:32:254535834
> TEST: dst_mac match (skip_hw)					    [FAIL]
>         Did not match on correct filter
> 
> In src_mac_match_test()

What is src_mac_match_test()?

> 45: swp13: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel
>     master vswp13 state UP mode DEFAULT group default qlen 1000
>     link/ether 7c:fe:90:fc:7d:f1 brd ff:ff:ff:ff:ff:ff promiscuity 0
> minmtu 0 maxmtu 65535
>     vrf_slave table 1 addrgenmode eui64 numtxqueues 1 numrxqueues 1
> gso_max_size 65536 gso_max_segs 65535 portname p13 switchid 7cfe90fc7dc0
> 17:58:34:446367468

Can you please really show the output of 'ip link show dev swp13 up'?
The format you are showing is not that and it is really confusing.

> TEST: src_mac match (skip_hw)                                       [ OK ]
> 
> In dst_ip_match_test()
> 45: swp13: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel
>     master vswp13 state UP mode DEFAULT group default qlen 1000
>     link/ether 7c:fe:90:fc:7d:f1 brd ff:ff:ff:ff:ff:ff promiscuity 0
> minmtu 0 maxmtu 65535
>     vrf_slave table 1 addrgenmode eui64 numtxqueues 1 numrxqueues 1
> gso_max_size 65536 gso_max_segs 65535 portname p13 switchid 7cfe90fc7dc0
> 17:58:35:633518622
> 
> In the example, after the setup_prepare() phase, the iface state was
> indeed 'UP' so the setup_wait() phase pass successfully. But since
> 'LOWER_UP' flag was not set yet, the next print, which was right before the
> first test case has started, the status turned into 'DOWN'.

Why?

> While, UP is an indicator that the interface has been enabled and running,
> LOWER_UP is a physical layer link flag. It indicates that an Ethernet
> cable was plugged in and that the device is connected to the network.
> 
> Therefore, the existence of the 'LOWER_UP' flag is necessary for
> indicating that the port is up before testing communication.

Documentation/networking/operstates.rst says:

IF_OPER_UP (6):
 Interface is operational up and can be used.

Additionally, RFC2863 says:

ifOperStatus OBJECT-TYPE
    SYNTAX  INTEGER {
                up(1),        -- ready to pass packets

You have not proven why the UP operstate is not sufficient and
additional checks must be made for link flags. Also you have not
explained how this fixes your problem.

> Change the indication for iface is up to include the existence of
> 'LOWER_UP'.
> 
> Signed-off-by: Danielle Ratson <danieller@nvidia.com>
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
>  tools/testing/selftests/net/forwarding/lib.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> index 42e28c983d41..a46076b8ebdd 100644
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -399,7 +399,7 @@ setup_wait_dev_with_timeout()
>  
>  	for ((i = 1; i <= $max_iterations; ++i)); do
>  		ip link show dev $dev up \
> -			| grep 'state UP' &> /dev/null
> +			| grep 'state UP' | grep 'LOWER_UP' &> /dev/null
>  		if [[ $? -ne 0 ]]; then
>  			sleep 1
>  		else
> -- 
> 2.31.1
>