[PATCH 1/2] ath5k: move reset to mac80211 workqueue

[PATCH 1/2] ath5k: move reset to mac80211 workqueue

[Bob Copeland]

We currently trigger a reset via a tasklet when certain error
conditions are detected so that the card will (eventually)
restart.  Unfortunately this makes locking complicated since
reset can also be called in process context (e.g. for channel
change).  Currently nothing protects against concurrent resets,
which can be the source of corruption bugs.

Reset takes too long to spinlock the whole thing, so this
patch moves deferred resets into the mac80211 workqueue to
enable use of sc->lock mutex.

Signed-off-by: Bob Copeland <me@bobcopeland.com>
---
 drivers/net/wireless/ath/ath5k/base.c  |   38 +++++++++++++++++--------------
 drivers/net/wireless/ath/ath5k/base.h  |    3 +-
 drivers/net/wireless/ath/ath5k/debug.c |    2 +-
 3 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 20328bd..b290cc6 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -388,7 +388,7 @@ static int 	ath5k_init(struct ath5k_softc *sc);
 static int 	ath5k_stop_locked(struct ath5k_softc *sc);
 static int 	ath5k_stop_hw(struct ath5k_softc *sc);
 static irqreturn_t ath5k_intr(int irq, void *dev_id);
-static void 	ath5k_tasklet_reset(unsigned long data);
+static void ath5k_reset_work(struct work_struct *work);
 
 static void 	ath5k_tasklet_calibrate(unsigned long data);
 
@@ -831,11 +831,12 @@ ath5k_attach(struct pci_dev *pdev, struct ieee80211_hw *hw)
 
 	tasklet_init(&sc->rxtq, ath5k_tasklet_rx, (unsigned long)sc);
 	tasklet_init(&sc->txtq, ath5k_tasklet_tx, (unsigned long)sc);
-	tasklet_init(&sc->restq, ath5k_tasklet_reset, (unsigned long)sc);
 	tasklet_init(&sc->calib, ath5k_tasklet_calibrate, (unsigned long)sc);
 	tasklet_init(&sc->beacontq, ath5k_tasklet_beacon, (unsigned long)sc);
 	tasklet_init(&sc->ani_tasklet, ath5k_tasklet_ani, (unsigned long)sc);
 
+	INIT_WORK(&sc->reset_work, ath5k_reset_work);
+
 	ret = ath5k_eeprom_read_mac(ah, mac);
 	if (ret) {
 		ATH5K_ERR(sc, "unable to read address from EEPROM: 0x%04x\n",
@@ -2294,8 +2295,8 @@ err_unmap:
  * frame contents are done as needed and the slot time is
  * also adjusted based on current state.
  *
- * This is called from software irq context (beacontq or restq
- * tasklets) or user context from ath5k_beacon_config.
+ * This is called from software irq context (beacontq tasklets)
+ * or user context from ath5k_beacon_config.
  */
 static void
 ath5k_beacon_send(struct ath5k_softc *sc)
@@ -2328,7 +2329,7 @@ ath5k_beacon_send(struct ath5k_softc *sc)
 				sc->bmisscount);
 			ATH5K_DBG(sc, ATH5K_DEBUG_RESET,
 				  "stuck beacon, resetting\n");
-			tasklet_schedule(&sc->restq);
+			ieee80211_queue_work(sc->hw, &sc->reset_work);
 		}
 		return;
 	}
@@ -2684,7 +2685,6 @@ ath5k_stop_hw(struct ath5k_softc *sc)
 
 	tasklet_kill(&sc->rxtq);
 	tasklet_kill(&sc->txtq);
-	tasklet_kill(&sc->restq);
 	tasklet_kill(&sc->calib);
 	tasklet_kill(&sc->beacontq);
 	tasklet_kill(&sc->ani_tasklet);
@@ -2737,7 +2737,7 @@ ath5k_intr(int irq, void *dev_id)
 			 */
 			ATH5K_DBG(sc, ATH5K_DEBUG_RESET,
 				  "fatal int, resetting\n");
-			tasklet_schedule(&sc->restq);
+			ieee80211_queue_work(sc->hw, &sc->reset_work);
 		} else if (unlikely(status & AR5K_INT_RXORN)) {
 			/*
 			 * Receive buffers are full. Either the bus is busy or
@@ -2752,7 +2752,7 @@ ath5k_intr(int irq, void *dev_id)
 			if (ah->ah_mac_srev < AR5K_SREV_AR5212) {
 				ATH5K_DBG(sc, ATH5K_DEBUG_RESET,
 					  "rx overrun, resetting\n");
-				tasklet_schedule(&sc->restq);
+				ieee80211_queue_work(sc->hw, &sc->reset_work);
 			}
 			else
 				tasklet_schedule(&sc->rxtq);
@@ -2799,14 +2799,6 @@ ath5k_intr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static void
-ath5k_tasklet_reset(unsigned long data)
-{
-	struct ath5k_softc *sc = (void *)data;
-
-	ath5k_reset(sc, sc->curchan);
-}
-
 /*
  * Periodically recalibrate the PHY to account
  * for temperature/environment changes.
@@ -2830,7 +2822,7 @@ ath5k_tasklet_calibrate(unsigned long data)
 		 * to load new gain values.
 		 */
 		ATH5K_DBG(sc, ATH5K_DEBUG_RESET, "calibration, resetting\n");
-		ath5k_reset(sc, sc->curchan);
+		ieee80211_queue_work(sc->hw, &sc->reset_work);
 	}
 	if (ath5k_hw_phy_calibrate(ah, sc->curchan))
 		ATH5K_ERR(sc, "calibration of channel %u failed\n",
@@ -2934,6 +2926,8 @@ drop_packet:
 /*
  * Reset the hardware.  If chan is not NULL, then also pause rx/tx
  * and change to the given channel.
+ *
+ * This should be called with sc->lock.
  */
 static int
 ath5k_reset(struct ath5k_softc *sc, struct ieee80211_channel *chan)
@@ -2990,6 +2984,16 @@ err:
 	return ret;
 }
 
+static void ath5k_reset_work(struct work_struct *work)
+{
+	struct ath5k_softc *sc = container_of(work, struct ath5k_softc,
+		reset_work);
+
+	mutex_lock(&sc->lock);
+	ath5k_reset(sc, sc->curchan);
+	mutex_unlock(&sc->lock);
+}
+
 static int ath5k_start(struct ieee80211_hw *hw)
 {
 	return ath5k_init(hw->priv);
diff --git a/drivers/net/wireless/ath/ath5k/base.h b/drivers/net/wireless/ath/ath5k/base.h
index 56221bc..86c90f4 100644
--- a/drivers/net/wireless/ath/ath5k/base.h
+++ b/drivers/net/wireless/ath/ath5k/base.h
@@ -47,6 +47,7 @@
 #include <linux/if_ether.h>
 #include <linux/leds.h>
 #include <linux/rfkill.h>
+#include <linux/workqueue.h>
 
 #include "ath5k.h"
 #include "debug.h"
@@ -189,7 +190,7 @@ struct ath5k_softc {
 	unsigned int		led_pin,	/* GPIO pin for driving LED */
 				led_on;		/* pin setting for LED on */
 
-	struct tasklet_struct	restq;		/* reset tasklet */
+	struct work_struct	reset_work;	/* deferred chip reset */
 
 	unsigned int		rxbufsize;	/* rx size based on mtu */
 	struct list_head	rxbuf;		/* receive buffer */
diff --git a/drivers/net/wireless/ath/ath5k/debug.c b/drivers/net/wireless/ath/ath5k/debug.c
index 8c63886..ebb9c23 100644
--- a/drivers/net/wireless/ath/ath5k/debug.c
+++ b/drivers/net/wireless/ath/ath5k/debug.c
@@ -279,7 +279,7 @@ static ssize_t write_file_reset(struct file *file,
 {
 	struct ath5k_softc *sc = file->private_data;
 	ATH5K_DBG(sc, ATH5K_DEBUG_RESET, "debug file triggered reset\n");
-	tasklet_schedule(&sc->restq);
+	ieee80211_queue_work(sc->hw, &sc->reset_work);
 	return count;
 }
 
-- 
1.6.3.3

[PATCH 2/2] ath5k: disable tasklets during reset

[Bob Copeland]

Based on a patch from Bruno Randolf, attempting useful
work while we are resetting the chip just leads to interface
lockups and bad descriptor data, and possibly DMAing to
freed buffers.  Let's suspend all tasklets while
reprogramming the registers in the card to avoid such
problems.

In the future we can convert the tasklets to threaded
interrupt handlers to simplify things.

Signed-off-by: Bob Copeland <me@bobcopeland.com>
---
 drivers/net/wireless/ath/ath5k/base.c |   20 ++++++++++++++------
 1 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index b290cc6..b0e1ca9 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -2639,6 +2639,15 @@ ath5k_stop_locked(struct ath5k_softc *sc)
 	return 0;
 }
 
+static void stop_tasklets(struct ath5k_softc *sc)
+{
+	tasklet_kill(&sc->rxtq);
+	tasklet_kill(&sc->txtq);
+	tasklet_kill(&sc->calib);
+	tasklet_kill(&sc->beacontq);
+	tasklet_kill(&sc->ani_tasklet);
+}
+
 /*
  * Stop the device, grabbing the top-level lock to protect
  * against concurrent entry through ath5k_init (which can happen
@@ -2683,11 +2692,7 @@ ath5k_stop_hw(struct ath5k_softc *sc)
 	mmiowb();
 	mutex_unlock(&sc->lock);
 
-	tasklet_kill(&sc->rxtq);
-	tasklet_kill(&sc->txtq);
-	tasklet_kill(&sc->calib);
-	tasklet_kill(&sc->beacontq);
-	tasklet_kill(&sc->ani_tasklet);
+	stop_tasklets(sc);
 
 	ath5k_rfkill_hw_stop(sc->ah);
 
@@ -2937,8 +2942,11 @@ ath5k_reset(struct ath5k_softc *sc, struct ieee80211_channel *chan)
 
 	ATH5K_DBG(sc, ATH5K_DEBUG_RESET, "resetting\n");
 
+	ath5k_hw_set_imr(ah, 0);
+	synchronize_irq(sc->pdev->irq);
+	stop_tasklets(sc);
+
 	if (chan) {
-		ath5k_hw_set_imr(ah, 0);
 		ath5k_txq_cleanup(sc);
 		ath5k_rx_stop(sc);
 
-- 
1.6.3.3

Re: [PATCH 1/2] ath5k: move reset to mac80211 workqueue

[Luis R. Rodriguez]

On Tue, Jul 13, 2010 at 08:32:40AM -0700, Bob Copeland wrote:
> We currently trigger a reset via a tasklet when certain error
> conditions are detected so that the card will (eventually)
> restart.  Unfortunately this makes locking complicated since
> reset can also be called in process context (e.g. for channel
> change).  Currently nothing protects against concurrent resets,
> which can be the source of corruption bugs.
> 
> Reset takes too long to spinlock the whole thing, so this
> patch moves deferred resets into the mac80211 workqueue to
> enable use of sc->lock mutex.
> 
> Signed-off-by: Bob Copeland <me@bobcopeland.com>

Neat! Good idea.

  Luis

Re: [PATCH 1/2] ath5k: move reset to mac80211 workqueue

[Bruno Randolf]

On Wed July 14 2010 00:32:40 Bob Copeland wrote:
> We currently trigger a reset via a tasklet when certain error
> conditions are detected so that the card will (eventually)
> restart.  Unfortunately this makes locking complicated since
> reset can also be called in process context (e.g. for channel
> change).  Currently nothing protects against concurrent resets,
> which can be the source of corruption bugs.
> 
> Reset takes too long to spinlock the whole thing, so this
> patch moves deferred resets into the mac80211 workqueue to
> enable use of sc->lock mutex.
> 
> Signed-off-by: Bob Copeland <me@bobcopeland.com>
> ---
>  drivers/net/wireless/ath/ath5k/base.c  |   38
> +++++++++++++++++-------------- drivers/net/wireless/ath/ath5k/base.h  |  
>  3 +-
>  drivers/net/wireless/ath/ath5k/debug.c |    2 +-
>  3 files changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath5k/base.c
> b/drivers/net/wireless/ath/ath5k/base.c index 20328bd..b290cc6 100644
> --- a/drivers/net/wireless/ath/ath5k/base.c
> +++ b/drivers/net/wireless/ath/ath5k/base.c
> @@ -388,7 +388,7 @@ static int 	ath5k_init(struct ath5k_softc *sc);
>  static int 	ath5k_stop_locked(struct ath5k_softc *sc);
>  static int 	ath5k_stop_hw(struct ath5k_softc *sc);
>  static irqreturn_t ath5k_intr(int irq, void *dev_id);
> -static void 	ath5k_tasklet_reset(unsigned long data);
> +static void ath5k_reset_work(struct work_struct *work);
> 
>  static void 	ath5k_tasklet_calibrate(unsigned long data);
> 
> @@ -831,11 +831,12 @@ ath5k_attach(struct pci_dev *pdev, struct
> ieee80211_hw *hw)
> 
>  	tasklet_init(&sc->rxtq, ath5k_tasklet_rx, (unsigned long)sc);
>  	tasklet_init(&sc->txtq, ath5k_tasklet_tx, (unsigned long)sc);
> -	tasklet_init(&sc->restq, ath5k_tasklet_reset, (unsigned long)sc);
>  	tasklet_init(&sc->calib, ath5k_tasklet_calibrate, (unsigned long)sc);
>  	tasklet_init(&sc->beacontq, ath5k_tasklet_beacon, (unsigned long)sc);
>  	tasklet_init(&sc->ani_tasklet, ath5k_tasklet_ani, (unsigned long)sc);
> 
> +	INIT_WORK(&sc->reset_work, ath5k_reset_work);
> +
>  	ret = ath5k_eeprom_read_mac(ah, mac);
>  	if (ret) {
>  		ATH5K_ERR(sc, "unable to read address from EEPROM: 0x%04x\n",
> @@ -2294,8 +2295,8 @@ err_unmap:
>   * frame contents are done as needed and the slot time is
>   * also adjusted based on current state.
>   *
> - * This is called from software irq context (beacontq or restq
> - * tasklets) or user context from ath5k_beacon_config.
> + * This is called from software irq context (beacontq tasklets)
> + * or user context from ath5k_beacon_config.
>   */
>  static void
>  ath5k_beacon_send(struct ath5k_softc *sc)
> @@ -2328,7 +2329,7 @@ ath5k_beacon_send(struct ath5k_softc *sc)
>  				sc->bmisscount);
>  			ATH5K_DBG(sc, ATH5K_DEBUG_RESET,
>  				  "stuck beacon, resetting\n");
> -			tasklet_schedule(&sc->restq);
> +			ieee80211_queue_work(sc->hw, &sc->reset_work);
>  		}
>  		return;
>  	}
> @@ -2684,7 +2685,6 @@ ath5k_stop_hw(struct ath5k_softc *sc)
> 
>  	tasklet_kill(&sc->rxtq);
>  	tasklet_kill(&sc->txtq);
> -	tasklet_kill(&sc->restq);
>  	tasklet_kill(&sc->calib);
>  	tasklet_kill(&sc->beacontq);
>  	tasklet_kill(&sc->ani_tasklet);
> @@ -2737,7 +2737,7 @@ ath5k_intr(int irq, void *dev_id)
>  			 */
>  			ATH5K_DBG(sc, ATH5K_DEBUG_RESET,
>  				  "fatal int, resetting\n");
> -			tasklet_schedule(&sc->restq);
> +			ieee80211_queue_work(sc->hw, &sc->reset_work);
>  		} else if (unlikely(status & AR5K_INT_RXORN)) {
>  			/*
>  			 * Receive buffers are full. Either the bus is busy or
> @@ -2752,7 +2752,7 @@ ath5k_intr(int irq, void *dev_id)
>  			if (ah->ah_mac_srev < AR5K_SREV_AR5212) {
>  				ATH5K_DBG(sc, ATH5K_DEBUG_RESET,
>  					  "rx overrun, resetting\n");
> -				tasklet_schedule(&sc->restq);
> +				ieee80211_queue_work(sc->hw, &sc->reset_work);
>  			}
>  			else
>  				tasklet_schedule(&sc->rxtq);
> @@ -2799,14 +2799,6 @@ ath5k_intr(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
> 
> -static void
> -ath5k_tasklet_reset(unsigned long data)
> -{
> -	struct ath5k_softc *sc = (void *)data;
> -
> -	ath5k_reset(sc, sc->curchan);
> -}
> -
>  /*
>   * Periodically recalibrate the PHY to account
>   * for temperature/environment changes.
> @@ -2830,7 +2822,7 @@ ath5k_tasklet_calibrate(unsigned long data)
>  		 * to load new gain values.
>  		 */
>  		ATH5K_DBG(sc, ATH5K_DEBUG_RESET, "calibration, resetting\n");
> -		ath5k_reset(sc, sc->curchan);
> +		ieee80211_queue_work(sc->hw, &sc->reset_work);
>  	}
>  	if (ath5k_hw_phy_calibrate(ah, sc->curchan))
>  		ATH5K_ERR(sc, "calibration of channel %u failed\n",
> @@ -2934,6 +2926,8 @@ drop_packet:
>  /*
>   * Reset the hardware.  If chan is not NULL, then also pause rx/tx
>   * and change to the given channel.
> + *
> + * This should be called with sc->lock.
>   */
>  static int
>  ath5k_reset(struct ath5k_softc *sc, struct ieee80211_channel *chan)
> @@ -2990,6 +2984,16 @@ err:
>  	return ret;
>  }
> 
> +static void ath5k_reset_work(struct work_struct *work)
> +{
> +	struct ath5k_softc *sc = container_of(work, struct ath5k_softc,
> +		reset_work);
> +
> +	mutex_lock(&sc->lock);
> +	ath5k_reset(sc, sc->curchan);
> +	mutex_unlock(&sc->lock);
> +}
> +
>  static int ath5k_start(struct ieee80211_hw *hw)
>  {
>  	return ath5k_init(hw->priv);
> diff --git a/drivers/net/wireless/ath/ath5k/base.h
> b/drivers/net/wireless/ath/ath5k/base.h index 56221bc..86c90f4 100644
> --- a/drivers/net/wireless/ath/ath5k/base.h
> +++ b/drivers/net/wireless/ath/ath5k/base.h
> @@ -47,6 +47,7 @@
>  #include <linux/if_ether.h>
>  #include <linux/leds.h>
>  #include <linux/rfkill.h>
> +#include <linux/workqueue.h>
> 
>  #include "ath5k.h"
>  #include "debug.h"
> @@ -189,7 +190,7 @@ struct ath5k_softc {
>  	unsigned int		led_pin,	/* GPIO pin for driving LED */
>  				led_on;		/* pin setting for LED on */
> 
> -	struct tasklet_struct	restq;		/* reset tasklet */
> +	struct work_struct	reset_work;	/* deferred chip reset */
> 
>  	unsigned int		rxbufsize;	/* rx size based on mtu */
>  	struct list_head	rxbuf;		/* receive buffer */
> diff --git a/drivers/net/wireless/ath/ath5k/debug.c
> b/drivers/net/wireless/ath/ath5k/debug.c index 8c63886..ebb9c23 100644
> --- a/drivers/net/wireless/ath/ath5k/debug.c
> +++ b/drivers/net/wireless/ath/ath5k/debug.c
> @@ -279,7 +279,7 @@ static ssize_t write_file_reset(struct file *file,
>  {
>  	struct ath5k_softc *sc = file->private_data;
>  	ATH5K_DBG(sc, ATH5K_DEBUG_RESET, "debug file triggered reset\n");
> -	tasklet_schedule(&sc->restq);
> +	ieee80211_queue_work(sc->hw, &sc->reset_work);
>  	return count;
>  }

please get this into 2.6.36...

Acked-by: Bruno Randolf <br1@einfach.org>

Re: [PATCH 2/2] ath5k: disable tasklets during reset

[Bruno Randolf]

On Wed July 14 2010 00:32:41 Bob Copeland wrote:
> Based on a patch from Bruno Randolf, attempting useful
> work while we are resetting the chip just leads to interface
> lockups and bad descriptor data, and possibly DMAing to
> freed buffers.  Let's suspend all tasklets while
> reprogramming the registers in the card to avoid such
> problems.
> 
> In the future we can convert the tasklets to threaded
> interrupt handlers to simplify things.
> 
> Signed-off-by: Bob Copeland <me@bobcopeland.com>
> ---
>  drivers/net/wireless/ath/ath5k/base.c |   20 ++++++++++++++------
>  1 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath5k/base.c
> b/drivers/net/wireless/ath/ath5k/base.c index b290cc6..b0e1ca9 100644
> --- a/drivers/net/wireless/ath/ath5k/base.c
> +++ b/drivers/net/wireless/ath/ath5k/base.c
> @@ -2639,6 +2639,15 @@ ath5k_stop_locked(struct ath5k_softc *sc)
>  	return 0;
>  }
> 
> +static void stop_tasklets(struct ath5k_softc *sc)
> +{
> +	tasklet_kill(&sc->rxtq);
> +	tasklet_kill(&sc->txtq);
> +	tasklet_kill(&sc->calib);
> +	tasklet_kill(&sc->beacontq);
> +	tasklet_kill(&sc->ani_tasklet);
> +}
> +
>  /*
>   * Stop the device, grabbing the top-level lock to protect
>   * against concurrent entry through ath5k_init (which can happen
> @@ -2683,11 +2692,7 @@ ath5k_stop_hw(struct ath5k_softc *sc)
>  	mmiowb();
>  	mutex_unlock(&sc->lock);
> 
> -	tasklet_kill(&sc->rxtq);
> -	tasklet_kill(&sc->txtq);
> -	tasklet_kill(&sc->calib);
> -	tasklet_kill(&sc->beacontq);
> -	tasklet_kill(&sc->ani_tasklet);
> +	stop_tasklets(sc);
> 
>  	ath5k_rfkill_hw_stop(sc->ah);
> 
> @@ -2937,8 +2942,11 @@ ath5k_reset(struct ath5k_softc *sc, struct
> ieee80211_channel *chan)
> 
>  	ATH5K_DBG(sc, ATH5K_DEBUG_RESET, "resetting\n");
> 
> +	ath5k_hw_set_imr(ah, 0);
> +	synchronize_irq(sc->pdev->irq);
> +	stop_tasklets(sc);
> +
>  	if (chan) {
> -		ath5k_hw_set_imr(ah, 0);
>  		ath5k_txq_cleanup(sc);
>  		ath5k_rx_stop(sc);

please get this into 2.6.36...

Acked-by: Bruno Randolf <br1@einfach.org>