Re: [PATCH net-next v1 4/9] net: ethtool: Add attributes for cable test reports

[Andrew Lunn <andrew@lunn.ch>]

> > +
> > + +-------------------------------------------+--------+-----------------------+
> > + | ``ETHTOOL_A_CABLE_TEST_HEADER``           | nested | reply header          |
> > + +-------------------------------------------+--------+-----------------------+
> > + | ``ETHTOOL_A_CABLE_TEST_NTF_RESULT``       | nested | cable test result     |
> > + +-+-----------------------------------------+--------+-----------------------+
> > + | | ``ETHTOOL_A_CABLE_RESULTS_PAIR``        | u8     | pair number           |
> > + +-+-----------------------------------------+--------+-----------------------+
> > + | | ``ETHTOOL_A_CABLE_RESULTS_CODE``        | u8     | result code           |
> > + +-+-----------------------------------------+--------+-----------------------+
> > + | ``ETHTOOL_A_CABLE_TEST_NTF_RESULT``       | nested | cable test results    |
> > + +-+-----------------------------------------+--------+-----------------------+
> > + | | ``ETHTOOL_A_CABLE_RESULTS_PAIR``        | u8     | pair number           |
> > + +-+-----------------------------------------+--------+-----------------------+
> > + | | ``ETHTOOL_A_CABLE_RESULTS_CODE``        | u8     | result code           |
> > + +-+-----------------------------------------+--------+-----------------------+
> > + | ``ETHTOOL_A_CABLE_TEST_NTF_FAULT_LENGTH`` | nested | cable length          |
> > + +-+-----------------------------------------+--------+-----------------------+
> > + | | ``ETHTOOL_A_CABLE_FAULT_LENGTH_PAIR``   | u8     | pair number           |
> > + +-+-----------------------------------------+--------+-----------------------+
> > + | | ``ETHTOOL_A_CABLE_FAULT_LENGTH_CM``     | u8     | length in cm          |
> > + +-+-----------------------------------------+--------+-----------------------+
> 
> Would it be complicated to gather all information for each pair
> together? I.e. to have one nest for each pair with pair number, result
> code and possibly other information (if available). I believe it would
> make the message easier to process.

It is something i considered, but decided against. Attributes give us
flexibility to report whatever the PHY gives us. There is no
standardisation here. Some PHYs will report the first fault on a
pair. Others will report multiple faults on a pair. Some PHYs can tell
you pair X is shorted to pair Y, etc, while some PHYs just tell you it
is shorted. It also keeps the driver code simple. Report whatever you
have in any order and it does not matter. And it means i don't need
complex helper code trying to coordinating information from the
driver.

So far, a plain dump of the netlink message in user space also seems
readable. But when we have more PHYs supported and more variability
between PHYs we might need to consider if user space should do some
sorting before printing the test results.

> > +enum {
> > +	ETHTOOL_A_CABLE_PAIR_0,
> > +	ETHTOOL_A_CABLE_PAIR_1,
> > +	ETHTOOL_A_CABLE_PAIR_2,
> > +	ETHTOOL_A_CABLE_PAIR_3,
> > +};
> 
> Do we really need this enum, couldn't we simply use a number (possibly
> with a sanity check of maximum value)?

They are not strictly required. But it helps with consistence. Are the
pairs numbered 0, 1, 2, 3, or 1, 2, 3, 4?

> > +enum {
> > +	ETHTOOL_A_CABLE_RESULT_UNSPEC,
> > +	ETHTOOL_A_CABLE_RESULT_PAIR,		/* u8 ETHTOOL_A_CABLE_PAIR_ */
> > +	ETHTOOL_A_CABLE_RESULT_CODE,		/* u8 ETHTOOL_A_CABLE_RESULT_CODE_ */
> > +
> > +	__ETHTOOL_A_CABLE_RESULT_CNT,
> > +	ETHTOOL_A_CABLE_RESULT_MAX = (__ETHTOOL_A_CABLE_RESULT_CNT - 1)
> > +};
> > +
> > +enum {
> > +	ETHTOOL_A_CABLE_FAULT_LENGTH_UNSPEC,
> > +	ETHTOOL_A_CABLE_FAULT_LENGTH_PAIR,	/* u8 ETHTOOL_A_CABLE_PAIR_ */
> > +	ETHTOOL_A_CABLE_FAULT_LENGTH_CM,	/* u16 */
> 
> The example above says "u8" (which is obviously wrong).

Yep, will fix that.

> I would rather suggest u32 here to be as future proof as possible.

Yes, i've been going backwards and forwards on that. I did not realize
netlink messages were space inefficient. A u8 takes as much space as a
u32. I picked u16 because that allows up 65535cm, or 655.35m. All the
IEEE 802.3 Base-T standards have a maximum cable length of 100m, or
shorter. They might work with longer cables, but i doubt a cable 6
times longer than the specified max will work. So a u16 is ample.

The only argument i can see for a u32 is if somebody can implement
cable testing for fibre optical cables. Then a u16 is not big enough.
But so far, i've never seen an SFP module offering this.

> One more idea: it would be IMHO useful to also send a notification when
> the test is started. It could be distinguished by a status attribute
> which would describe status of the test as a whole (not a specific
> pair), e.g. started, finished, aborted.
 
Yes, give how long these tests take, i could be useful. There is also
already some hints about this, in that the last patch in the series
changes the RFC 2863 status of the interface, which i hope sends a
message to user space about the interface change of status.

	Andrew