| Message ID | 20210428074107.2378-1-qiangqing.zhang@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [V3,net] net: stmmac: fix MAC WoL unwork if PHY doesn't support WoL | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 5 maintainers not CCed: linux-arm-kernel@lists.infradead.org linux@armlinux.org.uk alexandre.torgue@foss.st.com linux-stm32@st-md-mailman.stormreply.com mcoquelin.stm32@gmail.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | fail | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 112 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
> static int stmmac_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) > { > struct stmmac_priv *priv = netdev_priv(dev); > - u32 support = WAKE_MAGIC | WAKE_UCAST; > + struct ethtool_wolinfo wol_phy = { .cmd = ETHTOOL_GWOL }; > + u32 support = WAKE_MAGIC | WAKE_UCAST | WAKE_MAGICSECURE | WAKE_BCAST; Reverse christmass tree please. > > - if (!device_can_wakeup(priv->device)) > - return -EOPNOTSUPP; > + if (wol->wolopts & ~support) > + return -EINVAL; Maybe -EOPNOTSUPP would be better. > > - if (!priv->plat->pmt) { > + /* First check if can WoL from PHY */ > + phylink_ethtool_get_wol(priv->phylink, &wol_phy); This could return an error. In which case, you probably should not trust wol_phy. > + if (wol->wolopts & wol_phy.supported) { This returns true if the PHY supports one or more of the requested WoL sources. > int ret = phylink_ethtool_set_wol(priv->phylink, wol); and here you request the PHY to enable all the requested WoL sources. If it only supports a subset, it is likely to return -EOPNOTSUPP, or -EINVAL, and do nothing. So here you only want to enable those sources the PHY actually supports. And let the MAC implement the rest. Andrew
On 4/28/2021 5:26 AM, Andrew Lunn wrote: >> static int stmmac_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) >> { >> struct stmmac_priv *priv = netdev_priv(dev); >> - u32 support = WAKE_MAGIC | WAKE_UCAST; >> + struct ethtool_wolinfo wol_phy = { .cmd = ETHTOOL_GWOL }; >> + u32 support = WAKE_MAGIC | WAKE_UCAST | WAKE_MAGICSECURE | WAKE_BCAST; > > Reverse christmass tree please. > >> >> - if (!device_can_wakeup(priv->device)) >> - return -EOPNOTSUPP; >> + if (wol->wolopts & ~support) >> + return -EINVAL; > > Maybe -EOPNOTSUPP would be better. > >> >> - if (!priv->plat->pmt) { >> + /* First check if can WoL from PHY */ >> + phylink_ethtool_get_wol(priv->phylink, &wol_phy); > > This could return an error. In which case, you probably should not > trust wol_phy. > >> + if (wol->wolopts & wol_phy.supported) { > > This returns true if the PHY supports one or more of the requested WoL > sources. > >> int ret = phylink_ethtool_set_wol(priv->phylink, wol); > > and here you request the PHY to enable all the requested WoL > sources. If it only supports a subset, it is likely to return > -EOPNOTSUPP, or -EINVAL, and do nothing. So here you only want to > enable those sources the PHY actually supports. And let the MAC > implement the rest. And when your resubmit, I do not believe that unwork is a word, you could provide the following subject: net: stmmac: Fix MAC WoL not working if PHY does not support WoL or something like that. Thanks!
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: 2021年4月28日 20:27 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com; > joabreu@synopsys.com; davem@davemloft.net; kuba@kernel.org; > f.fainelli@gmail.com; Jisheng.Zhang@synaptics.com; netdev@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH V3 net] net: stmmac: fix MAC WoL unwork if PHY doesn't > support WoL > > > static int stmmac_set_wol(struct net_device *dev, struct > > ethtool_wolinfo *wol) { > > struct stmmac_priv *priv = netdev_priv(dev); > > - u32 support = WAKE_MAGIC | WAKE_UCAST; > > + struct ethtool_wolinfo wol_phy = { .cmd = ETHTOOL_GWOL }; > > + u32 support = WAKE_MAGIC | WAKE_UCAST | WAKE_MAGICSECURE | > > +WAKE_BCAST; > > Reverse christmass tree please. Ok > > > > - if (!device_can_wakeup(priv->device)) > > - return -EOPNOTSUPP; > > + if (wol->wolopts & ~support) > > + return -EINVAL; > > Maybe -EOPNOTSUPP would be better. Ok > > > > - if (!priv->plat->pmt) { > > + /* First check if can WoL from PHY */ > > + phylink_ethtool_get_wol(priv->phylink, &wol_phy); > > This could return an error. In which case, you probably should not trust > wol_phy. phylink_ethtool_get_wol() is a void function, there is no return value. I think we can trust wol_phy, if PHY driver does not implement get_wol(), wol_phy.supported is 0; if PHY driver implement it, then it will fill the wol_phy.supported field. Please point me if I am misunderstanding. > > + if (wol->wolopts & wol_phy.supported) { > > This returns true if the PHY supports one or more of the requested WoL > sources. > > > int ret = phylink_ethtool_set_wol(priv->phylink, wol); > > and here you request the PHY to enable all the requested WoL sources. If it > only supports a subset, it is likely to return -EOPNOTSUPP, or -EINVAL, and do > nothing. So here you only want to enable those sources the PHY actually > supports. And let the MAC implement the rest. Yes, you are right! It should be: wol->wolopts &= wol_phy.supported; ret = phylink_ethtool_set_wol(priv->phylink, wol); Best Regards, Joakim Zhang > Andrew
> -----Original Message----- > From: Florian Fainelli <f.fainelli@gmail.com> > Sent: 2021年4月29日 0:22 > To: Andrew Lunn <andrew@lunn.ch>; Joakim Zhang > <qiangqing.zhang@nxp.com> > Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com; > joabreu@synopsys.com; davem@davemloft.net; kuba@kernel.org; > Jisheng.Zhang@synaptics.com; netdev@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com> > Subject: Re: [PATCH V3 net] net: stmmac: fix MAC WoL unwork if PHY doesn't > support WoL > > > > On 4/28/2021 5:26 AM, Andrew Lunn wrote: > >> static int stmmac_set_wol(struct net_device *dev, struct > >> ethtool_wolinfo *wol) { > >> struct stmmac_priv *priv = netdev_priv(dev); > >> - u32 support = WAKE_MAGIC | WAKE_UCAST; > >> + struct ethtool_wolinfo wol_phy = { .cmd = ETHTOOL_GWOL }; > >> + u32 support = WAKE_MAGIC | WAKE_UCAST | WAKE_MAGICSECURE | > >> +WAKE_BCAST; > > > > Reverse christmass tree please. > > > >> > >> - if (!device_can_wakeup(priv->device)) > >> - return -EOPNOTSUPP; > >> + if (wol->wolopts & ~support) > >> + return -EINVAL; > > > > Maybe -EOPNOTSUPP would be better. > > > >> > >> - if (!priv->plat->pmt) { > >> + /* First check if can WoL from PHY */ > >> + phylink_ethtool_get_wol(priv->phylink, &wol_phy); > > > > This could return an error. In which case, you probably should not > > trust wol_phy. > > > >> + if (wol->wolopts & wol_phy.supported) { > > > > This returns true if the PHY supports one or more of the requested WoL > > sources. > > > >> int ret = phylink_ethtool_set_wol(priv->phylink, wol); > > > > and here you request the PHY to enable all the requested WoL sources. > > If it only supports a subset, it is likely to return -EOPNOTSUPP, or > > -EINVAL, and do nothing. So here you only want to enable those sources > > the PHY actually supports. And let the MAC implement the rest. > > And when your resubmit, I do not believe that unwork is a word, you could > provide the following subject: > > net: stmmac: Fix MAC WoL not working if PHY does not support WoL > > or something like that. Ok, thanks Florian! Best Regards, Joakim Zhang > Thanks! > -- > Florian
> > > - if (!priv->plat->pmt) { > > > + /* First check if can WoL from PHY */ > > > + phylink_ethtool_get_wol(priv->phylink, &wol_phy); > > > > This could return an error. In which case, you probably should not trust > > wol_phy. > phylink_ethtool_get_wol() is a void function Upps. Yes, you are correct. Ignore what i said! Andre
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c index c5642985ef95..30358c1892f1 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c @@ -630,34 +630,46 @@ static void stmmac_get_strings(struct net_device *dev, u32 stringset, u8 *data) static void stmmac_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol) { struct stmmac_priv *priv = netdev_priv(dev); - - if (!priv->plat->pmt) - return phylink_ethtool_get_wol(priv->phylink, wol); + struct ethtool_wolinfo wol_phy = { .cmd = ETHTOOL_GWOL }; mutex_lock(&priv->lock); - if (device_can_wakeup(priv->device)) { + if (priv->plat->pmt) { wol->supported = WAKE_MAGIC | WAKE_UCAST; if (priv->hw_cap_support && !priv->dma_cap.pmt_magic_frame) wol->supported &= ~WAKE_MAGIC; - wol->wolopts = priv->wolopts; } + + phylink_ethtool_get_wol(priv->phylink, &wol_phy); + + /* Combine WoL capabilities both PHY and MAC */ + wol->supported |= wol_phy.supported; + wol->wolopts = priv->wolopts; + mutex_unlock(&priv->lock); } static int stmmac_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) { struct stmmac_priv *priv = netdev_priv(dev); - u32 support = WAKE_MAGIC | WAKE_UCAST; + struct ethtool_wolinfo wol_phy = { .cmd = ETHTOOL_GWOL }; + u32 support = WAKE_MAGIC | WAKE_UCAST | WAKE_MAGICSECURE | WAKE_BCAST; - if (!device_can_wakeup(priv->device)) - return -EOPNOTSUPP; + if (wol->wolopts & ~support) + return -EINVAL; - if (!priv->plat->pmt) { + /* First check if can WoL from PHY */ + phylink_ethtool_get_wol(priv->phylink, &wol_phy); + if (wol->wolopts & wol_phy.supported) { int ret = phylink_ethtool_set_wol(priv->phylink, wol); - if (!ret) - device_set_wakeup_enable(priv->device, !!wol->wolopts); - return ret; + if (!ret) { + pr_info("stmmac: phy wakeup enable\n"); + device_set_wakeup_capable(priv->device, 1); + device_set_wakeup_enable(priv->device, 1); + goto wolopts_update; + } else { + return ret; + } } /* By default almost all GMAC devices support the WoL via @@ -666,18 +678,21 @@ static int stmmac_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) if ((priv->hw_cap_support) && (!priv->dma_cap.pmt_magic_frame)) wol->wolopts &= ~WAKE_MAGIC; - if (wol->wolopts & ~support) - return -EINVAL; - - if (wol->wolopts) { - pr_info("stmmac: wakeup enable\n"); + if (priv->plat->pmt && wol->wolopts) { + pr_info("stmmac: mac wakeup enable\n"); + device_set_wakeup_capable(priv->device, 1); device_set_wakeup_enable(priv->device, 1); enable_irq_wake(priv->wol_irq); - } else { + goto wolopts_update; + } + + if (!wol->wolopts) { + device_set_wakeup_capable(priv->device, 0); device_set_wakeup_enable(priv->device, 0); disable_irq_wake(priv->wol_irq); } +wolopts_update: mutex_lock(&priv->lock); priv->wolopts = wol->wolopts; mutex_unlock(&priv->lock); diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index c6f24abf6432..d62d8c28463d 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -1076,7 +1076,6 @@ static void stmmac_check_pcs_mode(struct stmmac_priv *priv) */ static int stmmac_init_phy(struct net_device *dev) { - struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL }; struct stmmac_priv *priv = netdev_priv(dev); struct device_node *node; int ret; @@ -1102,9 +1101,6 @@ static int stmmac_init_phy(struct net_device *dev) ret = phylink_connect_phy(priv->phylink, phydev); } - phylink_ethtool_get_wol(priv->phylink, &wol); - device_set_wakeup_capable(priv->device, !!wol.supported); - return ret; } @@ -4787,10 +4783,8 @@ static int stmmac_hw_init(struct stmmac_priv *priv) if (priv->plat->tx_coe) dev_info(priv->device, "TX Checksum insertion supported\n"); - if (priv->plat->pmt) { + if (priv->plat->pmt) dev_info(priv->device, "Wake-Up On Lan supported\n"); - device_set_wakeup_capable(priv->device, 1); - } if (priv->dma_cap.tsoen) dev_info(priv->device, "TSO supported\n");
Both get and set WoL will check device_can_wakeup(), if MAC supports PMT, it will set device wakeup capability. After commit 1d8e5b0f3f2c ("net: stmmac: Support WOL with phy"), device wakeup capability will be overwrite in stmmac_init_phy() according to phy's Wol feature. If phy doesn't support WoL, then MAC will lose wakeup capability. This patch combines WoL capabilities both MAC and PHY from stmmac_get_wol(), set wakeup capability and give WoL priority to the PHY in stmmac_set_wol() when enable WoL. What PHYs do implement is WAKE_MAGIC, WAKE_UCAST, WAKE_MAGICSECURE and WAKE_BCAST. Fixes: commit 1d8e5b0f3f2c ("net: stmmac: Support WOL with phy") Suggested-by: Andrew Lunn <andrew@lunn.ch> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> --- ChangeLogs: V1->V2: * combine WoL capabilities both MAC and PHY V2->V3: * give WoL priority to the PHY. --- .../ethernet/stmicro/stmmac/stmmac_ethtool.c | 51 ++++++++++++------- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 8 +-- 2 files changed, 34 insertions(+), 25 deletions(-)