netdev - [RFT PATCH] net: fec: Fix MDIO polled IO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <813a0ccb-a309-137d-a340-b9b03b9e230c@vyex.com>
Date:   Sun, 9 Aug 2020 18:00:33 -0500
From:   Dave Karr <dkarr@...x.com>
To:     netdev@...r.kernel.org
Subject: [RFT PATCH] net: fec: Fix MDIO polled IO

Fixes problematic commit f166f890c8f026a931e1bb80f51561a1d2f41b27

Commit broke Ethernet functionality on i.MX53 with 1 GHz clock due to a
race condition which may not be observed in the unknown CPU(s) implied
by comments to depart from FEC behavior documented by Motorola,
Freescale and NXP.

During driver initialization the stated intent was to suppress the
potential generation of a MII interrupt resulting from a write to
MSCR, but for devices which behave as documented, an interrupt is
caused by any write to MMFR regardless of value. Testing confirms
the i.MX53 behaves as documented.

The subsequent attempt to clear any pending MII interrupt generated by
either the MMFR or MSCR write is a race condition dependent upon
CPU vs. MDC clock speed which permits the interrupt to occur after the
write to EIR.

Prior to the polled IO patch, regardless of what caused an MII
interrupt, the fec_enet_mdio_read/write functions relied upon the
interrupt service routine to clear the EIR MII bit and thus the
critical region between ISR exit and fec_enet_mdio_read/write entry
was small by comparison.

Together, this resulted in the subsequent read of PHYID during PHY
probing to return before the mdio bus transaction was complete
causing invalid data to be read from MMFR which in turn resulted in
improper identification of PHY devices on the bus.

Fix by eliminating dependency on other functions to clear the EIR MII
bit and shrink the critical region by clearing the bit immediately
prior to MMFR write within fec_enet_mdio_read/write functions.

Remove use of undocumented behavior and replace unconditional reset of
MII EIR bit with fec_enet_mdio_wait such that NXP can identify whether
regressions noted in 21615efa6a69891fa287bade979d56dd68b09878 and/or
0a699302be5986307b3dcf84ac7a0dd30f9e9305 are due to an intentional write
to MMFR with MSCR equal to zero prior to driver initialization, or the
result of unpublished or unrealized FEC errata.

Reuse FEC_MII_TIMEOUT #define in fec_enet_mdio_wait().

Signed-off-by: Dave Karr <dkarr@...x.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 9934421814b4..a56169a3b0a9 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1792,7 +1792,7 @@ static int fec_enet_mdio_wait(struct fec_enet_private *fep)
 	int ret;
 
 	ret = readl_poll_timeout_atomic(fep->hwp + FEC_IEVENT, ievent,
-					ievent & FEC_ENET_MII, 2, 30000);
+					ievent & FEC_ENET_MII, 2, FEC_MII_TIMEOUT);
 
 	if (!ret)
 		writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
@@ -1816,6 +1816,7 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 
 		/* write address */
 		frame_addr = (regnum >> 16);
+		writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
 		writel(frame_start | FEC_MMFR_OP_ADDR_WRITE |
 		       FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(frame_addr) |
 		       FEC_MMFR_TA | (regnum & 0xFFFF),
@@ -1838,6 +1839,7 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 	}
 
 	/* start a read op */
+	writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
 	writel(frame_start | frame_op |
 		FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(frame_addr) |
 		FEC_MMFR_TA, fep->hwp + FEC_MII_DATA);
@@ -1877,6 +1879,7 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 
 		/* write address */
 		frame_addr = (regnum >> 16);
+		writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
 		writel(frame_start | FEC_MMFR_OP_ADDR_WRITE |
 		       FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(frame_addr) |
 		       FEC_MMFR_TA | (regnum & 0xFFFF),
@@ -1895,6 +1898,7 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 	}
 
 	/* start a write op */
+	writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
 	writel(frame_start | FEC_MMFR_OP_WRITE |
 		FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(frame_addr) |
 		FEC_MMFR_TA | FEC_MMFR_DATA(value),
@@ -2114,20 +2118,14 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 	if (suppress_preamble)
 		fep->phy_speed |= BIT(7);
 
-	/* Clear MMFR to avoid to generate MII event by writing MSCR.
-	 * MII event generation condition:
-	 * - writing MSCR:
-	 *	- mmfr[31:0]_not_zero & mscr[7:0]_is_zero &
-	 *	  mscr_reg_data_in[7:0] != 0
-	 * - writing MMFR:
-	 *	- mscr[7:0]_not_zero
-	 */
-	writel(0, fep->hwp + FEC_MII_DATA);
+	/* start with mii interrupt flag clear */
+	writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
 
 	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
 
 	/* Clear any pending transaction complete indication */
-	writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
+	if (!(fec_enet_mdio_wait(fep)))
+		netdev_dbg(fep->netdev, "MSCR write during init caused mii interrupt\n");
 
 	fep->mii_bus = mdiobus_alloc();
 	if (fep->mii_bus == NULL) {
-- 
2.26.2