netdev - Re: [PATCH net-next 3/3] net: phy: move handling latched link-down to phylib state machine

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190528131524.unl7uvgzurcppu7s@shell.armlinux.org.uk>
Date:   Tue, 28 May 2019 14:15:24 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 3/3] net: phy: move handling latched link-down
 to phylib state machine

On Mon, May 27, 2019 at 08:29:45PM +0200, Heiner Kallweit wrote:
> Especially with fibre links there may be very short link drops. And if
> interrupt handling is slow we may miss such a link drop. To deal with
> this we remove the double link status read from the generic link status
> read functions, and call the state machine twice instead.
> The flag for double-reading link status can be set by phy_mac_interrupt
> from hard irq context, therefore we have to use an atomic operation.

I came up with a different solution to this - I haven't extensively
tested it yet though:

 drivers/net/phy/phy-c45.c    | 12 ------------
 drivers/net/phy/phy.c        | 29 +++++++++++++++++++----------
 drivers/net/phy/phy_device.c | 14 --------------
 3 files changed, 19 insertions(+), 36 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 9e24d9569424..756d7711cbc5 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -222,18 +222,6 @@ int genphy_c45_read_link(struct phy_device *phydev)
 		devad = __ffs(mmd_mask);
 		mmd_mask &= ~BIT(devad);
 
-		/* The link state is latched low so that momentary link
-		 * drops can be detected. Do not double-read the status
-		 * in polling mode to detect such short link drops.
-		 */
-		if (!phy_polling_mode(phydev)) {
-			val = phy_read_mmd(phydev, devad, MDIO_STAT1);
-			if (val < 0)
-				return val;
-			else if (val & MDIO_STAT1_LSTATUS)
-				continue;
-		}
-
 		val = phy_read_mmd(phydev, devad, MDIO_STAT1);
 		if (val < 0)
 			return val;
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 7b3c5eec0129..2e7f0428e8fa 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -507,20 +507,29 @@ static int phy_config_aneg(struct phy_device *phydev)
  */
 static int phy_check_link_status(struct phy_device *phydev)
 {
-	int err;
+	int err, i;
 
 	WARN_ON(!mutex_is_locked(&phydev->lock));
 
-	err = phy_read_status(phydev);
-	if (err)
-		return err;
+	/* The link state is latched low so that momentary link drops can
+	 * be detected. If the link has failed, re-read the link status
+	 * to ensure that we are up to date with the current link state,
+	 * while notifying that the link status has changed.
+	 */
+	for (i = 0; i < 2; i++) {
+		err = phy_read_status(phydev);
+		if (err)
+			return err;
 
-	if (phydev->link && phydev->state != PHY_RUNNING) {
-		phydev->state = PHY_RUNNING;
-		phy_link_up(phydev);
-	} else if (!phydev->link && phydev->state != PHY_NOLINK) {
-		phydev->state = PHY_NOLINK;
-		phy_link_down(phydev, true);
+		if (phydev->link && phydev->state != PHY_RUNNING) {
+			phydev->state = PHY_RUNNING;
+			phy_link_up(phydev);
+		} else if (!phydev->link && phydev->state != PHY_NOLINK) {
+			phydev->state = PHY_NOLINK;
+			phy_link_down(phydev, true);
+		}
+		if (phydev->link)
+			break;
 	}
 
 	return 0;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 77068c545de0..ccc292c0f585 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1704,20 +1704,6 @@ int genphy_update_link(struct phy_device *phydev)
 {
 	int status;
 
-	/* The link state is latched low so that momentary link
-	 * drops can be detected. Do not double-read the status
-	 * in polling mode to detect such short link drops.
-	 */
-	if (!phy_polling_mode(phydev)) {
-		status = phy_read(phydev, MII_BMSR);
-		if (status < 0) {
-			return status;
-		} else if (status & BMSR_LSTATUS) {
-			phydev->link = 1;
-			return 0;
-		}
-	}
-
 	/* Read link and autonegotiation status */
 	status = phy_read(phydev, MII_BMSR);
 	if (status < 0)
-- 
2.7.4


-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up