| Message ID | 20230307150030.527726-1-po-hsu.lin@canonical.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv2] selftests: net: devlink_port_split.py: skip test if no suitable device available | expand |
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__":
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?
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__": >
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/
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.
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 >
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 > >>
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 > > > >>
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 >> > >> >>
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) >
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 --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__":
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(-)