[RFT 0/4] ath9k: fix oops during driver unload

[RFT 0/4] ath9k: fix oops during driver unload

[Luis R. Rodriguez]

This should fix an oops reported by Vasanth.
Vasanth my rc3 poos out, can you let me know if this
fixes the issue you saw?

Luis R. Rodriguez (4):
  ath9k: move common->debug_mask setting to ath_init_softc()
  ath9k: fix oops during unload -- initialize hw prior to debugfs
  ath9k: add helper to un-init the hw properly
  ath9k: rename ath_beaconq_setup() to ath9k_hw_beaconq_setup()

 drivers/net/wireless/ath/ath.h          |    6 ++++
 drivers/net/wireless/ath/ath9k/ath9k.h  |    1 -
 drivers/net/wireless/ath/ath9k/beacon.c |   12 -------
 drivers/net/wireless/ath/ath9k/debug.c  |    5 ---
 drivers/net/wireless/ath/ath9k/hw.c     |   11 ++++++-
 drivers/net/wireless/ath/ath9k/mac.c    |   13 ++++++++
 drivers/net/wireless/ath/ath9k/mac.h    |    1 +
 drivers/net/wireless/ath/ath9k/main.c   |   50 ++++++++++++++++++------------
 8 files changed, 60 insertions(+), 39 deletions(-)

[RFT 1/4] ath9k: move common->debug_mask setting to ath_init_softc()

[Luis R. Rodriguez]

What this means is we can enable now debug prints without
requiring CONFIG_ATH9K_DEBUG.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/ath9k/debug.c |    5 -----
 drivers/net/wireless/ath/ath9k/main.c  |    5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
index 25ae88e..84f4426 100644
--- a/drivers/net/wireless/ath/ath9k/debug.c
+++ b/drivers/net/wireless/ath/ath9k/debug.c
@@ -23,9 +23,6 @@
 #define REG_READ_D(_ah, _reg) \
 	ath9k_hw_common(_ah)->ops->read((_ah), (_reg))
 
-static unsigned int ath9k_debug = ATH_DBG_DEFAULT;
-module_param_named(debug, ath9k_debug, uint, 0);
-
 static struct dentry *ath9k_debugfs_root;
 
 static int ath9k_debugfs_open(struct inode *inode, struct file *file)
@@ -565,8 +562,6 @@ int ath9k_init_debug(struct ath_hw *ah)
 	struct ath_common *common = ath9k_hw_common(ah);
 	struct ath_softc *sc = (struct ath_softc *) common->priv;
 
-	common->debug_mask = ath9k_debug;
-
 	if (!ath9k_debugfs_root)
 		return -ENOENT;
 
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 86374ad..7f90cb8 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -29,6 +29,10 @@ static int modparam_nohwcrypt;
 module_param_named(nohwcrypt, modparam_nohwcrypt, int, 0444);
 MODULE_PARM_DESC(nohwcrypt, "Disable hardware encryption");
 
+static unsigned int ath9k_debug = ATH_DBG_DEFAULT;
+module_param_named(debug, ath9k_debug, uint, 0);
+MODULE_PARM_DESC(ath9k_debug, "Debugging mask");
+
 /* We use the hw_value as an index into our private channel structure */
 
 #define CHAN2G(_freq, _idx)  { \
@@ -1637,6 +1641,7 @@ static int ath_init_softc(u16 devid, struct ath_softc *sc, u16 subsysid,
 	common->ah = ah;
 	common->hw = sc->hw;
 	common->priv = sc;
+	common->debug_mask = ath9k_debug;
 
 	/*
 	 * Cache line size is used to size and align various
-- 
1.6.0.4

[RFT 2/4] ath9k: fix oops during unload -- initialize hw prior to debugfs

[Luis R. Rodriguez]

debugfs uses the hardware for several debugfs files as such the
hardware must be initialized and available prior to its usage. The
same applies to when we free the hw structs -- free debufs file
entries prior to free'ing the hardware.

This should fix the oops which occurs during module unload
due to the dereferencig of ah upon debugfs exit.

IP: [<46412d6b>] 0x46412d6b
*pde = 00000000
Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
last sysfs file: /sys/class/power_supply/BAT0/energy_full
Modules linked in: ath9k(-) ath9k_hw mac80211 ath cfg80211 <bleh>

Pid: 3112, comm: rmmod Not tainted (2.6.32-rc2-wl #101) 9461DUU
EIP: 0060:[<46412d6b>] EFLAGS: 00010246 CPU: 0
EIP is at 0x46412d6b
EAX: f5870004 EBX: f6700d94 ECX: 00000000 EDX: c14313a7
ESI: f5870000 EDI: fb58ce70 EBP: f6661eb4 ESP: f6661ea8
 DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
Process rmmod (pid: 3112, ti=f6660000 task=f6579380 task.ti=f6660000)
Stack:
 fb57e5e5 f5ca5d50 fb58ce70 f6661ebc fb58629a f6661ec8 c11b715e f5ca5da8
<0> f6661ed8 c1223d98 f5ca5da8 f5ca5ddc f6661eec c1223e6f fb58ce70 fb58ce70
<0> c14958a0 f6661f00 c1222edb fb58ce70 fb58ce70 fb58cebc f6661f1c c12243c9
Call Trace:
 [<fb57e5e5>] ? ath_cleanup+0x35/0x50 [ath9k]
 [<fb58629a>] ? ath_pci_remove+0x1a/0x20 [ath9k]
 [<c11b715e>] ? pci_device_remove+0x1e/0x40
 [<c1223d98>] ? __device_release_driver+0x58/0xa0
 [<c1223e6f>] ? driver_detach+0x8f/0xa0
 [<c1222edb>] ? bus_remove_driver+0x7b/0xb0
 [<c12243c9>] ? driver_unregister+0x49/0x80
 [<c1158cf2>] ? sysfs_remove_file+0x12/0x20
 [<c11b73b5>] ? pci_unregister_driver+0x35/0x90
 [<fb586172>] ? ath_pci_exit+0x12/0x20 [ath9k]
 [<fb5883ec>] ? ath9k_exit+0x10/0x3d [ath9k]
 [<c131971d>] ? mutex_unlock+0xd/0x10
 [<c1088c0f>] ? sys_delete_module+0x16f/0x220
 [<c10e3d5d>] ? do_munmap+0x23d/0x290
 [<c11a629c>] ? trace_hardirqs_off_thunk+0xc/0x10
 [<c11a628c>] ? trace_hardirqs_on_thunk+0xc/0x10
 [<c1003b41>] ? sysenter_exit+0xf/0x1a
 [<c1003b08>] ? sysenter_do_call+0x12/0x3c
Code:  Bad EIP value.
EIP: [<46412d6b>] 0x46412d6b SS:ESP 0068:f6661ea8
CR2: 0000000046412d6b
---[ end trace 847f3b05ff3dcb19 ]---

Reported-by: Vasanthakumar Thiagarajan" <vasanth@atheros.com>
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/ath.h        |    6 ++++++
 drivers/net/wireless/ath/ath9k/hw.c   |   11 ++++++++++-
 drivers/net/wireless/ath/ath9k/main.c |   28 ++++++++++++++--------------
 3 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h
index b6cd752..5e19a73 100644
--- a/drivers/net/wireless/ath/ath.h
+++ b/drivers/net/wireless/ath/ath.h
@@ -23,6 +23,11 @@
 
 static const u8 ath_bcast_mac[ETH_ALEN] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
 
+enum ath_device_state {
+	ATH_HW_UNAVAILABLE,
+	ATH_HW_INITIALIZED,
+};
+
 struct reg_dmn_pair_mapping {
 	u16 regDmnEnum;
 	u16 reg_5ghz_ctl;
@@ -59,6 +64,7 @@ struct ath_common {
 	void *priv;
 	struct ieee80211_hw *hw;
 	int debug_mask;
+	enum ath_device_state state;
 
 	u16 cachelsz;
 	u16 curaid;
diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
index 692fd1d..cab17c6 100644
--- a/drivers/net/wireless/ath/ath9k/hw.c
+++ b/drivers/net/wireless/ath/ath9k/hw.c
@@ -999,6 +999,8 @@ int ath9k_hw_init(struct ath_hw *ah)
 
 	ath9k_init_nfcal_hist_buffer(ah);
 
+	common->state = ATH_HW_INITIALIZED;
+
 	return 0;
 }
 
@@ -1239,11 +1241,18 @@ const char *ath9k_hw_probe(u16 vendorid, u16 devid)
 
 void ath9k_hw_detach(struct ath_hw *ah)
 {
+	struct ath_common *common = ath9k_hw_common(ah);
+
+	if (common->state <= ATH_HW_INITIALIZED)
+		goto free_hw;
+
 	if (!AR_SREV_9100(ah))
 		ath9k_hw_ani_disable(ah);
 
-	ath9k_hw_rf_free(ah);
 	ath9k_hw_setpower(ah, ATH9K_PM_FULL_SLEEP);
+
+free_hw:
+	ath9k_hw_rf_free(ah);
 	kfree(ah);
 	ah = NULL;
 }
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 7f90cb8..0fe915a 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1365,8 +1365,8 @@ void ath_detach(struct ath_softc *sc)
 	    ah->btcoex_hw.scheme == ATH_BTCOEX_CFG_3WIRE)
 		ath_gen_timer_free(ah, sc->btcoex.no_stomp_timer);
 
-	ath9k_hw_detach(ah);
 	ath9k_exit_debug(ah);
+	ath9k_hw_detach(ah);
 	sc->sc_ah = NULL;
 }
 
@@ -1626,10 +1626,8 @@ static int ath_init_softc(u16 devid, struct ath_softc *sc, u16 subsysid,
 		     (unsigned long)sc);
 
 	ah = kzalloc(sizeof(struct ath_hw), GFP_KERNEL);
-	if (!ah) {
-		r = -ENOMEM;
-		goto bad_no_ah;
-	}
+	if (!ah)
+		return -ENOMEM;
 
 	ah->hw_version.devid = devid;
 	ah->hw_version.subsysid = subsysid;
@@ -1651,15 +1649,18 @@ static int ath_init_softc(u16 devid, struct ath_softc *sc, u16 subsysid,
 	/* XXX assert csz is non-zero */
 	common->cachelsz = csz << 2;	/* convert to bytes */
 
-	if (ath9k_init_debug(ah) < 0)
-		dev_err(sc->dev, "Unable to create debugfs files\n");
-
 	r = ath9k_hw_init(ah);
 	if (r) {
 		ath_print(common, ATH_DBG_FATAL,
 			  "Unable to initialize hardware; "
 			  "initialization status: %d\n", r);
-		goto bad;
+		goto bad_free_hw;
+	}
+
+	if (ath9k_init_debug(ah) < 0) {
+		ath_print(common, ATH_DBG_FATAL,
+			  "Unable to create debugfs files\n");
+		goto bad_free_hw;
 	}
 
 	/* Get the hardware key cache size. */
@@ -1848,12 +1849,11 @@ bad2:
 	for (i = 0; i < ATH9K_NUM_TX_QUEUES; i++)
 		if (ATH_TXQ_SETUP(sc, i))
 			ath_tx_cleanupq(sc, &sc->tx.txq[i]);
-bad:
+
+	ath9k_exit_debug(ah);
+bad_free_hw:
 	ath9k_hw_detach(ah);
-bad_no_ah:
-	ath9k_exit_debug(sc->sc_ah);
 	sc->sc_ah = NULL;
-
 	return r;
 }
 
@@ -1966,8 +1966,8 @@ error_attach:
 		if (ATH_TXQ_SETUP(sc, i))
 			ath_tx_cleanupq(sc, &sc->tx.txq[i]);
 
-	ath9k_hw_detach(ah);
 	ath9k_exit_debug(ah);
+	ath9k_hw_detach(ah);
 	sc->sc_ah = NULL;
 
 	return error;
-- 
1.6.0.4

[RFT 3/4] ath9k: add helper to un-init the hw properly

[Luis R. Rodriguez]

This is used in several places, ensure we do it right in all
callers by using a helper.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/ath9k/main.c |   23 ++++++++++++++---------
 1 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 0fe915a..e6842dd 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1325,6 +1325,17 @@ void ath_cleanup(struct ath_softc *sc)
 	ieee80211_free_hw(sc->hw);
 }
 
+static void ath9k_uninit_hw(struct ath_softc *sc)
+{
+	struct ath_hw *ah = sc->sc_ah;
+
+	BUG_ON(!ah);
+
+	ath9k_exit_debug(ah);
+	ath9k_hw_detach(ah);
+	sc->sc_ah = NULL;
+}
+
 void ath_detach(struct ath_softc *sc)
 {
 	struct ieee80211_hw *hw = sc->hw;
@@ -1365,9 +1376,7 @@ void ath_detach(struct ath_softc *sc)
 	    ah->btcoex_hw.scheme == ATH_BTCOEX_CFG_3WIRE)
 		ath_gen_timer_free(ah, sc->btcoex.no_stomp_timer);
 
-	ath9k_exit_debug(ah);
-	ath9k_hw_detach(ah);
-	sc->sc_ah = NULL;
+	ath9k_uninit_hw(sc);
 }
 
 static int ath9k_reg_notifier(struct wiphy *wiphy,
@@ -1850,10 +1859,8 @@ bad2:
 		if (ATH_TXQ_SETUP(sc, i))
 			ath_tx_cleanupq(sc, &sc->tx.txq[i]);
 
-	ath9k_exit_debug(ah);
 bad_free_hw:
-	ath9k_hw_detach(ah);
-	sc->sc_ah = NULL;
+	ath9k_uninit_hw(sc);
 	return r;
 }
 
@@ -1966,9 +1973,7 @@ error_attach:
 		if (ATH_TXQ_SETUP(sc, i))
 			ath_tx_cleanupq(sc, &sc->tx.txq[i]);
 
-	ath9k_exit_debug(ah);
-	ath9k_hw_detach(ah);
-	sc->sc_ah = NULL;
+	ath9k_uninit_hw(sc);
 
 	return error;
 }
-- 
1.6.0.4

[RFT 4/4] ath9k: rename ath_beaconq_setup() to ath9k_hw_beaconq_setup()

[Luis R. Rodriguez]

And move it to hw code on mac.c where it belongs.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/ath9k/ath9k.h  |    1 -
 drivers/net/wireless/ath/ath9k/beacon.c |   12 ------------
 drivers/net/wireless/ath/ath9k/mac.c    |   13 +++++++++++++
 drivers/net/wireless/ath/ath9k/mac.h    |    1 +
 drivers/net/wireless/ath/ath9k/main.c   |    2 +-
 5 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index 14ff38d..13dd020 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -421,7 +421,6 @@ struct ath_beacon {
 
 void ath_beacon_tasklet(unsigned long data);
 void ath_beacon_config(struct ath_softc *sc, struct ieee80211_vif *vif);
-int ath_beaconq_setup(struct ath_hw *ah);
 int ath_beacon_alloc(struct ath_wiphy *aphy, struct ieee80211_vif *vif);
 void ath_beacon_return(struct ath_softc *sc, struct ath_vif *avp);
 
diff --git a/drivers/net/wireless/ath/ath9k/beacon.c b/drivers/net/wireless/ath/ath9k/beacon.c
index 54be876..b10c884 100644
--- a/drivers/net/wireless/ath/ath9k/beacon.c
+++ b/drivers/net/wireless/ath/ath9k/beacon.c
@@ -242,18 +242,6 @@ static void ath_beacon_start_adhoc(struct ath_softc *sc,
 		  sc->beacon.beaconq, ito64(bf->bf_daddr), bf->bf_desc);
 }
 
-int ath_beaconq_setup(struct ath_hw *ah)
-{
-	struct ath9k_tx_queue_info qi;
-
-	memset(&qi, 0, sizeof(qi));
-	qi.tqi_aifs = 1;
-	qi.tqi_cwmin = 0;
-	qi.tqi_cwmax = 0;
-	/* NB: don't enable any interrupts */
-	return ath9k_hw_setuptxqueue(ah, ATH9K_TX_QUEUE_BEACON, &qi);
-}
-
 int ath_beacon_alloc(struct ath_wiphy *aphy, struct ieee80211_vif *vif)
 {
 	struct ath_softc *sc = aphy->sc;
diff --git a/drivers/net/wireless/ath/ath9k/mac.c b/drivers/net/wireless/ath/ath9k/mac.c
index e2c1ba3..46466ff 100644
--- a/drivers/net/wireless/ath/ath9k/mac.c
+++ b/drivers/net/wireless/ath/ath9k/mac.c
@@ -1018,3 +1018,16 @@ bool ath9k_hw_stopdmarecv(struct ath_hw *ah)
 #undef AH_RX_STOP_DMA_TIMEOUT
 }
 EXPORT_SYMBOL(ath9k_hw_stopdmarecv);
+
+int ath9k_hw_beaconq_setup(struct ath_hw *ah)
+{
+	struct ath9k_tx_queue_info qi;
+
+	memset(&qi, 0, sizeof(qi));
+	qi.tqi_aifs = 1;
+	qi.tqi_cwmin = 0;
+	qi.tqi_cwmax = 0;
+	/* NB: don't enable any interrupts */
+	return ath9k_hw_setuptxqueue(ah, ATH9K_TX_QUEUE_BEACON, &qi);
+}
+EXPORT_SYMBOL(ath9k_hw_beaconq_setup);
diff --git a/drivers/net/wireless/ath/ath9k/mac.h b/drivers/net/wireless/ath/ath9k/mac.h
index 9ab3431..fefb65d 100644
--- a/drivers/net/wireless/ath/ath9k/mac.h
+++ b/drivers/net/wireless/ath/ath9k/mac.h
@@ -667,5 +667,6 @@ void ath9k_hw_rxena(struct ath_hw *ah);
 void ath9k_hw_startpcureceive(struct ath_hw *ah);
 void ath9k_hw_stoppcurecv(struct ath_hw *ah);
 bool ath9k_hw_stopdmarecv(struct ath_hw *ah);
+int ath9k_hw_beaconq_setup(struct ath_hw *ah);
 
 #endif /* MAC_H */
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index e6842dd..8325c91 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1703,7 +1703,7 @@ static int ath_init_softc(u16 devid, struct ath_softc *sc, u16 subsysid,
 	 * priority.  Note that the hal handles reseting
 	 * these queues at the needed time.
 	 */
-	sc->beacon.beaconq = ath_beaconq_setup(ah);
+	sc->beacon.beaconq = ath9k_hw_beaconq_setup(ah);
 	if (sc->beacon.beaconq == -1) {
 		ath_print(common, ATH_DBG_FATAL,
 			  "Unable to setup a beacon xmit queue\n");
-- 
1.6.0.4

Re: [RFT 0/4] ath9k: fix oops during driver unload

[Vasanthakumar Thiagarajan]

On Tue, Oct 06, 2009 at 09:23:17AM +0530, Luis Rodriguez wrote:
> This should fix an oops reported by Vasanth.
> Vasanth my rc3 poos out, can you let me know if this
> fixes the issue you saw?
> 
> Luis R. Rodriguez (4):
>   ath9k: move common->debug_mask setting to ath_init_softc()
>   ath9k: fix oops during unload -- initialize hw prior to debugfs
>   ath9k: add helper to un-init the hw properly
>   ath9k: rename ath_beaconq_setup() to ath9k_hw_beaconq_setup()
> 
>  drivers/net/wireless/ath/ath.h          |    6 ++++
>  drivers/net/wireless/ath/ath9k/ath9k.h  |    1 -
>  drivers/net/wireless/ath/ath9k/beacon.c |   12 -------
>  drivers/net/wireless/ath/ath9k/debug.c  |    5 ---
>  drivers/net/wireless/ath/ath9k/hw.c     |   11 ++++++-
>  drivers/net/wireless/ath/ath9k/mac.c    |   13 ++++++++
>  drivers/net/wireless/ath/ath9k/mac.h    |    1 +
>  drivers/net/wireless/ath/ath9k/main.c   |   50 ++++++++++++++++++------------
>  8 files changed, 60 insertions(+), 39 deletions(-)
> 

The issue is not fixed completely, ah->common is used after ah is
freed in ath_cleanup() which makes rmmod ath9k stuck in the middle.

Vasanth

Re: [ath9k-devel] [RFT 0/4] ath9k: fix oops during driver unload

[Luis R. Rodriguez]

On Tue, Oct 6, 2009 at 1:52 AM, Vasanthakumar Thiagarajan
<vasanth@atheros.com> wrote:
> On Tue, Oct 06, 2009 at 09:23:17AM +0530, Luis Rodriguez wrote:
>> This should fix an oops reported by Vasanth.
>> Vasanth my rc3 poos out, can you let me know if this
>> fixes the issue you saw?
>>
>> Luis R. Rodriguez (4):
>>   ath9k: move common->debug_mask setting to ath_init_softc()
>>   ath9k: fix oops during unload -- initialize hw prior to debugfs
>>   ath9k: add helper to un-init the hw properly
>>   ath9k: rename ath_beaconq_setup() to ath9k_hw_beaconq_setup()
>>
>>  drivers/net/wireless/ath/ath.h          |    6 ++++
>>  drivers/net/wireless/ath/ath9k/ath9k.h  |    1 -
>>  drivers/net/wireless/ath/ath9k/beacon.c |   12 -------
>>  drivers/net/wireless/ath/ath9k/debug.c  |    5 ---
>>  drivers/net/wireless/ath/ath9k/hw.c     |   11 ++++++-
>>  drivers/net/wireless/ath/ath9k/mac.c    |   13 ++++++++
>>  drivers/net/wireless/ath/ath9k/mac.h    |    1 +
>>  drivers/net/wireless/ath/ath9k/main.c   |   50 ++++++++++++++++++------------
>>  8 files changed, 60 insertions(+), 39 deletions(-)
>>
>
> The issue is not fixed completely, ah->common is used after ah is
> freed in ath_cleanup() which makes rmmod ath9k stuck in the middle.

Thanks I'll fix that as well.

  Luis