netdev - Re: [PATCH] net: bcmgenet: Fix EPHY reset in power up

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]

Date:   Fri, 23 Sep 2016 09:54:35 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Jaedon Shin <jaedon.shin@...il.com>, Andrew Lunn <andrew@...n.ch>
Cc:     David Miller <davem@...emloft.net>,
        Philippe Reynes <tremyfr@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH] net: bcmgenet: Fix EPHY reset in power up

On 09/23/2016 08:04 AM, Jaedon Shin wrote:
> Hi Andrew,
> 
> On 23 Sep 2016, at 11:06 PM, Andrew Lunn <andrew@...n.ch> wrote:
>>
>> On Fri, Sep 23, 2016 at 10:20:04PM +0900, Jaedon Shin wrote:
>>> The bcmgenet_mii_reset() is always not running in power up sequence
>>> after 'commit 62469c76007e ("net: ethernet: bcmgenet: use phydev from
>>> struct net_device")'. This'll show extremely high latency and duplicate
>>> packets while interface down and up repeatedly.
>>>
>>> For now, adds again a private phydev for mii reset when runs power up to
>>> open interface.
>>
>> Hi Jaedon
>>
>> How does this fix the issue? It sounds like you are papering over the
>> crack, not truly fixing it.
>>
>>       Andrew
> 
> Yes, It feel like a workaround, but I think it must need v4.8 stable
> version. If we find better way that fixes internal PHY to initialize
> after re-open interface, this patch will be dropped.

I can observe the faulting behavior with 4.8-rc7 that the link below
fixed initially:

# ping fainelli-linux
PING fainelli-linux (10.112.156.244): 56 data bytes
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.352 ms
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.472 ms (DUP!)
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.496 ms (DUP!)
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.517 ms (DUP!)
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.536 ms (DUP!)
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.557 ms (DUP!)
64 bytes from 10.112.156.244: seq=1 ttl=61 time=752.448 ms (DUP!)
64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.291 ms
64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.421 ms (DUP!)
64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.444 ms (DUP!)
64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.464 ms (DUP!)
64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.483 ms (DUP!)
64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.505 ms (DUP!)
64 bytes from 10.112.156.244: seq=2 ttl=61 time=24.964 ms (DUP!)

If we revert this patch, we indeed get the normal and expected behavior
back:

# ping fainelli-linux
PING fainelli-linux (10.112.156.244): 56 data bytes
64 bytes from 10.112.156.244: seq=0 ttl=61 time=0.417 ms
64 bytes from 10.112.156.244: seq=1 ttl=61 time=0.415 ms
64 bytes from 10.112.156.244: seq=2 ttl=61 time=0.424 ms

Actually, the key thing is this:

- without Philippe's patch we call twice bcmgenet_mii_reset, and that is
intended:
	- first time from bcmgenet_power_up() to make sure the PHY is
initialized *before* we get to initialize the UniMAC, this is critical
	- second time from bcmgenet_mii_probe(), through the normal phy_init_hw()

- with Philippe's patch, we only get to call bcmgenet_mii_reset once, in
bcmgenet_mii_probe() because the first time in bcmgenet_power_up(),
dev->phydev is NULL, because of a prior call to phy_disconnect() in
bcmgenet_close(), unfortunately, there has been MAC activity, so the PHY
gets in a bad state

Jaedon, feel free to use the explanation above, and send a plain revert
of commit 62469c76007e11428e2ee3c6de90cbe74b588d44.

Thanks!

Thanks!
-- 
Florian