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: <87y1dr75j0.fsf@waldekranz.com>
Date: Mon, 18 Dec 2023 19:02:43 +0100
From: Tobias Waldekranz <tobias@...dekranz.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
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 fre, dec 15, 2023 at 15:44, "Russell King (Oracle)" <linux@...linux.org.uk> wrote:
> 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.

Right, I'll remove the entry for 1000BASE-X in v2.

>> +		{ 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.

Yes, I'll fix this in v2.

> 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.

This will be very hard for me to test, as I only have access to boards
without dedicated FLASHes. That said, I don't think I understand how
this is related to how the devices load their firmware. As I understand
it, we should pick up the device in the exact same state after the MDIO
load as we would if it had booted on its own, via a serial FLASH.

The selection of PDSTATE, based on the sample-at-reset pins, is
independent of how firmware is loaded.

>From the manual:

    The following registers can be set to force the units to power down.

I interpret this as the power-down bits only acting as gates to stop
firmware from powering up a particular unit. Conversely, clearing one of
these bits merely indicates that the firmware is free to power up the
unit in question.

On a device strapped to PDSTATE==0, I would expect all of these bits to
already be cleared, since the manual states that the value of PDSTATE is
latched into all these bits at reset.