RE: [PATCH net-next v3 1/3] selftests: nic_link_layer: Add link layer selftest for NIC driver

Hello Paolo,

Thank you for the review comments.

...

Add selftest file for the link layer tests of a NIC driver. Test for auto-negotiation is added. Add LinkConfig class for changing link layer configs. Selftest makes use of ksft modules and ethtool. Include selftest file in the Makefile.

Signed-off-by: Mohan Prasad J mohan.prasad@microchip.com

.../testing/selftests/drivers/net/hw/Makefile | 1 + .../drivers/net/hw/lib/py/__init__.py | 1 + .../drivers/net/hw/lib/py/linkconfig.py | 220 ++++++++++++++++++ .../drivers/net/hw/nic_link_layer.py | 84 +++++++ 4 files changed, 306 insertions(+) create mode 100644 tools/testing/selftests/drivers/net/hw/lib/py/linkconfig.py create mode 100644 tools/testing/selftests/drivers/net/hw/nic_link_layer.py

diff --git a/tools/testing/selftests/drivers/net/hw/Makefile b/tools/testing/selftests/drivers/net/hw/Makefile index c9f2f48fc..0dac40c4e 100644 --- a/tools/testing/selftests/drivers/net/hw/Makefile +++ b/tools/testing/selftests/drivers/net/hw/Makefile @@ -10,6 +10,7 @@ TEST_PROGS = \ hw_stats_l3.sh \ hw_stats_l3_gre.sh \ loopback.sh \

• nic_link_layer.py \
  pp_alloc_fail.py \
  rss_ctx.py \
  #
  

diff --git a/tools/testing/selftests/drivers/net/hw/lib/py/__init__.py b/tools/testing/selftests/drivers/net/hw/lib/py/__init__.py index b58288578..399789a96 100644 --- a/tools/testing/selftests/drivers/net/hw/lib/py/__init__.py +++ b/tools/testing/selftests/drivers/net/hw/lib/py/__init__.py @@ -9,6 +9,7 @@ try: sys.path.append(KSFT_DIR.as_posix()) from net.lib.py import * from drivers.net.lib.py import *

• from .linkconfig import LinkConfig

except ModuleNotFoundError as e: ksft_pr("Failed importing `net` library from kernel sources") ksft_pr(str(e)) diff --git a/tools/testing/selftests/drivers/net/hw/lib/py/linkconfig.py b/tools/testing/selftests/drivers/net/hw/lib/py/linkconfig.py new file mode 100644 index 000000000..86cbf10a3 --- /dev/null +++ b/tools/testing/selftests/drivers/net/hw/lib/py/linkconfig.py @@ -0,0 +1,220 @@ +# SPDX-License-Identifier: GPL-2.0

+from lib.py import cmd +from lib.py import ethtool +from lib.py import ksft_pr, ksft_eq +import re +import time +import json

+#The LinkConfig class is implemented to handle the link layer

configurations.

...

+#Required minimum ethtool version is 6.10 #The ethtool and ip would +require authentication to make changes, so better # to check them for +sudo privileges for interruption free testing.

+class LinkConfig:

• """Class for handling the link layer configurations"""
• def __init__(self, cfg):

•    self.cfg = cfg
  

•    self.partner_netif = self.get_partner_netif_name()
  

•    """Get the initial link configuration of local interface"""
  

•    self.common_link_modes = self.get_common_link_modes()
  

• def get_partner_netif_name(self):

You should use type hints for both the arguments and the return type.

I will update the type hints wherever applicable, you can find it in the next revision.

...

•    partner_netif = None
  

•    try:
  

•        """Get partner interface name"""
  

•        partner_cmd = f"ip -o -f inet addr show | grep
  

'{self.cfg.remote_addr}' " + "| awk '{print $2}'"

It's better if you use json output even here

I will update it for json output, you can find it in next version.

[...]

...

• def reset_interface(self, local=True, remote=True):

•    ksft_pr("Resetting interfaces in local and remote")
  

•    if remote:
  

•        if self.partner_netif is not None:
  

•            ifname = self.partner_netif
  

•            link_up_cmd = f"sudo ip link set up {ifname}"
  

•            link_down_cmd = f"sudo ip link set down {ifname}"
  

•            reset_cmd = f"{link_down_cmd} && sleep 5 && {link_up_cmd}"
  

•            try:
  

•                cmd(f"{reset_cmd}", host=self.cfg.remote)
  

•            except Exception as e:
  

•                ksft_pr("Check sudo permission for ip command")
  

•                ksft_pr(f"Unexpected error occurred: {e}")
  

Please, don't use sudo, just run the command directly. The selftests should be executed only by privileged users.

The sudo will be removed in the next revision.

[...]

...

• def check_autoneg_supported(self, remote=False):

•    if remote==False:
  

•        local_autoneg = self.get_ethtool_field("supports-auto-negotiation")
  

•        if local_autoneg is None:
  

•            ksft_pr(f"Unable to fetch auto-negotiation status for interface
  

{self.cfg.ifname}")

...

•        """Return autoneg status of the local interface"""
  

•        status = True if local_autoneg == True else False
  

•        return status
  

Out of sheer ignorance, in't

            return local_autoneg

enough?

This will be updated to return just the local_autoneg.

...

•    else:
  

•        """Check remote auto-negotiation support status"""
  

•        partner_autoneg = False
  

•        if self.partner_netif is not None:
  

•            partner_autoneg = self.get_ethtool_field("supports-auto-
  

negotiation", remote=True)

...

•            if partner_autoneg is None:
  

•                ksft_pr(f"Unable to fetch auto-negotiation status for interface
  

{partner_netif}")

...

•        status = True if partner_autoneg is True else False
  

•        return status
  

• def get_common_link_modes(self):

•    common_link_modes = None
  

•    """Populate common link modes"""
  

•    link_modes = self.get_ethtool_field("supported-link-modes")
  

•    partner_link_modes = self.get_ethtool_field("link-partner-advertised-
  

link-modes")

...

•    if link_modes is None:
  

•        raise Exception(f"Link modes not available for {self.cfg.ifname}")
  

•    if partner_link_modes is None:
  

•        raise Exception(f"Partner link modes not available for
  

{self.cfg.ifname}")

...

•    common_link_modes = set(link_modes) and set(partner_link_modes)
  

•    return common_link_modes
  

• def get_speed_duplex_values(self, link_modes):

•    speed = []
  

•    duplex = []
  

•    """Check the link modes"""
  

•    for data in link_modes:
  

•        parts = data.split('/')
  

•        speed_value = re.match(r'\d+', parts[0])
  

•        if speed_value:
  

•            speed.append(speed_value.group())
  

•        else:
  

•            ksft_pr(f"No speed value found for interface {self.ifname}")
  

•            return None, None
  

•        duplex.append(parts[1].lower())
  

•    return speed, duplex
  

• def get_ethtool_field(self, field: str, remote=False):

•    process = None
  

•    if remote == False:
  

•        """Get the ethtool field value for the local interface"""
  

•        ifname = self.cfg.ifname
  

•        try:
  

•            process = ethtool(f"--json {ifname}")
  

•        except Exception as e:
  

•            ksft_pr("Required minimum ethtool version is 6.10")
  

•            ksft_pr(f"Unexpected error occurred: {e}")
  

•    else:
  

•        """Get the ethtool field value for the remote interface"""
  

•        remote = True
  

•        ifname = self.partner_netif
  

•        self.cfg.require_cmd("ethtool", remote)
  

•        command = f"ethtool --json {ifname}"
  

•        try:
  

•            process = cmd(command, host=self.cfg.remote)
  

•        except Exception as e:
  

•            ksft_pr("Required minimum ethtool version is 6.10")
  

•            ksft_pr("Unexpected error occurred: {e}")
  

•    if process is None or process.ret != 0:
  

•        print(f"Error while getting the ethtool content for interface {ifname}.
  

Required minimum ethtool version is 6.10")

...

•        return None
  

•    output = json.loads(process.stdout)
  

•    json_data = output[0]
  

•    """Check if the field exist in the json data"""
  

•    if field not in json_data:
  

•        raise Exception(f"Field {field} does not exist in the output of
  

interface {json_data["ifname"]}")

...

•        return None
  

•    return json_data[field]
  

diff --git a/tools/testing/selftests/drivers/net/hw/nic_link_layer.py b/tools/testing/selftests/drivers/net/hw/nic_link_layer.py new file mode 100644 index 000000000..fb046efbe --- /dev/null +++ b/tools/testing/selftests/drivers/net/hw/nic_link_layer.py @@ -0,0 +1,84 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: GPL-2.0

+#Introduction: +#This file has basic link layer tests for generic NIC drivers. +#The test comprises of auto-negotiation, speed and duplex checks. +# +#Setup: +#Connect the DUT PC with NIC card to partner pc back via ethernet +medium of your choice(RJ45, T1) # +# DUT PC Partner PC +#┌───────────────────────┐

┌──────────────────────────┐

...

+#│ │ │ │ +#│ │ │ │ +#│ ┌───────────┐ │ │ +#│ │DUT NIC │ Eth │ │ +#│ │Interface ─┼─────────────────────────┼─ any eth Interface

│

...

+#│ └───────────┘ │ │ +#│ │ │ │ +#│ │ │ │ +#└───────────────────────┘

└──────────────────────────┘

...

+# +#Configurations: +#To prevent interruptions, Add ethtool, ip to the sudoers list in remote PC

and get the ssh key from remote.

...

+#Required minimum ethtool version is 6.10 #Change the below +configuration based on your hw needs. +# """Default values""" +time_delay = 8 #time taken to wait for transitions to happen, in seconds. +test_duration = 10 #performance test duration for the throughput check,

in seconds.

It would be probably useful to allow the user overriding the above values via env variables and/or command line arguments.

This shall be changed to take variables from env variables or command line arguments, given that, command line arguments given more priority.

'test_duration' declaration should probably moved to a later patch, where it's used.

This will be moved to later patch where it is used. You shall find all the changes in the next revision.

Thanks, Mohan Prasad J