linux-kernel - Re: [PATCH net-next v3 4/5] net: phy: mediatek: Extend 1G TX/RX link pulse time

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: Mon, 20 May 2024 15:17:33 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Sky Huang <SkyLake.Huang@...iatek.com>
Cc: Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Daniel Golle <daniel@...rotopia.org>,
	Qingfang Deng <dqfext@...il.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org,
	Steven Liu <Steven.Liu@...iatek.com>
Subject: Re: [PATCH net-next v3 4/5] net: phy: mediatek: Extend 1G TX/RX link
 pulse time

> +static void extend_an_new_lp_cnt_limit(struct phy_device *phydev)
> +{
> +	int mmd_read_ret;
> +	int ret;
> +	u32 reg_val;
> +
> +	ret = read_poll_timeout(mmd_read_ret = phy_read_mmd, reg_val,
> +				(mmd_read_ret < 0) || reg_val & MTK_PHY_FINAL_SPEED_1000,
> +				10000, 1000000, false, phydev,
> +				MDIO_MMD_VEND1, MTK_PHY_LINK_STATUS_MISC);
> +	if (mmd_read_ret < 0)
> +		ret = mmd_read_ret;
> +	/* If final_speed_1000 is raised, try to extend timeout period
> +	 * of auto downshift.
> +	 */
> + if (!ret) {

If you look at other Linux code, the general pattern is to look if a
function returned an error. If it does, either return immediately, or
jump to the end of the function where the cleanup is.

Since this is a void function:

> +	if (mmd_read_ret < 0)
> +		return;

And then you don't need the

> + if (!ret) {


> +		tr_modify(phydev, 0x0, 0xf, 0x3c, AN_NEW_LP_CNT_LIMIT_MASK,
> +			  FIELD_PREP(AN_NEW_LP_CNT_LIMIT_MASK, 0xf));
> +		mdelay(1500);
> +
> +		ret = read_poll_timeout(mmd_read_ret = tr_read, reg_val,
> +					(mmd_read_ret < 0) ||
> +					(reg_val & AN_STATE_MASK) !=
> +					(AN_STATE_TX_DISABLE << AN_STATE_SHIFT),
> +					10000, 1000000, false, phydev,
> +					0x0, 0xf, 0x2);
> +
> +		if (mmd_read_ret < 0)
> +			ret = mmd_read_ret;
> +
> +		if (!ret) {

This if can also be removed.

> +			mdelay(625);
> +			tr_modify(phydev, 0x0, 0xf, 0x3c, AN_NEW_LP_CNT_LIMIT_MASK,
> +				  FIELD_PREP(AN_NEW_LP_CNT_LIMIT_MASK, 0x8));
> +			mdelay(500);
> +			tr_modify(phydev, 0x0, 0xf, 0x3c, AN_NEW_LP_CNT_LIMIT_MASK,
> +				  FIELD_PREP(AN_NEW_LP_CNT_LIMIT_MASK, 0xf));
> +		}
> +	}

One question i have is, should this really be a void function? What
does it mean if read_poll_timeout() returns an error? Why is it safe
to ignore it? Why not return the error?

> +}
> +
> +int mtk_gphy_cl22_read_status(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = genphy_read_status(phydev);
> +	if (ret)
> +		return ret;
> +
> +	if (phydev->autoneg == AUTONEG_ENABLE && !phydev->autoneg_complete) {
> +		ret = phy_read(phydev, MII_CTRL1000);
> +		if ((ret & ADVERTISE_1000FULL) || (ret & ADVERTISE_1000HALF))
> +			extend_an_new_lp_cnt_limit(phydev);
> +	}
> +
> +	return 0;

If extend_an_new_lp_cnt_limit() fails, what does it mean? Do we
actually want mtk_gphy_cl22_read_status() to indicate something has
gone wrong? Or does extend_an_new_lp_cnt_limit() failing not matter?

	Andrew