netdev - Re: [PATCH] net: phy: marvell10g: report if the PHY fails to boot firmware

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]

Date:   Wed, 29 May 2019 12:03:15 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Maxime Chevallier <maxime.chevallier@...tlin.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH] net: phy: marvell10g: report if the PHY fails to boot
 firmware

On Tue, May 28, 2019 at 05:23:56PM +0100, Russell King - ARM Linux admin wrote:
> On Tue, May 28, 2019 at 06:11:39PM +0200, Andrew Lunn wrote:
> > > One question: are we happy with failing the probe like this, or would it
> > > be better to allow the probe to suceed?
> > > 
> > > As has been pointed out in the C45 MII access patch, we need the PHY
> > > to bind to the network driver for the MII bus to be accessible to
> > > userspace, so if we're going to have userspace tools to upload the
> > > firmware, rather than using u-boot, we need the PHY to be present and
> > > bound to the network interface.
> > 
> > Hi Russell
> > 
> > It is an interesting question. Failing the probe is the simple
> > solution. If we don't fail the probe, we then need to allow the
> > attach, but fail all normal operations, with a noisy kernel log.  That
> > probably means adding a new state to the state machine, PHY_BROKEN.
> > Enter that state if phy_start_aneg() returns an error?
> 
> Hi Andrew,
> 
> I don't think we need a new state - I think we can trap it in
> the link_change_notify() method, and force phydev->state to
> PHY_HALTED if it's in phy_is_started() mode.
> 
> Maybe something like:
> 
> 	if (phy_is_started(phydev) && priv->firmware_failed) {
> 		dev_warn(&phydev->mdio.dev,
> 			 "PHY firmware failure: link forced down");
> 		phydev->state = PHY_HALTED;
> 	}
> 
> Or maybe we just need to do that if phydev->state == PHY_UP or
> PHY_RESUMING (the two states entered from phy_start())?

Here's the fuller patch for what I'm suggesting:

 drivers/net/phy/marvell10g.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 754cde873dde..86333d98b384 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -60,6 +60,8 @@ enum {
 };
 
 struct mv3310_priv {
+	bool firmware_failed;
+
 	struct device *hwmon_dev;
 	char *hwmon_name;
 };
@@ -214,6 +216,10 @@ static int mv3310_probe(struct phy_device *phydev)
 	    (phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
 		return -ENODEV;
 
+	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
 	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT);
 	if (ret < 0)
 		return ret;
@@ -221,13 +227,9 @@ static int mv3310_probe(struct phy_device *phydev)
 	if (ret & MV_PMA_BOOT_FATAL) {
 		dev_warn(&phydev->mdio.dev,
 			 "PHY failed to boot firmware, status=%04x\n", ret);
-		return -ENODEV;
+		priv->firmware_failed = true;
 	}
 
-	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-
 	dev_set_drvdata(&phydev->mdio.dev, priv);
 
 	ret = mv3310_hwmon_probe(phydev);
@@ -247,6 +249,19 @@ static int mv3310_resume(struct phy_device *phydev)
 	return mv3310_hwmon_config(phydev, true);
 }
 
+static void mv3310_link_change_notify(struct phy_device *phydev)
+{
+	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
+	enum phy_state state = phydev->state;
+
+	if (priv->firmware_failed &&
+	    (state == PHY_UP || state == PHY_RESUMING)) {
+		dev_warn(&phydev->mdio.dev,
+			 "PHY firmware failure: link forced down");
+		phydev->state = state = PHY_HALTED;
+	}
+}
+
 /* Some PHYs in the Alaska family such as the 88X3310 and the 88E2010
  * don't set bit 14 in PMA Extended Abilities (1.11), although they do
  * support 2.5GBASET and 5GBASET. For these models, we can still read their
-- 
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