Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it - Russell King

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Antoine Tenart <atenart@kernel.org>,
	Quentin Schulz <quentin.schulz@bootlin.com>,
	Michael Walle <michael@walle.cc>,
	netdev@vger.kernel.org, Heiner Kallweit <hkallweit1@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Ioana Ciornei <ioana.ciornei@nxp.com>,
	Maxim Kochetkov <fido_max@inbox.ru>,
	Bjarni Jonasson <bjarni.jonasson@microchip.com>,
	Steen Hegelund <steen.hegelund@microchip.com>,
	UNGLinuxDriver@microchip.com
Subject: Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it
Date: Sun, 14 Feb 2021 10:35:29 +0000	[thread overview]
Message-ID: <20210214103529.GT1463@shell.armlinux.org.uk> (raw)
In-Reply-To: <20210212172341.3489046-2-olteanv@gmail.com>

On Fri, Feb 12, 2021 at 07:23:40PM +0200, Vladimir Oltean wrote:
> +	ret = phy_config_inband_aneg(phy,
> +				     (pl->cur_link_an_mode == MLO_AN_INBAND));

Please use phylink_autoneg_inband(pl->cur_link_an_mode) here.

> +	if (ret && ret != -EOPNOTSUPP) {
> +		phylink_warn(pl, "failed to configure PHY in-band autoneg: %d\n",
> +			     ret);

Please use %pe and ERR_PTR(ret) so we can get a symbolic errno value.

As mentioned in this thread, we have at least one PHY which is unable
to provide the inband signalling in any mode (BCM84881). Currently,
phylink detects this PHY on a SFP (in phylink_phy_no_inband()) and
adjusts not to use inband mode. This would need to be addressed if we
are creating an alterative way to discover whether the PHY supports
inband mode or not.

Also, there needs to be consideration of PHYs that dynamically change
their interface type, and whether they support inband signalling.
For example, a PHY may support a mode where it dynamically selects
between 10GBASE-R, 5GBASE-R, 2500BASE-X and SGMII, where the SGMII
mode may have inband signalling enabled or disabled. This is not a
theoretical case; we have a PHY like that supported in the kernel and
boards use it. What would the semantics of your new call be for a PHY
that performs this?

Should we also have a phydev->inband tristate, taking values "unknown,
enabled, disabled" which the PHY driver is required to update in their
read_status callback if they dynamically change their interface type?
(Although then phylink will need to figure out how to deal with that.)

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