| 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
| ||
|
Message-ID: <aMPw7kUddvGPJCzx@pengutronix.de> Date: Fri, 12 Sep 2025 12:07:42 +0200 From: Oleksij Rempel <o.rempel@...gutronix.de> To: Jakub Kicinski <kuba@...nel.org> 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 Hi Jakub, On Thu, Sep 11, 2025 at 07:34:40PM -0700, Jakub Kicinski wrote: > 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. No. I'll drop it. > > > + - > > + 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. We see two different design approaches in today's PHYs: - The "Quick Check" Approach (e.g., KSZ9477): This PHY uses a minimal sample size for a very fast check, capturing ~250 symbols over 2 microseconds. - The "Detailed Sample" Approach (e.g., KSZ9131): This PHY captures a much larger sample for a more statistically significant analysis, capturing 125,000 symbols over 1 millisecond. Now, let's see what happens when we apply these same design decisions to an 800 Gbit/s link. Applying the "Quick Check" (KSZ9477) Logic: If a future PHY designer wants to capture the same minimal amount of symbols (250), the required refresh interval on an 800G link would shrink to just 2.5 nanoseconds. Since future standards will be even faster, this demonstrates why picosecond-level granularity is necessary. In this specific minimal case, the values (250 symbols and 2,500 ps) would still fit within a u32. Applying the "Detailed Sample" Logic: If a designer follows the "detailed sample" approach or needs to run common diagnostics, the numbers become too large for a u32. - Scenario A (High-Granularity Sample): To get a dense sample over a 100 millisecond interval, the PHY would need to process ~10 billion symbols. This overflows the u32 for num-symbols. - Scenario B (Long-Term Monitoring): To run a standard 10 millisecond diagnostic, the interval measured in picoseconds is 10 billion ps. This overflows the u32 for refresh-rate-ps. > > + - > > + 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 Sorry, I can't follow here. What do you mean? > > > + 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. > > + - > > + 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. Best Regards, Oleksij -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Powered by blists - more mailing lists