| Message ID | 1334750537-14896-1-git-send-email-jonas.gorski@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Le 04/18/12 14:02, Jonas Gorski a écrit : > Only connect/disconnect the phy during probe and remove, not during open > and close. The phy seldom changes during the runtime, and disconnecting > the phy during close will prevent the phy driver from keeping any > configuration over a down/up cycle. > > Signed-off-by: Jonas Gorski<jonas.gorski@gmail.com> Acked-by: Florian Fainelli <florian@openwrt.org> > --- > drivers/net/ethernet/broadcom/bcm63xx_enet.c | 84 +++++++++++++------------- > 1 files changed, 41 insertions(+), 43 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/bcm63xx_enet.c b/drivers/net/ethernet/broadcom/bcm63xx_enet.c > index c7ca7ec..2744cf0 100644 > --- a/drivers/net/ethernet/broadcom/bcm63xx_enet.c > +++ b/drivers/net/ethernet/broadcom/bcm63xx_enet.c > @@ -784,10 +784,8 @@ static int bcm_enet_open(struct net_device *dev) > struct bcm_enet_priv *priv; > struct sockaddr addr; > struct device *kdev; > - struct phy_device *phydev; > int i, ret; > unsigned int size; > - char phy_id[MII_BUS_ID_SIZE + 3]; > void *p; > u32 val; > > @@ -795,40 +793,10 @@ static int bcm_enet_open(struct net_device *dev) > kdev =&priv->pdev->dev; > > if (priv->has_phy) { > - /* connect to PHY */ > - snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT, > - priv->mii_bus->id, priv->phy_id); > - > - phydev = phy_connect(dev, phy_id, bcm_enet_adjust_phy_link, 0, > - PHY_INTERFACE_MODE_MII); > - > - if (IS_ERR(phydev)) { > - dev_err(kdev, "could not attach to PHY\n"); > - return PTR_ERR(phydev); > - } > - > - /* mask with MAC supported features */ > - phydev->supported&= (SUPPORTED_10baseT_Half | > - SUPPORTED_10baseT_Full | > - SUPPORTED_100baseT_Half | > - SUPPORTED_100baseT_Full | > - SUPPORTED_Autoneg | > - SUPPORTED_Pause | > - SUPPORTED_MII); > - phydev->advertising = phydev->supported; > - > - if (priv->pause_auto&& priv->pause_rx&& priv->pause_tx) > - phydev->advertising |= SUPPORTED_Pause; > - else > - phydev->advertising&= ~SUPPORTED_Pause; > - > - dev_info(kdev, "attached PHY at address %d [%s]\n", > - phydev->addr, phydev->drv->name); > - > + /* Reset state */ > priv->old_link = 0; > priv->old_duplex = -1; > priv->old_pause = -1; > - priv->phydev = phydev; > } > > /* mask all interrupts and request them */ > @@ -838,7 +806,7 @@ static int bcm_enet_open(struct net_device *dev) > > ret = request_irq(dev->irq, bcm_enet_isr_mac, 0, dev->name, dev); > if (ret) > - goto out_phy_disconnect; > + return ret; > > ret = request_irq(priv->irq_rx, bcm_enet_isr_dma, IRQF_DISABLED, > dev->name, dev); > @@ -1025,9 +993,6 @@ out_freeirq_rx: > out_freeirq: > free_irq(dev->irq, dev); > > -out_phy_disconnect: > - phy_disconnect(priv->phydev); > - > return ret; > } > > @@ -1132,12 +1097,6 @@ static int bcm_enet_stop(struct net_device *dev) > free_irq(priv->irq_rx, dev); > free_irq(dev->irq, dev); > > - /* release phy */ > - if (priv->has_phy) { > - phy_disconnect(priv->phydev); > - priv->phydev = NULL; > - } > - > return 0; > } > > @@ -1714,6 +1673,8 @@ static int __devinit bcm_enet_probe(struct platform_device *pdev) > > /* MII bus registration */ > if (priv->has_phy) { > + struct phy_device *phydev; > + char phy_id[MII_BUS_ID_SIZE + 3]; > > priv->mii_bus = mdiobus_alloc(); > if (!priv->mii_bus) { > @@ -1750,6 +1711,38 @@ static int __devinit bcm_enet_probe(struct platform_device *pdev) > dev_err(&pdev->dev, "unable to register mdio bus\n"); > goto out_free_mdio; > } > + > + /* connect to PHY */ > + snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT, > + priv->mii_bus->id, priv->phy_id); > + > + phydev = phy_connect(dev, phy_id, bcm_enet_adjust_phy_link, 0, > + PHY_INTERFACE_MODE_MII); > + > + if (IS_ERR(phydev)) { > + dev_err(&pdev->dev, "could not attach to PHY\n"); > + goto out_unregister_mdio; > + } > + > + /* mask with MAC supported features */ > + phydev->supported&= (SUPPORTED_10baseT_Half | > + SUPPORTED_10baseT_Full | > + SUPPORTED_100baseT_Half | > + SUPPORTED_100baseT_Full | > + SUPPORTED_Autoneg | > + SUPPORTED_Pause | > + SUPPORTED_MII); > + phydev->advertising = phydev->supported; > + > + if (priv->pause_auto&& priv->pause_rx&& priv->pause_tx) > + phydev->advertising |= SUPPORTED_Pause; > + else > + phydev->advertising&= ~SUPPORTED_Pause; > + > + dev_info(&pdev->dev, "attached PHY at address %d [%s]\n", > + phydev->addr, phydev->drv->name); > + > + priv->phydev = phydev; > } else { > > /* run platform code to initialize PHY device */ > @@ -1795,6 +1788,9 @@ static int __devinit bcm_enet_probe(struct platform_device *pdev) > return 0; > > out_unregister_mdio: > + if (!IS_ERR_OR_NULL(priv->phydev)) > + phy_disconnect(priv->phydev); > + > if (priv->mii_bus) { > mdiobus_unregister(priv->mii_bus); > kfree(priv->mii_bus->irq); > @@ -1845,6 +1841,8 @@ static int __devexit bcm_enet_remove(struct platform_device *pdev) > enet_writel(priv, 0, ENET_MIISC_REG); > > if (priv->has_phy) { > + phy_disconnect(priv->phydev); > + priv->phydev = NULL; > mdiobus_unregister(priv->mii_bus); > kfree(priv->mii_bus->irq); > mdiobus_free(priv->mii_bus); -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2012-04-18 at 14:02 +0200, Jonas Gorski wrote: > Only connect/disconnect the phy during probe and remove, not during open > and close. The phy seldom changes during the runtime, and disconnecting > the phy during close will prevent the phy driver from keeping any > configuration over a down/up cycle. > > Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> please CC me, I wrote this driver > - phydev = phy_connect(dev, phy_id, bcm_enet_adjust_phy_link, 0, > - PHY_INTERFACE_MODE_MII); bcm_enet_adjust_link() may modify some dma registers that are reset by bcm_enet_open(), since it can now be called after probe, we may end up with broken flow control depending on whatever was called first.
Hi Maxime, On 18 April 2012 14:48, Maxime Bizon <mbizon@freebox.fr> wrote: > > On Wed, 2012-04-18 at 14:02 +0200, Jonas Gorski wrote: > >> Only connect/disconnect the phy during probe and remove, not during open >> and close. The phy seldom changes during the runtime, and disconnecting >> the phy during close will prevent the phy driver from keeping any >> configuration over a down/up cycle. >> >> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> > > please CC me, I wrote this driver Oops, sorry, that wasn't intentional. I used get_maintainer.pl, which didn't catch you, and I never checked the actual file. Duly noted for v2. > >> - phydev = phy_connect(dev, phy_id, bcm_enet_adjust_phy_link, 0, >> - PHY_INTERFACE_MODE_MII); > > bcm_enet_adjust_link() may modify some dma registers that are reset by > bcm_enet_open(), since it can now be called after probe, we may end up > with broken flow control depending on whatever was called first. I assume you mean bcm_enet_adjust_phy_link()? bcm_enet_adjust_link() gets only called if there is no phydev. I have to admit, I fail to see the race (but feel free to correct me): On boot: The phy state machine will start in PHY_READY after phy_connect, which will result in NOPs, so no call to bcm_enet_adjust_phy_link() after _probe() and before _open(). The state machine starts doing real work only after calling phy_start(), which happens only after the dma register accesses in open(). So no race here. In case of an down/up cycle: _close() will call phy_stop() first, which will either block until the current state machine run is complete if there is one, or will block the next run until it set the state to PHY_HALTED. When in PHY_HALTED, bcm_enet_adjust_phy_link() will be called once, but with phy_dev->link = 0, and only phy_dev->link = 1 can result in register writes in bcm_enet_adjust_phy_link(): if (phydev->link && phydev->duplex != priv->old_duplex) { bcm_enet_set_duplex(priv, (phydev->duplex == DUPLEX_FULL) ? 1 : 0); ... } if (phydev->link && phydev->pause != priv->old_pause) { ... bcm_enet_set_flow(priv, rx_pause_en, tx_pause_en); ... } So no problem there either. There is a theoretical race in writing to priv->old_link in _open() and the bcm_enet_adjust_phy_link() call right after phy_stop(), but since both write the same value to priv->old_link I see no problem here. Regards Jonas -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2012-04-18 at 21:30 +0200, Jonas Gorski wrote: > > The phy state machine will start in PHY_READY after phy_connect, which > will result in NOPs, so no call to bcm_enet_adjust_phy_link() after > _probe() and before _open(). The state machine starts doing real work > only after calling phy_start(), which happens only after the dma > register accesses in open(). So no race here. If I read the phylib code correctly, after phy_connect() the phy is in READY state, and you can access it via ethtool.
On 19 April 2012 15:33, Maxime Bizon <mbizon@freebox.fr> wrote: > > On Wed, 2012-04-18 at 21:30 +0200, Jonas Gorski wrote: >> >> The phy state machine will start in PHY_READY after phy_connect, which >> will result in NOPs, so no call to bcm_enet_adjust_phy_link() after >> _probe() and before _open(). The state machine starts doing real work >> only after calling phy_start(), which happens only after the dma >> register accesses in open(). So no race here. > > If I read the phylib code correctly, after phy_connect() the phy is in > READY state, and you can access it via ethtool. Yes, but none of the ethtool functions cause register writes in the priv->has_phy = true case when in PHY_READY or PHY_HALTED state. All they do is modify the phy_device's settings. The settings only get taken over when the phy is started and the phy_state_machine is running, so after phy_start() was and before phy_stop(). In the priv->has_phy = false case they do cause register writes since bcm_enet_adjust_link() might get called, but these never use the phy state machine, so my patch does not change anything there. Jonas -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2012-04-19 at 16:52 +0200, Jonas Gorski wrote: > Yes, but none of the ethtool functions cause register writes in the > priv->has_phy = true case when in PHY_READY or PHY_HALTED state. All > they do is modify the phy_device's settings. unless I'm mistaken: phy_ethtool_sset() => phy_start_aneg() will kick the state machine even when state is PHY_READY
On 19 April 2012 18:17, Maxime Bizon <mbizon@freebox.fr> wrote: > > On Thu, 2012-04-19 at 16:52 +0200, Jonas Gorski wrote: > >> Yes, but none of the ethtool functions cause register writes in the >> priv->has_phy = true case when in PHY_READY or PHY_HALTED state. All >> they do is modify the phy_device's settings. > > unless I'm mistaken: > > phy_ethtool_sset() => phy_start_aneg() > > will kick the state machine even when state is PHY_READY Hmm. I see what you mean. I wonder if it is intended that you can do that without having phy_start() called first. @Andy, can you perhaps shed some light on this? How are ethernet drivers supposed to behave/when should they call phy_connect()/phy_start()? Currently most drivers call phy_connect() in their _probe(), and phy_start() in _open(), so many seem to have the issue that the phy state machine is in PHY_READY after _probe(), and can be kicked into running through ethtool even if the interface is down. This problem goes away after the first ifup/ifdown cycle, since the phy state machine is then in PHY_HALTED, which gets properly caught in phy_start_aneg(). To me it looks like phy_start_aneg() should check for some more states, as it currently would also overwrite a PHY_STARTING or PHY_PENDING state, which looks definitely wrong to me. Jonas -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Apr 22, 2012 at 6:31 AM, Jonas Gorski <jonas.gorski@gmail.com> wrote: > On 19 April 2012 18:17, Maxime Bizon <mbizon@freebox.fr> wrote: >> >> On Thu, 2012-04-19 at 16:52 +0200, Jonas Gorski wrote: >> >>> Yes, but none of the ethtool functions cause register writes in the >>> priv->has_phy = true case when in PHY_READY or PHY_HALTED state. All >>> they do is modify the phy_device's settings. >> >> unless I'm mistaken: >> >> phy_ethtool_sset() => phy_start_aneg() >> >> will kick the state machine even when state is PHY_READY > > Hmm. I see what you mean. I wonder if it is intended that you can do > that without having phy_start() called first. > > @Andy, can you perhaps shed some light on this? How are ethernet > drivers supposed to behave/when should they call > phy_connect()/phy_start()? Currently most drivers call phy_connect() > in their _probe(), and phy_start() in _open(), so many seem to have > the issue that the phy state machine is in PHY_READY after _probe(), > and can be kicked into running through ethtool even if the interface > is down. > > This problem goes away after the first ifup/ifdown cycle, since the > phy state machine is then in PHY_HALTED, which gets properly caught in > phy_start_aneg(). > > To me it looks like phy_start_aneg() should check for some more > states, as it currently would also overwrite a PHY_STARTING or > PHY_PENDING state, which looks definitely wrong to me. Ugh, it looks like much has gone wrong with the state machine (possibly from when I wrote it). I'm thinking that what we should probably do is eliminate PHY_UP and PHY_READY, and have the PHY come up to PHY_HALTED. For some reason, PHY_RESUMING always enables interrupts, even if phydev->irq is in POLL mode, so that should be fixed. Other than that, it looks like PHY_RESUMING should work in the place of PHY_UP, and PHY_HALTED should be the same as PHY_READY... Andy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/broadcom/bcm63xx_enet.c b/drivers/net/ethernet/broadcom/bcm63xx_enet.c index c7ca7ec..2744cf0 100644 --- a/drivers/net/ethernet/broadcom/bcm63xx_enet.c +++ b/drivers/net/ethernet/broadcom/bcm63xx_enet.c @@ -784,10 +784,8 @@ static int bcm_enet_open(struct net_device *dev) struct bcm_enet_priv *priv; struct sockaddr addr; struct device *kdev; - struct phy_device *phydev; int i, ret; unsigned int size; - char phy_id[MII_BUS_ID_SIZE + 3]; void *p; u32 val; @@ -795,40 +793,10 @@ static int bcm_enet_open(struct net_device *dev) kdev = &priv->pdev->dev; if (priv->has_phy) { - /* connect to PHY */ - snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT, - priv->mii_bus->id, priv->phy_id); - - phydev = phy_connect(dev, phy_id, bcm_enet_adjust_phy_link, 0, - PHY_INTERFACE_MODE_MII); - - if (IS_ERR(phydev)) { - dev_err(kdev, "could not attach to PHY\n"); - return PTR_ERR(phydev); - } - - /* mask with MAC supported features */ - phydev->supported &= (SUPPORTED_10baseT_Half | - SUPPORTED_10baseT_Full | - SUPPORTED_100baseT_Half | - SUPPORTED_100baseT_Full | - SUPPORTED_Autoneg | - SUPPORTED_Pause | - SUPPORTED_MII); - phydev->advertising = phydev->supported; - - if (priv->pause_auto && priv->pause_rx && priv->pause_tx) - phydev->advertising |= SUPPORTED_Pause; - else - phydev->advertising &= ~SUPPORTED_Pause; - - dev_info(kdev, "attached PHY at address %d [%s]\n", - phydev->addr, phydev->drv->name); - + /* Reset state */ priv->old_link = 0; priv->old_duplex = -1; priv->old_pause = -1; - priv->phydev = phydev; } /* mask all interrupts and request them */ @@ -838,7 +806,7 @@ static int bcm_enet_open(struct net_device *dev) ret = request_irq(dev->irq, bcm_enet_isr_mac, 0, dev->name, dev); if (ret) - goto out_phy_disconnect; + return ret; ret = request_irq(priv->irq_rx, bcm_enet_isr_dma, IRQF_DISABLED, dev->name, dev); @@ -1025,9 +993,6 @@ out_freeirq_rx: out_freeirq: free_irq(dev->irq, dev); -out_phy_disconnect: - phy_disconnect(priv->phydev); - return ret; } @@ -1132,12 +1097,6 @@ static int bcm_enet_stop(struct net_device *dev) free_irq(priv->irq_rx, dev); free_irq(dev->irq, dev); - /* release phy */ - if (priv->has_phy) { - phy_disconnect(priv->phydev); - priv->phydev = NULL; - } - return 0; } @@ -1714,6 +1673,8 @@ static int __devinit bcm_enet_probe(struct platform_device *pdev) /* MII bus registration */ if (priv->has_phy) { + struct phy_device *phydev; + char phy_id[MII_BUS_ID_SIZE + 3]; priv->mii_bus = mdiobus_alloc(); if (!priv->mii_bus) { @@ -1750,6 +1711,38 @@ static int __devinit bcm_enet_probe(struct platform_device *pdev) dev_err(&pdev->dev, "unable to register mdio bus\n"); goto out_free_mdio; } + + /* connect to PHY */ + snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT, + priv->mii_bus->id, priv->phy_id); + + phydev = phy_connect(dev, phy_id, bcm_enet_adjust_phy_link, 0, + PHY_INTERFACE_MODE_MII); + + if (IS_ERR(phydev)) { + dev_err(&pdev->dev, "could not attach to PHY\n"); + goto out_unregister_mdio; + } + + /* mask with MAC supported features */ + phydev->supported &= (SUPPORTED_10baseT_Half | + SUPPORTED_10baseT_Full | + SUPPORTED_100baseT_Half | + SUPPORTED_100baseT_Full | + SUPPORTED_Autoneg | + SUPPORTED_Pause | + SUPPORTED_MII); + phydev->advertising = phydev->supported; + + if (priv->pause_auto && priv->pause_rx && priv->pause_tx) + phydev->advertising |= SUPPORTED_Pause; + else + phydev->advertising &= ~SUPPORTED_Pause; + + dev_info(&pdev->dev, "attached PHY at address %d [%s]\n", + phydev->addr, phydev->drv->name); + + priv->phydev = phydev; } else { /* run platform code to initialize PHY device */ @@ -1795,6 +1788,9 @@ static int __devinit bcm_enet_probe(struct platform_device *pdev) return 0; out_unregister_mdio: + if (!IS_ERR_OR_NULL(priv->phydev)) + phy_disconnect(priv->phydev); + if (priv->mii_bus) { mdiobus_unregister(priv->mii_bus); kfree(priv->mii_bus->irq); @@ -1845,6 +1841,8 @@ static int __devexit bcm_enet_remove(struct platform_device *pdev) enet_writel(priv, 0, ENET_MIISC_REG); if (priv->has_phy) { + phy_disconnect(priv->phydev); + priv->phydev = NULL; mdiobus_unregister(priv->mii_bus); kfree(priv->mii_bus->irq); mdiobus_free(priv->mii_bus);
Only connect/disconnect the phy during probe and remove, not during open and close. The phy seldom changes during the runtime, and disconnecting the phy during close will prevent the phy driver from keeping any configuration over a down/up cycle. Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> --- drivers/net/ethernet/broadcom/bcm63xx_enet.c | 84 +++++++++++++------------- 1 files changed, 41 insertions(+), 43 deletions(-)