| Message ID | 1271917018-5756-1-git-send-email-bryan.wu@canonical.com |
|---|---|
| State | Superseded |
| Delegated to: | Andy Whitcroft |
| Headers | show |
On 04/22/2010 12:16 AM, Bryan Wu wrote: > BugLink: http://bugs.launchpad.net/bugs/559065 > > v3: > if fec_enet_mii_probe() fails, we should exit the open and resume functions > properly and give up further operations without a proper PHY connected. > > v2: > Check fec_enet_mii_probe() return value directly, instead of fep->phy_dev > > v1: > In ethernet connection open/close function, we need to use phy_connect > and phy_disconnect operation before we start/stop phy. Otherwise it will > cause system hang. Also suspend/resume needs the same fix. > > Signed-off-by: Bryan Wu<bryan.wu@canonical.com> > --- > drivers/net/fec.c | 21 ++++++++++++++++++++- > 1 files changed, 20 insertions(+), 1 deletions(-) > > diff --git a/drivers/net/fec.c b/drivers/net/fec.c > index 2280373..b14e4d5 100644 > --- a/drivers/net/fec.c > +++ b/drivers/net/fec.c > @@ -929,7 +929,13 @@ fec_enet_open(struct net_device *dev) > return ret; > > /* schedule a link state check */ > + ret = fec_enet_mii_probe(dev); > + if (!ret) { > + fec_enet_free_buffers(dev); > + return ret; > + } > phy_start(fep->phy_dev); > + > netif_start_queue(dev); > fep->opened = 1; > return 0; > @@ -942,10 +948,13 @@ fec_enet_close(struct net_device *dev) > > /* Don't know what to do yet. */ > fep->opened = 0; > - phy_stop(fep->phy_dev); > netif_stop_queue(dev); > + > fec_stop(dev); > > + if (fep->phy_dev) > + phy_disconnect(fep->phy_dev); > + > fec_enet_free_buffers(dev); > clk_disable(fep->clk); > > @@ -1396,6 +1405,8 @@ fec_suspend(struct platform_device *dev, pm_message_t state) > fep = netdev_priv(ndev); > if (netif_running(ndev)) { > netif_device_detach(ndev); > + if (fep->phy_dev) > + phy_disconnect(fep->phy_dev); > fec_stop(ndev); > clk_disable(fep->clk); > } > @@ -1409,12 +1420,20 @@ fec_resume(struct platform_device *dev) > { > struct net_device *ndev = platform_get_drvdata(dev); > struct fec_enet_private *fep; > + int ret; > > if (ndev) { > fep = netdev_priv(ndev); > if (netif_running(ndev)) { > clk_enable(fep->clk); > fec_restart(ndev, 1); > + ret = fec_enet_mii_probe(ndev); > + if (!ret) { > + fec_stop(ndev); > + clk_disable(fep->clk); > + return ret; > + } > + phy_start(fep->phy_dev); > netif_device_attach(ndev); > } > } One final comment. fec_enet_mii_probe() should set fep->phy_dev=NULL upon entry so that if it errors out fep->phy_dev is not left with a bogus value (which could happen in fec_resume()). How is the fsl-imx51 physically constructed wrt to the ethernet phy? Is it possible for the phy to disappear during a suspend/resume cycle? rtg
On 04/22/2010 08:57 PM, Tim Gardner wrote: > On 04/22/2010 12:16 AM, Bryan Wu wrote: >> BugLink: http://bugs.launchpad.net/bugs/559065 >> >> v3: >> if fec_enet_mii_probe() fails, we should exit the open and resume >> functions >> properly and give up further operations without a proper PHY connected. >> >> v2: >> Check fec_enet_mii_probe() return value directly, instead of fep->phy_dev >> >> v1: >> In ethernet connection open/close function, we need to use phy_connect >> and phy_disconnect operation before we start/stop phy. Otherwise it will >> cause system hang. Also suspend/resume needs the same fix. >> >> Signed-off-by: Bryan Wu<bryan.wu@canonical.com> >> --- >> drivers/net/fec.c | 21 ++++++++++++++++++++- >> 1 files changed, 20 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/net/fec.c b/drivers/net/fec.c >> index 2280373..b14e4d5 100644 >> --- a/drivers/net/fec.c >> +++ b/drivers/net/fec.c >> @@ -929,7 +929,13 @@ fec_enet_open(struct net_device *dev) >> return ret; >> >> /* schedule a link state check */ >> + ret = fec_enet_mii_probe(dev); >> + if (!ret) { >> + fec_enet_free_buffers(dev); >> + return ret; >> + } >> phy_start(fep->phy_dev); >> + >> netif_start_queue(dev); >> fep->opened = 1; >> return 0; >> @@ -942,10 +948,13 @@ fec_enet_close(struct net_device *dev) >> >> /* Don't know what to do yet. */ >> fep->opened = 0; >> - phy_stop(fep->phy_dev); >> netif_stop_queue(dev); >> + >> fec_stop(dev); >> >> + if (fep->phy_dev) >> + phy_disconnect(fep->phy_dev); >> + >> fec_enet_free_buffers(dev); >> clk_disable(fep->clk); >> >> @@ -1396,6 +1405,8 @@ fec_suspend(struct platform_device *dev, >> pm_message_t state) >> fep = netdev_priv(ndev); >> if (netif_running(ndev)) { >> netif_device_detach(ndev); >> + if (fep->phy_dev) >> + phy_disconnect(fep->phy_dev); >> fec_stop(ndev); >> clk_disable(fep->clk); >> } >> @@ -1409,12 +1420,20 @@ fec_resume(struct platform_device *dev) >> { >> struct net_device *ndev = platform_get_drvdata(dev); >> struct fec_enet_private *fep; >> + int ret; >> >> if (ndev) { >> fep = netdev_priv(ndev); >> if (netif_running(ndev)) { >> clk_enable(fep->clk); >> fec_restart(ndev, 1); >> + ret = fec_enet_mii_probe(ndev); >> + if (!ret) { >> + fec_stop(ndev); >> + clk_disable(fep->clk); >> + return ret; >> + } >> + phy_start(fep->phy_dev); >> netif_device_attach(ndev); >> } >> } > > One final comment. fec_enet_mii_probe() should set fep->phy_dev=NULL > upon entry so that if it errors out fep->phy_dev is not left with a > bogus value (which could happen in fec_resume()). > Yeah, good point. I will provide another patch to add this. > How is the fsl-imx51 physically constructed wrt to the ethernet phy? Is > it possible for the phy to disappear during a suspend/resume cycle? > Actually, FEC is an on-chip controller of imx51 and PHY is an external chip which connected to imx51 via MDIO and MII bus. Since suspend/resume operation is very similar to close/open thing here, I just wanna keep them similar. Thanks,
diff --git a/drivers/net/fec.c b/drivers/net/fec.c index 2280373..b14e4d5 100644 --- a/drivers/net/fec.c +++ b/drivers/net/fec.c @@ -929,7 +929,13 @@ fec_enet_open(struct net_device *dev) return ret; /* schedule a link state check */ + ret = fec_enet_mii_probe(dev); + if (!ret) { + fec_enet_free_buffers(dev); + return ret; + } phy_start(fep->phy_dev); + netif_start_queue(dev); fep->opened = 1; return 0; @@ -942,10 +948,13 @@ fec_enet_close(struct net_device *dev) /* Don't know what to do yet. */ fep->opened = 0; - phy_stop(fep->phy_dev); netif_stop_queue(dev); + fec_stop(dev); + if (fep->phy_dev) + phy_disconnect(fep->phy_dev); + fec_enet_free_buffers(dev); clk_disable(fep->clk); @@ -1396,6 +1405,8 @@ fec_suspend(struct platform_device *dev, pm_message_t state) fep = netdev_priv(ndev); if (netif_running(ndev)) { netif_device_detach(ndev); + if (fep->phy_dev) + phy_disconnect(fep->phy_dev); fec_stop(ndev); clk_disable(fep->clk); } @@ -1409,12 +1420,20 @@ fec_resume(struct platform_device *dev) { struct net_device *ndev = platform_get_drvdata(dev); struct fec_enet_private *fep; + int ret; if (ndev) { fep = netdev_priv(ndev); if (netif_running(ndev)) { clk_enable(fep->clk); fec_restart(ndev, 1); + ret = fec_enet_mii_probe(ndev); + if (!ret) { + fec_stop(ndev); + clk_disable(fep->clk); + return ret; + } + phy_start(fep->phy_dev); netif_device_attach(ndev); } }
BugLink: http://bugs.launchpad.net/bugs/559065 v3: if fec_enet_mii_probe() fails, we should exit the open and resume functions properly and give up further operations without a proper PHY connected. v2: Check fec_enet_mii_probe() return value directly, instead of fep->phy_dev v1: In ethernet connection open/close function, we need to use phy_connect and phy_disconnect operation before we start/stop phy. Otherwise it will cause system hang. Also suspend/resume needs the same fix. Signed-off-by: Bryan Wu <bryan.wu@canonical.com> --- drivers/net/fec.c | 21 ++++++++++++++++++++- 1 files changed, 20 insertions(+), 1 deletions(-)