netdev - RE: [PATCH net-next v2 1/3] selftests: nic_basic_tests: Add selftest file for basic tests of NIC

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <DM6PR11MB4236AB0ADF93D8A4EBBC59EF83622@DM6PR11MB4236.namprd11.prod.outlook.com>
Date: Wed, 18 Sep 2024 10:35:16 +0000
From: <Mohan.Prasad@...rochip.com>
To: <andrew@...n.ch>
CC: <netdev@...r.kernel.org>, <davem@...emloft.net>, <kuba@...nel.org>,
	<edumazet@...gle.com>, <pabeni@...hat.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>
Subject: RE: [PATCH net-next v2 1/3] selftests: nic_basic_tests: Add selftest
 file for basic tests of NIC

Hi Andrew,

Thank you for the review comments.

> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Tue, Sep 17, 2024 at 08:04:07AM +0530, Mohan Prasad J wrote:
> > Add selftest file to test basic features of a NIC driver.
> > Tests for link modes, auto-negotiation are placed.
> > Selftest makes use of ksft modules and ethtool.
> > Add 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/nic_basic_tests.py         | 145 ++++++++++++++++++
> >  2 files changed, 146 insertions(+)
> >  create mode 100644
> > tools/testing/selftests/drivers/net/hw/nic_basic_tests.py
> >
> > diff --git a/tools/testing/selftests/drivers/net/hw/Makefile
> > b/tools/testing/selftests/drivers/net/hw/Makefile
> > index c9f2f48fc..9f105227c 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_basic_tests.py \
> >       pp_alloc_fail.py \
> >       rss_ctx.py \
> >       #
> > diff --git a/tools/testing/selftests/drivers/net/hw/nic_basic_tests.py
> > b/tools/testing/selftests/drivers/net/hw/nic_basic_tests.py
> > new file mode 100644
> > index 000000000..27f780032
> > --- /dev/null
> > +++ b/tools/testing/selftests/drivers/net/hw/nic_basic_tests.py
> > @@ -0,0 +1,145 @@
> > +#!/usr/bin/env python3
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +#Introduction:
> > +#This file has basic tests for generic NIC drivers.
> > +#The test comprises of auto-negotiation, speed and duplex checks.
> > +#Also has tests to check the throughput #
> > +#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:
> > +# Change the below configuration based on your hw needs.
> > +# """Default values"""
> > +sleep_time = 5 #time taken to wait for transitions to happen, in seconds.
> > +test_duration = 5  #performance test duration for the throughput check, in
> seconds.
> > +throughput_threshold = 0.8 #percentage of throughput required to pass
> > +the throughput
> > +
> > +import time
> > +import os
> > +import re
> > +import configparser
> > +import json
> > +from lib.py import ksft_run, ksft_exit, ksft_pr, ksft_eq from lib.py
> > +import KsftFailEx, KsftSkipEx from lib.py import NetDrvEpEnv from
> > +lib.py import cmd from lib.py import ethtool
> > +
> > +"""Global variables"""
> > +common_link_modes = []
> 
> Software engineers have a dislike for global variables. In this patch, it is not
> even used. Please consider replacing it by passing it as a parameter, or turn
> the code into a class and make it part of self.

I will change this in the next version.

> 
> > +def test_link_modes(cfg) -> None:
> > +    global common_link_modes
> > +    link_modes = get_ethtool_content(cfg.ifname, "Supported link modes:")
> > +    partner_link_modes = get_ethtool_content(cfg.ifname, "Link
> > +partner advertised link modes:")
> > +
> > +    if link_modes and partner_link_modes:
> > +        for idx1 in range(len(link_modes)):
> > +            for idx2 in range(len(partner_link_modes)):
> > +                if link_modes[idx1] == partner_link_modes[idx2]:
> > +                    common_link_modes.append(link_modes[idx1])
> 
> You can use the power of python here.
> 
>         "supported-link-modes": [
> "10baseT/Half","10baseT/Full","100baseT/Half","100baseT/Full","1000baseT/
> Full" ],
>         "link-partner-modes": [
> "10baseT/Half","10baseT/Full","100baseT/Half","100baseT/Full","1000baseT/
> Full" ],
> 
> convert the list into a set, and then use 'and'.
> 
>         common_modes = set(josn['supported-link-modes']) and set(json['link-
> partner-modes'])

I am figuring out the way to get the ethtool output in json format.
Once successful, I will change this implementation as suggested in next version.