Re: [PATCH] net: phy: add Marvell 88X2222 transceiver support

[Ivan Bornyakov]

On Tue, Feb 02, 2021 at 04:48:01PM +0000, Russell King - ARM Linux admin wrote:
> On Mon, Feb 01, 2021 at 10:22:51PM +0300, Ivan Bornyakov wrote:
> > +/* PMD Transmit Disable */
> > +#define	MV_TX_DISABLE		0x0009
> > +#define	MV_TX_DISABLE_GLOBAL	BIT(0)
> 
> Please use MDIO_PMA_TXDIS and MDIO_PMD_TXDIS_GLOBAL; this is an
> IEEE802.3 defined register.
> 
> > +/* 10GBASE-R PCS Status 1 */
> > +#define	MV_10GBR_STAT		MDIO_STAT1
> 
> Nothing Marvell specific here, please use MDIO_STAT1 directly.
> 
> > +/* 1000Base-X/SGMII Control Register */
> > +#define	MV_1GBX_CTRL		0x2000
> > +
> > +/* 1000BASE-X/SGMII Status Register */
> > +#define	MV_1GBX_STAT		0x2001
> > +
> > +/* 1000Base-X Auto-Negotiation Advertisement Register */
> > +#define	MV_1GBX_ADVERTISE	0x2004
> 
> Marvell have had a habbit of placing other PHY instances within the
> register space. This also looks like Clause 22 layout rather than
> Clause 45 layout - please use the Clause 22 definitions for the bits
> rather than Clause 45. (so BMCR_ANENABLE, BMCR_ANRESTART for
> MV_1GBX_CTRL, etc).
> 
> Please define these as:
> 
> +#define	MV_1GBX_CTRL		(0x2000 + MII_BMCR)
> +#define	MV_1GBX_STAT		(0x2000 + MII_BMSR)
> +#define	MV_1GBX_ADVERTISE	(0x2000 + MII_ADVERTISE)
> 
> to make it clear what is going on here.
> 
> > +static int sfp_module_insert(void *_priv, const struct sfp_eeprom_id *id)
> > +{
> > +	struct phy_device *phydev = _priv;
> > +	struct device *dev = &phydev->mdio.dev;
> > +	struct mv2222_data *priv = phydev->priv;
> > +	phy_interface_t interface;
> > +
> > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = { 0, };
> > +
> > +	sfp_parse_support(phydev->sfp_bus, id, supported);
> > +	interface = sfp_select_interface(phydev->sfp_bus, supported);
> > +
> > +	dev_info(dev, "%s SFP module inserted", phy_modes(interface));
> > +
> > +	switch (interface) {
> > +	case PHY_INTERFACE_MODE_10GBASER:
> > +		phydev->speed = SPEED_10000;
> > +		phydev->interface = PHY_INTERFACE_MODE_10GBASER;
> > +		linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseKR_Full_BIT,
> > +				 phydev->supported);
> > +
> > +		phy_write_mmd(phydev, MDIO_MMD_VEND2, MV_PCS_CONFIG,
> > +			      MV_PCS_HOST_XAUI | MV_PCS_LINE_10GBR);
> > +		mv2222_soft_reset(phydev);
> > +		break;
> > +	case PHY_INTERFACE_MODE_1000BASEX:
> > +	default:
> > +		phydev->speed = SPEED_1000;
> > +		phydev->interface = PHY_INTERFACE_MODE_1000BASEX;
> > +		linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKR_Full_BIT,
> > +				   phydev->supported);
> > +
> > +		phy_write_mmd(phydev, MDIO_MMD_VEND2, MV_PCS_CONFIG,
> > +			      MV_PCS_HOST_XAUI | MV_PCS_LINE_1GBX_AN);
> > +		mv2222_soft_reset(phydev);
> > +	}
> > +
> > +	priv->sfp_inserted = true;
> > +
> > +	if (priv->net_up)
> > +		mv2222_tx_enable(phydev);
> 
> This is racy. priv->net_up is modified via the suspend/resume
> callbacks, which are called with phydev->lock held. No other locks
> are guaranteed to be held.
> 
> However, this function is called with the SFP sm_mutex, and rtnl
> held. Consequently, the use of sfp_inserted and net_up in this
> function and the suspend/resume callbacks is racy.
> 
> Why are you disabling the transmitter anyway? Is this for power
> saving?
> 

Actually, the original thought was to down the link on the other side,
when network interface is down on our side. Power saving is a nice
side-effect.

> > +static void mv2222_update_interface(struct phy_device *phydev)
> > +{
> > +	if ((phydev->speed == SPEED_1000 ||
> > +	     phydev->speed == SPEED_100 ||
> > +	     phydev->speed == SPEED_10) &&
> > +	    phydev->interface != PHY_INTERFACE_MODE_1000BASEX) {
> > +		phydev->interface = PHY_INTERFACE_MODE_1000BASEX;
> > +
> > +		phy_write_mmd(phydev, MDIO_MMD_VEND2, MV_PCS_CONFIG,
> > +			      MV_PCS_HOST_XAUI | MV_PCS_LINE_1GBX_AN);
> > +		mv2222_soft_reset(phydev);
> > +	} else if (phydev->speed == SPEED_10000 &&
> > +		   phydev->interface != PHY_INTERFACE_MODE_10GBASER) {
> > +		phydev->interface = PHY_INTERFACE_MODE_10GBASER;
> > +
> > +		phy_write_mmd(phydev, MDIO_MMD_VEND2, MV_PCS_CONFIG,
> > +			      MV_PCS_HOST_XAUI | MV_PCS_LINE_10GBR);
> > +		mv2222_soft_reset(phydev);
> > +	}
> 
> This looks wrong. phydev->interface is the _host_ interface, which
> you are clearly setting to XAUI here. Some network drivers depend
> on this being correct (for instance, when used with the Marvell
> 88x3310 PHY which changes its host-side interface dynamically.)
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Overall, thank you for in-depth review, Russell.