netdev - [PATCH net-next] net: phy: consider latched link-down status in polling mode

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: <7180c54c-34b2-0a36-97ff-bbda302eb427@gmail.com>
Date:   Wed, 6 Feb 2019 19:39:52 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Russell King - ARM Linux <linux@...linux.org.uk>
Subject: [PATCH net-next] net: phy: consider latched link-down status in
 polling mode

The link status value latches link-down events. To get the current
status we read the register twice in genphy_update_link(). There's
a potential risk that we miss a link-down event in polling mode.
This may cause issues if the user e.g. connects his machine to a
different network.

On the other hand reading the latched value may cause issues in
interrupt mode. Following scenario:

- After boot link goes up
- phy_start() is called triggering an aneg restart, hence link goes
  down and link-down info is latched.
- After aneg has finished link goes up and triggers an interrupt.
  Interrupt handler reads link status, means it reads the latched
  "link is down" info. But there won't be another interrupt as long
  as link stays up, therefore phylib will never recognize that link
  is up.

Deal with both scenarios by reading the register twice in interrupt
mode only.

Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
---
 drivers/net/phy/phy-c45.c    | 10 ++++++++--
 drivers/net/phy/phy_device.c | 13 +++++++++----
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index c92d0fb7e..449f0f986 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -134,9 +134,15 @@ int genphy_c45_read_link(struct phy_device *phydev, u32 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
-		 * register if the link is down.
+		 * 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;
+		}
+
 		val = phy_read_mmd(phydev, devad, MDIO_STAT1);
 		if (val < 0)
 			return val;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 891e0178b..d490cd2a8 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1729,10 +1729,15 @@ int genphy_update_link(struct phy_device *phydev)
 {
 	int status;
 
-	/* Do a fake read */
-	status = phy_read(phydev, MII_BMSR);
-	if (status < 0)
-		return 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;
+	}
 
 	/* Read link and autonegotiation status */
 	status = phy_read(phydev, MII_BMSR);
-- 
2.20.1