netdev - Re: [PATCH] net: phy: Only resume phy if it is suspended

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 7 Dec 2023 09:56:01 -0800
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: Justin Chen <justin.chen@...adcom.com>, Andrew Lunn <andrew@...n.ch>,
 Doug Berger <opendmb@...il.com>
Cc: netdev@...r.kernel.org, bcm-kernel-feedback-list@...adcom.com,
 Heiner Kallweit <hkallweit1@...il.com>, Russell King
 <linux@...linux.org.uk>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net: phy: Only resume phy if it is suspended

Hi Andrew,

On 12/5/23 16:12, Florian Fainelli wrote:
> On 12/5/23 16:10, Justin Chen wrote:
>>
>>
>> On 12/5/23 4:03 PM, Andrew Lunn wrote:
>>> On Tue, Dec 05, 2023 at 03:42:29PM -0800, Justin Chen wrote:
>>>> Resuming the phy can take quite a bit of time. Lets only resume the
>>>> phy if it is suspended.
>>>
>>> Humm...
>>>
>>> https://lore.kernel.org/netdev/6d45f4da-c45e-4d35-869f-85dd4ec37b31@lunn.ch/T/
>>>
>>> If Broadcom PHYs are slow to resume, maybe you should solve this in
>>> the broadcom resume handler, read the status from the hardware and
>>> only do the resume if the hardware is suspended.
>>>
>>>       Andrew
>>
>> Right... Guess this won't work. It is odd that during resume we call 
>> __phy_resume twice. Once from phy_resume() and another at phy_start(). 
>> Let me rethink this. Thanks for the feedback.
> 
> This might be something for us to figure out on the driver side, I think 
> historically I have always followed the pattern of doing:
> 
> phy_suspend()
> phy_stop()
> 
> and
> 
> phy_resume()
> phy_start()
> 
> because it used to be necessary to do that way back when...

So we discussed with Justin and Doug about this yesterday and the main 
reason for the phy_resume() ... MAC initialization ... phy_start() 
pattern has to do with external RGMII PHYs and the clocking dependency 
between the MAC and the PHY on the RX path. And also a tiny bit of cargo 
culting, but shhh.

When the external RGMII PHYs are suspended they will stop providing a 
RXC back to the Ethernet MAC and our Ethernet MAC like a lot of designs 
out there require the RXC in order to be functional and complete its 
reset procedure correctly (you would think there would be a way to mux 
in a different clock, but that does not appear to be the case). If we 
reset the UniMAC block without a RXC we will typically see duplicate 
packets being received, or absurdly long round trip times as soon as we 
try to use the RX path.

Doug lifted that requirement in GENET with:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=88f6c8bf1aaed5039923fb4c701cab4d42176275

by delaying the MAC reset until we have a link UP confirmation. Since 
this is the same UniMAC design in the bcmasp driver we should be able to 
apply the same strategy and remove the initial phy_resume().

There are also other opportunities for avoiding link disruption upon 
suspend/resume when Wake-on-LAN is enabled and avoid a re-negotiation of 
the link, though that's for another set of changes.
-- 
Florian

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4221 bytes)