* [PATCH net-next v2 0/4] net: microchip_t1s: fix collision detection on PLCA status change
@ 2026-09-07 5:20 Parthiban Veerasooran
2026-09-07 5:20 ` [PATCH net-next v2 1/4] net: phy: " Parthiban Veerasooran
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Parthiban Veerasooran @ 2026-09-07 5:20 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, pabeni, hkallweit1, linux
Cc: netdev, linux-kernel, UNGLinuxDriver, Parthiban.Veerasooran,
Parthiban Veerasooran
In a 10BASE-T1S multidrop network, when PLCA mode is configured, the PHY
autonomously transitions between PLCA mode and CSMA/CD mode based on
BEACON availability, without any further user action. The existing
collision detection logic only adjusted on explicit ethtool PLCA
configuration changes, leaving it in the wrong state across these
autonomous mode transitions.
This series fixes that gap, wires up the complete interrupt path, and
improves collision detection handling for LAN867X Rev.D0.
Patch 1 fixes the collision detection handling in the PHY driver. It adds
the PLCA Status Changed (PSTC) interrupt handler for LAN86XX PHYs. On
each PSTC interrupt, the PLCA operational status is checked and collision
detection is adjusted accordingly: disabled when PLCA is online,
re-enabled when PLCA goes offline to restore correct CSMA/CD operation.
The interrupt handler uses a conditional assignment pattern without early
returns for better extensibility. The existing static CDEN write in
lan86xx_plca_set_cfg() is retained as a baseline for PHYs running with
phydev->irq == PHY_POLL on boards where the PHY interrupt is not routed
to the host. PHYs with routed interrupts (including LAN865X and LAN867X
with interrupt support) skip the static write since the interrupt handler
handles CDEN dynamically.
Patch 2 delivers the in-band PHY interrupt to phylib from the 10BASE-T1S
MAC-PHY SPI driver. The OA TC6 standard defines PHY interrupt delivery
via the SPI status register as optional; the OA_TC6_PHY_INT quirk flag is
introduced so drivers can opt in when the PHY interrupt is routed in-band.
When set, a nested virtual IRQ is created inside oa_tc6_mdiobus_register()
before mdiobus_register() is called. All mii_bus->irq[] entries are
populated with the virtual IRQ so phy_device_create() picks it up
regardless of the PHY's MDIO address, keeping mii_bus->irq[] and
phydev->irq consistent. The teardown is integrated into
oa_tc6_mdiobus_unregister(). When the PHYINT bit is seen in the extended
status, handle_nested_irq() is dispatched synchronously from the sleepable
threaded IRQ so phylib enters interrupt mode and drives the PHY driver's
config_intr/handle_interrupt callbacks.
Patch 3 enables the PHY interrupt via virtual IRQ for LAN865X by passing
the OA_TC6_PHY_INT quirk flag to oa_tc6_init(). 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.
Patch 4 fixes collision detection for LAN867X Rev.D0 by configuring the
hardware Collision Counting and MAC Forwarding Control field (CCMFC) in
the Collision Detector Control 0 register to the OA default value. The
hardware automatically 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 and forwarded when PLCA_Status is not
OK. This eliminates the inherent delay between a PLCA status change and
the software interrupt handler toggling CDEN, which was a limitation on
older revisions that had no hardware alternative. A dedicated
lan867x_revd0_handle_interrupt() is added to handle two separate interrupt
events: link status change events trigger the phylib state machine for
link re-evaluation; PLCA status change events update the link status
selection. The handler uses the same conditional assignment pattern for
extensibility. Collision detection gating is handled autonomously by
CCMFC in hardware. The .config_intr/.handle_interrupt handlers are wired
up for Rev.D0 using the shared lan86xx_config_intr() and the new handler
to unmask both link status change and PLCA status change interrupts.
Note: Patch 1 carries a Fixes: tag but requires patches 2 and 3 to be
applied together -- the interrupt handler in patch 1 cannot fire without
the virtual IRQ wiring in patch 2, and the virtual IRQ infrastructure is
activated for LAN865X by patch 3. This dependency applies to 10BASE-T1S
MAC-PHYs (e.g. LAN8650/1) where the PHY has no dedicated interrupt line
and relies on the MAC-PHY SPI driver to deliver the interrupt.
Changes in v2:
- Patch 2: Introduce OA_TC6_PHY_INT quirk flag to guard the virtual IRQ
infrastructure; PHYINT is optional per the OA TC6 standard. Move
oa_tc6_phy_irq_setup()/oa_tc6_phy_irq_teardown() into
oa_tc6_mdiobus_register()/oa_tc6_mdiobus_unregister(). Populate all
mii_bus->irq[] entries with the virtual IRQ before mdiobus_register()
so phy_device_create() picks it up regardless of the PHY's MDIO
address, keeping mii_bus->irq[] and phydev->irq consistent.
- Patch 3 (new): Pass OA_TC6_PHY_INT quirk flag from the LAN865X driver
to enable the virtual IRQ infrastructure for LAN865X.
Parthiban Veerasooran (4):
net: phy: microchip_t1s: fix collision detection on PLCA status change
net: ethernet: oa_tc6: deliver the PHY interrupt to phylib
microchip: lan865x: enable PHY interrupt via virtual IRQ
net: phy: microchip_t1s: fix collision detection for LAN867X Rev.D0
.../net/ethernet/microchip/lan865x/lan865x.c | 4 +-
drivers/net/ethernet/oa_tc6.c | 76 ++++++++-
drivers/net/phy/microchip_t1s.c | 159 ++++++++++++++++++
include/linux/oa_tc6.h | 3 +
4 files changed, 238 insertions(+), 4 deletions(-)
base-commit: 31f961de2f90fbf52eb2d4e15b3eeaa09f9b4fc2
--
2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net-next v2 1/4] net: phy: microchip_t1s: fix collision detection on PLCA status change
2026-09-07 5:20 [PATCH net-next v2 0/4] net: microchip_t1s: fix collision detection on PLCA status change Parthiban Veerasooran
@ 2026-09-07 5:20 ` Parthiban Veerasooran
2026-09-10 5:21 ` netdev-bot+sashiko
2026-09-07 5:20 ` [PATCH net-next v2 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-07 5:20 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, pabeni, hkallweit1, linux
Cc: netdev, linux-kernel, UNGLinuxDriver, Parthiban.Veerasooran,
Parthiban Veerasooran
The existing lan86xx_plca_set_cfg() adjusted collision detection
statically at the point the user configured PLCA via ethtool: disabled
when PLCA was enabled, re-enabled when PLCA was disabled. This only
handled the explicit user-driven mode change and missed the dynamic
transitions that the PHY performs autonomously.
In a 10BASE-T1S multidrop network, the PHY tracks BEACON availability
and continuously transitions between PLCA online (actively receiving
BEACONs from the coordinator) and PLCA offline (no BEACON present).
When PLCA goes offline after having been configured online, collision
detection remained disabled, causing the bus to operate in CSMA/CD mode
without collision detection — a silent and hard-to-diagnose error.
Fix this by monitoring the PLCA Status Changed (PSTC) interrupt. PSTC
fires on every PST bit transition in the PLCA Status register. Add
lan86xx_config_intr() to enable/disable the PSTCM mask bit in IMSK1
(bit 11, active-low enable) and lan86xx_handle_interrupt() to service
it. On each interrupt, PLCA operational status is retrieved via
genphy_c45_plca_get_status(). When PLCA comes online, collision
detection is disabled via COL_DET_CTRL0 (bit 15). When PLCA goes
offline, collision detection is re-enabled to restore correct CSMA/CD
operation.
Wire these handlers to all supported PHY variants: LAN867X Rev.B1, C1,
C2 and LAN865X Rev.B0/B1.
LAN867X PHYs may run with phydev->irq == PHY_POLL on boards where the
PHY interrupt is not routed to the host. The existing static CDEN write
in lan86xx_plca_set_cfg() is retained as a baseline so that collision
detection is correct even when the interrupt handler never runs. The
limitation is that autonomous PLCA mode transitions between ethtool
reconfigurations are not tracked on such boards. LAN865X is excluded
from the static write because its interrupt is always routed via the
MAC-PHY SPI driver and the interrupt handler always runs.
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 | 97 +++++++++++++++++++++++++++++++++
1 file changed, 97 insertions(+)
diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index 73c23d311d72..afb7e52594e7 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,6 +466,16 @@ static int lan86xx_plca_set_cfg(struct phy_device *phydev,
if (ret)
return ret;
+ /* PHYs with routed interrupts handle CDEN dynamically via the interrupt
+ * handler, so skip the static write. PHYs running with PHY_POLL have no
+ * interrupt handler, so apply the static CDEN write as a baseline on
+ * every ethtool PLCA reconfiguration. The limitation is that autonomous
+ * PLCA mode transitions between ethtool reconfigurations are not
+ * tracked on such boards.
+ */
+ if (phydev->irq != PHY_POLL)
+ return 0;
+
if (plca_cfg->enabled)
return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
LAN86XX_REG_COL_DET_CTRL0,
@@ -506,6 +524,77 @@ static int lan86xx_read_status(struct phy_device *phydev)
return 0;
}
+static int lan86xx_config_intr(struct phy_device *phydev)
+{
+ int ret;
+
+ if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+ /* Read to clear any pending status before enabling. */
+ ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_STS1);
+ if (ret < 0)
+ return ret;
+
+ /* A mask bit of 0 enables the corresponding interrupt. */
+ return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
+ LAN86XX_REG_IMSK1,
+ LAN86XX_STS1_PLCA_STS_CHANGED);
+ }
+
+ ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IMSK1,
+ LAN86XX_STS1_PLCA_STS_CHANGED);
+ if (ret)
+ return ret;
+
+ /* Read to clear any pending status after disabling. */
+ ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_STS1);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static irqreturn_t lan86xx_handle_interrupt(struct phy_device *phydev)
+{
+ struct phy_plca_status plca_st;
+ irqreturn_t ret_irq = IRQ_NONE;
+ int sts1, ret;
+
+ /* Reading the status register clears the latched event bits. */
+ sts1 = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_STS1);
+ if (sts1 < 0) {
+ phy_error(phydev);
+ return IRQ_NONE;
+ }
+
+ if (sts1 & LAN86XX_STS1_PLCA_STS_CHANGED) {
+ ret = genphy_c45_plca_get_status(phydev, &plca_st);
+ if (ret < 0) {
+ phy_error(phydev);
+ return IRQ_NONE;
+ }
+
+ /* AN1760/AN1699: disable collision detection in PLCA mode to
+ * improve signal quality; re-enable it in CSMA/CD mode.
+ *
+ * https://www.microchip.com/en-us/application-notes/an1760
+ * https://www.microchip.com/en-us/application-notes/an1699
+ */
+ ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
+ LAN86XX_REG_COL_DET_CTRL0,
+ COL_DET_CTRL0_ENABLE_BIT_MASK,
+ plca_st.pst ? COL_DET_DISABLE :
+ COL_DET_ENABLE);
+ if (ret < 0) {
+ phy_error(phydev);
+ return IRQ_NONE;
+ }
+
+ ret_irq = IRQ_HANDLED;
+ }
+
+ return ret_irq;
+}
+
static struct phy_driver microchip_t1s_driver[] = {
{
PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1),
@@ -513,6 +602,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 +614,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 +626,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 +651,8 @@ static struct phy_driver microchip_t1s_driver[] = {
.features = PHY_BASIC_T1S_P2MP_FEATURES,
.config_init = lan865x_revb_config_init,
.read_status = lan86xx_read_status,
+ .config_intr = lan86xx_config_intr,
+ .handle_interrupt = lan86xx_handle_interrupt,
.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
* [PATCH net-next v2 2/4] net: ethernet: oa_tc6: deliver the PHY interrupt to phylib
2026-09-07 5:20 [PATCH net-next v2 0/4] net: microchip_t1s: fix collision detection on PLCA status change Parthiban Veerasooran
2026-09-07 5:20 ` [PATCH net-next v2 1/4] net: phy: " Parthiban Veerasooran
@ 2026-09-07 5:20 ` Parthiban Veerasooran
2026-09-10 5:21 ` netdev-bot+sashiko
2026-09-07 5:20 ` [PATCH net-next v2 3/4] microchip: lan865x: enable PHY interrupt via virtual IRQ Parthiban Veerasooran
2026-09-07 5:20 ` [PATCH net-next v2 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-07 5:20 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. When implemented, the PHY has no dedicated interrupt
line; its interrupt is signalled through the MAC-PHY SPI interface.
phy_mac_interrupt() only triggers a link-status re-read and cannot make
the PHY driver read and acknowledge its interrupt source registers, so
expose the PHY interrupt to phylib as a nested virtual IRQ instead.
Introduce the OA_TC6_PHY_INT quirk flag so drivers can opt in when the
PHY interrupt is routed in-band.
When OA_TC6_PHY_INT is set, use dummy_irq_chip as the irqchip and map
a virtual IRQ inside oa_tc6_mdiobus_register() before mdiobus_register()
is called. Populate all mii_bus->irq[] entries with the virtual IRQ so
phy_device_create() picks it up regardless of the PHY's MDIO address,
keeping mii_bus->irq[] and phydev->irq consistent. The corresponding
teardown is integrated into oa_tc6_mdiobus_unregister().
Unmask PHYINT in INT_MASK0, and when it is seen in the extended status,
dispatch handle_nested_irq() synchronously from the sleepable threaded
IRQ. PHYINT is level triggered, so acking the PHY source there clears it
before the next data chunk, avoiding a storm.
Signed-off-by: Parthiban Veerasooran <parthiban.veerasooran@microchip.com>
---
drivers/net/ethernet/oa_tc6.c | 76 +++++++++++++++++++++++++++++++++--
include/linux/oa_tc6.h | 3 ++
2 files changed, 76 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index 6fcc5f561d56..00b0176c1fe7 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,8 @@ struct oa_tc6 {
struct phy_device *phydev;
struct mii_bus *mdiobus;
struct spi_device *spi;
+ struct irq_domain *phy_irq_domain;
+ int phy_virq;
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 +532,44 @@ int oa_tc6_mdiobus_write_c45(struct mii_bus *bus, int addr, int devnum,
}
EXPORT_SYMBOL_GPL(oa_tc6_mdiobus_write_c45);
+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, &dummy_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)
+{
+ 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);
+ 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)
+{
+ 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 +601,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 +630,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);
}
@@ -661,7 +721,7 @@ static int oa_tc6_sw_reset_macphy(struct oa_tc6 *tc6)
return oa_tc6_write_register(tc6, OA_TC6_REG_STATUS0, regval);
}
-static int oa_tc6_unmask_macphy_error_interrupts(struct oa_tc6 *tc6)
+static int oa_tc6_unmask_interrupts(struct oa_tc6 *tc6)
{
u32 regval;
int ret;
@@ -670,7 +730,8 @@ static int oa_tc6_unmask_macphy_error_interrupts(struct oa_tc6 *tc6)
if (ret)
return ret;
- regval &= ~(OA_TC6_INT_MASK0_TX_PROTOCOL_ERR_MASK |
+ regval &= ~(OA_TC6_INT_MASK0_PHY_INT_MASK |
+ OA_TC6_INT_MASK0_TX_PROTOCOL_ERR_MASK |
OA_TC6_INT_MASK0_RX_BUFFER_OVERFLOW_ERR_MASK |
OA_TC6_INT_MASK0_LOSS_OF_FRAME_ERR_MASK |
OA_TC6_INT_MASK0_HEADER_ERR_MASK);
@@ -810,6 +871,15 @@ 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. PHYINT is level
+ * triggered, so doing this synchronously here (in the sleepable
+ * threaded IRQ) clears the source before the next data chunk.
+ */
+ if ((tc6->quirk_flags & OA_TC6_PHY_INT) &&
+ FIELD_GET(OA_TC6_STATUS0_PHY_INT, value))
+ handle_nested_irq(tc6->phy_virq);
+
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",
@@ -1518,7 +1588,7 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev,
return NULL;
}
- ret = oa_tc6_unmask_macphy_error_interrupts(tc6);
+ ret = oa_tc6_unmask_interrupts(tc6);
if (ret) {
dev_err(&tc6->spi->dev,
"MAC-PHY error interrupts unmask failed: %d\n", ret);
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
* [PATCH net-next v2 3/4] microchip: lan865x: enable PHY interrupt via virtual IRQ
2026-09-07 5:20 [PATCH net-next v2 0/4] net: microchip_t1s: fix collision detection on PLCA status change Parthiban Veerasooran
2026-09-07 5:20 ` [PATCH net-next v2 1/4] net: phy: " Parthiban Veerasooran
2026-09-07 5:20 ` [PATCH net-next v2 2/4] net: ethernet: oa_tc6: deliver the PHY interrupt to phylib Parthiban Veerasooran
@ 2026-09-07 5:20 ` Parthiban Veerasooran
2026-09-10 5:21 ` netdev-bot+sashiko
2026-09-07 5:20 ` [PATCH net-next v2 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-07 5:20 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 in the OA TC6 framework. This allows
phylib to operate in interrupt mode and drives the PHY driver's
config_intr/handle_interrupt callbacks for LAN865X internal PHY.
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
* [PATCH net-next v2 4/4] net: phy: microchip_t1s: fix collision detection for LAN867X Rev.D0
2026-09-07 5:20 [PATCH net-next v2 0/4] net: microchip_t1s: fix collision detection on PLCA status change Parthiban Veerasooran
` (2 preceding siblings ...)
2026-09-07 5:20 ` [PATCH net-next v2 3/4] microchip: lan865x: enable PHY interrupt via virtual IRQ Parthiban Veerasooran
@ 2026-09-07 5:20 ` Parthiban Veerasooran
2026-09-10 5:21 ` netdev-bot+sashiko
3 siblings, 1 reply; 9+ messages in thread
From: Parthiban Veerasooran @ 2026-09-07 5:20 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, pabeni, hkallweit1, linux
Cc: netdev, linux-kernel, UNGLinuxDriver, Parthiban.Veerasooran,
Parthiban Veerasooran
LAN867X Rev.D0 introduces a Collision Counting and MAC Forwarding Control
field (CCMFC, bits 10:9) in the Collision Detector Control 0 register
(CDCTL0, 0x0087). When configured to the OA default value (0x1), the
hardware automatically 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 and forwarded when PLCA_Status is not
OK.
This eliminates the inherent delay between a PLCA status change and the
software interrupt handler toggling CDEN, which was a limitation on older
revisions that had no hardware alternative. Since CCMFC handles collision
gating autonomously, the PSTC interrupt handler for Rev.D0 only needs to
update the link status selection on each PLCA status transition.
Configure CCMFC to the OA default in lan867x_revd0_config_init(). Add
lan867x_revd0_handle_interrupt() to handle two separate events:
1. Link Status Change (LNKSTSC): Triggers the phylib state machine via
phy_trigger_machine() to re-evaluate link status and perform necessary
state transitions.
2. PLCA Status Change (PSTC): Reads the current PLCA operational status
via genphy_c45_plca_get_status() and calls
lan867x_revd0_link_active_selection() to update the link status
selection accordingly.
Unmask both link status change and PLCA status change interrupt masks for
Rev.D0 in lan86xx_config_intr(). Wire up .config_intr and .handle_interrupt
for Rev.D0 using the shared lan86xx_config_intr() and the new handler.
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 | 68 +++++++++++++++++++++++++++++++--
1 file changed, 65 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index afb7e52594e7..c3a738c7425b 100644
--- a/drivers/net/phy/microchip_t1s.c
+++ b/drivers/net/phy/microchip_t1s.c
@@ -33,6 +33,7 @@
#define LAN86XX_REG_STS1 0x0018
#define LAN86XX_REG_IMSK1 0x001C
+#define LAN86XX_STS1_LINK_STS_CHANGED BIT(13)
#define LAN86XX_STS1_PLCA_STS_CHANGED BIT(11)
/* Collision Detector Control 0 Register */
@@ -40,6 +41,9 @@
#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)
/* LAN8670/1/2 Rev.D0 Link Status Selection Register */
#define LAN867X_REG_LINK_STATUS_CTRL 0x0012
@@ -502,6 +506,18 @@ static int lan867x_revd0_config_init(struct phy_device *phydev)
return ret;
}
+ /* AN1760: configure CCMFC to OA default so that the hardware
+ * automatically gates collision forwarding to the MAC based on
+ * PLCA_Status. Collisions are neither counted nor forwarded when
+ * PLCA_Status = OK, eliminating the need for software-driven CDEN
+ * toggling in the interrupt handler. CDEN remains enabled.
+ */
+ ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_COL_DET_CTRL0,
+ COL_DET_CTRL0_CCMFC_MASK,
+ COL_DET_CTRL0_CCMFC_OA_DEFAULT);
+ 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.
@@ -526,8 +542,12 @@ static int lan86xx_read_status(struct phy_device *phydev)
static int lan86xx_config_intr(struct phy_device *phydev)
{
+ u16 mask = LAN86XX_STS1_PLCA_STS_CHANGED;
int ret;
+ if (phydev->phy_id == PHY_ID_LAN867X_REVD0)
+ mask |= LAN86XX_STS1_LINK_STS_CHANGED;
+
if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
/* Read to clear any pending status before enabling. */
ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_STS1);
@@ -536,12 +556,11 @@ static int lan86xx_config_intr(struct phy_device *phydev)
/* A mask bit of 0 enables the corresponding interrupt. */
return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
- LAN86XX_REG_IMSK1,
- LAN86XX_STS1_PLCA_STS_CHANGED);
+ LAN86XX_REG_IMSK1, mask);
}
ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IMSK1,
- LAN86XX_STS1_PLCA_STS_CHANGED);
+ mask);
if (ret)
return ret;
@@ -595,6 +614,47 @@ static irqreturn_t lan86xx_handle_interrupt(struct phy_device *phydev)
return ret_irq;
}
+static irqreturn_t lan867x_revd0_handle_interrupt(struct phy_device *phydev)
+{
+ struct phy_plca_status plca_st;
+ irqreturn_t ret_irq = IRQ_NONE;
+ int sts1, ret;
+
+ /* Reading the status register clears the latched event bits. */
+ sts1 = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_STS1);
+ if (sts1 < 0) {
+ phy_error(phydev);
+ return IRQ_NONE;
+ }
+
+ if (sts1 & LAN86XX_STS1_LINK_STS_CHANGED) {
+ phy_trigger_machine(phydev);
+ ret_irq = IRQ_HANDLED;
+ }
+
+ 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;
+ }
+
+ /* Collision detection is handled autonomously by the hardware
+ * via CCMFC. Only the link status selection needs to be updated
+ * on each PLCA status transition.
+ */
+ ret = lan867x_revd0_link_active_selection(phydev, plca_st.pst);
+ 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),
@@ -637,6 +697,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 = lan86xx_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 v2 1/4] net: phy: microchip_t1s: fix collision detection on PLCA status change
2026-09-07 5:20 ` [PATCH net-next v2 1/4] net: phy: " Parthiban Veerasooran
@ 2026-09-10 5:21 ` netdev-bot+sashiko
0 siblings, 0 replies; 9+ messages in thread
From: netdev-bot+sashiko @ 2026-09-10 5:21 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: 2 · Low: 2
- [High] lan86xx_config_intr() (drivers/net/phy/microchip_t1s.c) read-
clears the latched status register LAN86XX_REG_STS1 *before* unmasking…
- [Medium] lan86xx_plca_set_cfg() uses `if (phydev->irq != PHY_POLL)
return 0;` as the predicate for 'the interrupt handler will keep CDEN…
- [Medium] In the PHY_POLL branch retained by this patch,
lan86xx_plca_set_cfg() tests `if (plca_cfg->enabled)` on a tri-state…
- [Low] Reported concern (validated as RESOLVED later in this series,
retained for the consolidation stage): 'For the LAN865X Rev.B0/B1…
- [Low] Reported concern (validated as RESOLVED later in this series,
retained for the consolidation stage): 'LAN867X Rev.D0 uses…
--
net: phy: microchip_t1s: fix collision detection on PLCA status change
This adds lan86xx_config_intr() and lan86xx_handle_interrupt() to the
microchip_t1s driver so that the PLCA Status Changed (PSTC) interrupt drives
the collision detector enable bit in COL_DET_CTRL0. The static CDEN write in
lan86xx_plca_set_cfg() is now only performed when phydev->irq is PHY_POLL.
The new handlers are wired to LAN867X Rev.B1/C1/C2 and the LAN865X Rev.B0/B1
internal PHY.
A few questions on the changelog and on the new code below.
> Wire these handlers to all supported PHY variants: LAN867X Rev.B1, C1,
> C2 and LAN865X Rev.B0/B1.
[Severity: Low]
LAN867X Rev.D0 also uses lan86xx_plca_set_cfg() as its .set_plca_cfg, but the
Rev.D0 entry does not get .config_intr/.handle_interrupt here. Is "all
supported PHY variants" accurate for this patch?
Rev.D0 is covered by a later patch in the series ("net: phy: microchip_t1s:
fix collision detection for LAN867X Rev.D0"), which wires
lan867x_revd0_handle_interrupt() and programs CCMFC in
lan867x_revd0_config_init() so that collision forwarding is gated by
PLCA_Status in hardware. Rev.D0 keeps its pre-patch behaviour at this commit,
so this is only about the changelog wording for the intermediate state.
> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
> index 73c23d311d72a..afb7e52594e71 100644
> --- a/drivers/net/phy/microchip_t1s.c
> +++ b/drivers/net/phy/microchip_t1s.c
[ ... ]
> @@ -458,6 +466,16 @@ static int lan86xx_plca_set_cfg(struct phy_device *phydev,
> if (ret)
> return ret;
>
> + /* PHYs with routed interrupts handle CDEN dynamically via the interrupt
> + * handler, so skip the static write. PHYs running with PHY_POLL have no
> + * interrupt handler, so apply the static CDEN write as a baseline on
> + * every ethtool PLCA reconfiguration. The limitation is that autonomous
> + * PLCA mode transitions between ethtool reconfigurations are not
> + * tracked on such boards.
> + */
> + if (phydev->irq != PHY_POLL)
> + return 0;
> +
[Severity: Medium]
Is phydev->irq != PHY_POLL the right predicate for "the interrupt handler
will keep CDEN up to date"? phylib has a second non-interrupt sentinel:
include/linux/phy.h:
static inline bool phy_interrupt_is_valid(struct phy_device *phydev)
{
return phydev->irq != PHY_POLL && phydev->irq != PHY_MAC_INTERRUPT;
}
For phydev->irq == PHY_MAC_INTERRUPT phylib never calls
phy_request_interrupt(), so lan86xx_config_intr() is never called with
PHY_INTERRUPT_ENABLED and lan86xx_handle_interrupt() is never dispatched.
phy_mac_interrupt() only calls phy_trigger_machine(). phy_probe() also only
downgrades irq to PHY_POLL when phy_interrupt_is_valid() is true, so the
PHY_MAC_INTERRUPT value survives.
MAC drivers do set this sentinel on whatever PHY is attached, for example
tsnep_phy_open() in drivers/net/ethernet/engleder/tsnep_main.c does:
adapter->phydev->irq = PHY_MAC_INTERRUPT;
On such a board, does anything write COL_DET_CTRL0 at all after this change,
given that the static write is skipped and the handler never runs? Would
phy_interrupt_is_valid(phydev), or a test on phydev->interrupts, express the
intent more accurately here?
> if (plca_cfg->enabled)
> return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> LAN86XX_REG_COL_DET_CTRL0,
[Severity: Medium]
This truthiness test isn't introduced by this patch, but the patch keeps this
path as the documented baseline for PHY_POLL boards, so it seems worth
raising: plca_cfg->enabled is tri-state, not boolean.
ethnl_set_plca() in net/ethtool/plca.c does memset(&plca_cfg, 0xff,
sizeof(plca_cfg)), and plca_update_sint() returns early when the netlink
attribute is absent, so an omitted enable attribute reaches the driver as -1.
genphy_c45_plca_set_cfg() only clears PLCA_EN for enabled == 0 and only sets
it for enabled > 0, so -1 leaves the PLCA mode untouched.
For a request such as:
ethtool --set-plca-cfg <dev> node-id 5
on a PHY_POLL board, enabled is -1, which is truthy, so COL_DET_DISABLE is
written even though PLCA was left off. Does that not leave collision
detection disabled while the PHY keeps running CSMA/CD, with no interrupt
handler to correct it? Should the test be plca_cfg->enabled > 0?
> @@ -506,6 +524,77 @@ static int lan86xx_read_status(struct phy_device *phydev)
> return 0;
> }
>
> +static int lan86xx_config_intr(struct phy_device *phydev)
> +{
> + int ret;
> +
> + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> + /* Read to clear any pending status before enabling. */
> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_STS1);
> + if (ret < 0)
> + return ret;
> +
> + /* A mask bit of 0 enables the corresponding interrupt. */
> + return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
> + LAN86XX_REG_IMSK1,
> + LAN86XX_STS1_PLCA_STS_CHANGED);
> + }
[Severity: High]
Can a PLCA status change be lost across the masked window here? The enable
path reads STS1, which clears the latched PSTC event, and only then unmasks
PSTCM. Nothing in the patch programs COL_DET_CTRL0 from the current
PLCA_STATUS value, so CDEN on an interrupt-capable PHY is driven purely by
PSTC edges once lan86xx_plca_set_cfg() returns early.
Sequence for LAN867X Rev.C1/C2 with a routed IRQ, on any MAC that connects
the PHY at ndo_open:
ifup phy_request_interrupt() -> lan86xx_config_intr(enable)
ethtool lan86xx_plca_set_cfg() sets PLCA_EN and returns early without
touching CDEN; beacon present, PST 0->1 -> PSTC ->
lan86xx_handle_interrupt() writes CDEN=0
ifdown phy_disconnect() -> phy_free_interrupt() ->
phy_disable_interrupts() -> lan86xx_config_intr(disable)
masks PSTCM and read-clears STS1; PLCA_EN stays set in hardware
since the driver has no .suspend
coordinator stops beaconing, PST 1->0 latches PSTC under the mask
ifup phy_init_hw() -> lan867x_revc_config_init(), which does not write
0x0087, so CDEN stays 0; then lan86xx_config_intr(enable) reads
STS1 and discards the pending PSTC before unmasking
Does the bus then run CSMA/CD with the collision detector disabled until some
future beacon both appears and disappears again? That is the state the
changelog describes as the bug being fixed, and since lan86xx_plca_set_cfg()
no longer writes CDEN on such boards, is there any way for an administrator
to repair it by re-running ethtool?
lan865x_revb_config_init() also never writes 0x0087, so the same applies to
the LAN865X internal PHY.
Would sampling genphy_c45_plca_get_status() and programming CDEN from it when
interrupts are enabled, and in set_plca_cfg regardless of irq mode, close the
window rather than relying on edges only?
> +
> + ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IMSK1,
> + LAN86XX_STS1_PLCA_STS_CHANGED);
> + if (ret)
> + return ret;
> +
> + /* Read to clear any pending status after disabling. */
> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_STS1);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static irqreturn_t lan86xx_handle_interrupt(struct phy_device *phydev)
> +{
> + struct phy_plca_status plca_st;
> + irqreturn_t ret_irq = IRQ_NONE;
> + int sts1, ret;
> +
> + /* Reading the status register clears the latched event bits. */
> + sts1 = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_STS1);
> + if (sts1 < 0) {
> + phy_error(phydev);
> + return IRQ_NONE;
> + }
> +
> + if (sts1 & LAN86XX_STS1_PLCA_STS_CHANGED) {
> + ret = genphy_c45_plca_get_status(phydev, &plca_st);
> + if (ret < 0) {
> + phy_error(phydev);
> + return IRQ_NONE;
> + }
> +
> + /* AN1760/AN1699: disable collision detection in PLCA mode to
> + * improve signal quality; re-enable it in CSMA/CD mode.
> + *
> + * https://www.microchip.com/en-us/application-notes/an1760
> + * https://www.microchip.com/en-us/application-notes/an1699
> + */
> + ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> + LAN86XX_REG_COL_DET_CTRL0,
> + COL_DET_CTRL0_ENABLE_BIT_MASK,
> + plca_st.pst ? COL_DET_DISABLE :
> + COL_DET_ENABLE);
> + if (ret < 0) {
> + phy_error(phydev);
> + return IRQ_NONE;
> + }
> +
> + ret_irq = IRQ_HANDLED;
> + }
> +
> + return ret_irq;
> +}
> +
[ ... ]
> @@ -556,6 +651,8 @@ static struct phy_driver microchip_t1s_driver[] = {
> .features = PHY_BASIC_T1S_P2MP_FEATURES,
> .config_init = lan865x_revb_config_init,
> .read_status = lan86xx_read_status,
> + .config_intr = lan86xx_config_intr,
> + .handle_interrupt = lan86xx_handle_interrupt,
[Severity: Low]
At this commit oa_tc6 leaves phydev->irq as PHY_POLL for the LAN865X internal
PHY, so the changelog sentence "LAN865X is excluded from the static write
because its interrupt is always routed via the MAC-PHY SPI driver and the
interrupt handler always runs" does not hold yet.
The gap is closed inside the same series: "net: ethernet: oa_tc6: deliver the
PHY interrupt to phylib" adds an irq_domain, populates every mii_bus->irq[]
entry with the virtual IRQ before mdiobus_register(), and dispatches
handle_nested_irq(tc6->phy_virq) from the threaded IRQ when PHYINT is seen;
"microchip: lan865x: enable PHY interrupt via virtual IRQ" enables that path
for LAN8650/1. Behaviour of LAN865X at this commit alone is unchanged,
because irq == PHY_POLL keeps the static CDEN write.
Could the changelog be reworded so that it does not assert a property that
only becomes true two patches later, in case this patch is backported on its
own?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907052007.391972-1-parthiban.veerasooran%40microchip.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v2 2/4] net: ethernet: oa_tc6: deliver the PHY interrupt to phylib
2026-09-07 5:20 ` [PATCH net-next v2 2/4] net: ethernet: oa_tc6: deliver the PHY interrupt to phylib Parthiban Veerasooran
@ 2026-09-10 5:21 ` netdev-bot+sashiko
0 siblings, 0 replies; 9+ messages in thread
From: netdev-bot+sashiko @ 2026-09-10 5:21 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 4 potential
issue(s) to consider:
Critical: 0 · High: 2 · Medium: 0 · Low: 2
- [High] oa_tc6_unmask_interrupts() clears OA_TC6_INT_MASK0_PHY_INT_MASK
unconditionally for every MAC-PHY, while the only code that can make…
- [High] Use of `dummy_irq_chip` for the nested PHY interrupt creates an
unrecoverable interrupt storm (DoS) condition if the generic IRQ…
- [Low] The new code in oa_tc6_phy_irq_setup()/oa_tc6_phy_irq_teardown()
calls irq_domain_create_linear(), irq_create_mapping() and…
- [Low] Concern (baseline verdict: false positive) that the
OA_TC6_PHY_INT infrastructure has no in-tree producer, making the irq…
--
net: ethernet: oa_tc6: deliver the PHY interrupt to phylib
This adds an OA_TC6_PHY_INT quirk flag so MAC drivers can opt in when the
PHY interrupt is delivered in-band through the MAC-PHY SPI status register.
When the flag is set, a one-entry irq domain with a nested virtual IRQ is
created before mdiobus_register(), all mii_bus->irq[] entries are populated
with that virq, and PHYINT seen in the extended status is dispatched with
handle_nested_irq() from the threaded IRQ.
A few questions below.
> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 6fcc5f561d560..00b0176c1fe7a 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>
[ ... ]
> @@ -528,6 +532,44 @@ int oa_tc6_mdiobus_write_c45(struct mii_bus *bus, int addr, int devnum,
> }
> EXPORT_SYMBOL_GPL(oa_tc6_mdiobus_write_c45);
>
> +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, &dummy_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)
> +{
> + 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);
> + 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)
> +{
> + irq_dispose_mapping(tc6->phy_virq);
> + irq_domain_remove(tc6->phy_irq_domain);
> +}
[Severity: High]
Is dummy_irq_chip the right irqchip for a level-triggered, in-band
source that only stops asserting once the PHY is acked?
dummy_irq_chip (kernel/irq/dummychip.c) implements .irq_mask and
.irq_unmask as no-ops, so nothing the generic IRQ core does to this
virq can ever reach OA_TC6_INT_MASK0_PHY_INT_MASK in the MAC-PHY.
That matters because the core does try to mask this virq on its own:
handle_nested_irq() calls note_interrupt() for each dispatch (unless
irq_settings_no_debug()), and the spurious-IRQ accounting there will
eventually call __report_bad_irq() and disable the interrupt if the
PHY handler keeps returning IRQ_NONE. Note that oa_tc6_phy_irq_map()
sets IRQ_NOPROBE via irq_set_noprobe(), which is not IRQ_NO_DEBUG, so
that accounting does apply here - is that intended?
Once the core has disabled the virq, mask_irq() lands in the dummy
chip and does nothing, but the next PHYINT still reaches
oa_tc6_process_extended_status(), where handle_nested_irq() bails out
early in irq_can_handle_actions() without running phy_interrupt().
The PHY status registers are then never read, the level-triggered
source stays asserted, and with IRQF_TRIGGER_LOW | IRQF_ONESHOT the
SPI IRQ re-fires as soon as oa_tc6_macphy_threaded_irq() returns -
with no path left to re-mask PHYINT in hardware. Same reasoning
applies to any other caller that disables the virq, e.g. the
disable_irq_nosync() in phy_interrupt().
Would a small real irq_chip with .irq_mask / .irq_unmask that clear
and set OA_TC6_INT_MASK0_PHY_INT_MASK be preferable here? Since the
register access is over SPI and sleeps, that presumably needs
.irq_bus_lock / .irq_bus_sync_unlock to defer the actual transfer,
which is the usual pattern for nested SPI/I2C irqchips. If you would
rather keep the dummy chip, how is the disabled-virq case supposed to
be recovered, and should the IRQ_NONE return from handle_nested_irq()
be acted on instead of dropped?
[Severity: Low]
Does oa_tc6 need "select IRQ_DOMAIN" now?
irq_domain_create_linear(), irq_create_mapping() and irq_domain_remove()
are all declared inside the "#ifdef CONFIG_IRQ_DOMAIN" block of
include/linux/irqdomain.h. The !CONFIG_IRQ_DOMAIN branch only provides
stubs for irq_dispose_mapping() and irq_find_matching_fwnode(), so
oa_tc6_phy_irq_setup() and oa_tc6_phy_irq_teardown() would hit
implicit-function-declaration errors there.
The OA_TC6 entry in drivers/net/ethernet/Kconfig currently has:
config OA_TC6
tristate "OPEN Alliance TC6 10BASE-T1x MAC-PHY support" if COMPILE_TEST
depends on SPI
select PHYLIB
That allows CONFIG_SPI=y with CONFIG_IRQ_DOMAIN=n (m68k/ColdFire has SPI
and never selects IRQ_DOMAIN). drivers/net/dsa/mv88e6xxx/Kconfig and
drivers/net/dsa/realtek/Kconfig add "select IRQ_DOMAIN" for the same
reason.
> @@ -661,7 +721,7 @@ static int oa_tc6_sw_reset_macphy(struct oa_tc6 *tc6)
> return oa_tc6_write_register(tc6, OA_TC6_REG_STATUS0, regval);
> }
>
> -static int oa_tc6_unmask_macphy_error_interrupts(struct oa_tc6 *tc6)
> +static int oa_tc6_unmask_interrupts(struct oa_tc6 *tc6)
> {
> u32 regval;
> int ret;
> @@ -670,7 +730,8 @@ static int oa_tc6_unmask_macphy_error_interrupts(struct oa_tc6 *tc6)
> if (ret)
> return ret;
>
> - regval &= ~(OA_TC6_INT_MASK0_TX_PROTOCOL_ERR_MASK |
> + regval &= ~(OA_TC6_INT_MASK0_PHY_INT_MASK |
> + OA_TC6_INT_MASK0_TX_PROTOCOL_ERR_MASK |
> OA_TC6_INT_MASK0_RX_BUFFER_OVERFLOW_ERR_MASK |
> OA_TC6_INT_MASK0_LOSS_OF_FRAME_ERR_MASK |
> OA_TC6_INT_MASK0_HEADER_ERR_MASK);
[Severity: High]
Should the PHYINT unmask be gated on OA_TC6_PHY_INT the same way the
service path is?
oa_tc6_unmask_interrupts() clears OA_TC6_INT_MASK0_PHY_INT_MASK for every
MAC-PHY, but the only code that can make the level-triggered source drop is
quirk-gated in oa_tc6_process_extended_status():
if ((tc6->quirk_flags & OA_TC6_PHY_INT) &&
FIELD_GET(OA_TC6_STATUS0_PHY_INT, value))
handle_nested_irq(tc6->phy_virq);
As the commit message says, PHYINT is level triggered and only drops when
the PHY source is acked, so the STATUS0 write-back just above does not
clear it.
For a driver that does not set the new flag, is there anything that can
clear PHYINT once the PHY asserts it? adin1140_probe() sets only:
tc6_quirks.quirk_flags = OA_TC6_BROKEN_PHY;
and its PHY is polled (devm_mdiobus_alloc() leaves mii_bus->irq[] at
PHY_POLL), so PHYINT is now armed in hardware with no ack path.
Even on a device that does set OA_TC6_PHY_INT, can the dispatch return
without acking? drivers/net/phy/phy.c:phy_interrupt() has:
if (IS_ENABLED(CONFIG_PM_SLEEP) && phydev->irq_suspended) {
...
phydev->irq_rerun = 1;
disable_irq_nosync(irq);
return IRQ_HANDLED;
}
After that the virq is disabled, so handle_nested_irq() becomes a no-op
because irq_can_handle_actions() is false. resume_device_irqs() re-enables
the SPI IRQ in the noirq phase while mdio_bus_phy_resume() clears
irq_suspended only later in the device-resume phase, which looks like a
window where the PHY source stays asserted with the SPI IRQ live.
With IRQF_TRIGGER_LOW | IRQF_ONESHOT, wouldn't the still-asserted level
re-trigger oa_tc6_macphy_isr() as soon as oa_tc6_macphy_threaded_irq()
returns, spinning on empty data chunks plus two extra STATUS0 control
transactions per chunk footer? The result of handle_nested_irq() is not
used, PHYINT is never re-masked, and oa_tc6_disable_traffic() is only
reached on SPI transfer errors.
> @@ -810,6 +871,15 @@ 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. PHYINT is level
> + * triggered, so doing this synchronously here (in the sleepable
> + * threaded IRQ) clears the source before the next data chunk.
> + */
> + if ((tc6->quirk_flags & OA_TC6_PHY_INT) &&
> + FIELD_GET(OA_TC6_STATUS0_PHY_INT, value))
> + handle_nested_irq(tc6->phy_virq);
> +
[ ... ]
> diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h
> index 27f652d4920b2..1c8cc263c5e53 100644
> --- a/include/linux/oa_tc6.h
> +++ b/include/linux/oa_tc6.h
> @@ -68,6 +70,7 @@ struct oa_tc6;
>
> enum oa_tc6_quirk_flag {
> OA_TC6_BROKEN_PHY = BIT(0),
> + OA_TC6_PHY_INT = BIT(1),
> };
[Severity: Low]
This isn't a bug, just a note for anyone reading this commit alone: at this
point in the series no in-tree caller passes OA_TC6_PHY_INT, so the irq
domain, the virq, the mii_bus->irq[] population and the handle_nested_irq()
dispatch are all unreachable here.
The following patch in the series, "microchip: lan865x: enable PHY
interrupt via virtual IRQ", adds the producer in lan865x_probe():
tc6_quirks.quirk_flags = OA_TC6_PHY_INT;
priv->tc6 = oa_tc6_init(spi, netdev, &tc6_quirks);
so this resolves within the series and no change is needed.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907052007.391972-1-parthiban.veerasooran%40microchip.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v2 3/4] microchip: lan865x: enable PHY interrupt via virtual IRQ
2026-09-07 5:20 ` [PATCH net-next v2 3/4] microchip: lan865x: enable PHY interrupt via virtual IRQ Parthiban Veerasooran
@ 2026-09-10 5:21 ` netdev-bot+sashiko
0 siblings, 0 replies; 9+ messages in thread
From: netdev-bot+sashiko @ 2026-09-10 5:21 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: 2 (1) · Medium: 4 · Low: 0
- [High] Asymmetric interrupt enable/acknowledge in the OA TC6 framework:
oa_tc6_unmask_interrupts() (drivers/net/ethernet/oa_tc6.c, formerly…
- [High] Interrupt livelock when the nested PHY handler cannot
acknowledge the source.
- [Medium] Undisclosed behavioural change: by passing OA_TC6_PHY_INT,
lan865x_probe() gives the LAN865X internal PHY a valid phydev->irq…
- [Medium] This patch is a mandatory part of the collision-detection bug
fix for LAN865X, but it carries neither a Fixes: tag nor any statement…
- [Medium] lan86xx_plca_set_cfg() uses `if (phydev->irq != PHY_POLL)
return 0;` to decide that 'PHYs with routed interrupts handle CDEN…
- [Medium] Enabling OA_TC6_PHY_INT makes the LAN865x SPI data engine
dispatch the PHY interrupt synchronously from inside the RX-chunk-…
Pre-existing issues:
- [High] lan865x_remove() calls cancel_work_sync(&priv->multicast_work)
*before* unregister_netdev().
--
microchip: lan865x: enable PHY interrupt via virtual IRQ
This passes the new OA_TC6_PHY_INT quirk flag to oa_tc6_init() so that the
LAN865X internal PHY gets a virtual IRQ from the OA TC6 framework and phylib
can run the PHY driver's config_intr/handle_interrupt callbacks instead of
polling.
A few questions about the consequences of turning this on, mostly about the
interaction with the two preceding patches in the series.
> diff --git a/drivers/net/ethernet/microchip/lan865x/lan865x.c b/drivers/net/ethernet/microchip/lan865x/lan865x.c
> index 26a2761332a5a..043a3dc3f9657 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);
[Severity: Medium]
Does setting OA_TC6_PHY_INT here silently remove the deterministic
COL_DET_CTRL0 write for LAN865x?
With this quirk, oa_tc6_mdiobus_register() fills every mii_bus->irq[] entry
with tc6->phy_virq, so phydev->irq for the internal PHY is no longer
PHY_POLL, and lan86xx_plca_set_cfg() now takes the new early return added by
the preceding PHY patch:
drivers/net/phy/microchip_t1s.c:lan86xx_plca_set_cfg() {
...
if (phydev->irq != PHY_POLL)
return 0;
...
}
After that, the only writer of CDEN for this device is
lan86xx_handle_interrupt(), and only when the read-to-clear STS1 read
observes LAN86XX_STS1_PLCA_STS_CHANGED:
if (sts1 & LAN86XX_STS1_PLCA_STS_CHANGED) {
ret = genphy_c45_plca_get_status(phydev, &plca_st);
Two things follow from that. First, an administrative
"ethtool --set-plca-cfg ... enable off" performed while the PLCA status is
unchanged produces no PST edge, so nothing reprograms CDEN. Second,
lan86xx_config_intr() reads STS1 to clear pending status before unmasking
PSTCM, so a transition latched before that point is consumed without ever
being applied to CDEN, and it never evaluates the live PLCA status:
/* Read to clear any pending status before enabling. */
ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_STS1);
Is there anything that establishes the CDEN baseline for this part?
lan865x_revb_fixup_registers[] does not contain 0x0087, unlike the LAN867X
Rev.B1 AN1699 list which writes 0x0087 with mask 0x801C. If the correctness
now relies on the reset default of COL_DET_CTRL0 bit 15 and on the PHY
latching PSTC for every relevant change, could the changelog mention that,
since it currently reads only as "enable interrupt mode"?
[Severity: Medium]
Should this patch carry the same Fixes: tag as the PHY change, or at least
state the dependency?
The only commit in the series with a Fixes: tag is "net: phy: microchip_t1s:
fix collision detection on PLCA status change" (135306e057ab, Fixes:
78341049fbcd), and its changelog says:
LAN865X is excluded from the static write because its interrupt is
always routed via the MAC-PHY SPI driver and the interrupt handler
always runs.
Is that statement true before this patch is applied? Without OA_TC6_PHY_INT
the framework never populates mii_bus->irq[], mdiobus_alloc_size()
initialises every entry to PHY_POLL, phy_request_interrupt() is never called,
and lan86xx_handle_interrupt() never runs.
A stable selection that follows the Fixes: tag would then take the PHY commit
alone and leave LAN865x with neither the static write nor the interrupt path.
[Severity: High]
While looking at the framework side this patch opts into: is the PHYINT
unmask in the preceding oa_tc6 patch missing the OA_TC6_PHY_INT gate?
oa_tc6_unmask_interrupts() clears the mask for every MAC-PHY, unconditionally
from oa_tc6_init():
drivers/net/ethernet/oa_tc6.c:oa_tc6_unmask_interrupts() {
regval &= ~(OA_TC6_INT_MASK0_PHY_INT_MASK |
OA_TC6_INT_MASK0_TX_PROTOCOL_ERR_MASK |
...
}
but the only code that services and acknowledges it is quirk-gated:
drivers/net/ethernet/oa_tc6.c:oa_tc6_process_extended_status() {
...
if ((tc6->quirk_flags & OA_TC6_PHY_INT) &&
FIELD_GET(OA_TC6_STATUS0_PHY_INT, value))
handle_nested_irq(tc6->phy_virq);
...
}
drivers/net/ethernet/adi/adin1140.c:adin1140_probe() passes only
OA_TC6_BROKEN_PHY, so on that device PHYINT is armed while tc6->phy_virq is 0
and no path can read or ack the PHY's interrupt status. Since STATUS0.PHYINT
is a level reflection (the new comment says the STATUS0 write-back does not
clear it) and the host IRQ is requested as level-low with ONESHOT:
ret = devm_request_threaded_irq(&tc6->spi->dev, tc6->spi->irq,
oa_tc6_macphy_isr,
oa_tc6_macphy_threaded_irq,
IRQF_TRIGGER_LOW | IRQF_ONESHOT,
wouldn't an asserted PHYINT on a non-opt-in device loop forever? The hard ISR
sets tc6->int_flag, oa_tc6_macphy_threaded_irq() spins in
while (tc6->int_flag || ...), oa_tc6_try_spi_transfer() clears int_flag, the
thread returns and ONESHOT unmasks a still-low line. Should the unmask be
gated on OA_TC6_PHY_INT too?
[Severity: Medium]
Related to the branch this patch activates: should lan86xx_plca_set_cfg() use
phy_interrupt_is_valid() rather than comparing against PHY_POLL only?
include/linux/phy.h:phy_interrupt_is_valid() {
return phydev->irq != PHY_POLL && phydev->irq != PHY_MAC_INTERRUPT;
}
For PHY_MAC_INTERRUPT phylib never calls phy_request_interrupt(), so
.config_intr/.handle_interrupt are never invoked. A LAN867x on a bus that
assigns PHY_MAC_INTERRUPT, for example:
drivers/net/mdio/mdio-moxart.c:moxart_mdio_probe() {
for (i = 0; i < PHY_MAX_ADDR; i++)
bus->irq[i] = PHY_MAC_INTERRUPT;
}
would then skip the static COL_DET_CTRL0 write and never get the handler, so
CDEN is never programmed at all. The comment above the new early return says
"PHYs with routed interrupts handle CDEN dynamically via the interrupt
handler", which does not hold for that phylib mode.
[Severity: High]
Can the nested dispatch enabled here livelock when phylib has the virq
disabled?
handle_nested_irq() only sets IRQS_PENDING when !action or
irqd_irq_disabled(&desc->irq_data), and phylib disables this virq on its own
during system suspend:
drivers/net/phy/phy_device.c:mdio_bus_phy_suspend() {
if (phy_interrupt_is_valid(phydev)) {
phydev->irq_suspended = 1;
synchronize_irq(phydev->irq);
}
}
drivers/net/phy/phy.c:phy_interrupt() {
if (IS_ENABLED(CONFIG_PM_SLEEP) && phydev->irq_suspended) {
...
phydev->irq_rerun = 1;
disable_irq_nosync(irq);
return IRQ_HANDLED;
}
}
Neither oa_tc6 nor lan865x sets mac_managed_pm, so this branch is reachable
for this phydev. The PHY is never read, its latched status is not cleared,
and with dummy_irq_chip the mask is a no-op, so STATUS0.PHYINT stays
asserted. The level-low ONESHOT MAC IRQ then re-fires after each return of:
drivers/net/ethernet/oa_tc6.c:oa_tc6_macphy_threaded_irq() {
while (tc6->int_flag ||
(tc6->waiting_tx_skb && tc6->tx_credits)) {
ret = oa_tc6_try_spi_transfer(tc6);
...
}
Is there anything that bounds this, or that masks
OA_TC6_INT_MASK0_PHY_INT_MASK as a fallback when the nested handler cannot
ack? The same no-ack outcome seems possible for any PHYINT source not
represented in STS1, since lan86xx_handle_interrupt() reads only STS1 and
otherwise returns IRQ_NONE, which also feeds note_interrupt() on the nested
virq. If an SPI transfer fails during such a storm, oa_tc6_disable_traffic()
is reached and the interface stays dead.
[Severity: High]
This isn't a bug introduced by this patch, but while the teardown path is in
scope: 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 is still installed and callable in that window, and
lan865x_remove() does not hold rtnl:
static void lan865x_set_multicast_list(struct net_device *netdev)
{
struct lan865x_priv *priv = netdev_priv(netdev);
schedule_work(&priv->multicast_work);
}
Nothing cancels or flushes the work again, so can the handler run after
oa_tc6_exit() and free_netdev(), dereferencing freed netdev_priv memory?
if (priv->netdev->flags & IFF_PROMISC) {
...
ret = oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_NET_CFG, regval);
An "ip link set eth0 promisc on" or a multicast join reaching
dev_mc_add() -> __dev_set_rx_mode() during unbind would be enough to re-arm
it. Would unregister_netdev() first, then cancel_work_sync(), close this?
This patch also widens what the stale work can touch, since oa_tc6_exit() now
performs irq_dispose_mapping()/irq_domain_remove() as well.
[Severity: Medium]
Does dispatching the PHY interrupt from inside the chunk-processing loop
couple the data path to phydev->lock?
The call chain enabled by this quirk is
oa_tc6_macphy_threaded_irq() -> oa_tc6_try_spi_transfer() ->
oa_tc6_process_spi_data_rx_buf() -> oa_tc6_process_rx_chunk_footer() ->
oa_tc6_process_extended_status() -> handle_nested_irq(tc6->phy_virq), and
handle_nested_irq() calls the thread_fn directly:
drivers/net/phy/phy.c:phy_interrupt() {
...
mutex_lock(&phydev->lock);
ret = phydev->drv->handle_interrupt(phydev);
...
}
lan86xx_handle_interrupt() then issues several phy_read_mmd()/phy_modify_mmd()
calls, each taking the MDIO bus mutex and tc6->spi_ctrl_lock and doing a
synchronous SPI control transfer. So the single thread that performs all
TX/RX chunk transfers, running at RT priority with the MAC-PHY line masked by
IRQF_ONESHOT, now blocks on a sleeping mutex held by unrelated,
user-triggerable phylib contexts (ethtool PLCA get/set,
phy_do_ioctl_running() -> phy_start_aneg(), phy_start/phy_stop, phy_init_hw()
on resume, the state machine work).
while (tc6->int_flag ||
(tc6->waiting_tx_skb && tc6->tx_credits)) {
Mutexes give no priority inheritance, so a normal-priority holder stalls the
pump while RX chunks accumulate in the MAC-PHY RX buffer, which shows up as
OA_TC6_STATUS0_RX_BUFFER_OVERFLOW_ERROR and lost frames. Would deferring the
phylib dispatch out of the chunk-processing loop, for example after the
transfer loop or from a dedicated worker, keep the data path independent of
phydev->lock?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907052007.391972-1-parthiban.veerasooran%40microchip.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v2 4/4] net: phy: microchip_t1s: fix collision detection for LAN867X Rev.D0
2026-09-07 5:20 ` [PATCH net-next v2 4/4] net: phy: microchip_t1s: fix collision detection for LAN867X Rev.D0 Parthiban Veerasooran
@ 2026-09-10 5:21 ` netdev-bot+sashiko
0 siblings, 0 replies; 9+ messages in thread
From: netdev-bot+sashiko @ 2026-09-10 5:21 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: 3 · Low: 1
- [High] The new CCMFC-based collision gating for LAN867X Rev.D0 does not
own the CDCTL0.CDEN master enable it depends on.
- [Medium] lan867x_revd0_handle_interrupt()
(drivers/net/phy/microchip_t1s.c:646) passes the live PLCA operational…
- [Medium] lan86xx_plca_set_cfg()
(drivers/net/phy/microchip_t1s.c:456-491) treats the documented "don't…
- [Medium] For Rev.D0 the link-status-selection register
(LAN867X_REG_LINK_STATUS_CTRL, 0x0012) is now kept in sync with the…
- [Low] The comment justifying the new CCMFC programming in
lan867x_revd0_config_init() (drivers/net/phy/microchip_t1s.c:509)…
--
net: phy: microchip_t1s: fix collision detection for LAN867X Rev.D0
This programs the new CCMFC field of CDCTL0 on LAN867X Rev.D0 so the
hardware gates collision forwarding from PLCA_Status, adds
lan867x_revd0_handle_interrupt() for the LNKSTSC and PSTC events, and
unmasks both events for Rev.D0 in lan86xx_config_intr().
A few questions on the CDEN ownership, the value passed to
lan867x_revd0_link_active_selection(), and the event masking follow inline.
> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
> index afb7e52594e71..c3a738c7425bf 100644
> --- a/drivers/net/phy/microchip_t1s.c
> +++ b/drivers/net/phy/microchip_t1s.c
[ ... ]
> @@ -40,6 +41,9 @@
> #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)
[ ... ]
> @@ -502,6 +506,18 @@ static int lan867x_revd0_config_init(struct phy_device *phydev)
> return ret;
> }
>
> + /* AN1760: configure CCMFC to OA default so that the hardware
> + * automatically gates collision forwarding to the MAC based on
> + * PLCA_Status. Collisions are neither counted nor forwarded when
> + * PLCA_Status = OK, eliminating the need for software-driven CDEN
> + * toggling in the interrupt handler. CDEN remains enabled.
> + */
> + ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_COL_DET_CTRL0,
> + COL_DET_CTRL0_CCMFC_MASK,
> + COL_DET_CTRL0_CCMFC_OA_DEFAULT);
> + if (ret)
> + return ret;
[Severity: High]
Who owns CDEN for Rev.D0 after this change? This write only touches
GENMASK(10, 9) of CDCTL0, and 0x0087 is not present in
lan867x_revd0_fixup_regs[], so bit 15 is never programmed by
lan867x_revd0_config_init(). The comment states "CDEN remains enabled",
but that is the reset default rather than something the driver
establishes.
The driver can also clear it. The Rev.D0 entry still uses
.set_plca_cfg = lan86xx_plca_set_cfg, and that function only returns
early for interrupt-driven PHYs:
if (phydev->irq != PHY_POLL)
return 0;
if (plca_cfg->enabled)
return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
LAN86XX_REG_COL_DET_CTRL0,
COL_DET_CTRL0_ENABLE_BIT_MASK,
COL_DET_DISABLE);
So on a Rev.D0 board without a routed PHY interrupt (phydev->irq ==
PHY_POLL), an "ethtool --set-plca-cfg <dev> enable 1" turns the collision
detector off entirely. With CDEN cleared, can CCMFC still count or
forward collisions when PLCA_Status goes not-OK?
Is there any path that restores bit 15 afterwards?
lan867x_revd0_config_init() writes only bits 10:9, the Rev.D0 phy_driver
entry has no .soft_reset so phy_init_hw() performs no reset of the vendor
register, and lan867x_revd0_handle_interrupt() deliberately never touches
CDEN.
Would it make sense to program CDEN explicitly next to CCMFC here, and to
exclude Rev.D0 from the static CDEN write in lan86xx_plca_set_cfg()?
[Severity: Low]
Should this comment cite AN1699 rather than AN1760? Elsewhere in this
file AN1760 denotes the LAN8650/1 configuration note (see the comment
above lan865x_revb_fixup_registers[] and the AN1760 reference in
lan865x_revb_indirect_read()), while the Rev.D0 register data in this very
function is attributed to AN1699 Revision G (DS60001699G) above
lan867x_revd0_fixup_regs[].
The changelog also gives no document or revision for the CCMFC field
position (CDCTL0 bits 10:9) or for the "OA default value (0x1)" and the
CDEN reset-default claim, so the field layout cannot be cross-checked
against the reference given here.
> /* 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.
> @@ -526,8 +542,12 @@ static int lan86xx_read_status(struct phy_device *phydev)
>
> static int lan86xx_config_intr(struct phy_device *phydev)
> {
> + u16 mask = LAN86XX_STS1_PLCA_STS_CHANGED;
> int ret;
>
> + if (phydev->phy_id == PHY_ID_LAN867X_REVD0)
> + mask |= LAN86XX_STS1_LINK_STS_CHANGED;
> +
> if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> /* Read to clear any pending status before enabling. */
> ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_STS1);
> @@ -536,12 +556,11 @@ static int lan86xx_config_intr(struct phy_device *phydev)
>
> /* A mask bit of 0 enables the corresponding interrupt. */
> return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
> - LAN86XX_REG_IMSK1,
> - LAN86XX_STS1_PLCA_STS_CHANGED);
> + LAN86XX_REG_IMSK1, mask);
[Severity: Medium]
Can a PLCA status transition be lost here for Rev.D0? The read-to-clear
of LAN86XX_REG_STS1 discards any latched PSTC before unmasking, and
nothing samples the current PLCA status afterwards.
For Rev.D0 the PSTC branch of lan867x_revd0_handle_interrupt() is now the
only writer of LAN867X_REG_LINK_STATUS_CTRL from the live status, so a
dropped edge leaves that register stale until the next transition, which
may never occur on an idle segment.
The phy_init_hw() path looks deterministic:
drivers/net/phy/phy_device.c:phy_init_hw() {
...
if (phydev->drv->config_init) {
ret = phydev->drv->config_init(phydev);
...
if (phydev->drv->config_intr) {
ret = phydev->drv->config_intr(phydev);
...
}
lan867x_revd0_config_init() unconditionally ends with
return lan867x_revd0_link_active_selection(phydev, false);
and the Rev.D0 entry has no .soft_reset, so the PLCA enable bit in MMD 31
survives while config_intr's read-to-clear drops the pending PSTC. Does a
PHY that is PLCA-enabled with PLCA_Status OK then keep the forced-active
selection instead of the plca_status source across resume
(mdio_bus_phy_resume() -> phy_init_hw()) or PHY attach?
The reverse case looks reachable too: selection left at
LINK_STATUS_CONFIG_PLCA_STATUS while pst dropped to 0 during the masked
window, with no further edge to correct it.
> }
>
> ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IMSK1,
> - LAN86XX_STS1_PLCA_STS_CHANGED);
> + mask);
> if (ret)
> return ret;
>
> @@ -595,6 +614,47 @@ static irqreturn_t lan86xx_handle_interrupt(struct phy_device *phydev)
> return ret_irq;
> }
>
> +static irqreturn_t lan867x_revd0_handle_interrupt(struct phy_device *phydev)
> +{
> + struct phy_plca_status plca_st;
> + irqreturn_t ret_irq = IRQ_NONE;
> + int sts1, ret;
> +
> + /* Reading the status register clears the latched event bits. */
> + sts1 = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_STS1);
> + if (sts1 < 0) {
> + phy_error(phydev);
> + return IRQ_NONE;
> + }
> +
> + if (sts1 & LAN86XX_STS1_LINK_STS_CHANGED) {
> + phy_trigger_machine(phydev);
> + ret_irq = IRQ_HANDLED;
> + }
> +
> + 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;
> + }
> +
> + /* Collision detection is handled autonomously by the hardware
> + * via CCMFC. Only the link status selection needs to be updated
> + * on each PLCA status transition.
> + */
> + ret = lan867x_revd0_link_active_selection(phydev, plca_st.pst);
[Severity: Medium]
Is plca_st.pst the right value to feed into this argument? The parameter
is a mode selector:
static int lan867x_revd0_link_active_selection(struct phy_device *phydev,
bool plca_enabled)
and the other two callers pass a configuration value:
lan867x_revd0_config_init() passes false, and lan86xx_plca_set_cfg()
passes plca_cfg->enabled.
pst is a status bit, not the enable state:
drivers/net/phy/phy-c45.c:genphy_c45_plca_get_status() {
...
plca_st->pst = !!(ret & MDIO_OATC14_PLCA_PST);
...
}
include/linux/phy.h documents it as "The PLCA status as reported by the
PST bit in the PLCA STATUS register(31.CA03), indicating BEACON activity."
So while PLCA stays enabled but PLCA_Status goes not-OK, this call takes
the else branch and writes
value = FIELD_PREP(LINK_STATUS_CONFIGURATION,
LINK_STATUS_CONFIG_SEMAPHORE) |
FIELD_PREP(LINK_STATUS_SEMAPHORE,
LINK_STATUS_SEMAPHORE_SET);
which forces the link active and discards the
LINK_STATUS_CONFIG_PLCA_STATUS selection that lan86xx_plca_set_cfg()
programmed from the user's plca_cfg->enabled. Since the register is only
switched back to the plca_status source while pst == 1, can a
plca_status-derived link-down ever be observed? The Rev.D0 entry has no
.read_status override, so genphy_read_status() reads exactly this hardware
link status.
The ordering inside the handler makes the loss concrete when both bits are
latched in one invocation: the LNKSTSC branch only queues work
(phy_trigger_machine() -> phy_queue_state_machine(phydev, 0)), and the
forced-active write happens after it, so the queued state machine sees
link up.
Should the selection be driven from the configured PLCA enable state
instead, with the state machine left to report the PLCA-derived link
status?
> + if (ret < 0) {
> + phy_error(phydev);
> + return IRQ_NONE;
> + }
> +
> + ret_irq = IRQ_HANDLED;
> + }
> +
> + return ret_irq;
> +}
> +
> static struct phy_driver microchip_t1s_driver[] = {
[ ... ]
> @@ -637,6 +697,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 = lan86xx_config_intr,
> + .handle_interrupt = lan867x_revd0_handle_interrupt,
> .get_plca_cfg = genphy_c45_plca_get_cfg,
> .set_plca_cfg = lan86xx_plca_set_cfg,
[Severity: Medium]
Keeping the shared lan86xx_plca_set_cfg() for Rev.D0 also keeps the
plca_cfg->enabled == -1 case, which that function treats as "enabled".
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);
...
}
include/linux/phy.h describes the field as "-1 = not available / don't
set. 0 = disabled, anything else = enabled", and
genphy_c45_plca_set_cfg() honours that by testing "enabled == 0" and
"enabled > 0".
lan86xx_plca_set_cfg() instead does
if (phydev->phy_id == PHY_ID_LAN867X_REVD0) {
ret = lan867x_revd0_link_active_selection(phydev,
plca_cfg->enabled);
...
if (plca_cfg->enabled)
return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
LAN86XX_REG_COL_DET_CTRL0,
COL_DET_CTRL0_ENABLE_BIT_MASK,
COL_DET_DISABLE);
so does "ethtool --set-plca-cfg <dev> node-id 5" (which leaves enabled at
-1) select LINK_STATUS_CONFIG_PLCA_STATUS on Rev.D0 even with PLCA
disabled, and clear CDEN on PHY_POLL boards? Should the -1 "don't set"
case be handled here as well while touching this path?
> .get_plca_status = genphy_c45_plca_get_status,
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907052007.391972-1-parthiban.veerasooran%40microchip.com
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-09-10 5:21 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-07 5:20 [PATCH net-next v2 0/4] net: microchip_t1s: fix collision detection on PLCA status change Parthiban Veerasooran
2026-09-07 5:20 ` [PATCH net-next v2 1/4] net: phy: " Parthiban Veerasooran
2026-09-10 5:21 ` netdev-bot+sashiko
2026-09-07 5:20 ` [PATCH net-next v2 2/4] net: ethernet: oa_tc6: deliver the PHY interrupt to phylib Parthiban Veerasooran
2026-09-10 5:21 ` netdev-bot+sashiko
2026-09-07 5:20 ` [PATCH net-next v2 3/4] microchip: lan865x: enable PHY interrupt via virtual IRQ Parthiban Veerasooran
2026-09-10 5:21 ` netdev-bot+sashiko
2026-09-07 5:20 ` [PATCH net-next v2 4/4] net: phy: microchip_t1s: fix collision detection for LAN867X Rev.D0 Parthiban Veerasooran
2026-09-10 5:21 ` netdev-bot+sashiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®