Re: [PATCH net-next v23 3/3] net: phy: Add driver for Motorcomm Quad 2.5GbE phy

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 · Medium: 3 · Low: 2

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] In yt8824_config_utp_init_paged() (drivers/net/phy/motorcomm.c),
  U0 TX channels CH0-CH3 are put into reset and never released.
- [High] net: phy: motorcomm: missing phy_package_init_once in
  yt8824_config_init disrupts active ports
- [Medium] In yt8824_config_utp_init_paged(), the '100M template
  amplitude' write to YT8824_DAC_IMID_CH2_CH3_CFG_ORG_REG (0x46e) has a…
- [Medium] yt8824_read_status_paged() sets phydev->speed = SPEED_UNKNOWN
  and phydev->duplex = DUPLEX_UNKNOWN whenever the link is down (lines…
- [Medium] yt8824_sds_isolate_and_softreset_paged() (lines 1060-1078) and
  yt8824_extern_config_sds_init_paged() hold the bus-wide MDIO mutex…
- [Low] yt8824_read_status_paged() calls genphy_c45_read_lpa() only when
  autoneg is enabled and complete (line 4602).
- [Low] In yt8824_probe(), phy_package_probe_once() sets the package
  'probed' bit through test_and_set_bit() before…

--

net: phy: Add driver for Motorcomm Quad 2.5GbE phy

This adds support for the Motorcomm YT8824 quad-port 2.5G PHY to the
existing motorcomm driver. The driver uses the phy_package helpers to
reach the shared top extended register space. A package-wide mutex
serializes the UTP/SerDes space switching between the four ports.

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

[ ... ]

> @@ -626,11 +935,1125 @@ static int ytphy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)

[ ... ]

> +static int yt8824_sds_isolate_and_softreset_paged(struct phy_device *phydev)
> +{
> +	int old_page = YT8824_RSSR_UTP_SPACE;
> +	int val = 0;
> +	int ret = -1;
> +
> +	old_page = phy_select_page(phydev, YT8824_RSSR_SERDES_SPACE);
> +	if (old_page < 0)
> +		goto err_restore_page;
> +
> +	/* sds softreset and disable isolate */
> +	ret = __phy_modify(phydev, MII_BMCR, BMCR_RESET | BMCR_ISOLATE,
> +			   BMCR_RESET & ~BMCR_ISOLATE);
> +	if (ret < 0)
> +		goto err_restore_page;
> +
> +	/* poll while still holding the lock */
> +	ret = read_poll_timeout(__phy_read, val,
> +				(val < 0) || !(val & BMCR_RESET), 50000, 600000,
> +				true, phydev, MII_BMCR);

[Severity: Medium]
phy_select_page() takes the bus-wide mii_bus->mdio_lock and
phy_restore_page() releases it. That means this read_poll_timeout() sleeps
with the bus lock held. Because sleep_before_read is true, each call holds
it for at least 50ms and up to 600ms.

Is it ok to hold the global MDIO lock that long?

In 10G-QXGMII mode this path runs from soft_reset, suspend, resume and the
error restore paths, for each of the four ports. phy_init_hw() calls
.soft_reset and then .config_init, and yt8824_config_init() calls
yt8824_soft_reset() again. So a single port init goes through several of
these windows.

yt8824_extern_config_sds_init_paged() does the same thing. It holds the lock
across the whole SerDes init sequence and then runs another sleeping
BMCR_RESET poll at the end.

While the lock is held, the sibling ports' phy_state_machine() work, PHYs in
other packages and any switch on the same bus are all blocked. That can in
turn stall ethtool/RTNL users waiting on those phydev locks.

phy_read_poll_timeout() drops the bus lock between polls to avoid this. Here
that is harder, because the UTP/SerDes space select lives in the
package-common top-ext register.

> +	if (val < 0)
> +		ret = val;
> +
> +err_restore_page:
> +	return phy_restore_page(phydev, old_page, ret);
> +}

[ ... ]

> +static int yt8824_config_utp_init_paged(struct phy_device *phydev)
> +{

[ ... ]

> +	/* 100M template amplitude */
> +	ctrl = FIELD_PREP(YT8824_DAC_IMID_CH3_ORG, 0x45);
> +	ctrl |= FIELD_PREP(YT8824_DAC_IMID_CH2_ORG, 0x45);
> +	mask = YT8824_DAC_IMID_CH3_ORG | YT8824_DAC_IMID_CH3_ORG;
                                          ^^^^

[Severity: Medium]
Should the second term be YT8824_DAC_IMID_CH2_ORG?

ytphy_modify_ext() goes through __phy_modify(), which writes
(old & ~mask) | set. With CH2 left out of the mask, the CH2 field of
YT8824_DAC_IMID_CH2_CH3_CFG_ORG_REG becomes old | 0x45 instead of 0x45.

The three sibling writes to 0x46f, 0x470 and 0x471 include both fields in the
mask.

> +	ret = ytphy_modify_ext_with_lock(phydev,
> +					 YT8824_DAC_IMID_CH2_CH3_CFG_ORG_REG,
> +					 mask, ctrl);
> +	if (ret < 0)
> +		goto err_restore;

[ ... ]

> +	ctrl = FIELD_PREP(YT8824_U0_CSR_RESETB_TX_CH0, 0);
> +	ctrl |= FIELD_PREP(YT8824_U0_CSR_RESETB_TX_CH1, 0);
> +	ctrl |= FIELD_PREP(YT8824_U0_CSR_RESETB_TX_CH2, 0);
> +	ctrl |= FIELD_PREP(YT8824_U0_CSR_RESETB_TX_CH3, 0);
> +	mask  = YT8824_U0_CSR_RESETB_TX_CH0 | YT8824_U0_CSR_RESETB_TX_CH1 |
> +		YT8824_U0_CSR_RESETB_TX_CH2 | YT8824_U0_CSR_RESETB_TX_CH3;
> +	ret = ytphy_modify_ext_with_lock(phydev, YT8824_U0_CRS_RESET_REG,
> +					 mask, ctrl);
> +	if (ret < 0)
> +		goto err_restore;

[Severity: High]
Is the SerDes and PLL setup done here really per-port?

yt8824_config_init() runs the whole SerDes sequence in
yt8824_extern_config_sds_init_paged() and the UTP sequence in
yt8824_config_utp_init_paged() for every PHY in the package. It does
this each time phy_init_hw() is called for that PHY:

phy_attach_direct() / mdio_bus_phy_resume()
  phy_init_hw()
    yt8824_soft_reset()
    yt8824_config_init()
      yt8824_extern_config_sds_init_paged()
      yt8824_config_utp_init_paged()

In 10G-QXGMII mode all four ports are multiplexed onto one SerDes
lane, which is also why the init sets
YT8824_CSR_AFE_MANUAL_CTRL_10P3125G.

yt8824_extern_config_sds_init_paged() rewrites the SerDes PLL, LDO and
training registers. It restarts calibration through
YT8824_RESTART_CAL_CTRL_REG and ends with a BMCR_RESET on the SerDes.

yt8824_config_utp_init_paged() toggles YT8824_PLL_DAC_RST. It also
drives the TX RESETB bits in YT8824_U0_CRS_RESET_REG and
YT8824_U1_CRS_RESET_REG low before releasing them.

If any of these registers are package-wide rather than per-port, then
attaching or resuming one port resets the shared lane, PLL and TX
paths. That happens underneath sibling ports that already have link,
so their traffic drops until the lane retrains. Many MAC drivers
connect the PHY from ndo_open, so a plain ifup on one port is enough to
trigger this.

phy_package_init_once() exists for this case. Other multi-port PHY
drivers such as mscc and qca807x use it so that only the first port
runs the package-global setup.

Could the global part be split into a helper guarded by
phy_package_init_once(), leaving only the per-port writes in
yt8824_config_init()? Which of the registers touched here are actually
shared between the four ports?

If the shared SerDes and PLL must be reprogrammed after the package
loses power across suspend, how should that work with the once bit?

[ ... ]

> +	ctrl = FIELD_PREP(YT8824_U0_CSR_RESETB_TX_CH0, 1);
> +	ctrl |= FIELD_PREP(YT8824_U0_CSR_RESETB_TX_CH1, 1);
> +	ctrl |= FIELD_PREP(YT8824_U0_CSR_RESETB_TX_CH2, 1);
> +	ctrl |= FIELD_PREP(YT8824_U0_CSR_RESETB_TX_CH3, 1);
> +	mask = YT8824_U0_CSR_RESETB_TX_CH0 | YT8824_U0_CSR_RESETB_TX_CH1 |
> +	       YT8824_U0_CSR_RESETB_TX_CH2 | YT8824_U0_CSR_RESETB_TX_CH3;
> +
> +	ctrl = FIELD_PREP(YT8824_U1_CSR_RESETB_TX_CH0, 1);

[Severity: High]
Is a ytphy_modify_ext_with_lock() call on YT8824_U0_CRS_RESET_REG missing
here?

Earlier in yt8824_config_utp_init_paged(), the active-low U0 TX RESETB bits
are written to 0. The U0 release value and mask are built here, but ctrl and
mask are overwritten by the U1 values straight away. Only
YT8824_U1_CRS_RESET_REG is written afterwards.

This means U0 TX channels CH0-CH3 seem to stay in reset, while the function
still returns 0.

This path runs on every attach and resume, in both package modes:

phy_init_hw()
  yt8824_config_init()
    yt8824_config_utp_init_paged()

> +	ctrl |= FIELD_PREP(YT8824_U1_CSR_RESETB_TX_CH1, 1);
> +	ctrl |= FIELD_PREP(YT8824_U1_CSR_RESETB_TX_CH2, 1);
> +	ctrl |= FIELD_PREP(YT8824_U1_CSR_RESETB_TX_CH3, 1);
> +	mask = YT8824_U1_CSR_RESETB_TX_CH0 | YT8824_U1_CSR_RESETB_TX_CH1 |
> +	       YT8824_U1_CSR_RESETB_TX_CH2 | YT8824_U1_CSR_RESETB_TX_CH3;
> +	ret = ytphy_modify_ext_with_lock(phydev, YT8824_U1_CRS_RESET_REG,
> +					 mask, ctrl);

[ ... ]

> @@ -3104,6 +4527,449 @@ static int yt8821_resume(struct phy_device *phydev)

[ ... ]

> +static int yt8824_read_status_paged(struct phy_device *phydev)
> +{
> +	int ret;
> +	int val;
> +
> +	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = genphy_read_status(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
> +		ret = genphy_c45_read_lpa(phydev);
> +		if (ret < 0)
> +			return ret;
> +	}

[Severity: Low]
When autoneg is enabled but not complete (link down or renegotiation
pending), genphy_c45_read_lpa() is skipped. genphy_read_status() then calls
genphy_read_lpa(), which only clears the clause 22 partner bits:

drivers/net/phy/phy_device.c:genphy_read_lpa() {
    ...
	if (phydev->autoneg == AUTONEG_ENABLE) {
		if (!phydev->autoneg_complete) {
			mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising,
							0);
			mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
			return 0;
		}
    ...
}

Could a 2500baseT_Full bit from an earlier 2.5G negotiation stay in
phydev->lp_advertising and keep showing up in ethtool?

Other 2.5G drivers such as rtl822x_read_status() clear it explicitly with
mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising, 0). The existing
yt8821_read_status() has the same pattern.

> +
> +	if (!phydev->link) {
> +		phydev->speed = SPEED_UNKNOWN;
> +		phydev->duplex = DUPLEX_UNKNOWN;

[Severity: Medium]
Does this lose a forced speed/duplex when autoneg is disabled?

With AUTONEG_DISABLE, phylib keeps the user's forced setting only in
phydev->speed and phydev->duplex. genphy_read_status() has just restored
them from BMCR via genphy_read_status_fixed(). After one link-down poll they
are replaced with SPEED_UNKNOWN and DUPLEX_UNKNOWN.

On the next ifdown/ifup or resume:

phy_start() / mdio_bus_phy_resume()
  _phy_start_aneg()
    phy_sanitize_settings()
      phy_caps_lookup(SPEED_UNKNOWN, DUPLEX_UNKNOWN, supported, false)

No capability matches speed -1, so phy_caps_lookup() returns the slowest
supported mode. Then yt8824_config_aneg()->__genphy_config_aneg() takes the
speed < SPEED_1000 path, and genphy_setup_forced() programs 10Mb/s instead
of the forced 100/1000/2500.

genphy_read_status() and yt8821_read_status() leave the forced values alone
in this case.

> +		if (phydev->autoneg == AUTONEG_ENABLE)
> +			phy_resolve_aneg_pause(phydev);
> +		return 0;
> +	}

[ ... ]

> +static int yt8824_probe(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	struct yt8824_shared_priv *shared_priv;
> +	int ret;
> +
> +	ret = devm_of_phy_package_join(dev, phydev, sizeof(*shared_priv));
> +	if (ret)
> +		return ret;
> +
> +	/* Ensure other PHY probes wait for shared mutex initialization. */
> +	phy_package_lock(phydev);
> +	if (phy_package_probe_once(phydev)) {
> +		ret = yt8824_phy_package_probe_once(phydev);
> +		if (ret) {
> +			phy_package_unlock(phydev);
> +			return ret;
> +		}
> +	}
> +	phy_package_unlock(phydev);

[Severity: Low]
This may not be reachable in practice. phy_package_probe_once() sets the
package once bit with test_and_set_bit() before the DT parsing in
yt8824_phy_package_probe_once(), which can fail. The bit is not cleared
again on failure.

If another package member had already joined at that point, it would skip
the init and probe successfully with the default PHY_INTERFACE_MODE_INTERNAL.
It would not fail on the missing or unsupported motorcomm,package-mode.

However, phy_driver_register() sets PROBE_FORCE_SYNCHRONOUS. With
sequential probing, the failing PHY's devres release calls
phy_package_leave(), the refcount drops to zero and the shared struct is
freed, once bit included. The next joiner then starts fresh.

The !priv || !np check in yt8824_phy_package_probe_once() also looks
unreachable. of_phy_package_join() always sets shared->np, and priv is
allocated because priv_size is non-zero.

Would it be cleaner to validate the DT property before consuming the once
bit, or is the current ordering fine given synchronous probing?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930194513.663575-1-kyle.switch%40motor-comm.com