[PATCH v4 0/5] net: macb: Wake-on-Lan magic packet fixes and GEM handling

[PATCH v4 0/5] net: macb: Wake-on-Lan magic packet fixes and GEM handling

[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

[PATCH v4 0/5] net: macb: Wake-on-Lan magic packet fixes and GEM handling

[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 1/5] net: macb: fix wakeup test in runtime suspend/resume routines

[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

[PATCH v4 1/5] net: macb: fix wakeup test in runtime suspend/resume routines

[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 2/5] net: macb: mark device wake capable when "magic-packet" property present

[nicolas.ferre]

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

[PATCH v4 2/5] net: macb: mark device wake capable when "magic-packet" property present

[nicolas.ferre]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 3/5] net: macb: fix macb_get/set_wol() when moving to phylink

[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

[PATCH v4 3/5] net: macb: fix macb_get/set_wol() when moving to phylink

[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 4/5] net: macb: fix macb_suspend() by removing call to netif_carrier_off()

[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

[PATCH v4 4/5] net: macb: fix macb_suspend() by removing call to netif_carrier_off()

[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 5/5] net: macb: fix call to pm_runtime in the suspend/resume functions

[nicolas.ferre]

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

[PATCH v4 5/5] net: macb: fix call to pm_runtime in the suspend/resume functions

[nicolas.ferre]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/5] net: macb: fix wakeup test in runtime suspend/resume routines

[Jakub Kicinski]

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

Re: [PATCH v4 1/5] net: macb: fix wakeup test in runtime suspend/resume routines

[Jakub Kicinski]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/5] net: macb: fix wakeup test in runtime suspend/resume routines

[Nicolas Ferre]

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

Re: [PATCH v4 1/5] net: macb: fix wakeup test in runtime suspend/resume routines

[Nicolas Ferre]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/5] net: macb: fix macb_get/set_wol() when moving to phylink

[Russell King - ARM Linux admin]

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

Re: [PATCH v4 3/5] net: macb: fix macb_get/set_wol() when moving to phylink

[Russell King - ARM Linux admin]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/5] net: macb: fix macb_get/set_wol() when moving to phylink

[Nicolas Ferre]

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

Re: [PATCH v4 3/5] net: macb: fix macb_get/set_wol() when moving to phylink

[Nicolas Ferre]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/5] net: macb: fix wakeup test in runtime suspend/resume routines

[Nicolas Ferre]

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

Re: [PATCH v4 1/5] net: macb: fix wakeup test in runtime suspend/resume routines

[Nicolas Ferre]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/5] net: macb: fix macb_get/set_wol() when moving to phylink

[Russell King - ARM Linux admin]

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

Re: [PATCH v4 3/5] net: macb: fix macb_get/set_wol() when moving to phylink

[Russell King - ARM Linux admin]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/5] net: macb: fix wakeup test in runtime suspend/resume routines

[Russell King - ARM Linux admin]

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

Re: [PATCH v4 1/5] net: macb: fix wakeup test in runtime suspend/resume routines

[Russell King - ARM Linux admin]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel