Re: [PATCH net-next v2 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: 2 · Low: 2

- [High] lan86xx_config_intr() (drivers/net/phy/microchip_t1s.c) read-
  clears the latched status register LAN86XX_REG_STS1 *before* unmasking…
- [Medium] lan86xx_plca_set_cfg() uses `if (phydev->irq != PHY_POLL)
  return 0;` as the predicate for 'the interrupt handler will keep CDEN…
- [Medium] In the PHY_POLL branch retained by this patch,
  lan86xx_plca_set_cfg() tests `if (plca_cfg->enabled)` on a tri-state…
- [Low] Reported concern (validated as RESOLVED later in this series,
  retained for the consolidation stage): 'For the LAN865X Rev.B0/B1…
- [Low] Reported concern (validated as RESOLVED later in this series,
  retained for the consolidation stage): 'LAN867X Rev.D0 uses…

--

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

This adds lan86xx_config_intr() and lan86xx_handle_interrupt() to the
microchip_t1s driver so that the PLCA Status Changed (PSTC) interrupt drives
the collision detector enable bit in COL_DET_CTRL0.  The static CDEN write in
lan86xx_plca_set_cfg() is now only performed when phydev->irq is PHY_POLL.
The new handlers are wired to LAN867X Rev.B1/C1/C2 and the LAN865X Rev.B0/B1
internal PHY.

A few questions on the changelog and on the new code below.

> Wire these handlers to all supported PHY variants: LAN867X Rev.B1, C1,
> C2 and LAN865X Rev.B0/B1.

[Severity: Low]

LAN867X Rev.D0 also uses lan86xx_plca_set_cfg() as its .set_plca_cfg, but the
Rev.D0 entry does not get .config_intr/.handle_interrupt here.  Is "all
supported PHY variants" accurate for this patch?

Rev.D0 is covered by a later patch in the series ("net: phy: microchip_t1s:
fix collision detection for LAN867X Rev.D0"), which wires
lan867x_revd0_handle_interrupt() and programs CCMFC in
lan867x_revd0_config_init() so that collision forwarding is gated by
PLCA_Status in hardware.  Rev.D0 keeps its pre-patch behaviour at this commit,
so this is only about the changelog wording for the intermediate state.

> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
> index 73c23d311d72a..afb7e52594e71 100644
> --- a/drivers/net/phy/microchip_t1s.c
> +++ b/drivers/net/phy/microchip_t1s.c

[ ... ]

> @@ -458,6 +466,16 @@ static int lan86xx_plca_set_cfg(struct phy_device *phydev,
>  	if (ret)
>  		return ret;
>  
> +	/* PHYs with routed interrupts handle CDEN dynamically via the interrupt
> +	 * handler, so skip the static write. PHYs running with PHY_POLL have no
> +	 * interrupt handler, so apply the static CDEN write 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 (phydev->irq != PHY_POLL)
> +		return 0;
> +

[Severity: Medium]

Is phydev->irq != PHY_POLL the right predicate for "the interrupt handler
will keep CDEN up to date"?  phylib has a second non-interrupt sentinel:

include/linux/phy.h:
    static inline bool phy_interrupt_is_valid(struct phy_device *phydev)
    {
        return phydev->irq != PHY_POLL && phydev->irq != PHY_MAC_INTERRUPT;
    }

For phydev->irq == PHY_MAC_INTERRUPT phylib never calls
phy_request_interrupt(), so lan86xx_config_intr() is never called with
PHY_INTERRUPT_ENABLED and lan86xx_handle_interrupt() is never dispatched.
phy_mac_interrupt() only calls phy_trigger_machine().  phy_probe() also only
downgrades irq to PHY_POLL when phy_interrupt_is_valid() is true, so the
PHY_MAC_INTERRUPT value survives.

MAC drivers do set this sentinel on whatever PHY is attached, for example
tsnep_phy_open() in drivers/net/ethernet/engleder/tsnep_main.c does:

    adapter->phydev->irq = PHY_MAC_INTERRUPT;

On such a board, does anything write COL_DET_CTRL0 at all after this change,
given that the static write is skipped and the handler never runs?  Would
phy_interrupt_is_valid(phydev), or a test on phydev->interrupts, express the
intent more accurately here?

>  	if (plca_cfg->enabled)
>  		return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
>  				      LAN86XX_REG_COL_DET_CTRL0,

[Severity: Medium]

This truthiness test isn't introduced by this patch, but the patch keeps this
path as the documented baseline for PHY_POLL boards, so it seems worth
raising: plca_cfg->enabled is tri-state, not boolean.

ethnl_set_plca() in net/ethtool/plca.c does memset(&plca_cfg, 0xff,
sizeof(plca_cfg)), and plca_update_sint() returns early when the netlink
attribute is absent, so an omitted enable attribute reaches the driver as -1.
genphy_c45_plca_set_cfg() only clears PLCA_EN for enabled == 0 and only sets
it for enabled > 0, so -1 leaves the PLCA mode untouched.

For a request such as:

    ethtool --set-plca-cfg <dev> node-id 5

on a PHY_POLL board, enabled is -1, which is truthy, so COL_DET_DISABLE is
written even though PLCA was left off.  Does that not leave collision
detection disabled while the PHY keeps running CSMA/CD, with no interrupt
handler to correct it?  Should the test be plca_cfg->enabled > 0?

> @@ -506,6 +524,77 @@ static int lan86xx_read_status(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int lan86xx_config_intr(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> +		/* Read to clear any pending status before enabling. */
> +		ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_STS1);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* A mask bit of 0 enables the corresponding interrupt. */
> +		return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
> +					  LAN86XX_REG_IMSK1,
> +					  LAN86XX_STS1_PLCA_STS_CHANGED);
> +	}

[Severity: High]

Can a PLCA status change be lost across the masked window here?  The enable
path reads STS1, which clears the latched PSTC event, and only then unmasks
PSTCM.  Nothing in the patch programs COL_DET_CTRL0 from the current
PLCA_STATUS value, so CDEN on an interrupt-capable PHY is driven purely by
PSTC edges once lan86xx_plca_set_cfg() returns early.

Sequence for LAN867X Rev.C1/C2 with a routed IRQ, on any MAC that connects
the PHY at ndo_open:

ifup     phy_request_interrupt() -> lan86xx_config_intr(enable)

ethtool  lan86xx_plca_set_cfg() sets PLCA_EN and returns early without
         touching CDEN; beacon present, PST 0->1 -> PSTC ->
         lan86xx_handle_interrupt() writes CDEN=0

ifdown   phy_disconnect() -> phy_free_interrupt() ->
         phy_disable_interrupts() -> lan86xx_config_intr(disable)
         masks PSTCM and read-clears STS1; PLCA_EN stays set in hardware
         since the driver has no .suspend

         coordinator stops beaconing, PST 1->0 latches PSTC under the mask

ifup     phy_init_hw() -> lan867x_revc_config_init(), which does not write
         0x0087, so CDEN stays 0; then lan86xx_config_intr(enable) reads
         STS1 and discards the pending PSTC before unmasking

Does the bus then run CSMA/CD with the collision detector disabled until some
future beacon both appears and disappears again?  That is the state the
changelog describes as the bug being fixed, and since lan86xx_plca_set_cfg()
no longer writes CDEN on such boards, is there any way for an administrator
to repair it by re-running ethtool?

lan865x_revb_config_init() also never writes 0x0087, so the same applies to
the LAN865X internal PHY.

Would sampling genphy_c45_plca_get_status() and programming CDEN from it when
interrupts are enabled, and in set_plca_cfg regardless of irq mode, close the
window rather than relying on edges only?

> +
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IMSK1,
> +			       LAN86XX_STS1_PLCA_STS_CHANGED);
> +	if (ret)
> +		return ret;
> +
> +	/* Read to clear any pending status after disabling. */
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_STS1);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static irqreturn_t lan86xx_handle_interrupt(struct phy_device *phydev)
> +{
> +	struct phy_plca_status plca_st;
> +	irqreturn_t ret_irq = IRQ_NONE;
> +	int sts1, ret;
> +
> +	/* Reading the status register clears the latched event bits. */
> +	sts1 = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_STS1);
> +	if (sts1 < 0) {
> +		phy_error(phydev);
> +		return IRQ_NONE;
> +	}
> +
> +	if (sts1 & LAN86XX_STS1_PLCA_STS_CHANGED) {
> +		ret = genphy_c45_plca_get_status(phydev, &plca_st);
> +		if (ret < 0) {
> +			phy_error(phydev);
> +			return IRQ_NONE;
> +		}
> +
> +		/* AN1760/AN1699: disable collision detection in PLCA mode to
> +		 * improve signal quality; re-enable it in CSMA/CD mode.
> +		 *
> +		 * https://www.microchip.com/en-us/application-notes/an1760
> +		 * https://www.microchip.com/en-us/application-notes/an1699
> +		 */
> +		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 < 0) {
> +			phy_error(phydev);
> +			return IRQ_NONE;
> +		}
> +
> +		ret_irq = IRQ_HANDLED;
> +	}
> +
> +	return ret_irq;
> +}
> +

[ ... ]

> @@ -556,6 +651,8 @@ static struct phy_driver microchip_t1s_driver[] = {
>  		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
>  		.config_init        = lan865x_revb_config_init,
>  		.read_status        = lan86xx_read_status,
> +		.config_intr        = lan86xx_config_intr,
> +		.handle_interrupt   = lan86xx_handle_interrupt,

[Severity: Low]

At this commit oa_tc6 leaves phydev->irq as PHY_POLL for the LAN865X internal
PHY, so the changelog sentence "LAN865X is excluded from the static write
because its interrupt is always routed via the MAC-PHY SPI driver and the
interrupt handler always runs" does not hold yet.

The gap is closed inside the same series: "net: ethernet: oa_tc6: deliver the
PHY interrupt to phylib" adds an irq_domain, populates every mii_bus->irq[]
entry with the virtual IRQ before mdiobus_register(), and dispatches
handle_nested_irq(tc6->phy_virq) from the threaded IRQ when PHYINT is seen;
"microchip: lan865x: enable PHY interrupt via virtual IRQ" enables that path
for LAN8650/1.  Behaviour of LAN865X at this commit alone is unchanged,
because irq == PHY_POLL keeps the static CDEN write.

Could the changelog be reworded so that it does not assert a property that
only becomes true two patches later, in case this patch is backported on its
own?

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