* [PATCH v4 0/5] net: macb: Wake-on-Lan magic packet fixes and GEM handling @ 2020-05-06 11:37 nicolas.ferre 2020-05-06 11:37 ` [PATCH v4 1/5] net: macb: fix wakeup test in runtime suspend/resume routines nicolas.ferre ` (4 more replies) 0 siblings, 5 replies; 13+ messages in thread From: nicolas.ferre @ 2020-05-06 11:37 UTC (permalink / raw) To: linux-arm-kernel, netdev, Claudiu Beznea, harini.katakam Cc: linux-kernel, David S. Miller, Alexandre Belloni, antoine.tenart, f.fainelli, Nicolas Ferre From: Nicolas Ferre <nicolas.ferre@microchip.com> Hi, Here is a split series to fix WoL magic-packet on the current macb driver. Only fixes in this one based on current net/master. Best regards, Nicolas Changes in v4: - Pure bug fix series for 'net'. GEM addition and MACB update removed: will be sent later. Changes in v3: - Revert some of the v2 changes done in macb_resume(). Now the resume function supports in-depth re-configuration of the controller in order to deal with deeper sleep states. Basically as it was before changes introduced by this series - Tested for non-regression with our deeper Power Management mode which cuts power to the controller completely Changes in v2: - Add patch 4/7 ("net: macb: fix macb_suspend() by removing call to netif_carrier_off()") needed for keeping phy state consistent - Add patch 5/7 ("net: macb: fix call to pm_runtime in the suspend/resume functions") that prevent putting the macb in runtime pm suspend mode when WoL is used - Collect review tags on 3 first patches from Florian: Thanks! - Review of macb_resume() function - Addition of pm_wakeup_event() in both MACB and GEM WoL IRQ handlers Nicolas Ferre (5): net: macb: fix wakeup test in runtime suspend/resume routines net: macb: mark device wake capable when "magic-packet" property present net: macb: fix macb_get/set_wol() when moving to phylink net: macb: fix macb_suspend() by removing call to netif_carrier_off() net: macb: fix call to pm_runtime in the suspend/resume functions drivers/net/ethernet/cadence/macb_main.c | 31 +++++++++++++----------- 1 file changed, 17 insertions(+), 14 deletions(-) -- 2.26.2 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 1/5] net: macb: fix wakeup test in runtime suspend/resume routines 2020-05-06 11:37 [PATCH v4 0/5] net: macb: Wake-on-Lan magic packet fixes and GEM handling nicolas.ferre @ 2020-05-06 11:37 ` nicolas.ferre 2020-05-06 20:18 ` Jakub Kicinski 2020-05-06 11:37 ` [PATCH v4 2/5] net: macb: mark device wake capable when "magic-packet" property present nicolas.ferre ` (3 subsequent siblings) 4 siblings, 1 reply; 13+ messages in thread From: nicolas.ferre @ 2020-05-06 11:37 UTC (permalink / raw) To: linux-arm-kernel, netdev, Claudiu Beznea, harini.katakam Cc: linux-kernel, David S. Miller, Alexandre Belloni, antoine.tenart, f.fainelli, Nicolas Ferre From: Nicolas Ferre <nicolas.ferre@microchip.com> Use the proper struct device pointer to check if the wakeup flag and wakeup source are positioned. Use the one passed by function call which is equivalent to &bp->dev->dev.parent. It's preventing the trigger of a spurious interrupt in case the Wake-on-Lan feature is used. Fixes: bc1109d04c39 ("net: macb: Add pm runtime support") Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> Cc: Claudiu Beznea <claudiu.beznea@microchip.com> Cc: Harini Katakam <harini.katakam@xilinx.com> --- drivers/net/ethernet/cadence/macb_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index 36290a8e2a84..d11fae37d46b 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -4616,7 +4616,7 @@ static int __maybe_unused macb_runtime_suspend(struct device *dev) struct net_device *netdev = dev_get_drvdata(dev); struct macb *bp = netdev_priv(netdev); - if (!(device_may_wakeup(&bp->dev->dev))) { + if (!(device_may_wakeup(dev))) { clk_disable_unprepare(bp->tx_clk); clk_disable_unprepare(bp->hclk); clk_disable_unprepare(bp->pclk); @@ -4632,7 +4632,7 @@ static int __maybe_unused macb_runtime_resume(struct device *dev) struct net_device *netdev = dev_get_drvdata(dev); struct macb *bp = netdev_priv(netdev); - if (!(device_may_wakeup(&bp->dev->dev))) { + if (!(device_may_wakeup(dev))) { clk_prepare_enable(bp->pclk); clk_prepare_enable(bp->hclk); clk_prepare_enable(bp->tx_clk); -- 2.26.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/5] net: macb: fix wakeup test in runtime suspend/resume routines 2020-05-06 11:37 ` [PATCH v4 1/5] net: macb: fix wakeup test in runtime suspend/resume routines nicolas.ferre @ 2020-05-06 20:18 ` Jakub Kicinski 2020-05-07 10:03 ` Nicolas Ferre 0 siblings, 1 reply; 13+ messages in thread From: Jakub Kicinski @ 2020-05-06 20:18 UTC (permalink / raw) To: nicolas.ferre Cc: linux-arm-kernel, netdev, Claudiu Beznea, harini.katakam, linux-kernel, David S. Miller, Alexandre Belloni, antoine.tenart, f.fainelli On Wed, 6 May 2020 13:37:37 +0200 nicolas.ferre@microchip.com wrote: > From: Nicolas Ferre <nicolas.ferre@microchip.com> > > Use the proper struct device pointer to check if the wakeup flag > and wakeup source are positioned. > Use the one passed by function call which is equivalent to > &bp->dev->dev.parent. > > It's preventing the trigger of a spurious interrupt in case the > Wake-on-Lan feature is used. > > Fixes: bc1109d04c39 ("net: macb: Add pm runtime support") Fixes tag: Fixes: bc1109d04c39 ("net: macb: Add pm runtime support") Has these problem(s): - Target SHA1 does not exist > Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > Cc: Claudiu Beznea <claudiu.beznea@microchip.com> > Cc: Harini Katakam <harini.katakam@xilinx.com> > --- > drivers/net/ethernet/cadence/macb_main.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index 36290a8e2a84..d11fae37d46b 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -4616,7 +4616,7 @@ static int __maybe_unused macb_runtime_suspend(struct device *dev) > struct net_device *netdev = dev_get_drvdata(dev); > struct macb *bp = netdev_priv(netdev); > > - if (!(device_may_wakeup(&bp->dev->dev))) { > + if (!(device_may_wakeup(dev))) { > clk_disable_unprepare(bp->tx_clk); > clk_disable_unprepare(bp->hclk); > clk_disable_unprepare(bp->pclk); > @@ -4632,7 +4632,7 @@ static int __maybe_unused macb_runtime_resume(struct device *dev) > struct net_device *netdev = dev_get_drvdata(dev); > struct macb *bp = netdev_priv(netdev); > > - if (!(device_may_wakeup(&bp->dev->dev))) { > + if (!(device_may_wakeup(dev))) { > clk_prepare_enable(bp->pclk); > clk_prepare_enable(bp->hclk); > clk_prepare_enable(bp->tx_clk); ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/5] net: macb: fix wakeup test in runtime suspend/resume routines 2020-05-06 20:18 ` Jakub Kicinski @ 2020-05-07 10:03 ` Nicolas Ferre 2020-05-25 8:18 ` Nicolas Ferre 0 siblings, 1 reply; 13+ messages in thread From: Nicolas Ferre @ 2020-05-07 10:03 UTC (permalink / raw) To: Jakub Kicinski, netdev, David S. Miller Cc: linux-arm-kernel, Claudiu Beznea, harini.katakam, linux-kernel, Alexandre Belloni, antoine.tenart, f.fainelli On 06/05/2020 at 22:18, Jakub Kicinski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Wed, 6 May 2020 13:37:37 +0200 nicolas.ferre@microchip.com wrote: >> From: Nicolas Ferre <nicolas.ferre@microchip.com> >> >> Use the proper struct device pointer to check if the wakeup flag >> and wakeup source are positioned. >> Use the one passed by function call which is equivalent to >> &bp->dev->dev.parent. >> >> It's preventing the trigger of a spurious interrupt in case the >> Wake-on-Lan feature is used. >> >> Fixes: bc1109d04c39 ("net: macb: Add pm runtime support") > > Fixes tag: Fixes: bc1109d04c39 ("net: macb: Add pm runtime support") > Has these problem(s): > - Target SHA1 does not exist Indeed, it's: Fixes: d54f89af6cc4 ("net: macb: Add pm runtime support") David: do I have to respin or you can modify it? Thanks. Regards, Nicolas >> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com> >> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> >> Cc: Claudiu Beznea <claudiu.beznea@microchip.com> >> Cc: Harini Katakam <harini.katakam@xilinx.com> >> --- >> drivers/net/ethernet/cadence/macb_main.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c >> index 36290a8e2a84..d11fae37d46b 100644 >> --- a/drivers/net/ethernet/cadence/macb_main.c >> +++ b/drivers/net/ethernet/cadence/macb_main.c >> @@ -4616,7 +4616,7 @@ static int __maybe_unused macb_runtime_suspend(struct device *dev) >> struct net_device *netdev = dev_get_drvdata(dev); >> struct macb *bp = netdev_priv(netdev); >> >> - if (!(device_may_wakeup(&bp->dev->dev))) { >> + if (!(device_may_wakeup(dev))) { >> clk_disable_unprepare(bp->tx_clk); >> clk_disable_unprepare(bp->hclk); >> clk_disable_unprepare(bp->pclk); >> @@ -4632,7 +4632,7 @@ static int __maybe_unused macb_runtime_resume(struct device *dev) >> struct net_device *netdev = dev_get_drvdata(dev); >> struct macb *bp = netdev_priv(netdev); >> >> - if (!(device_may_wakeup(&bp->dev->dev))) { >> + if (!(device_may_wakeup(dev))) { >> clk_prepare_enable(bp->pclk); >> clk_prepare_enable(bp->hclk); >> clk_prepare_enable(bp->tx_clk); > -- Nicolas Ferre ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/5] net: macb: fix wakeup test in runtime suspend/resume routines 2020-05-07 10:03 ` Nicolas Ferre @ 2020-05-25 8:18 ` Nicolas Ferre 2020-05-25 8:47 ` Russell King - ARM Linux admin 0 siblings, 1 reply; 13+ messages in thread From: Nicolas Ferre @ 2020-05-25 8:18 UTC (permalink / raw) To: Jakub Kicinski, netdev, David S. Miller, f.fainelli, Russell King - ARM Linux admin Cc: Alexandre Belloni, antoine.tenart, linux-kernel, harini.katakam, Claudiu Beznea, linux-arm-kernel On 07/05/2020 at 12:03, Nicolas Ferre wrote: > On 06/05/2020 at 22:18, Jakub Kicinski wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> On Wed, 6 May 2020 13:37:37 +0200 nicolas.ferre@microchip.com wrote: >>> From: Nicolas Ferre <nicolas.ferre@microchip.com> >>> >>> Use the proper struct device pointer to check if the wakeup flag >>> and wakeup source are positioned. >>> Use the one passed by function call which is equivalent to >>> &bp->dev->dev.parent. >>> >>> It's preventing the trigger of a spurious interrupt in case the >>> Wake-on-Lan feature is used. >>> >>> Fixes: bc1109d04c39 ("net: macb: Add pm runtime support") >> >> Fixes tag: Fixes: bc1109d04c39 ("net: macb: Add pm runtime support") >> Has these problem(s): >> - Target SHA1 does not exist > > Indeed, it's: > Fixes: d54f89af6cc4 ("net: macb: Add pm runtime support") > > David: do I have to respin or you can modify it? David, all, I'm about to resend this series (alternative to "ping"), however: 1/ Now that it's late in the cycle, I'd like that you tell me if I rebase on net-next because it isn't not sensible to queue such (non urgeent) changes at rc7 2/ I didn't get answers from Russell and can't tell if there's a better way of handling underlying phylink error of phylink_ethtool_set_wol() in patch 3/5 Best regards, Nicolas >>> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com> >>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> >>> Cc: Claudiu Beznea <claudiu.beznea@microchip.com> >>> Cc: Harini Katakam <harini.katakam@xilinx.com> >>> --- >>> drivers/net/ethernet/cadence/macb_main.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c >>> index 36290a8e2a84..d11fae37d46b 100644 >>> --- a/drivers/net/ethernet/cadence/macb_main.c >>> +++ b/drivers/net/ethernet/cadence/macb_main.c >>> @@ -4616,7 +4616,7 @@ static int __maybe_unused macb_runtime_suspend(struct device *dev) >>> struct net_device *netdev = dev_get_drvdata(dev); >>> struct macb *bp = netdev_priv(netdev); >>> >>> - if (!(device_may_wakeup(&bp->dev->dev))) { >>> + if (!(device_may_wakeup(dev))) { >>> clk_disable_unprepare(bp->tx_clk); >>> clk_disable_unprepare(bp->hclk); >>> clk_disable_unprepare(bp->pclk); >>> @@ -4632,7 +4632,7 @@ static int __maybe_unused macb_runtime_resume(struct device *dev) >>> struct net_device *netdev = dev_get_drvdata(dev); >>> struct macb *bp = netdev_priv(netdev); >>> >>> - if (!(device_may_wakeup(&bp->dev->dev))) { >>> + if (!(device_may_wakeup(dev))) { >>> clk_prepare_enable(bp->pclk); >>> clk_prepare_enable(bp->hclk); >>> clk_prepare_enable(bp->tx_clk); >> > > -- Nicolas Ferre ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/5] net: macb: fix wakeup test in runtime suspend/resume routines 2020-05-25 8:18 ` Nicolas Ferre @ 2020-05-25 8:47 ` Russell King - ARM Linux admin 0 siblings, 0 replies; 13+ messages in thread From: Russell King - ARM Linux admin @ 2020-05-25 8:47 UTC (permalink / raw) To: Nicolas Ferre Cc: Jakub Kicinski, netdev, David S. Miller, f.fainelli, Alexandre Belloni, antoine.tenart, linux-kernel, harini.katakam, Claudiu Beznea, linux-arm-kernel On Mon, May 25, 2020 at 10:18:16AM +0200, Nicolas Ferre wrote: > On 07/05/2020 at 12:03, Nicolas Ferre wrote: > > On 06/05/2020 at 22:18, Jakub Kicinski wrote: > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > > > On Wed, 6 May 2020 13:37:37 +0200 nicolas.ferre@microchip.com wrote: > > > > From: Nicolas Ferre <nicolas.ferre@microchip.com> > > > > > > > > Use the proper struct device pointer to check if the wakeup flag > > > > and wakeup source are positioned. > > > > Use the one passed by function call which is equivalent to > > > > &bp->dev->dev.parent. > > > > > > > > It's preventing the trigger of a spurious interrupt in case the > > > > Wake-on-Lan feature is used. > > > > > > > > Fixes: bc1109d04c39 ("net: macb: Add pm runtime support") > > > > > > Fixes tag: Fixes: bc1109d04c39 ("net: macb: Add pm runtime support") > > > Has these problem(s): > > > - Target SHA1 does not exist > > > > Indeed, it's: > > Fixes: d54f89af6cc4 ("net: macb: Add pm runtime support") > > > > David: do I have to respin or you can modify it? > > David, all, I'm about to resend this series (alternative to "ping"), > however: > > 1/ Now that it's late in the cycle, I'd like that you tell me if I rebase on > net-next because it isn't not sensible to queue such (non urgeent) changes > at rc7 > > 2/ I didn't get answers from Russell and can't tell if there's a better way > of handling underlying phylink error of phylink_ethtool_set_wol() in patch > 3/5 I think you could have answered your own questions there, but I seemed easier to send an email. I've just read the code, typed out an appropriate description of the code's behaviour, and then derived the answer to your questions without anything else. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 2/5] net: macb: mark device wake capable when "magic-packet" property present 2020-05-06 11:37 [PATCH v4 0/5] net: macb: Wake-on-Lan magic packet fixes and GEM handling nicolas.ferre 2020-05-06 11:37 ` [PATCH v4 1/5] net: macb: fix wakeup test in runtime suspend/resume routines nicolas.ferre @ 2020-05-06 11:37 ` nicolas.ferre 2020-05-06 11:37 ` [PATCH v4 3/5] net: macb: fix macb_get/set_wol() when moving to phylink nicolas.ferre ` (2 subsequent siblings) 4 siblings, 0 replies; 13+ messages in thread From: nicolas.ferre @ 2020-05-06 11:37 UTC (permalink / raw) To: linux-arm-kernel, netdev, Claudiu Beznea, harini.katakam Cc: linux-kernel, David S. Miller, Alexandre Belloni, antoine.tenart, f.fainelli, Nicolas Ferre, Sergio Prado From: Nicolas Ferre <nicolas.ferre@microchip.com> Change the way the "magic-packet" DT property is handled in the macb_probe() function, matching DT binding documentation. Now we mark the device as "wakeup capable" instead of calling the device_init_wakeup() function that would enable the wakeup source. For Ethernet WoL, enabling the wakeup_source is done by using ethtool and associated macb_set_wol() function that already calls device_set_wakeup_enable() for this purpose. That would reduce power consumption by cutting more clocks if "magic-packet" property is set but WoL is not configured by ethtool. Fixes: 3e2a5e153906 ("net: macb: add wake-on-lan support via magic packet") Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> Cc: Claudiu Beznea <claudiu.beznea@microchip.com> Cc: Harini Katakam <harini.katakam@xilinx.com> Cc: Sergio Prado <sergio.prado@e-labworks.com> --- drivers/net/ethernet/cadence/macb_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index d11fae37d46b..53e81ab048ae 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -4384,7 +4384,7 @@ static int macb_probe(struct platform_device *pdev) bp->wol = 0; if (of_get_property(np, "magic-packet", NULL)) bp->wol |= MACB_WOL_HAS_MAGIC_PACKET; - device_init_wakeup(&pdev->dev, bp->wol & MACB_WOL_HAS_MAGIC_PACKET); + device_set_wakeup_capable(&pdev->dev, bp->wol & MACB_WOL_HAS_MAGIC_PACKET); spin_lock_init(&bp->lock); -- 2.26.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 3/5] net: macb: fix macb_get/set_wol() when moving to phylink 2020-05-06 11:37 [PATCH v4 0/5] net: macb: Wake-on-Lan magic packet fixes and GEM handling nicolas.ferre 2020-05-06 11:37 ` [PATCH v4 1/5] net: macb: fix wakeup test in runtime suspend/resume routines nicolas.ferre 2020-05-06 11:37 ` [PATCH v4 2/5] net: macb: mark device wake capable when "magic-packet" property present nicolas.ferre @ 2020-05-06 11:37 ` nicolas.ferre 2020-05-13 13:05 ` Russell King - ARM Linux admin 2020-05-06 11:37 ` [PATCH v4 4/5] net: macb: fix macb_suspend() by removing call to netif_carrier_off() nicolas.ferre 2020-05-06 11:37 ` [PATCH v4 5/5] net: macb: fix call to pm_runtime in the suspend/resume functions nicolas.ferre 4 siblings, 1 reply; 13+ messages in thread From: nicolas.ferre @ 2020-05-06 11:37 UTC (permalink / raw) To: linux-arm-kernel, netdev, Claudiu Beznea, harini.katakam Cc: linux-kernel, David S. Miller, Alexandre Belloni, antoine.tenart, f.fainelli, Nicolas Ferre From: Nicolas Ferre <nicolas.ferre@microchip.com> Keep previous function goals and integrate phylink actions to them. phylink_ethtool_get_wol() is not enough to figure out if Ethernet driver supports Wake-on-Lan. Initialization of "supported" and "wolopts" members is done in phylink function, no need to keep them in calling function. phylink_ethtool_set_wol() return value is not enough to determine if WoL is enabled for the calling Ethernet driver. Call it first but don't rely on its return value as most of simple PHY drivers don't implement a set_wol() function. Fixes: 7897b071ac3b ("net: macb: convert to phylink") Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> Cc: Claudiu Beznea <claudiu.beznea@microchip.com> Cc: Harini Katakam <harini.katakam@xilinx.com> Cc: Antoine Tenart <antoine.tenart@bootlin.com> --- drivers/net/ethernet/cadence/macb_main.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index 53e81ab048ae..24c044dc7fa0 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -2817,21 +2817,23 @@ static void macb_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol) { struct macb *bp = netdev_priv(netdev); - wol->supported = 0; - wol->wolopts = 0; - - if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET) + if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET) { phylink_ethtool_get_wol(bp->phylink, wol); + wol->supported |= WAKE_MAGIC; + + if (bp->wol & MACB_WOL_ENABLED) + wol->wolopts |= WAKE_MAGIC; + } } static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol) { struct macb *bp = netdev_priv(netdev); - int ret; - ret = phylink_ethtool_set_wol(bp->phylink, wol); - if (!ret) - return 0; + /* Pass the order to phylink layer. + * Don't test return value as set_wol() is often not supported. + */ + phylink_ethtool_set_wol(bp->phylink, wol); if (!(bp->wol & MACB_WOL_HAS_MAGIC_PACKET) || (wol->wolopts & ~WAKE_MAGIC)) -- 2.26.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v4 3/5] net: macb: fix macb_get/set_wol() when moving to phylink 2020-05-06 11:37 ` [PATCH v4 3/5] net: macb: fix macb_get/set_wol() when moving to phylink nicolas.ferre @ 2020-05-13 13:05 ` Russell King - ARM Linux admin 2020-05-13 14:16 ` Nicolas Ferre 0 siblings, 1 reply; 13+ messages in thread From: Russell King - ARM Linux admin @ 2020-05-13 13:05 UTC (permalink / raw) To: nicolas.ferre Cc: linux-arm-kernel, netdev, Claudiu Beznea, harini.katakam, Alexandre Belloni, f.fainelli, antoine.tenart, linux-kernel, David S. Miller On Wed, May 06, 2020 at 01:37:39PM +0200, nicolas.ferre@microchip.com wrote: > From: Nicolas Ferre <nicolas.ferre@microchip.com> > > Keep previous function goals and integrate phylink actions to them. > > phylink_ethtool_get_wol() is not enough to figure out if Ethernet driver > supports Wake-on-Lan. > Initialization of "supported" and "wolopts" members is done in phylink > function, no need to keep them in calling function. > > phylink_ethtool_set_wol() return value is not enough to determine > if WoL is enabled for the calling Ethernet driver. Call it first > but don't rely on its return value as most of simple PHY drivers > don't implement a set_wol() function. > > Fixes: 7897b071ac3b ("net: macb: convert to phylink") > Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > Cc: Claudiu Beznea <claudiu.beznea@microchip.com> > Cc: Harini Katakam <harini.katakam@xilinx.com> > Cc: Antoine Tenart <antoine.tenart@bootlin.com> > --- > drivers/net/ethernet/cadence/macb_main.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index 53e81ab048ae..24c044dc7fa0 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -2817,21 +2817,23 @@ static void macb_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol) > { > struct macb *bp = netdev_priv(netdev); > > - wol->supported = 0; > - wol->wolopts = 0; > - > - if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET) > + if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET) { > phylink_ethtool_get_wol(bp->phylink, wol); > + wol->supported |= WAKE_MAGIC; > + > + if (bp->wol & MACB_WOL_ENABLED) > + wol->wolopts |= WAKE_MAGIC; > + } > } > > static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol) > { > struct macb *bp = netdev_priv(netdev); > - int ret; > > - ret = phylink_ethtool_set_wol(bp->phylink, wol); > - if (!ret) > - return 0; > + /* Pass the order to phylink layer. > + * Don't test return value as set_wol() is often not supported. > + */ > + phylink_ethtool_set_wol(bp->phylink, wol); If this returns an error, does that mean WOL works or does it not? Note that if set_wol() is not supported, this will return -EOPNOTSUPP. What about other errors? If you want to just ignore the case where it's not supported, then this looks like a sledge hammer to crack a nut. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 3/5] net: macb: fix macb_get/set_wol() when moving to phylink 2020-05-13 13:05 ` Russell King - ARM Linux admin @ 2020-05-13 14:16 ` Nicolas Ferre 2020-05-25 8:41 ` Russell King - ARM Linux admin 0 siblings, 1 reply; 13+ messages in thread From: Nicolas Ferre @ 2020-05-13 14:16 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: linux-arm-kernel, netdev, Claudiu Beznea, harini.katakam, Alexandre Belloni, f.fainelli, antoine.tenart, linux-kernel, David S. Miller Russell, Thanks for the feedback. On 13/05/2020 at 15:05, Russell King - ARM Linux admin wrote: > On Wed, May 06, 2020 at 01:37:39PM +0200, nicolas.ferre@microchip.com wrote: >> From: Nicolas Ferre <nicolas.ferre@microchip.com> >> >> Keep previous function goals and integrate phylink actions to them. >> >> phylink_ethtool_get_wol() is not enough to figure out if Ethernet driver >> supports Wake-on-Lan. >> Initialization of "supported" and "wolopts" members is done in phylink >> function, no need to keep them in calling function. >> >> phylink_ethtool_set_wol() return value is not enough to determine >> if WoL is enabled for the calling Ethernet driver. Call it first >> but don't rely on its return value as most of simple PHY drivers >> don't implement a set_wol() function. >> >> Fixes: 7897b071ac3b ("net: macb: convert to phylink") >> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com> >> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> >> Cc: Claudiu Beznea <claudiu.beznea@microchip.com> >> Cc: Harini Katakam <harini.katakam@xilinx.com> >> Cc: Antoine Tenart <antoine.tenart@bootlin.com> >> --- >> drivers/net/ethernet/cadence/macb_main.c | 18 ++++++++++-------- >> 1 file changed, 10 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c >> index 53e81ab048ae..24c044dc7fa0 100644 >> --- a/drivers/net/ethernet/cadence/macb_main.c >> +++ b/drivers/net/ethernet/cadence/macb_main.c >> @@ -2817,21 +2817,23 @@ static void macb_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol) >> { >> struct macb *bp = netdev_priv(netdev); >> >> - wol->supported = 0; >> - wol->wolopts = 0; >> - >> - if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET) >> + if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET) { >> phylink_ethtool_get_wol(bp->phylink, wol); >> + wol->supported |= WAKE_MAGIC; >> + >> + if (bp->wol & MACB_WOL_ENABLED) >> + wol->wolopts |= WAKE_MAGIC; >> + } >> } >> >> static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol) >> { >> struct macb *bp = netdev_priv(netdev); >> - int ret; >> >> - ret = phylink_ethtool_set_wol(bp->phylink, wol); >> - if (!ret) >> - return 0; >> + /* Pass the order to phylink layer. >> + * Don't test return value as set_wol() is often not supported. >> + */ >> + phylink_ethtool_set_wol(bp->phylink, wol); > > If this returns an error, does that mean WOL works or does it not? In my use case (simple phy: "Micrel KSZ8081"), if I have the error "-EOPNOTSUPP", it simply means that this phy driver doesn't have the set_wol() function. But on the MAC side, I can perfectly wake-up on WoL event as the phy acts as a pass-through. > Note that if set_wol() is not supported, this will return -EOPNOTSUPP. > What about other errors? True, I don't manage them. But for now this patch is a fix that only reverts to previous behavior. In other terms, it only fixes the regression. But can I make the difference, and how, between? 1/ the phy doesn't support WoL and could prevent the WoL to happen on the MAC 2/ the phy doesn't implement (yet) the set_wol() function, if MAC can manage, it's fine > If you want to just ignore the case where it's not supported, then > this looks like a sledge hammer to crack a nut. Do you suggest that I just don't call phylink_ethtool_set_wol() at all? But what if the underlying phy does support WoL? Best regards, -- Nicolas Ferre ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 3/5] net: macb: fix macb_get/set_wol() when moving to phylink 2020-05-13 14:16 ` Nicolas Ferre @ 2020-05-25 8:41 ` Russell King - ARM Linux admin 0 siblings, 0 replies; 13+ messages in thread From: Russell King - ARM Linux admin @ 2020-05-25 8:41 UTC (permalink / raw) To: Nicolas Ferre Cc: linux-arm-kernel, netdev, Claudiu Beznea, harini.katakam, Alexandre Belloni, f.fainelli, antoine.tenart, linux-kernel, David S. Miller On Wed, May 13, 2020 at 04:16:04PM +0200, Nicolas Ferre wrote: > Russell, > > Thanks for the feedback. > > On 13/05/2020 at 15:05, Russell King - ARM Linux admin wrote: > > On Wed, May 06, 2020 at 01:37:39PM +0200, nicolas.ferre@microchip.com wrote: > > > From: Nicolas Ferre <nicolas.ferre@microchip.com> > > > > > > Keep previous function goals and integrate phylink actions to them. > > > > > > phylink_ethtool_get_wol() is not enough to figure out if Ethernet driver > > > supports Wake-on-Lan. > > > Initialization of "supported" and "wolopts" members is done in phylink > > > function, no need to keep them in calling function. > > > > > > phylink_ethtool_set_wol() return value is not enough to determine > > > if WoL is enabled for the calling Ethernet driver. Call it first > > > but don't rely on its return value as most of simple PHY drivers > > > don't implement a set_wol() function. > > > > > > Fixes: 7897b071ac3b ("net: macb: convert to phylink") > > > Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com> > > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > > Cc: Claudiu Beznea <claudiu.beznea@microchip.com> > > > Cc: Harini Katakam <harini.katakam@xilinx.com> > > > Cc: Antoine Tenart <antoine.tenart@bootlin.com> > > > --- > > > drivers/net/ethernet/cadence/macb_main.c | 18 ++++++++++-------- > > > 1 file changed, 10 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > > > index 53e81ab048ae..24c044dc7fa0 100644 > > > --- a/drivers/net/ethernet/cadence/macb_main.c > > > +++ b/drivers/net/ethernet/cadence/macb_main.c > > > @@ -2817,21 +2817,23 @@ static void macb_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol) > > > { > > > struct macb *bp = netdev_priv(netdev); > > > > > > - wol->supported = 0; > > > - wol->wolopts = 0; > > > - > > > - if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET) > > > + if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET) { > > > phylink_ethtool_get_wol(bp->phylink, wol); > > > + wol->supported |= WAKE_MAGIC; > > > + > > > + if (bp->wol & MACB_WOL_ENABLED) > > > + wol->wolopts |= WAKE_MAGIC; > > > + } > > > } > > > > > > static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol) > > > { > > > struct macb *bp = netdev_priv(netdev); > > > - int ret; > > > > > > - ret = phylink_ethtool_set_wol(bp->phylink, wol); > > > - if (!ret) > > > - return 0; > > > + /* Pass the order to phylink layer. > > > + * Don't test return value as set_wol() is often not supported. > > > + */ > > > + phylink_ethtool_set_wol(bp->phylink, wol); > > > > If this returns an error, does that mean WOL works or does it not? > > In my use case (simple phy: "Micrel KSZ8081"), if I have the error > "-EOPNOTSUPP", it simply means that this phy driver doesn't have the > set_wol() function. But on the MAC side, I can perfectly wake-up on WoL > event as the phy acts as a pass-through. > > > Note that if set_wol() is not supported, this will return -EOPNOTSUPP. > > What about other errors? > > True, I don't manage them. But for now this patch is a fix that only reverts > to previous behavior. In other terms, it only fixes the regression. > > But can I make the difference, and how, between? > 1/ the phy doesn't support WoL and could prevent the WoL to happen on the > MAC > 2/ the phy doesn't implement (yet) the set_wol() function, if MAC can > manage, it's fine I think you need to read and understand the code, but don't worry, I'll do it for you. There are not that many implementations in phylib, so it doesn't take long: m88e1318_set_wol(), dp83867_set_wol(), dp83822_set_wol(), at803x_set_wol(), lan88xx_set_wol(), and vsc85xx_wol_set(). For case 2, phylib returns -EOPNOTSUPP. m88e1318_set_wol() returns zero on success, or propagates an error from the MDIO bus accessors. dp83867_set_wol() returns zero on success, or -EINVAL if the MAC address is invalid. No bus errors are propagated. dp83822_set_wol() is the same as dp83867_set_wol(). at803x_set_wol() returns zero on success, or -ENODEV if there is no netdev attached (which means you shouldn't be calling this anyway), -EINVAL if the MAC address is invalid, or sometimes propagates an error from the MDIO bus accessors. lan88xx_set_wol() always returns zero, but the function does nothing other than saving the requested state, and uses that to avoid calling genphy_suspend() for this PHY. vsc85xx_wol_set() returns zero on success, or propagates an error from the MDIO bus accessors. So, what we can tell from the return code is: - If it returned zero, the PHY likely supports and properly configured WoL, and you may not need to configure the MAC (depends on whether the PHY can wake the system up on its own.) - If it returns -EOPNOTSUPP, there is no support for WoL at the PHY, and you need to program your MAC - assuming that the PHY is going to stay alive. - If it returns some other error code, there was a failure of some sort to configure the PHY for WoL, which probably means the PHY is not responding, and probably means the system isn't going to be capable of waking up through this PHY. For case 1, there is no code path that detects whether the PHY concerned supports WoL or doesn't - the code paths in each driver assume that if the PHY supports WoL, then it supports WoL. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 4/5] net: macb: fix macb_suspend() by removing call to netif_carrier_off() 2020-05-06 11:37 [PATCH v4 0/5] net: macb: Wake-on-Lan magic packet fixes and GEM handling nicolas.ferre ` (2 preceding siblings ...) 2020-05-06 11:37 ` [PATCH v4 3/5] net: macb: fix macb_get/set_wol() when moving to phylink nicolas.ferre @ 2020-05-06 11:37 ` nicolas.ferre 2020-05-06 11:37 ` [PATCH v4 5/5] net: macb: fix call to pm_runtime in the suspend/resume functions nicolas.ferre 4 siblings, 0 replies; 13+ messages in thread From: nicolas.ferre @ 2020-05-06 11:37 UTC (permalink / raw) To: linux-arm-kernel, netdev, Claudiu Beznea, harini.katakam Cc: linux-kernel, David S. Miller, Alexandre Belloni, antoine.tenart, f.fainelli, Nicolas Ferre From: Nicolas Ferre <nicolas.ferre@microchip.com> As we now use the phylink call to phylink_stop() in the non-WoL path, there is no need for this call to netif_carrier_off() anymore. It can disturb the underlying phylink FSM. Fixes: 7897b071ac3b ("net: macb: convert to phylink") Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> Cc: Claudiu Beznea <claudiu.beznea@microchip.com> Cc: Harini Katakam <harini.katakam@xilinx.com> Cc: Antoine Tenart <antoine.tenart@bootlin.com> --- Changes in v2: - new in v2 serries drivers/net/ethernet/cadence/macb_main.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index 24c044dc7fa0..ebc57cd5d286 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -4562,7 +4562,6 @@ static int __maybe_unused macb_suspend(struct device *dev) bp->pm_data.scrt2 = gem_readl_n(bp, ETHT, SCRT2_ETHT); } - netif_carrier_off(netdev); if (bp->ptp_info) bp->ptp_info->ptp_remove(netdev); pm_runtime_force_suspend(dev); -- 2.26.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 5/5] net: macb: fix call to pm_runtime in the suspend/resume functions 2020-05-06 11:37 [PATCH v4 0/5] net: macb: Wake-on-Lan magic packet fixes and GEM handling nicolas.ferre ` (3 preceding siblings ...) 2020-05-06 11:37 ` [PATCH v4 4/5] net: macb: fix macb_suspend() by removing call to netif_carrier_off() nicolas.ferre @ 2020-05-06 11:37 ` nicolas.ferre 4 siblings, 0 replies; 13+ messages in thread From: nicolas.ferre @ 2020-05-06 11:37 UTC (permalink / raw) To: linux-arm-kernel, netdev, Claudiu Beznea, harini.katakam Cc: linux-kernel, David S. Miller, Alexandre Belloni, antoine.tenart, f.fainelli, Nicolas Ferre, Sergio Prado From: Nicolas Ferre <nicolas.ferre@microchip.com> The calls to pm_runtime_force_suspend/resume() functions are only relevant if the device is not configured to act as a WoL wakeup source. Add the device_may_wakeup() test before calling them. Fixes: 3e2a5e153906 ("net: macb: add wake-on-lan support via magic packet") Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> Cc: Claudiu Beznea <claudiu.beznea@microchip.com> Cc: Harini Katakam <harini.katakam@xilinx.com> Cc: Sergio Prado <sergio.prado@e-labworks.com> --- Changes in v3: - remove the parenthesis around device_may_wakeup() Changes in v2: - new in v2 serries drivers/net/ethernet/cadence/macb_main.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index ebc57cd5d286..f01b9831b219 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -4564,7 +4564,8 @@ static int __maybe_unused macb_suspend(struct device *dev) if (bp->ptp_info) bp->ptp_info->ptp_remove(netdev); - pm_runtime_force_suspend(dev); + if (!device_may_wakeup(dev)) + pm_runtime_force_suspend(dev); return 0; } @@ -4579,7 +4580,8 @@ static int __maybe_unused macb_resume(struct device *dev) if (!netif_running(netdev)) return 0; - pm_runtime_force_resume(dev); + if (!device_may_wakeup(dev)) + pm_runtime_force_resume(dev); if (bp->wol & MACB_WOL_ENABLED) { macb_writel(bp, IDR, MACB_BIT(WOL)); -- 2.26.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-05-25 8:47 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-06 11:37 [PATCH v4 0/5] net: macb: Wake-on-Lan magic packet fixes and GEM handling nicolas.ferre 2020-05-06 11:37 ` [PATCH v4 1/5] net: macb: fix wakeup test in runtime suspend/resume routines nicolas.ferre 2020-05-06 20:18 ` Jakub Kicinski 2020-05-07 10:03 ` Nicolas Ferre 2020-05-25 8:18 ` Nicolas Ferre 2020-05-25 8:47 ` Russell King - ARM Linux admin 2020-05-06 11:37 ` [PATCH v4 2/5] net: macb: mark device wake capable when "magic-packet" property present nicolas.ferre 2020-05-06 11:37 ` [PATCH v4 3/5] net: macb: fix macb_get/set_wol() when moving to phylink nicolas.ferre 2020-05-13 13:05 ` Russell King - ARM Linux admin 2020-05-13 14:16 ` Nicolas Ferre 2020-05-25 8:41 ` Russell King - ARM Linux admin 2020-05-06 11:37 ` [PATCH v4 4/5] net: macb: fix macb_suspend() by removing call to netif_carrier_off() nicolas.ferre 2020-05-06 11:37 ` [PATCH v4 5/5] net: macb: fix call to pm_runtime in the suspend/resume functions nicolas.ferre
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).