Re: [PATCH v2] net: phy: micrel: ksz9031/ksz9131: add cabletest support

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

On Thu, Apr 07, 2022 at 04:08:12AM +0200, Marek Vasut wrote:
...

> +static int ksz9x31_cable_test_result_trans(u16 status)
> +{
> +	switch (FIELD_GET(KSZ9x31_LMD_VCT_ST_MASK, status)) {
> +	case KSZ9x31_LMD_VCT_ST_NORMAL:
> +		return ETHTOOL_A_CABLE_RESULT_CODE_OK;
> +	case KSZ9x31_LMD_VCT_ST_OPEN:
> +		return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
> +	case KSZ9x31_LMD_VCT_ST_SHORT:
> +		return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;

This conversation looks twisted:
VCT_ST_OPEN -> CODE_SAME_SHORT
VCT_ST_SHORT -> CODE_OPEN

> +	case KSZ9x31_LMD_VCT_ST_FAIL:
> +		fallthrough;
> +	default:
> +		return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
> +	}
> +}
> +
...
> +static int ksz9x31_cable_test_get_status(struct phy_device *phydev,
> +					 bool *finished)
> +{
> +	struct kszphy_priv *priv = phydev->priv;
> +	unsigned long pair_mask = 0xf;
> +	int retries = 20;
> +	int pair, ret, rv;
> +
> +	*finished = false;
> +
> +	/* Try harder if link partner is active */
> +	while (pair_mask && retries--) {
> +		for_each_set_bit(pair, &pair_mask, 4) {
> +			ret = ksz9x31_cable_test_one_pair(phydev, pair);
> +			if (ret == -EAGAIN)
> +				continue;
> +			if (ret < 0)
> +				return ret;
> +			clear_bit(pair, &pair_mask);
> +		}
> +		/* If link partner is in autonegotiation mode it will send 2ms
> +		 * of FLPs with at least 6ms of silence.
> +		 * Add 2ms sleep to have better chances to hit this silence.
> +		 */
> +		if (pair_mask)
> +			usleep_range(2000, 3000);

Not a blocker, just some ideas:
In my experience, active link partner may affect test result in way,
that the PHY wont report it as failed test. Especially, if the cable is
on the edge of specification (for example too long cable).
At same time 6ms of silence is PHY specific implementation. For example
KSZ PHYs tend to send burst of FLPs and the switch to other MDI-X pair
(if auto MDI-X is enabled). Some RTL PHYs tend to send random firework of pulses
on different pairs.

May be we should not fight against autoneg and misuse it a bit? For
example:
0. force MDI configuration
1. limit autoneg to 10mbit
2. allow the link
3. switch to test and run on unused pair
4. force MDI-X configuration and goto 1.

Other options can be:
- implement autoneg next page tx/rx and pass linux-vendor specific
  information over it (some thing like, please stop autoneg for X amount
  of sec)
- advertise remote fault

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 |