netdev - Re: [PATCH net 2/3] net: phy: broadcom: Only set BMCR.PDOWN to suspend

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: <ddb8b1f3-73ad-0d63-6022-69046cb15497@gmail.com>
Date:   Wed, 10 Mar 2021 14:56:08 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Heiner Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org
Cc:     andrew@...n.ch, kuba@...nel.org, davem@...emloft.net
Subject: Re: [PATCH net 2/3] net: phy: broadcom: Only set BMCR.PDOWN to
 suspend

On 3/10/21 1:43 PM, Heiner Kallweit wrote:
> On 10.03.2021 22:15, Florian Fainelli wrote:
>> On 3/10/21 1:07 PM, Heiner Kallweit wrote:
>>> On 10.03.2021 21:41, Florian Fainelli wrote:
>>>> B50212E PHYs have been observed to get into an incorrect state with the
>>>> visible effect of having both activity and link LEDs flashing
>>>> alternatively instead of being turned off as intended when
>>>> genphy_suspend() was issued. The BCM54810 is a similar design and
>>>> equally suffers from that issue.
>>>>
>>>> The datasheet is not particularly clear whether a read/modify/write
>>>> sequence is acceptable and only indicates that BMCR.PDOWN=1 should be
>>>> utilized to enter the power down mode. When this was done the PHYs were
>>>> always measured to have power levels that match the expectations and
>>>> LEDs powered off.
>>>>
>>>> Fixes: fe26821fa614 ("net: phy: broadcom: Wire suspend/resume for BCM54810")
>>>> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
>>>> ---
>>>>  drivers/net/phy/broadcom.c | 17 ++++++++++++++++-
>>>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
>>>> index b8eb736fb456..b33ffd44f799 100644
>>>> --- a/drivers/net/phy/broadcom.c
>>>> +++ b/drivers/net/phy/broadcom.c
>>>> @@ -388,6 +388,21 @@ static int bcm54xx_config_init(struct phy_device *phydev)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static int bcm54xx_suspend(struct phy_device *phydev)
>>>> +{
>>>> +	/* We cannot perform a read/modify/write like what genphy_suspend()
>>>> +	 * does because depending on the time we can observe the PHY having
>>>> +	 * both of its LEDs flashing indicating that it is in an incorrect
>>>> +	 * state and not powered down as expected.
>>>> +	 *
>>>> +	 * There is not a clear indication in the datasheet whether a
>>>> +	 * read/modify/write would be acceptable, but a blind write to the
>>>> +	 * register has been proven to be functional unlike the
>>>> +	 * Read/Modify/Write.
>>>> +	 */
>>>> +	return phy_write(phydev, MII_BMCR, BMCR_PDOWN);
>>>
>>> This clears all other bits in MII_BMCR, incl. ANENABLE and the ones used in
>>> forced mode. So you have to rely on somebody calling genphy_config_aneg()
>>> to sync the register bits with the values cached in struct phy_device
>>> on resume. Typically the phylib state machine takes care, but do we have
>>> to consider use cases where this is not the case?
>>
>> Good point, how about if we had forced the link before suspending, does
>> PHYLIB take care of re-applying the same parameters? It arguably should
>> do that in all cases given that power to the PHY can be cut depending on
>> the suspend mode.
>>
> 
> When entering power-down mode the link is lost and we go to HALTED state.
> On resume, phy_start() sets UP state and state machine calls
> phy_start_aneg(), which takes care of syncing the BMCR forced mode bits.
> A potential issue arises if we have a driver that doesn't use the
> phylib state machine and prefers to do it on its own.
> IIRC I once stumbled across this when I also relied on the phylib state
> machine running in a change.
> I'm not sure whether we can run into a problem, but it's worth spending
> a thought before somebody complains after applying the change.

That is a fair point, I could save the BMCR before modifying it and
restore it in bcm54xx_resume() and phy_start_aneg() should not issue an
additional re-negotiation in that case. Let me explore a bit more to
find out which of these BMCR bits makes the PHY go bonkers.

Thanks
-- 
Florian