| Message ID | E1tnf1S-0056LC-6H@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | net: stmmac: fix resume failures due to RX clock | expand |
> @@ -7927,13 +7925,9 @@ int stmmac_resume(struct device *dev) > } > > 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); > - } > + phylink_resume(priv->phylink); > + if (device_may_wakeup(priv->device) && !priv->plat->pmt) > + phylink_speed_up(priv->phylink); > rtnl_unlock(); > > rtnl_lock(); Unrelated to this patch, but unlock() followed by lock()? Seems like some more code which could be cleaned up? Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Thu, Feb 27, 2025 at 05:45:35PM +0100, Andrew Lunn wrote: > > @@ -7927,13 +7925,9 @@ int stmmac_resume(struct device *dev) > > } > > > > 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); > > - } > > + phylink_resume(priv->phylink); > > + if (device_may_wakeup(priv->device) && !priv->plat->pmt) > > + phylink_speed_up(priv->phylink); > > rtnl_unlock(); > > > > rtnl_lock(); > > Unrelated to this patch, but unlock() followed by lock()? Seems like > some more code which could be cleaned up? Indeed, this vanishes in the next patch due to phylink_resume() moving later.
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index aec230353ac4..fbcba6c71f12 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -7831,13 +7831,11 @@ int stmmac_suspend(struct device *dev) mutex_unlock(&priv->lock); rtnl_lock(); - if (device_may_wakeup(priv->device) && priv->plat->pmt) { - phylink_suspend(priv->phylink, true); - } else { - if (device_may_wakeup(priv->device)) - phylink_speed_down(priv->phylink, false); - phylink_suspend(priv->phylink, false); - } + if (device_may_wakeup(priv->device) && !priv->plat->pmt) + phylink_speed_down(priv->phylink, false); + + phylink_suspend(priv->phylink, + device_may_wakeup(priv->device) && priv->plat->pmt); rtnl_unlock(); if (stmmac_fpe_supported(priv)) @@ -7927,13 +7925,9 @@ int stmmac_resume(struct device *dev) } 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); - } + phylink_resume(priv->phylink); + if (device_may_wakeup(priv->device) && !priv->plat->pmt) + phylink_speed_up(priv->phylink); rtnl_unlock(); rtnl_lock();
Currently, the calls to phylink's suspend and resume functions are inside overly complex tests, and boil down to: if (device_may_wakeup(priv->device) && priv->plat->pmt) { call phylink } else { call phylink and if (device_may_wakeup(priv->device)) do something else } This results in phylink always being called, possibly with differing arguments for phylink_suspend(). Simplify this code, noting that each site is slightly different due to the order in which phylink is called and the "something else". Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 22 +++++++------------ 1 file changed, 8 insertions(+), 14 deletions(-)