* [PATCH net-next v3 1/4] net: phy: microchip_t1s: fix collision detection on PLCA status change
2026-09-18 14:32 [PATCH net-next v3 0/4] net: microchip_t1s: fix collision detection on PLCA status change Parthiban Veerasooran
@ 2026-09-18 14:32 ` Parthiban Veerasooran
2026-09-22 15:08 ` netdev-bot+sashiko
2026-09-18 14:33 ` [PATCH net-next v3 2/4] net: ethernet: oa_tc6: deliver the PHY interrupt to phylib Parthiban Veerasooran
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Parthiban Veerasooran @ 2026-09-18 14:32 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, pabeni, hkallweit1, linux
Cc: netdev, linux-kernel, UNGLinuxDriver, Parthiban.Veerasooran,
Parthiban Veerasooran
lan86xx_plca_set_cfg() only adjusted collision detection on explicit
ethtool PLCA changes, missing the autonomous online/offline transitions
the PHY performs based on BEACON availability. When PLCA went offline,
collision detection stayed disabled, leaving CSMA/CD running unprotected.
Fix this by monitoring the PLCA Status Changed (PSTC) interrupt.
lan86xx_config_intr() enables/disables PSTCM in IMSK1;
lan86xx_handle_interrupt() reads live PLCA status on each interrupt and
toggles CDEN in COL_DET_CTRL0 accordingly. 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.
lan86xx_read_clear_sts1() and lan86xx_set_intr_mask() factor out the
shared STS1/IMSK1 sequences; both are reused by the Rev.D0 handling
added later in this series.
Wired to LAN867X Rev.B1, C1, C2 and LAN865X Rev.B0/B1. Rev.D0 needs
separate handling (follow-on patch). Boards without a routed interrupt
(phydev->irq == PHY_POLL, including LAN865X until later patches) keep
the static CDEN write in lan86xx_plca_set_cfg() as a baseline, with the
known limitation that autonomous transitions between ethtool calls
aren't tracked there.
Fixes: 78341049fbcd ("net: phy: microchip_t1s: configure collision detection based on PLCA mode")
Signed-off-by: Parthiban Veerasooran <parthiban.veerasooran@microchip.com>
---
drivers/net/phy/microchip_t1s.c | 142 +++++++++++++++++++++++++++++++-
1 file changed, 139 insertions(+), 3 deletions(-)
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
@@ -27,6 +27,14 @@
#define LAN865X_REG_CFGPARAM_CTRL 0x00DA
#define LAN865X_REG_STS2 0x0019
+/* PHY interrupt status and mask registers (MDIO_MMD_VEND2). The status bits
+ * are read-to-clear; a mask bit is enabled by writing 0.
+ */
+#define LAN86XX_REG_STS1 0x0018
+#define LAN86XX_REG_IMSK1 0x001C
+
+#define LAN86XX_STS1_PLCA_STS_CHANGED BIT(11)
+
/* Collision Detector Control 0 Register */
#define LAN86XX_REG_COL_DET_CTRL0 0x0087
#define COL_DET_CTRL0_ENABLE_BIT_MASK BIT(15)
@@ -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;
+
+ 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;
}
static int lan867x_revd0_config_init(struct phy_device *phydev)
@@ -506,6 +529,111 @@ static int lan86xx_read_status(struct phy_device *phydev)
return 0;
}
+/* Read LAN86XX_REG_STS1, which clears the latched status bits on read. */
+static int lan86xx_read_clear_sts1(struct phy_device *phydev)
+{
+ return phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_STS1);
+}
+
+/* Mask (mask bit = 1) or unmask (mask bit = 0) the given STS1 bits in
+ * IMSK1.
+ */
+static int lan86xx_set_intr_mask(struct phy_device *phydev, u16 mask,
+ bool enable)
+{
+ if (enable)
+ /* A mask bit of 0 enables the corresponding interrupt. */
+ return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
+ LAN86XX_REG_IMSK1, mask);
+
+ return phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IMSK1,
+ mask);
+}
+
+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);
+ }
+
+ 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 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 = lan86xx_read_clear_sts1(phydev);
+ 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;
+}
+
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,
.get_plca_status = genphy_c45_plca_get_status,
@@ -523,6 +653,8 @@ static struct phy_driver microchip_t1s_driver[] = {
.features = PHY_BASIC_T1S_P2MP_FEATURES,
.config_init = lan867x_revc_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 = lan86xx_plca_set_cfg,
.get_plca_status = genphy_c45_plca_get_status,
@@ -533,6 +665,8 @@ static struct phy_driver microchip_t1s_driver[] = {
.features = PHY_BASIC_T1S_P2MP_FEATURES,
.config_init = lan867x_revc_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 = lan86xx_plca_set_cfg,
.get_plca_status = genphy_c45_plca_get_status,
@@ -556,6 +690,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,
.read_mmd = genphy_read_mmd_c45,
.write_mmd = genphy_write_mmd_c45,
.get_plca_cfg = genphy_c45_plca_get_cfg,
--
2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH net-next v3 1/4] net: phy: microchip_t1s: fix collision detection on PLCA status change
2026-09-18 14:32 ` [PATCH net-next v3 1/4] net: phy: " Parthiban Veerasooran
@ 2026-09-22 15:08 ` netdev-bot+sashiko
0 siblings, 0 replies; 9+ messages in thread
From: netdev-bot+sashiko @ 2026-09-22 15:08 UTC (permalink / raw)
To: parthiban.veerasooran
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, hkallweit1, linux,
netdev, linux-kernel, UNGLinuxDriver
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net-next v3 2/4] net: ethernet: oa_tc6: deliver the PHY interrupt to phylib
2026-09-18 14:32 [PATCH net-next v3 0/4] net: microchip_t1s: fix collision detection on PLCA status change Parthiban Veerasooran
2026-09-18 14:32 ` [PATCH net-next v3 1/4] net: phy: " Parthiban Veerasooran
@ 2026-09-18 14:33 ` Parthiban Veerasooran
2026-09-22 15:08 ` netdev-bot+sashiko
2026-09-18 14:33 ` [PATCH net-next v3 3/4] microchip: lan865x: enable PHY interrupt via virtual IRQ Parthiban Veerasooran
2026-09-18 14:33 ` [PATCH net-next v3 4/4] net: phy: microchip_t1s: fix collision detection for LAN867X Rev.D0 Parthiban Veerasooran
3 siblings, 1 reply; 9+ messages in thread
From: Parthiban Veerasooran @ 2026-09-18 14:33 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, pabeni, hkallweit1, linux
Cc: netdev, linux-kernel, UNGLinuxDriver, Parthiban.Veerasooran,
Parthiban Veerasooran
The OA TC6 standard defines PHY interrupt delivery via the SPI status
register as optional. The PHY has no dedicated interrupt line in that
case; its interrupt is signalled through the MAC-PHY SPI interface, but
phy_mac_interrupt() can't make the PHY driver read and acknowledge its
own status registers. Expose it to phylib instead as a nested virtual
IRQ, gated by a new OA_TC6_PHY_INT quirk flag for drivers that route
PHY interrupts in-band.
When set, a nested virtual IRQ is created in oa_tc6_mdiobus_register()
before mdiobus_register(), and all mii_bus->irq[] entries are populated
with it so phy_device_create() picks it up regardless of MDIO address.
Teardown is integrated into oa_tc6_mdiobus_unregister().
A custom irq_chip (oa_tc6_phy_irq_chip) implements mask/unmask via
irq_bus_lock/irq_bus_sync_unlock, writing the mask bit to hardware over
SPI. The interrupt starts masked (hardware reset default) and is only
unmasked when phylib requests it, so disabling the nested IRQ actually
masks the hardware source too, preventing interrupt storms.
Dispatch is deferred to a workqueue rather than run synchronously from
the threaded IRQ: phy_interrupt() takes phydev->lock and PHY
handle_interrupt() issues synchronous SPI transfers, either of which
would otherwise stall the single thread pumping every TX/RX data chunk.
PHYINT is level triggered and stays asserted until acked, so a no-op
reschedule on an already-pending work item can't lose or duplicate an
event.
Select IRQ_DOMAIN in Kconfig for the irq_domain APIs used here.
Prerequisite for "net: phy: microchip_t1s: fix collision detection on
PLCA status change" (Fixes: 78341049fbcd) to fully cover the LAN865X
internal PHY.
Signed-off-by: Parthiban Veerasooran <parthiban.veerasooran@microchip.com>
---
drivers/net/ethernet/Kconfig | 1 +
drivers/net/ethernet/oa_tc6.c | 145 ++++++++++++++++++++++++++++++++++
include/linux/oa_tc6.h | 3 +
3 files changed, 149 insertions(+)
diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
index 8581ccba1505..ea2161232a17 100644
--- a/drivers/net/ethernet/Kconfig
+++ b/drivers/net/ethernet/Kconfig
@@ -150,6 +150,7 @@ config OA_TC6
tristate "OPEN Alliance TC6 10BASE-T1x MAC-PHY support" if COMPILE_TEST
depends on SPI
select PHYLIB
+ select IRQ_DOMAIN
help
This library implements OPEN Alliance TC6 10BASE-T1x MAC-PHY
Serial Interface protocol for supporting 10BASE-T1x MAC-PHYs.
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index 6fcc5f561d56..b4a95155e924 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -8,6 +8,8 @@
#include <linux/bitfield.h>
#include <linux/iopoll.h>
#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
#include <linux/mdio.h>
#include <linux/phy.h>
#include <linux/oa_tc6.h>
@@ -70,6 +72,11 @@ struct oa_tc6 {
struct phy_device *phydev;
struct mii_bus *mdiobus;
struct spi_device *spi;
+ struct mutex phy_irq_lock; /* Serialises irq_bus_lock/sync_unlock */
+ bool phy_irq_masked; /* Shadow of OA_TC6_INT_MASK0_PHY_INT_MASK */
+ struct irq_domain *phy_irq_domain;
+ int phy_virq;
+ struct work_struct phy_irq_work;
struct mutex spi_ctrl_lock; /* Protects spi control transfer */
spinlock_t tx_skb_lock; /* Protects tx skb handling */
void *spi_ctrl_tx_buf;
@@ -528,6 +535,114 @@ int oa_tc6_mdiobus_write_c45(struct mii_bus *bus, int addr, int devnum,
}
EXPORT_SYMBOL_GPL(oa_tc6_mdiobus_write_c45);
+static void oa_tc6_phy_irq_work(struct work_struct *work)
+{
+ struct oa_tc6 *tc6 = container_of(work, struct oa_tc6, phy_irq_work);
+
+ /* Dispatched off the SPI chunk-processing thread so that
+ * phy_interrupt() taking phydev->lock and issuing synchronous SPI
+ * control transfers from PHY handle_interrupt() cannot stall the single
+ * thread pumping TX/RX data chunks.
+ */
+ handle_nested_irq(tc6->phy_virq);
+}
+
+static void oa_tc6_phy_irq_mask(struct irq_data *irqd)
+{
+ struct oa_tc6 *tc6 = irq_data_get_irq_chip_data(irqd);
+
+ tc6->phy_irq_masked = true;
+}
+
+static void oa_tc6_phy_irq_unmask(struct irq_data *irqd)
+{
+ struct oa_tc6 *tc6 = irq_data_get_irq_chip_data(irqd);
+
+ tc6->phy_irq_masked = false;
+}
+
+static void oa_tc6_phy_irq_bus_lock(struct irq_data *irqd)
+{
+ struct oa_tc6 *tc6 = irq_data_get_irq_chip_data(irqd);
+
+ mutex_lock(&tc6->phy_irq_lock);
+}
+
+static void oa_tc6_phy_irq_bus_sync_unlock(struct irq_data *irqd)
+{
+ struct oa_tc6 *tc6 = irq_data_get_irq_chip_data(irqd);
+ u32 regval;
+ int ret;
+
+ ret = oa_tc6_read_register(tc6, OA_TC6_REG_INT_MASK0, ®val);
+ if (ret) {
+ dev_err(&tc6->spi->dev, "Failed to read INT_MASK0: %d\n", ret);
+ goto unlock;
+ }
+
+ if (tc6->phy_irq_masked)
+ regval |= OA_TC6_INT_MASK0_PHY_INT_MASK;
+ else
+ regval &= ~OA_TC6_INT_MASK0_PHY_INT_MASK;
+
+ ret = oa_tc6_write_register(tc6, OA_TC6_REG_INT_MASK0, regval);
+ if (ret)
+ dev_err(&tc6->spi->dev, "Failed to write INT_MASK0: %d\n", ret);
+
+unlock:
+ mutex_unlock(&tc6->phy_irq_lock);
+}
+
+static struct irq_chip oa_tc6_phy_irq_chip = {
+ .name = "oa_tc6_phy",
+ .irq_mask = oa_tc6_phy_irq_mask,
+ .irq_unmask = oa_tc6_phy_irq_unmask,
+ .irq_bus_lock = oa_tc6_phy_irq_bus_lock,
+ .irq_bus_sync_unlock = oa_tc6_phy_irq_bus_sync_unlock,
+};
+
+static int oa_tc6_phy_irq_map(struct irq_domain *domain, unsigned int irq,
+ irq_hw_number_t hwirq)
+{
+ irq_set_chip_data(irq, domain->host_data);
+ irq_set_chip_and_handler(irq, &oa_tc6_phy_irq_chip, handle_simple_irq);
+ irq_set_nested_thread(irq, true);
+ irq_set_noprobe(irq);
+
+ return 0;
+}
+
+static const struct irq_domain_ops oa_tc6_phy_irq_domain_ops = {
+ .map = oa_tc6_phy_irq_map,
+};
+
+static int oa_tc6_phy_irq_setup(struct oa_tc6 *tc6)
+{
+ INIT_WORK(&tc6->phy_irq_work, oa_tc6_phy_irq_work);
+
+ tc6->phy_irq_domain =
+ irq_domain_create_linear(NULL, 1,
+ &oa_tc6_phy_irq_domain_ops, tc6);
+ if (!tc6->phy_irq_domain)
+ return -ENOMEM;
+
+ tc6->phy_virq = irq_create_mapping(tc6->phy_irq_domain, 0);
+ tc6->phy_irq_masked = true;
+ if (!tc6->phy_virq) {
+ irq_domain_remove(tc6->phy_irq_domain);
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static void oa_tc6_phy_irq_teardown(struct oa_tc6 *tc6)
+{
+ cancel_work_sync(&tc6->phy_irq_work);
+ irq_dispose_mapping(tc6->phy_virq);
+ irq_domain_remove(tc6->phy_irq_domain);
+}
+
static int oa_tc6_mdiobus_register(struct oa_tc6 *tc6)
{
int ret;
@@ -559,9 +674,25 @@ static int oa_tc6_mdiobus_register(struct oa_tc6 *tc6)
snprintf(tc6->mdiobus->id, ARRAY_SIZE(tc6->mdiobus->id), "%s",
dev_name(&tc6->spi->dev));
+ if (tc6->quirk_flags & OA_TC6_PHY_INT) {
+ ret = oa_tc6_phy_irq_setup(tc6);
+ if (ret) {
+ mdiobus_free(tc6->mdiobus);
+ return ret;
+ }
+ /* Populate all irq[] entries before registration so
+ * phy_device_create() picks up the virtual IRQ regardless of
+ * the PHY's MDIO address.
+ */
+ for (int i = 0; i < PHY_MAX_ADDR; i++)
+ tc6->mdiobus->irq[i] = tc6->phy_virq;
+ }
+
ret = mdiobus_register(tc6->mdiobus);
if (ret) {
netdev_err(tc6->netdev, "Could not register MDIO bus\n");
+ if (tc6->quirk_flags & OA_TC6_PHY_INT)
+ oa_tc6_phy_irq_teardown(tc6);
mdiobus_free(tc6->mdiobus);
return ret;
}
@@ -572,6 +703,8 @@ static int oa_tc6_mdiobus_register(struct oa_tc6 *tc6)
static void oa_tc6_mdiobus_unregister(struct oa_tc6 *tc6)
{
mdiobus_unregister(tc6->mdiobus);
+ if (tc6->quirk_flags & OA_TC6_PHY_INT)
+ oa_tc6_phy_irq_teardown(tc6);
mdiobus_free(tc6->mdiobus);
}
@@ -810,6 +943,17 @@ static int oa_tc6_process_extended_status(struct oa_tc6 *tc6)
return ret;
}
+ /* Dispatch the PHY interrupt to phylib via the nested virtual IRQ so
+ * the PHY driver reads and acknowledges its status. This is deferred
+ * to a workqueue rather than dispatched synchronously here, since
+ * phy_interrupt() takes phydev->lock and PHY handle_interrupt() issues
+ * synchronous SPI control transfers, which would otherwise block this
+ * thread.
+ */
+ if ((tc6->quirk_flags & OA_TC6_PHY_INT) &&
+ FIELD_GET(OA_TC6_STATUS0_PHY_INT, value))
+ schedule_work(&tc6->phy_irq_work);
+
if (FIELD_GET(OA_TC6_STATUS0_RX_BUFFER_OVERFLOW_ERROR, value)) {
oa_tc6_look_for_new_frame(tc6);
net_err_ratelimited("%s: Receive buffer overflow error\n",
@@ -1468,6 +1612,7 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev,
tc6->spi = spi;
tc6->netdev = netdev;
SET_NETDEV_DEV(netdev, &spi->dev);
+ mutex_init(&tc6->phy_irq_lock);
mutex_init(&tc6->spi_ctrl_lock);
spin_lock_init(&tc6->tx_skb_lock);
diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h
index 27f652d4920b..1c8cc263c5e5 100644
--- a/include/linux/oa_tc6.h
+++ b/include/linux/oa_tc6.h
@@ -30,6 +30,7 @@
/* Status Register #0 */
#define OA_TC6_REG_STATUS0 0x0008
+#define OA_TC6_STATUS0_PHY_INT BIT(7)
#define OA_TC6_STATUS0_RESETC BIT(6) /* Reset Complete */
#define OA_TC6_STATUS0_HEADER_ERROR BIT(5)
#define OA_TC6_STATUS0_LOSS_OF_FRAME_ERROR BIT(4)
@@ -43,6 +44,7 @@
/* Interrupt Mask Register #0 */
#define OA_TC6_REG_INT_MASK0 0x000C
+#define OA_TC6_INT_MASK0_PHY_INT_MASK BIT(7)
#define OA_TC6_INT_MASK0_HEADER_ERR_MASK BIT(5)
#define OA_TC6_INT_MASK0_LOSS_OF_FRAME_ERR_MASK BIT(4)
#define OA_TC6_INT_MASK0_RX_BUFFER_OVERFLOW_ERR_MASK BIT(3)
@@ -68,6 +70,7 @@ struct oa_tc6;
enum oa_tc6_quirk_flag {
OA_TC6_BROKEN_PHY = BIT(0),
+ OA_TC6_PHY_INT = BIT(1),
};
struct oa_tc6_quirks {
--
2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH net-next v3 2/4] net: ethernet: oa_tc6: deliver the PHY interrupt to phylib
2026-09-18 14:33 ` [PATCH net-next v3 2/4] net: ethernet: oa_tc6: deliver the PHY interrupt to phylib Parthiban Veerasooran
@ 2026-09-22 15:08 ` netdev-bot+sashiko
0 siblings, 0 replies; 9+ messages in thread
From: netdev-bot+sashiko @ 2026-09-22 15:08 UTC (permalink / raw)
To: parthiban.veerasooran
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, hkallweit1, linux,
netdev, linux-kernel, UNGLinuxDriver
Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 1 · Low: 4
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 new irq_chip (drivers/net/ethernet/oa_tc6.c:596-602)
provides only .irq_mask/.irq_unmask (shadow flag flushed from…
- [Medium] In oa_tc6_process_extended_status()
(drivers/net/ethernet/oa_tc6.c:953-955) the driver only calls…
- [Low] Concern that OA_TC6_PHY_INT and all code it gates is dead in-tree
because no oa_tc6 consumer sets the flag.
- [Low] Inconsistent quirk gating: oa_tc6_process_extended_status()
(oa_tc6.c:953) schedules tc6->phy_irq_work based only on…
- [Low] Concern that oa_tc6_phy_irq_bus_sync_unlock() (oa_tc6.c:571-594)
only logs a failed INT_MASK0 read or write, leaving the hardware mask…
- [Low] oa_tc6_phy_irq_bus_sync_unlock()
(drivers/net/ethernet/oa_tc6.c:571-594) performs a read-modify-write…
--
net: ethernet: oa_tc6: deliver the PHY interrupt to phylib
This adds a nested virtual IRQ plus a custom irq_chip to oa_tc6 so an
in-band MAC-PHY PHY interrupt can be delivered to phylib, gated by a new
OA_TC6_PHY_INT quirk flag. Dispatch of the nested IRQ is deferred to a
work item, and the mask bit is pushed to INT_MASK0 from
irq_bus_sync_unlock().
A few questions below about the mask handling and the dispatch gating.
> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 6fcc5f561d56..b4a95155e924 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c
[ ... ]
> @@ -528,6 +535,114 @@ int oa_tc6_mdiobus_write_c45(struct mii_bus *bus, int addr, int devnum,
> }
> EXPORT_SYMBOL_GPL(oa_tc6_mdiobus_write_c45);
>
> +static void oa_tc6_phy_irq_work(struct work_struct *work)
> +{
> + struct oa_tc6 *tc6 = container_of(work, struct oa_tc6, phy_irq_work);
> +
> + /* Dispatched off the SPI chunk-processing thread so that
> + * phy_interrupt() taking phydev->lock and issuing synchronous SPI
> + * control transfers from PHY handle_interrupt() cannot stall the single
> + * thread pumping TX/RX data chunks.
> + */
> + handle_nested_irq(tc6->phy_virq);
> +}
> +
[ ... ]
> +static void oa_tc6_phy_irq_bus_sync_unlock(struct irq_data *irqd)
> +{
> + struct oa_tc6 *tc6 = irq_data_get_irq_chip_data(irqd);
> + u32 regval;
> + int ret;
> +
> + ret = oa_tc6_read_register(tc6, OA_TC6_REG_INT_MASK0, ®val);
> + if (ret) {
> + dev_err(&tc6->spi->dev, "Failed to read INT_MASK0: %d\n", ret);
> + goto unlock;
> + }
> +
> + if (tc6->phy_irq_masked)
> + regval |= OA_TC6_INT_MASK0_PHY_INT_MASK;
> + else
> + regval &= ~OA_TC6_INT_MASK0_PHY_INT_MASK;
> +
> + ret = oa_tc6_write_register(tc6, OA_TC6_REG_INT_MASK0, regval);
> + if (ret)
> + dev_err(&tc6->spi->dev, "Failed to write INT_MASK0: %d\n", ret);
> +
> +unlock:
> + mutex_unlock(&tc6->phy_irq_lock);
> +}
[Severity: Low]
This read-modify-write of INT_MASK0 is serialised by the new
tc6->phy_irq_lock, but is that lock taken by the other writer of the same
register?
oa_tc6_disable_traffic() writes INT_MASK0 from the threaded IRQ context
without phy_irq_lock:
oa_tc6_write_register(tc6, OA_TC6_REG_INT_MASK0, regval);
tc6->spi_ctrl_lock only makes each individual control transfer atomic, not
the read/modify/write pair here, so can this sequence lose an update?
CPU0 (oa_tc6_macphy_threaded_irq -> oa_tc6_disable_traffic)
writes INT_MASK0 = OA_TC6_INT_MASK0_ALL_INTERRUPTS
CPU1 (phylib mask/unmask -> oa_tc6_phy_irq_bus_sync_unlock)
read INT_MASK0 (stale copy, taken before CPU0's write)
write INT_MASK0 (reverts the error-interrupt masking)
The reverse order would clobber the PHY_INT mask bit phylib just asked
for. The practical effect looks small since
oa_tc6_macphy_threaded_irq() calls disable_irq_nosync(tc6->spi->irq)
before oa_tc6_disable_traffic(), but should all INT_MASK0 modifiers share
one lock or a lock-protected shadow value?
[Severity: Low]
This isn't a bug, but both error paths here only log and return, so
tc6->phy_irq_masked (and phylib's view of the line) can stay permanently
out of sync with the hardware mask bit, with no retry. The same
log-and-continue shape is used by regmap_irq_sync_unlock() in
drivers/base/regmap/regmap-irq.c, since the genirq callback returns void
and has nowhere to propagate an error, and a failing oa_tc6 control
transfer already means the SPI link to the MAC-PHY is broken. Is it worth
noting in a comment that a failed sync leaves the mask bit undefined?
> +
> +static struct irq_chip oa_tc6_phy_irq_chip = {
> + .name = "oa_tc6_phy",
> + .irq_mask = oa_tc6_phy_irq_mask,
> + .irq_unmask = oa_tc6_phy_irq_unmask,
> + .irq_bus_lock = oa_tc6_phy_irq_bus_lock,
> + .irq_bus_sync_unlock = oa_tc6_phy_irq_bus_sync_unlock,
> +};
> +
> +static int oa_tc6_phy_irq_map(struct irq_domain *domain, unsigned int irq,
> + irq_hw_number_t hwirq)
> +{
> + irq_set_chip_data(irq, domain->host_data);
> + irq_set_chip_and_handler(irq, &oa_tc6_phy_irq_chip, handle_simple_irq);
> + irq_set_nested_thread(irq, true);
> + irq_set_noprobe(irq);
> +
> + return 0;
> +}
[Severity: High]
The commit message says:
"The interrupt starts masked (hardware reset default) and is only
unmasked when phylib requests it, so disabling the nested IRQ actually
masks the hardware source too, preventing interrupt storms."
Is that true for this chip? There is no .irq_disable callback and
oa_tc6_phy_irq_map() does not set IRQ_DISABLE_UNLAZY, so
disable_irq()/disable_irq_nosync() takes the lazy path in kernel/irq/chip.c:
irq_state_set_disabled(desc);
if (desc->irq_data.chip->irq_disable) {
desc->irq_data.chip->irq_disable(&desc->irq_data);
irq_state_set_masked(desc);
} else if (mask) {
mask_irq(desc);
}
With no .irq_disable and mask == false, OA_TC6_INT_MASK0_PHY_INT_MASK is
never written, so the in-band source stays armed.
The usual lazy-disable safety net, where the flow handler masks the line on
the next occurrence, does not apply to nested IRQs. handle_nested_irq()
goes through irq_can_handle_actions():
if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
desc->istate |= IRQS_PENDING;
return false;
}
so phy_interrupt() is never called and nothing masks the line.
Can this then be reached from phylib itself? phy_interrupt() in
drivers/net/phy/phy.c does:
phydev->irq_rerun = 1;
disable_irq_nosync(irq);
return IRQ_HANDLED;
i.e. the virq is disabled from inside the nested handler without the PHY
status being read or acked, and it is only re-enabled later in
mdio_bus_phy_resume():
if (phydev->irq_rerun) {
phydev->irq_rerun = 0;
enable_irq(phydev->irq);
In that window PHYINT is unmasked in INT_MASK0 and unacknowledged, so the
MAC-PHY keeps IRQn asserted and oa_tc6_macphy_isr() ->
oa_tc6_macphy_threaded_irq() -> oa_tc6_process_extended_status() keeps
looping with an empty-chunk transfer plus a STATUS0 read/write and a no-op
schedule_work() each round, with nothing able to clear the condition.
Would adding an .irq_disable callback, or
irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY) in oa_tc6_phy_irq_map(),
make a disabled virq really mask the in-band source as the commit message
describes?
[ ... ]
> @@ -810,6 +943,17 @@ static int oa_tc6_process_extended_status(struct oa_tc6 *tc6)
> return ret;
> }
>
> + /* Dispatch the PHY interrupt to phylib via the nested virtual IRQ so
> + * the PHY driver reads and acknowledges its status. This is deferred
> + * to a workqueue rather than dispatched synchronously here, since
> + * phy_interrupt() takes phydev->lock and PHY handle_interrupt() issues
> + * synchronous SPI control transfers, which would otherwise block this
> + * thread.
> + */
> + if ((tc6->quirk_flags & OA_TC6_PHY_INT) &&
> + FIELD_GET(OA_TC6_STATUS0_PHY_INT, value))
> + schedule_work(&tc6->phy_irq_work);
> +
[Severity: Medium]
Should OA_TC6_INT_MASK0_PHY_INT_MASK be set here for the duration of the
deferral?
As the commit message notes, PHYINT "stays asserted until acked", and the
ack only happens later in oa_tc6_phy_irq_work() -> handle_nested_irq() ->
phy_interrupt() -> drv->handle_interrupt(). Until the worker runs, STATUS0
keeps bit 7 set, so the existing per-chunk short-circuit earlier in this
function:
if (!value)
return 0;
never fires, and every RX chunk footer carrying EXT_STS pays an extra
STATUS0 read plus STATUS0 write control transfer on the single SPI
chunk-processing thread.
Also, the MAC-PHY IRQ is requested with IRQF_TRIGGER_LOW | IRQF_ONESHOT in
oa_tc6_init(), so does the still-asserted line re-trigger the hard ISR as
soon as the threaded handler returns, adding another empty-chunk transfer
per round until the work item is scheduled? The irq thread runs at
realtime priority while the ack worker runs on system_percpu_wq at normal
priority, so that window is under the worker's scheduling latency. The
mask bit and the shadow/bus_lock machinery added by this patch already give
a way to quiesce the source while the ack is deferred.
[Severity: Low]
This dispatch is gated on OA_TC6_PHY_INT alone, while the only initializer
of tc6->phy_irq_work and tc6->phy_virq, oa_tc6_phy_irq_setup() called from
oa_tc6_mdiobus_register(), is skipped when OA_TC6_BROKEN_PHY is set:
oa_tc6_phy_init()
if (tc6->quirk_flags & OA_TC6_BROKEN_PHY)
return 0;
For a consumer passing OA_TC6_BROKEN_PHY | OA_TC6_PHY_INT, would this
schedule_work() then run on a devm_kzalloc()-zeroed work_struct that never
saw INIT_WORK(), and would the worker call handle_nested_irq(0)? No
in-tree caller combines the two flags today, so this is not reachable as
things stand, but the flags are independent bits in an exported enum in
include/linux/oa_tc6.h. Would gating the dispatch on the resource actually
created, for example a non-zero tc6->phy_virq, make the lifecycle
self-consistent?
> if (FIELD_GET(OA_TC6_STATUS0_RX_BUFFER_OVERFLOW_ERROR, value)) {
> oa_tc6_look_for_new_frame(tc6);
> net_err_ratelimited("%s: Receive buffer overflow error\n",
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918143302.76909-1-parthiban.veerasooran%40microchip.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net-next v3 3/4] microchip: lan865x: enable PHY interrupt via virtual IRQ
2026-09-18 14:32 [PATCH net-next v3 0/4] net: microchip_t1s: fix collision detection on PLCA status change Parthiban Veerasooran
2026-09-18 14:32 ` [PATCH net-next v3 1/4] net: phy: " Parthiban Veerasooran
2026-09-18 14:33 ` [PATCH net-next v3 2/4] net: ethernet: oa_tc6: deliver the PHY interrupt to phylib Parthiban Veerasooran
@ 2026-09-18 14:33 ` Parthiban Veerasooran
2026-09-22 15:08 ` netdev-bot+sashiko
2026-09-18 14:33 ` [PATCH net-next v3 4/4] net: phy: microchip_t1s: fix collision detection for LAN867X Rev.D0 Parthiban Veerasooran
3 siblings, 1 reply; 9+ messages in thread
From: Parthiban Veerasooran @ 2026-09-18 14:33 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, pabeni, hkallweit1, linux
Cc: netdev, linux-kernel, UNGLinuxDriver, Parthiban.Veerasooran,
Parthiban Veerasooran
The LAN865X integrates the PHY directly into the MAC-PHY SPI device.
The PHY has no dedicated interrupt line; its interrupt is always
delivered as the PHYINT bit in STATUS0.
Pass the OA_TC6_PHY_INT quirk flag to oa_tc6_init() to enable the
virtual IRQ infrastructure. This lets phylib operate in interrupt mode
and drives the PHY driver's config_intr/handle_interrupt callbacks for
the LAN865X internal PHY.
This completes the collision-detection fix started in "net: phy:
microchip_t1s: fix collision detection on PLCA status change"; without
this patch (and its prerequisite, "net: ethernet: oa_tc6: deliver the
PHY interrupt to phylib"), LAN865X stays on that commit's static CDEN
fallback instead of tracking PLCA transitions dynamically.
lan865x_revb_fixup_registers[] does not program COL_DET_CTRL0
(0x0087), so CDEN correctness relies on that bit's hardware reset
default together with lan86xx_handle_interrupt() tracking every
subsequent PLCA transition via the PSTC interrupt this patch enables.
Fixes: 78341049fbcd ("net: phy: microchip_t1s: configure collision detection based on PLCA mode")
Signed-off-by: Parthiban Veerasooran <parthiban.veerasooran@microchip.com>
---
drivers/net/ethernet/microchip/lan865x/lan865x.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/microchip/lan865x/lan865x.c b/drivers/net/ethernet/microchip/lan865x/lan865x.c
index 26a2761332a5..043a3dc3f965 100644
--- a/drivers/net/ethernet/microchip/lan865x/lan865x.c
+++ b/drivers/net/ethernet/microchip/lan865x/lan865x.c
@@ -332,6 +332,7 @@ static const struct net_device_ops lan865x_netdev_ops = {
static int lan865x_probe(struct spi_device *spi)
{
+ struct oa_tc6_quirks tc6_quirks = {};
struct net_device *netdev;
struct lan865x_priv *priv;
int ret;
@@ -346,7 +347,8 @@ static int lan865x_probe(struct spi_device *spi)
spi_set_drvdata(spi, priv);
INIT_WORK(&priv->multicast_work, lan865x_multicast_work_handler);
- priv->tc6 = oa_tc6_init(spi, netdev, NULL);
+ tc6_quirks.quirk_flags = OA_TC6_PHY_INT;
+ priv->tc6 = oa_tc6_init(spi, netdev, &tc6_quirks);
if (!priv->tc6) {
ret = -ENODEV;
goto free_netdev;
--
2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH net-next v3 3/4] microchip: lan865x: enable PHY interrupt via virtual IRQ
2026-09-18 14:33 ` [PATCH net-next v3 3/4] microchip: lan865x: enable PHY interrupt via virtual IRQ Parthiban Veerasooran
@ 2026-09-22 15:08 ` netdev-bot+sashiko
0 siblings, 0 replies; 9+ messages in thread
From: netdev-bot+sashiko @ 2026-09-22 15:08 UTC (permalink / raw)
To: parthiban.veerasooran
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, hkallweit1, linux,
netdev, linux-kernel, UNGLinuxDriver
Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] The commit message's final paragraph is factually wrong about how
collision detection is initialized: "lan865x_revb_fixup_registers[]…
- [Low] Reported concern: enabling OA_TC6_PHY_INT routes PHY IRQ
mask/unmask through oa_tc6_phy_irq_mask()/unmask()…
Pre-existing issues:
- [High] lan865x_remove()
(drivers/net/ethernet/microchip/lan865x/lan865x.c:420) calls…
--
microchip: lan865x: enable PHY interrupt via virtual IRQ
This passes the OA_TC6_PHY_INT quirk flag to oa_tc6_init() so the LAN865X
internal PHY interrupt, delivered as STATUS0.PHYINT, is exposed to phylib
as a virtual IRQ. That lets phylib run the PHY driver's config_intr and
handle_interrupt callbacks instead of polling.
A few questions below, one about the commit message and two about the
code paths this flag now activates.
> lan865x_revb_fixup_registers[] does not program COL_DET_CTRL0
> (0x0087), so CDEN correctness relies on that bit's hardware reset
> default together with lan86xx_handle_interrupt() tracking every
> subsequent PLCA transition via the PSTC interrupt this patch enables.
[Severity: Low]
Is the reliance on the reset default accurate here? Once this patch makes
the PHY IRQ valid, phy_request_interrupt() calls phy_enable_interrupts(),
which reaches lan86xx_config_intr() in drivers/net/phy/microchip_t1s.c,
and that callback programs COL_DET_CTRL0 from the live PLCA status before
unmasking PSTC:
drivers/net/phy/microchip_t1s.c:lan86xx_config_intr() {
...
ret = genphy_c45_plca_get_status(phydev, &plca_st);
...
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);
...
}
Would it be more precise to say CDEN is explicitly synchronized by
config_intr when interrupts are enabled, and then tracked by
lan86xx_handle_interrupt() on each PSTC event? As written the paragraph
may leave readers thinking the absence of 0x0087 from the fixup table is
an open gap.
> diff --git a/drivers/net/ethernet/microchip/lan865x/lan865x.c b/drivers/net/ethernet/microchip/lan865x/lan865x.c
> index 26a2761332a5..043a3dc3f965 100644
> --- a/drivers/net/ethernet/microchip/lan865x/lan865x.c
> +++ b/drivers/net/ethernet/microchip/lan865x/lan865x.c
[ ... ]
> @@ -346,7 +347,8 @@ static int lan865x_probe(struct spi_device *spi)
> spi_set_drvdata(spi, priv);
> INIT_WORK(&priv->multicast_work, lan865x_multicast_work_handler);
>
> - priv->tc6 = oa_tc6_init(spi, netdev, NULL);
> + tc6_quirks.quirk_flags = OA_TC6_PHY_INT;
> + priv->tc6 = oa_tc6_init(spi, netdev, &tc6_quirks);
[Severity: Low]
Setting OA_TC6_PHY_INT routes the PHY mask/unmask through the virtual
irq_chip in drivers/net/ethernet/oa_tc6.c. oa_tc6_phy_irq_unmask() sets
tc6->phy_irq_masked = false immediately, and the actual INT_MASK0
read-modify-write happens later in oa_tc6_phy_irq_bus_sync_unlock(),
where a failure is only logged:
drivers/net/ethernet/oa_tc6.c:oa_tc6_phy_irq_bus_sync_unlock() {
...
ret = oa_tc6_write_register(tc6, OA_TC6_REG_INT_MASK0, regval);
if (ret)
dev_err(&tc6->spi->dev, "Failed to write INT_MASK0: %d\n", ret);
...
}
If a transient SPI control transfer failure hits just that access, can
STATUS0.PHYINT stay masked in hardware while the software shadow says
unmasked, so later autonomous PLCA transitions never reach
lan86xx_handle_interrupt() and CDEN goes stale?
For context, irq_bus_sync_unlock() returns void so there is no way to
propagate the error to the IRQ core, and regmap_irq_sync_unlock() logs and
continues in the same way, so this may simply be the accepted behaviour
for slow-bus irq_chips. Also, if the config_intr sequence itself fails,
phy_request_interrupt() falls back to PHY_POLL and the static CDEN write
in lan86xx_plca_set_cfg() is used again. Is a retry or a warning on the
shadow/hardware divergence worth it here, or is log-and-continue fine?
> if (!priv->tc6) {
> ret = -ENODEV;
> goto free_netdev;
[Severity: High]
This isn't a bug introduced by this patch, but while looking at the probe
and teardown paths in this file: lan865x_remove() cancels the multicast
work before unregistering the netdev:
drivers/net/ethernet/microchip/lan865x/lan865x.c:lan865x_remove() {
cancel_work_sync(&priv->multicast_work);
unregister_netdev(priv->netdev);
oa_tc6_exit(priv->tc6);
free_netdev(priv->netdev);
}
ndo_set_rx_mode for this driver does nothing but re-queue that work:
static void lan865x_set_multicast_list(struct net_device *netdev)
{
struct lan865x_priv *priv = netdev_priv(netdev);
schedule_work(&priv->multicast_work);
}
Can an rx-mode change (allmulti/promisc change, multicast join) that
already holds rtnl_lock re-queue the work after cancel_work_sync() and
before unregister_netdev() takes rtnl? There is no second cancel or flush
afterwards, so lan865x_multicast_work_handler() would either issue
oa_tc6_write_register() after oa_tc6_exit(), or still be queued when
free_netdev() frees the netdev private area that priv->multicast_work
lives in.
Would moving unregister_netdev() ahead of cancel_work_sync() close that
window?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918143302.76909-1-parthiban.veerasooran%40microchip.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net-next v3 4/4] net: phy: microchip_t1s: fix collision detection for LAN867X Rev.D0
2026-09-18 14:32 [PATCH net-next v3 0/4] net: microchip_t1s: fix collision detection on PLCA status change Parthiban Veerasooran
` (2 preceding siblings ...)
2026-09-18 14:33 ` [PATCH net-next v3 3/4] microchip: lan865x: enable PHY interrupt via virtual IRQ Parthiban Veerasooran
@ 2026-09-18 14:33 ` Parthiban Veerasooran
2026-09-22 15:08 ` netdev-bot+sashiko
3 siblings, 1 reply; 9+ messages in thread
From: Parthiban Veerasooran @ 2026-09-18 14:33 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, pabeni, hkallweit1, linux
Cc: netdev, linux-kernel, UNGLinuxDriver, Parthiban.Veerasooran,
Parthiban Veerasooran
LAN867X Rev.D0 adds a Collision Counting and MAC Forwarding Control
field (CCMFC, bits 10:9) in COL_DET_CTRL0 (0x0087). Set to the OA
default (0x1), the hardware autonomously gates collision forwarding
based on live PLCA_Status, removing the delay a software-driven CDEN
toggle had on older revisions. The PSTC interrupt handler for Rev.D0
therefore only needs to update the link status selection on each PLCA
transition.
Configure CCMFC to the OA default in lan867x_revd0_config_init(). Add
lan867x_revd0_handle_interrupt() for two events: Link Status Change
triggers the phylib state machine; PLCA Status Change re-reads the PLCA
config and updates the link status selection.
Rev.D0 can also be configured (PRSCTL1, 0x0035, bit 10) to autonomously
fall back to CSMA/CD when no BEACON is seen. When that's active, the
PHY's own hardware transition already handles CSMA/CD correctly, so
driving link status from PLCA_Status reports nothing meaningful. Force
the semaphore (forced-active) source whenever fallback is enabled; only
when fallback is disabled - the PHY stays pinned to PLCA mode - does
tracking PLCA_Status serve its purpose.
Wire up .config_intr/.handle_interrupt for Rev.D0 via a dedicated
lan867x_revd0_config_intr(), reusing the STS1/IMSK1 helpers from the
other LAN86XX PHYs.
Fixes: e7e756779afa ("net: phy: microchip_t1s: add support for Microchip LAN867X Rev.D0 PHY")
Signed-off-by: Parthiban Veerasooran <parthiban.veerasooran@microchip.com>
---
drivers/net/phy/microchip_t1s.c | 169 +++++++++++++++++++++++++++++++-
1 file changed, 164 insertions(+), 5 deletions(-)
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
@@ -20,7 +20,8 @@
#define LAN867X_REG_STS2 0x0019
-#define LAN867x_RESET_COMPLETE_STS BIT(11)
+#define LAN86XX_STS1_LINK_STS_CHANGED BIT(13)
+#define LAN867x_RESET_COMPLETE_STS BIT(11)
#define LAN865X_REG_CFGPARAM_ADDR 0x00D8
#define LAN865X_REG_CFGPARAM_DATA 0x00D9
@@ -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)
/* LAN8670/1/2 Rev.D0 Link Status Selection Register */
#define LAN867X_REG_LINK_STATUS_CTRL 0x0012
@@ -438,6 +450,40 @@ static int lan867x_revd0_link_active_selection(struct phy_device *phydev,
LAN867X_REG_LINK_STATUS_CTRL, value);
}
+static int lan867x_revd0_fallback_to_csmacd(struct phy_device *phydev)
+{
+ int ret;
+
+ ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_PRSCTL1);
+ if (ret < 0)
+ return ret;
+
+ return !!(ret & PRSCTL1_PLCA_FALLB_TO_CSMACD_EN);
+}
+
+/* When the PHY autonomously falls back to CSMA/CD once BEACONs stop (PRSCTL1
+ * bit 10 set), the hardware fallback already provides correct CSMA/CD
+ * operation; selecting link status from PLCA_STATUS in that case reports
+ * nothing meaningful, since the PHY may already be running CSMA/CD regardless
+ * of the stale PLCA_STATUS value. Force the semaphore (forced-active) source
+ * in that case instead. Only when fallback is disabled - the PHY is pinned
+ * to PLCA mode - does tracking PLCA_STATUS serve its intended purpose.
+ */
+static int lan867x_revd0_update_link_selection(struct phy_device *phydev,
+ bool plca_operational)
+{
+ int fallback;
+
+ fallback = lan867x_revd0_fallback_to_csmacd(phydev);
+ if (fallback < 0)
+ return fallback;
+
+ if (fallback)
+ return lan867x_revd0_link_active_selection(phydev, false);
+
+ return lan867x_revd0_link_active_selection(phydev, plca_operational);
+}
+
/* As per LAN8650/1 Rev.B0/B1 AN1760 (Revision F (DS60001760G - June 2024)) and
* LAN8670/1/2 Rev.C1/C2 AN1699 (Revision E (DS60001699F - June 2024)), under
* normal operation, the device should be operated in PLCA mode. Disabling
@@ -446,6 +492,11 @@ static int lan867x_revd0_link_active_selection(struct phy_device *phydev,
* distortion cause poor signal quality. Collision detection must be re-enabled
* if the device is configured to operate in CSMA/CD mode.
*
+ * LAN867X Rev.D0 has autonomous collision detection gating via CCMFC and
+ * does not toggle CDEN in the interrupt handler. CDEN remains permanently
+ * enabled in config_init(), so no software-driven CDEN toggling is needed
+ * here.
+ *
* AN1760: https://www.microchip.com/en-us/application-notes/an1760
* AN1699: https://www.microchip.com/en-us/application-notes/an1699
*/
@@ -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;
@@ -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;
+
/* 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);
+
+ /* 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);
+ }
+
+ ret = lan86xx_set_intr_mask(phydev, mask, 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 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;
+ }
+
+ /* 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;
+}
+
static struct phy_driver microchip_t1s_driver[] = {
{
PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1),
@@ -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,
--
2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH net-next v3 4/4] net: phy: microchip_t1s: fix collision detection for LAN867X Rev.D0
2026-09-18 14:33 ` [PATCH net-next v3 4/4] net: phy: microchip_t1s: fix collision detection for LAN867X Rev.D0 Parthiban Veerasooran
@ 2026-09-22 15:08 ` netdev-bot+sashiko
0 siblings, 0 replies; 9+ messages in thread
From: netdev-bot+sashiko @ 2026-09-22 15:08 UTC (permalink / raw)
To: parthiban.veerasooran
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, hkallweit1, linux,
netdev, linux-kernel, UNGLinuxDriver
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
^ permalink raw reply [flat|nested] 9+ messages in thread