netdev - Re: [PATCH 2/2] net: phy: Add driver for Motorcomm yt8821 2.5G ethernet phy

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zqd/6u5b7z1bCFaT@shell.armlinux.org.uk>
Date: Mon, 29 Jul 2024 12:41:30 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: "Frank.Sae" <Frank.Sae@...or-comm.com>
Cc: andrew@...n.ch, hkallweit1@...il.com, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
	netdev@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, yuanlai.cui@...or-comm.com,
	hua.sun@...or-comm.com, xiaoyong.li@...or-comm.com,
	suting.hu@...or-comm.com, jie.han@...or-comm.com
Subject: Re: [PATCH 2/2] net: phy: Add driver for Motorcomm yt8821 2.5G
 ethernet phy

On Sat, Jul 27, 2024 at 02:20:31AM -0700, Frank.Sae wrote:
> +/**
> + * yt8821_config_init() - phy initializatioin
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * returns 0 or negative errno code
> + */
> +static int yt8821_config_init(struct phy_device *phydev)
> +{
> +	struct yt8821_priv *priv = phydev->priv;
> +	int ret, val;
> +
> +	phydev->irq = PHY_POLL;
> +
> +	val = ytphy_read_ext_with_lock(phydev, YT8521_CHIP_CONFIG_REG);
> +	if (priv->chip_mode == YT8821_CHIP_MODE_AUTO_BX2500_SGMII) {
> +		ret = ytphy_modify_ext_with_lock(phydev,
> +						 YT8521_CHIP_CONFIG_REG,
> +						 YT8521_CCR_MODE_SEL_MASK,
> +						 FIELD_PREP(YT8521_CCR_MODE_SEL_MASK, 0));
> +		if (ret < 0)
> +			return ret;
> +
> +		__assign_bit(PHY_INTERFACE_MODE_2500BASEX,
> +			     phydev->possible_interfaces,
> +			     true);
> +		__assign_bit(PHY_INTERFACE_MODE_SGMII,
> +			     phydev->possible_interfaces,
> +			     true);

Before each and every call to .config_init, phydev->possible_interfaces
will be cleared. So, please use __set_bit() here.

> +static int yt8821_read_status(struct phy_device *phydev)
> +{
> +	struct yt8821_priv *priv = phydev->priv;
> +	int old_page;
> +	int ret = 0;
> +	int link;
> +	int val;
> +
> +	if (phydev->autoneg == AUTONEG_ENABLE) {
> +		int lpadv = phy_read_mmd(phydev,
> +					 MDIO_MMD_AN, MDIO_AN_10GBT_STAT);
> +
> +		if (lpadv < 0)
> +			return lpadv;
> +
> +		mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising,
> +						  lpadv);
> +	}
> +
> +	ret = ytphy_write_ext_with_lock(phydev,
> +					YT8521_REG_SPACE_SELECT_REG,
> +					YT8521_RSSR_UTP_SPACE);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = genphy_read_status(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	old_page = phy_select_page(phydev, YT8521_RSSR_UTP_SPACE);
> +	if (old_page < 0)
> +		goto err_restore_page;
> +
> +	val = __phy_read(phydev, YTPHY_SPECIFIC_STATUS_REG);
> +	if (val < 0) {
> +		ret = val;
> +		goto err_restore_page;
> +	}
> +
> +	link = val & YTPHY_SSR_LINK;

What link status is this reporting? For interface switching to work,
phydev->link must _only_ indicate whether the _media_ side interface
is up or down. It must _not_ include the status of the MAC facing
interface from the PHY.

Why? The interface configuration of the MAC is only performed when
the _media_ link comes up, denoted by phydev->link becoming true.
If the MAC interface configuration mismatches the PHY interface
configuration, then the MAC facing interface of the PHY will
remain down, and if phydev->link is forced to false, then the link
will never come up.

So, I hope that this isn't testing the MAC facing interface status
of the PHY!

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