Re: [PATCH net-next v5 1/5] ethtool: introduce core UAPI and driver API for PHY MSE diagnostics

[Oleksij Rempel <o.rempel@pengutronix.de>]

On Thu, Sep 11, 2025 at 07:23:18PM -0700, Jakub Kicinski wrote:
> On Mon,  8 Sep 2025 14:46:06 +0200 Oleksij Rempel wrote:
> > Add the base infrastructure for Mean Square Error (MSE) diagnostics,
> > as proposed by the OPEN Alliance "Advanced diagnostic features for
> > 100BASE-T1 automotive Ethernet PHYs" [1] specification.
> > 
> > The OPEN Alliance spec defines only average MSE and average peak MSE
> > over a fixed number of symbols. However, other PHYs, such as the
> > KSZ9131, additionally expose a worst-peak MSE value latched since the
> > last channel capture. This API accounts for such vendor extensions by
> > adding a distinct capability bit and snapshot field.
> > 
> > Channel-to-pair mapping is normally straightforward, but in some cases
> > (e.g. 100BASE-TX with MDI-X resolution unknown) the mapping is ambiguous.
> > If hardware does not expose MDI-X status, the exact pair cannot be
> > determined. To avoid returning misleading per-channel data in this case,
> > a LINK selector is defined for aggregate MSE measurements.
> > 
> > All investigated devices differ in MSE configuration parameters, such
> > as sample rate, number of analyzed symbols, and scaling factors.
> > For example, the KSZ9131 uses different scaling for MSE and pMSE.
> > To make this visible to userspace, scale limits and timing information
> > are returned via get_mse_config().
> 
> But the parameter set is set by the standard? If not we should annotate
> which one is and which isn't.

Do you mean we should show which parameters are defined by a standard
(for example Open-Alliance - MSE/pMSE) or which parts of the measurement
method - like how many samples in what time - are vendor or product
specific?

And should we only write this in comments/docs, or add a flag/enum so
user space can detect it?

> > +  -
> > +    name: phy-mse-capability
> > +    doc: |
> > +      Bitmask flags for MSE capabilities.
> > +
> > +      These flags are used in the 'supported_caps' field of struct
> > +      phy_mse_config to indicate which measurement capabilities are supported
> > +      by the PHY hardware.
> > +    type: flags
> > +    name-prefix: phy-mse-cap-
> > +    entries:
> > +      -
> > +        name: avg
> > +        doc: Average MSE value is supported.
> > +      -
> > +        name: peak
> > +        doc: Current peak MSE value is supported.
> > +      -
> > +        name: worst-peak
> > +        doc: Worst-case peak MSE (latched high-water mark) is supported.
> > +      -
> > +        name: channel-a
> > +        doc: Diagnostics for Channel A are supported.
> > +      -
> > +        name: channel-b
> > +        doc: Diagnostics for Channel B are supported.
> > +      -
> > +        name: channel-c
> > +        doc: Diagnostics for Channel C are supported.
> > +      -
> > +        name: channel-d
> > +        doc: Diagnostics for Channel D are supported.
> > +      -
> > +        name: worst-channel
> > +        doc: |
> > +          Hardware or drivers can identify the single worst-performing channel
> > +          without needing to query each one individually.
> > +      -
> > +        name: link
> > +        doc: |
> > +          Hardware provides only a link-wide aggregate MSE or cannot map
> > +          the measurement to a specific channel/pair. Typical for media where
> > +          the MDI/MDI-X resolution or pair mapping is unknown (e.g. 100BASE-TX).
> 
> Should we invert the order here? I think it's more likely we'd
> encounter new statistical measures rather than new channels.
> So channels should go first, and then the measures?

ack, sounds good.

> > +  -
> > +    name: phy-mse-channel
> > +    doc: |
> > +      Identifiers for the 'channel' parameter used to select which diagnostic
> > +      data to retrieve.
> > +    type: enum
> > +    name-prefix: phy-mse-channel-
> > +    entries:
> > +      -
> > +        name: a
> > +        value: 0
> 
> Don't enums default to starting from 0?  I think setting value is unnecessary

ack.

> > +        doc: Request data for channel A.
> 

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