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]

Message-ID: <20190528154238.ifudfslyofk22xoe@shell.armlinux.org.uk>
Date:   Tue, 28 May 2019 16:42:38 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc:     "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 10:34:42AM +0100, Russell King wrote:
> Some boards do not have the PHY firmware programmed in the 3310's flash,
> which leads to the PHY not working as expected.  Warn the user when the
> PHY fails to boot the firmware and refuse to initialise.
> 
> Signed-off-by: Russell King <rmk+kernel@...linux.org.uk>
> ---
> I think this patch needs testing with the Marvell 88x2110 PHY before
> this can be merged into mainline, but I think it should go into -rc
> and be back-ported to stable trees to avoid user frustration. I spent
> some time last night debugging one such instance, and the user
> afterwards indicated that they'd had the problem for a long time, and
> had thought of throwing the hardware out the window!  Clearly not a
> good user experience.
> 
>  drivers/net/phy/marvell10g.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index 100b401b1f4a..754cde873dde 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -31,6 +31,9 @@
>  #define MV_PHY_ALASKA_NBT_QUIRK_REV	(MARVELL_PHY_ID_88X3310 | 0xa)
>  
>  enum {
> +	MV_PMA_BOOT		= 0xc050,
> +	MV_PMA_BOOT_FATAL	= BIT(0),
> +
>  	MV_PCS_BASE_T		= 0x0000,
>  	MV_PCS_BASE_R		= 0x1000,
>  	MV_PCS_1000BASEX	= 0x2000,
> @@ -211,6 +214,16 @@ static int mv3310_probe(struct phy_device *phydev)
>  	    (phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
>  		return -ENODEV;
>  
> +	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret & MV_PMA_BOOT_FATAL) {
> +		dev_warn(&phydev->mdio.dev,
> +			 "PHY failed to boot firmware, status=%04x\n", ret);
> +		return -ENODEV;

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.

> +	}
> +
>  	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
> -- 
> 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