Skip to contentSkip to conversation

ath5k: disable all tasklets while resetting

PATCH In review Bruno Randolf 3 participants 10 messages Jun 11, 2010 – Jun 15, 2010 Atheros ATH5K wireless driver

lore
Share & tools
lore

Bruno Randolf

author
#0 lore raw

PATCH 1/1 ath5k: disable all tasklets while resetting

Make sure no tasklets can run concurrently to a reset.

Signed-off-by Bruno Randolf <[email protected]>
---
 drivers/net/wireless/ath/ath5k/base.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
drivers/net/wireless/ath/ath5k/base.c +12 -0
index 9d37c1a..585c517 100644
@@ -2908,6 +2908,12 @@ ath5k_reset(struct ath5k_softc *sc, struct ieee80211_channel *chan)
 
 	ATH5K_DBG(sc, ATH5K_DEBUG_RESET, "resetting\n");
 
+	tasklet_disable(&sc->rxtq);		/* ath5k_tasklet_rx */
+	tasklet_disable(&sc->txtq);		/* ath5k_tasklet_tx */
+	tasklet_disable(&sc->calib);		/* ath5k_tasklet_calibrate */
+	tasklet_disable(&sc->beacontq);		/* ath5k_tasklet_beacon */
+	tasklet_disable(&sc->ani_tasklet);	/* ath5k_tasklet_ani */
+
 	if (chan) {
 		ath5k_hw_set_imr(ah, 0);
 		ath5k_txq_cleanup(sc);

@@ -2948,6 +2954,12 @@ ath5k_reset(struct ath5k_softc *sc, struct ieee80211_channel *chan)
 	ath5k_beacon_config(sc);
 	/* intrs are enabled by ath5k_beacon_config */
 
+	tasklet_enable(&sc->rxtq);		/* ath5k_tasklet_rx */
+	tasklet_enable(&sc->txtq);		/* ath5k_tasklet_tx */
+	tasklet_enable(&sc->calib);		/* ath5k_tasklet_calibrate */
+	tasklet_enable(&sc->beacontq);		/* ath5k_tasklet_beacon */
+	tasklet_enable(&sc->ani_tasklet);	/* ath5k_tasklet_ani */
+
 	ieee80211_wake_queues(sc->hw);
 
 	return 0;
#1 lore raw
Quoted 12 lines · Bruno Randolf

Make sure no tasklets can run concurrently to a reset.

Signed-off-by Bruno Randolf <[email protected]>
---
 drivers/net/wireless/ath/ath5k/base.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
drivers/net/wireless/ath/ath5k/base.c +0 -0
index 9d37c1a..585c517 100644

drivers/net/wireless/ath/ath5k/base.c ath5k_reset(struct ath5k_softc *sc, struct ieee80211_channel *chan) · line 2915

Quoted 8 lines
drivers/net/wireless/ath/ath5k/base.c +5 -0
@@ -2908,6 +2908,12 @@ ath5k_reset(struct ath5k_softc *sc, struct ieee80211_channel *chan)
 
 	ATH5K_DBG(sc, ATH5K_DEBUG_RESET, "resetting\n");
 
+	tasklet_disable(&sc->rxtq);		/* ath5k_tasklet_rx */
+	tasklet_disable(&sc->txtq);		/* ath5k_tasklet_tx */
+	tasklet_disable(&sc->calib);		/* ath5k_tasklet_calibrate */
+	tasklet_disable(&sc->beacontq);		/* ath5k_tasklet_beacon */
+	tasklet_disable(&sc->ani_tasklet);	/* ath5k_tasklet_ani */

I have no idea how long a reset can take, but this means that all these tasklets will spin while your reset is running if they were scheduled.

johannes

#2 lore raw
Quoted 2 lines · Johannes Berg

I have no idea how long a reset can take, but this means that all these tasklets will spin while your reset is running if they were scheduled.

It looks to me like tasklet_action() will bail out when the tasklet has a positive t->count (which disable elevates) rather than spin. What did I miss?

Signature · 1 line
Bob Copeland %% www.bobcopeland.com
#3 lore raw
Quoted 9 lines · Bob Copeland
Quoted 2 lines · Johannes Berg

I have no idea how long a reset can take, but this means that all these tasklets will spin while your reset is running if they were scheduled.

It looks to me like tasklet_action() will bail out when the tasklet has a positive t->count (which disable elevates) rather than spin.  What did I miss?

As Johannes pointed out on irc, I missed that it re-raises the softirq. doh!

Signature · 1 line
Bob Copeland %% www.bobcopeland.com
author
#4 lore raw
Quoted 7 lines · On Friday 11 June 2010 23:38:15 Bob Copeland wrote:
Quoted 6 lines · Bob Copeland

On Fri, Jun 11, 2010 at 6:41 AM, Johannes Berg

Quoted 2 lines · <[email protected]> wrote:

I have no idea how long a reset can take, but this means that all these tasklets will spin while your reset is running if they were scheduled.

Quoted 6 lines
Quoted 3 lines

It looks to me like tasklet_action() will bail out when the tasklet has a positive t->count (which disable elevates) rather than spin. What did I miss?

As Johannes pointed out on irc, I missed that it re-raises the softirq. doh!

well, what else can we do? we have to make sure the tasklets don't run concurrently to a reset to keep rx and tx buffers consistent.

we disable interrupts right after disabling the tasklets, so they should not be scheduled again, right? actually, we should disable interrupts first, and then disable tasklets... but then it should be safe, no?

bruno

#5 lore raw
Quoted 3 lines · Bruno Randolf

we disable interrupts right after disabling the tasklets, so they should not be scheduled again, right? actually, we should disable interrupts first, and then disable tasklets... but then it should be safe, no?

Disable interrupts then tasklet_kill should do it.

Signature · 1 line
Bob Copeland %% www.bobcopeland.com
author
#6 lore raw
Quoted 6 lines · On Mon June 14 2010 20:43:02 you wrote:
Quoted 3 lines · Bruno Randolf

we disable interrupts right after disabling the tasklets, so they should not be scheduled again, right? actually, we should disable interrupts first, and then disable tasklets... but then it should be safe, no?

Disable interrupts then tasklet_kill should do it.

what's wrong with first disable interrupts and tasklet_disable?

bruno

#7 lore raw
Quoted 9 lines · Bruno Randolf
Quoted 6 lines · On Mon June 14 2010 20:43:02 you wrote:
Quoted 3 lines · Bruno Randolf

we disable interrupts right after disabling the tasklets, so they should not be scheduled again, right? actually, we should disable interrupts first, and then disable tasklets... but then it should be safe, no?

Disable interrupts then tasklet_kill should do it.

what's wrong with first disable interrupts and tasklet_disable?

Look at the code for tasklet_disable... it only waits for tasks that are in the run state but doesn't do anything for scheduled tasks. So you can still get the spinning behavior if the interrupt runs and schedules the tasklet on another CPU.

Signature · 1 line
Bob Copeland %% www.bobcopeland.com
author
#8 lore raw
Quoted 8 lines · On Tue June 15 2010 13:10:16 Bob Copeland wrote:
Quoted 7 lines · Bruno Randolf
Quoted 6 lines · On Mon June 14 2010 20:43:02 you wrote:
Quoted 4 lines · Bruno Randolf

we disable interrupts right after disabling the tasklets, so they should not be scheduled again, right? actually, we should disable interrupts first, and then disable tasklets... but then it should be safe, no?

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

Disable interrupts then tasklet_kill should do it.

what's wrong with first disable interrupts and tasklet_disable?

Look at the code for tasklet_disable... it only waits for tasks that are in the run state but doesn't do anything for scheduled tasks. So you can still get the spinning behavior if the interrupt runs and schedules the tasklet on another CPU.

if we disable interrupts in the chip (ath5k_hw_set_imr) , the hardware does not generate any interrupts. so no tasklets will get scheduled...

bruno

#9 lore raw
Quoted 2 lines · Bruno Randolf

if we disable interrupts in the chip (ath5k_hw_set_imr) , the hardware does not generate any interrupts. so no tasklets will get scheduled...

The tasklet might already be scheduled on another CPU:

cpu0 cpu1
ath5k_reset()

                                ath5k_intr()

ath5k_hw_set_imr(0)
synchronize_irq()
// spins until irq finishes
tasklet_schedule(foo)
tasklet_disable(foo)
// reset here
tasklet_action() // spins

Signature · 1 line
Bob Copeland %% www.bobcopeland.com