Re: [RFC net-next 2/2] net: ethtool: add phy(dev) specific stats over netlink

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 29 Aug 2024 20:47:04 +0200 Andrew Lunn wrote:
> > +/* Additional PHY statistics, not defined by IEEE */
> > +struct ethtool_phy_stats {
> > +	/* Basic packet / byte counters are meant for PHY drivers */
> > +	u64 rx_packets;
> > +	u64 rx_bytes;
> > +	u64 rx_error; /* TODO: we need to define here whether packet
> > +		       * counted here is also counted as rx_packets,
> > +		       * and whether it's passed to the MAC with some
> > +		       * error indication or MAC never sees it.
> > +		       */
> > +	u64 tx_packets;
> > +	u64 tx_bytes;
> > +	u64 tx_error; /* TODO: same as for rx */
> > +};  
> 
> I'm not sure these are actually useful.
> 
> adin.c:
>         { "total_frames_checked_count",         0x940A, 0x940B }, /* hi + lo */
>         { "length_error_frames_count",          0x940C },
>         { "alignment_error_frames_count",       0x940D },
>         { "symbol_error_count",                 0x940E },

This one's IEEE, from patch 1.

>         { "oversized_frames_count",             0x940F },
>         { "undersized_frames_count",            0x9410 },

bunch of IEEE stats, but from the MAC space :S

>         { "odd_nibble_frames_count",            0x9411 },
>         { "odd_preamble_packet_count",          0x9412 },
>         { "dribble_bits_frames_count",          0x9413 },
>         { "false_carrier_events_count",         0x9414 },

These may be interesting?

> bcm-phy-lib.c:
>         { "phy_receive_errors", -1, MII_BRCM_CORE_BASE12, 0, 16 },

matching rx errors

>         { "phy_serdes_ber_errors", -1, MII_BRCM_CORE_BASE13, 8, 8 },

Dunno what BER errors is 🤔️

>         { "phy_false_carrier_sense_errors", -1, MII_BRCM_CORE_BASE13, 0, 8 },

false carrier like in adin.c

>         { "phy_local_rcvr_nok", -1, MII_BRCM_CORE_BASE14, 8, 8 },
>         { "phy_remote_rcv_nok", -1, MII_BRCM_CORE_BASE14, 0, 8 },

nok is not okay ? ... 🤷️

>         { "phy_lpi_count", MDIO_MMD_AN, BRCM_CL45VEN_EEE_LPI_CNT, 0, 16 },

Sounds standard :)

> icplus.c:
>         { "phy_crc_errors", 1 },
>         { "phy_symbol_errors", 11, },

Why the PHY wants to check CRC I can only guess, but the other one 
is in patch 1.

... I think going thru them right now is not super useful.

> 802.3 does not define in PHY statistics, the same as it does not
> define any MAC statistics. As a result it is a wild west, vendors
> doing whatever they want.

I think IEEE does define the MIB including some counters. It just does 
a shit job and defines very few.

> The exception is the Open Alliance, which have defined a number of
> different standards defining statistics which Automotive vendors can
> optionally follow.
> 
> https://opensig.org/automotive-ethernet-specifications/
> 
> These could be defined as either one or three groups, with the
> expectation more will be added later.

SG.

To be clear, I'm adding the pkt/error counters because Oleksij was
trying to add defines for these into linux/phy.h

https://lore.kernel.org/all/20240822115939.1387015-3-o.rempel@pengutronix.de/

You acked that which I read as an agreement that there's sufficient
commonality :)

I threw in the byte counters, perhaps unnecessarily. We can drop those
for sure.