diff mbox series

[V2,1/2] net: phylink: add a function to resume phy alone to fix resume issue with WoL enabled

Message ID 20230201103837.3258752-1-xiaoning.wang@nxp.com (mailing list archive)
State New, archived
Headers show
Series [V2,1/2] net: phylink: add a function to resume phy alone to fix resume issue with WoL enabled | expand

Commit Message

Clark Wang Feb. 1, 2023, 10:38 a.m. UTC 
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 <xiaoning.wang@nxp.com>
---
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(-)

Comments

kernel test robot Feb. 1, 2023, 12:14 p.m. UTC | #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/202302012009.TmS9IEE9-lkp@intel.com/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 <lkp@intel.com>

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
kernel test robot Feb. 2, 2023, 12:26 p.m. UTC | #2 
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/202302022040.NzeFwwSF-lkp@intel.com/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 <lkp@intel.com>

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
diff mbox series

Patch

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 *);