Re: [PATCH v2 net-next 03/12] docs: ethtool-netlink: document interface for MAC Merge layer

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 11 Jan 2023 18:16:57 +0200 Vladimir Oltean wrote:
> Show details about the structures passed back and forth related to MAC
> Merge layer configuration, state and statistics. The rendered htmldocs
> will be much more verbose due to the kerneldoc references.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> v1->v2: patch is new
> 
>  Documentation/networking/ethtool-netlink.rst | 103 +++++++++++++++++++
>  Documentation/networking/statistics.rst      |   1 +
>  2 files changed, 104 insertions(+)
> 
> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index f10f8eb44255..490c2280ce4f 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -223,6 +223,8 @@ Userspace to kernel:
>    ``ETHTOOL_MSG_PSE_SET``               set PSE parameters
>    ``ETHTOOL_MSG_PSE_GET``               get PSE parameters
>    ``ETHTOOL_MSG_RSS_GET``               get RSS settings
> +  ``ETHTOOL_MSG_MM_GET``                get MAC merge layer state
> +  ``ETHTOOL_MSG_MM_SET``                set MAC merge layer parameters
>    ===================================== =================================
>  
>  Kernel to userspace:
> @@ -265,6 +267,7 @@ Kernel to userspace:
>    ``ETHTOOL_MSG_MODULE_GET_REPLY``         transceiver module parameters
>    ``ETHTOOL_MSG_PSE_GET_REPLY``            PSE parameters
>    ``ETHTOOL_MSG_RSS_GET_REPLY``            RSS settings
> +  ``ETHTOOL_MSG_MM_GET_REPLY``             MAC merge layer status
>    ======================================== =================================
>  
>  ``GET`` requests are sent by userspace applications to retrieve device
> @@ -1716,6 +1719,104 @@ being used. Current supported options are toeplitz, xor or crc32.
>  ETHTOOL_A_RSS_INDIR attribute returns RSS indrection table where each byte
>  indicates queue number.
>  
> +MM_GET
> +======
> +
> +Retrieve 802.3 MAC Merge parameters.
> +
> +Request contents:
> +
> +  ====================================  ======  ==========================
> +  ``ETHTOOL_A_MM_HEADER``               nested  request header
> +  ====================================  ======  ==========================
> +
> +Kernel response contents:
> +
> +  ================================  ======  ===================================
> +  ``ETHTOOL_A_MM_HEADER``           Nested  request header
> +
> +  ``ETHTOOL_A_MM_SUPPORTED``        Bool    set if device supports the MM layer

I'm guessing the empty lines are to improve readability?
(They are not required for the table to render correctly.)

Why did you capitalize the types, tho?

> +  ``ETHTOOL_A_MM_PMAC_ENABLED``     Bool    set if RX of preemptible and SMD-V
> +                                            frames is enabled

s/is/are/ ?

> +  ``ETHTOOL_A_MM_TX_ENABLED``       Bool    set if TX of preemptible frames is
> +                                            administratively enabled (might be
> +                                            inactive if verification failed)
> +
> +  ``ETHTOOL_A_MM_TX_ACTIVE``        Bool    set if TX of preemptible frames is
> +                                            operationally enabled
> +
> +  ``ETHTOOL_A_MM_ADD_FRAG_SIZE``    U32     minimum size of transmitted
> +                                            non-final fragments, in octets
> +
> +  ``ETHTOOL_A_MM_VERIFY_ENABLED``   Bool    set if TX of SMD-V frames is
> +                                            administratively enabled (TX will
> +                                            not take place when port is not up)

The sentence in the brackets seems obvious, is there some special 
MM meaning to "port is not up"? You're not talking about the link
being up?

> +  ``ETHTOOL_A_MM_VERIFY_STATUS``    U8      state of the Verify function

Only places you say "Verify function" rather than just "verification",
not sure that's on purpose.

> +  ``ETHTOOL_A_MM_VERIFY_TIME``      U32     delay between verification attempts
> +
> +  ``ETHTOOL_A_MM_MAX_VERIFY_TIME``  U32     maximum interval supported by

s/interval/verification interval/ ?

> +                                            device
> +
> +  ``ETHTOOL_A_MM_STATS``            Nested  IEEE 802.3-2018 subclause 30.14.1
> +                                            oMACMergeEntity statistics counters
> +

The empty line between last entry and delimiter can go

> +  ================================  ======  ===================================
> +
> +If ``ETHTOOL_A_MM_SUPPORTED`` is reported as false, the other netlink
> +attributes will be absent.

Why not return -EOPNOTSUPP?
You do so in case driver does not have the op:

+	if (!ops->get_mm)
+		return -EOPNOTSUPP;