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 18:32:03 +0300
From:   Ivan Bornyakov <i.bornyakov@...rotek.ru>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, system@...rotek.ru, hkallweit1@...il.com,
        linux@...linux.org.uk, 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 11:56:01PM +0100, Andrew Lunn wrote:
> > +static int mv2222_config_init(struct phy_device *phydev)
> > +{
> > +	linkmode_zero(phydev->supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, phydev->supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT, phydev->supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, phydev->supported);
> > +
> > +	phydev->pause = 0;
> > +	phydev->asym_pause = 0;
> > +	phydev->duplex = DUPLEX_FULL;
> > +	phydev->autoneg = AUTONEG_DISABLE;
> > +
> > +	return 0;
> > +}
> > +
> > +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;
> 
> The speeds 10 and 100 seem odd here. 1000BaseX only supports 1G. It
> would have to be SGMII in order to also support 10Mbps and 100Mbps.
> Plus you are not listing 10 and 100 as a supported value.
> 
> > +/* Returns negative on error, 0 if link is down, 1 if link is up */
> > +static int mv2222_read_status_1g(struct phy_device *phydev)
> > +{
> > +	int val, link = 0;
> > +
> > +	val = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_1GBX_STAT);
> > +	if (val < 0)
> > +		return val;
> > +
> > +	if (!(val & MDIO_STAT1_LSTATUS) ||
> > +	    (phydev->autoneg == AUTONEG_ENABLE && !(val & MDIO_AN_STAT1_COMPLETE)))
> > +		return 0;
> > +
> > +	link = 1;
> > +
> > +	if (phydev->autoneg == AUTONEG_DISABLE) {
> > +		phydev->speed = SPEED_1000;
> > +		phydev->duplex = DUPLEX_FULL;
> > +
> > +		return link;
> > +	}
> > +
> > +	val = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_1GBX_PHY_STAT);
> > +	if (val < 0)
> > +		return val;
> > +
> > +	if (val & MV_1GBX_PHY_STAT_AN_RESOLVED) {
> > +		if (val & MV_1GBX_PHY_STAT_DUPLEX)
> > +			phydev->duplex = DUPLEX_FULL;
> > +		else
> > +			phydev->duplex = DUPLEX_HALF;
> > +
> > +		if (val & MV_1GBX_PHY_STAT_SPEED1000)
> > +			phydev->speed = SPEED_1000;
> > +		else if (val & MV_1GBX_PHY_STAT_SPEED100)
> > +			phydev->speed = SPEED_100;
> > +		else
> > +			phydev->speed = SPEED_10;
> 
> Are you sure it is not doing SGMII? Maybe it can do both, 1000BaseX
> and SGMII? You would generally use 1000BaseX to connect to a fibre
> SFP, and SGMII to connect to a copper SFP. So ideally you want to be
> able to swap between these modes as needed.
> 
> > +static int mv2222_read_status(struct phy_device *phydev)
> > +{
> > +	int link;
> > +
> > +	linkmode_zero(phydev->lp_advertising);
> > +	phydev->pause = 0;
> > +	phydev->asym_pause = 0;
> > +
> > +	switch (phydev->interface) {
> > +	case PHY_INTERFACE_MODE_10GBASER:
> > +		link = mv2222_read_status_10g(phydev);
> > +		break;
> > +	case PHY_INTERFACE_MODE_1000BASEX:
> > +	default:
> > +		link = mv2222_read_status_1g(phydev);
> > +		break;
> > +	}
> > +
> > +	if (link < 0)
> > +		return link;
> > +
> > +	phydev->link = link;
> > +
> > +	if (phydev->link)
> > +		mv2222_link_led_on(phydev);
> > +	else
> > +		mv2222_link_led_off(phydev);
> 
> You have to manually control the LED? That is odd for a PHY. Normally
> you just select a mode and it will do it all in hardware.
> 
>     Andrew

Thanks for the review, Andrew, your remarks are all valid. I'll
implement 1000Base-X/SGMII swapping. As for the LED, I'll just drop it
for now, as it is not essential. I'll commit LEDs config when core
funtionality will be accepted.