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.
