diff mbox series

[PATCHv2] selftests: net: devlink_port_split.py: skip test if no suitable device available

Message ID 20230307150030.527726-1-po-hsu.lin@canonical.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [PATCHv2] selftests: net: devlink_port_split.py: skip test if no suitable device available | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 20 this patch: 20
netdev/cc_maintainers success CCed 10 of 10 maintainers
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 20 this patch: 20
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 67 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Po-Hsu Lin March 7, 2023, 3 p.m. UTC 
The `devlink -j port show` command output may not contain the "flavour"
key, an example from s390x LPAR with Ubuntu 22.10 (5.19.0-37-generic),
iproute2-5.15.0:
  {"port":{"pci/0001:00:00.0/1":{"type":"eth","netdev":"ens301"},
           "pci/0001:00:00.0/2":{"type":"eth","netdev":"ens301d1"},
           "pci/0002:00:00.0/1":{"type":"eth","netdev":"ens317"},
           "pci/0002:00:00.0/2":{"type":"eth","netdev":"ens317d1"}}}

This will cause a KeyError exception.

Create a validate_devlink_output() to check for this "flavour" from
devlink command output to avoid this KeyError exception. Also let
it handle the check for `devlink -j dev show` output in main().

Apart from this, if the test was not started because of any reason
(e.g. "lanes" does not exist, max lanes is 0 or the flavour of the
designated device is not "physical" and etc.) The script will still
return 0 and thus causing a false-negative test result.

Use a test_ran flag to determine if these tests were skipped and
return KSFT_SKIP to make it more clear.

V2: factor out the skip logic from main(), update commit message and
    skip reasons accordingly.
Link: https://bugs.launchpad.net/bugs/1937133
Fixes: f3348a82e727 ("selftests: net: Add port split test")
Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
---
 tools/testing/selftests/net/devlink_port_split.py | 36 +++++++++++++++++++----
 1 file changed, 31 insertions(+), 5 deletions(-)

Comments

Jakub Kicinski March 8, 2023, 1:02 a.m. UTC | #1 
On Tue,  7 Mar 2023 23:00:30 +0800 Po-Hsu Lin wrote:
> The `devlink -j port show` command output may not contain the "flavour"
> key, an example from s390x LPAR with Ubuntu 22.10 (5.19.0-37-generic),
> iproute2-5.15.0:
>   {"port":{"pci/0001:00:00.0/1":{"type":"eth","netdev":"ens301"},
>            "pci/0001:00:00.0/2":{"type":"eth","netdev":"ens301d1"},
>            "pci/0002:00:00.0/1":{"type":"eth","netdev":"ens317"},
>            "pci/0002:00:00.0/2":{"type":"eth","netdev":"ens317d1"}}}
> 
> This will cause a KeyError exception.

I looked closer and I don't understand why the key is not there.
Both 5.19 kernel should always put this argument out, and 5.15
iproute2 should always interpret it.

Am I looking wrong? Do you see how we can get a dump with no flavor?

I worry that this is some endianness problem, and we just misreport
stuff on big-endian.

> Create a validate_devlink_output() to check for this "flavour" from
> devlink command output to avoid this KeyError exception. Also let
> it handle the check for `devlink -j dev show` output in main().
> 
> Apart from this, if the test was not started because of any reason
> (e.g. "lanes" does not exist, max lanes is 0 or the flavour of the
> designated device is not "physical" and etc.) The script will still
> return 0 and thus causing a false-negative test result.
> 
> Use a test_ran flag to determine if these tests were skipped and
> return KSFT_SKIP to make it more clear.
> 
> V2: factor out the skip logic from main(), update commit message and
>     skip reasons accordingly.
> Link: https://bugs.launchpad.net/bugs/1937133
> Fixes: f3348a82e727 ("selftests: net: Add port split test")
> Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
> ---
>  tools/testing/selftests/net/devlink_port_split.py | 36 +++++++++++++++++++----
>  1 file changed, 31 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/devlink_port_split.py b/tools/testing/selftests/net/devlink_port_split.py
> index 2b5d6ff..749606c 100755
> --- a/tools/testing/selftests/net/devlink_port_split.py
> +++ b/tools/testing/selftests/net/devlink_port_split.py
> @@ -59,6 +59,8 @@ class devlink_ports(object):
>          assert stderr == ""
>          ports = json.loads(stdout)['port']
>  
> +        validate_devlink_output(ports, 'flavour')

If it's just a matter of kernel/iproute2 version we shouldn't need to
check here again?

>          for port in ports:
>              if dev in port:
>                  if ports[port]['flavour'] == 'physical':
> @@ -220,6 +222,27 @@ def split_splittable_port(port, k, lanes, dev):
>      unsplit(port.bus_info)
>  
>  
> +def validate_devlink_output(devlink_data, target_property=None):
> +    """
> +    Determine if test should be skipped by checking:
> +      1. devlink_data contains values
> +      2. The target_property exist in devlink_data
> +    """
> +    skip_reason = None
> +    if any(devlink_data.values()):
> +        if target_property:
> +            skip_reason = "{} not found in devlink output, test skipped".format(target_property)
> +            for key in devlink_data:
> +                if target_property in devlink_data[key]:
> +                    skip_reason = None
> +    else:
> +        skip_reason = 'devlink output is empty, test skipped'
> +
> +    if skip_reason:
> +        print(skip_reason)
> +        sys.exit(KSFT_SKIP)

Looks good, so..

>  def make_parser():
>      parser = argparse.ArgumentParser(description='A test for port splitting.')
>      parser.add_argument('--dev',
> @@ -231,6 +254,7 @@ def make_parser():
>  
>  
>  def main(cmdline=None):
> +    test_ran = False

I don't think we need the test_ran tracking any more?

>      parser = make_parser()
>      args = parser.parse_args(cmdline)
>  
> @@ -240,12 +264,9 @@ def main(cmdline=None):
>          stdout, stderr = run_command(cmd)
>          assert stderr == ""
>  
> +        validate_devlink_output(json.loads(stdout))
>          devs = json.loads(stdout)['dev']
> -        if devs:
> -            dev = list(devs.keys())[0]
> -        else:
> -            print("no devlink device was found, test skipped")
> -            sys.exit(KSFT_SKIP)
> +        dev = list(devs.keys())[0]
>  
>      cmd = "devlink dev show %s" % dev
>      stdout, stderr = run_command(cmd)
> @@ -277,6 +298,11 @@ def main(cmdline=None):
>                  split_splittable_port(port, lane, max_lanes, dev)
>  
>                  lane //= 2
> +        test_ran = True
> +
> +    if not test_ran:
> +        print("Test not started, no suitable device for the test")
> +        sys.exit(KSFT_SKIP)
>  
>  
>  if __name__ == "__main__":
Jiri Pirko March 8, 2023, 9:31 a.m. UTC | #2 
Tue, Mar 07, 2023 at 04:00:30PM CET, po-hsu.lin@canonical.com wrote:
>The `devlink -j port show` command output may not contain the "flavour"
>key, an example from s390x LPAR with Ubuntu 22.10 (5.19.0-37-generic),
>iproute2-5.15.0:
>  {"port":{"pci/0001:00:00.0/1":{"type":"eth","netdev":"ens301"},
>           "pci/0001:00:00.0/2":{"type":"eth","netdev":"ens301d1"},
>           "pci/0002:00:00.0/1":{"type":"eth","netdev":"ens317"},
>           "pci/0002:00:00.0/2":{"type":"eth","netdev":"ens317d1"}}}

As Jakub wrote, this is odd. Could you debug if kernel sends the flavour
attr and if not why? Also, could you try with most recent kernel?
Po-Hsu Lin March 8, 2023, 10:02 a.m. UTC | #3 
On Wed, Mar 8, 2023 at 9:02 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue,  7 Mar 2023 23:00:30 +0800 Po-Hsu Lin wrote:
> > The `devlink -j port show` command output may not contain the "flavour"
> > key, an example from s390x LPAR with Ubuntu 22.10 (5.19.0-37-generic),
> > iproute2-5.15.0:
> >   {"port":{"pci/0001:00:00.0/1":{"type":"eth","netdev":"ens301"},
> >            "pci/0001:00:00.0/2":{"type":"eth","netdev":"ens301d1"},
> >            "pci/0002:00:00.0/1":{"type":"eth","netdev":"ens317"},
> >            "pci/0002:00:00.0/2":{"type":"eth","netdev":"ens317d1"}}}
> >
> > This will cause a KeyError exception.
>
> I looked closer and I don't understand why the key is not there.
> Both 5.19 kernel should always put this argument out, and 5.15
> iproute2 should always interpret it.
>
> Am I looking wrong? Do you see how we can get a dump with no flavor?
>
> I worry that this is some endianness problem, and we just misreport
> stuff on big-endian.
>
> > Create a validate_devlink_output() to check for this "flavour" from
> > devlink command output to avoid this KeyError exception. Also let
> > it handle the check for `devlink -j dev show` output in main().
> >
> > Apart from this, if the test was not started because of any reason
> > (e.g. "lanes" does not exist, max lanes is 0 or the flavour of the
> > designated device is not "physical" and etc.) The script will still
> > return 0 and thus causing a false-negative test result.
> >
> > Use a test_ran flag to determine if these tests were skipped and
> > return KSFT_SKIP to make it more clear.
> >
> > V2: factor out the skip logic from main(), update commit message and
> >     skip reasons accordingly.
> > Link: https://bugs.launchpad.net/bugs/1937133
> > Fixes: f3348a82e727 ("selftests: net: Add port split test")
> > Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
> > ---
> >  tools/testing/selftests/net/devlink_port_split.py | 36 +++++++++++++++++++----
> >  1 file changed, 31 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/devlink_port_split.py b/tools/testing/selftests/net/devlink_port_split.py
> > index 2b5d6ff..749606c 100755
> > --- a/tools/testing/selftests/net/devlink_port_split.py
> > +++ b/tools/testing/selftests/net/devlink_port_split.py
> > @@ -59,6 +59,8 @@ class devlink_ports(object):
> >          assert stderr == ""
> >          ports = json.loads(stdout)['port']
> >
> > +        validate_devlink_output(ports, 'flavour')
>
> If it's just a matter of kernel/iproute2 version we shouldn't need to
> check here again?
>
> >          for port in ports:
> >              if dev in port:
> >                  if ports[port]['flavour'] == 'physical':
> > @@ -220,6 +222,27 @@ def split_splittable_port(port, k, lanes, dev):
> >      unsplit(port.bus_info)
> >
> >
> > +def validate_devlink_output(devlink_data, target_property=None):
> > +    """
> > +    Determine if test should be skipped by checking:
> > +      1. devlink_data contains values
> > +      2. The target_property exist in devlink_data
> > +    """
> > +    skip_reason = None
> > +    if any(devlink_data.values()):
> > +        if target_property:
> > +            skip_reason = "{} not found in devlink output, test skipped".format(target_property)
> > +            for key in devlink_data:
> > +                if target_property in devlink_data[key]:
> > +                    skip_reason = None
> > +    else:
> > +        skip_reason = 'devlink output is empty, test skipped'
> > +
> > +    if skip_reason:
> > +        print(skip_reason)
> > +        sys.exit(KSFT_SKIP)
>
> Looks good, so..
>
> >  def make_parser():
> >      parser = argparse.ArgumentParser(description='A test for port splitting.')
> >      parser.add_argument('--dev',
> > @@ -231,6 +254,7 @@ def make_parser():
> >
> >
> >  def main(cmdline=None):
> > +    test_ran = False
>
> I don't think we need the test_ran tracking any more?
We still need this here to check if the test was actually started.

Take the following output for example (ARM64 server with Mellanox
Ethernet controller, running Ubuntu 22.10 5.19.0-35):
$ devlink port show
pci/0000:01:00.0/65535: type eth netdev enp1s0f0 flavour physical port
0 splittable false
pci/0000:01:00.1/131071: type eth netdev enp1s0f1 flavour physical
port 1 splittable false
There is no "lanes" attribute here, thus the max_lanes in main() will
be 0. The test won't be started at all but returns 0.


>
> >      parser = make_parser()
> >      args = parser.parse_args(cmdline)
> >
> > @@ -240,12 +264,9 @@ def main(cmdline=None):
> >          stdout, stderr = run_command(cmd)
> >          assert stderr == ""
> >
> > +        validate_devlink_output(json.loads(stdout))
> >          devs = json.loads(stdout)['dev']
> > -        if devs:
> > -            dev = list(devs.keys())[0]
> > -        else:
> > -            print("no devlink device was found, test skipped")
> > -            sys.exit(KSFT_SKIP)
> > +        dev = list(devs.keys())[0]
> >
> >      cmd = "devlink dev show %s" % dev
> >      stdout, stderr = run_command(cmd)
> > @@ -277,6 +298,11 @@ def main(cmdline=None):
> >                  split_splittable_port(port, lane, max_lanes, dev)
> >
> >                  lane //= 2
> > +        test_ran = True
> > +
> > +    if not test_ran:
> > +        print("Test not started, no suitable device for the test")
> > +        sys.exit(KSFT_SKIP)
> >
> >
> >  if __name__ == "__main__":
>
Po-Hsu Lin March 8, 2023, 10:21 a.m. UTC | #4 
On Wed, Mar 8, 2023 at 5:31 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Tue, Mar 07, 2023 at 04:00:30PM CET, po-hsu.lin@canonical.com wrote:
> >The `devlink -j port show` command output may not contain the "flavour"
> >key, an example from s390x LPAR with Ubuntu 22.10 (5.19.0-37-generic),
> >iproute2-5.15.0:
> >  {"port":{"pci/0001:00:00.0/1":{"type":"eth","netdev":"ens301"},
> >           "pci/0001:00:00.0/2":{"type":"eth","netdev":"ens301d1"},
> >           "pci/0002:00:00.0/1":{"type":"eth","netdev":"ens317"},
> >           "pci/0002:00:00.0/2":{"type":"eth","netdev":"ens317d1"}}}
>
> As Jakub wrote, this is odd. Could you debug if kernel sends the flavour
> attr and if not why? Also, could you try with most recent kernel?

I did a quick check on another s390x LPAR instance which is running
with Ubuntu 23.04 (6.1.0-16-generic) iproute2-6.1.0, there is still no
"flavour" attribute.
$ devlink port show
pci/0001:00:00.0/1: type eth netdev ens301
pci/0001:00:00.0/2: type eth netdev ens301d1
pci/0002:00:00.0/1: type eth netdev ens317
pci/0002:00:00.0/2: type eth netdev ens317d1

The behaviour didn't change with iproute2 built from source [1]

[1] https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/
Jiri Pirko March 8, 2023, 11:41 a.m. UTC | #5 
Wed, Mar 08, 2023 at 11:21:57AM CET, po-hsu.lin@canonical.com wrote:
>On Wed, Mar 8, 2023 at 5:31 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Tue, Mar 07, 2023 at 04:00:30PM CET, po-hsu.lin@canonical.com wrote:
>> >The `devlink -j port show` command output may not contain the "flavour"
>> >key, an example from s390x LPAR with Ubuntu 22.10 (5.19.0-37-generic),
>> >iproute2-5.15.0:
>> >  {"port":{"pci/0001:00:00.0/1":{"type":"eth","netdev":"ens301"},
>> >           "pci/0001:00:00.0/2":{"type":"eth","netdev":"ens301d1"},
>> >           "pci/0002:00:00.0/1":{"type":"eth","netdev":"ens317"},
>> >           "pci/0002:00:00.0/2":{"type":"eth","netdev":"ens317d1"}}}
>>
>> As Jakub wrote, this is odd. Could you debug if kernel sends the flavour
>> attr and if not why? Also, could you try with most recent kernel?
>
>I did a quick check on another s390x LPAR instance which is running
>with Ubuntu 23.04 (6.1.0-16-generic) iproute2-6.1.0, there is still no
>"flavour" attribute.
>$ devlink port show
>pci/0001:00:00.0/1: type eth netdev ens301
>pci/0001:00:00.0/2: type eth netdev ens301d1
>pci/0002:00:00.0/1: type eth netdev ens317
>pci/0002:00:00.0/2: type eth netdev ens317d1
>
>The behaviour didn't change with iproute2 built from source [1]

Could you paste output of "devlink dev info"?
Looks like something might be wrong in the kernel devlink/driver code.
Po-Hsu Lin March 8, 2023, 2:37 p.m. UTC | #6 
On Wed, Mar 8, 2023 at 7:41 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Wed, Mar 08, 2023 at 11:21:57AM CET, po-hsu.lin@canonical.com wrote:
> >On Wed, Mar 8, 2023 at 5:31 PM Jiri Pirko <jiri@resnulli.us> wrote:
> >>
> >> Tue, Mar 07, 2023 at 04:00:30PM CET, po-hsu.lin@canonical.com wrote:
> >> >The `devlink -j port show` command output may not contain the "flavour"
> >> >key, an example from s390x LPAR with Ubuntu 22.10 (5.19.0-37-generic),
> >> >iproute2-5.15.0:
> >> >  {"port":{"pci/0001:00:00.0/1":{"type":"eth","netdev":"ens301"},
> >> >           "pci/0001:00:00.0/2":{"type":"eth","netdev":"ens301d1"},
> >> >           "pci/0002:00:00.0/1":{"type":"eth","netdev":"ens317"},
> >> >           "pci/0002:00:00.0/2":{"type":"eth","netdev":"ens317d1"}}}
> >>
> >> As Jakub wrote, this is odd. Could you debug if kernel sends the flavour
> >> attr and if not why? Also, could you try with most recent kernel?
> >
> >I did a quick check on another s390x LPAR instance which is running
> >with Ubuntu 23.04 (6.1.0-16-generic) iproute2-6.1.0, there is still no
> >"flavour" attribute.
> >$ devlink port show
> >pci/0001:00:00.0/1: type eth netdev ens301
> >pci/0001:00:00.0/2: type eth netdev ens301d1
> >pci/0002:00:00.0/1: type eth netdev ens317
> >pci/0002:00:00.0/2: type eth netdev ens317d1
> >
> >The behaviour didn't change with iproute2 built from source [1]
>
> Could you paste output of "devlink dev info"?
> Looks like something might be wrong in the kernel devlink/driver code.
>
The `devlink dev info` output is empty. The following output is from
that Ubuntu 23.04 s390x LPAR, run as root:
# devlink dev show
pci/0001:00:00.0
pci/0002:00:00.0
# devlink dev show pci/0001:00:00.0
pci/0001:00:00.0
# devlink dev info
# devlink dev info pci/0001:00:00.0
kernel answers: Operation not supported

>
Jiri Pirko March 8, 2023, 6:24 p.m. UTC | #7 
Wed, Mar 08, 2023 at 03:37:41PM CET, po-hsu.lin@canonical.com wrote:
>On Wed, Mar 8, 2023 at 7:41 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Wed, Mar 08, 2023 at 11:21:57AM CET, po-hsu.lin@canonical.com wrote:
>> >On Wed, Mar 8, 2023 at 5:31 PM Jiri Pirko <jiri@resnulli.us> wrote:
>> >>
>> >> Tue, Mar 07, 2023 at 04:00:30PM CET, po-hsu.lin@canonical.com wrote:
>> >> >The `devlink -j port show` command output may not contain the "flavour"
>> >> >key, an example from s390x LPAR with Ubuntu 22.10 (5.19.0-37-generic),
>> >> >iproute2-5.15.0:
>> >> >  {"port":{"pci/0001:00:00.0/1":{"type":"eth","netdev":"ens301"},
>> >> >           "pci/0001:00:00.0/2":{"type":"eth","netdev":"ens301d1"},
>> >> >           "pci/0002:00:00.0/1":{"type":"eth","netdev":"ens317"},
>> >> >           "pci/0002:00:00.0/2":{"type":"eth","netdev":"ens317d1"}}}
>> >>
>> >> As Jakub wrote, this is odd. Could you debug if kernel sends the flavour
>> >> attr and if not why? Also, could you try with most recent kernel?
>> >
>> >I did a quick check on another s390x LPAR instance which is running
>> >with Ubuntu 23.04 (6.1.0-16-generic) iproute2-6.1.0, there is still no
>> >"flavour" attribute.
>> >$ devlink port show
>> >pci/0001:00:00.0/1: type eth netdev ens301
>> >pci/0001:00:00.0/2: type eth netdev ens301d1
>> >pci/0002:00:00.0/1: type eth netdev ens317
>> >pci/0002:00:00.0/2: type eth netdev ens317d1
>> >
>> >The behaviour didn't change with iproute2 built from source [1]
>>
>> Could you paste output of "devlink dev info"?
>> Looks like something might be wrong in the kernel devlink/driver code.
>>
>The `devlink dev info` output is empty. The following output is from
>that Ubuntu 23.04 s390x LPAR, run as root:
># devlink dev show
>pci/0001:00:00.0
>pci/0002:00:00.0
># devlink dev show pci/0001:00:00.0
>pci/0001:00:00.0
># devlink dev info
># devlink dev info pci/0001:00:00.0

Interesting, could you try ethtool -i to get the driver name?


>kernel answers: Operation not supported
>
>>
Po-Hsu Lin March 9, 2023, 3:44 p.m. UTC | #8 
On Thu, Mar 9, 2023 at 2:24 AM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Wed, Mar 08, 2023 at 03:37:41PM CET, po-hsu.lin@canonical.com wrote:
> >On Wed, Mar 8, 2023 at 7:41 PM Jiri Pirko <jiri@resnulli.us> wrote:
> >>
> >> Wed, Mar 08, 2023 at 11:21:57AM CET, po-hsu.lin@canonical.com wrote:
> >> >On Wed, Mar 8, 2023 at 5:31 PM Jiri Pirko <jiri@resnulli.us> wrote:
> >> >>
> >> >> Tue, Mar 07, 2023 at 04:00:30PM CET, po-hsu.lin@canonical.com wrote:
> >> >> >The `devlink -j port show` command output may not contain the "flavour"
> >> >> >key, an example from s390x LPAR with Ubuntu 22.10 (5.19.0-37-generic),
> >> >> >iproute2-5.15.0:
> >> >> >  {"port":{"pci/0001:00:00.0/1":{"type":"eth","netdev":"ens301"},
> >> >> >           "pci/0001:00:00.0/2":{"type":"eth","netdev":"ens301d1"},
> >> >> >           "pci/0002:00:00.0/1":{"type":"eth","netdev":"ens317"},
> >> >> >           "pci/0002:00:00.0/2":{"type":"eth","netdev":"ens317d1"}}}
> >> >>
> >> >> As Jakub wrote, this is odd. Could you debug if kernel sends the flavour
> >> >> attr and if not why? Also, could you try with most recent kernel?
> >> >
> >> >I did a quick check on another s390x LPAR instance which is running
> >> >with Ubuntu 23.04 (6.1.0-16-generic) iproute2-6.1.0, there is still no
> >> >"flavour" attribute.
> >> >$ devlink port show
> >> >pci/0001:00:00.0/1: type eth netdev ens301
> >> >pci/0001:00:00.0/2: type eth netdev ens301d1
> >> >pci/0002:00:00.0/1: type eth netdev ens317
> >> >pci/0002:00:00.0/2: type eth netdev ens317d1
> >> >
> >> >The behaviour didn't change with iproute2 built from source [1]
> >>
> >> Could you paste output of "devlink dev info"?
> >> Looks like something might be wrong in the kernel devlink/driver code.
> >>
> >The `devlink dev info` output is empty. The following output is from
> >that Ubuntu 23.04 s390x LPAR, run as root:
> ># devlink dev show
> >pci/0001:00:00.0
> >pci/0002:00:00.0
> ># devlink dev show pci/0001:00:00.0
> >pci/0001:00:00.0
> ># devlink dev info
> ># devlink dev info pci/0001:00:00.0
>
> Interesting, could you try ethtool -i to get the driver name?
>
Hi,

Here you go:
$ ethtool -i ens301
driver: mlx4_en
version: 4.0-0
firmware-version: 2.35.5100
expansion-rom-version:
bus-info: 0001:00:00.0
supports-statistics: yes
supports-test: yes
supports-eeprom-access: no
supports-register-dump: no
supports-priv-flags: yes

$ ethtool -i ens317
driver: mlx4_en
version: 4.0-0
firmware-version: 2.35.5100
expansion-rom-version:
bus-info: 0002:00:00.0
supports-statistics: yes
supports-test: yes
supports-eeprom-access: no
supports-register-dump: no
supports-priv-flags: yes

HTH


>
> >kernel answers: Operation not supported
> >
> >>
Jiri Pirko March 10, 2023, 8:32 a.m. UTC | #9 
Thu, Mar 09, 2023 at 04:44:10PM CET, po-hsu.lin@canonical.com wrote:
>On Thu, Mar 9, 2023 at 2:24 AM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Wed, Mar 08, 2023 at 03:37:41PM CET, po-hsu.lin@canonical.com wrote:
>> >On Wed, Mar 8, 2023 at 7:41 PM Jiri Pirko <jiri@resnulli.us> wrote:
>> >>
>> >> Wed, Mar 08, 2023 at 11:21:57AM CET, po-hsu.lin@canonical.com wrote:
>> >> >On Wed, Mar 8, 2023 at 5:31 PM Jiri Pirko <jiri@resnulli.us> wrote:
>> >> >>
>> >> >> Tue, Mar 07, 2023 at 04:00:30PM CET, po-hsu.lin@canonical.com wrote:
>> >> >> >The `devlink -j port show` command output may not contain the "flavour"
>> >> >> >key, an example from s390x LPAR with Ubuntu 22.10 (5.19.0-37-generic),
>> >> >> >iproute2-5.15.0:
>> >> >> >  {"port":{"pci/0001:00:00.0/1":{"type":"eth","netdev":"ens301"},
>> >> >> >           "pci/0001:00:00.0/2":{"type":"eth","netdev":"ens301d1"},
>> >> >> >           "pci/0002:00:00.0/1":{"type":"eth","netdev":"ens317"},
>> >> >> >           "pci/0002:00:00.0/2":{"type":"eth","netdev":"ens317d1"}}}
>> >> >>
>> >> >> As Jakub wrote, this is odd. Could you debug if kernel sends the flavour
>> >> >> attr and if not why? Also, could you try with most recent kernel?
>> >> >
>> >> >I did a quick check on another s390x LPAR instance which is running
>> >> >with Ubuntu 23.04 (6.1.0-16-generic) iproute2-6.1.0, there is still no
>> >> >"flavour" attribute.
>> >> >$ devlink port show
>> >> >pci/0001:00:00.0/1: type eth netdev ens301
>> >> >pci/0001:00:00.0/2: type eth netdev ens301d1
>> >> >pci/0002:00:00.0/1: type eth netdev ens317
>> >> >pci/0002:00:00.0/2: type eth netdev ens317d1
>> >> >
>> >> >The behaviour didn't change with iproute2 built from source [1]
>> >>
>> >> Could you paste output of "devlink dev info"?
>> >> Looks like something might be wrong in the kernel devlink/driver code.
>> >>
>> >The `devlink dev info` output is empty. The following output is from
>> >that Ubuntu 23.04 s390x LPAR, run as root:
>> ># devlink dev show
>> >pci/0001:00:00.0
>> >pci/0002:00:00.0
>> ># devlink dev show pci/0001:00:00.0
>> >pci/0001:00:00.0
>> ># devlink dev info
>> ># devlink dev info pci/0001:00:00.0
>>
>> Interesting, could you try ethtool -i to get the driver name?
>>
>Hi,
>
>Here you go:
>$ ethtool -i ens301
>driver: mlx4_en
>version: 4.0-0
>firmware-version: 2.35.5100
>expansion-rom-version:
>bus-info: 0001:00:00.0
>supports-statistics: yes
>supports-test: yes
>supports-eeprom-access: no
>supports-register-dump: no
>supports-priv-flags: yes
>
>$ ethtool -i ens317
>driver: mlx4_en

mlx4 is indeed not setting attrs. So you patch is needed.


>version: 4.0-0
>firmware-version: 2.35.5100
>expansion-rom-version:
>bus-info: 0002:00:00.0
>supports-statistics: yes
>supports-test: yes
>supports-eeprom-access: no
>supports-register-dump: no
>supports-priv-flags: yes
>
>HTH
>
>
>>
>> >kernel answers: Operation not supported
>> >
>> >>
Jakub Kicinski March 11, 2023, 12:05 a.m. UTC | #10 
On Tue,  7 Mar 2023 23:00:30 +0800 Po-Hsu Lin wrote:
>  def main(cmdline=None):
> +    test_ran = False

Could you move this variable init right before the 

	for port in ports.if_names:

line, and call it something like found_max_lanes ?

>      parser = make_parser()
>      args = parser.parse_args(cmdline)
>  
> @@ -240,12 +264,9 @@ def main(cmdline=None):
>          stdout, stderr = run_command(cmd)
>          assert stderr == ""
>  
> +        validate_devlink_output(json.loads(stdout))
>          devs = json.loads(stdout)['dev']
> -        if devs:
> -            dev = list(devs.keys())[0]
> -        else:
> -            print("no devlink device was found, test skipped")
> -            sys.exit(KSFT_SKIP)
> +        dev = list(devs.keys())[0]
>  
>      cmd = "devlink dev show %s" % dev
>      stdout, stderr = run_command(cmd)
> @@ -277,6 +298,11 @@ def main(cmdline=None):
>                  split_splittable_port(port, lane, max_lanes, dev)
>  
>                  lane //= 2
> +        test_ran = True
> +
> +    if not test_ran:
> +        print("Test not started, no suitable device for the test")

Then change the message to 

	f"Test not started, no port of device {dev} reports max_lanes"

> +        sys.exit(KSFT_SKIP)
>
Po-Hsu Lin March 15, 2023, 9:11 a.m. UTC | #11 
On Sat, Mar 11, 2023 at 8:05 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue,  7 Mar 2023 23:00:30 +0800 Po-Hsu Lin wrote:
> >  def main(cmdline=None):
> > +    test_ran = False
>
> Could you move this variable init right before the
>
>         for port in ports.if_names:
>
> line, and call it something like found_max_lanes ?
>
> >      parser = make_parser()
> >      args = parser.parse_args(cmdline)
> >
> > @@ -240,12 +264,9 @@ def main(cmdline=None):
> >          stdout, stderr = run_command(cmd)
> >          assert stderr == ""
> >
> > +        validate_devlink_output(json.loads(stdout))
> >          devs = json.loads(stdout)['dev']
> > -        if devs:
> > -            dev = list(devs.keys())[0]
> > -        else:
> > -            print("no devlink device was found, test skipped")
> > -            sys.exit(KSFT_SKIP)
> > +        dev = list(devs.keys())[0]
> >
> >      cmd = "devlink dev show %s" % dev
> >      stdout, stderr = run_command(cmd)
> > @@ -277,6 +298,11 @@ def main(cmdline=None):
> >                  split_splittable_port(port, lane, max_lanes, dev)
> >
> >                  lane //= 2
> > +        test_ran = True
> > +
> > +    if not test_ran:
> > +        print("Test not started, no suitable device for the test")
>
> Then change the message to
>
>         f"Test not started, no port of device {dev} reports max_lanes"
>

Sure,
will update this in V3
Thanks for the feedback!

> > +        sys.exit(KSFT_SKIP)
> >
diff mbox series

Patch

diff --git a/tools/testing/selftests/net/devlink_port_split.py b/tools/testing/selftests/net/devlink_port_split.py
index 2b5d6ff..749606c 100755
--- a/tools/testing/selftests/net/devlink_port_split.py
+++ b/tools/testing/selftests/net/devlink_port_split.py
@@ -59,6 +59,8 @@  class devlink_ports(object):
         assert stderr == ""
         ports = json.loads(stdout)['port']
 
+        validate_devlink_output(ports, 'flavour')
+
         for port in ports:
             if dev in port:
                 if ports[port]['flavour'] == 'physical':
@@ -220,6 +222,27 @@  def split_splittable_port(port, k, lanes, dev):
     unsplit(port.bus_info)
 
 
+def validate_devlink_output(devlink_data, target_property=None):
+    """
+    Determine if test should be skipped by checking:
+      1. devlink_data contains values
+      2. The target_property exist in devlink_data
+    """
+    skip_reason = None
+    if any(devlink_data.values()):
+        if target_property:
+            skip_reason = "{} not found in devlink output, test skipped".format(target_property)
+            for key in devlink_data:
+                if target_property in devlink_data[key]:
+                    skip_reason = None
+    else:
+        skip_reason = 'devlink output is empty, test skipped'
+
+    if skip_reason:
+        print(skip_reason)
+        sys.exit(KSFT_SKIP)
+
+
 def make_parser():
     parser = argparse.ArgumentParser(description='A test for port splitting.')
     parser.add_argument('--dev',
@@ -231,6 +254,7 @@  def make_parser():
 
 
 def main(cmdline=None):
+    test_ran = False
     parser = make_parser()
     args = parser.parse_args(cmdline)
 
@@ -240,12 +264,9 @@  def main(cmdline=None):
         stdout, stderr = run_command(cmd)
         assert stderr == ""
 
+        validate_devlink_output(json.loads(stdout))
         devs = json.loads(stdout)['dev']
-        if devs:
-            dev = list(devs.keys())[0]
-        else:
-            print("no devlink device was found, test skipped")
-            sys.exit(KSFT_SKIP)
+        dev = list(devs.keys())[0]
 
     cmd = "devlink dev show %s" % dev
     stdout, stderr = run_command(cmd)
@@ -277,6 +298,11 @@  def main(cmdline=None):
                 split_splittable_port(port, lane, max_lanes, dev)
 
                 lane //= 2
+        test_ran = True
+
+    if not test_ran:
+        print("Test not started, no suitable device for the test")
+        sys.exit(KSFT_SKIP)
 
 
 if __name__ == "__main__":