Re: [PATCH net-next v5 2/5] ethtool: netlink: add ETHTOOL_MSG_MSE_GET and wire up PHY MSE access
From: Jakub Kicinski <kuba@kernel.org>
Date: 2025-09-12 02:34:42
Also in:
lkml, netdev
On Mon, 8 Sep 2025 14:46:07 +0200 Oleksij Rempel wrote:
quoted hunk ↗ jump to hunk
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?