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

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]

Date:   Tue, 2 Feb 2021 16:48:01 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Ivan Bornyakov <i.bornyakov@...rotek.ru>
Cc:     netdev@...r.kernel.org, system@...rotek.ru, andrew@...n.ch,
        hkallweit1@...il.com, davem@...emloft.net, kuba@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: phy: add Marvell 88X2222 transceiver support

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?

> +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!