Re: [PATCH net-next v5 2/5] ethtool: netlink: add ETHTOOL_MSG_MSE_GET and wire up PHY MSE access

[Jakub Kicinski <kuba@kernel.org>]

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
...