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

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

[Po-Hsu Lin]

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(-)

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__":
-- 
2.7.4

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

[Jakub Kicinski]

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__":

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

[Po-Hsu Lin]

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__":
>

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

[Jiri Pirko]

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?

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

[Po-Hsu Lin]

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/

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

[Jiri Pirko]

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.

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

[Po-Hsu Lin]

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

>

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

[Jiri Pirko]

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
>
>>

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

[Po-Hsu Lin]

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
> >
> >>

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

[Jiri Pirko]

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
>> >
>> >>

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

[Jakub Kicinski]

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)
>  

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

[Po-Hsu Lin]

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)
> >