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] [day] [month] [year] [list]

Date:   Wed, 10 Mar 2021 19:31:20 -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 2:56 PM, Florian Fainelli wrote:
> 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.

I re-tested with different kernels and was convinced that the
problem I saw due to the lack of locking in the 4.9 kernel around
phydrv->suspend() since 4.9 was where I started working on this. It
turns out this was reproducible with different kernel versions as well
so there is really something that is possibly specific to the BCM54810
PHY and the specific design.

I will run more tests to get to the bottom of this hopefully.
-- 
Florian