Re: [PATCH net-next v3 1/4] net: phy: microchip_t1s: fix collision detection on PLCA status change

[netdev-bot+sashiko@kernel.org]

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

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

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] The failure the commit message sets out to fix — an autonomous
  (BEACON-driven) PLCA offline transition leaving CDEN cleared so…
- [Medium] lan86xx_plca_set_cfg() silently changes the interpretation of
  plca_cfg->enabled from a plain truthiness test (`if…
- [Medium] The LAN867X Rev.B1 driver entry gains .config_intr =
  lan86xx_config_intr and .handle_interrupt = lan86xx_handle_interrupt…
- [Medium] In lan86xx_config_intr() the CDEN re-sync is performed BEFORE
  unmasking PSTCM, contradicting the commit message, which states that…
- [Medium] lan86xx_config_intr() performs a multi-register read-modify
  sequence (read-to-clear STS1 -> read live PLCA status -> write CDEN in…

--

net: phy: microchip_t1s: fix collision detection on PLCA status change

This adds .config_intr and .handle_interrupt callbacks to the microchip_t1s
driver so that the PLCA Status Changed (PSTC) interrupt toggles CDEN in
COL_DET_CTRL0, instead of only adjusting collision detection on explicit
ethtool PLCA writes.  The callbacks are wired to LAN867X Rev.B1, C1, C2 and
LAN865X Rev.B0/B1.

> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
> index 73c23d311d72..3333e4801aae 100644
> --- a/drivers/net/phy/microchip_t1s.c
> +++ b/drivers/net/phy/microchip_t1s.c

[ ... ]

> @@ -458,14 +466,29 @@ static int lan86xx_plca_set_cfg(struct phy_device *phydev,
>  	if (ret)
>  		return ret;
>  
> -	if (plca_cfg->enabled)
> +	/* phylib dispatches handle_interrupt() only for PHYs with a real IRQ
> +	 * number (phy_interrupt_is_valid()). For PHY_POLL and PHY_MAC_INTERRUPT
> +	 * handle_interrupt() is never called, so apply the static CDEN write
> +	 * here as a baseline on every ethtool PLCA reconfiguration. The
> +	 * limitation is that autonomous PLCA mode transitions between ethtool
> +	 * reconfigurations are not tracked on such boards.
> +	 */
> +	if (phy_interrupt_is_valid(phydev))
> +		return 0;

[Severity: High]
Does this early return leave the failure described in the changelog reachable
on boards where the PHY interrupt is not routed?

The commit message says the problem is that "When PLCA went offline,
collision detection stayed disabled, leaving CSMA/CD running unprotected".
For phydev->irq == PHY_POLL or PHY_MAC_INTERRUPT, phy_interrupt_is_valid()
is false:

include/linux/phy.h:phy_interrupt_is_valid() {
	return phydev->irq != PHY_POLL && phydev->irq != PHY_MAC_INTERRUPT;
}

so phylib never dispatches lan86xx_handle_interrupt(), and the only remaining
CDEN write in lan86xx_plca_set_cfg() is derived from the administratively
requested plca_cfg->enabled, never from live PST.  Nothing in the polled path
re-reads PLCA status either — lan86xx_read_status() runs every poll interval
and contains no CDEN logic:

	phydev->link = 1;
	phydev->duplex = DUPLEX_HALF;

That covers all LAN867X Rev.B1/C1/C2 boards whose device tree does not
describe a PHY interrupt, and at this commit every LAN865X as well.  Would
re-syncing CDEN from genphy_c45_plca_get_status() in lan86xx_read_status()
cover the polled case too?

> +
> +	if (plca_cfg->enabled > 0)
>  		return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
>  				      LAN86XX_REG_COL_DET_CTRL0,
>  				      COL_DET_CTRL0_ENABLE_BIT_MASK,
>  				      COL_DET_DISABLE);
>  
> -	return phy_modify_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_COL_DET_CTRL0,
> -			      COL_DET_CTRL0_ENABLE_BIT_MASK, COL_DET_ENABLE);
> +	if (plca_cfg->enabled == 0)
> +		return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> +				      LAN86XX_REG_COL_DET_CTRL0,
> +				      COL_DET_CTRL0_ENABLE_BIT_MASK,
> +				      COL_DET_ENABLE);
> +
> +	return 0;
>  }

[Severity: Medium]
Is this change from "if (plca_cfg->enabled)" to a three-way test a second,
independent fix that the changelog does not mention?

ethnl_set_plca() prefills the request and only overwrites the attributes
actually present:

net/ethtool/plca.c:ethnl_set_plca() {
	memset(&plca_cfg, 0xff, sizeof(plca_cfg));
	plca_update_sint(&plca_cfg.enabled, tb, ETHTOOL_A_PLCA_ENABLED, &mod);
	...
}

so a partial write such as "ethtool --set-plca-cfg <dev> node-id 3" arrives
with enabled == -1, which genphy_c45_plca_set_cfg() documents as "not to be
changed":

drivers/net/phy/phy-c45.c
 * @plca_cfg: structure containing the PLCA configuration. Fields set to -1 are
 * not to be changed.

The old truthiness test treated -1 as enabled and wrote COL_DET_DISABLE even
though PLCA enablement was untouched.  The new "> 0" / "== 0" split honours
the -1 contract, which looks like a user-visible fix in its own right.

Since the patch carries a Fixes: tag and is a stable candidate, could the
changelog call this out separately from the PSTC interrupt work, so it is
clear which part helps boards without a routed PHY IRQ?

> @@ -506,6 +529,111 @@ static int lan86xx_read_status(struct phy_device *phydev)
>  	return 0;
>  }
>  

[ ... ]

> +static int lan86xx_config_intr(struct phy_device *phydev)
> +{
> +	struct phy_plca_status plca_st;
> +	int ret;
> +
> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> +		/* Read to clear any pending status before enabling. */
> +		ret = lan86xx_read_clear_sts1(phydev);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* STS1 may have cleared a PSTC event that occurred while the
> +		 * interrupt was masked, so synchronize CDEN with the current
> +		 * PLCA state before enabling PSTC.
> +		 */
> +		ret = genphy_c45_plca_get_status(phydev, &plca_st);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> +				     LAN86XX_REG_COL_DET_CTRL0,
> +				     COL_DET_CTRL0_ENABLE_BIT_MASK,
> +				     plca_st.pst ? COL_DET_DISABLE :
> +				     COL_DET_ENABLE);
> +		if (ret)
> +			return ret;
> +
> +		return lan86xx_set_intr_mask(phydev,
> +					     LAN86XX_STS1_PLCA_STS_CHANGED,
> +					     true);
> +	}

[Severity: Medium]
Does the implemented ordering here match the commit message?

The changelog states:

    CDEN is re-synced against current PLCA status right after unmasking
    PSTCM, since a transition during the masked window is otherwise
    silently dropped by the STS1 read-to-clear.

The code does the opposite order in lan86xx_config_intr():

	lan86xx_read_clear_sts1()		/* drops pending PSTC */
	genphy_c45_plca_get_status()		/* sample */
	phy_modify_mmd(COL_DET_CTRL0, ...)	/* act on the sample */
	lan86xx_set_intr_mask(..., true)	/* only now unmask PSTCM */

Under the hardware model the commit message describes, a PLCA transition
occurring between the status sample and the unmask is neither reflected in the
CDEN value just written nor delivered as an interrupt, so CDEN would stay
stale until the next autonomous PLCA transition.  Should the re-sync be moved
after lan86xx_set_intr_mask(), or should the changelog be corrected to match
the code?

[Severity: Medium]
Can lan86xx_config_intr() race with lan86xx_handle_interrupt() over
COL_DET_CTRL0 and the read-to-clear STS1 latch?

lan86xx_handle_interrupt() runs with phydev->lock held:

drivers/net/phy/phy.c:phy_interrupt() {
	mutex_lock(&phydev->lock);
	ret = phydev->drv->handle_interrupt(phydev);
	mutex_unlock(&phydev->lock);
}

but lan86xx_config_intr() is reached via phy_config_interrupt() from
phy_request_interrupt(), phy_free_interrupt() and phy_init_hw(), none of which
hold that mutex.  phy_request_interrupt() also registers the handler before
enabling:

drivers/net/phy/phy.c:phy_request_interrupt() {
	err = request_threaded_irq(phydev->irq, NULL, phy_interrupt,
				   IRQF_ONESHOT | IRQF_SHARED,
				   phydev_name(phydev), phydev);
	...
		if (phy_enable_interrupts(phydev)) {
}

and phy_free_interrupt() calls phy_disable_interrupts() while PSTCM is still
unmasked and the handler is still registered.

So an interleaving like this looks possible:

CPU0 lan86xx_config_intr()		CPU1 lan86xx_handle_interrupt()
  lan86xx_read_clear_sts1()
  plca_get_status() -> pst = 0
					  lan86xx_read_clear_sts1() -> PSTC
					  plca_get_status() -> pst = 1
					  phy_modify_mmd(CDEN = DISABLE)
  phy_modify_mmd(CDEN = ENABLE)
  lan86xx_set_intr_mask(..., true)

phy_modify_mmd() only serializes the individual register read-modify-write on
the MDIO bus, so the status-read-then-CDEN-write pair is not atomic.  Since
the PSTC latch was already consumed, would CDEN stay wrong until the next
autonomous PLCA transition?

> +
> +	ret = lan86xx_set_intr_mask(phydev, LAN86XX_STS1_PLCA_STS_CHANGED,
> +				    false);
> +	if (ret)
> +		return ret;
> +
> +	/* Read to clear any pending status after disabling. */
> +	ret = lan86xx_read_clear_sts1(phydev);
> +	return ret < 0 ? ret : 0;
> +}
> +

[ ... ]

>  static struct phy_driver microchip_t1s_driver[] = {
>  	{
>  		PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1),
> @@ -513,6 +641,8 @@ static struct phy_driver microchip_t1s_driver[] = {
>  		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
>  		.config_init        = lan867x_revb1_config_init,
>  		.read_status        = lan86xx_read_status,
> +		.config_intr        = lan86xx_config_intr,
> +		.handle_interrupt   = lan86xx_handle_interrupt,
>  		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
>  		.set_plca_cfg	    = genphy_c45_plca_set_cfg,

[Severity: Medium]
Is the Rev.B1 entry intentional?  It gains the two CDEN-managing callbacks
but keeps .set_plca_cfg = genphy_c45_plca_set_cfg, while the Rev.C1/C2 and
LAN865X entries use lan86xx_plca_set_cfg.

Two consequences follow from that combination.

First, the new comment in lan86xx_plca_set_cfg() about applying "the static
CDEN write here as a baseline" for PHY_POLL and PHY_MAC_INTERRUPT boards is
unreachable for Rev.B1, since genphy_c45_plca_set_cfg() never touches
COL_DET_CTRL0.  A Rev.B1 board on PHY_POLL therefore gets no CDEN management
at all, while a Rev.B1 board with a routed IRQ gets PLCA-tracked CDEN.

Second, Rev.B1's mandatory AN1699 init table programs COL_DET_CTRL0 with
CDEN set:

	0xC000, 0x801C, 0x033F, 0x0404,   /* values, 0x0087 = 0x801C */
	0xF800, 0x801C, 0x1FFF, 0xFFFF,   /* masks,  mask 0x801C */

lan86xx_handle_interrupt() has no per-revision guard, so it clears that bit
whenever PLCA is operational.  Commit 78341049fbcd converted only Rev.C1/C2
and LAN865X to lan86xx_plca_set_cfg and deliberately left Rev.B1 out.  Should
Rev.B1 either also use lan86xx_plca_set_cfg, or not be wired to the new
callbacks?

[ ... ]

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