netdev - Re: [RFC PATCH net-next 2/7] net: ethtool: add support for Frame Preemption and MAC Merge layer

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: <20220817114100.7pu6y4lpnedyc3fg@skbuf>
Date:   Wed, 17 Aug 2022 11:41:01 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Michal Kubecek <mkubecek@...e.cz>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Vinicius Costa Gomes <vinicius.gomes@...el.com>,
        Xiaoliang Yang <xiaoliang.yang_1@....com>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Rui Sousa <rui.sousa@....com>,
        Ferenc Fejes <ferenc.fejes@...csson.com>
Subject: Re: [RFC PATCH net-next 2/7] net: ethtool: add support for Frame
 Preemption and MAC Merge layer

On Tue, Aug 16, 2022 at 08:22:09PM -0700, Jakub Kicinski wrote:
> On Wed, 17 Aug 2022 01:29:15 +0300 Vladimir Oltean wrote:
> > +/**
> > + * struct ethtool_mm_state - 802.3 MAC merge layer state
> > + */
> > +struct ethtool_mm_state {
> > +	u32 verify_time;
> > +	enum ethtool_mm_verify_status verify_status;
> > +	bool supported;
> > +	bool enabled;
> > +	bool active;
> 
> The enabled vs active piqued my interest. Is there some handshake /
> aneg or such?

This is where writing the kdocs, as mentioned in the cover letter, would
have helped. Yes, the handshake is described in 802.3 clause "99.4.3
Verifying preemption operation". It says:

| Verification (see Figure 99–3) checks that the link can support the
| preemption capability.
|
| If verification is enabled, the preemption capability shall be active
| only after verification has completed
| successfully.
|
| If the preemption capability is enabled but has not been verified yet,
| the MAC Merge sublayer initiates verification. Verification relies on
| the transmission of a verify mPacket and receipt of a respond mPacket to
| confirm that the remote station supports the preemption capability.

In fact, the verify_time and verify_status fields right above
enabled/active are related exactly to this handshake process.

> > +	nest_table = nla_nest_start(skb, ETHTOOL_A_FP_PARAM_TABLE);
> > +	if (!nest_table)
> > +		return -EMSGSIZE;
> 
> Don't warp tables in nests, let the elements repeat in the parent.

Ok, can do. I did this because 802.1Q actually specifies in the
IEEE8021-Preemption-MIB that there is a ieee8021PreemptionParameterTable
structure containing pairs of ieee8021PreemptionPriority and
ieee8021FramePreemptionAdminStatus.

Do you have actual issues with the structuring of the FP parameters
though? They look like this currently:

ethtool --json --show-fp eno0
[ {
        "ifname": "eno0",
        "parameter-table": [ {
                "prio": 0,
                "admin-status": "preemptable"
            },{
                "prio": 1,
                "admin-status": "preemptable"
            },{
                "prio": 2,
                "admin-status": "preemptable"
            },{
                "prio": 3,
                "admin-status": "preemptable"
            },{
                "prio": 4,
                "admin-status": "preemptable"
            },{
                "prio": 5,
                "admin-status": "preemptable"
            },{
                "prio": 6,
                "admin-status": "preemptable"
            },{
                "prio": 7,
                "admin-status": "preemptable"
            } ],
        "hold-advance": 0,
        "release-advance": 0
    } ]