Re: [PATCH net-next v4 4/4] net: phy: microchip_t1s: fix collision detection for LAN867X Rev.D0

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 (1) · 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).

- [Medium] On LAN867X Rev.D0 the driver never actually enables CDEN
  (collision detection, COL_DET_CTRL0 bit 15), although the design…
- [Medium] lan867x_revd0_config_intr() and
  lan867x_revd0_handle_interrupt() choose the Rev.D0 link-status source…
- [Low] In steady state, the PLCA_STS_CHANGED branch of
  lan867x_revd0_handle_interrupt() changes nothing.
- [Low] lan867x_revd0_config_intr() reads priv->plca_enabled and does a
  read-then-write of PRSCTL1 and LINK_STATUS_CTRL without taking…

Pre-existing issues:
- [Medium] In polling mode (no PHY IRQ), the Rev.D0 link-status selection
  is not resynced after phy_init_hw().

--

net: phy: microchip_t1s: fix collision detection for LAN867X Rev.D0

This commit configures CCMFC in COL_DET_CTRL0 on LAN867X Rev.D0 so that
collision forwarding is gated by PLCA_Status in hardware. It also stops
software CDEN toggling on Rev.D0. It adds Rev.D0 specific config_intr and
handle_interrupt callbacks that re-evaluate the link status selection
(forcing the semaphore source when CSMA/CD fallback is enabled), and sets
PHY_POLL_CABLE_TEST for Rev.D0.

> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
> index 5ce0304bf0952..d667f57aa4a8f 100644
> --- a/drivers/net/phy/microchip_t1s.c
> +++ b/drivers/net/phy/microchip_t1s.c

[ ... ]

> @@ -542,7 +574,7 @@ static int lan86xx_plca_set_cfg(struct phy_device *phydev,
>  	 * (not -1, which means "don't change").
>  	 */
>  	if (phydev->phy_id == PHY_ID_LAN867X_REVD0 && plca_cfg->enabled != -1) {
> -		ret = lan867x_revd0_link_active_selection(phydev,
> +		ret = lan867x_revd0_update_link_selection(phydev,
>  							  plca_cfg->enabled);
>  		if (ret)
>  			return ret;
> @@ -555,6 +587,12 @@ static int lan86xx_plca_set_cfg(struct phy_device *phydev,
>  	if (plca_cfg->enabled != -1)
>  		priv->plca_enabled = plca_cfg->enabled;
>  

[Severity: Low]
Can the Rev.D0 link selection end up based on the old enable state here?
lan86xx_plca_set_cfg() writes the selection from plca_cfg->enabled above.
It updates priv->plca_enabled only here and does not recompute afterwards.

lan867x_revd0_config_intr() reads priv->plca_enabled and then does the
PRSCTL1 read and the LINK_STATUS_CTRL write without taking phydev->lock
or priv->cden_lock. The sibling lan86xx_config_intr() does take cden_lock
for its resync. The commit that added it notes that config_intr() is not
guaranteed to run under phydev->lock.

If lan867x_revd0_config_intr() runs between those two steps, it would
write a selection based on the old cached value. Nothing would correct
that until the next PSTC event. The unlocked read of priv->plca_enabled
is also a data race.

I couldn't find a concrete caller for this PHY that runs phy_init_hw()
at the same time as an ethtool PLCA set. Some MAC drivers do call
phy_init_hw() from their own work or reset paths, though.

> +	/* LAN867X Rev.D0 uses CCMFC for autonomous collision detection
> +	 * gating; CDEN remains enabled and does not require software toggling.
> +	 */
> +	if (phydev->phy_id == PHY_ID_LAN867X_REVD0)
> +		return 0;
> +

[ ... ]

> @@ -582,6 +620,20 @@ static int lan867x_revd0_config_init(struct phy_device *phydev)
>  			return ret;
>  	}
>  
> +	/* AN1699: Configure CCMFC (Collision Counting and MAC Forwarding
> +	 * Control) to OA default (0x1) so that the hardware autonomously gates
> +	 * collision forwarding to the MAC based on the live PLCA_Status:
> +	 * collisions are neither counted nor forwarded when PLCA_Status is OK,
> +	 * and are counted/forwarded when not OK. This eliminates the need for
> +	 * software-driven CDEN toggling. CDEN defaults to enabled on Rev.D0
> +	 * and remains enabled.
> +	 */
> +	ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_COL_DET_CTRL0,
> +			     COL_DET_CTRL0_CCMFC_MASK,
> +			     COL_DET_CTRL0_CCMFC_OA_DEFAULT);

[Severity: Medium]
This call only changes bits 10:9 of COL_DET_CTRL0, so bit 15 (CDEN)
keeps whatever value the PHY already has. Does anything on Rev.D0
actually set CDEN?

The comment added above lan86xx_plca_set_cfg() says:

  CDEN remains permanently enabled in config_init()

However, this phy_modify_mmd() does not write COL_DET_ENABLE, and
lan867x_revd0_fixup_regs[] has no entry for 0x0087. Rev.D0 also has no
.soft_reset, so phy_init_hw() does not reset the PHY back to its
defaults before config_init() runs.

Before this patch, lan86xx_plca_set_cfg() cleared CDEN on Rev.D0
whenever PLCA was enabled. It did this through lan86xx_update_cden(),
and through a direct COL_DET_DISABLE write since commit 78341049fbcd.
With the new early return in lan86xx_plca_set_cfg():

	if (phydev->phy_id == PHY_ID_LAN867X_REVD0)
		return 0;

no Rev.D0 path writes CDEN any more.

Suppose the PHY comes up with CDEN=0 and is not power cycled. Two
examples are a warm reboot or kexec from an older kernel after
"ethtool --set-plca-cfg ... enable on", and a bootloader that followed
the older AN1699 advice. Would CDEN then stay disabled for good?

CCMFC only controls whether detected collisions are counted and
forwarded. In that case, CSMA/CD operation (PLCA disabled, or the
autonomous fallback) would run with no collision detection.

Could COL_DET_CTRL0_ENABLE_BIT_MASK and COL_DET_ENABLE be added to this
phy_modify_mmd() call?

[ ... ]

> @@ -712,6 +764,76 @@ static irqreturn_t lan86xx_handle_interrupt(struct phy_device *phydev)
>  	return ret_irq;
>  }
>  
> +static int lan867x_revd0_config_intr(struct phy_device *phydev)
> +{
> +	u16 mask = LAN86XX_STS1_PLCA_STS_CHANGED |
> +		   LAN86XX_STS1_LINK_STS_CHANGED;
> +	struct lan86xx_priv *priv = phydev->priv;
> +	int sts1, ret;
> +
> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> +		/* Read to clear any pending status before enabling. */
> +		sts1 = lan86xx_read_clear_sts1(phydev);
> +		if (sts1 < 0)
> +			return sts1;
> +
> +		/* STS1 may have cleared a pending PSTC while masked, and a
> +		 * missed PSTC leaves no trace to key off, so unconditionally
> +		 * resync the link-status-selection source from the current
> +		 * PLCA enable state and fallback configuration.
> +		 */
> +		ret = lan867x_revd0_update_link_selection(phydev,
> +							  priv->plca_enabled);

[Severity: Medium]
Can priv->plca_enabled be stale at this point? Only
lan86xx_plca_set_cfg() writes it, and the PHY can be hardware reset
while the driver stays bound.

For example, phy_detach() asserts the reset GPIO with
phy_device_reset(phydev, 1). A MAC that connects the PHY in ndo_open and
disconnects it in ndo_stop therefore resets it on every ifdown/ifup.
Losing power across suspend has the same effect.

Suppose PLCA was enabled with ethtool, so priv->plca_enabled == 1. The
next attach then does:

phy_attach_direct()
  phy_init_hw()
    lan867x_revd0_config_init()
      lan867x_revd0_link_active_selection(phydev, false)  /* SEMAPHORE */
...
phy_request_interrupt()
  phy_enable_interrupts()
    phy_config_interrupt()
      lan867x_revd0_config_intr()
        lan867x_revd0_update_link_selection(phydev, 1)    /* PLCA_STATUS */

If PRSCTL1 fallback reads 0, this selects LINK_STATUS_CONFIG_PLCA_STATUS
while PLCA_EN is 0 in hardware. Would the BMSR link then stay down until
userspace runs ethtool --set-plca-cfg again? The PSTC branch in
lan867x_revd0_handle_interrupt() uses the same cached value, so it would
not recover this either.

Before this patch, Rev.D0 had no config_intr. The SEMAPHORE selection
from config_init() stayed in place and the link came up.

Would reading the PLCA_EN bit from MDIO_OATC14_PLCA_CTRL0 here, or
resyncing priv->plca_enabled from hardware in config_init(), avoid this?

> +		if (ret < 0)
> +			return ret;
> +
> +		return lan86xx_set_intr_mask(phydev, mask, true);
> +	}
> +
> +	ret = lan86xx_set_intr_mask(phydev, mask, false);

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch.
The resync above only runs in the PHY_INTERRUPT_ENABLED branch.

In polling mode, phy_init_hw() calls lan867x_revd0_config_init(), which
always forces the SEMAPHORE source. It then calls
lan867x_revd0_config_intr(), which takes this disable branch and never
calls lan867x_revd0_update_link_selection().

If the PHY was not reset (resume without power loss, or re-attach
without a reset GPIO), PLCA_EN and priv->plca_enabled both remain 1.
With fallback disabled, is the link then forced up instead of following
PLCA_Status until ethtool --set-plca-cfg runs again?

config_init() has forced SEMAPHORE on every re-init since commit
07f5765f26c3. This patch adds the resync only for interrupt mode.

[ ... ]

> +static irqreturn_t lan867x_revd0_handle_interrupt(struct phy_device *phydev)
> +{

[ ... ]

> +	if (sts1 & LAN86XX_STS1_PLCA_STS_CHANGED) {
> +		/* Re-evaluate the link-status selection when PLCA status
> +		 * changes. A resulting link-status change raises
> +		 * LINK_STS_CHANGED, which triggers the PHY state machine.
> +		 */
> +		ret = lan867x_revd0_update_link_selection(phydev,
> +							  priv->plca_enabled);

[Severity: Low]
Can this call ever change LINK_STATUS_CTRL in steady state?
lan867x_revd0_update_link_selection() only looks at PRSCTL1 bit 10
(through lan86xx_fallback_to_csmacd()) and the cached
priv->plca_enabled. It never reads PLCA_STATUS.

This driver only reads PRSCTL1 and never writes it. priv->plca_enabled
only changes in lan86xx_plca_set_cfg(), which has already written the
selection for the new value. So each BEACON gain or loss seems to cost a
PRSCTL1 read plus a rewrite of the same LINK_STATUS_CTRL value.

The comment says a resulting link-status change raises
LINK_STS_CHANGED. When the selection is PLCA_STATUS, though, the hardware
already changes the link and raises LINK_STS_CHANGED on its own.

The commit message also says:

  The PSTC interrupt handler for Rev.D0 therefore only needs to update
  the link status selection on each PLCA transition.

Does Rev.D0 need LAN86XX_STS1_PLCA_STS_CHANGED unmasked at all? If it
does, could the commit message and this comment be updated to say what
the branch actually does?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929125928.611784-1-parthiban.veerasooran%40microchip.com