| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<DS0PR11MB8050F500D3422F8D06DB374B834E2@DS0PR11MB8050.namprd11.prod.outlook.com>
Date: Thu, 24 Oct 2024 07:26:18 +0000
From: <Mohan.Prasad@...rochip.com>
To: <pabeni@...hat.com>, <netdev@...r.kernel.org>
CC: <davem@...emloft.net>, <kuba@...nel.org>, <andrew@...n.ch>,
<edumazet@...gle.com>, <shuah@...nel.org>, <linux-kernel@...r.kernel.org>,
<linux-kselftest@...r.kernel.org>, <horms@...nel.org>,
<brett.creeley@....com>, <rosenp@...il.com>, <UNGLinuxDriver@...rochip.com>,
<willemb@...gle.com>, <petrm@...dia.com>
Subject: 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@...rochip.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
Powered by blists - more mailing lists