Re: [PATCH net-next v3 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 5 potential
issue(s) to consider.

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

- [High] Rev.D0 collision detection can stay permanently disabled: after
  this patch no code path ever programs COL_DET_CTRL0 (0x0087) bit 15…
- [High] Wiring .config_intr/.handle_interrupt into the LAN867X Rev.D0
  driver entry (lines 836-837) regresses ethtool cable testing on any…
- [Medium] net: phy: redundant MDIO traffic in
  lan867x_revd0_handle_interrupt()
- [Low] lan867x_revd0_handle_interrupt() discards an already-established
  IRQ_HANDLED verdict.
- [Low] Concern as filed: lan867x_revd0_config_intr() (lines 709-747)
  performs a multi-step read-modify-write of PHY state (read-clear STS1…

Pre-existing issues:
- [Medium] In lan86xx_plca_set_cfg() the Rev.D0 link-status-selection
  register (LAN867X_REG_LINK_STATUS_CTRL, 0x0012) is programmed at lines…

--

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

This configures the CCMFC field in COL_DET_CTRL0 to the OA default so the
LAN867X Rev.D0 hardware gates collision forwarding from PLCA_Status, and
stops the software CDEN toggling for that revision.  It also adds
lan867x_revd0_config_intr()/lan867x_revd0_handle_interrupt() handling Link
Status Change and PLCA Status Change, forcing the semaphore link-status
source whenever autonomous fallback to CSMA/CD (PRSCTL1 bit 10) is enabled.

A few questions below.

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

[ ... ]

> @@ -40,6 +41,17 @@
>  #define COL_DET_CTRL0_ENABLE_BIT_MASK	BIT(15)
>  #define COL_DET_ENABLE			BIT(15)
>  #define COL_DET_DISABLE			0x0000
> +#define COL_DET_CTRL0_CCMFC_MASK	GENMASK(10, 9)
> +/* OA default: collisions gated by PLCA_Status in hardware */
> +#define COL_DET_CTRL0_CCMFC_OA_DEFAULT	BIT(9)
> +
> +/* PLCA Reconciliation Sublayer Control 1 Register (PRSCTL1). Bit 10
> + * controls whether the PHY autonomously falls back to CSMA/CD mode when
> + * no BEACON is observed while PLCA is enabled, versus staying pinned to
> + * PLCA mode regardless of BEACON presence.
> + */
> +#define LAN867X_REG_PRSCTL1               0x0035
> +#define PRSCTL1_PLCA_FALLB_TO_CSMACD_EN   BIT(10)

[ ... ]

> @@ -454,9 +505,12 @@ static int lan86xx_plca_set_cfg(struct phy_device *phydev,
>  {
>  	int ret;
>  
> -	/* Link status selection must be configured for LAN8670/1/2 Rev.D0 */
> -	if (phydev->phy_id == PHY_ID_LAN867X_REVD0) {
> -		ret = lan867x_revd0_link_active_selection(phydev,
> +	/* Link status selection must be configured for LAN8670/1/2 Rev.D0.
> +	 * Only update link status selection if enabled is explicitly specified
> +	 * (not -1, which means "don't change").
> +	 */
> +	if (phydev->phy_id == PHY_ID_LAN867X_REVD0 && plca_cfg->enabled != -1) {
> +		ret = lan867x_revd0_update_link_selection(phydev,
>  							  plca_cfg->enabled);
>  		if (ret)
>  			return ret;
>  	}

[Severity: Medium]
This isn't a bug introduced by this patch, but the ordering here looks
questionable in both the old and new code: LAN867X_REG_LINK_STATUS_CTRL is
programmed to the PLCA_STATUS source before genphy_c45_plca_set_cfg() sets
MDIO_OATC14_PLCA_EN.

genphy_c45_plca_set_cfg() performs several independent MMD writes
(PLCA_CTRL1, TOTMR, BURST) before it finally enables PLCA:

	// if we need to enable PLCA, do it at the end
	if (plca_cfg->enabled > 0) {
		ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2,
				       MDIO_OATC14_PLCA_CTRL0,
				       MDIO_OATC14_PLCA_EN);

If any of those writes fails, lan86xx_plca_set_cfg() returns the error to
phy_ethtool_set_plca_cfg() with no unwind, leaving 0x0012 selecting
PLCA_STATUS while PLCA is still disabled.

Does that leave the PHY reporting link-down until the next successful
ethtool PLCA write or a re-init?  Would it be better to enable PLCA first
and switch the link-status source afterwards, restoring the semaphore source
if the PLCA programming failed?

> @@ -472,8 +526,12 @@ static int lan86xx_plca_set_cfg(struct phy_device *phydev,
>  	 * 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.
> +	 *
> +	 * LAN867X Rev.D0 has autonomous collision detection via CCMFC, so skip
> +	 * the software CDEN toggling for that revision.
>  	 */
> -	if (phy_interrupt_is_valid(phydev))
> +	if (phy_interrupt_is_valid(phydev) ||
> +	    phydev->phy_id == PHY_ID_LAN867X_REVD0)
>  		return 0;
>  
>  	if (plca_cfg->enabled > 0)

> @@ -507,6 +565,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 is enabled by default and remains
> +	 * permanently enabled for Rev.D0.
> +	 */
> +	ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_COL_DET_CTRL0,
> +			     COL_DET_CTRL0_CCMFC_MASK,
> +			     COL_DET_CTRL0_CCMFC_OA_DEFAULT);
> +	if (ret)
> +		return ret;

[Severity: High]
Where does anything actually set CDEN (COL_DET_CTRL0 bit 15) for Rev.D0?

The comment says "CDEN is enabled by default and remains permanently enabled
for Rev.D0", but the read-modify-write above only covers bits 10:9:

	#define COL_DET_CTRL0_CCMFC_MASK	GENMASK(10, 9)

The Rev.D0 init table does not touch 0x0087 either:

	static const u16 lan867x_revd0_fixup_regs[8] = {
		0x0037, 0x008A, 0x0118, 0x00D6,
		0x0082, 0x00FD, 0x00FD, 0x0091,
	};

And the new early return in lan86xx_plca_set_cfg() removes the last
remaining writer of COL_DET_ENABLE/COL_DET_DISABLE for this revision:

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

So if bit 15 happens to be clear when lan867x_revd0_config_init() runs,
does the part end up running CSMA/CD with the collision detector off
permanently, with nothing left to re-arm it?  The CCMFC gating only decides
whether detected collisions are counted/forwarded.

Is the "enabled by default" assumption safe here?  This driver provides no
.soft_reset, and phy_init_hw() only resets when the driver supplies one:

	if (phydev->drv->soft_reset) {
		ret = phydev->drv->soft_reset(phydev);

so on a board without a PHY reset line config_init() runs against leftover
register state, for example after an unbind/rebind or a kexec from a
pre-patch kernel where lan86xx_plca_set_cfg() wrote COL_DET_DISABLE
whenever PLCA was enabled (pre-patch Rev.D0 was always PHY_POLL, so that
branch always ran).

The AN1699 Rev.B1 table in this same file explicitly sets bit 15 for the
same register:

	/* RMW 0x1F 0x0087 0x801C 0x801C */

Would including COL_DET_CTRL0_ENABLE_BIT_MASK/COL_DET_ENABLE in the same
read-modify-write be preferable to documenting an invariant that no code
establishes?

>  	/* Initially the PHY will be in CSMA/CD mode by default. So it is
>  	 * required to set the link always active as it doesn't support
>  	 * autoneg.

> @@ -634,6 +706,91 @@ 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 phy_plca_cfg plca_cfg;
> +	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;
> +
> +		if (sts1 & LAN86XX_STS1_LINK_STS_CHANGED)
> +			phy_trigger_machine(phydev);

[Severity: Low]
This isn't a bug, but is the ordering here intentional?  phy_trigger_machine()
runs before the link-status-selection source is resynced a few lines further
down by lan867x_revd0_update_link_selection(), so the state machine can read
the link state through the old selection source.

On the related concurrency question: lan867x_revd0_config_intr() does its
read-clear STS1 / genphy_c45_plca_get_cfg() / LINK_STATUS_CTRL write sequence
without phydev->lock, while lan867x_revd0_handle_interrupt() touches the same
registers under that lock via phy_interrupt().  That looks benign as written,
since both writers compute the 0x0012 value from the same two static inputs
(PLCA_CTRL0.PLCA_EN and PRSCTL1 bit 10) and the only mutator of those inputs
runs under rtnl and phydev->lock, so the two paths write identical values.
Would swapping the trigger to after the resync still be worth doing?

> +
> +		/* 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. CDEN is
> +		 * never touched here - see lan867x_revd0_config_init().
> +		 */
> +		ret = genphy_c45_plca_get_cfg(phydev, &plca_cfg);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = lan867x_revd0_update_link_selection(phydev,
> +							  plca_cfg.enabled);
> +		if (ret < 0)
> +			return ret;
> +
> +		return lan86xx_set_intr_mask(phydev, mask, true);
> +	}

[ ... ]

> +static irqreturn_t lan867x_revd0_handle_interrupt(struct phy_device *phydev)
> +{
> +	irqreturn_t ret_irq = IRQ_NONE;
> +	struct phy_plca_cfg plca_cfg;
> +	int sts1, ret;
> +
> +	sts1 = lan86xx_read_clear_sts1(phydev);
> +	if (sts1 < 0) {
> +		phy_error(phydev);
> +		return IRQ_NONE;
> +	}
> +
> +	if (sts1 & LAN86XX_STS1_LINK_STS_CHANGED) {
> +		phy_trigger_machine(phydev);
> +		ret_irq = IRQ_HANDLED;
> +	}
> +
> +	if (sts1 & LAN86XX_STS1_PLCA_STS_CHANGED) {
> +		ret = genphy_c45_plca_get_cfg(phydev, &plca_cfg);
> +		if (ret < 0) {
> +			phy_error(phydev);
> +			return IRQ_NONE;
> +		}

[Severity: Medium]
What does the PSTC arm here actually accomplish?  It re-derives the
link-status-selection source from two inputs that a PLCA_Status
transition cannot have changed: lan867x_revd0_update_link_selection()
computes the 0x0012 value from PLCA_CTRL0.PLCA_EN, fetched via
genphy_c45_plca_get_cfg(), and PRSCTL1 bit 10, fetched by
lan867x_revd0_fallback_to_csmacd().  Both are static configuration.
The only mutator of PLCA_EN is lan86xx_plca_set_cfg(), which already
programs LINK_STATUS_CTRL itself, and lan867x_revd0_config_intr()
resyncs it when interrupts are enabled.  So every PLCA flap ends in
lan867x_revd0_link_active_selection() writing 0x0012 with the value it
already holds.

The cost is five MMD reads in genphy_c45_plca_get_cfg() (PLCA_IDVER,
CTRL0, CTRL1, TOTMR, BURST), one more for PRSCTL1, and one write -
seven management-bus transactions per transition to produce no state
change.  On a multidrop segment where BEACONs come and go, PSTC can
flap frequently.

The error handling makes that worse than just wasted bandwidth: a
single transient MDIO failure in either call lands in phy_error(),
i.e. WARN_ON(1) plus PHY_ERROR, which parks the PHY until something
re-initialises it.  That is a hard failure taken on behalf of a
no-op register write.

The commit message says the Rev.D0 handler "only needs to update the
link status selection on each PLCA transition", but with CCMFC gating
collision forwarding from live PLCA_Status in hardware, and 0x0012
already pointing at PLCA_STATUS (or the semaphore when fallback is
enabled), does Rev.D0 need to subscribe to PSTC at all?  Could the
mask in lan867x_revd0_config_intr() drop
LAN86XX_STS1_PLCA_STS_CHANGED and keep only the link-status event?  If
there is a hardware reason the selection must be rewritten on every
transition, please spell it out in the comment - as written the code
reads as if the value were expected to differ.

[Severity: Low]
Should these error paths return ret_irq instead of IRQ_NONE?

When STS1 latches both LAN86XX_STS1_LINK_STS_CHANGED and
LAN86XX_STS1_PLCA_STS_CHANGED, ret_irq is already IRQ_HANDLED and
phy_trigger_machine() has run, and the latched bits were cleared by
lan86xx_read_clear_sts1().  A failing genphy_c45_plca_get_cfg() (or the
lan867x_revd0_update_link_selection() call below) then reports IRQ_NONE for
an interrupt the driver did consume.

phy_error() only sets PHY_ERROR and queues the state machine, and
phy_interrupt() forwards the driver verdict unchanged:

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

	return ret;
}

so genirq accounts the serviced interrupt as unhandled.

> +
> +		/* lan867x_revd0_update_link_selection() drives the selection
> +		 * source from the configured PLCA enable state, forcing
> +		 * semaphore mode instead whenever autonomous fallback to
> +		 * CSMA/CD is configured (see PRSCTL1 above).
> +		 */
> +		ret = lan867x_revd0_update_link_selection(phydev,
> +							  plca_cfg.enabled);
> +		if (ret < 0) {
> +			phy_error(phydev);
> +			return IRQ_NONE;
> +		}
> +
> +		ret_irq = IRQ_HANDLED;
> +	}
> +
> +	return ret_irq;
> +}
> +

> @@ -676,6 +833,8 @@ static struct phy_driver microchip_t1s_driver[] = {
>  		.name               = "LAN867X Rev.D0",
>  		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
>  		.config_init        = lan867x_revd0_config_init,
> +		.config_intr        = lan867x_revd0_config_intr,
> +		.handle_interrupt   = lan867x_revd0_handle_interrupt,
>  		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
>  		.set_plca_cfg	    = lan86xx_plca_set_cfg,
>  		.get_plca_status    = genphy_c45_plca_get_status,

[Severity: High]
Does wiring .config_intr/.handle_interrupt into this entry break ethtool
cable testing for Rev.D0 on boards with a real PHY interrupt?

phy_drv_supports_irq() returns config_intr && handle_interrupt, so phylib no
longer rewrites phydev->irq to PHY_POLL as it did before this patch.  The
same entry registers .cable_test_start/.cable_test_get_status but sets no
.flags, so PHY_POLL_CABLE_TEST is absent:

include/linux/phy.h:phy_polling_mode() {
	if (phydev->state == PHY_CABLETEST)
		if (phydev->drv->flags & PHY_POLL_CABLE_TEST)
			return true;

	if (phydev->drv->update_stats)
		return true;

	return phydev->irq == PHY_POLL;
}

With a valid IRQ and no PHY_POLL_CABLE_TEST this returns false during
PHY_CABLETEST, and phy_start_cable_test() only queues the state machine in
polling mode:

drivers/net/phy/phy.c:phy_start_cable_test() {
	...
	phydev->state = PHY_CABLETEST;

	if (phy_polling_mode(phydev))
		phy_trigger_machine(phydev);
	...
}

The PHY_CABLETEST arm of _phy_state_machine() is the only caller of
drv->cable_test_get_status():

	case PHY_CABLETEST:
		err = phydev->drv->cable_test_get_status(phydev, &finished);

and genphy_c45_oatc14_cable_test_get_status() detects completion purely by
polling:

	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MDIO_OATC14_HDD);
	...
	if (!(ret & OATC14_HDD_VALID))
		return 0;

Nothing in lan867x_revd0_handle_interrupt() signals diagnostic completion, so
does "ethtool --cable-test" now leave the interface with the carrier down and
netif_testing_on() set until phy_stop() aborts the test?  Would adding
.flags = PHY_POLL_CABLE_TEST to this entry, as at803x/qca808x/micrel/
microchip_t1 do when combining .config_intr with cable testing, address it?

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