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

[Vladimir Oltean <vladimir.oltean@nxp.com>]

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
    } ]