| Message ID | 20201207113849.27930-1-qiangqing.zhang@nxp.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC] ethernet: stmmac: clean up the code for release/suspend/resume function | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Mon, 7 Dec 2020 19:38:49 +0800 Joakim Zhang wrote: > > commit 1c35cc9cf6a0 ("net: stmmac: remove redundant null check before clk_disable_unprepare()"), > have not clean up check NULL clock parameter completely, this patch did it. > > commit e8377e7a29efb ("net: stmmac: only call pmt() during suspend/resume if HW enables PMT"), > after this patch, we use > if (device_may_wakeup(priv->device) && priv->plat->pmt) check MAC wakeup > if (device_may_wakeup(priv->device)) check PHY wakeup > Add oneline comment for readability. > > commit 77b2898394e3b ("net: stmmac: Speed down the PHY if WoL to save energy"), > slow down phy speed when release net device under any condition. > > Slightly adjust the order of the codes so that suspend/resume look more > symmetrical, generally speaking they should appear symmetrically. > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > --- > .../net/ethernet/stmicro/stmmac/stmmac_main.c | 22 +++++++++---------- > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index c33db79cdd0a..a46e865c4acc 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -2908,8 +2908,7 @@ static int stmmac_release(struct net_device *dev) > struct stmmac_priv *priv = netdev_priv(dev); > u32 chan; > > - if (device_may_wakeup(priv->device)) This check is to prevent link speed down if the stmmac isn't a wakeup device. > - phylink_speed_down(priv->phylink, false); > + phylink_speed_down(priv->phylink, false); > /* Stop and disconnect the PHY */ > phylink_stop(priv->phylink); > phylink_disconnect_phy(priv->phylink); > @@ -5183,6 +5182,7 @@ int stmmac_suspend(struct device *dev) > } else { > mutex_unlock(&priv->lock); > rtnl_lock(); > + /* For PHY wakeup case */ > if (device_may_wakeup(priv->device)) > phylink_speed_down(priv->phylink, false); > phylink_stop(priv->phylink); > @@ -5260,11 +5260,17 @@ int stmmac_resume(struct device *dev) > /* enable the clk previously disabled */ > clk_prepare_enable(priv->plat->stmmac_clk); > clk_prepare_enable(priv->plat->pclk); > - if (priv->plat->clk_ptp_ref) > - clk_prepare_enable(priv->plat->clk_ptp_ref); > + clk_prepare_enable(priv->plat->clk_ptp_ref); I think this 3 line modifications can be a separated patch. > /* reset the phy so that it's ready */ > if (priv->mii) > stmmac_mdio_reset(priv->mii); > + > + rtnl_lock(); > + phylink_start(priv->phylink); > + /* We may have called phylink_speed_down before */ > + if (device_may_wakeup(priv->device)) > + phylink_speed_up(priv->phylink); > + rtnl_unlock(); This is moving phylink op before mac setup, I'm not sure whether this is safe. > } > > if (priv->plat->serdes_powerup) { > @@ -5275,14 +5281,6 @@ int stmmac_resume(struct device *dev) > return ret; > } > > - if (!device_may_wakeup(priv->device) || !priv->plat->pmt) { > - rtnl_lock(); > - phylink_start(priv->phylink); > - /* We may have called phylink_speed_down before */ > - phylink_speed_up(priv->phylink); > - rtnl_unlock(); > - } > - > rtnl_lock(); > mutex_lock(&priv->lock); > > -- > 2.17.1 >
> -----Original Message----- > From: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > Sent: 2020年12月8日 18:24 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com; > joabreu@synopsys.com; davem@davemloft.net; kuba@kernel.org; > netdev@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH RFC] ethernet: stmmac: clean up the code for > release/suspend/resume function > > On Mon, 7 Dec 2020 19:38:49 +0800 Joakim Zhang wrote: > > > > > > commit 1c35cc9cf6a0 ("net: stmmac: remove redundant null check before > > clk_disable_unprepare()"), have not clean up check NULL clock parameter > completely, this patch did it. > > > > commit e8377e7a29efb ("net: stmmac: only call pmt() during > > suspend/resume if HW enables PMT"), after this patch, we use if > > (device_may_wakeup(priv->device) && priv->plat->pmt) check MAC wakeup > > if (device_may_wakeup(priv->device)) check PHY wakeup Add oneline > > comment for readability. > > > > commit 77b2898394e3b ("net: stmmac: Speed down the PHY if WoL to save > > energy"), slow down phy speed when release net device under any condition. > > > > Slightly adjust the order of the codes so that suspend/resume look > > more symmetrical, generally speaking they should appear symmetrically. > > > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > > --- > > .../net/ethernet/stmicro/stmmac/stmmac_main.c | 22 > > +++++++++---------- > > 1 file changed, 10 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > index c33db79cdd0a..a46e865c4acc 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > @@ -2908,8 +2908,7 @@ static int stmmac_release(struct net_device *dev) > > struct stmmac_priv *priv = netdev_priv(dev); > > u32 chan; > > > > - if (device_may_wakeup(priv->device)) > > This check is to prevent link speed down if the stmmac isn't a wakeup device. When we invoke .ndo_stop, we down the net device. Per my understanding, we can speed down the phy, no matter it is a wakeup device or not. Since when invoke .ndo_open to up the net devce, we will re-config mac and phy. Please point out to me if I mis-understand something. Thanks. > > - phylink_speed_down(priv->phylink, false); > > + phylink_speed_down(priv->phylink, false); > > /* Stop and disconnect the PHY */ > > phylink_stop(priv->phylink); > > phylink_disconnect_phy(priv->phylink); > > @@ -5183,6 +5182,7 @@ int stmmac_suspend(struct device *dev) > > } else { > > mutex_unlock(&priv->lock); > > rtnl_lock(); > > + /* For PHY wakeup case */ > > if (device_may_wakeup(priv->device)) > > phylink_speed_down(priv->phylink, false); > > phylink_stop(priv->phylink); @@ -5260,11 +5260,17 @@ > > int stmmac_resume(struct device *dev) > > /* enable the clk previously disabled */ > > clk_prepare_enable(priv->plat->stmmac_clk); > > clk_prepare_enable(priv->plat->pclk); > > - if (priv->plat->clk_ptp_ref) > > - clk_prepare_enable(priv->plat->clk_ptp_ref); > > + clk_prepare_enable(priv->plat->clk_ptp_ref); > > I think this 3 line modifications can be a separated patch. Yes, this just a RFC to export issue. > > /* reset the phy so that it's ready */ > > if (priv->mii) > > stmmac_mdio_reset(priv->mii); > > + > > + rtnl_lock(); > > + phylink_start(priv->phylink); > > + /* We may have called phylink_speed_down before */ > > + if (device_may_wakeup(priv->device)) > > + phylink_speed_up(priv->phylink); > > + rtnl_unlock(); > > This is moving phylink op before mac setup, I'm not sure whether this is safe. We encounter an issue, need move phylink before mac setup, please see below patch. https://www.spinics.net/lists/netdev/msg706458.html Have not found problems after test. Is there ang risk? Best Regards, Joakim Zhang > > } > > > > if (priv->plat->serdes_powerup) { @@ -5275,14 +5281,6 @@ int > > stmmac_resume(struct device *dev) > > return ret; > > } > > > > - if (!device_may_wakeup(priv->device) || !priv->plat->pmt) { > > - rtnl_lock(); > > - phylink_start(priv->phylink); > > - /* We may have called phylink_speed_down before */ > > - phylink_speed_up(priv->phylink); > > - rtnl_unlock(); > > - } > > - > > rtnl_lock(); > > mutex_lock(&priv->lock); > > > > -- > > 2.17.1 > >
On Tue, 8 Dec 2020 10:49:03 +0000 Joakim Zhang <qiangqing.zhang@nxp.com> wrote: > > > > -----Original Message----- > > From: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > > Sent: 2020年12月8日 18:24 > > To: Joakim Zhang <qiangqing.zhang@nxp.com> > > Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com; > > joabreu@synopsys.com; davem@davemloft.net; kuba@kernel.org; > > netdev@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > > Subject: Re: [PATCH RFC] ethernet: stmmac: clean up the code for > > release/suspend/resume function > > > > On Mon, 7 Dec 2020 19:38:49 +0800 Joakim Zhang wrote: > > > > > > > > > > commit 1c35cc9cf6a0 ("net: stmmac: remove redundant null check before > > > clk_disable_unprepare()"), have not clean up check NULL clock parameter > > completely, this patch did it. > > > > > > commit e8377e7a29efb ("net: stmmac: only call pmt() during > > > suspend/resume if HW enables PMT"), after this patch, we use if > > > (device_may_wakeup(priv->device) && priv->plat->pmt) check MAC wakeup > > > if (device_may_wakeup(priv->device)) check PHY wakeup Add oneline > > > comment for readability. > > > > > > commit 77b2898394e3b ("net: stmmac: Speed down the PHY if WoL to save > > > energy"), slow down phy speed when release net device under any condition. > > > > > > Slightly adjust the order of the codes so that suspend/resume look > > > more symmetrical, generally speaking they should appear symmetrically. > > > > > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > > > --- > > > .../net/ethernet/stmicro/stmmac/stmmac_main.c | 22 > > > +++++++++---------- > > > 1 file changed, 10 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > index c33db79cdd0a..a46e865c4acc 100644 > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > @@ -2908,8 +2908,7 @@ static int stmmac_release(struct net_device *dev) > > > struct stmmac_priv *priv = netdev_priv(dev); > > > u32 chan; > > > > > > - if (device_may_wakeup(priv->device)) > > > > This check is to prevent link speed down if the stmmac isn't a wakeup device. > > When we invoke .ndo_stop, we down the net device. Per my understanding, we can speed down the phy, no matter it is a wakeup device or not. The problem is if the device can't wake up, then phy link will be turned off No need to speed down the phy before turning off it. PS: It seems your email client isn't properly setup..
> -----Original Message----- > From: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > Sent: 2020年12月10日 17:03 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com; > joabreu@synopsys.com; davem@davemloft.net; kuba@kernel.org; > netdev@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH RFC] ethernet: stmmac: clean up the code for > release/suspend/resume function > > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > > @@ -2908,8 +2908,7 @@ static int stmmac_release(struct net_device > *dev) > > > > struct stmmac_priv *priv = netdev_priv(dev); > > > > u32 chan; > > > > > > > > - if (device_may_wakeup(priv->device)) > > > > > > This check is to prevent link speed down if the stmmac isn't a wakeup > device. > > > > When we invoke .ndo_stop, we down the net device. Per my understanding, > we can speed down the phy, no matter it is a wakeup device or not. > > The problem is if the device can't wake up, then phy link will be turned off No > need to speed down the phy before turning off it. Yes, it makes sense. Thanks for your explanation. Do you encounter dev_watchdog timeout after a few hundred times suspend/resume stress time with stmmac driver? I am not familiar with ethernet driver now, could you give me some suggestions? This issue seems more related to stmmac core driver. [ 305.273935] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready [ 315.983474] ------------[ cut here ]------------ [ 315.988158] NETDEV WATCHDOG: eth0 (imx-dwmac): transmit queue 0 timed out [ 315.995044] WARNING: CPU: 0 PID: 0 at net/sched/sch_generic.c:450 dev_watchdog+0x2fc/0x30c [ 316.003315] Modules linked in: [ 316.006389] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.10.0-rc6-04451-g8ff26f5af83b-dirty #55 [ 316.015012] Hardware name: Freescale i.MX8DXL EVK (DT) [ 316.020158] pstate: 20000005 (nzCv daif -PAN -UAO -TCO BTYPE=--) [ 316.026178] pc : dev_watchdog+0x2fc/0x30c [ 316.030198] lr : dev_watchdog+0x2fc/0x30c [ 316.034208] sp : ffff800011c7bd90 [ 316.037528] x29: ffff800011c7bd90 x28: ffff00001695b940 [ 316.042852] x27: 0000000000000004 x26: ffff000016dd8480 [ 316.048178] x25: 0000000000000140 x24: 00000000ffffffff [ 316.053504] x23: ffff000016dd83dc x22: 0000000000000000 [ 316.058827] x21: ffff800011a76000 x20: ffff000016dd8000 [ 316.064154] x19: 0000000000000000 x18: 0000000000000030 [ 316.069480] x17: 0000000000000000 x16: 0000000000000000 [ 316.074805] x15: ffff800011a82738 x14: ffffffffffffffff [ 316.080133] x13: ffff800011a91678 x12: 0000000000002061 [ 316.085456] x11: 0000000000000acb x10: ffff800011ae9678 [ 316.090781] x9 : 00000000fffff000 x8 : ffff800011a91678 [ 316.096107] x7 : ffff800011ae9678 x6 : 0000000000000003 [ 316.101432] x5 : 0000000000000000 x4 : 0000000000000000 [ 316.106758] x3 : 0000000000000000 x2 : 0000000000000100 [ 316.112081] x1 : 7290e915aa0d5b00 x0 : 0000000000000000 [ 316.117409] Call trace: [ 316.119866] dev_watchdog+0x2fc/0x30c [ 316.123542] call_timer_fn.constprop.0+0x24/0x80 [ 316.128171] __run_timers.part.0+0x1f0/0x224 [ 316.132451] run_timer_softirq+0x3c/0x7c [ 316.136380] efi_header_end+0x124/0x290 [ 316.140227] irq_exit+0xdc/0xfc [ 316.143371] __handle_domain_irq+0x80/0xe0 [ 316.147473] gic_handle_irq+0xc0/0x140 [ 316.151232] el1_irq+0xbc/0x180 [ 316.154379] arch_cpu_idle+0x14/0x1c [ 316.157959] do_idle+0x230/0x2a0 [ 316.161190] cpu_startup_entry+0x28/0x70 [ 316.165116] rest_init+0xd8/0xe8 [ 316.168350] arch_call_rest_init+0x10/0x1c [ 316.172458] start_kernel+0x4bc/0x4f4 [ 316.176121] ---[ end trace a0311227e418a672 ]--- > PS: It seems your email client isn't properly setup.. Do you mean mail timestamp? Best Regards, Joakim Zhang
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index c33db79cdd0a..a46e865c4acc 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -2908,8 +2908,7 @@ static int stmmac_release(struct net_device *dev) struct stmmac_priv *priv = netdev_priv(dev); u32 chan; - if (device_may_wakeup(priv->device)) - phylink_speed_down(priv->phylink, false); + phylink_speed_down(priv->phylink, false); /* Stop and disconnect the PHY */ phylink_stop(priv->phylink); phylink_disconnect_phy(priv->phylink); @@ -5183,6 +5182,7 @@ int stmmac_suspend(struct device *dev) } else { mutex_unlock(&priv->lock); rtnl_lock(); + /* For PHY wakeup case */ if (device_may_wakeup(priv->device)) phylink_speed_down(priv->phylink, false); phylink_stop(priv->phylink); @@ -5260,11 +5260,17 @@ int stmmac_resume(struct device *dev) /* enable the clk previously disabled */ clk_prepare_enable(priv->plat->stmmac_clk); clk_prepare_enable(priv->plat->pclk); - if (priv->plat->clk_ptp_ref) - clk_prepare_enable(priv->plat->clk_ptp_ref); + clk_prepare_enable(priv->plat->clk_ptp_ref); /* reset the phy so that it's ready */ if (priv->mii) stmmac_mdio_reset(priv->mii); + + rtnl_lock(); + phylink_start(priv->phylink); + /* We may have called phylink_speed_down before */ + if (device_may_wakeup(priv->device)) + phylink_speed_up(priv->phylink); + rtnl_unlock(); } if (priv->plat->serdes_powerup) { @@ -5275,14 +5281,6 @@ int stmmac_resume(struct device *dev) return ret; } - if (!device_may_wakeup(priv->device) || !priv->plat->pmt) { - rtnl_lock(); - phylink_start(priv->phylink); - /* We may have called phylink_speed_down before */ - phylink_speed_up(priv->phylink); - rtnl_unlock(); - } - rtnl_lock(); mutex_lock(&priv->lock);
commit 1c35cc9cf6a0 ("net: stmmac: remove redundant null check before clk_disable_unprepare()"), have not clean up check NULL clock parameter completely, this patch did it. commit e8377e7a29efb ("net: stmmac: only call pmt() during suspend/resume if HW enables PMT"), after this patch, we use if (device_may_wakeup(priv->device) && priv->plat->pmt) check MAC wakeup if (device_may_wakeup(priv->device)) check PHY wakeup Add oneline comment for readability. commit 77b2898394e3b ("net: stmmac: Speed down the PHY if WoL to save energy"), slow down phy speed when release net device under any condition. Slightly adjust the order of the codes so that suspend/resume look more symmetrical, generally speaking they should appear symmetrically. Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> --- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-)