Re: [PATCH net-next] net: phy: motorcomm: Add cable test support for internal YT861x PHYs

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] yt861x_cable_test_get_status() in drivers/net/phy/motorcomm.c
  uses one variable, `ret`, for two different things.
- [Medium] yt861x_cable_test_start() clears YT8521_ESC1R_SLEEP_SW (auto-
  sleep) in ext register 0x27 at lines 2568-2570.
- [Low] The MOTORCOMM_PHY Kconfig help in drivers/net/phy/Kconfig still
  says "Currently supports YT85xx Gigabit Ethernet PHYs." The patch adds…

--

net: phy: motorcomm: Add cable test support for internal YT861x PHYs

This patch adds a driver entry for the YT861x family PHYs (PHY ID
0x01e04281) built into YT921x switches. Before this, they bound to the
generic PHY driver. The new entry adds TDR cable test start and status
callbacks.

> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
> index 90a4f86f2758b..ef15fef304aa9 100644
> --- a/drivers/net/phy/motorcomm.c
> +++ b/drivers/net/phy/motorcomm.c

[ ... ]

> @@ -2538,6 +2554,95 @@ static int yt8521_get_features(struct phy_device *phydev)
>  	return ret;
>  }
>  
> +/**
> + * yt861x_cable_test_start() - start a cable diagnostic (TDR) test
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * Returns: 0 or negative errno code
> + */
> +static int yt861x_cable_test_start(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	/* auto sleep would abort the TDR test */
> +	ret = ytphy_modify_ext_with_lock(phydev,
> +					 YT8521_EXTREG_SLEEP_CONTROL1_REG,
> +					 YT8521_ESC1R_SLEEP_SW, 0);
> +	if (ret)
> +		return ret;
> +
> +	return ytphy_write_ext_with_lock(phydev, YT861X_TDR_CTRL_REG,
> +					 YT861X_TDR_CTRL_START);
> +}

[Severity: Medium]
Can auto-sleep stay disabled if the test does not finish normally?

yt861x_cable_test_start() clears YT8521_ESC1R_SLEEP_SW. The only code
that sets it again is the success path at the end of
yt861x_cable_test_get_status(). These exits skip that restore:

- the YT861X_TDR_CTRL_REG write above fails, and the error is returned
  without restoring the sleep bit
- the YT861X_TDR_STATUS_REG read in yt861x_cable_test_get_status() fails,
  so the state machine calls phy_abort_cable_test()
- phy_stop() aborts the test, for example on "ip link set down" while a
  test is running
- the restoring ytphy_modify_ext_with_lock() call itself fails

The abort path is:

phy_stop()
  phy_abort_cable_test()
    ethnl_cable_test_finished()
    phy_init_hw()
      genphy_soft_reset()

The new driver entry has no config_init, so phy_init_hw() never writes
YT8521_EXTREG_SLEEP_CONTROL1_REG again. The success path sets SLEEP_SW
itself before calling genphy_soft_reset(). That suggests a BMCR reset does
not restore this ext register.

The restore also always writes SLEEP_SW=1 instead of the value that was
set before the test.

Could the previous value be saved and restored on every exit path? Or
could the bit be set again from a config_init callback?

> +
> +/**
> + * yt861x_cable_test_get_status() - report cable diagnostic test results
> + * @phydev: a pointer to a &struct phy_device
> + * @finished: set to true when the test is complete
> + *
> + * Returns: 0 or negative errno code
> + */
> +static int yt861x_cable_test_get_status(struct phy_device *phydev,
> +					bool *finished)
> +{
> +	int ret;
> +
> +	*finished = false;
> +
> +	ret = ytphy_read_ext_with_lock(phydev, YT861X_TDR_STATUS_REG);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret & YT861X_TDR_STATUS_BUSY)
> +		return 0;
> +
> +	for (int pair = ETHTOOL_A_CABLE_PAIR_A; pair <= ETHTOOL_A_CABLE_PAIR_D;
> +	     pair++) {
> +		u8 code;
> +
> +		switch ((ret >> (2 * pair)) & 0x3) {

[ ... ]

> +		ethnl_cable_test_result(phydev, pair, code);
> +
> +		if (code != ETHTOOL_A_CABLE_RESULT_CODE_OK &&
> +		    code != ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC) {
> +			ret = ytphy_read_ext_with_lock(phydev,
> +						       YT861X_TDR_PAIR_LENGTH_REG(pair));

[Severity: High]
Does this overwrite the status word in ret while the switch above still
needs it for the remaining pairs?

If a pair is SHORT or OPEN, ret is replaced by that pair's fault length in
cm, or by a negative errno if the read fails. The next pairs are then
decoded from bits of that value, not from YT861X_TDR_STATUS_REG.

Take an unplugged cable, where the status is 0x00ff (all pairs open).
Pair A is reported OPEN. If each pair's length reads back as 300 (0x12c),
pair B decodes as OPEN, pair C as SHORT and pair D as OK.

If the length read returns -EIO, later pairs decode as SHORT or OPEN from
the errno bits. Length registers are then read for faults the hardware
never reported.

Should the status word be kept in a separate variable from the length
read?

> +			if (ret >= 0)
> +				ethnl_cable_test_fault_length(phydev, pair,
> +							      ret);
> +		}
> +	}

[ ... ]

> @@ -3194,7 +3308,7 @@ static struct phy_driver motorcomm_phy_drvs[] = {
>  
>  module_phy_driver(motorcomm_phy_drvs);
>  
> -MODULE_DESCRIPTION("Motorcomm 8511/8521/8531/8531S/8821 PHY driver");
> +MODULE_DESCRIPTION("Motorcomm 8511/8521/8531/8531S/861x/8821 PHY driver");

[Severity: Low]
This isn't a bug, but the MOTORCOMM_PHY help text in
drivers/net/phy/Kconfig still says:

	  Currently supports YT85xx Gigabit Ethernet PHYs.

The header comment and MODULE_DESCRIPTION now mention 861x. Should the
Kconfig help be updated to match? It was already missing YT8821 before
this patch.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261004184246.1260426-1-mmyangfl%40gmail.com