| Message ID | 20210426090447.14323-1-qiangqing.zhang@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [V2,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, 95 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
> + if (wol->wolopts & WAKE_PHY) { > + int ret = phylink_ethtool_set_wol(priv->phylink, wol); This is wrong. No PHY actually implements WAKE_PHY. What PHYs do implement is WAKE_MAGIC, WAKE_MAGICSEC, WAKE_UCAST, and WAKE_BCAST. So there is a clear overlap with what the MAC can do. So you need to decide which is going to provide each of these wakeups, the MAC or the PHY, and make sure only one does the actual implementation. Andrew
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: 2021年4月26日 21:05 > 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 V2 net] net: stmmac: fix MAC WoL unwork if PHY doesn't > support WoL > > > + if (wol->wolopts & WAKE_PHY) { > > + int ret = phylink_ethtool_set_wol(priv->phylink, wol); > > This is wrong. No PHY actually implements WAKE_PHY. > > What PHYs do implement is WAKE_MAGIC, WAKE_MAGICSEC, WAKE_UCAST, > and WAKE_BCAST. So there is a clear overlap with what the MAC can do. > > So you need to decide which is going to provide each of these wakeups, the > MAC or the PHY, and make sure only one does the actual implementation. Hi Andrew, Thanks for your review:-), PHY wakeup have not test at my side, need @Jisheng.Zhang@synaptics.com have a test if possible. According to your comments, I did a quick draft, and have not test yet, could you please review the logic to see if I understand you correctly? Thanks. --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c @@ -647,18 +647,7 @@ static void stmmac_get_wol(struct net_device *dev, struct ethtool_wolinfo *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; - - if (!device_can_wakeup(priv->device)) - return -EOPNOTSUPP; - - if (!priv->plat->pmt) { - int ret = phylink_ethtool_set_wol(priv->phylink, wol); - - if (!ret) - device_set_wakeup_enable(priv->device, !!wol->wolopts); - return ret; - } + u32 support = WAKE_MAGIC | WAKE_UCAST | WAKE_MAGICSECURE | WAKE_BCAST; /* By default almost all GMAC devices support the WoL via * magic frame but we can disable it if the HW capability @@ -669,13 +658,24 @@ static int stmmac_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) if (wol->wolopts & ~support) return -EINVAL; - if (wol->wolopts) { - pr_info("stmmac: wakeup enable\n"); - device_set_wakeup_enable(priv->device, 1); - enable_irq_wake(priv->wol_irq); - } else { + if (!wol->wolopts) { + device_set_wakeup_capable(priv->device, 0); device_set_wakeup_enable(priv->device, 0); disable_irq_wake(priv->wol_irq); + } else { + if (priv->plat->pmt && ((wol->wolopts & WAKE_MAGIC) || (wol->wolopts & WAKE_UCAST))) { + pr_info("stmmac: mac wakeup enable\n"); + enable_irq_wake(priv->wol_irq); + } else { + int ret = phylink_ethtool_set_wol(priv->phylink, wol); + + if (!ret) + pr_info("stmmac: phy wakeup enable\n"); + else + return ret; + } + device_set_wakeup_capable(priv->device, 1); + device_set_wakeup_enable(priv->device, 1); } mutex_lock(&priv->lock); Best Regards, Joakim Zhang > Andrew
> According to your comments, I did a quick draft, and have not test > yet, could you please review the logic to see if I understand you > correctly? > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > @@ -647,18 +647,7 @@ static void stmmac_get_wol(struct net_device *dev, struct ethtool_wolinfo *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; > - > - if (!device_can_wakeup(priv->device)) > - return -EOPNOTSUPP; > - > - if (!priv->plat->pmt) { > - int ret = phylink_ethtool_set_wol(priv->phylink, wol); > - > - if (!ret) > - device_set_wakeup_enable(priv->device, !!wol->wolopts); > - return ret; > - } > + u32 support = WAKE_MAGIC | WAKE_UCAST | WAKE_MAGICSECURE | WAKE_BCAST; > > /* By default almost all GMAC devices support the WoL via > * magic frame but we can disable it if the HW capability > @@ -669,13 +658,24 @@ static int stmmac_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) > if (wol->wolopts & ~support) > return -EINVAL; > > - if (wol->wolopts) { > - pr_info("stmmac: wakeup enable\n"); > - device_set_wakeup_enable(priv->device, 1); > - enable_irq_wake(priv->wol_irq); > - } else { > + if (!wol->wolopts) { > + device_set_wakeup_capable(priv->device, 0); > device_set_wakeup_enable(priv->device, 0); > disable_irq_wake(priv->wol_irq); > + } else { > + if (priv->plat->pmt && ((wol->wolopts & WAKE_MAGIC) || (wol->wolopts & WAKE_UCAST))) { > + pr_info("stmmac: mac wakeup enable\n"); > + enable_irq_wake(priv->wol_irq); > + } else { > + int ret = phylink_ethtool_set_wol(priv->phylink, wol); You can have multiple wake up sources enabled at the same time. So the if/else is wrong here. It could be, some are provided by the MAC and some by the PHY. If you are trying to save power, it is better if the PHY provides the WoL sources. If the PHY can provide all the required WoL sources, you can turn the MAC off and save more power. So give priority to the PHY. Andrew
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c index c5642985ef95..13430419ddfd 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c @@ -629,36 +629,28 @@ static void stmmac_get_strings(struct net_device *dev, u32 stringset, u8 *data) /* Currently only support WOL through Magic packet. */ static void stmmac_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol) { + struct ethtool_wolinfo wol_phy = { .cmd = ETHTOOL_GWOL }; struct stmmac_priv *priv = netdev_priv(dev); - if (!priv->plat->pmt) - return phylink_ethtool_get_wol(priv->phylink, wol); - 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); + + 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; - - if (!device_can_wakeup(priv->device)) - return -EOPNOTSUPP; - - if (!priv->plat->pmt) { - int ret = phylink_ethtool_set_wol(priv->phylink, wol); - - if (!ret) - device_set_wakeup_enable(priv->device, !!wol->wolopts); - return ret; - } + u32 support = WAKE_MAGIC | WAKE_UCAST | WAKE_PHY; /* By default almost all GMAC devices support the WoL via * magic frame but we can disable it if the HW capability @@ -669,11 +661,23 @@ static int stmmac_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) if (wol->wolopts & ~support) return -EINVAL; + if (wol->wolopts & WAKE_PHY) { + int ret = phylink_ethtool_set_wol(priv->phylink, wol); + + if (!ret) { + device_set_wakeup_capable(priv->device, 1); + device_set_wakeup_enable(priv->device, 1); + } + return ret; + } + if (wol->wolopts) { pr_info("stmmac: wakeup enable\n"); + device_set_wakeup_capable(priv->device, 1); device_set_wakeup_enable(priv->device, 1); enable_irq_wake(priv->wol_irq); } else { + device_set_wakeup_capable(priv->device, 0); device_set_wakeup_enable(priv->device, 0); disable_irq_wake(priv->wol_irq); } 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(), and set wakeup capability in stmmac_set_wol() when enable WoL. 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> --- .../ethernet/stmicro/stmmac/stmmac_ethtool.c | 38 ++++++++++--------- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 8 +--- 2 files changed, 22 insertions(+), 24 deletions(-)