[PATCH] ath5k: disable all tasklets while resetting

[PATCH] ath5k: disable all tasklets while resetting

[Bruno Randolf]

Make sure no tasklets can run concurrently to a reset.

Signed-off-by: Bruno Randolf <br1@einfach.org>
---
 drivers/net/wireless/ath/ath5k/base.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 9d37c1a..585c517 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -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;

Re: [PATCH] ath5k: disable all tasklets while resetting

[Johannes Berg]

On Fri, 2010-06-11 at 19:12 +0900, Bruno Randolf wrote:
> Make sure no tasklets can run concurrently to a reset.
> 
> Signed-off-by: Bruno Randolf <br1@einfach.org>
> ---
>  drivers/net/wireless/ath/ath5k/base.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
> index 9d37c1a..585c517 100644
> --- a/drivers/net/wireless/ath/ath5k/base.c
> +++ b/drivers/net/wireless/ath/ath5k/base.c
> @@ -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

Re: [ath5k-devel] [PATCH] ath5k: disable all tasklets while resetting

[Bob Copeland]

On Fri, Jun 11, 2010 at 6:41 AM, Johannes Berg
<johannes@sipsolutions.net> 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.

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?

-- 
Bob Copeland %% www.bobcopeland.com

Re: [ath5k-devel] [PATCH] ath5k: disable all tasklets while resetting

[Bob Copeland]

On Fri, Jun 11, 2010 at 10:21 AM, Bob Copeland <me@bobcopeland.com> wrote:
> On Fri, Jun 11, 2010 at 6:41 AM, Johannes Berg
> <johannes@sipsolutions.net> 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.
>
> 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!

-- 
Bob Copeland %% www.bobcopeland.com

Re: [ath5k-devel] [PATCH] ath5k: disable all tasklets while resetting

[Bruno Randolf]

On Friday 11 June 2010 23:38:15 Bob Copeland wrote:
> On Fri, Jun 11, 2010 at 10:21 AM, Bob Copeland <me@bobcopeland.com> wrote:
> > On Fri, Jun 11, 2010 at 6:41 AM, Johannes Berg
> > 
> > <johannes@sipsolutions.net> 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.
> > 
> > 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

Re: [ath5k-devel] [PATCH] ath5k: disable all tasklets while resetting

[Bob Copeland]

On Mon, Jun 14, 2010 at 10:50:59AM +0900, Bruno Randolf wrote:
> 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.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [ath5k-devel] [PATCH] ath5k: disable all tasklets while resetting

[Bruno Randolf]

On Mon June 14 2010 20:43:02 you wrote:
> On Mon, Jun 14, 2010 at 10:50:59AM +0900, Bruno Randolf wrote:
> > 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

Re: [ath5k-devel] [PATCH] ath5k: disable all tasklets while resetting

[Bob Copeland]

On Tue, Jun 15, 2010 at 10:07:21AM +0900, Bruno Randolf wrote:
> On Mon June 14 2010 20:43:02 you wrote:
> > On Mon, Jun 14, 2010 at 10:50:59AM +0900, Bruno Randolf wrote:
> > > 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.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [ath5k-devel] [PATCH] ath5k: disable all tasklets while resetting

[Bruno Randolf]

On Tue June 15 2010 13:10:16 Bob Copeland wrote:
> On Tue, Jun 15, 2010 at 10:07:21AM +0900, Bruno Randolf wrote:
> > On Mon June 14 2010 20:43:02 you wrote:
> > > On Mon, Jun 14, 2010 at 10:50:59AM +0900, Bruno Randolf wrote:
> > > > 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.

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

Re: [ath5k-devel] [PATCH] ath5k: disable all tasklets while resetting

[Bob Copeland]

On Tue, Jun 15, 2010 at 01:54:43PM +0900, Bruno Randolf wrote:
> 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

-- 
Bob Copeland %% www.bobcopeland.com