| Message ID | 20160923132004.5734-1-jaedon.shin@gmail.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
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 <andrew@lunn.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. 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 <andrew@lunn.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!
Hi Florian, > On 24 Sep 2016, at 1:54 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: > > On 09/23/2016 08:04 AM, Jaedon Shin wrote: >> Hi Andrew, >> >> On 23 Sep 2016, at 11:06 PM, Andrew Lunn <andrew@lunn.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. > Will send revert patch. Thanks, Jaedon > Thanks! > > Thanks! > -- > Florian
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;
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 <jaedon.shin@gmail.com> --- drivers/net/ethernet/broadcom/genet/bcmgenet.h | 1 + drivers/net/ethernet/broadcom/genet/bcmmii.c | 9 ++++++--- 2 files changed, 7 insertions(+), 3 deletions(-)