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