Issue we met:
On some platforms, mac cannot work after resumed from the suspend with WoL
enabled.
The cause of the issue:
1. phylink_resolve() is in a workqueue which will not be executed immediately.
This is the call sequence:
phylink_resolve()->phylink_link_up()->pl->mac_ops->mac_link_up()
For stmmac driver, mac_link_up() will set the correct speed/duplex...
values which are from link_state.
2. In stmmac_resume(), it will call stmmac_hw_setup() after called the
phylink_resume(), because mac need phy rx_clk to do the reset.
stmmac_core_init() is called in function stmmac_hw_setup(), which will
reset the mac and set the speed/duplex... to default value.
Conclusion: Because phylink_resolve() cannot determine when it is called, it
cannot be guaranteed to be called after stmmac_core_init().
Once stmmac_core_init() is called after phylink_resolve(),
the mac will be misconfigured and cannot be used.
In order to avoid this problem, add a function called phylink_phy_resume()
to resume phy separately. This eliminates the need to call phylink_resume()
before stmmac_hw_setup().
Add another judgement before called phy_start() in phylink_start(). This way
phy_start() will not be called multiple times when resumes. At the same time,
it may not affect other drivers that do not use phylink_phy_resume().
Signed-off-by: Clark Wang <[email protected]>
---
V2 change:
- add mac_resume_phy_separately flag to struct phylink to mark if the mac
driver uses the phylink_phy_resume() first.
---
drivers/net/phy/phylink.c | 29 ++++++++++++++++++++++++++++-
include/linux/phylink.h | 1 +
2 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 319790221d7f..687562869c33 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -80,6 +80,8 @@ struct phylink {
DECLARE_PHY_INTERFACE_MASK(sfp_interfaces);
__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
u8 sfp_port;
+
+ bool mac_resume_phy_separately;
};
#define phylink_printk(level, pl, fmt, ...) \
@@ -1509,6 +1511,7 @@ struct phylink *phylink_create(struct phylink_config *config,
return ERR_PTR(-EINVAL);
}
+ pl->mac_resume_phy_separately = false;
pl->using_mac_select_pcs = using_mac_select_pcs;
pl->phy_state.interface = iface;
pl->link_interface = iface;
@@ -1944,7 +1947,10 @@ void phylink_start(struct phylink *pl)
if (poll)
mod_timer(&pl->link_poll, jiffies + HZ);
if (pl->phydev)
- phy_start(pl->phydev);
+ if (!pl->mac_resume_phy_separately)
+ phy_start(pl->phydev);
+ else
+ pl->mac_resume_phy_separately = false;
if (pl->sfp_bus)
sfp_upstream_start(pl->sfp_bus);
}
@@ -2024,6 +2030,27 @@ void phylink_suspend(struct phylink *pl, bool mac_wol)
}
EXPORT_SYMBOL_GPL(phylink_suspend);
+/**
+ * phylink_phy_resume() - resume phy alone
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ *
+ * In the MAC driver using phylink, if the MAC needs the clock of the phy
+ * when it resumes, can call this function to resume the phy separately.
+ * Then proceed to MAC resume operations.
+ */
+void phylink_phy_resume(struct phylink *pl)
+{
+ ASSERT_RTNL();
+
+ if (!test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state)
+ && pl->phydev) {
+ phy_start(pl->phydev);
+ pl->mac_resume_phy_separately = true;
+ }
+
+}
+EXPORT_SYMBOL_GPL(phylink_phy_resume);
+
/**
* phylink_resume() - handle a network device resume event
* @pl: a pointer to a &struct phylink returned from phylink_create()
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index c492c26202b5..6edfab5f754c 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -589,6 +589,7 @@ void phylink_stop(struct phylink *);
void phylink_suspend(struct phylink *pl, bool mac_wol);
void phylink_resume(struct phylink *pl);
+void phylink_phy_resume(struct phylink *pl);
void phylink_ethtool_get_wol(struct phylink *, struct ethtool_wolinfo *);
int phylink_ethtool_set_wol(struct phylink *, struct ethtool_wolinfo *);
--
2.34.1
On some platforms, mac cannot work after resumed from the suspend with WoL
enabled.
We found the stmmac_hw_setup() when system resumes will called after the
stmmac_mac_link_up(). So the correct values set in stmmac_mac_link_up()
are overwritten by stmmac_core_init() in phylink_resume().
So call the new added function in phylink to resume phy fristly.
Then can call the stmmac_hw_setup() before calling phy_resume().
Fixes: 90702dcd19c0 ("net: stmmac: fix MAC not working when system resume back with WoL active")
Signed-off-by: Clark Wang <[email protected]>
Reviewed-by: Piotr Raczynski <[email protected]>
---
V2:
- add Fixes tag
---
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 1a5b8dab5e9b..38777e408d96 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7538,16 +7538,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);
- }
- rtnl_unlock();
- rtnl_lock();
+ phylink_phy_resume(priv->phylink);
+
mutex_lock(&priv->lock);
stmmac_reset_queues_param(priv);
@@ -7565,6 +7558,11 @@ int stmmac_resume(struct device *dev)
stmmac_enable_all_dma_irq(priv);
mutex_unlock(&priv->lock);
+
+ phylink_resume(priv->phylink);
+ if (device_may_wakeup(priv->device) && !priv->plat->pmt)
+ phylink_speed_up(priv->phylink);
+
rtnl_unlock();
netif_device_attach(ndev);
--
2.34.1
Hi Clark,
I love your patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
[also build test WARNING on net/master linus/master v6.2-rc6 next-20230201]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Clark-Wang/net-stmmac-resume-phy-separately-before-calling-stmmac_hw_setup/20230201-184223
patch link: https://lore.kernel.org/r/20230201103837.3258752-1-xiaoning.wang%40nxp.com
patch subject: [PATCH V2 1/2] net: phylink: add a function to resume phy alone to fix resume issue with WoL enabled
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230201/[email protected]/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/6df0562fc6133175ff3932188af0d9126858c42c
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Clark-Wang/net-stmmac-resume-phy-separately-before-calling-stmmac_hw_setup/20230201-184223
git checkout 6df0562fc6133175ff3932188af0d9126858c42c
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/net/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/net/phy/phylink.c: In function 'phylink_start':
>> drivers/net/phy/phylink.c:1949:12: warning: suggest explicit braces to avoid ambiguous 'else' [-Wdangling-else]
1949 | if (pl->phydev)
| ^
vim +/else +1949 drivers/net/phy/phylink.c
7b3b0e89bcf3ac Russell King 2019-05-28 1887
8796c8923d9c42 Russell King 2017-12-01 1888 /**
8796c8923d9c42 Russell King 2017-12-01 1889 * phylink_start() - start a phylink instance
8796c8923d9c42 Russell King 2017-12-01 1890 * @pl: a pointer to a &struct phylink returned from phylink_create()
8796c8923d9c42 Russell King 2017-12-01 1891 *
8796c8923d9c42 Russell King 2017-12-01 1892 * Start the phylink instance specified by @pl, configuring the MAC for the
8796c8923d9c42 Russell King 2017-12-01 1893 * desired link mode(s) and negotiation style. This should be called from the
8796c8923d9c42 Russell King 2017-12-01 1894 * network device driver's &struct net_device_ops ndo_open() method.
8796c8923d9c42 Russell King 2017-12-01 1895 */
9525ae83959b60 Russell King 2017-07-25 1896 void phylink_start(struct phylink *pl)
9525ae83959b60 Russell King 2017-07-25 1897 {
5c05c1dbb17729 Russell King 2020-04-23 1898 bool poll = false;
5c05c1dbb17729 Russell King 2020-04-23 1899
8b874514c11d6f Russell King 2017-12-15 1900 ASSERT_RTNL();
9525ae83959b60 Russell King 2017-07-25 1901
17091180b1521e Ioana Ciornei 2019-05-28 1902 phylink_info(pl, "configuring for %s/%s link mode\n",
24cf0e693bb50a Russell King 2019-12-11 1903 phylink_an_mode_str(pl->cur_link_an_mode),
9525ae83959b60 Russell King 2017-07-25 1904 phy_modes(pl->link_config.interface));
9525ae83959b60 Russell King 2017-07-25 1905
aeeb2e8fdefdd5 Antoine Tenart 2018-09-19 1906 /* Always set the carrier off */
43de61959b9992 Ioana Ciornei 2019-05-28 1907 if (pl->netdev)
aeeb2e8fdefdd5 Antoine Tenart 2018-09-19 1908 netif_carrier_off(pl->netdev);
aeeb2e8fdefdd5 Antoine Tenart 2018-09-19 1909
9525ae83959b60 Russell King 2017-07-25 1910 /* Apply the link configuration to the MAC when starting. This allows
9525ae83959b60 Russell King 2017-07-25 1911 * a fixed-link to start with the correct parameters, and also
cc1122b00d624e Colin Ian King 2018-03-01 1912 * ensures that we set the appropriate advertisement for Serdes links.
4c0d6d3a7a81fc Russell King 2020-03-30 1913 *
4c0d6d3a7a81fc Russell King 2020-03-30 1914 * Restart autonegotiation if using 802.3z to ensure that the link
85b43945cf7587 Russell King 2017-12-01 1915 * parameters are properly negotiated. This is necessary for DSA
85b43945cf7587 Russell King 2017-12-01 1916 * switches using 802.3z negotiation to ensure they see our modes.
85b43945cf7587 Russell King 2017-12-01 1917 */
4c0d6d3a7a81fc Russell King 2020-03-30 1918 phylink_mac_initial_config(pl, true);
85b43945cf7587 Russell King 2017-12-01 1919
aa729c439441aa Russell King 2021-11-30 1920 phylink_enable_and_run_resolve(pl, PHYLINK_DISABLE_STOPPED);
9525ae83959b60 Russell King 2017-07-25 1921
24cf0e693bb50a Russell King 2019-12-11 1922 if (pl->cfg_link_an_mode == MLO_AN_FIXED && pl->link_gpio) {
7b3b0e89bcf3ac Russell King 2019-05-28 1923 int irq = gpiod_to_irq(pl->link_gpio);
7b3b0e89bcf3ac Russell King 2019-05-28 1924
7b3b0e89bcf3ac Russell King 2019-05-28 1925 if (irq > 0) {
7b3b0e89bcf3ac Russell King 2019-05-28 1926 if (!request_irq(irq, phylink_link_handler,
7b3b0e89bcf3ac Russell King 2019-05-28 1927 IRQF_TRIGGER_RISING |
7b3b0e89bcf3ac Russell King 2019-05-28 1928 IRQF_TRIGGER_FALLING,
7b3b0e89bcf3ac Russell King 2019-05-28 1929 "netdev link", pl))
7b3b0e89bcf3ac Russell King 2019-05-28 1930 pl->link_irq = irq;
7b3b0e89bcf3ac Russell King 2019-05-28 1931 else
7b3b0e89bcf3ac Russell King 2019-05-28 1932 irq = 0;
7b3b0e89bcf3ac Russell King 2019-05-28 1933 }
7b3b0e89bcf3ac Russell King 2019-05-28 1934 if (irq <= 0)
5c05c1dbb17729 Russell King 2020-04-23 1935 poll = true;
5c05c1dbb17729 Russell King 2020-04-23 1936 }
5c05c1dbb17729 Russell King 2020-04-23 1937
5c05c1dbb17729 Russell King 2020-04-23 1938 switch (pl->cfg_link_an_mode) {
5c05c1dbb17729 Russell King 2020-04-23 1939 case MLO_AN_FIXED:
5c05c1dbb17729 Russell King 2020-04-23 1940 poll |= pl->config->poll_fixed_state;
5c05c1dbb17729 Russell King 2020-04-23 1941 break;
5c05c1dbb17729 Russell King 2020-04-23 1942 case MLO_AN_INBAND:
7137e18f6f889a Russell King 2020-07-21 1943 if (pl->pcs)
7137e18f6f889a Russell King 2020-07-21 1944 poll |= pl->pcs->poll;
5c05c1dbb17729 Russell King 2020-04-23 1945 break;
7b3b0e89bcf3ac Russell King 2019-05-28 1946 }
5c05c1dbb17729 Russell King 2020-04-23 1947 if (poll)
9cd00a8aa42e44 Russell King 2018-05-10 1948 mod_timer(&pl->link_poll, jiffies + HZ);
9525ae83959b60 Russell King 2017-07-25 @1949 if (pl->phydev)
6df0562fc61331 Clark Wang 2023-02-01 1950 if (!pl->mac_resume_phy_separately)
9525ae83959b60 Russell King 2017-07-25 1951 phy_start(pl->phydev);
6df0562fc61331 Clark Wang 2023-02-01 1952 else
6df0562fc61331 Clark Wang 2023-02-01 1953 pl->mac_resume_phy_separately = false;
c7fa7f567cab65 Arseny Solokha 2019-07-24 1954 if (pl->sfp_bus)
c7fa7f567cab65 Arseny Solokha 2019-07-24 1955 sfp_upstream_start(pl->sfp_bus);
9525ae83959b60 Russell King 2017-07-25 1956 }
9525ae83959b60 Russell King 2017-07-25 1957 EXPORT_SYMBOL_GPL(phylink_start);
9525ae83959b60 Russell King 2017-07-25 1958
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Clark,
I love your patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
[also build test WARNING on net/master linus/master v6.2-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Clark-Wang/net-stmmac-resume-phy-separately-before-calling-stmmac_hw_setup/20230201-184223
patch link: https://lore.kernel.org/r/20230201103837.3258752-1-xiaoning.wang%40nxp.com
patch subject: [PATCH V2 1/2] net: phylink: add a function to resume phy alone to fix resume issue with WoL enabled
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20230202/[email protected]/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/6df0562fc6133175ff3932188af0d9126858c42c
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Clark-Wang/net-stmmac-resume-phy-separately-before-calling-stmmac_hw_setup/20230201-184223
git checkout 6df0562fc6133175ff3932188af0d9126858c42c
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/net/phy/ kernel/bpf/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/net/phy/phylink.c:1952:3: warning: add explicit braces to avoid dangling else [-Wdangling-else]
else
^
1 warning generated.
vim +1952 drivers/net/phy/phylink.c
1887
1888 /**
1889 * phylink_start() - start a phylink instance
1890 * @pl: a pointer to a &struct phylink returned from phylink_create()
1891 *
1892 * Start the phylink instance specified by @pl, configuring the MAC for the
1893 * desired link mode(s) and negotiation style. This should be called from the
1894 * network device driver's &struct net_device_ops ndo_open() method.
1895 */
1896 void phylink_start(struct phylink *pl)
1897 {
1898 bool poll = false;
1899
1900 ASSERT_RTNL();
1901
1902 phylink_info(pl, "configuring for %s/%s link mode\n",
1903 phylink_an_mode_str(pl->cur_link_an_mode),
1904 phy_modes(pl->link_config.interface));
1905
1906 /* Always set the carrier off */
1907 if (pl->netdev)
1908 netif_carrier_off(pl->netdev);
1909
1910 /* Apply the link configuration to the MAC when starting. This allows
1911 * a fixed-link to start with the correct parameters, and also
1912 * ensures that we set the appropriate advertisement for Serdes links.
1913 *
1914 * Restart autonegotiation if using 802.3z to ensure that the link
1915 * parameters are properly negotiated. This is necessary for DSA
1916 * switches using 802.3z negotiation to ensure they see our modes.
1917 */
1918 phylink_mac_initial_config(pl, true);
1919
1920 phylink_enable_and_run_resolve(pl, PHYLINK_DISABLE_STOPPED);
1921
1922 if (pl->cfg_link_an_mode == MLO_AN_FIXED && pl->link_gpio) {
1923 int irq = gpiod_to_irq(pl->link_gpio);
1924
1925 if (irq > 0) {
1926 if (!request_irq(irq, phylink_link_handler,
1927 IRQF_TRIGGER_RISING |
1928 IRQF_TRIGGER_FALLING,
1929 "netdev link", pl))
1930 pl->link_irq = irq;
1931 else
1932 irq = 0;
1933 }
1934 if (irq <= 0)
1935 poll = true;
1936 }
1937
1938 switch (pl->cfg_link_an_mode) {
1939 case MLO_AN_FIXED:
1940 poll |= pl->config->poll_fixed_state;
1941 break;
1942 case MLO_AN_INBAND:
1943 if (pl->pcs)
1944 poll |= pl->pcs->poll;
1945 break;
1946 }
1947 if (poll)
1948 mod_timer(&pl->link_poll, jiffies + HZ);
1949 if (pl->phydev)
1950 if (!pl->mac_resume_phy_separately)
1951 phy_start(pl->phydev);
> 1952 else
1953 pl->mac_resume_phy_separately = false;
1954 if (pl->sfp_bus)
1955 sfp_upstream_start(pl->sfp_bus);
1956 }
1957 EXPORT_SYMBOL_GPL(phylink_start);
1958
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests