| Message ID | 20221019123643.1937889-1-xiaoning.wang@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: stmmac: linkup phy after enabled mac when system resume | expand |
On Wed, Oct 19, 2022 at 08:36:43PM +0800, Clark Wang wrote: > + mutex_unlock(&priv->lock); > + if (device_may_wakeup(priv->device) && priv->plat->pmt) { > + phylink_resume(priv->phylink); > + } else { > + phylink_resume(priv->phylink); > + if (device_may_wakeup(priv->device)) > + phylink_speed_up(priv->phylink); > + } > + mutex_lock(&priv->lock); First, is there a reason this isn't coded as: mutex_unlock(&priv->lock); phylink_resume(priv->phylink); if (!priv->plat->pmt && device_may_wakeup(priv->device)) phylink_speed_up(priv->phylink); mutex_lock(&priv->lock); And secondly, is it really safe to drop this lock? What specifically is the lock protecting? I see this isn't documented in the driver...
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 65c96773c6d2..79c9ea451a81 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -7515,16 +7515,6 @@ int stmmac_resume(struct device *dev) return ret; } - rtnl_lock(); - if (device_may_wakeup(priv->device) && priv->plat->pmt) { - phylink_resume(priv->phylink); - } else { - phylink_resume(priv->phylink); - if (device_may_wakeup(priv->device)) - phylink_speed_up(priv->phylink); - } - rtnl_unlock(); - rtnl_lock(); mutex_lock(&priv->lock); @@ -7539,6 +7529,16 @@ int stmmac_resume(struct device *dev) stmmac_restore_hw_vlan_rx_fltr(priv, ndev, priv->hw); + mutex_unlock(&priv->lock); + if (device_may_wakeup(priv->device) && priv->plat->pmt) { + phylink_resume(priv->phylink); + } else { + phylink_resume(priv->phylink); + if (device_may_wakeup(priv->device)) + phylink_speed_up(priv->phylink); + } + mutex_lock(&priv->lock); + stmmac_enable_all_queues(priv); stmmac_enable_all_dma_irq(priv);
Here is an issue: after enabled the WoL function, EQoS cannot send and receive data after resumed because of the wrong setting of MAC_CONFIGURATION register. When enable the WoL function, stmmac_resume will call stmmac_hw_setup and phylink_resume. - When do stmmac_hw_setup, it will reset the stmmac, and re-config the register GMAC_CONFIG with a fixed default value GMAC_CORE_INIT. - When do phylink_resume in stmmac_resume, it will call stmmac_mac_link_up in a workqueue. stmmac_mac_link_up will set the correct speed/duplex states provided by phy to the CONFIG register. So when resume the stmmac, the workqueue of phylink_resume must be run after the stmmac_hw_setup to ensure that the configuration of the CONFIG register is correct. In order to ensure this, put the place of phylink_resume consistent with stmmac_open. Make sure that stmmac_mac_link_up is called after stmmac_hw_setup even the workqueue of phylink_resume is run immediately. Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> --- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)