linux-kernel - Re: [PATCH net-next v5 2/5] ethtool: netlink: add ETHTOOL_MSG_MSE

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] [day] [month] [year] [list]

Message-ID: <20250911193440.1db7c6b4@kernel.org>
Date: Thu, 11 Sep 2025 19:34:40 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Andrew Lunn <andrew@...n.ch>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Simon
 Horman <horms@...nel.org>, Donald Hunter <donald.hunter@...il.com>,
 Jonathan Corbet <corbet@....net>, Heiner Kallweit <hkallweit1@...il.com>,
 Russell King <linux@...linux.org.uk>, Kory Maincent
 <kory.maincent@...tlin.com>, Maxime Chevallier
 <maxime.chevallier@...tlin.com>, Nishanth Menon <nm@...com>,
 kernel@...gutronix.de, linux-kernel@...r.kernel.org,
 netdev@...r.kernel.org, UNGLinuxDriver@...rochip.com,
 linux-doc@...r.kernel.org, Michal Kubecek <mkubecek@...e.cz>, Roan van Dijk
 <roan@...tonic.nl>
Subject: Re: [PATCH net-next v5 2/5] ethtool: netlink: add
 ETHTOOL_MSG_MSE_GET and wire up PHY MSE access

On Mon,  8 Sep 2025 14:46:07 +0200 Oleksij Rempel wrote:
> diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
> index 969477f50d84..d69dd3fb534b 100644
> --- a/Documentation/netlink/specs/ethtool.yaml
> +++ b/Documentation/netlink/specs/ethtool.yaml
> @@ -1899,6 +1899,79 @@ attribute-sets:
>          type: uint
>          enum: pse-event
>          doc: List of events reported by the PSE controller
> +  -
> +    name: mse-config
> +    attr-cnt-name: --ethtool-a-mse-config-cnt
> +    attributes:
> +      -
> +        name: unspec
> +        type: unused
> +        value: 0

Are you actually using this somewhere?
It's good to not use attr ID 0 in case we encounter an uninitialized
attr, but there's no need to define a name for it, usually.
Just skip the entry 0 if you don't need then name.

> +      -
> +        name: max-average-mse
> +        type: u32
> +      -
> +        name: max-peak-mse
> +        type: u32
> +      -
> +        name: refresh-rate-ps
> +        type: u64
> +      -
> +        name: num-symbols
> +        type: u64

type: uint for all these?

> +      -
> +        name: supported-caps
> +        type: nest
> +        nested-attributes: bitset
> +      -
> +        name: pad
> +        type: pad

you shouldn't need it if you use uint

> +  -
> +    name: mse-snapshot
> +    attr-cnt-name: --ethtool-a-mse-snapshot-cnt
> +    attributes:
> +      -
> +        name: unspec
> +        type: unused
> +        value: 0
> +      -
> +        name: channel
> +        type: u32
> +        enum: phy-mse-channel
> +      -
> +        name: average-mse
> +        type: u32
> +      -
> +        name: peak-mse
> +        type: u32
> +      -
> +        name: worst-peak-mse
> +        type: u32
> +  -
> +    name: mse
> +    attr-cnt-name: --ethtool-a-mse-cnt
> +    attributes:
> +      -
> +        name: unspec
> +        type: unused
> +        value: 0
> +      -
> +        name: header
> +        type: nest
> +        nested-attributes: header
> +      -
> +        name: channel
> +        type: u32

Please annotate attrs which carry enums and flags with

	enum: $name

> +        enum: phy-mse-channel
> +      -
> +        name: config
> +        type: nest
> +        nested-attributes: mse-config

config sounds like something we'd be able to change
Looks like this is more of a capability struct?

> +      -
> +        name: snapshot
> +        type: nest
> +        multi-attr: true
> +        nested-attributes: mse-snapshot

This multi-attr feels un-netlinky to me.
You define an enum for IDs which are then carried inside
snapshot.channel. In netlink IDs should be used as attribute types.
Why not add an entry here for all snapshot types?