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

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.

Signed-off-by: Jaedon Shin <redacted>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.h | 1 +
 drivers/net/ethernet/broadcom/genet/bcmmii.c   | 9 ++++++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index 0f0868c56f05..1e2dc34d331a 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h

@@ -597,6 +597,7 @@ struct bcmgenet_priv {
 
 	/* MDIO bus variables */
 	wait_queue_head_t wq;
+	struct phy_device *phydev;
 	bool internal_phy;
 	struct device_node *phy_dn;
 	struct device_node *mdio_dn;

diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index e907acd81da9..b2bd5302c478 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c

@@ -183,9 +183,9 @@ void bcmgenet_mii_reset(struct net_device *dev)
 	if (GENET_IS_V4(priv))
 		return;
 
-	if (dev->phydev) {
-		phy_init_hw(dev->phydev);
-		phy_start_aneg(dev->phydev);
+	if (priv->phydev) {
+		phy_init_hw(priv->phydev);
+		phy_start_aneg(priv->phydev);
 	}
 }
 

@@ -383,6 +383,8 @@ int bcmgenet_mii_probe(struct net_device *dev)
 		}
 	}
 
+	priv->phydev = phydev;
+
 	/* Configure port multiplexer based on what the probed PHY device since
 	 * reading the 'max-speed' property determines the maximum supported
 	 * PHY speed which is needed for bcmgenet_mii_config() to configure

@@ -605,6 +607,7 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
 
 	}
 
+	priv->phydev = phydev;
 	priv->phy_interface = pd->phy_interface;
 
 	return 0;

-- 
2.10.0

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

Hi Andrew,

On 23 Sep 2016, at 11:06 PM, Andrew Lunn [off-list ref] wrote:

On Fri, Sep 23, 2016 at 10:20:04PM +0900, Jaedon Shin wrote:

quoted

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.

Additionally,
http://www.spinics.net/lists/netdev/msg350506.html

Thanks,
Jaedon

On 09/23/2016 08:04 AM, Jaedon Shin wrote:

Hi Andrew,

On 23 Sep 2016, at 11:06 PM, Andrew Lunn [off-list ref] wrote:

quoted

On Fri, Sep 23, 2016 at 10:20:04PM +0900, Jaedon Shin wrote:

quoted

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

Hi Florian,

On 24 Sep 2016, at 1:54 AM, Florian Fainelli [off-list ref] wrote:

On 09/23/2016 08:04 AM, Jaedon Shin wrote:

quoted

Hi Andrew,

On 23 Sep 2016, at 11:06 PM, Andrew Lunn [off-list ref] wrote:

quoted

On Fri, Sep 23, 2016 at 10:20:04PM +0900, Jaedon Shin wrote:

quoted

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.

Will send revert patch.

Thanks,
Jaedon

Thanks!

Thanks!
-- 
Florian