[PATCH net 0/3] gianfar: wol magic packet fixes

STALE4102d

5 messages, 2 authors, 2015-07-31 · open the first message on its own page

[PATCH net 0/3] gianfar: wol magic packet fixes

From: Claudiu Manoil <hidden>
Date: 2015-07-31 15:39:26

These changes were already validated as part of FSL SDK.
Patch 2 fixes occasional wake-on magic packet failures during
traffic, probably due to incorrect traffic stop/ device halt
sequence and incorrect usage of txlock.

Claudiu Manoil (3):
  gianfar: Fix warning when CONFIG_PM off
  gianfar: Fix suspend/resume for wol magic packet
  gianfar: Enable device wakeup when appropriate

 drivers/net/ethernet/freescale/gianfar.c         | 104 +++++++----------------
 drivers/net/ethernet/freescale/gianfar.h         |   3 -
 drivers/net/ethernet/freescale/gianfar_ethtool.c |   5 +-
 3 files changed, 33 insertions(+), 79 deletions(-)

-- 
1.7.11.7

[PATCH net 1/3] gianfar: Fix warning when CONFIG_PM off

From: Claudiu Manoil <hidden>
Date: 2015-07-31 15:39:26

  CC      drivers/net/ethernet/freescale/gianfar.o
drivers/net/ethernet/freescale/gianfar.c:568:13: warning: 'lock_tx_qs'
defined but not used [-Wunused-function]
 static void lock_tx_qs(struct gfar_private *priv)
             ^
drivers/net/ethernet/freescale/gianfar.c:576:13: warning: 'unlock_tx_qs'
defined but not used [-Wunused-function]
 static void unlock_tx_qs(struct gfar_private *priv)
             ^

Reported-by: Scott Wood <redacted>
Signed-off-by: Claudiu Manoil <redacted>
---
 drivers/net/ethernet/freescale/gianfar.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index ff87502..caad327 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c

@@ -565,6 +565,7 @@ static void gfar_ints_enable(struct gfar_private *priv)
 	}
 }
 
+#ifdef CONFIG_PM
 static void lock_tx_qs(struct gfar_private *priv)
 {
 	int i;

@@ -580,6 +581,7 @@ static void unlock_tx_qs(struct gfar_private *priv)
 	for (i = 0; i < priv->num_tx_queues; i++)
 		spin_unlock(&priv->tx_queue[i]->txlock);
 }
+#endif
 
 static int gfar_alloc_tx_queues(struct gfar_private *priv)
 {

-- 
1.7.11.7

[PATCH net 3/3] gianfar: Enable device wakeup when appropriate

From: Claudiu Manoil <hidden>
Date: 2015-07-31 15:39:30

The wol_en flag is 0 by default anyway, and we have the
following inconsistency: a MAGIC packet wol capable eth
interface is registered as a wake-up source but unable
to wake-up the system as wol_en is 0 (wake-on flag set to 'd').
Calling set_wakeup_enable() at netdev open is just redundant
because wol_en is 0 by default.
Let only ethtool call set_wakeup_enable() for now.

The bflock is obviously obsoleted, its utility has been corroded
over time.  The bitfield flags used today in gianfar are accessed
only on the init/ config path, with no real possibility of
concurrency - nothing that would justify smth. like bflock.

Signed-off-by: Claudiu Manoil <redacted>
---
 drivers/net/ethernet/freescale/gianfar.c         | 8 ++------
 drivers/net/ethernet/freescale/gianfar.h         | 3 ---
 drivers/net/ethernet/freescale/gianfar_ethtool.c | 5 +----
 3 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 084c25d..2b7610f 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c

@@ -1360,7 +1360,6 @@ static int gfar_probe(struct platform_device *ofdev)
 	priv->dev = &ofdev->dev;
 	SET_NETDEV_DEV(dev, &ofdev->dev);
 
-	spin_lock_init(&priv->bflock);
 	INIT_WORK(&priv->reset_task, gfar_reset_task);
 
 	platform_set_drvdata(ofdev, priv);

@@ -1454,9 +1453,8 @@ static int gfar_probe(struct platform_device *ofdev)
 		goto register_fail;
 	}
 
-	device_init_wakeup(&dev->dev,
-			   priv->device_flags &
-			   FSL_GIANFAR_DEV_HAS_MAGIC_PACKET);
+	device_set_wakeup_capable(&dev->dev, priv->device_flags &
+				  FSL_GIANFAR_DEV_HAS_MAGIC_PACKET);
 
 	/* fill out IRQ number and name fields */
 	for (i = 0; i < priv->num_grps; i++) {

@@ -2133,8 +2131,6 @@ static int gfar_enet_open(struct net_device *dev)
 	if (err)
 		return err;
 
-	device_set_wakeup_enable(&dev->dev, priv->wol_en);
-
 	return err;
 }

diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h
index daa1d37..5545e41 100644
--- a/drivers/net/ethernet/freescale/gianfar.h
+++ b/drivers/net/ethernet/freescale/gianfar.h

@@ -1145,9 +1145,6 @@ struct gfar_private {
 	int oldduplex;
 	int oldlink;
 
-	/* Bitfield update lock */
-	spinlock_t bflock;
-
 	uint32_t msg_enable;
 
 	struct work_struct reset_task;

diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index fda12fb..3c0a8f8 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c

@@ -653,7 +653,6 @@ static void gfar_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 static int gfar_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 {
 	struct gfar_private *priv = netdev_priv(dev);
-	unsigned long flags;
 
 	if (!(priv->device_flags & FSL_GIANFAR_DEV_HAS_MAGIC_PACKET) &&
 	    wol->wolopts != 0)

@@ -664,9 +663,7 @@ static int gfar_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 
 	device_set_wakeup_enable(&dev->dev, wol->wolopts & WAKE_MAGIC);
 
-	spin_lock_irqsave(&priv->bflock, flags);
-	priv->wol_en =  !!device_may_wakeup(&dev->dev);
-	spin_unlock_irqrestore(&priv->bflock, flags);
+	priv->wol_en = !!device_may_wakeup(&dev->dev);
 
 	return 0;
 }

-- 
1.7.11.7

[PATCH net 2/3] gianfar: Fix suspend/resume for wol magic packet

From: Claudiu Manoil <hidden>
Date: 2015-07-31 15:39:30

If we disable NAPI in the first place we can mask the device's
interrupts (and halt it) without fearing that imask may be
concurrently accessed from interrupt context, so there's
no need to do local_irq_save() around gfar_halt_nodisable().
lock_rx_qs()/unlock_tx_qs() are just obsoleted and potentially
buggy routines.  The txlock is currently used in the driver only
to manage TX congestion, it has nothing to do with halting the
device.  With these changes, the TX processing is stopped before
gfar_halt().

Compact gfar_halt() is used instead of gfar_halt_nodisable(),
as it disables Rx/TX DMA h/w blocks and the Rx/TX h/w queues.
gfar_start() re-enables all these blocks on resume.  Enabling
the magic-packet mode remains the same, note that the RX block
is re-enabled just before entering sleep mode.

Add IRQF_NO_SUSPEND flag for the error interrupt line, to signal
that the interrupt line must remain active during sleep in order
to wake the system by magic packet (MAG) reception interrupt.
(On some systems the MAG interrupt did trigger w/o this flag
as well, but on others it didn't.)

Without these fixes, when suspended during fair Tx traffic the
interface occasionally failed to be woken up by magic packet.

Signed-off-by: Claudiu Manoil <redacted>
---
 drivers/net/ethernet/freescale/gianfar.c | 98 ++++++++++----------------------
 1 file changed, 30 insertions(+), 68 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index caad327..084c25d 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c

@@ -565,24 +565,6 @@ static void gfar_ints_enable(struct gfar_private *priv)
 	}
 }
 
-#ifdef CONFIG_PM
-static void lock_tx_qs(struct gfar_private *priv)
-{
-	int i;
-
-	for (i = 0; i < priv->num_tx_queues; i++)
-		spin_lock(&priv->tx_queue[i]->txlock);
-}
-
-static void unlock_tx_qs(struct gfar_private *priv)
-{
-	int i;
-
-	for (i = 0; i < priv->num_tx_queues; i++)
-		spin_unlock(&priv->tx_queue[i]->txlock);
-}
-#endif
-
 static int gfar_alloc_tx_queues(struct gfar_private *priv)
 {
 	int i;

@@ -1542,48 +1524,37 @@ static int gfar_suspend(struct device *dev)
 	struct gfar_private *priv = dev_get_drvdata(dev);
 	struct net_device *ndev = priv->ndev;
 	struct gfar __iomem *regs = priv->gfargrp[0].regs;
-	unsigned long flags;
 	u32 tempval;
-
 	int magic_packet = priv->wol_en &&
 			   (priv->device_flags &
 			    FSL_GIANFAR_DEV_HAS_MAGIC_PACKET);
 
+	if (!netif_running(ndev))
+		return 0;
+
+	disable_napi(priv);
+	netif_tx_lock(ndev);
 	netif_device_detach(ndev);
+	netif_tx_unlock(ndev);
 
-	if (netif_running(ndev)) {
+	gfar_halt(priv);
 
-		local_irq_save(flags);
-		lock_tx_qs(priv);
+	if (magic_packet) {
+		/* Enable interrupt on Magic Packet */
+		gfar_write(&regs->imask, IMASK_MAG);
 
-		gfar_halt_nodisable(priv);
+		/* Enable Magic Packet mode */
+		tempval = gfar_read(&regs->maccfg2);
+		tempval |= MACCFG2_MPEN;
+		gfar_write(&regs->maccfg2, tempval);
 
-		/* Disable Tx, and Rx if wake-on-LAN is disabled. */
+		/* re-enable the Rx block */
 		tempval = gfar_read(&regs->maccfg1);
-
-		tempval &= ~MACCFG1_TX_EN;
-
-		if (!magic_packet)
-			tempval &= ~MACCFG1_RX_EN;
-
+		tempval |= MACCFG1_RX_EN;
 		gfar_write(&regs->maccfg1, tempval);
 
-		unlock_tx_qs(priv);
-		local_irq_restore(flags);
-
-		disable_napi(priv);
-
-		if (magic_packet) {
-			/* Enable interrupt on Magic Packet */
-			gfar_write(&regs->imask, IMASK_MAG);
-
-			/* Enable Magic Packet mode */
-			tempval = gfar_read(&regs->maccfg2);
-			tempval |= MACCFG2_MPEN;
-			gfar_write(&regs->maccfg2, tempval);
-		} else {
-			phy_stop(priv->phydev);
-		}
+	} else {
+		phy_stop(priv->phydev);
 	}
 
 	return 0;

@@ -1594,37 +1565,26 @@ static int gfar_resume(struct device *dev)
 	struct gfar_private *priv = dev_get_drvdata(dev);
 	struct net_device *ndev = priv->ndev;
 	struct gfar __iomem *regs = priv->gfargrp[0].regs;
-	unsigned long flags;
 	u32 tempval;
 	int magic_packet = priv->wol_en &&
 			   (priv->device_flags &
 			    FSL_GIANFAR_DEV_HAS_MAGIC_PACKET);
 
-	if (!netif_running(ndev)) {
-		netif_device_attach(ndev);
+	if (!netif_running(ndev))
 		return 0;
-	}
 
-	if (!magic_packet && priv->phydev)
+	if (magic_packet) {
+		/* Disable Magic Packet mode */
+		tempval = gfar_read(&regs->maccfg2);
+		tempval &= ~MACCFG2_MPEN;
+		gfar_write(&regs->maccfg2, tempval);
+	} else {
 		phy_start(priv->phydev);
-
-	/* Disable Magic Packet mode, in case something
-	 * else woke us up.
-	 */
-	local_irq_save(flags);
-	lock_tx_qs(priv);
-
-	tempval = gfar_read(&regs->maccfg2);
-	tempval &= ~MACCFG2_MPEN;
-	gfar_write(&regs->maccfg2, tempval);
+	}
 
 	gfar_start(priv);
 
-	unlock_tx_qs(priv);
-	local_irq_restore(flags);
-
 	netif_device_attach(ndev);
-
 	enable_napi(priv);
 
 	return 0;

@@ -2047,7 +2007,8 @@ static int register_grp_irqs(struct gfar_priv_grp *grp)
 		/* Install our interrupt handlers for Error,
 		 * Transmit, and Receive
 		 */
-		err = request_irq(gfar_irq(grp, ER)->irq, gfar_error, 0,
+		err = request_irq(gfar_irq(grp, ER)->irq, gfar_error,
+				  IRQF_NO_SUSPEND,
 				  gfar_irq(grp, ER)->name, grp);
 		if (err < 0) {
 			netif_err(priv, intr, dev, "Can't get IRQ %d\n",

@@ -2070,7 +2031,8 @@ static int register_grp_irqs(struct gfar_priv_grp *grp)
 			goto rx_irq_fail;
 		}
 	} else {
-		err = request_irq(gfar_irq(grp, TX)->irq, gfar_interrupt, 0,
+		err = request_irq(gfar_irq(grp, TX)->irq, gfar_interrupt,
+				  IRQF_NO_SUSPEND,
 				  gfar_irq(grp, TX)->name, grp);
 		if (err < 0) {
 			netif_err(priv, intr, dev, "Can't get IRQ %d\n",

-- 
1.7.11.7

Re: [PATCH net 0/3] gianfar: wol magic packet fixes

From: David Miller <davem@davemloft.net>
Date: 2015-07-31 22:42:20

From: Claudiu Manoil <redacted>
Date: Fri, 31 Jul 2015 18:38:30 +0300

These changes were already validated as part of FSL SDK.
Patch 2 fixes occasional wake-on magic packet failures during
traffic, probably due to incorrect traffic stop/ device halt
sequence and incorrect usage of txlock.

Series applied, thanks.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help