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: <20250912170053.24348da3@kernel.org>
Date: Fri, 12 Sep 2025 17:00:53 -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 Fri, 12 Sep 2025 12:07:42 +0200 Oleksij Rempel wrote:
> > > +      -
> > > +        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?  
> 
> I would prefer to keep u64 for refresh-rate-ps and num-symbols.
> 
> My reasoning comes from comparing the design decisions of today's industrial
> hardware to the projected needs of upcoming standards like 800 Gbit/s. This
> analysis shows that future PHYs will require values that exceed the limits of a
> u32.

but u64 may or may not also have some alignment expectations, which uint
explicitly excludes

> > > +      -
> > > +        name: header
> > > +        type: nest
> > > +        nested-attributes: header
> > > +      -
> > > +        name: channel
> > > +        type: u32  
> > 
> > Please annotate attrs which carry enums and flags with
> > 
> > 	enum: $name  
> 
> Sorry, I can't follow here. What do you mean?

The values carried by this attr are from enum phy-mse-channel right?
So you should annotate the attribute, this way C will use an enum
type, and Python will decode the values into a human readable string.

> > > +        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?  
> 
> Yes? mse-config describes haw the measurements in the snapshot should be
> interpreted.

Right. 'capability' is not great either, but as I said 'config' sounds
like something that's tunable by the user. 

> > > +      -
> > > +        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?  
> 
> Can you please give me some examples here? I feel under-caffeinated, sorry.

Instead of this attr:

	-
		name: channel-a
		type: nest
		nested-attributes: mse-snapshot	
	        multi-attr: true
	-
		name: channel-b
		type: nest
		nested-attributes: mse-snapshot	
	        multi-attr: true
...
	-
		name: worst-channel
		type: nest
		nested-attributes: mse-snapshot	
	        multi-attr: true
...