netdev - Re: [PATCH net-next] net: phylink: Support disabling autonegotiation for PCS

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: <20210701145222.GK22278@shell.armlinux.org.uk>
Date:   Thu, 1 Jul 2021 15:52:22 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Robert Hancock <robert.hancock@...ian.com>
Cc:     andrew@...n.ch, hkallweit1@...il.com, davem@...emloft.net,
        kuba@...nel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: phylink: Support disabling autonegotiation
 for PCS

On Wed, Jun 30, 2021 at 11:49:27AM -0600, Robert Hancock wrote:
> The auto-negotiation state in the PCS as set by
> phylink_mii_c22_pcs_config was previously always enabled when the driver is
> configured for in-band autonegotiation, even if autonegotiation was
> disabled on the interface with ethtool. Update the code to set the
> BMCR_ANENABLE bit based on the interface's autonegotiation enabled
> state.
> 
> Update phylink_mii_c22_pcs_get_state to not check
> autonegotiation-related fields when autonegotiation is disabled.
> 
> Update phylink_mac_pcs_get_state to initialize the state based on the
> interface's configured speed, duplex and pause parameters rather than to
> unknown when autonegotiation is disabled, before calling the driver's
> pcs_get_state functions, as they are not likely to provide meaningful data
> for these fields when autonegotiation is disabled. In this case the
> driver is really just filling in the link state field.
> 
> Note that in cases where there is a downstream PHY connected, such as
> with SGMII and a copper PHY, the configuration set by ethtool is handled by
> phy_ethtool_ksettings_set and not propagated to the PCS. This is correct
> since SGMII or 1000Base-X autonegotiation with the PCS should normally
> still be used even if the copper side has disabled it.

In theory, this seems to be correct, but...

We do have some cases where, if a port is in 1000Base-X mode, the
documentation explicitly states that AN must be enabled. So, I think
if we are introducing the possibility to disable the negotiation in
1000Base-X mode, we need to give an option to explicitly reject that
configuration attempt.

We also need this to be consistently applied over all the existing
phylink-using drivers that support 1000Base-X without AN - we shouldn't
end up in the situation where we have different behaviours with
different network drivers.

So, we need mvneta and mvpp2 to reject such a configuration - with
these ports in 1000Base-X mode, the documentation states:

"Bit 2 Field InBandAnEn In-band Auto-Negotiation enable. ...
When <PortType> = 1 (1000BASE-X) this field must be set to 1."

We should be aware that there may be other hardware out there which
may not support 1000BASE-X without inband.

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