[RFC/RFT] ath9k_htc: fix race conditions when stop device

[RFC/RFT] ath9k_htc: fix race conditions when stop device

[Stanislaw Gruszka]

Similar fix I already posted for ath9k. When stopping device, disable
interrupts, kill tasklets and then works, in correct order. Patch drop
mutex in them middle of a function, which I don't like and can possibly
not be correct. Perhaps this can be arranged differently. Also
there is no synchronize_irq, which IMHO should be added somewhere in
htc/wmi stop.

RFC/RTC for now (note I'm not able to test patch).

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/ath/ath9k/htc_drv_init.c |    3 ---
 drivers/net/wireless/ath/ath9k/htc_drv_main.c |   21 +++++++++++++++------
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_init.c b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
index 8e04586..a7bc26d 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_init.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
@@ -142,9 +142,6 @@ static void ath9k_deinit_priv(struct ath9k_htc_priv *priv)
 {
 	ath9k_htc_exit_debug(priv->ah);
 	ath9k_hw_deinit(priv->ah);
-	tasklet_kill(&priv->swba_tasklet);
-	tasklet_kill(&priv->rx_tasklet);
-	tasklet_kill(&priv->tx_tasklet);
 	kfree(priv->ah);
 	priv->ah = NULL;
 }
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
index f14f37d..a702089 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
@@ -1026,12 +1026,6 @@ static void ath9k_htc_stop(struct ieee80211_hw *hw)
 	int ret = 0;
 	u8 cmd_rsp;
 
-	/* Cancel all the running timers/work .. */
-	cancel_work_sync(&priv->fatal_work);
-	cancel_work_sync(&priv->ps_work);
-	cancel_delayed_work_sync(&priv->ath9k_led_blink_work);
-	ath9k_led_stop_brightness(priv);
-
 	mutex_lock(&priv->mutex);
 
 	if (priv->op_flags & OP_INVALID) {
@@ -1045,8 +1039,23 @@ static void ath9k_htc_stop(struct ieee80211_hw *hw)
 	WMI_CMD(WMI_DISABLE_INTR_CMDID);
 	WMI_CMD(WMI_DRAIN_TXQ_ALL_CMDID);
 	WMI_CMD(WMI_STOP_RECV_CMDID);
+
+	tasklet_kill(&priv->swba_tasklet);
+	tasklet_kill(&priv->rx_tasklet);
+	tasklet_kill(&priv->tx_tasklet);
+
 	skb_queue_purge(&priv->tx_queue);
 
+	mutex_unlock(&priv->mutex);
+
+	/* Cancel all the running timers/work .. */
+	cancel_work_sync(&priv->fatal_work);
+	cancel_work_sync(&priv->ps_work);
+	cancel_delayed_work_sync(&priv->ath9k_led_blink_work);
+	ath9k_led_stop_brightness(priv);
+
+	mutex_lock(&priv->mutex);
+
 	/* Remove monitor interface here */
 	if (ah->opmode == NL80211_IFTYPE_MONITOR) {
 		if (ath9k_htc_remove_monitor_interface(priv))
-- 
1.7.1

[RFC/RFT] ath9k_htc: fix race conditions when stop device

[Sujith]

Stanislaw Gruszka wrote:
> Similar fix I already posted for ath9k. When stopping device, disable
> interrupts, kill tasklets and then works, in correct order. Patch drop
> mutex in them middle of a function, which I don't like and can possibly
> not be correct. Perhaps this can be arranged differently. Also
> there is no synchronize_irq, which IMHO should be added somewhere in
> htc/wmi stop.

It should be okay if the work instances are canceled first before the mutex
is acquired. And I don't think synchronize_irq() is required since this is a USB driver.

But when the driver is loaded and unloaded without bringing the interface up,
stop() wouldn't be called at all, but I guess it's alright since no tasklet would
have been scheduled. So just moving the tasklet_kill() functions to stop() would
be fine. I tested with ath9k_htc and saw no issues.

Sujith

Re: [RFC/RFT] ath9k_htc: fix race conditions when stop device

[Stanislaw Gruszka]

On Fri, 21 Jan 2011 15:51:51 +0530
Sujith <m.sujith@gmail.com> wrote:

> Stanislaw Gruszka wrote:
> > Similar fix I already posted for ath9k. When stopping device, disable
> > interrupts, kill tasklets and then works, in correct order. Patch drop
> > mutex in them middle of a function, which I don't like and can possibly
> > not be correct. Perhaps this can be arranged differently. Also
> > there is no synchronize_irq, which IMHO should be added somewhere in
> > htc/wmi stop.
> 
> It should be okay if the work instances are canceled first before the mutex
> is acquired. 

IIRC, there is possibility to schedule priv->ps_work from ath9k_rx_tasklet (when
receive beacon frame and being in power save mode), so we should cancel that work
after assure tasklet will not be executed.

> And I don't think synchronize_irq() is required since this is a USB driver.
> 
> But when the driver is loaded and unloaded without bringing the interface up,
> stop() wouldn't be called at all, but I guess it's alright since no tasklet would
> have been scheduled. 

I assumed ath9k_htc_rxep() and ath9k_htc_txep() can not be called before _start()
(and after _stop() of course), hence removing tasklet_kill's from ath9k_deinit_priv()

> I tested with ath9k_htc and saw no issues.

Thanks!
Stanislaw