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]
Message-ID: <Zy3xaviqqT6X8Ows@shell.armlinux.org.uk>
Date: Fri, 8 Nov 2024 11:09:30 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, Florian Fainelli <f.fainelli@...il.com>,
	Vladimir Oltean <olteanv@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	upstream@...oha.com
Subject: Re: [net-next PATCH v3 3/3] net: phy: Add Airoha AN8855 Internal
 Switch Gigabit PHY

On Wed, Nov 06, 2024 at 01:22:38PM +0100, Christian Marangi wrote:
> +/* MII Registers Page 1 */
> +#define AN8855_PHY_EXT_REG_14			0x14
> +#define   AN8855_PHY_EN_DOWN_SHFIT		BIT(4)

Shouldn't "AN8855_PHY_EN_DOWN_SHFIT" be "AN8855_PHY_EN_DOWN_SHIFT"
(notice the I and F are swapped) ?

> +static int an8855_get_downshift(struct phy_device *phydev, u8 *data)
> +{
> +	int saved_page;
> +	int val;
> +	int ret;
> +
> +	saved_page = phy_select_page(phydev, AN8855_PHY_PAGE_EXTENDED_1);
> +	if (saved_page >= 0)
> +		val = __phy_read(phydev, AN8855_PHY_EXT_REG_14);
> +	ret = phy_restore_page(phydev, saved_page, val);
> +	if (ret)
> +		return ret;

This function is entirely broken.

phy_restore_page() will return "val" if everything went successfully,
so here you end up returning "val" via this very return statement
without executing any further code in the function. The only time
further code will be executed is if "val" was successfully read as
zero.

Please use the helpers provided:

	ret = phy_read_paged(phydev, AN8855_PHY_PAGE_EXTENDED_1,
			     AN8855_PHY_EXT_REG_14);
	if (ret < 0)
		return ret;

ret now contains what you're using as "val" below. No need to open code
phy_read_paged().

> +
> +	*data = val & AN8855_PHY_EXT_REG_14 ? DOWNSHIFT_DEV_DEFAULT_COUNT :
> +					      DOWNSHIFT_DEV_DISABLE;

Here, the test is against the register number rather than the bit that
controls downshift. Shouldn't AN8855_PHY_EXT_REG_14 be
AN8855_PHY_EN_DOWN_SH(F)I(F)T ?

> +static int an8855_set_downshift(struct phy_device *phydev, u8 cnt)
> +{
> +	int saved_page;
> +	int ret;
> +
> +	saved_page = phy_select_page(phydev, AN8855_PHY_PAGE_EXTENDED_1);
> +	if (saved_page >= 0) {
> +		if (cnt != DOWNSHIFT_DEV_DISABLE)
> +			ret = __phy_set_bits(phydev, AN8855_PHY_EXT_REG_14,
> +					     AN8855_PHY_EN_DOWN_SHFIT);
> +		else
> +			ret = __phy_clear_bits(phydev, AN8855_PHY_EXT_REG_14,
> +					       AN8855_PHY_EN_DOWN_SHFIT);
> +	}
> +
> +	return phy_restore_page(phydev, saved_page, ret);

This entire thing can be simplified to:

	u16 ds = cnt != DOWNSHIFT_DEV_DISABLE ? AN8855_PHY_EN_DOWN_SHFIT: 0;

	return phy_modify_paged(phydev, AN8855_PHY_PAGE_EXTENDED_1,
				AN8855_PHY_EXT_REG_14, AN8855_PHY_EN_DOWN_SHFIT,
				ds);

Thanks.

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ