linux-kernel - Re: [PATCH net v3 1/1] net: pcs: xpcs: fix incorrect CL37 AN sequence

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: <YVbbitGJDZhd6eW/@shell.armlinux.org.uk>
Date:   Fri, 1 Oct 2021 10:57:30 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     Wong Vee Khee <vee.khee.wong@...ux.intel.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jose Abreu <Jose.Abreu@...opsys.com>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Wong Vee Khee <veekhee@...il.com>
Subject: Re: [PATCH net v3 1/1] net: pcs: xpcs: fix incorrect CL37 AN sequence

On Fri, Oct 01, 2021 at 12:19:52AM +0000, Vladimir Oltean wrote:
> static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode)
> {
> 	int ret, mdio_ctrl1, old_an_ctrl, an_ctrl, old_dig_ctrl1, dig_ctrl1;
> 
> 	/* Disable SGMII AN in case it is already enabled */
> 	mdio_ctrl1 = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL);
> 	if (mdio_ctrl1 < 0)
> 		return mdio_ctrl1;
> 
> 	if (mdio_ctrl1 & AN_CL37_EN) {
> 		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL,
> 				 mdio_ctrl1 & ~AN_CL37_EN);
> 		if (ret < 0)
> 			return ret;
> 	}

This is fine...

> 	if (!(mdio_ctrl1 & AN_CL37_EN) && phylink_autoneg_inband(mode)) {
> 		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL,
> 				 mdio_ctrl1 | AN_CL37_EN);
> 		if (ret)
> 			return ret;
> 	}

This is not. If the control register had AN_CL37_EN set initially, then
in the first test above, we clear the bit. However, mdio_ctrl1 will
still contain the bit set. When we get here, we will skip setting the
register bit back to one even if in-band mode was requested.

As I said in a previous email, at this point there is no reason to check
the previous state, because if it was set on entry, we will have cleared
it, so the register state at this point has the bit clear no matter
what. If we need to set it, then we /always/ need to write it here - it
doesn't matter what the initial state was.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!