netdev - Re: [PATCH net-next 2/4] net: phy: marvell10g: Fix power-up when strapped to start powered down

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: <ZXx0eVzJ3I1PwOa0@shell.armlinux.org.uk>
Date: Fri, 15 Dec 2023 15:44:57 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Tobias Waldekranz <tobias@...dekranz.com>
Cc: davem@...emloft.net, kuba@...nel.org, kabel@...nel.org, andrew@...n.ch,
	hkallweit1@...il.com, robh+dt@...nel.org,
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
	netdev@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH net-next 2/4] net: phy: marvell10g: Fix power-up when
 strapped to start powered down

On Thu, Dec 14, 2023 at 09:14:40PM +0100, Tobias Waldekranz wrote:
> On devices which are hardware strapped to start powered down (PDSTATE
> == 1), make sure that we clear the power-down bit on all units
> affected by this setting.
> 
> Signed-off-by: Tobias Waldekranz <tobias@...dekranz.com>
> ---
>  drivers/net/phy/marvell10g.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index 83233b30d7b0..1c1333d867fb 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -344,11 +344,22 @@ static int mv3310_power_down(struct phy_device *phydev)
>  
>  static int mv3310_power_up(struct phy_device *phydev)
>  {
> +	static const u16 resets[][2] = {
> +		{ MDIO_MMD_PCS,    MV_PCS_BASE_R    + MDIO_CTRL1 },
> +		{ MDIO_MMD_PCS,    MV_PCS_1000BASEX + MDIO_CTRL1 },

This is not necessary. The documentation states that the power down
bit found at each of these is the same physical bit appearing in two
different locations. So only one is necessary.

> +		{ MDIO_MMD_PCS,    MV_PCS_BASE_T    + MDIO_CTRL1 },
> +		{ MDIO_MMD_PMAPMD, MDIO_CTRL1 },
> +		{ MDIO_MMD_VEND2,  MV_V2_PORT_CTRL },
> +	};
>  	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
> -	int ret;
> +	int i, ret;
>  
> -	ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
> -				 MV_V2_PORT_CTRL_PWRDOWN);
> +	for (i = 0; i < ARRAY_SIZE(resets); i++) {
> +		ret = phy_clear_bits_mmd(phydev, resets[i][0], resets[i][1],
> +					 MV_V2_PORT_CTRL_PWRDOWN);

While MV_V2_PORT_CTRL_PWRDOWN may correspond with the correct bit for
the MDIO CTRL1 register, we have MDIO_CTRL1_LPOWER which describes
this bit. Probably the simplest solution would be to leave the
existing phy_clear_bits_mmd(), remove the vendor 2 entry from the
table, and run through that table first.

Lastly, how does this impact a device which has firmware, and the
firmware manages the power-down state (the manual states that unused
blocks will be powered down - I assume by the firmware.) If this
causes blocks which had been powered down by the firmware because
they're not being used to then be powered up, that is a regression.
Please check that this is not the case.

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